Management API: Fix OAuth client registration permanently skipped after transient failure (closes #22356)

src/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddleware.cs

@@ -18,7 +18,7 @@
 public class BackOfficeAuthorizationInitializationMiddleware : IMiddleware
 {
     private SemaphoreSlim _firstBackOfficeRequestLocker = new(1); // this only works because this is a singleton
-    private ISet<string> _knownHosts = new HashSet<string>(); // this only works because this is a singleton
+    private ISet<string> _knownHosts = new HashSet<string>(StringComparer.OrdinalIgnoreCase); // this only works because this is a singleton
 
     private readonly UmbracoRequestPaths _umbracoRequestPaths;
     private readonly IServiceProvider _serviceProvider;
@@ -79,30 +79,48 @@
 
         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<string>(StringComparer.OrdinalIgnoreCase) { 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<IBackOfficeApplicationManager>();
-        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<IBackOfficeApplicationManager>();
+            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();
+        }
     }
 }

tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Middleware/BackOfficeAuthorizationInitializationMiddlewareTests.cs

@@ -0,0 +1,163 @@
+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<IRuntimeState> _mockRuntimeState = null!;
+    private Mock<IBackOfficeApplicationManager> _mockBackOfficeApplicationManager = null!;
+    private BackOfficeAuthorizationInitializationMiddleware _middleware = null!;
+
+    [SetUp]
+    public void SetUp()
+    {
+        _mockRuntimeState = new Mock<IRuntimeState>();
+        _mockRuntimeState.Setup(x => x.Level).Returns(RuntimeLevel.Run);
+
+        _mockBackOfficeApplicationManager = new Mock<IBackOfficeApplicationManager>();
+
+        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<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
+            .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.
+        Assert.ThrowsAsync<Exception>(async () => await _middleware.InvokeAsync(context, next));
+
+        // 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<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
+            .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<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
+            Times.Once);
+    }
+
+    [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
+        _mockBackOfficeApplicationManager
+            .Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
+            .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();
+            Assert.ThrowsAsync<Exception>(async () => await _middleware.InvokeAsync(context, next));
+        }
+
+        // Assert — all 3 attempts reached EnsureBackOfficeApplicationAsync (no deadlock).
+        _mockBackOfficeApplicationManager.Verify(
+            x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
+            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<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
+            Times.Never);
+    }
+
+    /// <summary>
+    /// Creates an HttpContext with a path that <see cref="UmbracoRequestPaths.IsBackOfficeRequest"/> recognizes.
+    /// The Management API path (/umbraco/management/api/) is a backoffice request.
+    /// </summary>
+    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<IHostingEnvironment>();
+        mockHostingEnvironment.Setup(x => x.ApplicationVirtualPath).Returns("/");
+        mockHostingEnvironment.Setup(x => x.ToAbsolute(It.IsAny<string>()))
+            .Returns<string>(path => path.TrimStart('~'));
+
+        var options = Options.Create(new UmbracoRequestPathsOptions());
+
+        return new UmbracoRequestPaths(mockHostingEnvironment.Object, options);
+    }
+}