diff --git a/src/Tgstation.Server.Api/Hubs/ConnectionAbortReason.cs b/src/Tgstation.Server.Api/Hubs/ConnectionAbortReason.cs deleted file mode 100644 index 05b1cac9b4..0000000000 --- a/src/Tgstation.Server.Api/Hubs/ConnectionAbortReason.cs +++ /dev/null @@ -1,18 +0,0 @@ -namespace Tgstation.Server.Api.Hubs -{ - /// - /// The reason an aborts a connection. - /// - public enum ConnectionAbortReason - { - /// - /// The provided token is no longer authenticated or authorized to keep the connection. - /// - TokenInvalid, - - /// - /// The server is restarting. - /// - ServerRestart, - } -} diff --git a/src/Tgstation.Server.Api/Hubs/IErrorHandlingHub.cs b/src/Tgstation.Server.Api/Hubs/IErrorHandlingHub.cs deleted file mode 100644 index 844c621d4f..0000000000 --- a/src/Tgstation.Server.Api/Hubs/IErrorHandlingHub.cs +++ /dev/null @@ -1,19 +0,0 @@ -using System.Threading; -using System.Threading.Tasks; - -namespace Tgstation.Server.Api.Hubs -{ - /// - /// Hub for handling communication errors. - /// - public interface IErrorHandlingHub - { - /// - /// Called if a hub connection or call is attempted with an invalid or unauthorized token. After calling this, the connection is aborted. - /// - /// The . - /// The for the operation. - /// A representing the running operation. - Task AbortingConnection(ConnectionAbortReason reason, CancellationToken cancellationToken); - } -} diff --git a/src/Tgstation.Server.Api/Hubs/IJobsHub.cs b/src/Tgstation.Server.Api/Hubs/IJobsHub.cs index 01b4aec8bd..4c595362cb 100644 --- a/src/Tgstation.Server.Api/Hubs/IJobsHub.cs +++ b/src/Tgstation.Server.Api/Hubs/IJobsHub.cs @@ -8,7 +8,7 @@ namespace Tgstation.Server.Api.Hubs /// /// SignalR client methods for receiving s. /// - public interface IJobsHub : IErrorHandlingHub + public interface IJobsHub { /// /// Push a update to the client. diff --git a/src/Tgstation.Server.Host/Extensions/ServiceCollectionExtensions.cs b/src/Tgstation.Server.Host/Extensions/ServiceCollectionExtensions.cs index 6693ca670e..c90433f1ff 100644 --- a/src/Tgstation.Server.Host/Extensions/ServiceCollectionExtensions.cs +++ b/src/Tgstation.Server.Host/Extensions/ServiceCollectionExtensions.cs @@ -11,7 +11,6 @@ using Serilog; using Serilog.Configuration; using Serilog.Sinks.Elasticsearch; -using Tgstation.Server.Api.Hubs; using Tgstation.Server.Host.Components.Chat.Providers; using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.IO; @@ -230,7 +229,7 @@ namespace Tgstation.Server.Host.Extensions /// The to add the to. public static void AddHub(this IServiceCollection services) where THub : ConnectionMappingHub - where THubMethods : class, IErrorHandlingHub + where THubMethods : class { ArgumentNullException.ThrowIfNull(services); diff --git a/src/Tgstation.Server.Host/Jobs/JobsHubGroupMapper.cs b/src/Tgstation.Server.Host/Jobs/JobsHubGroupMapper.cs index a05d3283b5..4f46c90014 100644 --- a/src/Tgstation.Server.Host/Jobs/JobsHubGroupMapper.cs +++ b/src/Tgstation.Server.Host/Jobs/JobsHubGroupMapper.cs @@ -71,7 +71,8 @@ namespace Tgstation.Server.Host.Jobs throw new InvalidOperationException("user.Id was null!"); logger.LogTrace("UserDisabled"); - return hub.NotifyAndAbortUnauthedConnections(user, cancellationToken); + hub.AbortUnauthedConnections(user); + return ValueTask.CompletedTask; } /// diff --git a/src/Tgstation.Server.Host/Security/AuthorizationContextHubFilter.cs b/src/Tgstation.Server.Host/Security/AuthorizationContextHubFilter.cs index 7820d66494..38360cab9b 100644 --- a/src/Tgstation.Server.Host/Security/AuthorizationContextHubFilter.cs +++ b/src/Tgstation.Server.Host/Security/AuthorizationContextHubFilter.cs @@ -5,8 +5,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Logging; -using Tgstation.Server.Api.Hubs; - namespace Tgstation.Server.Host.Security { /// @@ -41,7 +39,7 @@ namespace Tgstation.Server.Host.Security public async Task OnConnectedAsync(HubLifetimeContext context, Func next) { ArgumentNullException.ThrowIfNull(context); - if (await ValidateAuthenticationContext(context.Hub)) + if (ValidateAuthenticationContext(context.Hub)) await next(context); } @@ -49,7 +47,7 @@ namespace Tgstation.Server.Host.Security public async ValueTask InvokeMethodAsync(HubInvocationContext invocationContext, Func> next) { ArgumentNullException.ThrowIfNull(invocationContext); - if (await ValidateAuthenticationContext(invocationContext.Hub)) + if (ValidateAuthenticationContext(invocationContext.Hub)) return await next(invocationContext); return null; @@ -60,7 +58,7 @@ namespace Tgstation.Server.Host.Security /// /// The current . /// if the hub call should continue, if it shouldn't and has been aborted. - async ValueTask ValidateAuthenticationContext(Hub hub) + bool ValidateAuthenticationContext(Hub hub) { if (!authenticationContext.Valid) logger.LogTrace("The token for connection {connectionId} is no longer authenticated! Aborting...", hub.Context.ConnectionId); @@ -78,10 +76,6 @@ namespace Tgstation.Server.Host.Security var callerProperty = clients.GetType().GetProperty(nameof(hub.Clients.Caller)); var caller = callerProperty.GetValue(clients); - if (caller is not IErrorHandlingHub specifiedHub) - throw new InvalidOperationException("This filter only supports IErrorHandlingHubs"); - - await specifiedHub.AbortingConnection(ConnectionAbortReason.TokenInvalid, hub.Context.ConnectionAborted); hub.Context.Abort(); return false; } diff --git a/src/Tgstation.Server.Host/Security/README.md b/src/Tgstation.Server.Host/Security/README.md index da4d16464e..104db33ea4 100644 --- a/src/Tgstation.Server.Host/Security/README.md +++ b/src/Tgstation.Server.Host/Security/README.md @@ -67,7 +67,7 @@ 1. It checks the validity of the scope's [IAuthenticationContext](./IAuthenticationContext.cs). If it is invalid (indicating the user is not authorized either due to not existing (Only possible with a forged and signed JWT) or if their token was outdated compared to the last time their password or `Enabled` status was updated), HTTP 401 will be returned. 1. It checks the user's `Enabled` status. If the user is disabled, HTTP 403 will be returned. - For SignalR hub requests, this is the [AuthorizationContextHubFilter](./AuthorizationContextHubFilter.cs). - - If either [IAuthenticationContext](./IAuthenticationContext.cs) is either invalid OR unauthorized, it invokes `IErrorHandlingHub.AbortingConnection` with `ConnectionAbortReason.TokenInvalid` on the client before aborting the connection. + - If either [IAuthenticationContext](./IAuthenticationContext.cs) is either invalid OR unauthorized, it unceremoniously aborts the connection. 1. The `ApiController` base class inspects the request. 1. If the `ApiHeaders` could not be properly parsed, HTTP 400 (or 406 if the `Accept` header was bad) with an `ErrorMessageResponse` is returned. 1. If the request is to an Instance component path: diff --git a/src/Tgstation.Server.Host/Utils/SignalR/ComprehensiveHubContext.cs b/src/Tgstation.Server.Host/Utils/SignalR/ComprehensiveHubContext.cs index 411c424f08..9e2b053a65 100644 --- a/src/Tgstation.Server.Host/Utils/SignalR/ComprehensiveHubContext.cs +++ b/src/Tgstation.Server.Host/Utils/SignalR/ComprehensiveHubContext.cs @@ -8,9 +8,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Logging; -using Tgstation.Server.Api.Hubs; -using Tgstation.Server.Common.Extensions; -using Tgstation.Server.Host.Core; using Tgstation.Server.Host.Models; using Tgstation.Server.Host.Security; @@ -20,10 +17,10 @@ namespace Tgstation.Server.Host.Utils.SignalR /// An implementation of with connection ID mapping. /// /// The the is for. - /// The interface for implementing methods. - sealed class ComprehensiveHubContext : IConnectionMappedHubContext, IHubConnectionMapper, IRestartHandler + /// The for implementing methods. + sealed class ComprehensiveHubContext : IConnectionMappedHubContext, IHubConnectionMapper where THub : ConnectionMappingHub - where THubMethods : class, IErrorHandlingHub + where THubMethods : class { /// public IHubClients Clients => wrappedHubContext.Clients; @@ -53,20 +50,15 @@ namespace Tgstation.Server.Host.Utils.SignalR /// Initializes a new instance of the class. /// /// The value of . - /// The to with. /// The value of . public ComprehensiveHubContext( IHubContext wrappedHubContext, - IServerControl serverControl, ILogger> logger) { this.wrappedHubContext = wrappedHubContext ?? throw new ArgumentNullException(nameof(wrappedHubContext)); - ArgumentNullException.ThrowIfNull(serverControl); this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); userConnections = new ConcurrentDictionary>(); - - serverControl.RegisterForRestart(this); } /// @@ -123,7 +115,7 @@ namespace Tgstation.Server.Host.Utils.SignalR } /// - public ValueTask NotifyAndAbortUnauthedConnections(User user, CancellationToken cancellationToken) + public void AbortUnauthedConnections(User user) { ArgumentNullException.ThrowIfNull(user); logger.LogTrace("NotifyAndAbortUnauthedConnections. UID {userId}", user.Id.Value); @@ -143,23 +135,8 @@ namespace Tgstation.Server.Host.Utils.SignalR return old; }); - async ValueTask NotifyAndAbortConnection(HubCallerContext context) - { - await Clients - .Client(context.ConnectionId) - .AbortingConnection(ConnectionAbortReason.TokenInvalid, cancellationToken); + foreach (var context in connections) context.Abort(); - } - - return ValueTaskExtensions.WhenAll(connections.Select(NotifyAndAbortConnection)); - } - - /// - public async ValueTask HandleRestart(Version updateVersion, bool handlerMayDelayShutdownWithExtremelyLongRunningTasks, CancellationToken cancellationToken) - { - logger.LogTrace("HandleRestart. {connectionCount} active connections", userConnections.Count); - await Clients.All.AbortingConnection(ConnectionAbortReason.ServerRestart, cancellationToken); - userConnections.Clear(); } } } diff --git a/src/Tgstation.Server.Host/Utils/SignalR/ConnectionMappingHub.cs b/src/Tgstation.Server.Host/Utils/SignalR/ConnectionMappingHub.cs index 5f81010e8b..05e2eb8742 100644 --- a/src/Tgstation.Server.Host/Utils/SignalR/ConnectionMappingHub.cs +++ b/src/Tgstation.Server.Host/Utils/SignalR/ConnectionMappingHub.cs @@ -4,7 +4,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.SignalR; -using Tgstation.Server.Api.Hubs; using Tgstation.Server.Host.Security; namespace Tgstation.Server.Host.Utils.SignalR @@ -13,11 +12,11 @@ namespace Tgstation.Server.Host.Utils.SignalR /// Base for s that want to map their connection IDs to s. /// /// The child inheriting from the . - /// The interface for implementing methods. + /// The for implementing methods. [TgsAuthorize] abstract class ConnectionMappingHub : Hub where TChildHub : ConnectionMappingHub - where THubMethods : class, IErrorHandlingHub + where THubMethods : class { /// /// The used to map connections. diff --git a/src/Tgstation.Server.Host/Utils/SignalR/IConnectionMappedHubContext.cs b/src/Tgstation.Server.Host/Utils/SignalR/IConnectionMappedHubContext.cs index d44ad96108..3877881702 100644 --- a/src/Tgstation.Server.Host/Utils/SignalR/IConnectionMappedHubContext.cs +++ b/src/Tgstation.Server.Host/Utils/SignalR/IConnectionMappedHubContext.cs @@ -5,7 +5,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR; -using Tgstation.Server.Api.Hubs; using Tgstation.Server.Host.Models; using Tgstation.Server.Host.Security; @@ -18,7 +17,7 @@ namespace Tgstation.Server.Host.Utils.SignalR /// The interface for implementing methods. interface IConnectionMappedHubContext : IHubContext where THub : Hub - where THubMethods : class, IErrorHandlingHub + where THubMethods : class { /// /// Called when a user connects. Should return an of hub group names the given belongs in. @@ -33,11 +32,9 @@ namespace Tgstation.Server.Host.Utils.SignalR List UserConnectionIds(User user); /// - /// Calls with on and aborts the connections associated with the given . + /// Aborts the connections associated with the given . /// /// The to abort the connections of. - /// The for the operation. - /// A representing the running operation. - ValueTask NotifyAndAbortUnauthedConnections(User user, CancellationToken cancellationToken); + void AbortUnauthedConnections(User user); } } diff --git a/src/Tgstation.Server.Host/Utils/SignalR/IHubConnectionMapper.cs b/src/Tgstation.Server.Host/Utils/SignalR/IHubConnectionMapper.cs index f941ac28fe..4521c87fcc 100644 --- a/src/Tgstation.Server.Host/Utils/SignalR/IHubConnectionMapper.cs +++ b/src/Tgstation.Server.Host/Utils/SignalR/IHubConnectionMapper.cs @@ -3,7 +3,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR; -using Tgstation.Server.Api.Hubs; using Tgstation.Server.Host.Models; using Tgstation.Server.Host.Security; @@ -13,10 +12,10 @@ namespace Tgstation.Server.Host.Utils.SignalR /// Handles mapping connection IDs to s for a given . /// /// The whose connections are being mapped. - /// The interface for implementing methods. + /// The for implementing methods. interface IHubConnectionMapper where THub : ConnectionMappingHub - where THubMethods : class, IErrorHandlingHub + where THubMethods : class { /// /// To be called when a hub connection is made. diff --git a/tests/Tgstation.Server.Tests/Live/Instance/JobsHubTests.cs b/tests/Tgstation.Server.Tests/Live/Instance/JobsHubTests.cs index 228d9296ce..bf683503c1 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/JobsHubTests.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/JobsHubTests.cs @@ -19,8 +19,6 @@ namespace Tgstation.Server.Tests.Live.Instance { sealed class JobsHubTests : IJobsHub { - const int ActiveConnections = 2; - readonly IServerClient permedUser; readonly IServerClient permlessUser; @@ -31,7 +29,6 @@ namespace Tgstation.Server.Tests.Live.Instance readonly HashSet permlessSeenJobs; HubConnection conn1, conn2; - int expectedReboots; bool permlessIsPermed; long? permlessPsId; @@ -78,10 +75,6 @@ namespace Tgstation.Server.Tests.Live.Instance class ShouldNeverReceiveUpdates : IJobsHub { public Action Callback { get; set; } - public Func Error { get; set; } - - public Task AbortingConnection(ConnectionAbortReason reason, CancellationToken cancellationToken) - => Error(reason, cancellationToken); public Task ReceiveJobUpdate(JobResponse job, CancellationToken cancellationToken) { @@ -102,7 +95,6 @@ namespace Tgstation.Server.Tests.Live.Instance lock (permlessSeenJobs) permlessSeenJobs.Add(job.Id.Value); }, - Error = AbortingConnection, }; await using (conn1 = (HubConnection)await permedUser.SubscribeToJobUpdates( @@ -208,19 +200,16 @@ namespace Tgstation.Server.Tests.Live.Instance Assert.AreEqual(HubConnectionState.Connected, conn3.State); await permlessUser.DisposeAsync(); await permedUser.DisposeAsync(); - Assert.AreEqual(0, expectedReboots); } public void ExpectShutdown() { - Assert.AreEqual(0, Interlocked.Exchange(ref expectedReboots, ActiveConnections)); Assert.AreEqual(HubConnectionState.Connected, conn1.State); Assert.AreEqual(HubConnectionState.Connected, conn2.State); } public async ValueTask WaitForReconnect(CancellationToken cancellationToken) { - Assert.AreEqual(0, expectedReboots); await Task.WhenAll(conn1.StopAsync(cancellationToken), conn2.StopAsync(cancellationToken)); Assert.AreEqual(HubConnectionState.Disconnected, conn1.State); @@ -270,20 +259,5 @@ namespace Tgstation.Server.Tests.Live.Instance } public void CompleteNow() => finishTcs.TrySetResult(); - - public Task AbortingConnection(ConnectionAbortReason reason, CancellationToken cancellationToken) - { - try - { - Assert.AreEqual(ConnectionAbortReason.ServerRestart, reason); - var remaining = Interlocked.Decrement(ref expectedReboots); - Assert.IsTrue(remaining >= 0); - } - catch (Exception ex) - { - finishTcs.TrySetException(ex); - } - return Task.CompletedTask; - } } } diff --git a/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs b/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs index bb028397b4..7e4a101001 100644 --- a/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs +++ b/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs @@ -359,10 +359,6 @@ namespace Tgstation.Server.Tests.Live class FuncProxiedJobsHub : IJobsHub { public Func ProxyFunc { get; set; } - public Func ErrorFunc { get; set; } - - public Task AbortingConnection(ConnectionAbortReason reason, CancellationToken cancellationToken) - => ErrorFunc(reason); public Task ReceiveJobUpdate(JobResponse job, CancellationToken cancellationToken) => ProxyFunc(job, cancellationToken); @@ -402,13 +398,6 @@ namespace Tgstation.Server.Tests.Live }); var proxy = new FuncProxiedJobsHub(); - var errorTcs = new TaskCompletionSource(); - proxy.ErrorFunc = reason => - { - errorTcs.SetException(new Exception($"Aborted: {reason}")); - return Task.CompletedTask; - }; - HubConnection hubConnection; HardFailLoggerProvider.BlockFails = true; try @@ -431,8 +420,6 @@ namespace Tgstation.Server.Tests.Live Assert.AreEqual(HubConnectionState.Disconnected, hubConnection.State); - Assert.IsFalse(errorTcs.Task.IsCompleted); - var createRequest = new UserCreateRequest { Enabled = true, @@ -441,33 +428,27 @@ namespace Tgstation.Server.Tests.Live }; var testUser = await serverClient.Users.Create(createRequest, cancellationToken); - await using (var testUserClient = await serverClientFactory.CreateFromLogin(serverClient.Url, createRequest.Name, createRequest.Password, cancellationToken: cancellationToken)) + await using var testUserClient = await serverClientFactory.CreateFromLogin(serverClient.Url, createRequest.Name, createRequest.Password, cancellationToken: cancellationToken); + await using var testUserConn1 = (HubConnection)await testUserClient.SubscribeToJobUpdates(proxy, cancellationToken: cancellationToken); + + await serverClient.Users.Update(new UserUpdateRequest { - errorTcs = new TaskCompletionSource(); - await using var testUserConn1 = await testUserClient.SubscribeToJobUpdates(proxy, cancellationToken: cancellationToken); + Id = testUser.Id, + Enabled = false, + }, cancellationToken); - Assert.IsFalse(errorTcs.Task.IsCompleted); + // need a second here + for (var i = 0; i < 10 && testUserConn1.State == HubConnectionState.Connected; ++i) + await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); - await serverClient.Users.Update(new UserUpdateRequest - { - Id = testUser.Id, - Enabled = false, - }, cancellationToken); + Assert.AreNotEqual(HubConnectionState.Connected, testUserConn1.State); - // need a second here - for (var i = 0; i < 10 && !errorTcs.Task.IsCompleted; ++i) - await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); + await using var testUserConn2 = (HubConnection)await testUserClient.SubscribeToJobUpdates(proxy, cancellationToken: cancellationToken); - Assert.IsTrue(errorTcs.Task.IsCompleted); + for (var i = 0; i < 10 && testUserConn2.State == HubConnectionState.Connected; ++i) + await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); - errorTcs = new TaskCompletionSource(); - await using var testUserConn2 = await testUserClient.SubscribeToJobUpdates(proxy, cancellationToken: cancellationToken); - for (var i = 0; i < 10 && !errorTcs.Task.IsCompleted; ++i) - await Task.Delay(TimeSpan.FromSeconds(1), cancellationToken); - } - - Assert.IsTrue(errorTcs.Task.IsCompleted); - await Assert.ThrowsExceptionAsync(() => errorTcs.Task); + Assert.AreNotEqual(HubConnectionState.Connected, testUserConn2.State); } finally {