From 4a52b85f5a7e18a239a8ff6f80b1703d3b5016b8 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 7 Sep 2020 13:34:43 -0400 Subject: [PATCH] Cleaned up instance header validation --- .../Controllers/AdministrationController.cs | 2 +- .../Controllers/ApiController.cs | 33 +++++-------------- .../Controllers/HomeController.cs | 3 +- .../Controllers/InstanceController.cs | 2 +- .../Controllers/InstanceRequiredController.cs | 8 +++-- .../Controllers/UserController.cs | 4 +-- 6 files changed, 19 insertions(+), 33 deletions(-) diff --git a/src/Tgstation.Server.Host/Controllers/AdministrationController.cs b/src/Tgstation.Server.Host/Controllers/AdministrationController.cs index bb68d12467..c35de94da2 100644 --- a/src/Tgstation.Server.Host/Controllers/AdministrationController.cs +++ b/src/Tgstation.Server.Host/Controllers/AdministrationController.cs @@ -103,7 +103,7 @@ namespace Tgstation.Server.Host.Controllers databaseContext, authenticationContextFactory, logger, - false) + true) { this.gitHubClientFactory = gitHubClientFactory ?? throw new ArgumentNullException(nameof(gitHubClientFactory)); this.serverUpdater = serverUpdater ?? throw new ArgumentNullException(nameof(serverUpdater)); diff --git a/src/Tgstation.Server.Host/Controllers/ApiController.cs b/src/Tgstation.Server.Host/Controllers/ApiController.cs index 6c66ec832e..90a5c5f448 100644 --- a/src/Tgstation.Server.Host/Controllers/ApiController.cs +++ b/src/Tgstation.Server.Host/Controllers/ApiController.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Extensions.Logging; @@ -48,11 +48,6 @@ namespace Tgstation.Server.Host.Controllers /// protected Models.Instance Instance { get; } - /// - /// If permissions are required to access the - /// - readonly bool requireInstance; - /// /// If are required /// @@ -64,14 +59,12 @@ namespace Tgstation.Server.Host.Controllers /// The value of /// The for the /// The value of - /// The value of /// The value of public ApiController( IDatabaseContext databaseContext, IAuthenticationContextFactory authenticationContextFactory, ILogger logger, - bool requireInstance, - bool requireHeaders = true) + bool requireHeaders) { DatabaseContext = databaseContext ?? throw new ArgumentNullException(nameof(databaseContext)); if (authenticationContextFactory == null) @@ -79,7 +72,6 @@ namespace Tgstation.Server.Host.Controllers Logger = logger ?? throw new ArgumentNullException(nameof(logger)); AuthenticationContext = authenticationContextFactory.CurrentAuthenticationContext; Instance = AuthenticationContext?.InstanceUser?.Instance; - this.requireInstance = requireInstance; this.requireHeaders = requireHeaders; } @@ -124,11 +116,11 @@ namespace Tgstation.Server.Host.Controllers protected ObjectResult Created(object payload) => StatusCode((int)HttpStatusCode.Created, payload); /// - /// Performs validation steps for an instance request. + /// Performs validation a request. /// /// The for the operation. /// A resulting in an appropriate on validation failure, otherwise. - protected virtual Task ValidateInstanceRequest(CancellationToken cancellationToken) + protected virtual Task ValidateRequest(CancellationToken cancellationToken) => Task.FromResult(null); /// @@ -193,20 +185,11 @@ namespace Tgstation.Server.Host.Controllers return; } - if (requireInstance) + var errorCase = await ValidateRequest(context.HttpContext.RequestAborted).ConfigureAwait(false); + if (errorCase != null) { - IActionResult errorCase = null; - if (!ApiHeaders.InstanceId.HasValue) - errorCase = BadRequest(new ErrorMessage(ErrorCode.InstanceHeaderRequired)); - else if (AuthenticationContext.InstanceUser == null) - errorCase = Forbid(); - - errorCase ??= await ValidateInstanceRequest(context.HttpContext.RequestAborted).ConfigureAwait(false); - if (errorCase != null) - { - await errorCase.ExecuteResultAsync(context).ConfigureAwait(false); - return; - } + await errorCase.ExecuteResultAsync(context).ConfigureAwait(false); + return; } } catch (HeadersException) diff --git a/src/Tgstation.Server.Host/Controllers/HomeController.cs b/src/Tgstation.Server.Host/Controllers/HomeController.cs index f8c5195413..58b20a3fe4 100644 --- a/src/Tgstation.Server.Host/Controllers/HomeController.cs +++ b/src/Tgstation.Server.Host/Controllers/HomeController.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; @@ -99,7 +99,6 @@ namespace Tgstation.Server.Host.Controllers databaseContext, authenticationContextFactory, logger, - false, (browserResolver ?? throw new ArgumentNullException(nameof(browserResolver))).Browser.Type != BrowserType.Generic && (controlPanelConfigurationOptions?.Value ?? throw new ArgumentNullException(nameof(controlPanelConfigurationOptions))).Enable) { diff --git a/src/Tgstation.Server.Host/Controllers/InstanceController.cs b/src/Tgstation.Server.Host/Controllers/InstanceController.cs index aacf3a0fe6..126458dba5 100644 --- a/src/Tgstation.Server.Host/Controllers/InstanceController.cs +++ b/src/Tgstation.Server.Host/Controllers/InstanceController.cs @@ -90,7 +90,7 @@ namespace Tgstation.Server.Host.Controllers databaseContext, authenticationContextFactory, logger, - false) + true) { this.jobManager = jobManager ?? throw new ArgumentNullException(nameof(jobManager)); this.instanceManager = instanceManager ?? throw new ArgumentNullException(nameof(instanceManager)); diff --git a/src/Tgstation.Server.Host/Controllers/InstanceRequiredController.cs b/src/Tgstation.Server.Host/Controllers/InstanceRequiredController.cs index a2474c0c34..2ec482ef59 100644 --- a/src/Tgstation.Server.Host/Controllers/InstanceRequiredController.cs +++ b/src/Tgstation.Server.Host/Controllers/InstanceRequiredController.cs @@ -37,15 +37,19 @@ namespace Tgstation.Server.Host.Controllers databaseContext, authenticationContextFactory, logger, - true, true) { this.instanceManager = instanceManager ?? throw new ArgumentNullException(nameof(instanceManager)); } /// - protected override async Task ValidateInstanceRequest(CancellationToken cancellationToken) + protected override async Task ValidateRequest(CancellationToken cancellationToken) { + if (!ApiHeaders.InstanceId.HasValue) + return BadRequest(new ErrorMessage(ErrorCode.InstanceHeaderRequired)); + if (AuthenticationContext.InstanceUser == null) + return Forbid(); + if (ValidateInstanceOnlineStatus(instanceManager, Logger, Instance)) await DatabaseContext.Save(cancellationToken).ConfigureAwait(false); diff --git a/src/Tgstation.Server.Host/Controllers/UserController.cs b/src/Tgstation.Server.Host/Controllers/UserController.cs index 5144039371..485216faa4 100644 --- a/src/Tgstation.Server.Host/Controllers/UserController.cs +++ b/src/Tgstation.Server.Host/Controllers/UserController.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -58,7 +58,7 @@ namespace Tgstation.Server.Host.Controllers databaseContext, authenticationContextFactory, logger, - false) + true) { this.systemIdentityFactory = systemIdentityFactory ?? throw new ArgumentNullException(nameof(systemIdentityFactory)); this.cryptographySuite = cryptographySuite ?? throw new ArgumentNullException(nameof(cryptographySuite));