Cleans up IBackofficeSecurity, ensures authn for the AuthenticationController/BackOfficeController

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,41 +9,38 @@ namespace Umbraco.Core.Security
         /// <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>
         /// 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.Core/Security/ValidateRequestAttempt.cs

@@ -1,14 +0,0 @@
-namespace Umbraco.Core.Security
-{
-    public enum ValidateRequestAttempt
-    {
-        Success = 0,
-
-        FailedNoPrivileges = 100,
-
-        //FailedTimedOut,
-
-        FailedNoContextId = 101,
-        FailedNoSsl = 102
-    }
-}

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

@@ -120,8 +120,6 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting
                 .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.Web.BackOffice/Authorization/BackOfficeHandler.cs

@@ -22,18 +22,17 @@ namespace Umbraco.Web.BackOffice.Authorization
 
         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
+
+            switch (_runtimeState.Level)
             {
-                // if not configured (install or upgrade) then we can continue
+                case RuntimeLevel.Install:
-                // otherwise we need to ensure that a user is logged in
+                case RuntimeLevel.Upgrade:
-                var isAuth = _runtimeState.Level == RuntimeLevel.Install
+                    return Task.FromResult(true);
-                    || _runtimeState.Level == RuntimeLevel.Upgrade
+                default:
-                    || _backOfficeSecurity.BackOfficeSecurity?.ValidateCurrentUser(false, requirement.RequireApproval) == ValidateRequestAttempt.Success;
+                    var userApprovalSucceeded = !requirement.RequireApproval || (_backOfficeSecurity.BackOfficeSecurity.CurrentUser?.IsApproved ?? false);
-                return Task.FromResult(isAuth);
+                    return Task.FromResult(userApprovalSucceeded);
-            }
-            catch (Exception)
-            {
-                return Task.FromResult(false);
             }
         }
 

src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs

@@ -51,6 +51,9 @@ namespace Umbraco.Web.BackOffice.Controllers
     [IsBackOffice]
     public class AuthenticationController : UmbracoApiControllerBase
     {
+        // NOTE: Each action must either be explicitly authorized or explicitly [AllowAnonymous], the latter is optional because
+        // this controller itself doesn't require authz but it's more clear what the intention is.
+
         private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor;
         private readonly IBackOfficeUserManager _userManager;
         private readonly IBackOfficeSignInManager _signInManager;
@@ -211,6 +214,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [HttpGet]
+        [AllowAnonymous]
         public async Task<double> GetRemainingTimeoutSeconds()
         {
             // force authentication to occur since this is not an authorized endpoint
@@ -242,6 +246,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// </summary>
         /// <returns></returns>
         [HttpGet]
+        [AllowAnonymous]
         public async Task<bool> IsAuthenticated()
         {
             // force authentication to occur since this is not an authorized endpoint
@@ -399,6 +404,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// </summary>
         /// <returns></returns>
         [SetAngularAntiForgeryTokens]
+        [AllowAnonymous]
         public async Task<ActionResult<IEnumerable<string>>> Get2FAProviders()
         {
             var user = await _signInManager.GetTwoFactorAuthenticationUserAsync();
@@ -413,6 +419,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [SetAngularAntiForgeryTokens]
+        [AllowAnonymous]
         public async Task<IActionResult> PostSend2FACode([FromBody] string provider)
         {
             if (provider.IsNullOrWhiteSpace())
@@ -458,6 +465,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [SetAngularAntiForgeryTokens]
+        [AllowAnonymous]
         public async Task<ActionResult<UserDetail>> PostVerify2FACode(Verify2FACodeModel model)
         {
             if (ModelState.IsValid == false)
@@ -495,6 +503,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// </summary>
         /// <returns></returns>
         [SetAngularAntiForgeryTokens]
+        [AllowAnonymous]
         public async Task<IActionResult> PostSetPassword(SetPasswordModel model)
         {
             var identityUser = await _userManager.FindByIdAsync(model.UserId.ToString());
@@ -559,6 +568,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// </summary>
         /// <returns></returns>
         [ValidateAngularAntiForgeryToken]
+        [AllowAnonymous]
         public async Task<IActionResult> PostLogout()
         {
             // force authentication to occur since this is not an authorized endpoint

src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs

@@ -36,15 +36,19 @@ using Umbraco.Web.BackOffice.Security;
 using Umbraco.Web.Common.ActionsResults;
 using Microsoft.AspNetCore.Authorization;
 using Umbraco.Web.Common.Authorization;
+using Microsoft.AspNetCore.Authentication;
 
 namespace Umbraco.Web.BackOffice.Controllers
 {
     [DisableBrowserCache]
-    //[UmbracoRequireHttps] //TODO Reintroduce
+    [UmbracoRequireHttps]
     [PluginController(Constants.Web.Mvc.BackOfficeArea)]
     [IsBackOffice]
     public class BackOfficeController : UmbracoController
     {
+        // NOTE: Each action must either be explicitly authorized or explicitly [AllowAnonymous], the latter is optional because
+        // this controller itself doesn't require authz but it's more clear what the intention is.
+
         private readonly IBackOfficeUserManager _userManager;
         private readonly IRuntimeMinifier _runtimeMinifier;
         private readonly GlobalSettings _globalSettings;
@@ -96,23 +100,31 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [HttpGet]
+        [AllowAnonymous]
         public async Task<IActionResult> Default()
         {
+            // force authentication to occur since this is not an authorized endpoint
+            var result = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType);            
+
             var viewPath = Path.Combine(_globalSettings.UmbracoPath , Constants.Web.Mvc.BackOfficeArea, nameof(Default) + ".cshtml")
                 .Replace("\\", "/"); // convert to forward slashes since it's a virtual path
 
             return await RenderDefaultOrProcessExternalLoginAsync(
+                result,
                 () => View(viewPath),
                 () => View(viewPath));
         }
 
         [HttpGet]
+        [AllowAnonymous]
         public async Task<IActionResult> VerifyInvite(string invite)
         {
+            var authenticate = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType);
+
             //if you are hitting VerifyInvite, you're already signed in as a different user, and the token is invalid
             //you'll exit on one of the return RedirectToAction(nameof(Default)) but you're still logged in so you just get
             //dumped at the default admin view with no detail
-            if (_backofficeSecurityAccessor.BackOfficeSecurity.IsAuthenticated())
+            if (authenticate.Succeeded)
             {
                 await _signInManager.SignOutAsync();
             }
@@ -174,10 +186,16 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// <returns></returns>
         [HttpGet]
         [StatusCodeResult(System.Net.HttpStatusCode.ServiceUnavailable)]
+        [AllowAnonymous]
         public async Task<IActionResult> AuthorizeUpgrade()
         {
-            var viewPath = Path.Combine(_globalSettings.UmbracoPath, Umbraco.Core.Constants.Web.Mvc.BackOfficeArea, nameof(AuthorizeUpgrade) + ".cshtml");
+            // force authentication to occur since this is not an authorized endpoint
+            var result = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType);
+
+            var viewPath = Path.Combine(_globalSettings.UmbracoPath, Constants.Web.Mvc.BackOfficeArea, nameof(AuthorizeUpgrade) + ".cshtml");
+
             return await RenderDefaultOrProcessExternalLoginAsync(
+                result,
                 //The default view to render when there is no external login info or errors
                 () => View(viewPath),
                 //The IActionResult to perform if external login is successful
@@ -190,6 +208,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// <returns></returns>
         [MinifyJavaScriptResult(Order = 0)]
         [HttpGet]
+        [AllowAnonymous]
         public async Task<IActionResult> Application()
         {
             var result = await _runtimeMinifier.GetScriptForLoadingBackOfficeAsync(_globalSettings, _hostingEnvironment);
@@ -203,6 +222,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// <param name="culture"></param>
         /// <returns></returns>
         [HttpGet]
+        [AllowAnonymous]
         public Dictionary<string, Dictionary<string, string>> LocalizedText(string culture = null)
         {
             var isAuthenticated = _backofficeSecurityAccessor.BackOfficeSecurity.IsAuthenticated();
@@ -265,6 +285,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [HttpPost]
+        [AllowAnonymous]
         public ActionResult ExternalLogin(string provider, string redirectUrl = null)
         {
             if (redirectUrl == null)
@@ -297,6 +318,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         }
 
         [HttpGet]
+        [AllowAnonymous]
         public async Task<IActionResult> ValidatePasswordResetCode([Bind(Prefix = "u")]int userId, [Bind(Prefix = "r")]string resetCode)
         {
             var user = await _userManager.FindByIdAsync(userId.ToString());
@@ -362,6 +384,7 @@ namespace Umbraco.Web.BackOffice.Controllers
         /// </summary>
         /// <returns></returns>
         private async Task<IActionResult> RenderDefaultOrProcessExternalLoginAsync(
+            AuthenticateResult authenticateResult,
             Func<IActionResult> defaultResponse,
             Func<IActionResult> externalSignInResponse)
         {
@@ -382,7 +405,7 @@ namespace Umbraco.Web.BackOffice.Controllers
             if (loginInfo == null || loginInfo.Principal == null)
             {
                 // if the user is not logged in, check if there's any auto login redirects specified
-                if (!_backofficeSecurityAccessor.BackOfficeSecurity.ValidateCurrentUser())
+                if (!authenticateResult.Succeeded)
                 {
                     var oauthRedirectAuthProvider = _externalLogins.GetAutoLoginProvider();
                     if (!oauthRedirectAuthProvider.IsNullOrWhiteSpace())

src/Umbraco.Web.Common/Install/InstallAuthorizeAttribute.cs

@@ -13,38 +13,32 @@ namespace Umbraco.Web.Common.Install
     /// </summary>
     public class InstallAuthorizeAttribute : TypeFilterAttribute
     {
-        // NOTE: This doesn't need to be an authz policy, it's only used for the installer
-
         public InstallAuthorizeAttribute() : base(typeof(InstallAuthorizeFilter))
         {
         }
 
         private class InstallAuthorizeFilter : IAuthorizationFilter
         {
-            private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor;
             private readonly IRuntimeState _runtimeState;
             private readonly ILogger<InstallAuthorizeFilter> _logger;
 
             public InstallAuthorizeFilter(
-                IBackOfficeSecurityAccessor backOfficeSecurityAccessor,
                 IRuntimeState runtimeState,
                 ILogger<InstallAuthorizeFilter> logger)
             {
-                _backOfficeSecurityAccessor = backOfficeSecurityAccessor;
                 _runtimeState = runtimeState;
                 _logger = logger;
             }
 
             public void OnAuthorization(AuthorizationFilterContext authorizationFilterContext)
             {
-                if (!IsAllowed())
+                if (!IsAllowed(authorizationFilterContext))
                 {
                     authorizationFilterContext.Result = new ForbidResult();
                 }
-
             }
 
-            private bool IsAllowed()
+            private bool IsAllowed(AuthorizationFilterContext authorizationFilterContext)
             {
                 try
                 {
@@ -52,7 +46,7 @@ namespace Umbraco.Web.Common.Install
                     // otherwise we need to ensure that a user is logged in
                     return _runtimeState.Level == RuntimeLevel.Install
                            || _runtimeState.Level == RuntimeLevel.Upgrade
-                           || (_backOfficeSecurityAccessor?.BackOfficeSecurity?.ValidateCurrentUser() ?? false);
+                           || (authorizationFilterContext.HttpContext.User?.Identity?.IsAuthenticated ?? false);
                 }
                 catch (Exception ex)
                 {

src/Umbraco.Web.Common/Install/InstallController.cs

@@ -15,6 +15,7 @@ using Umbraco.Web.Install;
 using Umbraco.Web.Security;
 using Umbraco.Core.Configuration.Models;
 using Microsoft.Extensions.Options;
+using Microsoft.AspNetCore.Authentication;
 
 namespace Umbraco.Web.Common.Install
 {
@@ -73,13 +74,11 @@ namespace Umbraco.Web.Common.Install
                 // Update ClientDependency version and delete its temp directories to make sure we get fresh caches
                 _runtimeMinifier.Reset();
 
-                var result = _backofficeSecurityAccessor.BackOfficeSecurity.ValidateCurrentUser(false);
+                var authResult = await HttpContext.AuthenticateAsync(Core.Constants.Security.BackOfficeAuthenticationType);
 
-                switch (result)
+                if (!authResult.Succeeded)
                 {
-                    case ValidateRequestAttempt.FailedNoPrivileges:
+                    return Redirect(_globalSettings.UmbracoPath + "/AuthorizeUpgrade?redir=" + Request.GetEncodedUrl());
-                    case ValidateRequestAttempt.FailedNoContextId:
-                        return Redirect(_globalSettings.UmbracoPath + "/AuthorizeUpgrade?redir=" + Request.GetEncodedUrl());
                 }
             }
 

src/Umbraco.Web.Common/Security/BackofficeSecurity.cs

@@ -72,41 +72,5 @@ namespace Umbraco.Web.Common.Security
             return user.HasSectionAccess(section);
         }
 
-        /// <inheritdoc />
-        public bool ValidateCurrentUser()
-        {
-            return ValidateCurrentUser(false, true) == ValidateRequestAttempt.Success;
-        }
-
-        /// <inheritdoc />
-        public ValidateRequestAttempt ValidateCurrentUser(bool throwExceptions, bool requiresApproval = true)
-        {
-            //This will first check if the current user is already authenticated - which should be the case in nearly all circumstances
-            // since the authentication happens in the Module, that authentication also checks the ticket expiry. We don't
-            // need to check it a second time because that requires another decryption phase and nothing can tamper with it during the request.
-
-            if (IsAuthenticated() == false)
-            {
-                //There is no user
-                if (throwExceptions) throw new InvalidOperationException("The user has no umbraco contextid - try logging in");
-                return ValidateRequestAttempt.FailedNoContextId;
-            }
-
-            var user = CurrentUser;
-
-            // TODO: All of this is done as part of identity/backofficesigninmanager
-            // Check for console access
-            if (user == null || (requiresApproval && user.IsApproved == false) || (user.IsLockedOut && RequestIsInUmbracoApplication(_httpContextAccessor, _globalSettings, _hostingEnvironment)))
-            {
-                if (throwExceptions) throw new ArgumentException("You have no privileges to the umbraco console. Please contact your administrator");
-                return ValidateRequestAttempt.FailedNoPrivileges;
-            }
-            return ValidateRequestAttempt.Success;
-        }
-
-        private static bool RequestIsInUmbracoApplication(IHttpContextAccessor httpContextAccessor, GlobalSettings globalSettings, IHostingEnvironment hostingEnvironment)
-        {
-            return httpContextAccessor.GetRequiredHttpContext().Request.Path.ToString().IndexOf(hostingEnvironment.ToAbsolute(globalSettings.UmbracoPath), StringComparison.InvariantCultureIgnoreCase) > -1;
-        }
     }
 }

src/Umbraco.Web/Mvc/UmbracoAuthorizeAttribute.cs

@@ -76,7 +76,7 @@ namespace Umbraco.Web.Mvc
                 // otherwise we need to ensure that a user is logged in
                 return RuntimeState.Level == RuntimeLevel.Install
                     || RuntimeState.Level == RuntimeLevel.Upgrade
-                    || BackOfficeSecurity.ValidateCurrentUser();
+                    || (httpContext.User?.Identity?.IsAuthenticated ?? false);
             }
             catch (Exception)
             {

src/Umbraco.Web/Security/BackofficeSecurity.cs

@@ -11,11 +11,6 @@ namespace Umbraco.Web.Security
     {
         public IUser CurrentUser => throw new NotImplementedException();
 
-        public ValidateRequestAttempt AuthorizeRequest(bool throwExceptions = false)
-        {
-            throw new NotImplementedException();
-        }
-
         public Attempt<int> GetUserId()
         {
             throw new NotImplementedException();
@@ -31,14 +26,5 @@ namespace Umbraco.Web.Security
             throw new NotImplementedException();
         }
 
-        public bool ValidateCurrentUser()
-        {
-            throw new NotImplementedException();
-        }
-
-        public ValidateRequestAttempt ValidateCurrentUser(bool throwExceptions, bool requiresApproval = true)
-        {
-            throw new NotImplementedException();
-        }
     }
 }

src/Umbraco.Web/WebApi/UmbracoAuthorizeAttribute.cs

@@ -53,17 +53,17 @@ namespace Umbraco.Web.WebApi
                 return true;
             }
 
-            try
+            // if not configured (install or upgrade) then we can continue
+            // otherwise we need to ensure that a user is logged in
+
+            switch (_runtimeState.Level)
             {
-                // if not configured (install or upgrade) then we can continue
+                case RuntimeLevel.Install:
-                // otherwise we need to ensure that a user is logged in
+                case RuntimeLevel.Upgrade:
-                return RuntimeState.Level == RuntimeLevel.Install
+                    return true;
-                    || RuntimeState.Level == RuntimeLevel.Upgrade
+                default:
-                    || BackOfficeSecurity.ValidateCurrentUser(false, _requireApproval) == ValidateRequestAttempt.Success;
+                    var userApprovalSucceeded = !_requireApproval || (BackOfficeSecurity.CurrentUser?.IsApproved ?? false);
-            }
+                    return userApprovalSucceeded;
-            catch (Exception)
-            {
-                return false;
             }
         }
     }