From 6176046053bb3c751c4d786f357435a1a70a8e24 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 30 Nov 2020 22:23:10 +1100 Subject: [PATCH] Deals with the Xsrf notes and handling in sign in manager --- .../BackOffice/IBackOfficeUserManager.cs | 7 ++++ .../Controllers/BackOfficeController.cs | 36 ++++++++----------- .../Security/AutoLinkSignInResult.cs | 5 +-- .../Security/BackOfficeSignInManager.cs | 33 +++++++++++++---- 4 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/Umbraco.Infrastructure/BackOffice/IBackOfficeUserManager.cs b/src/Umbraco.Infrastructure/BackOffice/IBackOfficeUserManager.cs index ca22567418..ee61359a70 100644 --- a/src/Umbraco.Infrastructure/BackOffice/IBackOfficeUserManager.cs +++ b/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 @@ namespace Umbraco.Core.BackOffice public interface IBackOfficeUserManager: IDisposable where TUser : BackOfficeIdentityUser { + Task GetUserIdAsync(TUser user); + + Task GetUserAsync(ClaimsPrincipal principal); + + string GetUserId(ClaimsPrincipal principal); + Task> GetLoginsAsync(TUser user); Task DeleteAsync(TUser user); diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index 3998a65cd6..8f406e451b 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -268,10 +268,9 @@ namespace Umbraco.Web.BackOffice.Controllers redirectUrl = Url.Action(nameof(Default), this.GetControllerName()); } + // Configures the redirect URL and user identifier for the specified external login var properties = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl); - // TODO: I believe we will have to fill in our own XsrfKey like we use to do since I think - // we validate against that key? - // see https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web/Editors/ChallengeResult.cs#L48 + return Challenge(properties, provider); } @@ -286,10 +285,10 @@ namespace Umbraco.Web.BackOffice.Controllers { // Request a redirect to the external login provider to link a login for the current user var redirectUrl = Url.Action(nameof(ExternalLinkLoginCallback), this.GetControllerName()); - var properties = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl, User.Identity.GetUserId()); - // TODO: I believe we will have to fill in our own XsrfKey like we use to do since I think - // we validate against that key? - // see https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web/Editors/ChallengeResult.cs#L48 + + // Configures the redirect URL and user identifier for the specified external login including xsrf data + var properties = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl, _userManager.GetUserId(User)); + return Challenge(properties, provider); } @@ -320,12 +319,15 @@ namespace Umbraco.Web.BackOffice.Controllers [HttpGet] public async Task ExternalLinkLoginCallback() { - // TODO: Do we need/want to tell it an expected xsrf. - // In v8 the xsrf used to be set to the user id which was verified manually, in this case I think we don't specify - // the key and that is up to the underlying sign in manager to set so we'd just tell it to expect the user id, - // the XSRF value used to be set in our ChallengeResult but now we don't have that so this needs to be set in the - // BackOfficeController when we issue a Challenge, see TODO notes there. - var loginInfo = await _signInManager.GetExternalLoginInfoAsync(); + var user = await _userManager.GetUserAsync(User); + if (user == null) + { + // ... this should really not happen + TempData[ViewDataExtensions.TokenExternalSignInError] = new[] { "Local user does not exist" }; + return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); + } + + var loginInfo = await _signInManager.GetExternalLoginInfoAsync(await _userManager.GetUserIdAsync(user)); if (loginInfo == null) { @@ -334,14 +336,6 @@ namespace Umbraco.Web.BackOffice.Controllers return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); } - var user = await _userManager.FindByIdAsync(User.Identity.GetUserId()); - if (user == null) - { - // ... this should really not happen - TempData[ViewDataExtensions.TokenExternalSignInError] = new[] { "Local user does not exist" }; - return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); - } - var addLoginResult = await _userManager.AddLoginAsync(user, loginInfo); if (addLoginResult.Succeeded) { diff --git a/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs b/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs index da9b5b311c..06481d1d86 100644 --- a/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs +++ b/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs @@ -1,11 +1,12 @@ using Microsoft.AspNetCore.Identity; using System; -using System.Collections; using System.Collections.Generic; -using System.Linq; namespace Umbraco.Web.Common.Security { + /// + /// Result returned from signing in when auto-linking takes place + /// public class AutoLinkSignInResult : SignInResult { public static AutoLinkSignInResult FailedNotLinked = new AutoLinkSignInResult() diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index bb4928b1f4..47e0e249e8 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -24,9 +24,9 @@ namespace Umbraco.Web.Common.Security public class BackOfficeSignInManager : SignInManager { // borrowed from https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs - private const string LoginProviderKey = "LoginProvider"; + private const string UmbracoSignInMgrLoginProviderKey = "LoginProvider"; // borrowed from https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs - private const string XsrfKey = "XsrfId"; + private const string UmbracoSignInMgrXsrfKey = "XsrfId"; private BackOfficeUserManager _userManager; private readonly IBackOfficeExternalLoginProviders _externalLogins; @@ -277,18 +277,18 @@ namespace Umbraco.Web.Common.Security var auth = await Context.AuthenticateAsync(Constants.Security.BackOfficeExternalAuthenticationType); var items = auth?.Properties?.Items; - if (auth?.Principal == null || items == null || !items.ContainsKey(LoginProviderKey)) + if (auth?.Principal == null || items == null || !items.ContainsKey(UmbracoSignInMgrLoginProviderKey)) { return null; } if (expectedXsrf != null) { - if (!items.ContainsKey(XsrfKey)) + if (!items.ContainsKey(UmbracoSignInMgrXsrfKey)) { return null; } - var userId = items[XsrfKey] as string; + var userId = items[UmbracoSignInMgrXsrfKey]; if (userId != expectedXsrf) { return null; @@ -296,7 +296,7 @@ namespace Umbraco.Web.Common.Security } var providerKey = auth.Principal.FindFirstValue(ClaimTypes.NameIdentifier); - var provider = items[LoginProviderKey] as string; + var provider = items[UmbracoSignInMgrLoginProviderKey] as string; if (providerKey == null || provider == null) { return null; @@ -348,6 +348,27 @@ namespace Umbraco.Web.Common.Security return await SignInOrTwoFactorAsync(user, isPersistent, loginInfo.LoginProvider, bypassTwoFactor); } + /// + /// Configures the redirect URL and user identifier for the specified external login . + /// + /// The provider to configure. + /// The external login URL users should be redirected to during the login flow. + /// The current user's identifier, which will be used to provide CSRF protection. + /// A configured . + public override AuthenticationProperties ConfigureExternalAuthenticationProperties(string provider, string redirectUrl, string userId = null) + { + // borrowed from https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs + // to be able to use our own XsrfKey/LoginProviderKey because the default is private :/ + + var properties = new AuthenticationProperties { RedirectUri = redirectUrl }; + properties.Items[UmbracoSignInMgrLoginProviderKey] = provider; + if (userId != null) + { + properties.Items[UmbracoSignInMgrXsrfKey] = userId; + } + return properties; + } + public override Task> GetExternalAuthenticationSchemesAsync() { // TODO: We can filter these so that they only include the back office ones.