Back office user updates to support OAuth and 8.9 features

src/Umbraco.Core/BackOffice/ClaimsPrincipalExtensions.cs

@@ -17,6 +17,8 @@ public static class ClaimsPrincipalExtensions
         /// <returns></returns>
         public static UmbracoBackOfficeIdentity GetUmbracoIdentity(this IPrincipal user)
         {
+            // TODO: It would be nice to get rid of this and only rely on Claims, not a strongly typed identity instance
+
             //If it's already a UmbracoBackOfficeIdentity
             if (user.Identity is UmbracoBackOfficeIdentity backOfficeIdentity) return backOfficeIdentity;
 
@@ -53,10 +55,10 @@ public static UmbracoBackOfficeIdentity GetUmbracoIdentity(this IPrincipal user)
         /// <returns></returns>
         public static double GetRemainingAuthSeconds(this IPrincipal user, DateTimeOffset now)
         {
-            var umbIdentity = user.GetUmbracoIdentity();
-            if (umbIdentity == null) return 0;
+            var claimsPrincipal = user as ClaimsPrincipal;
+            if (claimsPrincipal == null) return 0;
 
-            var ticketExpires = umbIdentity.FindFirstValue(Constants.Security.TicketExpiresClaimType);
+            var ticketExpires = claimsPrincipal.FindFirst(Constants.Security.TicketExpiresClaimType)?.Value;
             if (ticketExpires.IsNullOrWhiteSpace()) return 0;
 
             var utcExpired = DateTimeOffset.Parse(ticketExpires, null, DateTimeStyles.RoundtripKind);

src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs

@@ -12,6 +12,10 @@ namespace Umbraco.Core.BackOffice
     [Serializable]
     public class UmbracoBackOfficeIdentity : ClaimsIdentity
     {
+        // TODO: Ideally we remove this class and only deal with ClaimsIdentity as a best practice. All things relevant to our own
+        // identity are part of claims. This class would essentially become extension methods on a ClaimsIdentity for resolving
+        // values from it. 
+
         public static bool FromClaimsIdentity(ClaimsIdentity identity, out UmbracoBackOfficeIdentity backOfficeIdentity)
         {
             //validate that all claims exist

src/Umbraco.Core/Security/IBackofficeSecurity.cs

@@ -9,48 +9,38 @@ public interface IBackOfficeSecurity
         /// <summary>
         /// Gets the current user.
         /// </summary>
-        /// <value>The current user.</value>
+        /// <returns>The current user that has been authenticated for the request.</returns>
+        /// <remarks>If authentication hasn't taken place this will be null.</remarks>
+        // TODO: This is used a lot but most of it can be refactored to not use this at all since the IUser instance isn't
+        // needed in most cases. Where an IUser is required this could be an ext method on the ClaimsIdentity/ClaimsPrincipal that passes in
+        // an IUserService, like HttpContext.User.GetUmbracoUser(_userService);
+        // This one isn't as easy to remove as the others below.
         IUser CurrentUser { get; }
 
         /// <summary>
         /// Gets the current user's id.
         /// </summary>
-        /// <returns></returns>
+        /// <returns>The current user's Id that has been authenticated for the request.</returns>
+        /// <remarks>If authentication hasn't taken place this will be unsuccessful.</remarks>
+        // TODO: This should just be an extension method on ClaimsIdentity
         Attempt<int> GetUserId();
 
-        /// <summary>
-        /// Validates the currently logged in user and ensures they are not timed out
-        /// </summary>
-        /// <returns></returns>
-        bool ValidateCurrentUser();
-
-        /// <summary>
-        /// Validates the current user assigned to the request and ensures the stored user data is valid
-        /// </summary>
-        /// <param name="throwExceptions">set to true if you want exceptions to be thrown if failed</param>
-        /// <param name="requiresApproval">If true requires that the user is approved to be validated</param>
-        /// <returns></returns>
-        ValidateRequestAttempt ValidateCurrentUser(bool throwExceptions, bool requiresApproval = true);
-
-        /// <summary>
-        /// Authorizes the full request, checks for SSL and validates the current user
-        /// </summary>
-        /// <param name="throwExceptions">set to true if you want exceptions to be thrown if failed</param>
-        /// <returns></returns>
-        ValidateRequestAttempt AuthorizeRequest(bool throwExceptions = false);
-
         /// <summary>
         /// Checks if the specified user as access to the app
         /// </summary>
         /// <param name="section"></param>
         /// <param name="user"></param>
         /// <returns></returns>
+        /// <remarks>If authentication hasn't taken place this will be unsuccessful.</remarks>
+        // TODO: Should be part of IBackOfficeUserManager
         bool UserHasSectionAccess(string section, IUser user);
 
         /// <summary>
         /// Ensures that a back office user is logged in
         /// </summary>
         /// <returns></returns>
+        /// <remarks>This does not force authentication, that must be done before calls to this are made.</remarks>
+        // TODO: Should be removed, this should not be necessary
         bool IsAuthenticated();
     }
 }

src/Umbraco.Infrastructure/BackOffice/BackOfficeUserStore.cs

@@ -12,6 +12,7 @@
 using Umbraco.Core.Models;
 using Umbraco.Core.Models.Identity;
 using Umbraco.Core.Models.Membership;
+using Umbraco.Core.Scoping;
 using Umbraco.Core.Services;
 
 namespace Umbraco.Core.BackOffice
@@ -22,24 +23,27 @@ public class BackOfficeUserStore : DisposableObjectSlim,
             IUserLoginStore<BackOfficeIdentityUser>,
             IUserRoleStore<BackOfficeIdentityUser>,
             IUserSecurityStampStore<BackOfficeIdentityUser>,
-            IUserLockoutStore<BackOfficeIdentityUser>,
-            IUserTwoFactorStore<BackOfficeIdentityUser>,
+            IUserLockoutStore<BackOfficeIdentityUser>,            
             IUserSessionStore<BackOfficeIdentityUser>
 
-    // TODO: This would require additional columns/tables for now people will need to implement this on their own
-    //IUserPhoneNumberStore<BackOfficeIdentityUser, int>,
-    // TODO: To do this we need to implement IQueryable -  we'll have an IQuerable implementation soon with the UmbracoLinqPadDriver implementation
-    //IQueryableUserStore<BackOfficeIdentityUser, int>
+        // TODO: This would require additional columns/tables and then a lot of extra coding support to make this happen natively within umbraco
+        //IUserTwoFactorStore<BackOfficeIdentityUser>,
+        // TODO: This would require additional columns/tables for now people will need to implement this on their own
+        //IUserPhoneNumberStore<BackOfficeIdentityUser, int>,
+        // TODO: To do this we need to implement IQueryable -  we'll have an IQuerable implementation soon with the UmbracoLinqPadDriver implementation
+        //IQueryableUserStore<BackOfficeIdentityUser, int>
     {
+        private readonly IScopeProvider _scopeProvider;
         private readonly IUserService _userService;
         private readonly IEntityService _entityService;
         private readonly IExternalLoginService _externalLoginService;
         private readonly GlobalSettings _globalSettings;
         private readonly UmbracoMapper _mapper;
         private bool _disposed = false;
 
-        public BackOfficeUserStore(IUserService userService, IEntityService entityService, IExternalLoginService externalLoginService, IOptions<GlobalSettings> globalSettings, UmbracoMapper mapper)
+        public BackOfficeUserStore(IScopeProvider scopeProvider, IUserService userService, IEntityService entityService, IExternalLoginService externalLoginService, IOptions<GlobalSettings> globalSettings, UmbracoMapper mapper)
         {
+            _scopeProvider = scopeProvider;
             _userService = userService;
             _entityService = entityService;
             _externalLoginService = externalLoginService;
@@ -156,7 +160,7 @@ public Task SetNormalizedUserNameAsync(BackOfficeIdentityUser user, string norma
         /// <param name="user"/>
         /// <param name="cancellationToken"></param>
         /// <returns/>
-        public async Task<IdentityResult> UpdateAsync(BackOfficeIdentityUser user, CancellationToken cancellationToken = default(CancellationToken))
+        public Task<IdentityResult> UpdateAsync(BackOfficeIdentityUser user, CancellationToken cancellationToken = default(CancellationToken))
         {
             cancellationToken.ThrowIfCancellationRequested();
             ThrowIfDisposed();
@@ -168,31 +172,34 @@ public Task SetNormalizedUserNameAsync(BackOfficeIdentityUser user, string norma
                 throw new InvalidOperationException("The user id must be an integer to work with the Umbraco");
             }
 
-            // TODO: Wrap this in a scope!
-
-            var found = _userService.GetUserById(asInt.Result);
-            if (found != null)
+            using (var scope = _scopeProvider.CreateScope())
             {
-                // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it.
-                var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(BackOfficeIdentityUser.Logins));
-
-                if (UpdateMemberProperties(found, user))
+                var found = _userService.GetUserById(asInt.Result);
+                if (found != null)
                 {
-                    _userService.Save(found);
-                }
+                    // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it.
+                    var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(BackOfficeIdentityUser.Logins));
 
-                if (isLoginsPropertyDirty)
-                {
-                    _externalLoginService.Save(
-                        found.Id,
-                        user.Logins.Select(x => new ExternalLogin(
-                            x.LoginProvider,
-                            x.ProviderKey,
-                            x.UserData)));
+                    if (UpdateMemberProperties(found, user))
+                    {
+                        _userService.Save(found);
+                    }
+
+                    if (isLoginsPropertyDirty)
+                    {
+                        _externalLoginService.Save(
+                            found.Id,
+                            user.Logins.Select(x => new ExternalLogin(
+                                x.LoginProvider,
+                                x.ProviderKey,
+                                x.UserData)));
+                    }
                 }
+
+                scope.Complete();
             }
 
-            return IdentityResult.Success;
+            return Task.FromResult(IdentityResult.Success);
         }
 
         /// <summary>
@@ -627,35 +634,6 @@ private BackOfficeIdentityUser AssignLoginsCallback(BackOfficeIdentityUser user)
             return user;
         }
 
-        /// <summary>
-        /// Sets whether two factor authentication is enabled for the user
-        /// </summary>
-        /// <param name="user"/>
-        /// <param name="enabled"/>
-        /// <param name="cancellationToken"></param>
-        /// <returns/>
-        public virtual Task SetTwoFactorEnabledAsync(BackOfficeIdentityUser user, bool enabled, CancellationToken cancellationToken = default(CancellationToken))
-        {
-            cancellationToken.ThrowIfCancellationRequested();
-            ThrowIfDisposed();
-
-            user.TwoFactorEnabled = false;
-            return Task.CompletedTask;
-        }
-
-        /// <summary>
-        /// Returns whether two factor authentication is enabled for the user
-        /// </summary>
-        /// <param name="user"/>
-        /// <returns/>
-        public virtual Task<bool> GetTwoFactorEnabledAsync(BackOfficeIdentityUser user, CancellationToken cancellationToken = default(CancellationToken))
-        {
-            cancellationToken.ThrowIfCancellationRequested();
-            ThrowIfDisposed();
-
-            return Task.FromResult(false);
-        }
-
         #region IUserLockoutStore
 
         /// <summary>

src/Umbraco.Infrastructure/BackOffice/IBackOfficeUserManager.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Security.Claims;
 using System.Security.Principal;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Identity;
@@ -15,6 +16,12 @@ public interface IBackOfficeUserManager : IBackOfficeUserManager<BackOfficeIdent
     public interface  IBackOfficeUserManager<TUser>: IDisposable
         where TUser : BackOfficeIdentityUser
     {
+        Task<string> GetUserIdAsync(TUser user);
+
+        Task<TUser> GetUserAsync(ClaimsPrincipal principal);
+
+        string GetUserId(ClaimsPrincipal principal);
+
         Task<IList<UserLoginInfo>> GetLoginsAsync(TUser user);
 
         Task<IdentityResult> DeleteAsync(TUser user);
@@ -304,13 +311,14 @@ Task<IdentityResult> ChangePasswordAsync(TUser user, string currentPassword,
         /// </remarks>
         Task<string> GetPhoneNumberAsync(TUser user);
 
+        // TODO: These are raised from outside the signinmanager and usermanager in the auth and user controllers,
+        // let's see if there's a way to avoid that and only have these called within signinmanager and usermanager
+        // which means we can remove these from the interface (things like invite seems like they cannot be moved)
         void RaiseForgotPasswordRequestedEvent(IPrincipal currentUser, int userId);
         void RaiseForgotPasswordChangedSuccessEvent(IPrincipal currentUser, int userId);
         SignOutAuditEventArgs RaiseLogoutSuccessEvent(IPrincipal currentUser, int userId);
         UserInviteEventArgs RaiseSendingUserInvite(IPrincipal currentUser, UserInvite invite, IUser createdUser);
-
-        void RaiseLoginSuccessEvent(TUser currentUser, int userId);
-
         bool HasSendingUserInviteEventHandler { get; }
+
     }
 }

src/Umbraco.Tests.Integration/TestServerTest/TestAuthHandler.cs

@@ -16,11 +16,11 @@ public class TestAuthHandler : AuthenticationHandler<AuthenticationSchemeOptions
     {
         public const string TestAuthenticationScheme = "Test";
 
-        private readonly BackOfficeSignInManager _backOfficeSignInManager;
+        private readonly IBackOfficeSignInManager _backOfficeSignInManager;
 
         private readonly BackOfficeIdentityUser _fakeUser;
         public TestAuthHandler(IOptionsMonitor<AuthenticationSchemeOptions> options,
-            ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock, BackOfficeSignInManager backOfficeSignInManager, IUserService userService, UmbracoMapper umbracoMapper)
+            ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock, IBackOfficeSignInManager backOfficeSignInManager, IUserService userService, UmbracoMapper umbracoMapper)
             : base(options, logger, encoder, clock)
         {
             _backOfficeSignInManager = backOfficeSignInManager;

src/Umbraco.Tests.UnitTests/AutoFixture/AutoMoqDataAttribute.cs

@@ -18,6 +18,7 @@
 using Umbraco.Web.BackOffice.Controllers;
 using Umbraco.Web.BackOffice.Routing;
 using Umbraco.Web.Common.Install;
+using Umbraco.Web.Common.Security;
 using Umbraco.Web.WebApi;
 
 namespace Umbraco.Tests.UnitTests.AutoFixture

src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs

@@ -120,8 +120,6 @@ IHttpController IHttpControllerActivator.Create(HttpRequestMessage request, Http
                 .Returns(mockUser.Object);
 
             //mock Validate
-            backofficeSecurity.Setup(x => x.ValidateCurrentUser())
-                .Returns(() => true);
             backofficeSecurity.Setup(x => x.UserHasSectionAccess(It.IsAny<string>(), It.IsAny<IUser>()))
                 .Returns(() => true);
 

src/Umbraco.Tests/Testing/TestingTests/MockTests.cs

@@ -116,7 +116,6 @@ public void Can_Mock_UmbracoApiController_Dependencies_With_Injected_UmbracoMapp
             var memberService = Mock.Of<IMemberService>();
             var memberTypeService = Mock.Of<IMemberTypeService>();
             var membershipProvider = new MembersMembershipProvider(memberService, memberTypeService, Mock.Of<IUmbracoVersion>(), TestHelper.GetHostingEnvironment(), TestHelper.GetIpResolver());
-            var membershipHelper = new MembershipHelper(Mock.Of<IHttpContextAccessor>(), Mock.Of<IPublishedMemberCache>(), membershipProvider, Mock.Of<RoleProvider>(), memberService, memberTypeService, Mock.Of<IPublicAccessService>(), AppCaches.Disabled, NullLoggerFactory.Instance, ShortStringHelper, Mock.Of<IEntityService>());
             var umbracoMapper = new UmbracoMapper(new MapDefinitionCollection(new[] { Mock.Of<IMapDefinition>() }));
 
             var umbracoApiController = new FakeUmbracoApiController(new GlobalSettings(), Mock.Of<IUmbracoContextAccessor>(), Mock.Of<IBackOfficeSecurityAccessor>(), Mock.Of<ISqlContext>(), ServiceContext.CreatePartial(), AppCaches.NoCache, profilingLogger , Mock.Of<IRuntimeState>(), umbracoMapper, Mock.Of<IPublishedUrlProvider>());

src/Umbraco.Web.BackOffice/Authorization/BackOfficeHandler.cs

@@ -22,18 +22,17 @@ public BackOfficeHandler(IBackOfficeSecurityAccessor backOfficeSecurity, IRuntim
 
         protected override Task<bool> IsAuthorized(AuthorizationHandlerContext context, BackOfficeRequirement requirement)
         {
-            try
-            {
-                // if not configured (install or upgrade) then we can continue
-                // otherwise we need to ensure that a user is logged in
-                var isAuth = _runtimeState.Level == RuntimeLevel.Install
-                    || _runtimeState.Level == RuntimeLevel.Upgrade
-                    || _backOfficeSecurity.BackOfficeSecurity?.ValidateCurrentUser(false, requirement.RequireApproval) == ValidateRequestAttempt.Success;
-                return Task.FromResult(isAuth);
-            }
-            catch (Exception)
+            // if not configured (install or upgrade) then we can continue
+            // otherwise we need to ensure that a user is logged in
+
+            switch (_runtimeState.Level)
             {
-                return Task.FromResult(false);
+                case RuntimeLevel.Install:
+                case RuntimeLevel.Upgrade:
+                    return Task.FromResult(true);
+                default:
+                    var userApprovalSucceeded = !requirement.RequireApproval || (_backOfficeSecurity.BackOfficeSecurity.CurrentUser?.IsApproved ?? false);
+                    return Task.FromResult(userApprovalSucceeded);
             }
         }
 

src/Umbraco.Web.BackOffice/Authorization/DenyLocalLoginHandler.cs

@@ -1,5 +1,6 @@
 using Microsoft.AspNetCore.Authorization;
 using System.Threading.Tasks;
+using Umbraco.Web.BackOffice.Security;
 using Umbraco.Web.Common.Security;
 
 namespace Umbraco.Web.BackOffice.Authorization