From 4286b16d3e8d8fbf74c13839662e5dfc16f235c8 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Sun, 19 Apr 2020 21:59:49 -0400 Subject: [PATCH] Fix some multi-query issues. Remove unused dependencies --- .../Controllers/InstanceController.cs | 51 +++++--- src/Tgstation.Server.Host/Core/Application.cs | 11 ++ .../Security/AuthenticationContextFactory.cs | 30 ++--- .../Tgstation.Server.Host.csproj | 1 + .../Tgstation.Server.Tests/IntegrationTest.cs | 121 +++++++++--------- 5 files changed, 116 insertions(+), 98 deletions(-) diff --git a/src/Tgstation.Server.Host/Controllers/InstanceController.cs b/src/Tgstation.Server.Host/Controllers/InstanceController.cs index dee8ef6ce5..93cb6e514e 100644 --- a/src/Tgstation.Server.Host/Controllers/InstanceController.cs +++ b/src/Tgstation.Server.Host/Controllers/InstanceController.cs @@ -174,6 +174,8 @@ namespace Tgstation.Server.Host.Controllers // Validate it's not a child of any other instance using (var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken)) + { + var newCancellationToken = cts.Token; try { await DatabaseContext.Instances.ForEachAsync( @@ -184,16 +186,17 @@ namespace Tgstation.Server.Host.Controllers else earlyOut = CheckInstanceNotChildOf(otherInstance.Path); - if (earlyOut != null) + if (earlyOut != null && !newCancellationToken.IsCancellationRequested) cts.Cancel(); }, - cts.Token) + newCancellationToken) .ConfigureAwait(false); } catch (OperationCanceledException) { cancellationToken.ThrowIfCancellationRequested(); } + } if (earlyOut != null) return earlyOut; @@ -353,9 +356,9 @@ namespace Tgstation.Server.Host.Controllers if (model == null) throw new ArgumentNullException(nameof(model)); - var instanceQuery = DatabaseContext.Instances.Where(x => x.Id == model.Id); + IQueryable InstanceQuery() => DatabaseContext.Instances.Where(x => x.Id == model.Id); - var moveJob = await instanceQuery + var moveJob = await InstanceQuery() .SelectMany(x => x.Jobs). #pragma warning disable CA1307 // Specify StringComparison Where(x => !x.StoppedAt.HasValue && x.Description.StartsWith(MoveInstanceJobPrefix)) @@ -368,9 +371,7 @@ namespace Tgstation.Server.Host.Controllers if (moveJob != default) await jobManager.CancelJob(moveJob, AuthenticationContext.User, true, cancellationToken).ConfigureAwait(false); // cancel it now - var usersInstanceUserTask = instanceQuery.SelectMany(x => x.InstanceUsers).Where(x => x.UserId == AuthenticationContext.User.Id).FirstOrDefaultAsync(cancellationToken); - - var originalModel = await instanceQuery + var originalModel = await InstanceQuery() .Include(x => x.RepositorySettings) .Include(x => x.ChatSettings) .ThenInclude(x => x.Channels) @@ -428,7 +429,11 @@ namespace Tgstation.Server.Host.Controllers return Forbid(); // ensure the current user has write privilege on the instance - var usersInstanceUser = await usersInstanceUserTask.ConfigureAwait(false); + var usersInstanceUser = await InstanceQuery() + .SelectMany(x => x.InstanceUsers) + .Where(x => x.UserId == AuthenticationContext.User.Id) + .FirstOrDefaultAsync(cancellationToken) + .ConfigureAwait(false); if (usersInstanceUser == default) { var instanceAdminUser = InstanceAdminUser(); @@ -541,20 +546,17 @@ namespace Tgstation.Server.Host.Controllers [ProducesResponseType(410)] public async Task GetId(long id, CancellationToken cancellationToken) { - var query = DatabaseContext.Instances.Where(x => x.Id == id); var cantList = !AuthenticationContext.User.InstanceManagerRights.Value.HasFlag(InstanceManagerRights.List); + IQueryable QueryForUser() + { + var query = DatabaseContext.Instances.Where(x => x.Id == id); - if (cantList) - query = query.Include(x => x.InstanceUsers); + if (cantList) + query = query.Include(x => x.InstanceUsers); + return query; + } - var moveJobTask = query - .SelectMany(x => x.Jobs) -#pragma warning disable CA1307 // Specify StringComparison - .Where(x => !x.StoppedAt.HasValue && x.Description.StartsWith(MoveInstanceJobPrefix)) -#pragma warning restore CA1307 // Specify StringComparison - .Include(x => x.StartedBy).ThenInclude(x => x.CreatedBy) - .FirstOrDefaultAsync(cancellationToken); - var instance = await query.FirstOrDefaultAsync(cancellationToken).ConfigureAwait(false); + var instance = await QueryForUser().FirstOrDefaultAsync(cancellationToken).ConfigureAwait(false); if (instance == null) return StatusCode((int)HttpStatusCode.Gone); @@ -563,7 +565,16 @@ namespace Tgstation.Server.Host.Controllers return Forbid(); var api = instance.ToApi(); - api.MoveJob = (await moveJobTask.ConfigureAwait(false))?.ToApi(); + + var moveJob = await QueryForUser() + .SelectMany(x => x.Jobs) +#pragma warning disable CA1307 // Specify StringComparison + .Where(x => !x.StoppedAt.HasValue && x.Description.StartsWith(MoveInstanceJobPrefix)) +#pragma warning restore CA1307 // Specify StringComparison + .Include(x => x.StartedBy).ThenInclude(x => x.CreatedBy) + .FirstOrDefaultAsync(cancellationToken) + .ConfigureAwait(false); + api.MoveJob = moveJob?.ToApi(); return Json(api); } } diff --git a/src/Tgstation.Server.Host/Core/Application.cs b/src/Tgstation.Server.Host/Core/Application.cs index 8233f77fd5..0a4caca211 100644 --- a/src/Tgstation.Server.Host/Core/Application.cs +++ b/src/Tgstation.Server.Host/Core/Application.cs @@ -1,4 +1,5 @@ using Byond.TopicSender; +using Cyberboss.AspNetCore.AsyncInitializer; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Cors.Infrastructure; @@ -18,6 +19,7 @@ using Serilog.Formatting.Display; using System; using System.Globalization; using System.IdentityModel.Tokens.Jwt; +using System.Threading.Tasks; using Tgstation.Server.Api; using Tgstation.Server.Api.Models; using Tgstation.Server.Host.Components; @@ -405,6 +407,15 @@ namespace Tgstation.Server.Host.Core // Final point where we wrap exceptions in a 500 (ErrorMessage) response applicationBuilder.UseServerErrorHandling(); + // 503 requests made while the application is starting + applicationBuilder.UseAsyncInitialization(async (applicationLifetime, cancellationToken) => + { + var tcs = new TaskCompletionSource(); + using (cancellationToken.Register(() => tcs.SetCanceled())) + using (applicationLifetime.ApplicationStarted.Register(() => tcs.SetResult(null))) + await tcs.Task.ConfigureAwait(false); + }); + // should anything after this throw an exception, catch it and display a detailed html page if (hostingEnvironment.IsDevelopment()) applicationBuilder.UseDeveloperExceptionPage(); // it is not worth it to limit this, you should only ever get it if you're an authorized user diff --git a/src/Tgstation.Server.Host/Security/AuthenticationContextFactory.cs b/src/Tgstation.Server.Host/Security/AuthenticationContextFactory.cs index 948a92fa55..af26926fd2 100644 --- a/src/Tgstation.Server.Host/Security/AuthenticationContextFactory.cs +++ b/src/Tgstation.Server.Host/Security/AuthenticationContextFactory.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; using Tgstation.Server.Host.Database; -using Tgstation.Server.Host.Models; namespace Tgstation.Server.Host.Security { @@ -14,11 +13,6 @@ namespace Tgstation.Server.Host.Security /// public IAuthenticationContext CurrentAuthenticationContext { get; private set; } - /// - /// The for the - /// - readonly ISystemIdentityFactory systemIdentityFactory; - /// /// The for the /// @@ -32,12 +26,10 @@ namespace Tgstation.Server.Host.Security /// /// Construct an /// - /// The value of /// The value of /// The value of - public AuthenticationContextFactory(ISystemIdentityFactory systemIdentityFactory, IDatabaseContext databaseContext, IIdentityCache identityCache) + public AuthenticationContextFactory(IDatabaseContext databaseContext, IIdentityCache identityCache) { - this.systemIdentityFactory = systemIdentityFactory ?? throw new ArgumentNullException(nameof(systemIdentityFactory)); this.databaseContext = databaseContext ?? throw new ArgumentNullException(nameof(databaseContext)); this.identityCache = identityCache ?? throw new ArgumentNullException(nameof(identityCache)); } @@ -51,16 +43,10 @@ namespace Tgstation.Server.Host.Security if (CurrentAuthenticationContext != null) throw new InvalidOperationException("Authentication context has already been loaded"); - var userQuery = databaseContext.Users.Where(x => x.Id == userId) + var user = await databaseContext.Users.Where(x => x.Id == userId) .Include(x => x.CreatedBy) - .FirstOrDefaultAsync(cancellationToken); - - var instanceUserQuery = instanceId.HasValue ? databaseContext.InstanceUsers - .Where(x => x.UserId == userId && x.InstanceId == instanceId && x.Instance.Online.Value) - .Include(x => x.Instance) - .FirstOrDefaultAsync(cancellationToken) : Task.FromResult(null); - - var user = await userQuery.ConfigureAwait(false); + .FirstOrDefaultAsync(cancellationToken) + .ConfigureAwait(false); if (user == default) { CurrentAuthenticationContext = new AuthenticationContext(); @@ -83,7 +69,13 @@ namespace Tgstation.Server.Host.Security try { - var instanceUser = await instanceUserQuery.ConfigureAwait(false); + var instanceUser = instanceId.HasValue + ? await databaseContext.InstanceUsers + .Where(x => x.UserId == userId && x.InstanceId == instanceId && x.Instance.Online.Value) + .Include(x => x.Instance) + .FirstOrDefaultAsync(cancellationToken) + .ConfigureAwait(false) + : null; CurrentAuthenticationContext = new AuthenticationContext(systemIdentity, user, instanceUser); } diff --git a/src/Tgstation.Server.Host/Tgstation.Server.Host.csproj b/src/Tgstation.Server.Host/Tgstation.Server.Host.csproj index a006f7b139..d69f390ec4 100644 --- a/src/Tgstation.Server.Host/Tgstation.Server.Host.csproj +++ b/src/Tgstation.Server.Host/Tgstation.Server.Host.csproj @@ -47,6 +47,7 @@ + diff --git a/tests/Tgstation.Server.Tests/IntegrationTest.cs b/tests/Tgstation.Server.Tests/IntegrationTest.cs index e793f41702..8ff5a82d0f 100644 --- a/tests/Tgstation.Server.Tests/IntegrationTest.cs +++ b/tests/Tgstation.Server.Tests/IntegrationTest.cs @@ -165,70 +165,73 @@ namespace Tgstation.Server.Tests [TestMethod] public async Task TestStandardOperation() { - RequireDiscordToken(); - var server = new TestingServer(clientFactory, null); - using (var serverCts = new CancellationTokenSource()) + while (true) { - var cancellationToken = serverCts.Token; - var serverTask = server.RunAsync(cancellationToken); - try + RequireDiscordToken(); + var server = new TestingServer(clientFactory, null); + using (var serverCts = new CancellationTokenSource()) { - IServerClient adminClient; - - var giveUpAt = DateTimeOffset.Now.AddSeconds(60); - do - { - try - { - adminClient = await clientFactory.CreateServerClient(server.Url, User.AdminName, User.DefaultAdminPassword).ConfigureAwait(false); - break; - } - catch (HttpRequestException) - { - //migrating, to be expected - if (DateTimeOffset.Now > giveUpAt) - throw; - await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); - } - catch (ServiceUnavailableException) - { - // migrating, to be expected - if (DateTimeOffset.Now > giveUpAt) - throw; - await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); - } - } while (true); - - using (adminClient) - { - var serverInfo = await adminClient.Version(default).ConfigureAwait(false); - - Assert.AreEqual(ApiHeaders.Version, serverInfo.ApiVersion); - Assert.AreEqual(typeof(IServer).Assembly.GetName().Version, serverInfo.Version); - - //check that modifying the token even slightly fucks up the auth - var newToken = new Token - { - ExpiresAt = adminClient.Token.ExpiresAt, - Bearer = adminClient.Token.Bearer + '0' - }; - - var badClient = clientFactory.CreateServerClient(server.Url, newToken); - await Assert.ThrowsExceptionAsync(() => badClient.Version(cancellationToken)).ConfigureAwait(false); - - await new AdministrationTest(adminClient.Administration).Run(cancellationToken).ConfigureAwait(false); - await new UsersTest(adminClient.Users).Run(cancellationToken).ConfigureAwait(false); - await new InstanceManagerTest(adminClient.Instances, server.Directory).Run(cancellationToken).ConfigureAwait(false); - } - } - finally - { - serverCts.Cancel(); + var cancellationToken = serverCts.Token; + var serverTask = server.RunAsync(cancellationToken); try { - await serverTask.ConfigureAwait(false); + IServerClient adminClient; + + var giveUpAt = DateTimeOffset.Now.AddSeconds(60); + do + { + try + { + adminClient = await clientFactory.CreateServerClient(server.Url, User.AdminName, User.DefaultAdminPassword).ConfigureAwait(false); + break; + } + catch (HttpRequestException) + { + //migrating, to be expected + if (DateTimeOffset.Now > giveUpAt) + throw; + await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); + } + catch (ServiceUnavailableException) + { + // migrating, to be expected + if (DateTimeOffset.Now > giveUpAt) + throw; + await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); + } + } while (true); + + using (adminClient) + { + var serverInfo = await adminClient.Version(default).ConfigureAwait(false); + + Assert.AreEqual(ApiHeaders.Version, serverInfo.ApiVersion); + Assert.AreEqual(typeof(IServer).Assembly.GetName().Version, serverInfo.Version); + + //check that modifying the token even slightly fucks up the auth + var newToken = new Token + { + ExpiresAt = adminClient.Token.ExpiresAt, + Bearer = adminClient.Token.Bearer + '0' + }; + + var badClient = clientFactory.CreateServerClient(server.Url, newToken); + await Assert.ThrowsExceptionAsync(() => badClient.Version(cancellationToken)).ConfigureAwait(false); + + await new AdministrationTest(adminClient.Administration).Run(cancellationToken).ConfigureAwait(false); + await new UsersTest(adminClient.Users).Run(cancellationToken).ConfigureAwait(false); + await new InstanceManagerTest(adminClient.Instances, server.Directory).Run(cancellationToken).ConfigureAwait(false); + } + } + finally + { + serverCts.Cancel(); + try + { + await serverTask.ConfigureAwait(false); + } + catch (OperationCanceledException) { } } - catch (OperationCanceledException) { } } } }