cyberpresidentvanellope/tgstation-server
1
0
Fork 0
mirror of https://github.com/tgstation/tgstation-server.git synced 2026-08-30 16:39:21 +01:00
Code Issues Packages Projects Releases Wiki Activity

Remove hub abort notifications

Browse Source 
Because SignalR buffers messages, we can't guarantee these will be delivered before the connection is aborted.

We'll have to rely on the client not being pants-on-head.
This commit is contained in:
Jordan Dominion
2023-11-05 09:38:57 -05:00
parent d3563394fb
commit ed8453f08e
13 changed files with 35 additions and 151 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Tgstation.Server.Api/Hubs/ConnectionAbortReason.cs
-18
View File
@@ -1,18 +0,0 @@
namespace Tgstation.Server.Api.Hubs
{
/// <summary>
/// The reason an <see cref="IErrorHandlingHub"/> aborts a connection.
/// </summary>
public enum ConnectionAbortReason
{
/// <summary>
/// The provided token is no longer authenticated or authorized to keep the connection.
/// </summary>
TokenInvalid,
/// <summary>
/// The server is restarting.
/// </summary>
ServerRestart,
}
}
src/Tgstation.Server.Api/Hubs/IErrorHandlingHub.cs
-19
View File
@@ -1,19 +0,0 @@
using System.Threading;
using System.Threading.Tasks;
namespace Tgstation.Server.Api.Hubs
{
/// <summary>
/// Hub for handling communication errors.
/// </summary>
public interface IErrorHandlingHub
{
/// <summary>
/// Called if a hub connection or call is attempted with an invalid or unauthorized token. After calling this, the connection is aborted.
/// </summary>
/// <param name="reason">The <see cref="ConnectionAbortReason"/>.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
Task AbortingConnection(ConnectionAbortReason reason, CancellationToken cancellationToken);
}
}
src/Tgstation.Server.Api/Hubs/IJobsHub.cs
+1 -1
View File
@@ -8,7 +8,7 @@ namespace Tgstation.Server.Api.Hubs
/// <summary>
/// SignalR client methods for receiving <see cref="JobResponse"/>s.
/// </summary>
public interface IJobsHub : IErrorHandlingHub
public interface IJobsHub
{
/// <summary>
/// Push a <paramref name="job"/> update to the client.
src/Tgstation.Server.Host/Extensions/ServiceCollectionExtensions.cs
+1 -2
View File
@@ -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
/// <param name="services">The <see cref="IServiceCollection"/> to add the <typeparamref name="THub"/> to.</param>
public static void AddHub<THub, THubMethods>(this IServiceCollection services)
where THub : ConnectionMappingHub<THub, THubMethods>
where THubMethods : class, IErrorHandlingHub
where THubMethods : class
{
ArgumentNullException.ThrowIfNull(services);
src/Tgstation.Server.Host/Jobs/JobsHubGroupMapper.cs
+2 -1
View File
@@ -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;
}
/// <inheritdoc />
src/Tgstation.Server.Host/Security/AuthorizationContextHubFilter.cs
+3 -9
View File
@@ -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
{
/// <summary>
@@ -41,7 +39,7 @@ namespace Tgstation.Server.Host.Security
public async Task OnConnectedAsync(HubLifetimeContext context, Func<HubLifetimeContext, Task> 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<object> InvokeMethodAsync(HubInvocationContext invocationContext, Func<HubInvocationContext, ValueTask<object>> 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
/// </summary>
/// <param name="hub">The current <see cref="Hub"/>.</param>
/// <returns><see langword="true"/> if the hub call should continue, <see langword="false"/> if it shouldn't and has been aborted.</returns>
async ValueTask<bool> 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;
}
src/Tgstation.Server.Host/Security/README.md
+1 -1
View File
@@ -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:
src/Tgstation.Server.Host/Utils/SignalR/ComprehensiveHubContext.cs
+5 -28
View File
@@ -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 <see cref="IHubContext{THub}"/> with <see cref="User"/> connection ID mapping.
/// </summary>
/// <typeparam name="THub">The <see cref="Hub"/> the <see cref="ComprehensiveHubContext{THub, THubMethods}"/> is for.</typeparam>
/// <typeparam name="THubMethods">The interface <see cref="IErrorHandlingHub"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
sealed class ComprehensiveHubContext<THub, THubMethods> : IConnectionMappedHubContext<THub, THubMethods>, IHubConnectionMapper<THub, THubMethods>, IRestartHandler
/// <typeparam name="THubMethods">The <see langword="interface"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
sealed class ComprehensiveHubContext<THub, THubMethods> : IConnectionMappedHubContext<THub, THubMethods>, IHubConnectionMapper<THub, THubMethods>
where THub : ConnectionMappingHub<THub, THubMethods>
where THubMethods : class, IErrorHandlingHub
where THubMethods : class
{
/// <inheritdoc />
public IHubClients<THubMethods> Clients => wrappedHubContext.Clients;
@@ -53,20 +50,15 @@ namespace Tgstation.Server.Host.Utils.SignalR
/// Initializes a new instance of the <see cref="ComprehensiveHubContext{THub, THubMethods}"/> class.
/// </summary>
/// <param name="wrappedHubContext">The value of <see cref="wrappedHubContext"/>.</param>
/// <param name="serverControl">The <see cref="IServerControl"/> to <see cref="IServerControl.RegisterForRestart(IRestartHandler)"/> with.</param>
/// <param name="logger">The value of <see cref="logger"/>.</param>
public ComprehensiveHubContext(
IHubContext<THub, THubMethods> wrappedHubContext,
IServerControl serverControl,
ILogger<ComprehensiveHubContext<THub, THubMethods>> logger)
{
this.wrappedHubContext = wrappedHubContext ?? throw new ArgumentNullException(nameof(wrappedHubContext));
ArgumentNullException.ThrowIfNull(serverControl);
this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
userConnections = new ConcurrentDictionary<long, Dictionary<string, HubCallerContext>>();
serverControl.RegisterForRestart(this);
}
/// <inheritdoc />
@@ -123,7 +115,7 @@ namespace Tgstation.Server.Host.Utils.SignalR
}
/// <inheritdoc />
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));
}
/// <inheritdoc />
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();
}
}
}
src/Tgstation.Server.Host/Utils/SignalR/ConnectionMappingHub.cs
+2 -3
View File
@@ -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 <see langword="class"/> for <see cref="Hub{T}"/>s that want to map their connection IDs to <see cref="Models.PermissionSet"/>s.
/// </summary>
/// <typeparam name="TChildHub">The child <see langword="class"/> inheriting from the <see cref="ConnectionMappingHub{TChildHub, THubMethods}"/>.</typeparam>
/// <typeparam name="THubMethods">The interface <see cref="IErrorHandlingHub"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
/// <typeparam name="THubMethods">The <see langword="interface"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
[TgsAuthorize]
abstract class ConnectionMappingHub<TChildHub, THubMethods> : Hub<THubMethods>
where TChildHub : ConnectionMappingHub<TChildHub, THubMethods>
where THubMethods : class, IErrorHandlingHub
where THubMethods : class
{
/// <summary>
/// The <see cref="IHubConnectionMapper{THub, THubMethods}"/> used to map connections.
src/Tgstation.Server.Host/Utils/SignalR/IConnectionMappedHubContext.cs
+3 -6
View File
@@ -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
/// <typeparam name="THubMethods">The interface <see langword="class"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
interface IConnectionMappedHubContext<THub, THubMethods> : IHubContext<THub, THubMethods>
where THub : Hub<THubMethods>
where THubMethods : class, IErrorHandlingHub
where THubMethods : class
{
/// <summary>
/// Called when a user connects. Should return an <see cref="IEnumerable{T}"/> of hub group names the given <see cref="IAuthenticationContext"/> belongs in.
@@ -33,11 +32,9 @@ namespace Tgstation.Server.Host.Utils.SignalR
List<string> UserConnectionIds(User user);
/// <summary>
/// Calls <see cref="IErrorHandlingHub.AbortingConnection(ConnectionAbortReason, CancellationToken)"/> with <see cref="ConnectionAbortReason.TokenInvalid"/> on and aborts the connections associated with the given <paramref name="user"/>.
/// Aborts the connections associated with the given <paramref name="user"/>.
/// </summary>
/// <param name="user">The <see cref="User"/> to abort the connections of.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
ValueTask NotifyAndAbortUnauthedConnections(User user, CancellationToken cancellationToken);
void AbortUnauthedConnections(User user);
}
}
src/Tgstation.Server.Host/Utils/SignalR/IHubConnectionMapper.cs
+2 -3
View File
@@ -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 <see cref="User"/>s for a given <typeparamref name="THub"/>.
/// </summary>
/// <typeparam name="THub">The <see cref="Hub"/> whose connections are being mapped.</typeparam>
/// <typeparam name="THubMethods">The interface <see cref="IErrorHandlingHub"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
/// <typeparam name="THubMethods">The <see langword="interface"/> for implementing <see cref="Hub{T}"/> methods.</typeparam>
interface IHubConnectionMapper<THub, THubMethods>
where THub : ConnectionMappingHub<THub, THubMethods>
where THubMethods : class, IErrorHandlingHub
where THubMethods : class
{
/// <summary>
/// To be called when a hub connection is made.
tests/Tgstation.Server.Tests/Live/Instance/JobsHubTests.cs
-26
View File
@@ -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<long> 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<JobResponse> Callback { get; set; }
public Func<ConnectionAbortReason, CancellationToken, Task> 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;
}
}
}
tests/Tgstation.Server.Tests/Live/RawRequestTests.cs
+15 -34
View File
@@ -359,10 +359,6 @@ namespace Tgstation.Server.Tests.Live
class FuncProxiedJobsHub : IJobsHub
{
public Func<JobResponse, CancellationToken, Task> ProxyFunc { get; set; }
public Func<ConnectionAbortReason, Task> 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<Exception>(() => errorTcs.Task);
Assert.AreNotEqual(HubConnectionState.Connected, testUserConn2.State);
}
finally
{