From 348f69734ba90292e3816d7cea3bcd18ac033bf3 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 27 Nov 2020 13:15:54 +0100 Subject: [PATCH 1/6] Revert "FIxes more of the auth procedure" Signed-off-by: Bjarke Berg --- .../Controllers/AuthenticationController.cs | 12 ++++-------- .../Security/BackOfficeSignInManager.cs | 3 +-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 0d338291e9..e9f9c9fa69 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -216,7 +216,7 @@ namespace Umbraco.Web.BackOffice.Controllers return 0; } - var remainingSeconds = result.Principal.GetRemainingAuthSeconds(); + var remainingSeconds = HttpContext.User.GetRemainingAuthSeconds(); if (remainingSeconds <= 30) { var username = result.Principal.FindFirst(ClaimTypes.Name)?.Value; @@ -572,17 +572,13 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [ValidateAngularAntiForgeryToken] - public async Task PostLogout() + public IActionResult PostLogout() { - // force authentication to occur since this is not an authorized endpoint - var result = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType); - if (!result.Succeeded) return Ok(); - - await _signInManager.SignOutAsync(); + HttpContext.SignOutAsync(Constants.Security.BackOfficeAuthenticationType); _logger.LogInformation("User {UserName} from IP address {RemoteIpAddress} has logged out", User.Identity == null ? "UNKNOWN" : User.Identity.Name, HttpContext.Connection.RemoteIpAddress); - var userId = int.Parse(result.Principal.Identity.GetUserId()); + var userId = int.Parse(User.Identity.GetUserId()); var args = _userManager.RaiseLogoutSuccessEvent(User, userId); if (!args.SignOutRedirectUrl.IsNullOrWhiteSpace()) { diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index bb4928b1f4..df838856f1 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -200,8 +200,7 @@ namespace Umbraco.Web.Common.Security await Context.SignOutAsync(Constants.Security.BackOfficeAuthenticationType); await Context.SignOutAsync(Constants.Security.BackOfficeExternalAuthenticationType); - // TODO: Put this back in when we implement it - //await Context.SignOutAsync(Constants.Security.BackOfficeTwoFactorAuthenticationType); + await Context.SignOutAsync(Constants.Security.BackOfficeTwoFactorAuthenticationType); } From 96ef2fd9b7abf55f47b7aa1aeee7cbdf1421a142 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 27 Nov 2020 13:16:22 +0100 Subject: [PATCH 2/6] Revert "Cleans up BackofficeSecurity, fixes up AuthenticationController for endpoints that aren't authorized (and simplifies)" Signed-off-by: Bjarke Berg --- .../BackOffice/ClaimsPrincipalExtensions.cs | 8 ++--- .../Security/IBackofficeSecurity.cs | 7 +++++ .../Controllers/AuthenticationController.cs | 29 ++++++++----------- ...uthenticateAsBackOfficeSchemeConvention.cs | 16 ++++++++++ .../BackOfficeApplicationModelProvider.cs | 3 +- .../EnsureUmbracoBackOfficeAuthentication.cs | 25 ++++++++++++++++ .../UmbracoBackOfficeAuthorizeAttribute.cs | 9 +----- .../Security/BackofficeSecurity.cs | 13 ++++++++- 8 files changed, 78 insertions(+), 32 deletions(-) create mode 100644 src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs create mode 100644 src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs diff --git a/src/Umbraco.Core/BackOffice/ClaimsPrincipalExtensions.cs b/src/Umbraco.Core/BackOffice/ClaimsPrincipalExtensions.cs index 7cbca0428a..6a792ce69b 100644 --- a/src/Umbraco.Core/BackOffice/ClaimsPrincipalExtensions.cs +++ b/src/Umbraco.Core/BackOffice/ClaimsPrincipalExtensions.cs @@ -17,8 +17,6 @@ namespace Umbraco.Extensions /// 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; @@ -55,10 +53,10 @@ namespace Umbraco.Extensions /// public static double GetRemainingAuthSeconds(this IPrincipal user, DateTimeOffset now) { - var claimsPrincipal = user as ClaimsPrincipal; - if (claimsPrincipal == null) return 0; + var umbIdentity = user.GetUmbracoIdentity(); + if (umbIdentity == null) return 0; - var ticketExpires = claimsPrincipal.FindFirst(Constants.Security.TicketExpiresClaimType)?.Value; + var ticketExpires = umbIdentity.FindFirstValue(Constants.Security.TicketExpiresClaimType); if (ticketExpires.IsNullOrWhiteSpace()) return 0; var utcExpired = DateTimeOffset.Parse(ticketExpires, null, DateTimeStyles.RoundtripKind); diff --git a/src/Umbraco.Core/Security/IBackofficeSecurity.cs b/src/Umbraco.Core/Security/IBackofficeSecurity.cs index 12d6dc1688..4ba20f7bfa 100644 --- a/src/Umbraco.Core/Security/IBackofficeSecurity.cs +++ b/src/Umbraco.Core/Security/IBackofficeSecurity.cs @@ -32,6 +32,13 @@ namespace Umbraco.Core.Security /// ValidateRequestAttempt ValidateCurrentUser(bool throwExceptions, bool requiresApproval = true); + /// + /// Authorizes the full request, checks for SSL and validates the current user + /// + /// set to true if you want exceptions to be thrown if failed + /// + ValidateRequestAttempt AuthorizeRequest(bool throwExceptions = false); + /// /// Checks if the specified user as access to the app /// diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index e9f9c9fa69..bf1271c910 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.Linq; using System.Net; -using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Identity; @@ -207,26 +206,18 @@ namespace Umbraco.Web.BackOffice.Controllers } [HttpGet] - public async Task GetRemainingTimeoutSeconds() + public double GetRemainingTimeoutSeconds() { - // force authentication to occur since this is not an authorized endpoint - var result = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType); - if (!result.Succeeded) - { - return 0; - } - + var backOfficeIdentity = HttpContext.User.GetUmbracoIdentity(); var remainingSeconds = HttpContext.User.GetRemainingAuthSeconds(); - if (remainingSeconds <= 30) + if (remainingSeconds <= 30 && backOfficeIdentity != null) { - var username = result.Principal.FindFirst(ClaimTypes.Name)?.Value; - //NOTE: We are using 30 seconds because that is what is coded into angular to force logout to give some headway in // the timeout process. _logger.LogInformation( "User logged will be logged out due to timeout: {Username}, IP Address: {IPAddress}", - username ?? "unknown", + backOfficeIdentity.Name, _ipResolver.GetCurrentRequestIpAddress()); } @@ -238,11 +229,14 @@ namespace Umbraco.Web.BackOffice.Controllers /// /// [HttpGet] - public async Task IsAuthenticated() + public bool IsAuthenticated() { - // force authentication to occur since this is not an authorized endpoint - var result = await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType); - return result.Succeeded; + var attempt = _backofficeSecurityAccessor.BackOfficeSecurity.AuthorizeRequest(); + if (attempt == ValidateRequestAttempt.Success) + { + return true; + } + return false; } /// @@ -592,6 +586,7 @@ namespace Umbraco.Web.BackOffice.Controllers } + /// /// Return the for the given /// diff --git a/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs b/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs new file mode 100644 index 0000000000..838cc59ac4 --- /dev/null +++ b/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs @@ -0,0 +1,16 @@ +using Microsoft.AspNetCore.Mvc.ApplicationModels; +using Umbraco.Web.Common.Filters; + +namespace Umbraco.Web.Common.ApplicationModels +{ + /// + /// Ensures all requests with this convention are authenticated with the back office scheme + /// + public class AuthenticateAsBackOfficeSchemeConvention : IActionModelConvention + { + public void Apply(ActionModel action) + { + action.Filters.Add(new EnsureUmbracoBackOfficeAuthentication()); + } + } +} diff --git a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs index 11d82d4db5..dc0816e1e2 100644 --- a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs +++ b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs @@ -17,7 +17,8 @@ namespace Umbraco.Web.Common.ApplicationModels { ActionModelConventions = new List() { - new BackOfficeIdentityCultureConvention() + new BackOfficeIdentityCultureConvention(), + new AuthenticateAsBackOfficeSchemeConvention() }; } diff --git a/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs b/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs new file mode 100644 index 0000000000..5ad43dc922 --- /dev/null +++ b/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs @@ -0,0 +1,25 @@ +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc.Filters; +using Umbraco.Web.Common.ApplicationModels; +using Umbraco.Web.Common.Attributes; + +namespace Umbraco.Web.Common.Filters +{ + + /// + /// Assigned as part of the umbraco back office application model + /// to always ensure that back office authentication occurs for all controller/actions with + /// applied. + /// + public class EnsureUmbracoBackOfficeAuthentication : IAuthorizationFilter, IAuthorizeData + { + // Implements IAuthorizeData only to return the back office scheme + public string AuthenticationSchemes { get; set; } = Umbraco.Core.Constants.Security.BackOfficeAuthenticationType; + public string Policy { get; set; } + public string Roles { get; set; } + + public void OnAuthorization(AuthorizationFilterContext context) + { + } + } +} diff --git a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs index 5ddf285601..40f534f289 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs @@ -6,15 +6,8 @@ namespace Umbraco.Web.Common.Filters /// /// Ensures authorization is successful for a back office user. /// - public class UmbracoBackOfficeAuthorizeAttribute : TypeFilterAttribute, IAuthorizeData + public class UmbracoBackOfficeAuthorizeAttribute : TypeFilterAttribute { - // Implements IAuthorizeData to return the back office scheme so that all requests with this attributes - // get authenticated with this scheme. - // TODO: We'll have to refactor this as part of the authz policy changes. - public string AuthenticationSchemes { get; set; } = Umbraco.Core.Constants.Security.BackOfficeAuthenticationType; - public string Policy { get; set; } - public string Roles { get; set; } - /// /// Default constructor /// diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index 3d9d1e8622..23ff63a328 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -52,6 +52,18 @@ namespace Umbraco.Web.Common.Security } } + /// + public ValidateRequestAttempt AuthorizeRequest(bool throwExceptions = false) + { + // check for secure connection + if (_globalSettings.UseHttps && !_httpContextAccessor.GetRequiredHttpContext().Request.IsHttps) + { + if (throwExceptions) throw new SecurityException("This installation requires a secure connection (via SSL). Please update the URL to include https://"); + return ValidateRequestAttempt.FailedNoSsl; + } + return ValidateCurrentUser(throwExceptions); + } + /// public Attempt GetUserId() { @@ -94,7 +106,6 @@ namespace Umbraco.Web.Common.Security 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))) { From 7d610202145bc847f431e3d85bf31691045d72d7 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 27 Nov 2020 13:16:50 +0100 Subject: [PATCH 3/6] Revert "Ensures that all back office controllers are authenticated under the back office scheme" Signed-off-by: Bjarke Berg --- .../Controllers/AuthenticationController.cs | 2 +- .../Controllers/BackOfficeController.cs | 1 - .../PublishedSnapshotCacheStatusController.cs | 3 ++- .../Extensions/UmbracoBuilderExtensions.cs | 10 ++++++-- .../IBackOfficeExternalLoginProviders.cs | 11 +++----- ...uthenticateAsBackOfficeSchemeConvention.cs | 16 ------------ .../BackOfficeApplicationModelProvider.cs | 14 ++++++----- .../BackOfficeIdentityCultureConvention.cs | 3 --- ...racoApiBehaviorApplicationModelProvider.cs | 3 +-- .../UmbracoJsonModelBinderConvention.cs | 5 ---- .../EnsureUmbracoBackOfficeAuthentication.cs | 25 ------------------- .../UmbracoBackOfficeAuthorizeAttribute.cs | 3 +-- .../UmbracoBackOfficeAuthorizeFilter.cs | 2 -- 13 files changed, 24 insertions(+), 74 deletions(-) delete mode 100644 src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs delete mode 100644 src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index bf1271c910..9d76e58982 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -45,7 +45,7 @@ namespace Umbraco.Web.BackOffice.Controllers [PluginController(Constants.Web.Mvc.BackOfficeApiArea)] // TODO: Maybe this could be applied with our Application Model conventions //[ValidationFilter] // TODO: I don't actually think this is required with our custom Application Model conventions applied [AngularJsonOnlyConfiguration] // TODO: This could be applied with our Application Model conventions - [IsBackOffice] + [IsBackOffice] // TODO: This could be applied with our Application Model conventions public class AuthenticationController : UmbracoApiControllerBase { private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index 3998a65cd6..83e94c1e30 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -41,7 +41,6 @@ namespace Umbraco.Web.BackOffice.Controllers [DisableBrowserCache] //[UmbracoRequireHttps] //TODO Reintroduce [PluginController(Constants.Web.Mvc.BackOfficeArea)] - [IsBackOffice] public class BackOfficeController : UmbracoController { private readonly IBackOfficeUserManager _userManager; diff --git a/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs b/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs index c9c420f254..1a0e3457ca 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs @@ -8,7 +8,8 @@ using Umbraco.Web.PublishedCache; namespace Umbraco.Web.BackOffice.Controllers { - [PluginController(Constants.Web.Mvc.BackOfficeApiArea)] + [PluginController(Constants.Web.Mvc.BackOfficeApiArea)] + [IsBackOffice] public class PublishedSnapshotCacheStatusController : UmbracoAuthorizedApiController { private readonly IPublishedSnapshotService _publishedSnapshotService; diff --git a/src/Umbraco.Web.BackOffice/Extensions/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.BackOffice/Extensions/UmbracoBuilderExtensions.cs index fe6eacbb35..75d53c91de 100644 --- a/src/Umbraco.Web.BackOffice/Extensions/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.BackOffice/Extensions/UmbracoBuilderExtensions.cs @@ -32,9 +32,15 @@ namespace Umbraco.Extensions builder.Services.AddAntiforgery(); builder.Services.AddSingleton(); + // TODO: We need to see if we are 'allowed' to do this, the docs say: + // "The call to AddIdentity configures the default scheme settings. The AddAuthentication(String) overload sets the DefaultScheme property. The AddAuthentication(Action) overload allows configuring authentication options, which can be used to set up default authentication schemes for different purposes. Subsequent calls to AddAuthentication override previously configured AuthenticationOptions properties." + // So if someone calls services.AddAuthentication() ... in Startup does that overwrite all of this? + // It also says "When the app requires multiple providers, chain the provider extension methods behind AddAuthentication" + // Which leads me to believe it all gets overwritten? :/ + // UPDATE: I have tested this breifly in Startup doing Services.AddAuthentication().AddGoogle() ... and the back office auth + // still seems to work. We'll see how it goes i guess. builder.Services - .AddAuthentication() // This just creates a builder, nothing more - // Add our custom schemes which are cookie handlers + .AddAuthentication(Core.Constants.Security.BackOfficeAuthenticationType) .AddCookie(Core.Constants.Security.BackOfficeAuthenticationType) .AddCookie(Core.Constants.Security.BackOfficeExternalAuthenticationType, o => { diff --git a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs index 6b78e58ead..6d0b64e84f 100644 --- a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs @@ -26,11 +26,6 @@ namespace Umbraco.Web.BackOffice.Security _loginProviderOptions = loginProviderOptions; } - public string SchemeForBackOffice(string scheme) - { - return Constants.Security.BackOfficeExternalAuthenticationTypePrefix + scheme; - } - /// /// Overridden to track the final authenticationScheme being registered for the external login /// @@ -41,11 +36,11 @@ namespace Umbraco.Web.BackOffice.Security /// /// public override AuthenticationBuilder AddRemoteScheme(string authenticationScheme, string displayName, Action configureOptions) - { - // Validate that the prefix is set + { + //Ensure the prefix is set if (!authenticationScheme.StartsWith(Constants.Security.BackOfficeExternalAuthenticationTypePrefix)) { - throw new InvalidOperationException($"The {nameof(authenticationScheme)} is not prefixed with {Constants.Security.BackOfficeExternalAuthenticationTypePrefix}. The scheme must be created with a call to the method {nameof(SchemeForBackOffice)}"); + authenticationScheme = Constants.Security.BackOfficeExternalAuthenticationTypePrefix + authenticationScheme; } // add our login provider to the container along with a custom options configuration diff --git a/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs b/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs deleted file mode 100644 index 838cc59ac4..0000000000 --- a/src/Umbraco.Web.Common/ApplicationModels/AuthenticateAsBackOfficeSchemeConvention.cs +++ /dev/null @@ -1,16 +0,0 @@ -using Microsoft.AspNetCore.Mvc.ApplicationModels; -using Umbraco.Web.Common.Filters; - -namespace Umbraco.Web.Common.ApplicationModels -{ - /// - /// Ensures all requests with this convention are authenticated with the back office scheme - /// - public class AuthenticateAsBackOfficeSchemeConvention : IActionModelConvention - { - public void Apply(ActionModel action) - { - action.Filters.Add(new EnsureUmbracoBackOfficeAuthentication()); - } - } -} diff --git a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs index dc0816e1e2..0ad6c4ec1a 100644 --- a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs +++ b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeApplicationModelProvider.cs @@ -6,8 +6,6 @@ using Umbraco.Web.Common.Attributes; namespace Umbraco.Web.Common.ApplicationModels { - // TODO: This should just exist in the back office project - /// /// An application model provider for all Umbraco Back Office controllers /// @@ -17,8 +15,7 @@ namespace Umbraco.Web.Common.ApplicationModels { ActionModelConventions = new List() { - new BackOfficeIdentityCultureConvention(), - new AuthenticateAsBackOfficeSchemeConvention() + new BackOfficeIdentityCultureConvention() }; } @@ -52,7 +49,12 @@ namespace Umbraco.Web.Common.ApplicationModels } private bool IsBackOfficeController(ControllerModel controller) - => controller.Attributes.OfType().Any(); - + { + var pluginControllerAttribute = controller.Attributes.OfType().FirstOrDefault(); + return pluginControllerAttribute != null + && (pluginControllerAttribute.AreaName == Core.Constants.Web.Mvc.BackOfficeArea + || pluginControllerAttribute.AreaName == Core.Constants.Web.Mvc.BackOfficeApiArea + || pluginControllerAttribute.AreaName == Core.Constants.Web.Mvc.BackOfficeTreeArea); + } } } diff --git a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeIdentityCultureConvention.cs b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeIdentityCultureConvention.cs index 0a5a1f9945..d3e2096dd3 100644 --- a/src/Umbraco.Web.Common/ApplicationModels/BackOfficeIdentityCultureConvention.cs +++ b/src/Umbraco.Web.Common/ApplicationModels/BackOfficeIdentityCultureConvention.cs @@ -3,9 +3,6 @@ using Umbraco.Web.Common.Filters; namespace Umbraco.Web.Common.ApplicationModels { - - // TODO: This should just exist in the back office project - public class BackOfficeIdentityCultureConvention : IActionModelConvention { public void Apply(ActionModel action) diff --git a/src/Umbraco.Web.Common/ApplicationModels/UmbracoApiBehaviorApplicationModelProvider.cs b/src/Umbraco.Web.Common/ApplicationModels/UmbracoApiBehaviorApplicationModelProvider.cs index be296969e7..918bc3776f 100644 --- a/src/Umbraco.Web.Common/ApplicationModels/UmbracoApiBehaviorApplicationModelProvider.cs +++ b/src/Umbraco.Web.Common/ApplicationModels/UmbracoApiBehaviorApplicationModelProvider.cs @@ -81,7 +81,6 @@ namespace Umbraco.Web.Common.ApplicationModels } } - private bool IsUmbracoApiController(ControllerModel controller) - => controller.Attributes.OfType().Any(); + private bool IsUmbracoApiController(ControllerModel controller) => controller.Attributes.OfType().Any(); } } diff --git a/src/Umbraco.Web.Common/ApplicationModels/UmbracoJsonModelBinderConvention.cs b/src/Umbraco.Web.Common/ApplicationModels/UmbracoJsonModelBinderConvention.cs index 42d23b33b3..96c60398f0 100644 --- a/src/Umbraco.Web.Common/ApplicationModels/UmbracoJsonModelBinderConvention.cs +++ b/src/Umbraco.Web.Common/ApplicationModels/UmbracoJsonModelBinderConvention.cs @@ -2,9 +2,6 @@ using Microsoft.AspNetCore.Mvc.ModelBinding; using Umbraco.Web.Common.ModelBinding; using System.Linq; -using Umbraco.Web.Common.Attributes; -using Umbraco.Web.Actions; -using Umbraco.Web.Common.Filters; namespace Umbraco.Web.Common.ApplicationModels { @@ -24,6 +21,4 @@ namespace Umbraco.Web.Common.ApplicationModels } } } - - } diff --git a/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs b/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs deleted file mode 100644 index 5ad43dc922..0000000000 --- a/src/Umbraco.Web.Common/Filters/EnsureUmbracoBackOfficeAuthentication.cs +++ /dev/null @@ -1,25 +0,0 @@ -using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Mvc.Filters; -using Umbraco.Web.Common.ApplicationModels; -using Umbraco.Web.Common.Attributes; - -namespace Umbraco.Web.Common.Filters -{ - - /// - /// Assigned as part of the umbraco back office application model - /// to always ensure that back office authentication occurs for all controller/actions with - /// applied. - /// - public class EnsureUmbracoBackOfficeAuthentication : IAuthorizationFilter, IAuthorizeData - { - // Implements IAuthorizeData only to return the back office scheme - public string AuthenticationSchemes { get; set; } = Umbraco.Core.Constants.Security.BackOfficeAuthenticationType; - public string Policy { get; set; } - public string Roles { get; set; } - - public void OnAuthorization(AuthorizationFilterContext context) - { - } - } -} diff --git a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs index 40f534f289..1f4abbaa25 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs @@ -1,5 +1,4 @@ -using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc; namespace Umbraco.Web.Common.Filters { diff --git a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs index c8ae0aacd8..8fad886f27 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs @@ -12,8 +12,6 @@ using IHostingEnvironment = Umbraco.Core.Hosting.IHostingEnvironment; namespace Umbraco.Web.Common.Filters { - - /// /// Ensures authorization is successful for a back office user. /// From efce67fe8a508f93cbade33581c10391ebc8ab93 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 27 Nov 2020 13:17:13 +0100 Subject: [PATCH 4/6] Revert "Moves auto linking logic to the BackOfficeSignInManager" Signed-off-by: Bjarke Berg --- .../Controllers/BackOfficeController.cs | 199 ++++++++++++++---- .../Security/AutoLinkSignInResult.cs | 47 ----- .../BackOfficeExternalLoginProviderOptions.cs | 4 +- .../Security/BackOfficeSignInManager.cs | 167 +-------------- .../UmbracoBackOfficeAuthorizeFilter.cs | 4 +- 5 files changed, 168 insertions(+), 253 deletions(-) delete mode 100644 src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index 83e94c1e30..c905876f51 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -403,55 +403,182 @@ namespace Umbraco.Web.BackOffice.Controllers { if (loginInfo == null) throw new ArgumentNullException(nameof(loginInfo)); if (response == null) throw new ArgumentNullException(nameof(response)); + ExternalSignInAutoLinkOptions autoLinkOptions = null; - // Sign in the user with this external login provider (which auto links, etc...) - var result = await _signInManager.ExternalLoginSignInAsync(loginInfo, isPersistent: false); + var authType = (await _signInManager.GetExternalAuthenticationSchemesAsync()) + .FirstOrDefault(x => x.Name == loginInfo.LoginProvider); - var errors = new List(); - - if (result == Microsoft.AspNetCore.Identity.SignInResult.Success) + if (authType == null) { - + _logger.LogWarning("Could not find external authentication provider registered: {LoginProvider}", loginInfo.LoginProvider); } - else if (result == Microsoft.AspNetCore.Identity.SignInResult.LockedOut) + else { - // TODO: We've never actually dealt with this before - } - else if (result == Microsoft.AspNetCore.Identity.SignInResult.TwoFactorRequired) - { - // TODO: We've never actually dealt with this before - } - else if (result == Microsoft.AspNetCore.Identity.SignInResult.NotAllowed) - { - // TODO: We've never actually dealt with this before - } - else if (result == Microsoft.AspNetCore.Identity.SignInResult.Failed) - { - // Failed only occurs when the user does not exist - errors.Add("The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office."); - } - else if (result == AutoLinkSignInResult.FailedNotLinked) - { - errors.Add("The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office."); - } - else if (result == AutoLinkSignInResult.FailedNoEmail) - { - errors.Add($"The requested provider ({loginInfo.LoginProvider}) has not provided the email claim {ClaimTypes.Email}, the account cannot be linked."); - } - else if (result is AutoLinkSignInResult autoLinkSignInResult && autoLinkSignInResult.Errors.Count > 0) - { - errors.AddRange(autoLinkSignInResult.Errors); + autoLinkOptions = _externalLogins.Get(authType.Name)?.Options?.AutoLinkOptions; } - if (errors.Count > 0) + // Sign in the user with this external login provider if the user already has a login + + var user = await _userManager.FindByLoginAsync(loginInfo.LoginProvider, loginInfo.ProviderKey); + if (user != null) + { + var shouldSignIn = true; + if (autoLinkOptions != null && autoLinkOptions.OnExternalLogin != null) + { + shouldSignIn = autoLinkOptions.OnExternalLogin(user, loginInfo); + if (shouldSignIn == false) + { + _logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, user.Id); + } + } + + if (shouldSignIn) + { + //sign in + await _signInManager.SignInAsync(user, false); + } + } + else + { + if (await AutoLinkAndSignInExternalAccount(loginInfo, autoLinkOptions) == false) + { + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + new[] { "The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office." })); + } + + //Remove the cookie otherwise this message will keep appearing + Response.Cookies.Delete(Constants.Security.BackOfficeExternalCookieName); + } + + return response(); + } + + private async Task AutoLinkAndSignInExternalAccount(ExternalLoginInfo loginInfo, ExternalSignInAutoLinkOptions autoLinkOptions) + { + if (autoLinkOptions == null) + return false; + + if (autoLinkOptions.AutoLinkExternalAccount == false) + { + return false; + } + + var email = loginInfo.Principal.FindFirstValue(ClaimTypes.Email); + + //we are allowing auto-linking/creating of local accounts + if (email.IsNullOrWhiteSpace()) { ViewData.SetExternalSignInProviderErrors( new BackOfficeExternalLoginProviderErrors( loginInfo.LoginProvider, - errors)); + new[] { $"The requested provider ({loginInfo.LoginProvider}) has not provided the email claim {ClaimTypes.Email}, the account cannot be linked." })); + } + else + { + //Now we need to perform the auto-link, so first we need to lookup/create a user with the email address + var autoLinkUser = await _userManager.FindByEmailAsync(email); + if (autoLinkUser != null) + { + try + { + //call the callback if one is assigned + autoLinkOptions.OnAutoLinking?.Invoke(autoLinkUser, loginInfo); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not link login provider {LoginProvider}.", loginInfo.LoginProvider); + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + new[] { "Could not link login provider " + loginInfo.LoginProvider + ". " + ex.Message })); + return true; + } + + await LinkUser(autoLinkUser, loginInfo); + } + else + { + var name = loginInfo.Principal?.Identity?.Name; + if (name.IsNullOrWhiteSpace()) throw new InvalidOperationException("The Name value cannot be null"); + + autoLinkUser = BackOfficeIdentityUser.CreateNew(_globalSettings, email, email, autoLinkOptions.GetUserAutoLinkCulture(_globalSettings), name); + + foreach (var userGroup in autoLinkOptions.DefaultUserGroups) + { + autoLinkUser.AddRole(userGroup); + } + + //call the callback if one is assigned + try + { + autoLinkOptions.OnAutoLinking?.Invoke(autoLinkUser, loginInfo); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not link login provider {LoginProvider}.", loginInfo.LoginProvider); + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + new[] { "Could not link login provider " + loginInfo.LoginProvider + ". " + ex.Message })); + return true; + } + + var userCreationResult = await _userManager.CreateAsync(autoLinkUser); + + if (userCreationResult.Succeeded == false) + { + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + userCreationResult.Errors.Select(x => x.Description).ToList())); + } + else + { + await LinkUser(autoLinkUser, loginInfo); + } + } + } + return true; + } + + private async Task LinkUser(BackOfficeIdentityUser autoLinkUser, ExternalLoginInfo loginInfo) + { + var existingLogins = await _userManager.GetLoginsAsync(autoLinkUser); + var exists = existingLogins.FirstOrDefault(x => x.LoginProvider == loginInfo.LoginProvider && x.ProviderKey == loginInfo.ProviderKey); + + // if it already exists (perhaps it was added in the AutoLink callbak) then we just continue + if (exists != null) + { + //sign in + await _signInManager.SignInAsync(autoLinkUser, isPersistent: false); + return; } - return response(); + var linkResult = await _userManager.AddLoginAsync(autoLinkUser, loginInfo); + if (linkResult.Succeeded) + { + //we're good! sign in + await _signInManager.SignInAsync(autoLinkUser, isPersistent: false); + return; + } + + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + linkResult.Errors.Select(x => x.Description).ToList())); + + //If this fails, we should really delete the user since it will be in an inconsistent state! + var deleteResult = await _userManager.DeleteAsync(autoLinkUser); + if (!deleteResult.Succeeded) + { + //DOH! ... this isn't good, combine all errors to be shown + ViewData.SetExternalSignInProviderErrors( + new BackOfficeExternalLoginProviderErrors( + loginInfo.LoginProvider, + linkResult.Errors.Concat(deleteResult.Errors).Select(x => x.Description).ToList())); + } } private IActionResult RedirectToLocal(string returnUrl) diff --git a/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs b/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs deleted file mode 100644 index f0d79e5ec6..0000000000 --- a/src/Umbraco.Web.BackOffice/Security/AutoLinkSignInResult.cs +++ /dev/null @@ -1,47 +0,0 @@ -using Microsoft.AspNetCore.Identity; -using System; -using System.Collections; -using System.Collections.Generic; -using System.Linq; - -namespace Umbraco.Web.Common.Security -{ - public class AutoLinkSignInResult : SignInResult - { - public static AutoLinkSignInResult FailedNotLinked = new() - { - Succeeded = false - }; - - public static AutoLinkSignInResult FailedNoEmail = new() - { - Succeeded = false - }; - - public static AutoLinkSignInResult FailedException(string error) => new(new[] { error }) - { - Succeeded = false - }; - - public static AutoLinkSignInResult FailedCreatingUser(IReadOnlyCollection errors) => new(errors) - { - Succeeded = false - }; - - public static AutoLinkSignInResult FailedLinkingUser(IReadOnlyCollection errors) => new(errors) - { - Succeeded = false - }; - - public AutoLinkSignInResult(IReadOnlyCollection errors) - { - Errors = errors ?? throw new ArgumentNullException(nameof(errors)); - } - - public AutoLinkSignInResult() - { - } - - public IReadOnlyCollection Errors { get; } = Array.Empty(); - } -} diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs index b6c1c7f2d2..a3ce87e404 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs @@ -11,7 +11,7 @@ namespace Umbraco.Web.BackOffice.Security public class BackOfficeExternalLoginProviderOptions { public BackOfficeExternalLoginProviderOptions( - string buttonStyle, string icon, + string buttonStyle, string icon, string callbackPath, ExternalSignInAutoLinkOptions autoLinkOptions = null, bool denyLocalLogin = false, bool autoRedirectLoginToExternalProvider = false, @@ -19,6 +19,7 @@ namespace Umbraco.Web.BackOffice.Security { ButtonStyle = buttonStyle; Icon = icon; + CallbackPath = callbackPath; AutoLinkOptions = autoLinkOptions ?? new ExternalSignInAutoLinkOptions(); DenyLocalLogin = denyLocalLogin; AutoRedirectLoginToExternalProvider = autoRedirectLoginToExternalProvider; @@ -27,6 +28,7 @@ namespace Umbraco.Web.BackOffice.Security public string ButtonStyle { get; } public string Icon { get; } + public string CallbackPath { get; } /// /// Options used to control how users can be auto-linked/created/updated based on the external login provider diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index df838856f1..6f34a85c79 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -10,9 +10,7 @@ using System.Security.Claims; using System.Threading.Tasks; using Umbraco.Core; using Umbraco.Core.BackOffice; -using Umbraco.Core.Configuration.Models; using Umbraco.Extensions; -using Umbraco.Web.BackOffice.Security; namespace Umbraco.Web.Common.Security { @@ -29,25 +27,18 @@ namespace Umbraco.Web.Common.Security private const string XsrfKey = "XsrfId"; private BackOfficeUserManager _userManager; - private readonly IBackOfficeExternalLoginProviders _externalLogins; - private readonly GlobalSettings _globalSettings; - public BackOfficeSignInManager( BackOfficeUserManager userManager, IHttpContextAccessor contextAccessor, - IBackOfficeExternalLoginProviders externalLogins, IUserClaimsPrincipalFactory claimsFactory, IOptions optionsAccessor, - IOptions globalSettings, ILogger> logger, IAuthenticationSchemeProvider schemes, IUserConfirmation confirmation) : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes, confirmation) { _userManager = userManager; - _externalLogins = externalLogins; - _globalSettings = globalSettings.Value; } // TODO: Need to migrate more from Umbraco.Web.Security.BackOfficeSignInManager @@ -309,51 +300,7 @@ namespace Umbraco.Web.Common.Security }; } - /// - /// Custom ExternalLoginSignInAsync overload for handling external sign in with auto-linking - /// - /// - /// - /// - /// - /// - public async Task ExternalLoginSignInAsync(ExternalLoginInfo loginInfo, bool isPersistent, bool bypassTwoFactor = false) - { - // borrowed from https://github.com/dotnet/aspnetcore/blob/master/src/Identity/Core/src/SignInManager.cs - // to be able to deal with auto-linking and reduce duplicate lookups - - var autoLinkOptions = _externalLogins.Get(loginInfo.LoginProvider)?.Options?.AutoLinkOptions; - var user = await UserManager.FindByLoginAsync(loginInfo.LoginProvider, loginInfo.ProviderKey); - if (user == null) - { - // user doesn't exist so see if we can auto link - return await AutoLinkAndSignInExternalAccount(loginInfo, autoLinkOptions); - } - - if (autoLinkOptions != null && autoLinkOptions.OnExternalLogin != null) - { - var shouldSignIn = autoLinkOptions.OnExternalLogin(user, loginInfo); - if (shouldSignIn == false) - { - Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, user.Id); - } - } - - var error = await PreSignInCheck(user); - if (error != null) - { - return error; - } - return await SignInOrTwoFactorAsync(user, isPersistent, loginInfo.LoginProvider, bypassTwoFactor); - } - - public override Task> GetExternalAuthenticationSchemesAsync() - { - // TODO: We can filter these so that they only include the back office ones. - // That can be done by either checking the scheme (maybe) or comparing it to what we have registered in the collection of BackOfficeExternalLoginProvider - return base.GetExternalAuthenticationSchemesAsync(); - } - + /// protected override async Task SignInOrTwoFactorAsync(BackOfficeIdentityUser user, bool isPersistent, string loginProvider = null, bool bypassTwoFactor = false) { @@ -524,117 +471,5 @@ namespace Umbraco.Web.Common.Security public string UserId { get; set; } public string LoginProvider { get; set; } } - - - /// - /// Used for auto linking/creating user accounts for external logins - /// - /// - /// - /// - private async Task AutoLinkAndSignInExternalAccount(ExternalLoginInfo loginInfo, ExternalSignInAutoLinkOptions autoLinkOptions) - { - // If there are no autolink options then the attempt is failed (user does not exist) - if (autoLinkOptions == null || !autoLinkOptions.AutoLinkExternalAccount) - { - return SignInResult.Failed; - } - - var email = loginInfo.Principal.FindFirstValue(ClaimTypes.Email); - - //we are allowing auto-linking/creating of local accounts - if (email.IsNullOrWhiteSpace()) - { - return AutoLinkSignInResult.FailedNoEmail; - } - else - { - //Now we need to perform the auto-link, so first we need to lookup/create a user with the email address - var autoLinkUser = await UserManager.FindByEmailAsync(email); - if (autoLinkUser != null) - { - try - { - //call the callback if one is assigned - autoLinkOptions.OnAutoLinking?.Invoke(autoLinkUser, loginInfo); - } - catch (Exception ex) - { - Logger.LogError(ex, "Could not link login provider {LoginProvider}.", loginInfo.LoginProvider); - return AutoLinkSignInResult.FailedException(ex.Message); - } - - return await LinkUser(autoLinkUser, loginInfo); - } - else - { - var name = loginInfo.Principal?.Identity?.Name; - if (name.IsNullOrWhiteSpace()) throw new InvalidOperationException("The Name value cannot be null"); - - autoLinkUser = BackOfficeIdentityUser.CreateNew(_globalSettings, email, email, autoLinkOptions.GetUserAutoLinkCulture(_globalSettings), name); - - foreach (var userGroup in autoLinkOptions.DefaultUserGroups) - { - autoLinkUser.AddRole(userGroup); - } - - //call the callback if one is assigned - try - { - autoLinkOptions.OnAutoLinking?.Invoke(autoLinkUser, loginInfo); - } - catch (Exception ex) - { - Logger.LogError(ex, "Could not link login provider {LoginProvider}.", loginInfo.LoginProvider); - return AutoLinkSignInResult.FailedException(ex.Message); - } - - var userCreationResult = await _userManager.CreateAsync(autoLinkUser); - - if (!userCreationResult.Succeeded) - { - return AutoLinkSignInResult.FailedCreatingUser(userCreationResult.Errors.Select(x => x.Description).ToList()); - } - else - { - return await LinkUser(autoLinkUser, loginInfo); - } - } - } - } - - private async Task LinkUser(BackOfficeIdentityUser autoLinkUser, ExternalLoginInfo loginInfo) - { - var existingLogins = await _userManager.GetLoginsAsync(autoLinkUser); - var exists = existingLogins.FirstOrDefault(x => x.LoginProvider == loginInfo.LoginProvider && x.ProviderKey == loginInfo.ProviderKey); - - // if it already exists (perhaps it was added in the AutoLink callbak) then we just continue - if (exists != null) - { - //sign in - return await SignInOrTwoFactorAsync(autoLinkUser, isPersistent: false, loginInfo.LoginProvider); - } - - var linkResult = await _userManager.AddLoginAsync(autoLinkUser, loginInfo); - if (linkResult.Succeeded) - { - //we're good! sign in - return await SignInOrTwoFactorAsync(autoLinkUser, isPersistent: false, loginInfo.LoginProvider); - } - - //If this fails, we should really delete the user since it will be in an inconsistent state! - var deleteResult = await _userManager.DeleteAsync(autoLinkUser); - if (deleteResult.Succeeded) - { - var errors = linkResult.Errors.Select(x => x.Description).ToList(); - return AutoLinkSignInResult.FailedLinkingUser(errors); - } - else - { - //DOH! ... this isn't good, combine all errors to be shown - var errors = linkResult.Errors.Concat(deleteResult.Errors).Select(x => x.Description).ToList(); - return AutoLinkSignInResult.FailedLinkingUser(errors); - } - } } } diff --git a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs index 8fad886f27..0c30b25ced 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs @@ -1,5 +1,4 @@ -using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Routing; using System; @@ -29,7 +28,6 @@ namespace Umbraco.Web.Common.Filters private readonly LinkGenerator _linkGenerator; private readonly bool _redirectToUmbracoLogin; private string _redirectUrl; - private UmbracoBackOfficeAuthorizeFilter( IHostingEnvironment hostingEnvironment, From 1833bfa1a95caabe97a5d9a5369740080684a871 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 27 Nov 2020 13:17:34 +0100 Subject: [PATCH 5/6] Revert "Gets oauth working (with google) now need to test others and debug why the styles aren't working" Signed-off-by: Bjarke Berg --- .../Controllers/AuthenticationController.cs | 2 +- .../Controllers/BackOfficeController.cs | 6 +- .../Controllers/BackOfficeServerVariables.cs | 13 +- .../BackOfficeExternalLoginProviderOptions.cs | 8 +- .../Security/BackOfficeSignInManager.cs | 1 + .../Security/ExternalSignInAutoLinkOptions.cs | 2 + .../IBackOfficeExternalLoginProviders.cs | 131 ++++-------------- .../services/externallogininfo.service.js | 2 +- .../src/views/common/overlays/user/user.html | 6 +- .../components/application/umb-login.html | 2 +- 10 files changed, 47 insertions(+), 126 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 9d76e58982..8189641c3a 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -180,7 +180,7 @@ namespace Umbraco.Web.BackOffice.Controllers } else { - if (!opt.Options.AutoLinkOptions.AllowManualLinking) + if (!opt.AutoLinkOptions.AllowManualLinking) { // If AllowManualLinking is disabled for this provider we cannot unlink return BadRequest(); diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index c905876f51..2a0bba8c1f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -414,7 +414,7 @@ namespace Umbraco.Web.BackOffice.Controllers } else { - autoLinkOptions = _externalLogins.Get(authType.Name)?.Options?.AutoLinkOptions; + autoLinkOptions = _externalLogins.Get(authType.Name)?.AutoLinkOptions; } // Sign in the user with this external login provider if the user already has a login @@ -445,7 +445,7 @@ namespace Umbraco.Web.BackOffice.Controllers ViewData.SetExternalSignInProviderErrors( new BackOfficeExternalLoginProviderErrors( loginInfo.LoginProvider, - new[] { "The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office." })); + new[] { "The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account" })); } //Remove the cookie otherwise this message will keep appearing @@ -462,7 +462,7 @@ namespace Umbraco.Web.BackOffice.Controllers if (autoLinkOptions.AutoLinkExternalAccount == false) { - return false; + return true; // TODO: This seems weird to return true, but it was like that before so must be a reason? } var email = loginInfo.Principal.FindFirstValue(ClaimTypes.Email); diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs index 470e62c2c2..2fa3f62cb8 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeServerVariables.cs @@ -135,7 +135,7 @@ namespace Umbraco.Web.BackOffice.Controllers /// Returns the server variables for authenticated users /// /// - internal Task> GetServerVariablesAsync() + internal async Task> GetServerVariablesAsync() { var globalSettings = _globalSettings; var backOfficeControllerName = ControllerExtensions.GetControllerName(); @@ -149,8 +149,8 @@ namespace Umbraco.Web.BackOffice.Controllers // having each url defined here explicitly - we can do that in v8! for now // for umbraco services we'll stick to explicitly defining the endpoints. - {"externalLoginsUrl", _linkGenerator.GetPathByAction(nameof(BackOfficeController.ExternalLogin), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, - {"externalLinkLoginsUrl", _linkGenerator.GetPathByAction(nameof(BackOfficeController.LinkLogin), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, + // {"externalLoginsUrl", _linkGenerator.GetPathByAction(nameof(BackOfficeController.ExternalLogin), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, + // {"externalLinkLoginsUrl", _linkGenerator.GetPathByAction(nameof(BackOfficeController.LinkLogin), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, {"gridConfig", _linkGenerator.GetPathByAction(nameof(BackOfficeController.GetGridConfig), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, // TODO: This is ultra confusing! this same key is used for different things, when returning the full app when authenticated it is this URL but when not auth'd it's actually the ServerVariables address {"serverVarsJs", _linkGenerator.GetPathByAction(nameof(BackOfficeController.Application), backOfficeControllerName, new { area = Constants.Web.Mvc.BackOfficeArea })}, @@ -418,13 +418,10 @@ namespace Umbraco.Web.BackOffice.Controllers "externalLogins", new Dictionary { { - // TODO: It would be nicer to not have to manually translate these properties - // but then needs to be changed in quite a few places in angular "providers", _externalLogins.GetBackOfficeProviders() .Select(p => new { - authType = p.AuthenticationType, - caption = p.Name, + authType = p.AuthenticationType, caption = p.Name, properties = p.Options }) .ToArray() @@ -444,7 +441,7 @@ namespace Umbraco.Web.BackOffice.Controllers } } }; - return Task.FromResult(defaultVals); + return defaultVals; } [DataContract] diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs index a3ce87e404..58ec79e51b 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviderOptions.cs @@ -4,6 +4,7 @@ using System.Runtime.Serialization; namespace Umbraco.Web.BackOffice.Security { + // TODO: This is only for the back office, does it need to be in common? /// /// Options used to configure back office external login providers @@ -11,13 +12,13 @@ namespace Umbraco.Web.BackOffice.Security public class BackOfficeExternalLoginProviderOptions { public BackOfficeExternalLoginProviderOptions( - string buttonStyle, string icon, string callbackPath, + string style, string icon, string callbackPath, ExternalSignInAutoLinkOptions autoLinkOptions = null, bool denyLocalLogin = false, bool autoRedirectLoginToExternalProvider = false, string customBackOfficeView = null) { - ButtonStyle = buttonStyle; + Style = style; Icon = icon; CallbackPath = callbackPath; AutoLinkOptions = autoLinkOptions ?? new ExternalSignInAutoLinkOptions(); @@ -26,10 +27,11 @@ namespace Umbraco.Web.BackOffice.Security CustomBackOfficeView = customBackOfficeView; } - public string ButtonStyle { get; } + public string Style { get; } public string Icon { get; } public string CallbackPath { get; } + /// /// Options used to control how users can be auto-linked/created/updated based on the external login provider /// diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 6f34a85c79..0cbcd85923 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -14,6 +14,7 @@ using Umbraco.Extensions; namespace Umbraco.Web.Common.Security { + // TODO: This is only for the back office, does it need to be in common? using Constants = Umbraco.Core.Constants; diff --git a/src/Umbraco.Web.BackOffice/Security/ExternalSignInAutoLinkOptions.cs b/src/Umbraco.Web.BackOffice/Security/ExternalSignInAutoLinkOptions.cs index 8d9f57945b..2098d90773 100644 --- a/src/Umbraco.Web.BackOffice/Security/ExternalSignInAutoLinkOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ExternalSignInAutoLinkOptions.cs @@ -7,6 +7,8 @@ using SecurityConstants = Umbraco.Core.Constants.Security; namespace Umbraco.Web.BackOffice.Security { + // TODO: This is only for the back office, does it need to be in common? + /// /// Options used to configure auto-linking external OAuth providers /// diff --git a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs index 6d0b64e84f..b631227470 100644 --- a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs @@ -1,109 +1,27 @@ -using Microsoft.AspNetCore.Authentication; -using Microsoft.AspNetCore.Authentication.OAuth; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.DependencyInjection.Extensions; -using Microsoft.Extensions.Options; +using Microsoft.AspNetCore.Authentication.OAuth; using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Text; -using Umbraco.Core; -using Umbraco.Core.Builder; + namespace Umbraco.Web.BackOffice.Security { - /// - /// Custom used to associate external logins with umbraco external login options - /// - public class BackOfficeAuthenticationBuilder : AuthenticationBuilder - { - private readonly BackOfficeExternalLoginProviderOptions _loginProviderOptions; - - public BackOfficeAuthenticationBuilder(IServiceCollection services, BackOfficeExternalLoginProviderOptions loginProviderOptions) - : base(services) - { - _loginProviderOptions = loginProviderOptions; - } - - /// - /// Overridden to track the final authenticationScheme being registered for the external login - /// - /// - /// - /// - /// - /// - /// - public override AuthenticationBuilder AddRemoteScheme(string authenticationScheme, string displayName, Action configureOptions) - { - //Ensure the prefix is set - if (!authenticationScheme.StartsWith(Constants.Security.BackOfficeExternalAuthenticationTypePrefix)) - { - authenticationScheme = Constants.Security.BackOfficeExternalAuthenticationTypePrefix + authenticationScheme; - } - - // add our login provider to the container along with a custom options configuration - Services.AddSingleton(x => new BackOfficeExternalLoginProvider(displayName, authenticationScheme, _loginProviderOptions)); - Services.TryAddEnumerable(ServiceDescriptor.Singleton, EnsureBackOfficeScheme>()); - - return base.AddRemoteScheme(authenticationScheme, displayName, configureOptions); - } - - // TODO: We could override and throw NotImplementedException for other methods? - - // Ensures that the sign in scheme is always the Umbraco back office external type - private class EnsureBackOfficeScheme : IPostConfigureOptions where TOptions : RemoteAuthenticationOptions - { - public void PostConfigure(string name, TOptions options) - { - options.SignInScheme = Constants.Security.BackOfficeExternalAuthenticationType; - } - } - } - - /// - /// Used to add back office login providers - /// - public class BackOfficeExternalLoginsBuilder - { - public BackOfficeExternalLoginsBuilder(IServiceCollection services) - { - _services = services; - } - - private readonly IServiceCollection _services; - - /// - /// Add a back office login provider with options - /// - /// - /// - /// - public BackOfficeExternalLoginsBuilder AddBackOfficeLogin( - BackOfficeExternalLoginProviderOptions loginProviderOptions, - Action build) - { - build(new BackOfficeAuthenticationBuilder(_services, loginProviderOptions)); - return this; - } - } - - public static class AuthenticationBuilderExtensions - { - public static IUmbracoBuilder AddBackOfficeExternalLogins(this IUmbracoBuilder umbracoBuilder, Action builder) - { - builder(new BackOfficeExternalLoginsBuilder(umbracoBuilder.Services)); - return umbracoBuilder; - } - } + // TODO: This is only for the back office, does it need to be in common? // TODO: We need to implement this and extend it to support the back office external login options // basically migrate things from AuthenticationManagerExtensions & AuthenticationOptionsExtensions // and use this to get the back office external login infos public interface IBackOfficeExternalLoginProviders { - BackOfficeExternalLoginProvider Get(string authenticationType); + /// + /// Register a login provider for the back office + /// + /// + void Register(BackOfficeExternalLoginProvider provider); + + BackOfficeExternalLoginProviderOptions Get(string authenticationType); IEnumerable GetBackOfficeProviders(); @@ -114,41 +32,42 @@ namespace Umbraco.Web.BackOffice.Security /// string GetAutoLoginProvider(); - /// - /// Returns true if there is any external provider that has the Deny Local Login option configured - /// - /// bool HasDenyLocalLogin(); } + // TODO: This class is just a placeholder for later public class BackOfficeExternalLoginProviders : IBackOfficeExternalLoginProviders { - public BackOfficeExternalLoginProviders(IEnumerable externalLogins) + private ConcurrentDictionary _providers = new ConcurrentDictionary(); + + public void Register(BackOfficeExternalLoginProvider provider) { - _externalLogins = externalLogins; + _providers.TryAdd(provider.AuthenticationType, provider); + + // TODO: we need to be able to set things like we were doing in ForUmbracoBackOffice. + // Ok, most is done but we'll also need to take into account the callback path to ignore when we + // do front-end routing } - private readonly IEnumerable _externalLogins; - - public BackOfficeExternalLoginProvider Get(string authenticationType) + public BackOfficeExternalLoginProviderOptions Get(string authenticationType) { - return _externalLogins.FirstOrDefault(x => x.AuthenticationType == authenticationType); + return _providers.TryGetValue(authenticationType, out var opt) ? opt.Options : null; } public string GetAutoLoginProvider() { - var found = _externalLogins.Where(x => x.Options.AutoRedirectLoginToExternalProvider).ToList(); - return found.Count > 0 ? found[0].AuthenticationType : null; + var found = _providers.Where(x => x.Value.Options.AutoRedirectLoginToExternalProvider).ToList(); + return found.Count > 0 ? found[0].Key : null; } public IEnumerable GetBackOfficeProviders() { - return _externalLogins; + return _providers.Values; } public bool HasDenyLocalLogin() { - var found = _externalLogins.Where(x => x.Options.DenyLocalLogin).ToList(); + var found = _providers.Where(x => x.Value.Options.DenyLocalLogin).ToList(); return found.Count > 0; } } diff --git a/src/Umbraco.Web.UI.Client/src/common/services/externallogininfo.service.js b/src/Umbraco.Web.UI.Client/src/common/services/externallogininfo.service.js index f1dbb0f651..31914f4e58 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/externallogininfo.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/externallogininfo.service.js @@ -50,7 +50,7 @@ function externalLoginInfoService(externalLoginInfo, umbRequestHelper) { return true; } else { - return x.properties.AutoLinkOptions.AllowManualLinking; + return x.properties.ExternalSignInAutoLinkOptions.AllowManualLinking; } }); return providers; diff --git a/src/Umbraco.Web.UI.Client/src/views/common/overlays/user/user.html b/src/Umbraco.Web.UI.Client/src/views/common/overlays/user/user.html index 330a57ab7d..fdd2671200 100644 --- a/src/Umbraco.Web.UI.Client/src/views/common/overlays/user/user.html +++ b/src/Umbraco.Web.UI.Client/src/views/common/overlays/user/user.html @@ -52,11 +52,11 @@
-
+