From a851a1fb40142d3466f143610cfe5f3619830f5d Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Fri, 12 Apr 2024 12:29:10 +0200 Subject: [PATCH 1/5] Added mostly working linking methods to the backoffice controller Cleanup still required --- .../Security/BackOfficeController.cs | 160 +++++++++++++++++- 1 file changed, 158 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs index e65eafb457e4..3425c9f17bfc 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs @@ -1,8 +1,11 @@ -using System.Security.Claims; +using System.ComponentModel.DataAnnotations; +using System.Runtime.Serialization; +using System.Security.Claims; using Asp.Versioning; using Microsoft.AspNetCore; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Cors; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; @@ -39,6 +42,7 @@ public class BackOfficeController : SecurityControllerBase private readonly ILogger _logger; private readonly IBackOfficeTwoFactorOptions _backOfficeTwoFactorOptions; private readonly IUserTwoFactorLoginService _userTwoFactorLoginService; + private readonly IBackOfficeExternalLoginProviders _backOfficeExternalLoginProviders; public BackOfficeController( IHttpContextAccessor httpContextAccessor, @@ -47,7 +51,8 @@ public BackOfficeController( IOptions securitySettings, ILogger logger, IBackOfficeTwoFactorOptions backOfficeTwoFactorOptions, - IUserTwoFactorLoginService userTwoFactorLoginService) + IUserTwoFactorLoginService userTwoFactorLoginService, + IBackOfficeExternalLoginProviders backOfficeExternalLoginProviders) { _httpContextAccessor = httpContextAccessor; _backOfficeSignInManager = backOfficeSignInManager; @@ -56,6 +61,7 @@ public BackOfficeController( _logger = logger; _backOfficeTwoFactorOptions = backOfficeTwoFactorOptions; _userTwoFactorLoginService = userTwoFactorLoginService; + _backOfficeExternalLoginProviders = backOfficeExternalLoginProviders; } [HttpPost("login")] @@ -184,6 +190,156 @@ public async Task Signout(CancellationToken cancellationToken) return SignOut(Constants.Security.BackOfficeAuthenticationType, OpenIddictServerAspNetCoreDefaults.AuthenticationScheme); } + /// + /// Called when a user links an external login provider in the back office + /// + /// + /// + [HttpPost("link-login")] + [MapToApiVersion("1.0")] + public IActionResult LinkLogin(string provider) + { + // Request a redirect to the external login provider to link a login for the current user + var redirectUrl = Url.Action(nameof(ExternalLinkLoginCallback), this.GetControllerName()); + + // Configures the redirect URL and user identifier for the specified external login including xsrf data + AuthenticationProperties properties = + _backOfficeSignInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl, _backOfficeUserManager.GetUserId(User)); + + return Challenge(properties, provider); + } + + /// + /// Callback path when the user initiates a link login request from the back office to the external provider from the + /// action + /// + /// + /// An example of this is here + /// https://github.com/dotnet/aspnetcore/blob/main/src/Identity/samples/IdentitySample.Mvc/Controllers/AccountController.cs#L155 + /// which this is based on + /// + [HttpGet("ExternalLinkLoginCallback")] + [AllowAnonymous] + [MapToApiVersion("1.0")] + public async Task ExternalLinkLoginCallback() + { + var cookieAuthenticatedUserAttempt = + await HttpContext.AuthenticateAsync(Constants.Security.BackOfficeAuthenticationType); + + if (cookieAuthenticatedUserAttempt.Succeeded == false) + { + return Unauthorized(); + } + + BackOfficeIdentityUser? user = await _backOfficeUserManager.GetUserAsync(cookieAuthenticatedUserAttempt.Principal); + if (user == null) + { + // ... this should really not happen + // TempData[ViewDataExtensions.TokenExternalSignInError] = new[] { "Local user does not exist" }; + // return RedirectToLogin(new { flow = "external-login", status = "localUserNotFound", logout = "true"}); + // todo verify + return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder() + .WithTitle("No user found") + .Build()); + } + + ExternalLoginInfo? info = + await _backOfficeSignInManager.GetExternalLoginInfoAsync(); + // await _backOfficeUserManager.GetExternalLoginInfoAsync(await _backOfficeUserManager.GetUserIdAsync(user)); //todo verify + + if (info == null) + { + // Add error and redirect for it to be displayed + // TempData[ViewDataExtensions.TokenExternalSignInError] = + // new[] { "An error occurred, could not get external login info" }; + // return RedirectToLogin(new { flow = "external-login", status = "externalLoginInfoNotFound", logout = "true"}); + return StatusCode(StatusCodes.Status404NotFound, new ProblemDetailsBuilder() + .WithTitle("External login info not found") + .Build()); + } + + IdentityResult addLoginResult = await _backOfficeUserManager.AddLoginAsync(user, info); + if (addLoginResult.Succeeded) + { + // Update any authentication tokens if succeeded + // todo? + // await _backOfficeUserManager.UpdateExternalAuthenticationTokensAsync(info); + + // return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); + return Redirect("/umbraco"); // todo shouldn't this come from configuration + } + + // Add errors and redirect for it to be displayed + // TempData[ViewDataExtensions.TokenExternalSignInError] = addLoginResult.Errors; + // return RedirectToLogin(new { flow = "external-login", status = "failed", logout = "true" }); + // todo + return BadRequest(); + } + + // todo move this and cleanup namespaces + public class UnLinkLoginModel + { + [Required] + [DataMember(Name = "loginProvider", IsRequired = true)] + public required string LoginProvider { get; set; } + + [Required] + [DataMember(Name = "providerKey", IsRequired = true)] + public required string ProviderKey { get; set; } + } + + // todo cleanup unhappy responses + [HttpPost("unlink-login")] + public async Task PostUnLinkLogin(UnLinkLoginModel unlinkLoginModel) + { + var userId = User.Identity?.GetUserId(); + if (userId is null) + { + throw new InvalidOperationException("Could not find userId"); + } + var user = await _backOfficeUserManager.FindByIdAsync(userId); + if (user == null) throw new InvalidOperationException("Could not find user"); + + AuthenticationScheme? authType = (await _backOfficeSignInManager.GetExternalAuthenticationSchemesAsync()) + .FirstOrDefault(x => x.Name == unlinkLoginModel.LoginProvider); + + if (authType == null) + { + _logger.LogWarning("Could not find external authentication provider registered: {LoginProvider}", unlinkLoginModel.LoginProvider); + } + else + { + BackOfficeExternaLoginProviderScheme? opt = await _backOfficeExternalLoginProviders.GetAsync(authType.Name); + if (opt == null) + { + return BadRequest( + $"Could not find external authentication options registered for provider {unlinkLoginModel.LoginProvider}"); + } + + if (!opt.ExternalLoginProvider.Options.AutoLinkOptions.AllowManualLinking) + { + // If AllowManualLinking is disabled for this provider we cannot unlink + return BadRequest(); + } + } + + IdentityResult result = await _backOfficeUserManager.RemoveLoginAsync( + user, + unlinkLoginModel.LoginProvider, + unlinkLoginModel.ProviderKey); + + if (result.Succeeded) + { + await _backOfficeSignInManager.SignInAsync(user, true); + return Ok(); + } + + // AddModelErrors(result); + // return new ValidationErrorResult(ModelState); + // todo + return BadRequest(); + } + /// /// Retrieve the user principal stored in the authentication cookie. /// From f4befcd12455de85a63c8b30ce33b165ffa12590 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 15 Apr 2024 13:25:32 +0200 Subject: [PATCH 2/5] Added proposed default error handling extionsion methods --- .../Routing/BackOfficeAreaRoutes.cs | 2 +- .../Configuration/Models/SecuritySettings.cs | 3 + .../Extensions/OAuthOptionsExtensions.cs | 73 +++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs diff --git a/src/Umbraco.Cms.Api.Management/Routing/BackOfficeAreaRoutes.cs b/src/Umbraco.Cms.Api.Management/Routing/BackOfficeAreaRoutes.cs index a5e93c6f67a0..e51da4246a70 100644 --- a/src/Umbraco.Cms.Api.Management/Routing/BackOfficeAreaRoutes.cs +++ b/src/Umbraco.Cms.Api.Management/Routing/BackOfficeAreaRoutes.cs @@ -82,6 +82,6 @@ private void MapMinimalBackOffice(IEndpointRouteBuilder endpoints) Controller = ControllerExtensions.GetControllerName(), Action = nameof(BackOfficeDefaultController.Index), }, - constraints: new { slug = @"^(section.*|upgrade|install|logout)$" }); + constraints: new { slug = @"^(section.*|upgrade|install|logout|error)$" }); } } diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index f7bb1fe2f46d..13208b26ac5a 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -26,6 +26,7 @@ public class SecuritySettings internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const string StaticAuthorizeCallbackPathName = "/umbraco"; + internal const string StaticAuthorizeCallbackErrorPathName = StaticAuthorizeCallbackPathName + "/error"; /// /// Gets or sets a value indicating whether to keep the user logged in. @@ -116,4 +117,6 @@ public class SecuritySettings /// [DefaultValue(StaticAuthorizeCallbackPathName)] public string AuthorizeCallbackPathName { get; set; } = StaticAuthorizeCallbackPathName; + + public string AuthorizeCallbackErrorPathName { get; set; } = StaticAuthorizeCallbackErrorPathName; } diff --git a/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs b/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs new file mode 100644 index 000000000000..b25ece8fa96c --- /dev/null +++ b/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs @@ -0,0 +1,73 @@ +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.OAuth; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.DependencyInjection; + +namespace Umbraco.Cms.Web.Common.Extensions; + +public static class OAuthOptionsExtensions +{ + /// + /// Applies SetUmbracoRedirectWithFilteredParams to both OnAccessDenied and OnRemoteFailure + /// on the OAuthOptions so Umbraco can do its best to nicely display the error messages + /// that are passed back from the external login provider on failure. + /// + /// + /// + /// + public static T SetDefaultErrorEventHandling(this T oAuthOptions) where T : OAuthOptions + { + oAuthOptions.Events.OnAccessDenied = HandleResponseWithDefaultUmbracoRedirect; + oAuthOptions.Events.OnRemoteFailure = HandleResponseWithDefaultUmbracoRedirect; + + return oAuthOptions; + } + + private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContext context) + { + context.SetUmbracoRedirectWithFilteredParams() + .HandleResponse(); + + return Task.FromResult(0); + } + + /// + /// Sets the context to redirect to the path with all parameters, except state, that are passed to the initial server callback configured for the configured external login provider + /// + /// + public static T SetUmbracoRedirectWithFilteredParams(this T context) + where T : HandleRequestContext + { + var callbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value + .AuthorizeCallbackErrorPathName; + context.Response.Redirect($"{callbackPath}?" + string.Join( + '&', + context.Request.Query.Where(q => + q.Key.Equals("state", StringComparison.InvariantCultureIgnoreCase) == false) + .Select(q => q.Key + "=" + q.Value))); + + return context; + } + + /// + /// Sets the callbackPath for the RemoteAuthenticationOptions based on the configured Umbraco path and the path supplied. + /// By default this will result in "/umbraco/your-supplied-path". + /// + /// The options object to set the path on. + /// The path that should go after the umbraco path, will add a leading slash if it's missing. + /// + public static RemoteAuthenticationOptions SetUmbracoBasedCallbackPath(this RemoteAuthenticationOptions options, string path) + { + var umbracoDallbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value + .AuthorizeCallbackPathName; + if (path.StartsWith('/') == false) + { + path = "/" + path; + } + + options.CallbackPath = umbracoDallbackPath + path; + return options; + } +} From 1947ad9247d70c51d881c63cd4a36fe40a169c97 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 16 Apr 2024 13:32:34 +0200 Subject: [PATCH 3/5] Cleanup, clarification and PR feedback --- .../Security/BackOfficeController.cs | 88 ++++++++----------- .../Security/UnLinkLoginRequestModel.cs | 15 ++++ .../Configuration/Models/SecuritySettings.cs | 3 +- .../Extensions/OAuthOptionsExtensions.cs | 49 ++++++----- 4 files changed, 82 insertions(+), 73 deletions(-) create mode 100644 src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs index 3425c9f17bfc..a4f466e58c4d 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs @@ -1,11 +1,8 @@ -using System.ComponentModel.DataAnnotations; -using System.Runtime.Serialization; -using System.Security.Claims; +using System.Security.Claims; using Asp.Versioning; using Microsoft.AspNetCore; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Cors; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; @@ -228,44 +225,34 @@ public async Task ExternalLinkLoginCallback() if (cookieAuthenticatedUserAttempt.Succeeded == false) { - return Unauthorized(); + return Redirect(_securitySettings.Value.AuthorizeCallbackErrorPathName.AppendQueryStringToUrl( + "flow=external-login-callback", + "status=unauthorized")); } BackOfficeIdentityUser? user = await _backOfficeUserManager.GetUserAsync(cookieAuthenticatedUserAttempt.Principal); if (user == null) { - // ... this should really not happen - // TempData[ViewDataExtensions.TokenExternalSignInError] = new[] { "Local user does not exist" }; - // return RedirectToLogin(new { flow = "external-login", status = "localUserNotFound", logout = "true"}); - // todo verify - return StatusCode(StatusCodes.Status401Unauthorized, new ProblemDetailsBuilder() - .WithTitle("No user found") - .Build()); + return Redirect(_securitySettings.Value.AuthorizeCallbackErrorPathName.AppendQueryStringToUrl( + "flow=external-login-callback", + "status=user-not-found")); } ExternalLoginInfo? info = await _backOfficeSignInManager.GetExternalLoginInfoAsync(); - // await _backOfficeUserManager.GetExternalLoginInfoAsync(await _backOfficeUserManager.GetUserIdAsync(user)); //todo verify if (info == null) { - // Add error and redirect for it to be displayed - // TempData[ViewDataExtensions.TokenExternalSignInError] = - // new[] { "An error occurred, could not get external login info" }; - // return RedirectToLogin(new { flow = "external-login", status = "externalLoginInfoNotFound", logout = "true"}); - return StatusCode(StatusCodes.Status404NotFound, new ProblemDetailsBuilder() - .WithTitle("External login info not found") - .Build()); + return Redirect(_securitySettings.Value.AuthorizeCallbackErrorPathName.AppendQueryStringToUrl( + "flow=external-login-callback", + "status=external-info-not-found")); } IdentityResult addLoginResult = await _backOfficeUserManager.AddLoginAsync(user, info); if (addLoginResult.Succeeded) { // Update any authentication tokens if succeeded - // todo? - // await _backOfficeUserManager.UpdateExternalAuthenticationTokensAsync(info); - - // return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); + await _backOfficeSignInManager.UpdateExternalAuthenticationTokensAsync(info); return Redirect("/umbraco"); // todo shouldn't this come from configuration } @@ -273,60 +260,60 @@ public async Task ExternalLinkLoginCallback() // TempData[ViewDataExtensions.TokenExternalSignInError] = addLoginResult.Errors; // return RedirectToLogin(new { flow = "external-login", status = "failed", logout = "true" }); // todo - return BadRequest(); - } - - // todo move this and cleanup namespaces - public class UnLinkLoginModel - { - [Required] - [DataMember(Name = "loginProvider", IsRequired = true)] - public required string LoginProvider { get; set; } - - [Required] - [DataMember(Name = "providerKey", IsRequired = true)] - public required string ProviderKey { get; set; } + return Redirect(_securitySettings.Value.AuthorizeCallbackErrorPathName.AppendQueryStringToUrl( + "flow=external-login-callback", + "status=failed")); } // todo cleanup unhappy responses [HttpPost("unlink-login")] - public async Task PostUnLinkLogin(UnLinkLoginModel unlinkLoginModel) + [MapToApiVersion("1.0")] + public async Task PostUnLinkLogin(UnLinkLoginRequestModel unlinkLoginRequestModel) { var userId = User.Identity?.GetUserId(); if (userId is null) { throw new InvalidOperationException("Could not find userId"); } - var user = await _backOfficeUserManager.FindByIdAsync(userId); - if (user == null) throw new InvalidOperationException("Could not find user"); + + BackOfficeIdentityUser? user = await _backOfficeUserManager.FindByIdAsync(userId); + if (user == null) + { + throw new InvalidOperationException("Could not find user"); + } AuthenticationScheme? authType = (await _backOfficeSignInManager.GetExternalAuthenticationSchemesAsync()) - .FirstOrDefault(x => x.Name == unlinkLoginModel.LoginProvider); + .FirstOrDefault(x => x.Name == unlinkLoginRequestModel.LoginProvider); if (authType == null) { - _logger.LogWarning("Could not find external authentication provider registered: {LoginProvider}", unlinkLoginModel.LoginProvider); + _logger.LogWarning("Could not find the supplied external authentication provider"); } else { BackOfficeExternaLoginProviderScheme? opt = await _backOfficeExternalLoginProviders.GetAsync(authType.Name); if (opt == null) { - return BadRequest( - $"Could not find external authentication options registered for provider {unlinkLoginModel.LoginProvider}"); + return StatusCode(StatusCodes.Status400BadRequest, new ProblemDetailsBuilder() + .WithTitle("Missing Authentication options") + .WithDetail($"Could not find external authentication options registered for provider {authType.Name}") + .Build()); } if (!opt.ExternalLoginProvider.Options.AutoLinkOptions.AllowManualLinking) { // If AllowManualLinking is disabled for this provider we cannot unlink - return BadRequest(); + return StatusCode(StatusCodes.Status400BadRequest, new ProblemDetailsBuilder() + .WithTitle("Unlinking disabled") + .WithDetail($"Manual linking is disabled for provider {authType.Name}") + .Build()); } } IdentityResult result = await _backOfficeUserManager.RemoveLoginAsync( user, - unlinkLoginModel.LoginProvider, - unlinkLoginModel.ProviderKey); + unlinkLoginRequestModel.LoginProvider, + unlinkLoginRequestModel.ProviderKey); if (result.Succeeded) { @@ -334,10 +321,9 @@ public async Task PostUnLinkLogin(UnLinkLoginModel unlinkLoginMod return Ok(); } - // AddModelErrors(result); - // return new ValidationErrorResult(ModelState); - // todo - return BadRequest(); + return StatusCode(StatusCodes.Status400BadRequest, new ProblemDetailsBuilder() + .WithTitle("Unlinking failed") + .Build()); } /// diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs new file mode 100644 index 000000000000..a82c9078ad99 --- /dev/null +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs @@ -0,0 +1,15 @@ +using System.ComponentModel.DataAnnotations; +using System.Runtime.Serialization; + +namespace Umbraco.Cms.Api.Management.ViewModels.Security; + +public class UnLinkLoginRequestModel +{ + [Required] + [DataMember(Name = "loginProvider", IsRequired = true)] + public required string LoginProvider { get; set; } + + [Required] + [DataMember(Name = "providerKey", IsRequired = true)] + public required string ProviderKey { get; set; } +} diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index 13208b26ac5a..80a9b38d4f0e 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -26,7 +26,7 @@ public class SecuritySettings internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const string StaticAuthorizeCallbackPathName = "/umbraco"; - internal const string StaticAuthorizeCallbackErrorPathName = StaticAuthorizeCallbackPathName + "/error"; + internal const string StaticAuthorizeCallbackErrorPathName = "/umbraco/error"; /// /// Gets or sets a value indicating whether to keep the user logged in. @@ -118,5 +118,6 @@ public class SecuritySettings [DefaultValue(StaticAuthorizeCallbackPathName)] public string AuthorizeCallbackPathName { get; set; } = StaticAuthorizeCallbackPathName; + [DefaultValue(StaticAuthorizeCallbackErrorPathName)] public string AuthorizeCallbackErrorPathName { get; set; } = StaticAuthorizeCallbackErrorPathName; } diff --git a/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs b/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs index b25ece8fa96c..6cb8e8dd8fe9 100644 --- a/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs @@ -4,30 +4,34 @@ using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.DependencyInjection; +using Umbraco.Extensions; namespace Umbraco.Cms.Web.Common.Extensions; public static class OAuthOptionsExtensions { + // https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + // we omit "state" and "error_uri" here as it hold no value in determining the message to display to the user + private static IReadOnlyCollection oathCallbackErrorParams = new string[] { "error", "error_description" }; + /// /// Applies SetUmbracoRedirectWithFilteredParams to both OnAccessDenied and OnRemoteFailure /// on the OAuthOptions so Umbraco can do its best to nicely display the error messages /// that are passed back from the external login provider on failure. /// - /// - /// - /// - public static T SetDefaultErrorEventHandling(this T oAuthOptions) where T : OAuthOptions + public static T SetDefaultErrorEventHandling(this T oAuthOptions, string providerFriendlyName) where T : OAuthOptions { - oAuthOptions.Events.OnAccessDenied = HandleResponseWithDefaultUmbracoRedirect; - oAuthOptions.Events.OnRemoteFailure = HandleResponseWithDefaultUmbracoRedirect; + oAuthOptions.Events.OnAccessDenied = + context => HandleResponseWithDefaultUmbracoRedirect(context, providerFriendlyName, "OnAccessDenied"); + oAuthOptions.Events.OnRemoteFailure = + context => HandleResponseWithDefaultUmbracoRedirect(context, providerFriendlyName, "OnRemoteFailure"); return oAuthOptions; } - private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContext context) + private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContext context, string providerFriendlyName, string eventName) { - context.SetUmbracoRedirectWithFilteredParams() + context.SetUmbracoRedirectWithFilteredParams(providerFriendlyName,eventName) .HandleResponse(); return Task.FromResult(0); @@ -36,18 +40,25 @@ private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContex /// /// Sets the context to redirect to the path with all parameters, except state, that are passed to the initial server callback configured for the configured external login provider /// - /// - public static T SetUmbracoRedirectWithFilteredParams(this T context) + public static T SetUmbracoRedirectWithFilteredParams(this T context, string providerFriendlyName, string eventName) where T : HandleRequestContext { var callbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value .AuthorizeCallbackErrorPathName; - context.Response.Redirect($"{callbackPath}?" + string.Join( - '&', - context.Request.Query.Where(q => - q.Key.Equals("state", StringComparison.InvariantCultureIgnoreCase) == false) - .Select(q => q.Key + "=" + q.Value))); + callbackPath = callbackPath.AppendQueryStringToUrl("flow=external-login") + .AppendQueryStringToUrl($"provider={providerFriendlyName}") + .AppendQueryStringToUrl($"callback-event={eventName}"); + + foreach (var oathCallbackErrorParam in oathCallbackErrorParams) + { + if (context.Request.Query.ContainsKey(oathCallbackErrorParam)) + { + callbackPath = callbackPath.AppendQueryStringToUrl($"{oathCallbackErrorParam}={context.Request.Query[oathCallbackErrorParam]}"); + } + } + + context.Response.Redirect(callbackPath); return context; } @@ -60,14 +71,10 @@ public static T SetUmbracoRedirectWithFilteredParams(this T context) /// public static RemoteAuthenticationOptions SetUmbracoBasedCallbackPath(this RemoteAuthenticationOptions options, string path) { - var umbracoDallbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value + var umbracoCallbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value .AuthorizeCallbackPathName; - if (path.StartsWith('/') == false) - { - path = "/" + path; - } - options.CallbackPath = umbracoDallbackPath + path; + options.CallbackPath = umbracoCallbackPath + path.EnsureStartsWith("/"); return options; } } From 97846a28d171929762c5e489a4e0b0db1fef3316 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 16 Apr 2024 14:58:45 +0200 Subject: [PATCH 4/5] More cleanup --- .../ViewModels/Security/UnLinkLoginRequestModel.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs index a82c9078ad99..50d34810b417 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Security/UnLinkLoginRequestModel.cs @@ -6,10 +6,8 @@ namespace Umbraco.Cms.Api.Management.ViewModels.Security; public class UnLinkLoginRequestModel { [Required] - [DataMember(Name = "loginProvider", IsRequired = true)] public required string LoginProvider { get; set; } [Required] - [DataMember(Name = "providerKey", IsRequired = true)] public required string ProviderKey { get; set; } } From e4835fe500cbbf836758743e1ad857c7819ad95b Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 16 Apr 2024 15:54:53 +0200 Subject: [PATCH 5/5] Transformed the OAuthOptionsExtensions into a helper class this allows for proper DI for the dependencies --- .../UmbracoBuilder.BackOffice.cs | 1 + .../BackOfficeAuthenticationBuilder.cs | 2 +- .../UmbracoBuilderExtensions.cs | 8 +++++ .../OAuthOptionsHelper.cs} | 33 ++++++++++--------- 4 files changed, 28 insertions(+), 16 deletions(-) rename src/Umbraco.Web.Common/{Extensions/OAuthOptionsExtensions.cs => Helpers/OAuthOptionsHelper.cs} (66%) diff --git a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOffice.cs b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOffice.cs index 4cc22890cd6b..907e3057af6c 100644 --- a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOffice.cs +++ b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOffice.cs @@ -24,6 +24,7 @@ public static IUmbracoBuilder .AddConfiguration() .AddUmbracoCore() .AddWebComponents() + .AddHelpers() .AddBackOfficeCore() .AddBackOfficeIdentity() .AddBackOfficeAuthentication() diff --git a/src/Umbraco.Cms.Api.Management/Security/BackOfficeAuthenticationBuilder.cs b/src/Umbraco.Cms.Api.Management/Security/BackOfficeAuthenticationBuilder.cs index f688d138bd25..98489a849c8b 100644 --- a/src/Umbraco.Cms.Api.Management/Security/BackOfficeAuthenticationBuilder.cs +++ b/src/Umbraco.Cms.Api.Management/Security/BackOfficeAuthenticationBuilder.cs @@ -20,7 +20,7 @@ public BackOfficeAuthenticationBuilder( : base(services) => _loginProviderOptions = loginProviderOptions ?? (x => { }); - public string? SchemeForBackOffice(string scheme) + public static string? SchemeForBackOffice(string scheme) => scheme?.EnsureStartsWith(Constants.Security.BackOfficeExternalAuthenticationTypePrefix); /// diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs index 480ae903978e..685adf2fbdea 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs @@ -46,6 +46,7 @@ using Umbraco.Cms.Web.Common.Configuration; using Umbraco.Cms.Web.Common.DependencyInjection; using Umbraco.Cms.Web.Common.FileProviders; +using Umbraco.Cms.Web.Common.Helpers; using Umbraco.Cms.Web.Common.Localization; using Umbraco.Cms.Web.Common.Middleware; using Umbraco.Cms.Web.Common.ModelBinders; @@ -306,6 +307,13 @@ public static IUmbracoBuilder AddWebComponents(this IUmbracoBuilder builder) return builder; } + public static IUmbracoBuilder AddHelpers(this IUmbracoBuilder builder) + { + builder.Services.AddSingleton(); + + return builder; + } + // TODO: Does this need to exist and/or be public? public static IUmbracoBuilder AddWebServer(this IUmbracoBuilder builder) { diff --git a/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs b/src/Umbraco.Web.Common/Helpers/OAuthOptionsHelper.cs similarity index 66% rename from src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs rename to src/Umbraco.Web.Common/Helpers/OAuthOptionsHelper.cs index 6cb8e8dd8fe9..4179819f73dd 100644 --- a/src/Umbraco.Web.Common/Extensions/OAuthOptionsExtensions.cs +++ b/src/Umbraco.Web.Common/Helpers/OAuthOptionsHelper.cs @@ -1,25 +1,30 @@ using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.OAuth; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; -using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Extensions; -namespace Umbraco.Cms.Web.Common.Extensions; +namespace Umbraco.Cms.Web.Common.Helpers; -public static class OAuthOptionsExtensions +public class OAuthOptionsHelper { // https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 // we omit "state" and "error_uri" here as it hold no value in determining the message to display to the user - private static IReadOnlyCollection oathCallbackErrorParams = new string[] { "error", "error_description" }; + private static readonly IReadOnlyCollection _oathCallbackErrorParams = new string[] { "error", "error_description" }; + + private readonly IOptions _securitySettings; + + public OAuthOptionsHelper(IOptions securitySettings) + { + _securitySettings = securitySettings; + } /// /// Applies SetUmbracoRedirectWithFilteredParams to both OnAccessDenied and OnRemoteFailure /// on the OAuthOptions so Umbraco can do its best to nicely display the error messages /// that are passed back from the external login provider on failure. /// - public static T SetDefaultErrorEventHandling(this T oAuthOptions, string providerFriendlyName) where T : OAuthOptions + public T SetDefaultErrorEventHandling(T oAuthOptions, string providerFriendlyName) where T : OAuthOptions { oAuthOptions.Events.OnAccessDenied = context => HandleResponseWithDefaultUmbracoRedirect(context, providerFriendlyName, "OnAccessDenied"); @@ -29,9 +34,9 @@ public static T SetDefaultErrorEventHandling(this T oAuthOptions, string prov return oAuthOptions; } - private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContext context, string providerFriendlyName, string eventName) + private Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContext context, string providerFriendlyName, string eventName) { - context.SetUmbracoRedirectWithFilteredParams(providerFriendlyName,eventName) + SetUmbracoRedirectWithFilteredParams(context, providerFriendlyName, eventName) .HandleResponse(); return Task.FromResult(0); @@ -40,17 +45,16 @@ private static Task HandleResponseWithDefaultUmbracoRedirect(HandleRequestContex /// /// Sets the context to redirect to the path with all parameters, except state, that are passed to the initial server callback configured for the configured external login provider /// - public static T SetUmbracoRedirectWithFilteredParams(this T context, string providerFriendlyName, string eventName) + public T SetUmbracoRedirectWithFilteredParams(T context, string providerFriendlyName, string eventName) where T : HandleRequestContext { - var callbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value - .AuthorizeCallbackErrorPathName; + var callbackPath = _securitySettings.Value.AuthorizeCallbackErrorPathName; callbackPath = callbackPath.AppendQueryStringToUrl("flow=external-login") .AppendQueryStringToUrl($"provider={providerFriendlyName}") .AppendQueryStringToUrl($"callback-event={eventName}"); - foreach (var oathCallbackErrorParam in oathCallbackErrorParams) + foreach (var oathCallbackErrorParam in _oathCallbackErrorParams) { if (context.Request.Query.ContainsKey(oathCallbackErrorParam)) { @@ -69,10 +73,9 @@ public static T SetUmbracoRedirectWithFilteredParams(this T context, string p /// The options object to set the path on. /// The path that should go after the umbraco path, will add a leading slash if it's missing. /// - public static RemoteAuthenticationOptions SetUmbracoBasedCallbackPath(this RemoteAuthenticationOptions options, string path) + public RemoteAuthenticationOptions SetUmbracoBasedCallbackPath(RemoteAuthenticationOptions options, string path) { - var umbracoCallbackPath = StaticServiceProvider.Instance.GetRequiredService>().Value - .AuthorizeCallbackPathName; + var umbracoCallbackPath = _securitySettings.Value.AuthorizeCallbackPathName; options.CallbackPath = umbracoCallbackPath + path.EnsureStartsWith("/"); return options;