From ea520630745e2ad448d51637bb0a7f01a7af3aa7 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 7 Apr 2026 16:17:19 +0200 Subject: [PATCH 1/2] Prevent OAuth client registration from being permanently skipped after transient failure. --- ...ceAuthorizationInitializationMiddleware.cs | 58 ++++-- ...horizationInitializationMiddlewareTests.cs | 176 ++++++++++++++++++ 2 files changed, 214 insertions(+), 20 deletions(-) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs diff --git a/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs b/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs index 60bab0d14e15..53bf535ab2a6 100644 --- a/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs +++ b/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs @@ -79,30 +79,48 @@ private async Task InitializeBackOfficeAuthorizationOnceAsync(HttpContext contex await _firstBackOfficeRequestLocker.WaitAsync(); - // NOTE: _knownHosts is not thread safe; check again after entering the semaphore - if (_knownHosts.Add(host) is false) + try { - _firstBackOfficeRequestLocker.Release(); - return; - } + // NOTE: _knownHosts is not thread safe; check again after entering the semaphore. + if (_knownHosts.Contains(host)) + { + return; + } - // ensure we explicitly add UmbracoApplicationUrl if configured (https://github.com/umbraco/Umbraco-CMS/issues/16179) - if (_webRoutingSettings.UmbracoApplicationUrl.IsNullOrWhiteSpace() is false) - { - _knownHosts.Add(_webRoutingSettings.UmbracoApplicationUrl); - } + // Ensure we explicitly add UmbracoApplicationUrl if configured (https://github.com/umbraco/Umbraco-CMS/issues/16179). + var hostsToRegister = new HashSet { host }; + if (_webRoutingSettings.UmbracoApplicationUrl.IsNullOrWhiteSpace() is false) + { + hostsToRegister.Add(_webRoutingSettings.UmbracoApplicationUrl); + } - Uri[] backOfficeHosts = _knownHosts - .Select(host => Uri.TryCreate(host, UriKind.Absolute, out Uri? hostUri) - ? hostUri - : null) - .WhereNotNull() - .ToArray(); + // Merge with already-known hosts so the OpenIddict application includes all redirect URIs. + foreach (var knownHost in _knownHosts) + { + hostsToRegister.Add(knownHost); + } - using IServiceScope scope = _serviceProvider.CreateScope(); - IBackOfficeApplicationManager backOfficeApplicationManager = scope.ServiceProvider.GetRequiredService(); - await backOfficeApplicationManager.EnsureBackOfficeApplicationAsync(backOfficeHosts); + Uri[] backOfficeHosts = hostsToRegister + .Select(h => Uri.TryCreate(h, UriKind.Absolute, out Uri? hostUri) + ? hostUri + : null) + .WhereNotNull() + .ToArray(); - _firstBackOfficeRequestLocker.Release(); + using IServiceScope scope = _serviceProvider.CreateScope(); + IBackOfficeApplicationManager backOfficeApplicationManager = scope.ServiceProvider.GetRequiredService(); + await backOfficeApplicationManager.EnsureBackOfficeApplicationAsync(backOfficeHosts); + + // Only mark hosts as known after successful registration, so a transient failure + // (e.g. database contention during an unattended upgrade) is retried on the next request. + foreach (var registered in hostsToRegister) + { + _knownHosts.Add(registered); + } + } + finally + { + _firstBackOfficeRequestLocker.Release(); + } } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs new file mode 100644 index 000000000000..ebbe640126a6 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs @@ -0,0 +1,176 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Api.Management.Middleware; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Core.Routing; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Infrastructure.Security; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Cms.Api.Management.Middleware; + +[TestFixture] +public class BackOfficeAuthorizationInitializationMiddlewareTests +{ + private Mock _mockRuntimeState = null!; + private Mock _mockBackOfficeApplicationManager = null!; + private BackOfficeAuthorizationInitializationMiddleware _middleware = null!; + + [SetUp] + public void SetUp() + { + _mockRuntimeState = new Mock(); + _mockRuntimeState.Setup(x => x.Level).Returns(RuntimeLevel.Run); + + _mockBackOfficeApplicationManager = new Mock(); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(_mockBackOfficeApplicationManager.Object); + ServiceProvider serviceProvider = serviceCollection.BuildServiceProvider(); + + UmbracoRequestPaths umbracoRequestPaths = CreateUmbracoRequestPaths(); + var webRoutingSettings = Options.Create(new WebRoutingSettings()); + + _middleware = new BackOfficeAuthorizationInitializationMiddleware( + umbracoRequestPaths, + serviceProvider, + _mockRuntimeState.Object, + webRoutingSettings); + } + + [Test] + public async Task Registration_Failure_Does_Not_Cache_Host_So_Next_Request_Retries() + { + // Arrange — first call throws (simulating database contention during upgrade). + var callCount = 0; + _mockBackOfficeApplicationManager + .Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny())) + .Returns(() => + { + callCount++; + if (callCount == 1) + { + throw new Exception("Database is locked"); + } + + return Task.CompletedTask; + }); + + RequestDelegate next = _ => Task.CompletedTask; + HttpContext context = CreateBackOfficeHttpContext(); + + // Act — first request: registration fails. + try + { + await _middleware.InvokeAsync(context, next); + } + catch + { + // Expected — the middleware lets the exception propagate. + } + + // Act — second request: should retry (host was NOT cached). + await _middleware.InvokeAsync(context, next); + + // Assert — EnsureBackOfficeApplicationAsync was called twice (retried after failure). + Assert.That(callCount, Is.EqualTo(2)); + } + + [Test] + public async Task Successful_Registration_Caches_Host_So_Subsequent_Requests_Skip() + { + // Arrange + _mockBackOfficeApplicationManager + .Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny())) + .Returns(Task.CompletedTask); + + RequestDelegate next = _ => Task.CompletedTask; + HttpContext context = CreateBackOfficeHttpContext(); + + // Act + await _middleware.InvokeAsync(context, next); + await _middleware.InvokeAsync(context, next); + + // Assert — only called once (second request skipped because host was cached) + _mockBackOfficeApplicationManager.Verify( + x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny()), + Times.Once); + } + + [Test] + public async Task Registration_Failure_Does_Not_Leak_Semaphore() + { + // Arrange — always throws + _mockBackOfficeApplicationManager + .Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny())) + .ThrowsAsync(new Exception("Database is locked")); + + RequestDelegate next = _ => Task.CompletedTask; + + // Act — multiple failing requests should not deadlock (semaphore released via finally). + for (var i = 0; i < 3; i++) + { + HttpContext context = CreateBackOfficeHttpContext(); + try + { + await _middleware.InvokeAsync(context, next); + } + catch + { + // Expected + } + } + + // Assert — all 3 attempts reached EnsureBackOfficeApplicationAsync (no deadlock). + _mockBackOfficeApplicationManager.Verify( + x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny()), + Times.Exactly(3)); + } + + [Test] + public async Task RuntimeLevel_Below_Upgrade_Skips_Registration() + { + // Arrange + _mockRuntimeState.Setup(x => x.Level).Returns(RuntimeLevel.Install); + + RequestDelegate next = _ => Task.CompletedTask; + HttpContext context = CreateBackOfficeHttpContext(); + + // Act + await _middleware.InvokeAsync(context, next); + + // Assert + _mockBackOfficeApplicationManager.Verify( + x => x.EnsureBackOfficeApplicationAsync(It.IsAny>(), It.IsAny()), + Times.Never); + } + + /// + /// Creates an HttpContext with a path that recognizes. + /// The Management API path (/umbraco/management/api/) is a backoffice request. + /// + private static HttpContext CreateBackOfficeHttpContext() + { + var context = new DefaultHttpContext(); + context.Request.Scheme = "https"; + context.Request.Host = new HostString("localhost", 44339); + context.Request.Path = "/umbraco/management/api/v1/server/status"; + return context; + } + + private static UmbracoRequestPaths CreateUmbracoRequestPaths() + { + var mockHostingEnvironment = new Mock(); + mockHostingEnvironment.Setup(x => x.ApplicationVirtualPath).Returns("/"); + mockHostingEnvironment.Setup(x => x.ToAbsolute(It.IsAny())) + .Returns(path => path.TrimStart('~')); + + var options = Options.Create(new UmbracoRequestPathsOptions()); + + return new UmbracoRequestPaths(mockHostingEnvironment.Object, options); + } +} From d7d75c267ac5b62dd90de87c7bdfc1fd722d14d1 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 8 Apr 2026 09:27:25 +0200 Subject: [PATCH 2/2] Addressed code review feedback. --- ...ceAuthorizationInitializationMiddleware.cs | 4 ++-- ...horizationInitializationMiddlewareTests.cs | 19 +++---------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs b/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs index 53bf535ab2a6..7b963fc9dd9d 100644 --- a/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs +++ b/src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs @@ -18,7 +18,7 @@ namespace Umbraco.Cms.Api.Management.Middleware; public class BackOfficeAuthorizationInitializationMiddleware : IMiddleware { private SemaphoreSlim _firstBackOfficeRequestLocker = new(1); // this only works because this is a singleton - private ISet _knownHosts = new HashSet(); // this only works because this is a singleton + private ISet _knownHosts = new HashSet(StringComparer.OrdinalIgnoreCase); // this only works because this is a singleton private readonly UmbracoRequestPaths _umbracoRequestPaths; private readonly IServiceProvider _serviceProvider; @@ -88,7 +88,7 @@ private async Task InitializeBackOfficeAuthorizationOnceAsync(HttpContext contex } // Ensure we explicitly add UmbracoApplicationUrl if configured (https://github.com/umbraco/Umbraco-CMS/issues/16179). - var hostsToRegister = new HashSet { host }; + var hostsToRegister = new HashSet(StringComparer.OrdinalIgnoreCase) { host }; if (_webRoutingSettings.UmbracoApplicationUrl.IsNullOrWhiteSpace() is false) { hostsToRegister.Add(_webRoutingSettings.UmbracoApplicationUrl); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs index ebbe640126a6..1c60c6b02f57 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs @@ -64,14 +64,7 @@ public async Task Registration_Failure_Does_Not_Cache_Host_So_Next_Request_Retri HttpContext context = CreateBackOfficeHttpContext(); // Act — first request: registration fails. - try - { - await _middleware.InvokeAsync(context, next); - } - catch - { - // Expected — the middleware lets the exception propagate. - } + Assert.ThrowsAsync(async () => await _middleware.InvokeAsync(context, next)); // Act — second request: should retry (host was NOT cached). await _middleware.InvokeAsync(context, next); @@ -102,6 +95,7 @@ public async Task Successful_Registration_Caches_Host_So_Subsequent_Requests_Ski } [Test] + [Timeout(5000)] // Fail fast if the semaphore-release regression is reintroduced (deadlock). public async Task Registration_Failure_Does_Not_Leak_Semaphore() { // Arrange — always throws @@ -115,14 +109,7 @@ public async Task Registration_Failure_Does_Not_Leak_Semaphore() for (var i = 0; i < 3; i++) { HttpContext context = CreateBackOfficeHttpContext(); - try - { - await _middleware.InvokeAsync(context, next); - } - catch - { - // Expected - } + Assert.ThrowsAsync(async () => await _middleware.InvokeAsync(context, next)); } // Assert — all 3 attempts reached EnsureBackOfficeApplicationAsync (no deadlock).