From 11e5257b56c92b4c74ae701719619d6dbe593642 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 17 May 2024 16:06:26 +0200 Subject: [PATCH] V14: Untangle the preview functionality from the auth cookie (#16308) * AB40660 - untangle the preview cookie from the auth cookie * Clean up * Allow anonymous to end preview sessions * Some refinements * update OpenApi.json * Fix enter preview test * correct tests to match new expectations of the preview cookie * sync preview tests with correct expectations of access level --------- Co-authored-by: Sven Geusens Co-authored-by: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> --- .../Preview/EndPreviewController.cs | 6 +- .../Preview/EnterPreviewController.cs | 21 +++++-- .../UmbracoBuilder.BackOfficeIdentity.cs | 1 + src/Umbraco.Cms.Api.Management/OpenApi.json | 10 +-- .../DependencyInjection/UmbracoBuilder.cs | 2 + .../Preview/IPreviewTokenGenerator.cs | 7 +++ .../Preview/NoopPreviewTokenGenerator.cs | 8 +++ .../Security/ICoreBackOfficeSignInManager.cs | 8 +++ src/Umbraco.Core/Services/IPreviewService.cs | 11 +++- src/Umbraco.Core/Services/PreviewService.cs | 61 +++++++++++++++++-- .../UmbracoBuilderExtensions.cs | 3 + .../PreviewAuthenticationMiddleware.cs | 54 ++++++++-------- .../Preview/UserBasedPreviewTokenGenerator.cs | 56 +++++++++++++++++ .../Security/IBackOfficeSignInManager.cs | 2 +- .../Security/UmbracoSignInManager.cs | 8 +++ .../ManagementApi/Preview/EndPreviewTests.cs | 4 +- .../Preview/EnterPreviewTests.cs | 7 +-- 17 files changed, 211 insertions(+), 58 deletions(-) create mode 100644 src/Umbraco.Core/Preview/IPreviewTokenGenerator.cs create mode 100644 src/Umbraco.Core/Preview/NoopPreviewTokenGenerator.cs create mode 100644 src/Umbraco.Core/Security/ICoreBackOfficeSignInManager.cs create mode 100644 src/Umbraco.Web.Common/Preview/UserBasedPreviewTokenGenerator.cs diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Preview/EndPreviewController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Preview/EndPreviewController.cs index 31f8d21e77..f7b333a2ad 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Preview/EndPreviewController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Preview/EndPreviewController.cs @@ -1,4 +1,5 @@ using Asp.Versioning; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Umbraco.Cms.Core.Services; @@ -15,9 +16,10 @@ public class EndPreviewController : PreviewControllerBase [HttpDelete] [MapToApiVersion("1.0")] [ProducesResponseType(StatusCodes.Status200OK)] - public IActionResult End(CancellationToken cancellationToken) + [AllowAnonymous] // It's okay the client can do this from the website without having a token + public async Task End(CancellationToken cancellationToken) { - _previewService.EndPreview(); + await _previewService.EndPreviewAsync(); return Ok(); } } diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Preview/EnterPreviewController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Preview/EnterPreviewController.cs index a6defefb71..5f8ebea670 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Preview/EnterPreviewController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Preview/EnterPreviewController.cs @@ -1,6 +1,7 @@ using Asp.Versioning; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Api.Management.Controllers.Preview; @@ -9,15 +10,27 @@ namespace Umbraco.Cms.Api.Management.Controllers.Preview; public class EnterPreviewController : PreviewControllerBase { private readonly IPreviewService _previewService; + private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; - public EnterPreviewController(IPreviewService previewService) => _previewService = previewService; + public EnterPreviewController(IPreviewService previewService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor) + { + _previewService = previewService; + _backOfficeSecurityAccessor = backOfficeSecurityAccessor; + } [HttpPost] [MapToApiVersion("1.0")] [ProducesResponseType(StatusCodes.Status200OK)] - public IActionResult Enter(CancellationToken cancellationToken) + public async Task Enter(CancellationToken cancellationToken) { - _previewService.EnterPreview(); - return Ok(); + return await _previewService.TryEnterPreviewAsync(CurrentUser(_backOfficeSecurityAccessor)) + ? Ok() + : StatusCode(StatusCodes.Status500InternalServerError, new ProblemDetails + { + Title = "Could not enter preview", + Detail = "Something unexpected went wrong trying to activate preview mode for the current user", + Status = StatusCodes.Status500InternalServerError, + Type = "Error", + }); } } diff --git a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs index 6e0f79cf48..a445268db7 100644 --- a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs +++ b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs @@ -62,6 +62,7 @@ public static partial class UmbracoBuilderExtensions // We also need to register the store as a core-friendly interface that doesn't leak technology. services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/Umbraco.Cms.Api.Management/OpenApi.json b/src/Umbraco.Cms.Api.Management/OpenApi.json index c8af27d070..3da37e1149 100644 --- a/src/Umbraco.Cms.Api.Management/OpenApi.json +++ b/src/Umbraco.Cms.Api.Management/OpenApi.json @@ -22393,16 +22393,8 @@ } } } - }, - "401": { - "description": "The resource is protected and requires an authentication token" } - }, - "security": [ - { - "Backoffice User": [ ] - } - ] + } }, "post": { "tags": [ diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index 725cacac81..11c9e91099 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -35,6 +35,7 @@ using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Services.ContentTypeEditing; using Umbraco.Cms.Core.DynamicRoot; +using Umbraco.Cms.Core.Preview; using Umbraco.Cms.Core.Security.Authorization; using Umbraco.Cms.Core.Services.FileSystem; using Umbraco.Cms.Core.Services.ImportExport; @@ -349,6 +350,7 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddSingleton(); Services.AddSingleton(); + Services.AddUnique(); Services.AddUnique(); // Register a noop IHtmlSanitizer & IMarkdownSanitizer to be replaced diff --git a/src/Umbraco.Core/Preview/IPreviewTokenGenerator.cs b/src/Umbraco.Core/Preview/IPreviewTokenGenerator.cs new file mode 100644 index 0000000000..1b1d649268 --- /dev/null +++ b/src/Umbraco.Core/Preview/IPreviewTokenGenerator.cs @@ -0,0 +1,7 @@ +namespace Umbraco.Cms.Core.Preview; + +public interface IPreviewTokenGenerator +{ + Task> GenerateTokenAsync(Guid userKey); + Task> VerifyAsync(string token); +} diff --git a/src/Umbraco.Core/Preview/NoopPreviewTokenGenerator.cs b/src/Umbraco.Core/Preview/NoopPreviewTokenGenerator.cs new file mode 100644 index 0000000000..8a9c0e8234 --- /dev/null +++ b/src/Umbraco.Core/Preview/NoopPreviewTokenGenerator.cs @@ -0,0 +1,8 @@ +namespace Umbraco.Cms.Core.Preview; + +public class NoopPreviewTokenGenerator : IPreviewTokenGenerator +{ + public Task> GenerateTokenAsync(Guid userKey) => Task.FromResult(Attempt.Fail(string.Empty)); + + public Task> VerifyAsync(string token) => Task.FromResult(Attempt.Fail(null)); +} diff --git a/src/Umbraco.Core/Security/ICoreBackOfficeSignInManager.cs b/src/Umbraco.Core/Security/ICoreBackOfficeSignInManager.cs new file mode 100644 index 0000000000..e2ec297608 --- /dev/null +++ b/src/Umbraco.Core/Security/ICoreBackOfficeSignInManager.cs @@ -0,0 +1,8 @@ +using System.Security.Claims; + +namespace Umbraco.Cms.Core.Security; + +public interface ICoreBackOfficeSignInManager +{ + Task CreateUserPrincipalAsync(Guid userKey); +} diff --git a/src/Umbraco.Core/Services/IPreviewService.cs b/src/Umbraco.Core/Services/IPreviewService.cs index 2bcd4e496c..c9e276d83e 100644 --- a/src/Umbraco.Core/Services/IPreviewService.cs +++ b/src/Umbraco.Core/Services/IPreviewService.cs @@ -1,14 +1,19 @@ -namespace Umbraco.Cms.Core.Services; +using System.Security.Claims; +using Umbraco.Cms.Core.Models.Membership; + +namespace Umbraco.Cms.Core.Services; public interface IPreviewService { /// /// Enters preview mode for a given user that calls this /// - void EnterPreview(); + Task TryEnterPreviewAsync(IUser user); /// /// Exits preview mode for a given user that calls this /// - void EndPreview(); + Task EndPreviewAsync(); + + Task> TryGetPreviewClaimsIdentityAsync(); } diff --git a/src/Umbraco.Core/Services/PreviewService.cs b/src/Umbraco.Core/Services/PreviewService.cs index 1103a1314b..bc11ab53bf 100644 --- a/src/Umbraco.Core/Services/PreviewService.cs +++ b/src/Umbraco.Core/Services/PreviewService.cs @@ -1,15 +1,68 @@ -using Umbraco.Cms.Core.Web; +using System.Security.Claims; +using Microsoft.Extensions.DependencyInjection; +using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Preview; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Web; +using Umbraco.Extensions; namespace Umbraco.Cms.Core.Services; public class PreviewService : IPreviewService { private readonly ICookieManager _cookieManager; + private readonly IPreviewTokenGenerator _previewTokenGenerator; + private readonly IServiceScopeFactory _serviceScopeFactory; - public PreviewService(ICookieManager cookieManager) => _cookieManager = cookieManager; + public PreviewService( + ICookieManager cookieManager, + IPreviewTokenGenerator previewTokenGenerator, + IServiceScopeFactory serviceScopeFactory) + { + _cookieManager = cookieManager; + _previewTokenGenerator = previewTokenGenerator; + _serviceScopeFactory = serviceScopeFactory; + } - public void EnterPreview() => _cookieManager.SetCookieValue(Constants.Web.PreviewCookieName, "preview", true); + public async Task TryEnterPreviewAsync(IUser user) + { + Attempt attempt = await _previewTokenGenerator.GenerateTokenAsync(user.Key); + if (attempt.Success) + { + _cookieManager.SetCookieValue(Constants.Web.PreviewCookieName, attempt.Result!, true); + } - public void EndPreview() => _cookieManager.ExpireCookie(Constants.Web.PreviewCookieName); + return attempt.Success; + } + + public Task EndPreviewAsync() + { + _cookieManager.ExpireCookie(Constants.Web.PreviewCookieName); + return Task.CompletedTask; + } + + public async Task> TryGetPreviewClaimsIdentityAsync() + { + var cookieValue = _cookieManager.GetCookieValue(Constants.Web.PreviewCookieName); + if (string.IsNullOrWhiteSpace(cookieValue)) + { + return Attempt.Fail(); + } + + Attempt userKeyAttempt = await _previewTokenGenerator.VerifyAsync(cookieValue); + + if (userKeyAttempt.Success is false) + { + return Attempt.Fail(); + } + + IServiceScope serviceScope = _serviceScopeFactory.CreateScope(); + ICoreBackOfficeSignInManager coreBackOfficeSignInManager = serviceScope.ServiceProvider.GetRequiredService(); + + ClaimsPrincipal? principal = await coreBackOfficeSignInManager.CreateUserPrincipalAsync(userKeyAttempt.Result!.Value); + ClaimsIdentity? backOfficeIdentity = principal?.GetUmbracoIdentity(); + + return Attempt.Succeed(backOfficeIdentity); + } } diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs index bc34221aad..85a9d7ded4 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs @@ -27,6 +27,7 @@ using Umbraco.Cms.Core.Logging; using Umbraco.Cms.Core.Net; using Umbraco.Cms.Core.Notifications; using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Preview; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Templates; @@ -52,6 +53,7 @@ using Umbraco.Cms.Web.Common.Localization; using Umbraco.Cms.Web.Common.Middleware; using Umbraco.Cms.Web.Common.ModelBinders; using Umbraco.Cms.Web.Common.Mvc; +using Umbraco.Cms.Web.Common.Preview; using Umbraco.Cms.Web.Common.Profiler; using Umbraco.Cms.Web.Common.Repositories; using Umbraco.Cms.Web.Common.Security; @@ -164,6 +166,7 @@ public static partial class UmbracoBuilderExtensions builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddTransient(); + builder.Services.AddUnique(); return builder; } diff --git a/src/Umbraco.Web.Common/Middleware/PreviewAuthenticationMiddleware.cs b/src/Umbraco.Web.Common/Middleware/PreviewAuthenticationMiddleware.cs index 5bd16867ca..e61b8682af 100644 --- a/src/Umbraco.Web.Common/Middleware/PreviewAuthenticationMiddleware.cs +++ b/src/Umbraco.Web.Common/Middleware/PreviewAuthenticationMiddleware.cs @@ -5,6 +5,12 @@ using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Preview; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.Common.AspNetCore; +using Umbraco.Cms.Web.Common.Security; using Umbraco.Extensions; namespace Umbraco.Cms.Web.Common.Middleware; @@ -16,8 +22,19 @@ namespace Umbraco.Cms.Web.Common.Middleware; public class PreviewAuthenticationMiddleware : IMiddleware { private readonly ILogger _logger; + private readonly IPreviewTokenGenerator _previewTokenGenerator; + private readonly IPreviewService _previewService; - public PreviewAuthenticationMiddleware(ILogger logger) => _logger = logger; + + public PreviewAuthenticationMiddleware( + ILogger logger, + IPreviewTokenGenerator previewTokenGenerator, + IPreviewService previewService) + { + _logger = logger; + _previewTokenGenerator = previewTokenGenerator; + _previewService = previewService; + } /// public async Task InvokeAsync(HttpContext context, RequestDelegate next) @@ -38,37 +55,18 @@ public class PreviewAuthenticationMiddleware : IMiddleware if (isPreview) { - CookieAuthenticationOptions? cookieOptions = context.RequestServices - .GetRequiredService>() - .Get(Core.Constants.Security.BackOfficeAuthenticationType); + Attempt backOfficeIdentityAttempt = await _previewService.TryGetPreviewClaimsIdentityAsync(); - if (cookieOptions == null) + if (backOfficeIdentityAttempt.Success) { - throw new InvalidOperationException("No cookie options found with name " + - Core.Constants.Security.BackOfficeAuthenticationType); + // Ok, we've got a real ticket, now we can add this ticket's identity to the current + // Principal, this means we'll have 2 identities assigned to the principal which we can + // use to authorize the preview and allow for a back office User. + context.User.AddIdentity(backOfficeIdentityAttempt.Result!); } - - // If we've gotten this far it means a preview cookie has been set and a front-end umbraco document request is executing. - // In this case, authentication will not have occurred for an Umbraco back office User, however we need to perform the authentication - // for the user here so that the preview capability can be authorized otherwise only the non-preview page will be rendered. - if (cookieOptions.Cookie.Name != null) + else { - var chunkingCookieManager = new ChunkingCookieManager(); - var cookie = chunkingCookieManager.GetRequestCookie(context, cookieOptions.Cookie.Name); - - if (!string.IsNullOrEmpty(cookie)) - { - AuthenticationTicket? unprotected = cookieOptions.TicketDataFormat.Unprotect(cookie); - ClaimsIdentity? backOfficeIdentity = unprotected?.Principal.GetUmbracoIdentity(); - - if (backOfficeIdentity != null) - { - // Ok, we've got a real ticket, now we can add this ticket's identity to the current - // Principal, this means we'll have 2 identities assigned to the principal which we can - // use to authorize the preview and allow for a back office User. - context.User.AddIdentity(backOfficeIdentity); - } - } + _logger.LogDebug("Could not transform previewCookie value into a claimsIdentity"); } } } diff --git a/src/Umbraco.Web.Common/Preview/UserBasedPreviewTokenGenerator.cs b/src/Umbraco.Web.Common/Preview/UserBasedPreviewTokenGenerator.cs new file mode 100644 index 0000000000..c84701f495 --- /dev/null +++ b/src/Umbraco.Web.Common/Preview/UserBasedPreviewTokenGenerator.cs @@ -0,0 +1,56 @@ +using Microsoft.AspNetCore.DataProtection; +using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Preview; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.Common.Security; + +namespace Umbraco.Cms.Web.Common.Preview; + +public class UserBasedPreviewTokenGenerator : IPreviewTokenGenerator +{ + private readonly IDataProtectionProvider _dataProtectionProvider; + private readonly IUserService _userService; + private readonly ILogger _logger; + + public UserBasedPreviewTokenGenerator( + IDataProtectionProvider dataProtectionProvider, + IUserService userService, + ILogger logger) + { + _dataProtectionProvider = dataProtectionProvider; + _userService = userService; + _logger = logger; + } + + public Task> GenerateTokenAsync(Guid userKey) + { + var token = EncryptionHelper.Encrypt(userKey.ToString(), _dataProtectionProvider); + + return Task.FromResult(Attempt.Succeed(token)); + } + + public async Task> VerifyAsync(string token) + { + try + { + var decrypted = EncryptionHelper.Decrypt(token, _dataProtectionProvider); + if (Guid.TryParse(decrypted, out Guid key)) + { + IUser? user = await _userService.GetAsync(key); + + if (user is { IsApproved: true, IsLockedOut: false }) + { + return Attempt.Succeed(user.Key); + } + } + } + catch (Exception e) + { + _logger.LogDebug(e, "An error occured when trying to get the user from the encrypted token"); + } + + return Attempt.Fail(null); + } +} diff --git a/src/Umbraco.Web.Common/Security/IBackOfficeSignInManager.cs b/src/Umbraco.Web.Common/Security/IBackOfficeSignInManager.cs index 51cc795704..ec6cf62828 100644 --- a/src/Umbraco.Web.Common/Security/IBackOfficeSignInManager.cs +++ b/src/Umbraco.Web.Common/Security/IBackOfficeSignInManager.cs @@ -9,7 +9,7 @@ namespace Umbraco.Cms.Web.Common.Security; /// A for the back office with a /// /// -public interface IBackOfficeSignInManager +public interface IBackOfficeSignInManager : ICoreBackOfficeSignInManager { AuthenticationProperties ConfigureExternalAuthenticationProperties(string provider, string? redirectUrl, string? userId = null); diff --git a/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs b/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs index 716cb2f032..f0ab7e02e2 100644 --- a/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs +++ b/src/Umbraco.Web.Common/Security/UmbracoSignInManager.cs @@ -85,6 +85,14 @@ public abstract class UmbracoSignInManager : SignInManager return result; } + public virtual async Task CreateUserPrincipalAsync(Guid userKey) + { + TUser? user = await UserManager.FindByIdAsync(userKey.ToString()); + return user is null + ? null + : await this.ClaimsFactory.CreateAsync(user); + } + /// public override async Task GetExternalLoginInfoAsync(string? expectedXsrf = null) { diff --git a/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EndPreviewTests.cs b/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EndPreviewTests.cs index 9e037fb6fc..88443584cc 100644 --- a/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EndPreviewTests.cs +++ b/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EndPreviewTests.cs @@ -13,10 +13,8 @@ public class EndPreviewTests : ManagementApiTest [Test] - public virtual async Task As_Admin_I_Have_Access() + public virtual async Task As_Anonymous_I_Can_End_Preview_Mode() { - await AuthenticateClientAsync(Client, "admin@umbraco.com", "1234567890", true); - var response = await Client.DeleteAsync(Url); // Check if the set cookie header is sent diff --git a/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EnterPreviewTests.cs b/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EnterPreviewTests.cs index 998fc316da..37c7bcb5dc 100644 --- a/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EnterPreviewTests.cs +++ b/tests/Umbraco.Tests.Integration/ManagementApi/Preview/EnterPreviewTests.cs @@ -13,18 +13,17 @@ public class EnterPreviewTests : ManagementApiTest [Test] - public virtual async Task As_Admin_I_Have_Access() + public virtual async Task As_Editor_I_Can_Enter_Preview_Mode() { - await AuthenticateClientAsync(Client, "admin@umbraco.com", "1234567890", true); + await AuthenticateClientAsync(Client, "admin@umbraco.com", "1234567890", false); var response = await Client.PostAsync(Url, null); // Check if the set cookie header is sent var doesHeaderExist = response.Headers.TryGetValues("Set-Cookie", out var setCookieValues) && - setCookieValues.Any(value => value.Contains($"{Constants.Web.PreviewCookieName}=preview; path=/")); + setCookieValues.Any(value => value.Contains($"{Constants.Web.PreviewCookieName}=") && value.Contains("path=/") && value.Contains("httponly")); Assert.IsTrue(doesHeaderExist); Assert.AreEqual(HttpStatusCode.OK, response.StatusCode, await response.Content.ReadAsStringAsync()); } - }