Skip to content

Commit 4b1f7e5

Browse files
AndyButlandAndyButland
committed
Management API: Fix OAuth client registration permanently skipped after transient failure (closes #22356) (#22368)
* Prevent OAuth client registration from being permanently skipped after transient failure. * Addressed code review feedback.
1 parent 79cf047 commit 4b1f7e5

2 files changed

Lines changed: 202 additions & 21 deletions

File tree

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

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace Umbraco.Cms.Api.Management.Middleware;
1414
public class BackOfficeAuthorizationInitializationMiddleware : IMiddleware
1515
{
1616
private SemaphoreSlim _firstBackOfficeRequestLocker = new(1); // this only works because this is a singleton
17-
private ISet<string> _knownHosts = new HashSet<string>(); // this only works because this is a singleton
17+
private ISet<string> _knownHosts = new HashSet<string>(StringComparer.OrdinalIgnoreCase); // this only works because this is a singleton
1818

1919
private readonly UmbracoRequestPaths _umbracoRequestPaths;
2020
private readonly IServiceProvider _serviceProvider;
@@ -61,30 +61,48 @@ private async Task InitializeBackOfficeAuthorizationOnceAsync(HttpContext contex
6161

6262
await _firstBackOfficeRequestLocker.WaitAsync();
6363

64-
// NOTE: _knownHosts is not thread safe; check again after entering the semaphore
65-
if (_knownHosts.Add(host) is false)
64+
try
6665
{
67-
_firstBackOfficeRequestLocker.Release();
68-
return;
69-
}
66+
// NOTE: _knownHosts is not thread safe; check again after entering the semaphore.
67+
if (_knownHosts.Contains(host))
68+
{
69+
return;
70+
}
7071

71-
// ensure we explicitly add UmbracoApplicationUrl if configured (https://github.com/umbraco/Umbraco-CMS/issues/16179)
72-
if (_webRoutingSettings.UmbracoApplicationUrl.IsNullOrWhiteSpace() is false)
73-
{
74-
_knownHosts.Add(_webRoutingSettings.UmbracoApplicationUrl);
75-
}
72+
// Ensure we explicitly add UmbracoApplicationUrl if configured (https://github.com/umbraco/Umbraco-CMS/issues/16179).
73+
var hostsToRegister = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { host };
74+
if (_webRoutingSettings.UmbracoApplicationUrl.IsNullOrWhiteSpace() is false)
75+
{
76+
hostsToRegister.Add(_webRoutingSettings.UmbracoApplicationUrl);
77+
}
7678

77-
Uri[] backOfficeHosts = _knownHosts
78-
.Select(host => Uri.TryCreate(host, UriKind.Absolute, out Uri? hostUri)
79-
? hostUri
80-
: null)
81-
.WhereNotNull()
82-
.ToArray();
79+
// Merge with already-known hosts so the OpenIddict application includes all redirect URIs.
80+
foreach (var knownHost in _knownHosts)
81+
{
82+
hostsToRegister.Add(knownHost);
83+
}
8384

84-
using IServiceScope scope = _serviceProvider.CreateScope();
85-
IBackOfficeApplicationManager backOfficeApplicationManager = scope.ServiceProvider.GetRequiredService<IBackOfficeApplicationManager>();
86-
await backOfficeApplicationManager.EnsureBackOfficeApplicationAsync(backOfficeHosts);
85+
Uri[] backOfficeHosts = hostsToRegister
86+
.Select(h => Uri.TryCreate(h, UriKind.Absolute, out Uri? hostUri)
87+
? hostUri
88+
: null)
89+
.WhereNotNull()
90+
.ToArray();
8791

88-
_firstBackOfficeRequestLocker.Release();
92+
using IServiceScope scope = _serviceProvider.CreateScope();
93+
IBackOfficeApplicationManager backOfficeApplicationManager = scope.ServiceProvider.GetRequiredService<IBackOfficeApplicationManager>();
94+
await backOfficeApplicationManager.EnsureBackOfficeApplicationAsync(backOfficeHosts);
95+
96+
// Only mark hosts as known after successful registration, so a transient failure
97+
// (e.g. database contention during an unattended upgrade) is retried on the next request.
98+
foreach (var registered in hostsToRegister)
99+
{
100+
_knownHosts.Add(registered);
101+
}
102+
}
103+
finally
104+
{
105+
_firstBackOfficeRequestLocker.Release();
106+
}
89107
}
90108
}

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

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
using Microsoft.AspNetCore.Http;
2+
using Microsoft.Extensions.DependencyInjection;
3+
using Microsoft.Extensions.Options;
4+
using Moq;
5+
using NUnit.Framework;
6+
using Umbraco.Cms.Api.Management.Middleware;
7+
using Umbraco.Cms.Core;
8+
using Umbraco.Cms.Core.Configuration.Models;
9+
using Umbraco.Cms.Core.Hosting;
10+
using Umbraco.Cms.Core.Routing;
11+
using Umbraco.Cms.Core.Services;
12+
using Umbraco.Cms.Infrastructure.Security;
13+
14+
namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Cms.Api.Management.Middleware;
15+
16+
[TestFixture]
17+
public class BackOfficeAuthorizationInitializationMiddlewareTests
18+
{
19+
private Mock<IRuntimeState> _mockRuntimeState = null!;
20+
private Mock<IBackOfficeApplicationManager> _mockBackOfficeApplicationManager = null!;
21+
private BackOfficeAuthorizationInitializationMiddleware _middleware = null!;
22+
23+
[SetUp]
24+
public void SetUp()
25+
{
26+
_mockRuntimeState = new Mock<IRuntimeState>();
27+
_mockRuntimeState.Setup(x => x.Level).Returns(RuntimeLevel.Run);
28+
29+
_mockBackOfficeApplicationManager = new Mock<IBackOfficeApplicationManager>();
30+
31+
var serviceCollection = new ServiceCollection();
32+
serviceCollection.AddSingleton(_mockBackOfficeApplicationManager.Object);
33+
ServiceProvider serviceProvider = serviceCollection.BuildServiceProvider();
34+
35+
UmbracoRequestPaths umbracoRequestPaths = CreateUmbracoRequestPaths();
36+
var webRoutingSettings = Options.Create(new WebRoutingSettings());
37+
38+
_middleware = new BackOfficeAuthorizationInitializationMiddleware(
39+
umbracoRequestPaths,
40+
serviceProvider,
41+
_mockRuntimeState.Object,
42+
webRoutingSettings);
43+
}
44+
45+
[Test]
46+
public async Task Registration_Failure_Does_Not_Cache_Host_So_Next_Request_Retries()
47+
{
48+
// Arrange — first call throws (simulating database contention during upgrade).
49+
var callCount = 0;
50+
_mockBackOfficeApplicationManager
51+
.Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
52+
.Returns(() =>
53+
{
54+
callCount++;
55+
if (callCount == 1)
56+
{
57+
throw new Exception("Database is locked");
58+
}
59+
60+
return Task.CompletedTask;
61+
});
62+
63+
RequestDelegate next = _ => Task.CompletedTask;
64+
HttpContext context = CreateBackOfficeHttpContext();
65+
66+
// Act — first request: registration fails.
67+
Assert.ThrowsAsync<Exception>(async () => await _middleware.InvokeAsync(context, next));
68+
69+
// Act — second request: should retry (host was NOT cached).
70+
await _middleware.InvokeAsync(context, next);
71+
72+
// Assert — EnsureBackOfficeApplicationAsync was called twice (retried after failure).
73+
Assert.That(callCount, Is.EqualTo(2));
74+
}
75+
76+
[Test]
77+
public async Task Successful_Registration_Caches_Host_So_Subsequent_Requests_Skip()
78+
{
79+
// Arrange
80+
_mockBackOfficeApplicationManager
81+
.Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
82+
.Returns(Task.CompletedTask);
83+
84+
RequestDelegate next = _ => Task.CompletedTask;
85+
HttpContext context = CreateBackOfficeHttpContext();
86+
87+
// Act
88+
await _middleware.InvokeAsync(context, next);
89+
await _middleware.InvokeAsync(context, next);
90+
91+
// Assert — only called once (second request skipped because host was cached)
92+
_mockBackOfficeApplicationManager.Verify(
93+
x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
94+
Times.Once);
95+
}
96+
97+
[Test]
98+
[Timeout(5000)] // Fail fast if the semaphore-release regression is reintroduced (deadlock).
99+
public async Task Registration_Failure_Does_Not_Leak_Semaphore()
100+
{
101+
// Arrange — always throws
102+
_mockBackOfficeApplicationManager
103+
.Setup(x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()))
104+
.ThrowsAsync(new Exception("Database is locked"));
105+
106+
RequestDelegate next = _ => Task.CompletedTask;
107+
108+
// Act — multiple failing requests should not deadlock (semaphore released via finally).
109+
for (var i = 0; i < 3; i++)
110+
{
111+
HttpContext context = CreateBackOfficeHttpContext();
112+
Assert.ThrowsAsync<Exception>(async () => await _middleware.InvokeAsync(context, next));
113+
}
114+
115+
// Assert — all 3 attempts reached EnsureBackOfficeApplicationAsync (no deadlock).
116+
_mockBackOfficeApplicationManager.Verify(
117+
x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
118+
Times.Exactly(3));
119+
}
120+
121+
[Test]
122+
public async Task RuntimeLevel_Below_Upgrade_Skips_Registration()
123+
{
124+
// Arrange
125+
_mockRuntimeState.Setup(x => x.Level).Returns(RuntimeLevel.Install);
126+
127+
RequestDelegate next = _ => Task.CompletedTask;
128+
HttpContext context = CreateBackOfficeHttpContext();
129+
130+
// Act
131+
await _middleware.InvokeAsync(context, next);
132+
133+
// Assert
134+
_mockBackOfficeApplicationManager.Verify(
135+
x => x.EnsureBackOfficeApplicationAsync(It.IsAny<IEnumerable<Uri>>(), It.IsAny<CancellationToken>()),
136+
Times.Never);
137+
}
138+
139+
/// <summary>
140+
/// Creates an HttpContext with a path that <see cref="UmbracoRequestPaths.IsBackOfficeRequest"/> recognizes.
141+
/// The Management API path (/umbraco/management/api/) is a backoffice request.
142+
/// </summary>
143+
private static HttpContext CreateBackOfficeHttpContext()
144+
{
145+
var context = new DefaultHttpContext();
146+
context.Request.Scheme = "https";
147+
context.Request.Host = new HostString("localhost", 44339);
148+
context.Request.Path = "/umbraco/management/api/v1/server/status";
149+
return context;
150+
}
151+
152+
private static UmbracoRequestPaths CreateUmbracoRequestPaths()
153+
{
154+
var mockHostingEnvironment = new Mock<IHostingEnvironment>();
155+
mockHostingEnvironment.Setup(x => x.ApplicationVirtualPath).Returns("/");
156+
mockHostingEnvironment.Setup(x => x.ToAbsolute(It.IsAny<string>()))
157+
.Returns<string>(path => path.TrimStart('~'));
158+
159+
var options = Options.Create(new UmbracoRequestPathsOptions());
160+
161+
return new UmbracoRequestPaths(mockHostingEnvironment.Object, options);
162+
}
163+
}

0 commit comments

Comments
 (0)