diff --git a/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs b/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs index 93acb279dc..9a60c5d64f 100644 --- a/src/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentity.cs +++ b/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 diff --git a/src/Umbraco.Core/Security/IBackofficeSecurity.cs b/src/Umbraco.Core/Security/IBackofficeSecurity.cs index 12d6dc1688..8d0e0df6d8 100644 --- a/src/Umbraco.Core/Security/IBackofficeSecurity.cs +++ b/src/Umbraco.Core/Security/IBackofficeSecurity.cs @@ -9,41 +9,38 @@ namespace Umbraco.Core.Security /// /// Gets the current user. /// - /// The current user. + /// The current user that has been authenticated for the request. + /// If authentication hasn't taken place this will be null. + // 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; } /// /// Gets the current user's id. /// - /// + /// The current user's Id that has been authenticated for the request. + /// If authentication hasn't taken place this will be unsuccessful. + // TODO: This should just be an extension method on ClaimsIdentity Attempt GetUserId(); - /// - /// Validates the currently logged in user and ensures they are not timed out - /// - /// - bool ValidateCurrentUser(); - - /// - /// Validates the current user assigned to the request and ensures the stored user data is valid - /// - /// set to true if you want exceptions to be thrown if failed - /// If true requires that the user is approved to be validated - /// - ValidateRequestAttempt ValidateCurrentUser(bool throwExceptions, bool requiresApproval = true); - /// /// Checks if the specified user as access to the app /// /// /// /// + /// If authentication hasn't taken place this will be unsuccessful. + // TODO: Should be part of IBackOfficeUserManager bool UserHasSectionAccess(string section, IUser user); /// /// Ensures that a back office user is logged in /// /// + /// This does not force authentication, that must be done before calls to this are made. + // TODO: Should be removed, this should not be necessary bool IsAuthenticated(); } } diff --git a/src/Umbraco.Core/Security/ValidateRequestAttempt.cs b/src/Umbraco.Core/Security/ValidateRequestAttempt.cs deleted file mode 100644 index a88e18d463..0000000000 --- a/src/Umbraco.Core/Security/ValidateRequestAttempt.cs +++ /dev/null @@ -1,14 +0,0 @@ -namespace Umbraco.Core.Security -{ - public enum ValidateRequestAttempt - { - Success = 0, - - FailedNoPrivileges = 100, - - //FailedTimedOut, - - FailedNoContextId = 101, - FailedNoSsl = 102 - } -} diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs index 664b00b513..23f7e09f5d 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs +++ b/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(), It.IsAny())) .Returns(() => true); diff --git a/src/Umbraco.Web.BackOffice/Authorization/BackOfficeHandler.cs b/src/Umbraco.Web.BackOffice/Authorization/BackOfficeHandler.cs index 6cee04deae..065f60b3db 100644 --- a/src/Umbraco.Web.BackOffice/Authorization/BackOfficeHandler.cs +++ b/src/Umbraco.Web.BackOffice/Authorization/BackOfficeHandler.cs @@ -22,18 +22,17 @@ namespace Umbraco.Web.BackOffice.Authorization protected override Task 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 - // 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) - { - 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); } } diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 7d0e99d0e1..718681ac3b 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/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 GetRemainingTimeoutSeconds() { // force authentication to occur since this is not an authorized endpoint @@ -242,6 +246,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [HttpGet] + [AllowAnonymous] public async Task IsAuthenticated() { // force authentication to occur since this is not an authorized endpoint @@ -399,6 +404,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [SetAngularAntiForgeryTokens] + [AllowAnonymous] public async Task>> Get2FAProviders() { var user = await _signInManager.GetTwoFactorAuthenticationUserAsync(); @@ -413,6 +419,7 @@ namespace Umbraco.Web.BackOffice.Controllers } [SetAngularAntiForgeryTokens] + [AllowAnonymous] public async Task PostSend2FACode([FromBody] string provider) { if (provider.IsNullOrWhiteSpace()) @@ -458,6 +465,7 @@ namespace Umbraco.Web.BackOffice.Controllers } [SetAngularAntiForgeryTokens] + [AllowAnonymous] public async Task> PostVerify2FACode(Verify2FACodeModel model) { if (ModelState.IsValid == false) @@ -495,6 +503,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [SetAngularAntiForgeryTokens] + [AllowAnonymous] public async Task PostSetPassword(SetPasswordModel model) { var identityUser = await _userManager.FindByIdAsync(model.UserId.ToString()); @@ -559,6 +568,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [ValidateAngularAntiForgeryToken] + [AllowAnonymous] public async Task PostLogout() { // force authentication to occur since this is not an authorized endpoint diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index fa9ca98707..59ace894b1 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/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 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 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 /// [HttpGet] [StatusCodeResult(System.Net.HttpStatusCode.ServiceUnavailable)] + [AllowAnonymous] public async Task 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 /// [MinifyJavaScriptResult(Order = 0)] [HttpGet] + [AllowAnonymous] public async Task Application() { var result = await _runtimeMinifier.GetScriptForLoadingBackOfficeAsync(_globalSettings, _hostingEnvironment); @@ -203,6 +222,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [HttpGet] + [AllowAnonymous] public Dictionary> 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 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 /// /// private async Task RenderDefaultOrProcessExternalLoginAsync( + AuthenticateResult authenticateResult, Func defaultResponse, Func 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()) diff --git a/src/Umbraco.Web.Common/Install/InstallAuthorizeAttribute.cs b/src/Umbraco.Web.Common/Install/InstallAuthorizeAttribute.cs index 04f743144e..0989de5ba4 100644 --- a/src/Umbraco.Web.Common/Install/InstallAuthorizeAttribute.cs +++ b/src/Umbraco.Web.Common/Install/InstallAuthorizeAttribute.cs @@ -13,38 +13,32 @@ namespace Umbraco.Web.Common.Install /// 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 _logger; public InstallAuthorizeFilter( - IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IRuntimeState runtimeState, ILogger 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) { diff --git a/src/Umbraco.Web.Common/Install/InstallController.cs b/src/Umbraco.Web.Common/Install/InstallController.cs index 1854d8dfbc..5d8d3bf76f 100644 --- a/src/Umbraco.Web.Common/Install/InstallController.cs +++ b/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: - case ValidateRequestAttempt.FailedNoContextId: - return Redirect(_globalSettings.UmbracoPath + "/AuthorizeUpgrade?redir=" + Request.GetEncodedUrl()); + return Redirect(_globalSettings.UmbracoPath + "/AuthorizeUpgrade?redir=" + Request.GetEncodedUrl()); } } diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index 3d9d1e8622..1ea44b1596 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -72,41 +72,5 @@ namespace Umbraco.Web.Common.Security return user.HasSectionAccess(section); } - /// - public bool ValidateCurrentUser() - { - return ValidateCurrentUser(false, true) == ValidateRequestAttempt.Success; - } - - /// - 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; - } } } diff --git a/src/Umbraco.Web/Mvc/UmbracoAuthorizeAttribute.cs b/src/Umbraco.Web/Mvc/UmbracoAuthorizeAttribute.cs index 8ada898c21..73b2674706 100644 --- a/src/Umbraco.Web/Mvc/UmbracoAuthorizeAttribute.cs +++ b/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) { diff --git a/src/Umbraco.Web/Security/BackofficeSecurity.cs b/src/Umbraco.Web/Security/BackofficeSecurity.cs index 9cd2616c39..839acba834 100644 --- a/src/Umbraco.Web/Security/BackofficeSecurity.cs +++ b/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 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(); - } } } diff --git a/src/Umbraco.Web/WebApi/UmbracoAuthorizeAttribute.cs b/src/Umbraco.Web/WebApi/UmbracoAuthorizeAttribute.cs index dea72b7be9..168c55e71f 100644 --- a/src/Umbraco.Web/WebApi/UmbracoAuthorizeAttribute.cs +++ b/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 - // otherwise we need to ensure that a user is logged in - return RuntimeState.Level == RuntimeLevel.Install - || RuntimeState.Level == RuntimeLevel.Upgrade - || BackOfficeSecurity.ValidateCurrentUser(false, _requireApproval) == ValidateRequestAttempt.Success; - } - catch (Exception) - { - return false; + case RuntimeLevel.Install: + case RuntimeLevel.Upgrade: + return true; + default: + var userApprovalSucceeded = !_requireApproval || (BackOfficeSecurity.CurrentUser?.IsApproved ?? false); + return userApprovalSucceeded; } } }