From 2f9e92eee78219ee698e4a123da0e7a72c38d721 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 3 Dec 2020 20:30:35 +1100 Subject: [PATCH] Make BackOfficeClaimsPrincipalFactory not be generic, doesn't need to be, cleans up code as per rules --- .../BackOfficeClaimsPrincipalFactory.cs | 39 +++-- ...kOfficeServiceCollectionExtensionsTests.cs | 4 +- .../BackOfficeClaimsPrincipalFactoryTests.cs | 64 +++---- .../Security/BackOfficeCookieManagerTests.cs | 39 +---- .../BackOfficeServiceCollectionExtensions.cs | 4 +- .../Security/BackOfficeUserManager.cs | 165 ++++++++++-------- .../ConfigureBackOfficeCookieOptions.cs | 100 ++++++----- 7 files changed, 216 insertions(+), 199 deletions(-) diff --git a/src/Umbraco.Infrastructure/BackOffice/BackOfficeClaimsPrincipalFactory.cs b/src/Umbraco.Infrastructure/BackOffice/BackOfficeClaimsPrincipalFactory.cs index 568c028e67..22ea4423d2 100644 --- a/src/Umbraco.Infrastructure/BackOffice/BackOfficeClaimsPrincipalFactory.cs +++ b/src/Umbraco.Infrastructure/BackOffice/BackOfficeClaimsPrincipalFactory.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Linq; using System.Security.Claims; using System.Threading.Tasks; @@ -7,27 +7,43 @@ using Microsoft.Extensions.Options; namespace Umbraco.Core.BackOffice { - public class BackOfficeClaimsPrincipalFactory : UserClaimsPrincipalFactory - where TUser : BackOfficeIdentityUser + /// + /// A + /// + public class BackOfficeClaimsPrincipalFactory : UserClaimsPrincipalFactory { - public BackOfficeClaimsPrincipalFactory(UserManager userManager, IOptions optionsAccessor) + + /// + /// Initializes a new instance of the class. + /// + /// The user manager + /// The + public BackOfficeClaimsPrincipalFactory(UserManager userManager, IOptions optionsAccessor) : base(userManager, optionsAccessor) { } - public override async Task CreateAsync(TUser user) + /// + /// + /// Returns a custom and allows flowing claims from the external identity + /// + public override async Task CreateAsync(BackOfficeIdentityUser user) { - if (user == null) throw new ArgumentNullException(nameof(user)); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } - var baseIdentity = await base.GenerateClaimsAsync(user); + ClaimsIdentity baseIdentity = await base.GenerateClaimsAsync(user); // now we can flow any custom claims that the actual user has currently assigned which could be done in the OnExternalLogin callback - foreach (var claim in user.Claims) + foreach (Models.Identity.IdentityUserClaim claim in user.Claims) { baseIdentity.AddClaim(new Claim(claim.ClaimType, claim.ClaimValue)); } - + // TODO: We want to remove UmbracoBackOfficeIdentity and only rely on ClaimsIdentity, once + // that is done then we'll create a ClaimsIdentity with all of the requirements here instead var umbracoIdentity = new UmbracoBackOfficeIdentity( baseIdentity, user.Id, @@ -43,7 +59,8 @@ namespace Umbraco.Core.BackOffice return new ClaimsPrincipal(umbracoIdentity); } - protected override async Task GenerateClaimsAsync(TUser user) + /// + protected override async Task GenerateClaimsAsync(BackOfficeIdentityUser user) { // TODO: Have a look at the base implementation https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Extensions.Core/src/UserClaimsPrincipalFactory.cs#L79 // since it's setting an authentication type that is probably not what we want. @@ -51,7 +68,7 @@ namespace Umbraco.Core.BackOffice // the method above just returns a principal that wraps the identity and we dont use a custom principal, // see https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Extensions.Core/src/UserClaimsPrincipalFactory.cs#L66 - var identity = await base.GenerateClaimsAsync(user); + ClaimsIdentity identity = await base.GenerateClaimsAsync(user); return identity; } diff --git a/src/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UmbracoBackOfficeServiceCollectionExtensionsTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UmbracoBackOfficeServiceCollectionExtensionsTests.cs index 450b3a341a..26c3f7875c 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UmbracoBackOfficeServiceCollectionExtensionsTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Web.BackOffice/UmbracoBackOfficeServiceCollectionExtensionsTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.DependencyInjection; using NUnit.Framework; @@ -26,7 +26,7 @@ namespace Umbraco.Tests.Integration.Umbraco.Web.BackOffice var principalFactory = Services.GetService>(); Assert.IsNotNull(principalFactory); - Assert.AreEqual(typeof(BackOfficeClaimsPrincipalFactory), principalFactory.GetType()); + Assert.AreEqual(typeof(BackOfficeClaimsPrincipalFactory), principalFactory.GetType()); } [Test] diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs index 13c73dfa96..5291c1b12e 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/BackOfficeClaimsPrincipalFactoryTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Security.Claims; using System.Threading.Tasks; @@ -8,50 +8,41 @@ using Moq; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.BackOffice; -using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Core.Models.Membership; using Umbraco.Extensions; -using Umbraco.Tests.Common.Builders; namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice { [TestFixture] public class BackOfficeClaimsPrincipalFactoryTests { - private const int _testUserId = 2; - private const string _testUserName = "bob"; - private const string _testUserGivenName = "Bob"; - private const string _testUserCulture = "en-US"; - private const string _testUserSecurityStamp = "B6937738-9C17-4C7D-A25A-628A875F5177"; + private const int TestUserId = 2; + private const string TestUserName = "bob"; + private const string TestUserGivenName = "Bob"; + private const string TestUserCulture = "en-US"; + private const string TestUserSecurityStamp = "B6937738-9C17-4C7D-A25A-628A875F5177"; private BackOfficeIdentityUser _testUser; private Mock> _mockUserManager; + private static Mock> GetMockedUserManager() + => new Mock>(new Mock>().Object, null, null, null, null, null, null, null, null); + [Test] public void Ctor_When_UserManager_Is_Null_Expect_ArgumentNullException() - { - Assert.Throws(() => new BackOfficeClaimsPrincipalFactory( - null, - new OptionsWrapper(new BackOfficeIdentityOptions()))); - } + => Assert.Throws(() => new BackOfficeClaimsPrincipalFactory( + null, + new OptionsWrapper(new BackOfficeIdentityOptions()))); [Test] public void Ctor_When_Options_Are_Null_Expect_ArgumentNullException() - { - Assert.Throws(() => new BackOfficeClaimsPrincipalFactory( - new Mock>(new Mock>().Object, - null, null, null, null, null, null, null, null).Object, - null)); - } + => Assert.Throws(() => new BackOfficeClaimsPrincipalFactory(GetMockedUserManager().Object, null)); [Test] public void Ctor_When_Options_Value_Is_Null_Expect_ArgumentNullException() - { - Assert.Throws(() => new BackOfficeClaimsPrincipalFactory( - new Mock>(new Mock>().Object, - null, null, null, null, null, null, null, null).Object, - new OptionsWrapper(null))); - } + => Assert.Throws(() => new BackOfficeClaimsPrincipalFactory( + GetMockedUserManager().Object, + new OptionsWrapper(null))); [Test] public void CreateAsync_When_User_Is_Null_Expect_ArgumentNullException() @@ -72,8 +63,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice Assert.IsNotNull(umbracoBackOfficeIdentity); } - [TestCase(ClaimTypes.NameIdentifier, _testUserId)] - [TestCase(ClaimTypes.Name, _testUserName)] + [TestCase(ClaimTypes.NameIdentifier, TestUserId)] + [TestCase(ClaimTypes.Name, TestUserName)] public async Task CreateAsync_Should_Include_Claim(string expectedClaimType, object expectedClaimValue) { var sut = CreateSut(); @@ -141,17 +132,16 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice { var globalSettings = new GlobalSettings { DefaultUILanguage = "test" }; - _testUser = new BackOfficeIdentityUser(globalSettings, _testUserId, new List()) + _testUser = new BackOfficeIdentityUser(globalSettings, TestUserId, new List()) { - UserName = _testUserName, - Name = _testUserGivenName, + UserName = TestUserName, + Name = TestUserGivenName, Email = "bob@umbraco.test", - SecurityStamp = _testUserSecurityStamp, - Culture = _testUserCulture + SecurityStamp = TestUserSecurityStamp, + Culture = TestUserCulture }; - _mockUserManager = new Mock>(new Mock>().Object, - null, null, null, null, null, null, null, null); + _mockUserManager = GetMockedUserManager(); _mockUserManager.Setup(x => x.GetUserIdAsync(_testUser)).ReturnsAsync(_testUser.Id.ToString); _mockUserManager.Setup(x => x.GetUserNameAsync(_testUser)).ReturnsAsync(_testUser.UserName); _mockUserManager.Setup(x => x.SupportsUserSecurityStamp).Returns(false); @@ -159,10 +149,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.BackOffice _mockUserManager.Setup(x => x.SupportsUserRole).Returns(false); } - private BackOfficeClaimsPrincipalFactory CreateSut() - { - return new BackOfficeClaimsPrincipalFactory(_mockUserManager.Object, + private BackOfficeClaimsPrincipalFactory CreateSut() => new BackOfficeClaimsPrincipalFactory( + _mockUserManager.Object, new OptionsWrapper(new BackOfficeIdentityOptions())); - } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeCookieManagerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeCookieManagerTests.cs index e1a8ff9c58..3270a003f5 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeCookieManagerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Security/BackOfficeCookieManagerTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Moq; @@ -28,8 +28,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security runtime, Mock.Of(), globalSettings, - Mock.Of(), - Mock.Of()); + Mock.Of()); var result = mgr.ShouldAuthenticateRequest(new Uri("http://localhost/umbraco")); @@ -47,8 +46,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security runtime, Mock.Of(x => x.ApplicationVirtualPath == "/" && x.ToAbsolute(globalSettings.UmbracoPath) == "/umbraco"), globalSettings, - Mock.Of(), - Mock.Of()); + Mock.Of()); var result = mgr.ShouldAuthenticateRequest(new Uri("http://localhost/umbraco")); @@ -67,8 +65,9 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security runtime, Mock.Of(x => x.ApplicationVirtualPath == "/" && x.ToAbsolute(globalSettings.UmbracoPath) == "/umbraco" && x.ToAbsolute(Constants.SystemDirectories.Install) == "/install"), globalSettings, - Mock.Of(), - GetMockLinkGenerator(out var remainingTimeoutSecondsPath, out var isAuthPath)); + Mock.Of()); + + GenerateAuthPaths(out var remainingTimeoutSecondsPath, out var isAuthPath); var result = mgr.ShouldAuthenticateRequest(new Uri($"http://localhost{remainingTimeoutSecondsPath}")); Assert.IsTrue(result); @@ -89,8 +88,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security runtime, Mock.Of(x => x.ApplicationVirtualPath == "/" && x.ToAbsolute(globalSettings.UmbracoPath) == "/umbraco" && x.ToAbsolute(Constants.SystemDirectories.Install) == "/install"), globalSettings, - Mock.Of(x => x.IsAvailable == true && x.Get(Constants.Security.ForceReAuthFlag) == "not null"), - GetMockLinkGenerator(out var remainingTimeoutSecondsPath, out var isAuthPath)); + Mock.Of(x => x.IsAvailable && x.Get(Constants.Security.ForceReAuthFlag) == "not null")); var result = mgr.ShouldAuthenticateRequest(new Uri($"http://localhost/notbackoffice")); Assert.IsTrue(result); @@ -108,8 +106,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security runtime, Mock.Of(x => x.ApplicationVirtualPath == "/" && x.ToAbsolute(globalSettings.UmbracoPath) == "/umbraco" && x.ToAbsolute(Constants.SystemDirectories.Install) == "/install"), globalSettings, - Mock.Of(), - GetMockLinkGenerator(out var remainingTimeoutSecondsPath, out var isAuthPath)); + Mock.Of()); var result = mgr.ShouldAuthenticateRequest(new Uri($"http://localhost/notbackoffice")); Assert.IsFalse(result); @@ -119,7 +116,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security Assert.IsFalse(result); } - private LinkGenerator GetMockLinkGenerator(out string remainingTimeoutSecondsPath, out string isAuthPath) + private void GenerateAuthPaths(out string remainingTimeoutSecondsPath, out string isAuthPath) { var controllerName = ControllerExtensions.GetControllerName(); @@ -129,24 +126,6 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Web.Backoffice.Security // this is on the same controller but is considered a back office request var aPath = isAuthPath = $"/umbraco/{Constants.Web.Mvc.BackOfficePathSegment}/{Constants.Web.Mvc.BackOfficeApiArea}/{controllerName}/{nameof(AuthenticationController.IsAuthenticated)}".ToLower(); - var linkGenerator = new Mock(); - linkGenerator.Setup(x => x.GetPathByAddress( - //It.IsAny(), - It.IsAny(), - //It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny())).Returns((RouteValuesAddress address, RouteValueDictionary routeVals1, PathString path, FragmentString fragment, LinkOptions options) => - { - if (routeVals1["action"].ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds)) - return rPath; - if (routeVals1["action"].ToString() == nameof(AuthenticationController.IsAuthenticated).ToLower()) - return aPath; - return null; - }); - - return linkGenerator.Object; } } } diff --git a/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs b/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs index 413a54a28b..74953b19be 100644 --- a/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.BackOffice/Extensions/BackOfficeServiceCollectionExtensions.cs @@ -1,4 +1,4 @@ -using System; +using System; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc.Filters; @@ -36,7 +36,7 @@ namespace Umbraco.Extensions .AddUserStore() .AddUserManager() .AddSignInManager() - .AddClaimsPrincipalFactory>(); + .AddClaimsPrincipalFactory(); // Configure the options specifically for the UmbracoBackOfficeIdentityOptions instance services.ConfigureOptions(); diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeUserManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeUserManager.cs index 464f2a38aa..2906d9d87a 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeUserManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeUserManager.cs @@ -67,8 +67,6 @@ namespace Umbraco.Web.Common.Security PasswordConfiguration = passwordConfiguration.Value ?? throw new ArgumentNullException(nameof(passwordConfiguration)); } - #region What we do not currently support - // We don't support an IUserClaimStore and don't need to (at least currently) public override bool SupportsUserClaim => false; @@ -83,8 +81,6 @@ namespace Umbraco.Web.Common.Security // We haven't needed to support this yet, though might be necessary for 2FA public override bool SupportsUserPhoneNumber => false; - #endregion - /// /// Replace the underlying options property with our own strongly typed version /// @@ -97,14 +93,18 @@ namespace Umbraco.Web.Common.Security /// /// Used to validate a user's session /// - /// - /// - /// + /// The user id + /// The sesion id + /// True if the sesion is valid, else false public virtual async Task ValidateSessionIdAsync(string userId, string sessionId) { var userSessionStore = Store as IUserSessionStore; - //if this is not set, for backwards compat (which would be super rare), we'll just approve it - if (userSessionStore == null) return true; + + // if this is not set, for backwards compat (which would be super rare), we'll just approve it + if (userSessionStore == null) + { + return true; + } return await userSessionStore.ValidateSessionIdAsync(userId, sessionId); } @@ -112,12 +112,9 @@ namespace Umbraco.Web.Common.Security /// /// This will determine which password hasher to use based on what is defined in config /// - /// - protected virtual IPasswordHasher GetDefaultPasswordHasher(IPasswordConfiguration passwordConfiguration) - { - // we can use the user aware password hasher (which will be the default and preferred way) - return new PasswordHasher(); - } + /// The + /// An + protected virtual IPasswordHasher GetDefaultPasswordHasher(IPasswordConfiguration passwordConfiguration) => new PasswordHasher(); /// /// Gets/sets the default back office user password checker @@ -129,10 +126,14 @@ namespace Umbraco.Web.Common.Security /// /// Helper method to generate a password for a user based on the current password validator /// - /// + /// The generated password public string GeneratePassword() { - if (_passwordGenerator == null) _passwordGenerator = new PasswordGenerator(PasswordConfiguration); + if (_passwordGenerator == null) + { + _passwordGenerator = new PasswordGenerator(PasswordConfiguration); + } + var password = _passwordGenerator.GeneratePassword(); return password; } @@ -160,14 +161,10 @@ namespace Umbraco.Web.Common.Security return await base.IsLockedOutAsync(user); } - #region Overrides for password logic - /// /// Logic used to validate a username and password /// - /// - /// - /// + /// /// /// By default this uses the standard ASP.Net Identity approach which is: /// * Get password store @@ -186,55 +183,61 @@ namespace Umbraco.Web.Common.Security { if (BackOfficeUserPasswordChecker != null) { - var result = await BackOfficeUserPasswordChecker.CheckPasswordAsync(user, password); + BackOfficeUserPasswordCheckerResult result = await BackOfficeUserPasswordChecker.CheckPasswordAsync(user, password); if (user.HasIdentity == false) { return false; } - //if the result indicates to not fallback to the default, then return true if the credentials are valid + // if the result indicates to not fallback to the default, then return true if the credentials are valid if (result != BackOfficeUserPasswordCheckerResult.FallbackToDefaultChecker) { return result == BackOfficeUserPasswordCheckerResult.ValidCredentials; } } - //we cannot proceed if the user passed in does not have an identity + // we cannot proceed if the user passed in does not have an identity if (user.HasIdentity == false) + { return false; + } - //use the default behavior + // use the default behavior return await base.CheckPasswordAsync(user, password); } /// /// This is a special method that will reset the password but will raise the Password Changed event instead of the reset event /// - /// - /// - /// - /// + /// The userId + /// The reset password token + /// The new password to set it to + /// The /// /// We use this because in the back office the only way an admin can change another user's password without first knowing their password /// is to generate a token and reset it, however, when we do this we want to track a password change, not a password reset /// public async Task ChangePasswordWithResetAsync(int userId, string token, string newPassword) { - var user = await base.FindByIdAsync(userId.ToString()); - if (user == null) throw new InvalidOperationException("Could not find user"); + T user = await FindByIdAsync(userId.ToString()); + if (user == null) + { + throw new InvalidOperationException("Could not find user"); + } - var result = await base.ResetPasswordAsync(user, token, newPassword); + IdentityResult result = await base.ResetPasswordAsync(user, token, newPassword); if (result.Succeeded) { RaisePasswordChangedEvent(_httpContextAccessor.HttpContext?.User, userId); } + return result; } public override async Task ChangePasswordAsync(T user, string currentPassword, string newPassword) { - var result = await base.ChangePasswordAsync(user, currentPassword, newPassword); + IdentityResult result = await base.ChangePasswordAsync(user, currentPassword, newPassword); if (result.Succeeded) { RaisePasswordChangedEvent(_httpContextAccessor.HttpContext?.User, user.Id); @@ -245,20 +248,14 @@ namespace Umbraco.Web.Common.Security /// /// Override to determine how to hash the password /// - /// - /// - /// - /// - /// - /// This method is called anytime the password needs to be hashed for storage (i.e. including when reset password is used) - /// + /// protected override async Task UpdatePasswordHash(T user, string newPassword, bool validatePassword) { user.LastPasswordChangeDateUtc = DateTime.UtcNow; if (validatePassword) { - var validate = await ValidatePasswordAsync(user, newPassword); + IdentityResult validate = await ValidatePasswordAsync(user, newPassword); if (!validate.Succeeded) { return validate; @@ -266,7 +263,10 @@ namespace Umbraco.Web.Common.Security } var passwordStore = Store as IUserPasswordStore; - if (passwordStore == null) throw new NotSupportedException("The current user store does not implement " + typeof(IUserPasswordStore<>)); + if (passwordStore == null) + { + throw new NotSupportedException("The current user store does not implement " + typeof(IUserPasswordStore<>)); + } var hash = newPassword != null ? PasswordHasher.HashPassword(user, newPassword) : null; await passwordStore.SetPasswordHashAsync(user, hash, CancellationToken); @@ -277,41 +277,44 @@ namespace Umbraco.Web.Common.Security /// /// This is copied from the underlying .NET base class since they decided to not expose it /// - /// - /// private async Task UpdateSecurityStampInternal(T user) { - if (SupportsUserSecurityStamp == false) return; + if (SupportsUserSecurityStamp == false) + { + return; + } + await GetSecurityStore().SetSecurityStampAsync(user, NewSecurityStamp(), CancellationToken.None); } /// /// This is copied from the underlying .NET base class since they decided to not expose it /// - /// private IUserSecurityStampStore GetSecurityStore() { var store = Store as IUserSecurityStampStore; - if (store == null) throw new NotSupportedException("The current user store does not implement " + typeof(IUserSecurityStampStore<>)); + if (store == null) + { + throw new NotSupportedException("The current user store does not implement " + typeof(IUserSecurityStampStore<>)); + } + return store; } /// /// This is copied from the underlying .NET base class since they decided to not expose it /// - /// - private static string NewSecurityStamp() - { - return Guid.NewGuid().ToString(); - } - - #endregion + private static string NewSecurityStamp() => Guid.NewGuid().ToString(); + /// public override async Task SetLockoutEndDateAsync(T user, DateTimeOffset? lockoutEnd) { - if (user == null) throw new ArgumentNullException(nameof(user)); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } - var result = await base.SetLockoutEndDateAsync(user, lockoutEnd); + IdentityResult result = await base.SetLockoutEndDateAsync(user, lockoutEnd); // The way we unlock is by setting the lockoutEnd date to the current datetime if (result.Succeeded && lockoutEnd >= DateTimeOffset.UtcNow) @@ -321,25 +324,33 @@ namespace Umbraco.Web.Common.Security else { RaiseAccountUnlockedEvent(_httpContextAccessor.HttpContext?.User, user.Id); - //Resets the login attempt fails back to 0 when unlock is clicked + + // Resets the login attempt fails back to 0 when unlock is clicked await ResetAccessFailedCountAsync(user); } return result; } + /// public override async Task ResetAccessFailedCountAsync(T user) { - if (user == null) throw new ArgumentNullException(nameof(user)); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } var lockoutStore = (IUserLockoutStore)Store; var accessFailedCount = await GetAccessFailedCountAsync(user); if (accessFailedCount == 0) + { return IdentityResult.Success; + } await lockoutStore.ResetAccessFailedCountAsync(user, CancellationToken.None); - //raise the event now that it's reset + + // raise the event now that it's reset RaiseResetAccessFailedCountEvent(_httpContextAccessor.HttpContext?.User, user.Id); return await UpdateAsync(user); } @@ -347,33 +358,33 @@ namespace Umbraco.Web.Common.Security /// /// Overrides the Microsoft ASP.NET user management method /// - /// - /// - /// returns a Async Task - /// - /// - /// Doesn't set fail attempts back to 0 - /// + /// public override async Task AccessFailedAsync(T user) { - if (user == null) throw new ArgumentNullException(nameof(user)); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } var lockoutStore = Store as IUserLockoutStore; - if (lockoutStore == null) throw new NotSupportedException("The current user store does not implement " + typeof(IUserLockoutStore<>)); + if (lockoutStore == null) + { + throw new NotSupportedException("The current user store does not implement " + typeof(IUserLockoutStore<>)); + } var count = await lockoutStore.IncrementAccessFailedCountAsync(user, CancellationToken.None); if (count >= Options.Lockout.MaxFailedAccessAttempts) { - await lockoutStore.SetLockoutEndDateAsync(user, DateTimeOffset.UtcNow.Add(Options.Lockout.DefaultLockoutTimeSpan), - CancellationToken.None); - //NOTE: in normal aspnet identity this would do set the number of failed attempts back to 0 - //here we are persisting the value for the back office + await lockoutStore.SetLockoutEndDateAsync(user, DateTimeOffset.UtcNow.Add(Options.Lockout.DefaultLockoutTimeSpan), CancellationToken.None); + + // NOTE: in normal aspnet identity this would do set the number of failed attempts back to 0 + // here we are persisting the value for the back office } - var result = await UpdateAsync(user); + IdentityResult result = await UpdateAsync(user); - //Slightly confusing: this will return a Success if we successfully update the AccessFailed count + // Slightly confusing: this will return a Success if we successfully update the AccessFailed count if (result.Succeeded) { RaiseLoginFailedEvent(_httpContextAccessor.HttpContext?.User, user.Id); @@ -384,16 +395,18 @@ namespace Umbraco.Web.Common.Security private int GetCurrentUserId(IPrincipal currentUser) { - var umbIdentity = currentUser?.GetUmbracoIdentity(); + UmbracoBackOfficeIdentity umbIdentity = currentUser?.GetUmbracoIdentity(); var currentUserId = umbIdentity?.GetUserId() ?? Core.Constants.Security.SuperUserId; return currentUserId; } + private IdentityAuditEventArgs CreateArgs(AuditEvent auditEvent, IPrincipal currentUser, int affectedUserId, string affectedUsername) { var currentUserId = GetCurrentUserId(currentUser); var ip = IpResolver.GetCurrentRequestIpAddress(); return new IdentityAuditEventArgs(auditEvent, ip, currentUserId, string.Empty, affectedUserId, affectedUsername); } + private IdentityAuditEventArgs CreateArgs(AuditEvent auditEvent, BackOfficeIdentityUser currentUser, int affectedUserId, string affectedUsername) { var currentUserId = currentUser.Id; diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 4a9ebcaf46..590edf397a 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; @@ -38,8 +38,21 @@ namespace Umbraco.Web.BackOffice.Security private readonly IUserService _userService; private readonly IIpResolver _ipResolver; private readonly ISystemClock _systemClock; - private readonly LinkGenerator _linkGenerator; + /// + /// Initializes a new instance of the class. + /// + /// The + /// The + /// The options + /// The options + /// The + /// The + /// The + /// The + /// The + /// The + /// The public ConfigureBackOfficeCookieOptions( IServiceProvider serviceProvider, IUmbracoContextAccessor umbracoContextAccessor, @@ -51,8 +64,7 @@ namespace Umbraco.Web.BackOffice.Security IRequestCache requestCache, IUserService userService, IIpResolver ipResolver, - ISystemClock systemClock, - LinkGenerator linkGenerator) + ISystemClock systemClock) { _serviceProvider = serviceProvider; _umbracoContextAccessor = umbracoContextAccessor; @@ -65,15 +77,20 @@ namespace Umbraco.Web.BackOffice.Security _userService = userService; _ipResolver = ipResolver; _systemClock = systemClock; - _linkGenerator = linkGenerator; } + /// public void Configure(string name, CookieAuthenticationOptions options) { - if (name != Constants.Security.BackOfficeAuthenticationType) return; + if (name != Constants.Security.BackOfficeAuthenticationType) + { + return; + } + Configure(options); } + /// public void Configure(CookieAuthenticationOptions options) { options.SlidingExpiration = true; @@ -94,21 +111,18 @@ namespace Umbraco.Web.BackOffice.Security // NOTE: This is borrowed directly from aspnetcore source // Note: the purpose for the data protector must remain fixed for interop to work. - var dataProtector = options.DataProtectionProvider.CreateProtector("Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationMiddleware", Constants.Security.BackOfficeAuthenticationType, "v2"); + IDataProtector dataProtector = options.DataProtectionProvider.CreateProtector("Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationMiddleware", Constants.Security.BackOfficeAuthenticationType, "v2"); var ticketDataFormat = new TicketDataFormat(dataProtector); options.TicketDataFormat = new BackOfficeSecureDataFormat(_globalSettings.TimeOutInMinutes, ticketDataFormat); - //Custom cookie manager so we can filter requests + // Custom cookie manager so we can filter requests options.CookieManager = new BackOfficeCookieManager( _umbracoContextAccessor, _runtimeState, _hostingEnvironment, _globalSettings, - _requestCache, - _linkGenerator); - // _explicitPaths); TODO: Implement this once we do OAuth somehow - + _requestCache); // _explicitPaths); TODO: Implement this once we do OAuth somehow options.Events = new CookieAuthenticationEvents { @@ -119,22 +133,22 @@ namespace Umbraco.Web.BackOffice.Security // It would be possible to re-use the default behavior if any of these need to be set but that must be taken into account else // our back office requests will not function correctly. For now we don't need to set/configure any of these callbacks because // the defaults work fine with our setup. - OnValidatePrincipal = async ctx => { // We need to resolve the BackOfficeSecurityStampValidator per request as a requirement (even in aspnetcore they do this) - var securityStampValidator = ctx.HttpContext.RequestServices.GetRequiredService(); - // Same goes for the signinmanager - var signInManager = ctx.HttpContext.RequestServices.GetRequiredService(); + BackOfficeSecurityStampValidator securityStampValidator = ctx.HttpContext.RequestServices.GetRequiredService(); - var backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); + // Same goes for the signinmanager + IBackOfficeSignInManager signInManager = ctx.HttpContext.RequestServices.GetRequiredService(); + + UmbracoBackOfficeIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); if (backOfficeIdentity == null) { ctx.RejectPrincipal(); await signInManager.SignOutAsync(); } - //ensure the thread culture is set + // ensure the thread culture is set backOfficeIdentity.EnsureCulture(); await EnsureValidSessionId(ctx); @@ -154,19 +168,19 @@ namespace Umbraco.Web.BackOffice.Security OnSigningIn = ctx => { // occurs when sign in is successful but before the ticket is written to the outbound cookie - - var backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); + UmbracoBackOfficeIdentity backOfficeIdentity = ctx.Principal.GetUmbracoIdentity(); if (backOfficeIdentity != null) { - //generate a session id and assign it - //create a session token - if we are configured and not in an upgrade state then use the db, otherwise just generate one - var session = _runtimeState.Level == RuntimeLevel.Run + // generate a session id and assign it + // create a session token - if we are configured and not in an upgrade state then use the db, otherwise just generate one + Guid session = _runtimeState.Level == RuntimeLevel.Run ? _userService.CreateLoginSession(backOfficeIdentity.Id, _ipResolver.GetCurrentRequestIpAddress()) : Guid.NewGuid(); - //add our session claim + // add our session claim backOfficeIdentity.AddClaim(new Claim(Constants.Security.SessionIdClaimType, session.ToString(), ClaimValueTypes.String, UmbracoBackOfficeIdentity.Issuer, UmbracoBackOfficeIdentity.Issuer, backOfficeIdentity)); - //since it is a cookie-based authentication add that claim + + // since it is a cookie-based authentication add that claim backOfficeIdentity.AddClaim(new Claim(ClaimTypes.CookiePath, "/", ClaimValueTypes.String, UmbracoBackOfficeIdentity.Issuer, UmbracoBackOfficeIdentity.Issuer, backOfficeIdentity)); } @@ -177,18 +191,18 @@ namespace Umbraco.Web.BackOffice.Security // occurs when sign in is successful and after the ticket is written to the outbound cookie // When we are signed in with the cookie, assign the principal to the current HttpContext - ctx.HttpContext.User = ctx.Principal; + ctx.HttpContext.User = ctx.Principal; return Task.CompletedTask; }, OnSigningOut = ctx => { - //Clear the user's session on sign out + // Clear the user's session on sign out if (ctx.HttpContext?.User?.Identity != null) { var claimsIdentity = ctx.HttpContext.User.Identity as ClaimsIdentity; var sessionId = claimsIdentity.FindFirstValue(Constants.Security.SessionIdClaimType); - if (sessionId.IsNullOrWhiteSpace() == false && Guid.TryParse(sessionId, out var guidSession)) + if (sessionId.IsNullOrWhiteSpace() == false && Guid.TryParse(sessionId, out Guid guidSession)) { _userService.ClearLoginSession(guidSession); } @@ -225,10 +239,13 @@ namespace Umbraco.Web.BackOffice.Security /// private async Task EnsureValidSessionId(CookieValidatePrincipalContext context) { - if (_runtimeState.Level != RuntimeLevel.Run) return; - - using var scope = _serviceProvider.CreateScope(); - var validator = scope.ServiceProvider.GetRequiredService(); + if (_runtimeState.Level != RuntimeLevel.Run) + { + return; + } + + using IServiceScope scope = _serviceProvider.CreateScope(); + BackOfficeSessionIdValidator validator = scope.ServiceProvider.GetRequiredService(); await validator.ValidateSessionAsync(TimeSpan.FromMinutes(1), context); } @@ -236,21 +253,24 @@ namespace Umbraco.Web.BackOffice.Security /// Ensures the ticket is renewed if the is set to true /// and the current request is for the get user seconds endpoint /// - /// + /// The private void EnsureTicketRenewalIfKeepUserLoggedIn(CookieValidatePrincipalContext context) { - if (!_securitySettings.KeepUserLoggedIn) return; + if (!_securitySettings.KeepUserLoggedIn) + { + return; + } - var currentUtc = _systemClock.UtcNow; - var issuedUtc = context.Properties.IssuedUtc; - var expiresUtc = context.Properties.ExpiresUtc; + DateTimeOffset currentUtc = _systemClock.UtcNow; + DateTimeOffset? issuedUtc = context.Properties.IssuedUtc; + DateTimeOffset? expiresUtc = context.Properties.ExpiresUtc; if (expiresUtc.HasValue && issuedUtc.HasValue) { - var timeElapsed = currentUtc.Subtract(issuedUtc.Value); - var timeRemaining = expiresUtc.Value.Subtract(currentUtc); + TimeSpan timeElapsed = currentUtc.Subtract(issuedUtc.Value); + TimeSpan timeRemaining = expiresUtc.Value.Subtract(currentUtc); - //if it's time to renew, then do it + // if it's time to renew, then do it if (timeRemaining < timeElapsed) { context.ShouldRenew = true;