-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
In Auto setup, when IsUninitialized, response http status code 503 #16834
base: main
Are you sure you want to change the base?
Changes from 9 commits
5c0cfbe
cbb43d2
1573e53
ff715ae
5ffc4aa
c02123c
281daa4
511dc12
7a51f44
1fbc8c6
6f99df3
9c25005
2d52d80
37aaf73
1cf54b3
23333d7
4f665ce
4255865
ed75f46
889565e
b039be0
13ba574
debdf12
9d0902f
29a8a10
4b5ae4b
3c806ab
30cf024
d02b9e1
703a622
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using System.Text; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Options; | ||
|
@@ -112,20 +113,23 @@ public async Task InvokeAsync(HttpContext httpContext) | |
} | ||
|
||
// Check if the tenant was installed by another instance. | ||
using var settings = (await _shellSettingsManager | ||
.LoadSettingsAsync(_shellSettings.Name)) | ||
.AsDisposable(); | ||
using var settings = await _shellSettingsManager.LoadSettingsAsync(_shellSettings.Name); | ||
|
||
if (!settings.IsUninitialized()) | ||
if (settings != null) | ||
{ | ||
await _shellHost.ReloadShellContextAsync(_shellSettings, eventSource: false); | ||
httpContext.Response.StatusCode = 503; | ||
|
||
return; | ||
settings.AsDisposable(); | ||
if (!settings.IsUninitialized()) | ||
{ | ||
await _shellHost.ReloadShellContextAsync(_shellSettings, eventSource: false); | ||
httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable; | ||
await httpContext.Response.WriteAsync("The requested tenant is not initialized."); | ||
return; | ||
} | ||
} | ||
|
||
var autoSetupService = httpContext.RequestServices.GetRequiredService<IAutoSetupService>(); | ||
if (await autoSetupService.SetupTenantAsync(_setupOptions, _shellSettings)) | ||
(var setupContext, var isSuccess) = await autoSetupService.SetupTenantAsync(_setupOptions, _shellSettings); | ||
if (isSuccess) | ||
{ | ||
if (_setupOptions.IsDefault) | ||
{ | ||
|
@@ -140,13 +144,17 @@ public async Task InvokeAsync(HttpContext httpContext) | |
} | ||
|
||
httpContext.Response.Redirect(pathBase); | ||
|
||
return; | ||
} | ||
else | ||
{ | ||
httpContext.Response.StatusCode = 503; | ||
httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable; | ||
var stringBuilder = new StringBuilder(); | ||
foreach (var error in setupContext.Errors) | ||
{ | ||
stringBuilder.AppendLine($"{error.Key} : '{error.Value}'"); | ||
} | ||
|
||
await httpContext.Response.WriteAsync($"The AutoSetup failed installing the site '{_setupOptions.SiteName}' with errors: {stringBuilder}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You shouldn't write error messages to the response directly. For instance here the SiteName is user input and could contain XSS. I believe it was only logged before. If we want to keep doing it then we don't need the tuple as a return type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebastienros : Committed new one of shouldn't write error messages to the response directly |
||
return; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
using OrchardCore.AutoSetup; | ||
using OrchardCore.AutoSetup.Options; | ||
using OrchardCore.AutoSetup.Services; | ||
using OrchardCore.Environment.Shell; | ||
using OrchardCore.Environment.Shell.Models; | ||
using OrchardCore.Locking; | ||
using OrchardCore.Locking.Distributed; | ||
using OrchardCore.Setup.Services; | ||
|
||
namespace OrchardCore.Tests.Modules.OrchardCore.AutoSetup; | ||
|
||
public class AutoSetupMiddlewareTests | ||
{ | ||
private readonly Mock<IShellHost> _mockShellHost; | ||
private readonly ShellSettings _shellSettings; | ||
private readonly Mock<IShellSettingsManager> _mockShellSettingsManager; | ||
private readonly Mock<IDistributedLock> _mockDistributedLock; | ||
private readonly Mock<IOptions<AutoSetupOptions>> _mockOptions; | ||
private readonly Mock<IAutoSetupService> _mockAutoSetupService; | ||
|
||
public AutoSetupMiddlewareTests() | ||
{ | ||
_shellSettings = new ShellSettings(); | ||
_shellSettings.AsDefaultShell(); | ||
_mockShellHost = new Mock<IShellHost>(); | ||
_mockShellSettingsManager = new Mock<IShellSettingsManager>(); | ||
_mockDistributedLock = new Mock<IDistributedLock>(); | ||
_mockOptions = new Mock<IOptions<AutoSetupOptions>>(); | ||
_mockAutoSetupService = new Mock<IAutoSetupService>(); | ||
|
||
_mockOptions.Setup(o => o.Value).Returns(new AutoSetupOptions | ||
{ | ||
LockOptions = new LockOptions(), | ||
Tenants = new List<TenantSetupOptions> | ||
{ | ||
new TenantSetupOptions { ShellName = ShellSettings.DefaultShellName } | ||
} | ||
infofromca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
} | ||
|
||
[Fact] | ||
public async Task InvokeAsync_InitializedShell_SkipsSetup() | ||
{ | ||
// Arrange | ||
_shellSettings.State = TenantState.Running; | ||
|
||
var httpContext = new DefaultHttpContext(); | ||
var nextCalled = false; | ||
|
||
infofromca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var middleware = new AutoSetupMiddleware( | ||
next: (innerHttpContext) => { nextCalled = true; return Task.CompletedTask; }, | ||
infofromca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_mockShellHost.Object, | ||
_shellSettings, | ||
_mockShellSettingsManager.Object, | ||
_mockDistributedLock.Object, | ||
_mockOptions.Object); | ||
|
||
// Act | ||
await middleware.InvokeAsync(httpContext); | ||
|
||
// Assert | ||
Assert.True(nextCalled); | ||
_mockAutoSetupService.Verify(s => s.SetupTenantAsync(It.IsAny<TenantSetupOptions>(), It.IsAny<ShellSettings>()), Times.Never); | ||
} | ||
|
||
[Fact] | ||
public async Task InvokeAsync_FailedSetup_ReturnsServiceUnavailable() | ||
{ | ||
// Arrange | ||
_shellSettings.State = TenantState.Uninitialized; | ||
|
||
SetupDistributedLockMock(true); | ||
|
||
var setupContext = new SetupContext { Errors = new Dictionary<string, string> { { "Error", "Test error" } } }; | ||
_mockAutoSetupService.Setup(s => s.SetupTenantAsync(It.IsAny<TenantSetupOptions>(), It.IsAny<ShellSettings>())) | ||
.ReturnsAsync((setupContext, false)); | ||
|
||
var httpContext = new DefaultHttpContext(); | ||
httpContext.RequestServices = new ServiceCollection() | ||
.AddSingleton(_mockAutoSetupService.Object) | ||
.BuildServiceProvider(); | ||
|
||
var middleware = new AutoSetupMiddleware( | ||
next: (innerHttpContext) => Task.CompletedTask, | ||
_mockShellHost.Object, | ||
_shellSettings, | ||
_mockShellSettingsManager.Object, | ||
_mockDistributedLock.Object, | ||
_mockOptions.Object); | ||
|
||
// Act | ||
await middleware.InvokeAsync(httpContext); | ||
|
||
// Assert | ||
Assert.Equal(StatusCodes.Status503ServiceUnavailable, httpContext.Response.StatusCode); | ||
} | ||
|
||
[Fact] | ||
public async Task InvokeAsync_UnInitializedShell_PerformsSetup() | ||
{ | ||
// Arrange | ||
_shellSettings.State = TenantState.Uninitialized; | ||
|
||
SetupDistributedLockMock(true); | ||
|
||
var setupContext = new SetupContext(); | ||
_mockAutoSetupService.Setup(s => s.SetupTenantAsync(It.IsAny<TenantSetupOptions>(), It.IsAny<ShellSettings>())) | ||
.ReturnsAsync((setupContext, true)); | ||
|
||
var httpContext = new DefaultHttpContext(); | ||
httpContext.RequestServices = new ServiceCollection() | ||
.AddSingleton(_mockAutoSetupService.Object) | ||
.BuildServiceProvider(); | ||
|
||
var middleware = new AutoSetupMiddleware( | ||
next: (innerHttpContext) => Task.CompletedTask, | ||
_mockShellHost.Object, | ||
_shellSettings, | ||
_mockShellSettingsManager.Object, | ||
_mockDistributedLock.Object, | ||
_mockOptions.Object); | ||
|
||
// Act | ||
await middleware.InvokeAsync(httpContext); | ||
|
||
// Assert | ||
Assert.Equal(StatusCodes.Status302Found, httpContext.Response.StatusCode); // Redirect | ||
_mockAutoSetupService.Verify(s => s.SetupTenantAsync(It.IsAny<TenantSetupOptions>(), It.IsAny<ShellSettings>()), Times.Once); | ||
} | ||
|
||
[Fact] | ||
public async Task InvokeAsync_FailedLockAcquisition_ThrowsTimeoutException() | ||
{ | ||
// Arrange | ||
_shellSettings.State = TenantState.Uninitialized; | ||
|
||
SetupDistributedLockMock(false); | ||
|
||
var httpContext = new DefaultHttpContext(); | ||
|
||
var middleware = new AutoSetupMiddleware( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can create a |
||
next: (innerHttpContext) => Task.CompletedTask, | ||
_mockShellHost.Object, | ||
_shellSettings, | ||
_mockShellSettingsManager.Object, | ||
_mockDistributedLock.Object, | ||
_mockOptions.Object); | ||
|
||
// Act & Assert | ||
await Assert.ThrowsAsync<TimeoutException>(() => middleware.InvokeAsync(httpContext)); | ||
} | ||
|
||
private void SetupDistributedLockMock(bool acquireLock) | ||
{ | ||
var mockLocker = new Mock<ILocker>(); | ||
_mockDistributedLock | ||
.Setup(d => d.TryAcquireLockAsync(It.IsAny<string>(), It.IsAny<TimeSpan>(), It.IsAny<TimeSpan>())) | ||
.ReturnsAsync((mockLocker.Object, acquireLock)); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would prevent the error message from being displayed to the user. Like server-side validation or database creation error.
Even if you still decide to keep 503 as the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastienros : Just committed
Won't stop other Middlewares after success