From 1cfc4907515522161649b1d078fccae5f2d6cd6e Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 27 May 2020 18:27:49 +1000 Subject: [PATCH] More signinmanager, signin now works and we set the user principal in the correct place on login --- .../BackOffice/BackOfficeUserManager.cs | 12 +- .../Controllers/AuthenticationController.cs | 19 +-- .../Controllers/BackOfficeController.cs | 3 +- .../Controllers/BackOfficeServerVariables.cs | 4 +- .../HtmlHelperBackOfficeExtensions.cs | 8 +- ...coBackOfficeServiceCollectionExtensions.cs | 13 +- .../Security/BackOfficeSignInManager.cs | 132 +++++++++++++++++- ...ConfigureUmbracoBackOfficeCookieOptions.cs | 12 ++ .../Views/BackOffice/Default.cshtml | 8 +- .../Editors/AuthenticationController.cs | 8 +- .../Security/BackOfficeSignInManager.cs | 37 ----- 11 files changed, 179 insertions(+), 77 deletions(-) diff --git a/src/Umbraco.Infrastructure/BackOffice/BackOfficeUserManager.cs b/src/Umbraco.Infrastructure/BackOffice/BackOfficeUserManager.cs index 4f06db6471..0ef3ff47ff 100644 --- a/src/Umbraco.Infrastructure/BackOffice/BackOfficeUserManager.cs +++ b/src/Umbraco.Infrastructure/BackOffice/BackOfficeUserManager.cs @@ -362,8 +362,16 @@ namespace Umbraco.Core.BackOffice var currentUserId = umbIdentity?.GetUserId() ?? Constants.Security.SuperUserId; 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; + var ip = IpResolver.GetCurrentRequestIpAddress(); + return new IdentityAuditEventArgs(auditEvent, ip, currentUserId, string.Empty, affectedUserId, affectedUsername); + } + // TODO: Review where these are raised and see if they can be simplified and either done in the this usermanager or the signin manager, lastly we'll resort to the authenticaiton controller + // In some cases it will be nicer/easier to not pass in IPrincipal public void RaiseAccountLockedEvent(IPrincipal currentUser, int userId) => OnAccountLocked(CreateArgs(AuditEvent.AccountLocked, currentUser, userId, string.Empty)); public void RaiseAccountUnlockedEvent(IPrincipal currentUser, int userId) => OnAccountUnlocked(CreateArgs(AuditEvent.AccountUnlocked, currentUser, userId, string.Empty)); @@ -378,7 +386,7 @@ namespace Umbraco.Core.BackOffice public void RaiseLoginRequiresVerificationEvent(IPrincipal currentUser, int userId) => OnLoginRequiresVerification(CreateArgs(AuditEvent.LoginRequiresVerification, currentUser, userId, string.Empty)); - public void RaiseLoginSuccessEvent(IPrincipal currentUser, int userId) => OnLoginSuccess(CreateArgs(AuditEvent.LoginSucces, currentUser, userId, string.Empty)); + public void RaiseLoginSuccessEvent(BackOfficeIdentityUser currentUser, int userId) => OnLoginSuccess(CreateArgs(AuditEvent.LoginSucces, currentUser, userId, string.Empty)); public void RaiseLogoutSuccessEvent(IPrincipal currentUser, int userId) => OnLogoutSuccess(CreateArgs(AuditEvent.LogoutSuccess, currentUser, userId, string.Empty)); diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index c181b89bce..3a3c936cbe 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -37,6 +37,7 @@ namespace Umbraco.Web.BackOffice.Controllers private readonly IGlobalSettings _globalSettings; // TODO: We need to import the logic from Umbraco.Web.Editors.AuthenticationController + // TODO: We need to review all _userManager.Raise calls since many/most should be on the usermanager or signinmanager, very few should be here public AuthenticationController( IUmbracoContextAccessor umbracoContextAccessor, @@ -84,12 +85,8 @@ namespace Umbraco.Web.BackOffice.Controllers if (result.Succeeded) { - // get the user - var user = _userService.GetByUsername(loginModel.Username); - _userManager.RaiseLoginSuccessEvent(User, user.Id); - - // TODO: This is not correct, no user will be set :/ - return SetPrincipalAndReturnUserDetail(user, HttpContext.User); + // return the user detail + return GetUserDetail(_userService.GetByUsername(loginModel.Username)); } if (result.RequiresTwoFactor) @@ -140,23 +137,19 @@ namespace Umbraco.Web.BackOffice.Controllers } /// - /// This is used when the user is auth'd successfully and we need to return an OK with user details along with setting the current Principal in the request + /// Return the for the given /// /// /// /// - private UserDetail SetPrincipalAndReturnUserDetail(IUser user, ClaimsPrincipal principal) + private UserDetail GetUserDetail(IUser user) { - if (user == null) throw new ArgumentNullException("user"); - if (principal == null) throw new ArgumentNullException(nameof(principal)); + if (user == null) throw new ArgumentNullException(nameof(user)); var userDetail = _umbracoMapper.Map(user); // update the userDetail and set their remaining seconds userDetail.SecondsUntilTimeout = TimeSpan.FromMinutes(_globalSettings.TimeOutInMinutes).TotalSeconds; - // ensure the user is set for the current request - HttpContext.SetPrincipalForRequest(principal); - return userDetail; } } diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index 2cde0936dc..3a22da3a04 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -18,6 +18,7 @@ using Umbraco.Extensions; using Umbraco.Web.BackOffice.Filters; using Umbraco.Web.BackOffice.Security; using Umbraco.Web.Common.ActionResults; +using Umbraco.Web.Common.Attributes; using Umbraco.Web.Models; using Umbraco.Web.WebAssets; using Constants = Umbraco.Core.Constants; @@ -25,7 +26,7 @@ using Constants = Umbraco.Core.Constants; namespace Umbraco.Web.BackOffice.Controllers { - [Area(Constants.Web.Mvc.BackOfficeArea)] + [PluginController(Constants.Web.Mvc.BackOfficeArea)] public class BackOfficeController : Controller { private readonly BackOfficeUserManager _userManager; diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs index 4d3332bc1e..af5fa5b563 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs @@ -400,9 +400,9 @@ namespace Umbraco.Web.BackOffice.Controllers "externalLogins", new Dictionary { { - //TODO: I think this should be: _signInManager.GetExternalAuthenticationSchemesAsync() or however that works - "providers", (await _authenticationSchemeProvider.GetAllSchemesAsync()) + // Filter only external providers + .Where(x => !x.DisplayName.IsNullOrWhiteSpace()) // TODO: We need to filter only back office enabled schemes. // Before we used to have a property bag to check, now we don't so need to investigate the easiest/best // way to do this. We have the type so maybe we check for a marker interface, but maybe there's another way, diff --git a/src/Umbraco.Web.BackOffice/Extensions/HtmlHelperBackOfficeExtensions.cs b/src/Umbraco.Web.BackOffice/Extensions/HtmlHelperBackOfficeExtensions.cs index 5c9d2eab1d..4be24ee4a4 100644 --- a/src/Umbraco.Web.BackOffice/Extensions/HtmlHelperBackOfficeExtensions.cs +++ b/src/Umbraco.Web.BackOffice/Extensions/HtmlHelperBackOfficeExtensions.cs @@ -13,6 +13,7 @@ using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.Hosting; using Umbraco.Core.WebAssets; using Umbraco.Web.BackOffice.Controllers; +using Umbraco.Web.BackOffice.Security; using Umbraco.Web.Features; using Umbraco.Web.Models; using Umbraco.Web.WebApi; @@ -59,14 +60,13 @@ namespace Umbraco.Extensions /// Used to render the script that will pass in the angular "externalLoginInfo" service/value on page load /// /// - /// + /// /// public static async Task AngularValueExternalLoginInfoScriptAsync(this IHtmlHelper html, - IAuthenticationSchemeProvider authenticationSchemeProvider, + BackOfficeSignInManager signInManager, IEnumerable externalLoginErrors) { - //TODO: I think this should be: _signInManager.GetExternalAuthenticationSchemesAsync() or however that works - var providers = await authenticationSchemeProvider.GetAllSchemesAsync(); + var providers = await signInManager.GetExternalAuthenticationSchemesAsync(); var loginProviders = providers // TODO: We need to filter only back office enabled schemes. diff --git a/src/Umbraco.Web.BackOffice/Extensions/UmbracoBackOfficeServiceCollectionExtensions.cs b/src/Umbraco.Web.BackOffice/Extensions/UmbracoBackOfficeServiceCollectionExtensions.cs index ffc209edf9..979d059f2c 100644 --- a/src/Umbraco.Web.BackOffice/Extensions/UmbracoBackOfficeServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.BackOffice/Extensions/UmbracoBackOfficeServiceCollectionExtensions.cs @@ -21,6 +21,12 @@ namespace Umbraco.Extensions public static void AddUmbracoBackOffice(this IServiceCollection services) { services.AddAntiforgery(); + + services + .AddAuthentication(Constants.Security.BackOfficeAuthenticationType) + .AddCookie(Constants.Security.BackOfficeAuthenticationType); + + services.ConfigureOptions(); } /// @@ -42,12 +48,7 @@ namespace Umbraco.Extensions // Configure the options specifically for the UmbracoBackOfficeIdentityOptions instance services.ConfigureOptions(); - //services.TryAddScoped>(); - - services - .AddAuthentication(Constants.Security.BackOfficeAuthenticationType) - .AddCookie(Constants.Security.BackOfficeAuthenticationType); - services.ConfigureOptions(); + //services.TryAddScoped>(); } private static IdentityBuilder BuildUmbracoBackOfficeIdentity(this IServiceCollection services) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 4806d2feb7..404e38c396 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -3,14 +3,22 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Security.Claims; +using System.Threading.Tasks; +using Umbraco.Core; using Umbraco.Core.BackOffice; namespace Umbraco.Web.BackOffice.Security { public class BackOfficeSignInManager : SignInManager { + private readonly BackOfficeUserManager _userManager; + public BackOfficeSignInManager( - UserManager userManager, + BackOfficeUserManager userManager, IHttpContextAccessor contextAccessor, IUserClaimsPrincipalFactory claimsFactory, IOptions optionsAccessor, @@ -19,6 +27,128 @@ namespace Umbraco.Web.BackOffice.Security IUserConfirmation confirmation) : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation) { + _userManager = userManager; + } + + // TODO: Need to migrate more from Umbraco.Web.Security.BackOfficeSignInManager + // Things like dealing with auto-linking, cookie options, and a ton of other stuff. Some might not need to be ported but it + // will be a case by case basis. + // Have a look into RefreshSignInAsync since we might be able to use this new functionality for auto-cookie renewal in our middleware, though + // i suspect it's taken care of already. + + public override async Task PasswordSignInAsync(BackOfficeIdentityUser user, string password, bool isPersistent, bool lockoutOnFailure) + { + // override to handle logging/events + var result = await base.PasswordSignInAsync(user, password, isPersistent, lockoutOnFailure); + return HandlePasswordSignIn(user, user.UserName, result); + } + + public override async Task PasswordSignInAsync(string userName, string password, bool isPersistent, bool lockoutOnFailure) + { + // override to handle logging/events + var user = await UserManager.FindByNameAsync(userName); + if (user == null) + return HandlePasswordSignIn(null, userName, SignInResult.Failed); + return await PasswordSignInAsync(user, password, isPersistent, lockoutOnFailure); + } + + public override async Task TwoFactorSignInAsync(string provider, string code, bool isPersistent, bool rememberClient) + { + // override to handle logging/events + var result = await base.TwoFactorSignInAsync(provider, code, isPersistent, rememberClient); + var user = await GetTwoFactorAuthenticationUserAsync(); // will never be null if the above succeeds + return HandlePasswordSignIn(user, user?.UserName, result); + } + + public override bool IsSignedIn(ClaimsPrincipal principal) + { + // override to replace IdentityConstants.ApplicationScheme with Constants.Security.BackOfficeAuthenticationType + // code taken from aspnetcore: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs + + if (principal == null) throw new ArgumentNullException(nameof(principal)); + return principal?.Identities != null && + principal.Identities.Any(i => i.AuthenticationType == Constants.Security.BackOfficeAuthenticationType); + } + + public override async Task RefreshSignInAsync(BackOfficeIdentityUser user) + { + // override to replace IdentityConstants.ApplicationScheme with Constants.Security.BackOfficeAuthenticationType + // code taken from aspnetcore: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs + + var auth = await Context.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType); + var claims = new List(); + var authenticationMethod = auth?.Principal?.FindFirst(ClaimTypes.AuthenticationMethod); + if (authenticationMethod != null) + claims.Add(authenticationMethod); + var amr = auth?.Principal?.FindFirst("amr"); + if (amr != null) + claims.Add(amr); + await SignInWithClaimsAsync(user, auth?.Properties, claims); + } + + public override async Task SignInWithClaimsAsync(BackOfficeIdentityUser user, AuthenticationProperties authenticationProperties, IEnumerable additionalClaims) + { + // override to replace IdentityConstants.ApplicationScheme with Constants.Security.BackOfficeAuthenticationType + // code taken from aspnetcore: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs + // we also override to set the current HttpContext principal since this isn't done by default + + var userPrincipal = await CreateUserPrincipalAsync(user); + foreach (var claim in additionalClaims) + userPrincipal.Identities.First().AddClaim(claim); + + // FYI (just for informational purposes): + // This calls an ext method will eventually reaches `IAuthenticationService.SignInAsync` + // which then resolves the `IAuthenticationSignInHandler` for the current scheme + // by calling `IAuthenticationHandlerProvider.GetHandlerAsync(context, scheme);` + // which then calls `IAuthenticationSignInHandler.SignInAsync` = CookieAuthenticationHandler.HandleSignInAsync + + // Also note, that when the CookieAuthenticationHandler sign in is successful we handle that event within our + // own ConfigureUmbracoBackOfficeCookieOptions which assigns the current HttpContext.User to the IPrincipal created + + await Context.SignInAsync(Constants.Security.BackOfficeAuthenticationType, + userPrincipal, + authenticationProperties ?? new AuthenticationProperties()); + } + + public override async Task SignOutAsync() + { + // override to replace IdentityConstants.ApplicationScheme with Constants.Security.BackOfficeAuthenticationType + // code taken from aspnetcore: https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs + + await Context.SignOutAsync(Constants.Security.BackOfficeAuthenticationType); + await Context.SignOutAsync(Constants.Security.BackOfficeExternalAuthenticationType); + //await Context.SignOutAsync(IdentityConstants.TwoFactorUserIdScheme); + } + + private SignInResult HandlePasswordSignIn(BackOfficeIdentityUser user, string username, SignInResult result) + { + if (username.IsNullOrWhiteSpace()) + username = "UNKNOWN"; // could happen in 2fa or something else weird + + if (result.Succeeded) + { + Logger.LogInformation("User: {UserName} logged in from IP address {IpAddress}", username, Context.Connection.RemoteIpAddress); + if (user != null) + _userManager.RaiseLoginSuccessEvent(user, user.Id); + } + else if (result.IsLockedOut) + { + Logger.LogInformation("Login attempt failed for username {UserName} from IP address {IpAddress}, the user is locked", username, Context.Connection.RemoteIpAddress); + } + else if (result.RequiresTwoFactor) + { + Logger.LogInformation("Login attempt requires verification for username {UserName} from IP address {IpAddress}", username, Context.Connection.RemoteIpAddress); + } + else if (!result.Succeeded || result.IsNotAllowed) + { + Logger.LogInformation("Login attempt failed for username {UserName} from IP address {IpAddress}", username, Context.Connection.RemoteIpAddress); + } + else + { + throw new ArgumentOutOfRangeException(); + } + + return result; } } } diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureUmbracoBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureUmbracoBackOfficeCookieOptions.cs index 44eb923b40..70172bed37 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureUmbracoBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureUmbracoBackOfficeCookieOptions.cs @@ -1,4 +1,5 @@ using System; +using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.DataProtection; @@ -73,6 +74,17 @@ namespace Umbraco.Web.BackOffice.Security _globalSettings, _requestCache); // _explicitPaths); TODO: Implement this once we do OAuth somehow + + + options.Events = new CookieAuthenticationEvents + { + OnSignedIn = ctx => + { + // When we are signed in with the cookie, assign the principal to the current HttpContext + ctx.HttpContext.User = ctx.Principal; + return Task.CompletedTask; + } + }; } public void Configure(CookieAuthenticationOptions options) diff --git a/src/Umbraco.Web.UI.NetCore/Areas/UmbracoBackOffice/Views/BackOffice/Default.cshtml b/src/Umbraco.Web.UI.NetCore/Areas/UmbracoBackOffice/Views/BackOffice/Default.cshtml index 8378762cbc..cfd01a2534 100644 --- a/src/Umbraco.Web.UI.NetCore/Areas/UmbracoBackOffice/Views/BackOffice/Default.cshtml +++ b/src/Umbraco.Web.UI.NetCore/Areas/UmbracoBackOffice/Views/BackOffice/Default.cshtml @@ -2,14 +2,14 @@ @using Umbraco.Web.Composing @using Umbraco.Web @using Umbraco.Web.WebAssets +@using Umbraco.Web.BackOffice.Security @using Umbraco.Core.WebAssets @using Umbraco.Core.Configuration @using Umbraco.Core.Hosting @using Umbraco.Extensions @using Umbraco.Core.Logging @using Umbraco.Web.BackOffice.Controllers -@using Microsoft.AspNetCore.Authentication -@inject IAuthenticationSchemeProvider authenticationSchemeProvider +@inject BackOfficeSignInManager signInManager @inject BackOfficeServerVariables backOfficeServerVariables @inject IUmbracoVersion umbracoVersion @inject IHostingEnvironment hostingEnvironment @@ -111,7 +111,7 @@ - + diff --git a/src/Umbraco.Web/Editors/AuthenticationController.cs b/src/Umbraco.Web/Editors/AuthenticationController.cs index 99ee2293c4..cd2876647c 100644 --- a/src/Umbraco.Web/Editors/AuthenticationController.cs +++ b/src/Umbraco.Web/Editors/AuthenticationController.cs @@ -335,7 +335,6 @@ namespace Umbraco.Web.Editors var user = Services.UserService.GetByUsername(userName); if (result.Succeeded) { - UserManager.RaiseLoginSuccessEvent(User, user.Id); return SetPrincipalAndReturnUserDetail(user, owinContext.Request.User); } @@ -438,12 +437,7 @@ namespace Umbraco.Web.Editors return Request.CreateResponse(HttpStatusCode.OK); } - /// - /// This is used when the user is auth'd successfully and we need to return an OK with user details along with setting the current Principal in the request - /// - /// - /// - /// + // NOTE: This has been migrated to netcore, but in netcore we don't explicitly set the principal in this method, that's done in ConfigureUmbracoBackOfficeCookieOptions so don't worry about that private HttpResponseMessage SetPrincipalAndReturnUserDetail(IUser user, IPrincipal principal) { if (user == null) throw new ArgumentNullException("user"); diff --git a/src/Umbraco.Web/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web/Security/BackOfficeSignInManager.cs index 186b5165bb..021adaed97 100644 --- a/src/Umbraco.Web/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web/Security/BackOfficeSignInManager.cs @@ -65,43 +65,6 @@ namespace Umbraco.Web.Security context.Request); } - /// - /// Sign in the user in using the user name and password - /// - /// - /// - public async Task PasswordSignInAsync(string userName, string password, bool isPersistent, bool shouldLockout) - { - var result = await PasswordSignInAsyncImpl(userName, password, isPersistent, shouldLockout); - - if (result.Succeeded) - { - _logger.WriteCore(TraceEventType.Information, 0, - $"User: {userName} logged in from IP address {_request.RemoteIpAddress}", null, null); - } - else if (result.IsLockedOut) - { - _logger.WriteCore(TraceEventType.Information, 0, - $"Login attempt failed for username {userName} from IP address {_request.RemoteIpAddress}, the user is locked", null, null); - } - else if (result.RequiresTwoFactor) - { - _logger.WriteCore(TraceEventType.Information, 0, - $"Login attempt requires verification for username {userName} from IP address {_request.RemoteIpAddress}", null, null); - } - else if (!result.Succeeded || result.IsNotAllowed) - { - _logger.WriteCore(TraceEventType.Information, 0, - $"Login attempt failed for username {userName} from IP address {_request.RemoteIpAddress}", null, null); - } - else - { - throw new ArgumentOutOfRangeException(); - } - - return result; - } - /// /// Borrowed from Microsoft's underlying sign in manager which is not flexible enough to tell it to use a different cookie type ///