From 6846f67c203dcb9e4964d2e52f9f93dc9cce1f6f Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 2 Oct 2026 13:16:48 +0200 Subject: [PATCH] Server: send each item at the revision its ciphertext is sealed at, keep team archiving with the owner, refuse suspended accounts, and stop enrolment failing after an hour - Pull hydrated every historical change entry with the item's current ciphertext under that entry's old revision. The AAD binds the revision, so a client opened it at the wrong version, failed, and parked its own pending edit as undecryptable. Entries an item has since moved past are left out; the later entry carries the same payload under the right number, and a fresh device no longer downloads an item once per edit it ever had. - Deleting a team's last vault archived the team whoever deleted it, so an admin could archive it out from under its owner. Only the owner's delete archives it now, and TeamService.ArchiveAsync checks. - UserStatus was checked nowhere on the request path. ActiveAccountRequirement on both policies answers 403 account-not-active for a suspended or deprovisioned account. - The hourly last-seen update moves xmin under the tracked user, and enrolment then saved that user against the stale token: a 500 for anybody enrolling more than an hour after first signing in. The entity is reloaded after the update. --- .../Authorization/ActiveAccountRequirement.cs | 49 +++++++++++++++ .../Authorization/CurrentUserContext.cs | 17 ++++- .../Authorization/EnrolledRequirement.cs | 20 ++++++ src/DodoSSH.Api/Features/Sync/SyncService.cs | 23 +++++-- .../Features/Teams/TeamEndpoints.cs | 4 +- src/DodoSSH.Api/Features/Teams/TeamService.cs | 8 +++ .../Features/Teams/VaultGrantEndpoints.cs | 7 ++- src/DodoSSH.Api/Program.cs | 1 + src/DodoSSH.Api/Setup/Auth.cs | 11 +++- src/DodoSSH.Contracts/ProblemCodes.cs | 3 + src/DodoSSH.Contracts/PublicAPI.Unshipped.txt | 1 + .../IdentityEndpointTests.cs | 63 +++++++++++++++++++ tests/DodoSSH.Api.Tests/SyncEndpointTests.cs | 46 ++++++++++++++ tests/DodoSSH.Api.Tests/TeamEndpointTests.cs | 36 +++++++++++ 14 files changed, 277 insertions(+), 12 deletions(-) create mode 100644 src/DodoSSH.Api/Authorization/ActiveAccountRequirement.cs diff --git a/src/DodoSSH.Api/Authorization/ActiveAccountRequirement.cs b/src/DodoSSH.Api/Authorization/ActiveAccountRequirement.cs new file mode 100644 index 0000000..ce10b5e --- /dev/null +++ b/src/DodoSSH.Api/Authorization/ActiveAccountRequirement.cs @@ -0,0 +1,49 @@ +using DodoSSH.Domain; +using Microsoft.AspNetCore.Authorization; + +namespace DodoSSH.Api.Authorization; + +/// Requires the caller's account to be . +/// +/// +/// is documented as "sign-in blocked", and nothing blocked it: the identity +/// provider still issues tokens for the person, every endpoint resolved them through the current-user context, +/// and that context looked an account up by issuer, subject and deletion alone. A suspended account could pull +/// and push vaults, take relay tickets and hold the events socket open. This is the check, on both policies, so +/// that no endpoint can be reached without it. +/// +/// +/// A connection already open when the account is suspended — a relay tunnel, the events socket — is not +/// reached by this; it ends on its own idle and lifetime limits. +/// +/// +internal sealed class ActiveAccountRequirement : IAuthorizationRequirement; + +/// Satisfies from the account row. +internal sealed class ActiveAccountHandler(ICurrentUserContext currentUser, IHttpContextAccessor accessor) + : AuthorizationHandler +{ + /// + protected override async Task HandleRequirementAsync( + AuthorizationHandlerContext context, + ActiveAccountRequirement requirement) + { + ArgumentNullException.ThrowIfNull(context); + + // The policy's authentication requirement already refuses an anonymous caller with a 401; failing this + // as well would turn that into a 403. + if (context.User.Identity?.IsAuthenticated != true) + { + return; + } + + var cancellationToken = accessor.HttpContext?.RequestAborted ?? CancellationToken.None; + + var user = await currentUser.GetOrProvisionAsync(cancellationToken).ConfigureAwait(false); + + if (user.Status == UserStatus.Active) + { + context.Succeed(requirement); + } + } +} diff --git a/src/DodoSSH.Api/Authorization/CurrentUserContext.cs b/src/DodoSSH.Api/Authorization/CurrentUserContext.cs index a6d7bbe..aeca5d5 100644 --- a/src/DodoSSH.Api/Authorization/CurrentUserContext.cs +++ b/src/DodoSSH.Api/Authorization/CurrentUserContext.cs @@ -102,8 +102,14 @@ internal sealed class CurrentUserContext( /// read-then-write: user_account carries the xmin concurrency token, so two overlapping /// requests from one user would each read the row, each set the timestamp, and the second would /// fail on a version that had moved under it. This writes at most one row and cannot conflict. - /// The tracked entity is deliberately left alone — a value up to an hour stale in memory changes - /// nothing, and marking it modified would enlist the user row in whatever the request saves next. + /// + /// ◆ But the write moves xmin under the tracked entity, so the entity is re-read after it. It is + /// not marked modified — that would enlist the user row in whatever the request saves next — and a + /// stale timestamp in memory would change nothing on its own. A stale concurrency token does: the next + /// save in the same request that touches this user (enrolment stamps EnrolledAtUtc) compared + /// against the version from before the update and failed, so enrolling more than an hour after first + /// signing in answered 500. Only when a row was actually written, which is once an hour at most. + /// /// private async Task RefreshLastSeenAsync(UserAccount user, CancellationToken cancellationToken) { @@ -114,13 +120,18 @@ internal sealed class CurrentUserContext( return; } - await database.Users + var written = await database.Users .Where(u => u.Id == user.Id && (u.LastSeenAtUtc == null || u.LastSeenAtUtc < now - LastSeenWindow)) .ExecuteUpdateAsync( setters => setters.SetProperty(u => u.LastSeenAtUtc, now), cancellationToken) .ConfigureAwait(false); + + if (written > 0) + { + await database.Entry(user).ReloadAsync(cancellationToken).ConfigureAwait(false); + } } private Task FindAsync(string issuer, string subject, CancellationToken cancellationToken) => diff --git a/src/DodoSSH.Api/Authorization/EnrolledRequirement.cs b/src/DodoSSH.Api/Authorization/EnrolledRequirement.cs index d7818a2..eeebfa6 100644 --- a/src/DodoSSH.Api/Authorization/EnrolledRequirement.cs +++ b/src/DodoSSH.Api/Authorization/EnrolledRequirement.cs @@ -80,6 +80,26 @@ internal sealed class DodoAuthorizationResultHandler : IAuthorizationMiddlewareR ArgumentNullException.ThrowIfNull(context); ArgumentNullException.ThrowIfNull(authorizeResult); + var accountNotActive = authorizeResult.Forbidden + && authorizeResult.AuthorizationFailure is { } refusal + && refusal.FailedRequirements.OfType().Any(); + + if (accountNotActive) + { + await TypedResults.Problem( + detail: "This account has been suspended. Ask whoever runs this DodoSSH server.", + statusCode: StatusCodes.Status403Forbidden, + type: ProblemCodes.TypeBaseUri + ProblemCodes.AccountNotActive, + extensions: new Dictionary(StringComparer.Ordinal) + { + ["code"] = ProblemCodes.AccountNotActive, + }) + .ExecuteAsync(context) + .ConfigureAwait(false); + + return; + } + var enrollmentMissing = authorizeResult.Forbidden && authorizeResult.AuthorizationFailure is { } failure && failure.FailedRequirements.OfType().Any(); diff --git a/src/DodoSSH.Api/Features/Sync/SyncService.cs b/src/DodoSSH.Api/Features/Sync/SyncService.cs index ab2ce84..7245aaf 100644 --- a/src/DodoSSH.Api/Features/Sync/SyncService.cs +++ b/src/DodoSSH.Api/Features/Sync/SyncService.cs @@ -536,9 +536,21 @@ internal sealed class SyncService( /// Attaches current row state to change-log entries. /// + /// /// A page of changes can mix item types, so rows are loaded per type — one query each, not one per /// change — and then matched back in the log's order. Ordering is the log's, never the load's: a pull /// that returned changes grouped by type would hand the client a sequence its cursor cannot resume from. + /// + /// + /// ◆ An entry an item has since moved past is left out. The log keeps every revision, but the + /// table keeps only the latest ciphertext — so hydrating an old entry paired that entry's revision number + /// with the newest payload, and the payload's AAD binds the revision it was sealed at. A client opened it + /// at the old number, failed, and parked its own unsent edit to that item as undecryptable. The later + /// entry is in this page or a later one, since the item's ChangeSequence is that entry's sequence; + /// it carries the same payload under the right number, so nothing is lost by sending it once. Leaving + /// the old entries out also stops a fresh device downloading an item once per edit it ever had. The + /// cursor is computed from the unfiltered page by the caller, so it still moves past them. + /// /// private async Task> HydrateAsync( Vault vault, @@ -571,10 +583,13 @@ internal sealed class SyncService( } } - return [.. changes.Select(change => Describe( - change, - kinds.GetValueOrDefault(change.EntityType), - rows.GetValueOrDefault((change.EntityType, change.EntityId))))]; + return [.. changes + .Where(change => rows.GetValueOrDefault((change.EntityType, change.EntityId)) is not { } row + || row.ChangeSequence <= change.Sequence) + .Select(change => Describe( + change, + kinds.GetValueOrDefault(change.EntityType), + rows.GetValueOrDefault((change.EntityType, change.EntityId))))]; } /// Turns one change-log entry and its row into what a client receives. diff --git a/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs b/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs index 1d68e28..e4044a7 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs @@ -179,8 +179,8 @@ internal sealed class ArchiveTeamEndpoint(ICurrentUserContext currentUser, TeamS } // Owner, not admin. An admin the owner promoted must not be able to archive the team out - // from under them — that is the boundary IsOwner exists to draw. Nothing behind this - // re-checks it, unlike the transfer below, so this line is the whole of the guard. + // from under them — that is the boundary IsOwner exists to draw. TeamService.ArchiveAsync + // checks it again, because deleting a team's last vault archives it too. if (!access.IsOwner) { return Problems.Coded( diff --git a/src/DodoSSH.Api/Features/Teams/TeamService.cs b/src/DodoSSH.Api/Features/Teams/TeamService.cs index cf7ea3f..ec952fd 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamService.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamService.cs @@ -244,8 +244,16 @@ internal sealed class TeamService( Team team, CancellationToken cancellationToken) { + ArgumentNullException.ThrowIfNull(actor); ArgumentNullException.ThrowIfNull(team); + // Both callers check this first and answer the caller properly; this is what stops a third one that + // forgets from letting an admin archive the team out from under its owner. + if (!(await ResolveAsync(actor.Id, team.Id, cancellationToken).ConfigureAwait(false)).IsOwner) + { + throw new InvalidOperationException("Only the owner of a team can archive it."); + } + var vaultCount = await CountVaultsAsync(team.Id, cancellationToken).ConfigureAwait(false); if (vaultCount > 0) diff --git a/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs b/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs index 44e32f3..0c02447 100644 --- a/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs +++ b/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs @@ -191,7 +191,12 @@ internal sealed class DeleteVaultEndpoint( { var orphaned = await grants.DeleteVaultAsync(user, access.Vault!, ct).ConfigureAwait(false); - if (orphaned is not null) + // Archived only by its owner. Archiving revokes every membership, the owner's included, and an + // admin the owner promoted must not be able to do that by deleting vaults one at a time when the + // archive endpoint refuses them outright. An admin's delete stands; the empty team is left for + // the owner, who can archive it or make a vault in it. + if (orphaned is not null + && (await teams.ResolveAsync(user.Id, orphaned.Id, ct).ConfigureAwait(false)).IsOwner) { await teams.ArchiveAsync(user, orphaned, ct).ConfigureAwait(false); } diff --git a/src/DodoSSH.Api/Program.cs b/src/DodoSSH.Api/Program.cs index 7458dd9..a1a99dd 100644 --- a/src/DodoSSH.Api/Program.cs +++ b/src/DodoSSH.Api/Program.cs @@ -55,6 +55,7 @@ builder.Services.AddDodoRelay(); // Scoped rather than the AddAuthorization default of singleton: the handler reads the request's // DbContext, and a singleton would capture one for the lifetime of the process. builder.Services.AddScoped(); +builder.Services.AddScoped(); // Replaces the framework default, which is also registered as a singleton. Last registration wins. builder.Services.AddSingleton(); diff --git a/src/DodoSSH.Api/Setup/Auth.cs b/src/DodoSSH.Api/Setup/Auth.cs index 13df96d..ffd2f71 100644 --- a/src/DodoSSH.Api/Setup/Auth.cs +++ b/src/DodoSSH.Api/Setup/Auth.cs @@ -62,7 +62,14 @@ internal static class Auth private static void AddDodoPolicies(AuthorizationOptions options) { - options.AddPolicy(AuthenticatedPolicy, policy => policy.RequireAuthenticatedUser()); + // Both policies also require the account to be active. A suspended or deprovisioned account still + // holds a token the identity provider will vouch for until it expires — the status is this server's + // own decision, so it is this server that has to refuse. See ActiveAccountHandler. + options.AddPolicy( + AuthenticatedPolicy, + policy => policy + .RequireAuthenticatedUser() + .AddRequirements(new Authorization.ActiveAccountRequirement())); // Enrollment state lives in the database, so this is satisfied by EnrolledHandler. // An unmet EnrolledRequirement is rewritten into a ProblemDetails carrying @@ -72,7 +79,7 @@ internal static class Auth EnrolledPolicy, policy => policy .RequireAuthenticatedUser() - .AddRequirements(new Authorization.EnrolledRequirement())); + .AddRequirements(new Authorization.ActiveAccountRequirement(), new Authorization.EnrolledRequirement())); // Deny by default: an endpoint without an explicit policy still requires a caller. options.FallbackPolicy = options.GetPolicy(AuthenticatedPolicy); diff --git a/src/DodoSSH.Contracts/ProblemCodes.cs b/src/DodoSSH.Contracts/ProblemCodes.cs index 77f91a9..6e01ae7 100644 --- a/src/DodoSSH.Contracts/ProblemCodes.cs +++ b/src/DodoSSH.Contracts/ProblemCodes.cs @@ -28,6 +28,9 @@ public static class ProblemCodes /// The caller has not yet enrolled a public key, so no vault is reachable. public const string EnrollmentRequired = "enrollment-required"; + /// The caller's account has been suspended or deprovisioned by an operator. + public const string AccountNotActive = "account-not-active"; + /// Enrollment was attempted for a user who already holds a different current key. public const string AlreadyEnrolled = "already-enrolled"; diff --git a/src/DodoSSH.Contracts/PublicAPI.Unshipped.txt b/src/DodoSSH.Contracts/PublicAPI.Unshipped.txt index 41b4fe6..e45a481 100644 --- a/src/DodoSSH.Contracts/PublicAPI.Unshipped.txt +++ b/src/DodoSSH.Contracts/PublicAPI.Unshipped.txt @@ -1,6 +1,7 @@ #nullable enable const DodoSSH.Contracts.ProblemCodes.AlreadyEnrolled = "already-enrolled" -> string! const DodoSSH.Contracts.ProblemCodes.ClientTooOld = "client-too-old" -> string! +const DodoSSH.Contracts.ProblemCodes.AccountNotActive = "account-not-active" -> string! const DodoSSH.Contracts.ProblemCodes.EnrollmentRequired = "enrollment-required" -> string! const DodoSSH.Contracts.ProblemCodes.EventsUnavailable = "events-unavailable" -> string! const DodoSSH.Contracts.ProblemCodes.Forbidden = "forbidden" -> string! diff --git a/tests/DodoSSH.Api.Tests/IdentityEndpointTests.cs b/tests/DodoSSH.Api.Tests/IdentityEndpointTests.cs index e5fc36f..920038d 100644 --- a/tests/DodoSSH.Api.Tests/IdentityEndpointTests.cs +++ b/tests/DodoSSH.Api.Tests/IdentityEndpointTests.cs @@ -192,6 +192,69 @@ public sealed class IdentityEndpointTests(ApiFixture fixture) user.EnrolledAtUtc.ShouldNotBeNull(); } + /// + /// Suspended means "sign-in blocked", and nothing used to block it: the identity provider still vouches + /// for the person, and every endpoint resolved them by issuer and subject alone. The status is this + /// server's decision, so this server has to be the one refusing — on its own endpoints and on vaults. + /// + [Fact] + public async Task ASuspendedAccount_IsRefusedEverywhereWithACodeSayingWhy() + { + using var enrollment = NewEnrollment(); + var client = enrollment.CreateClient(fixture); + + await EnrollAsync(client, enrollment.Build()); + + await using (var scope = fixture.CreateScope()) + { + var database = scope.ServiceProvider.GetRequiredService(); + + await database.Users + .Where(u => u.Subject == enrollment.Subject) + .ExecuteUpdateAsync(setters => setters.SetProperty(u => u.Status, UserStatus.Suspended)); + } + + await ShouldBeProblemAsync( + await client.GetAsync(new Uri(MeUrl, UriKind.Relative)), + HttpStatusCode.Forbidden, + ProblemCodes.AccountNotActive); + + await ShouldBeProblemAsync( + await client.PostContractAsync( + $"/api/v1/vaults/{enrollment.VaultId}/sync/pull", new SyncPullRequest(null, null, null)), + HttpStatusCode.Forbidden, + ProblemCodes.AccountNotActive); + } + + /// + /// The last-seen stamp is written past the change tracker once an hour, which moves the row's + /// concurrency token under the entity the request is holding. Enrolment then saved that same user and + /// failed on the stale token — so anyone who signed in, left, and came back an hour later to finish + /// setting up got a 500 on their first try. + /// + [Fact] + public async Task Enroll_MoreThanAnHourAfterFirstSigningIn_Succeeds() + { + using var enrollment = NewEnrollment(); + var client = enrollment.CreateClient(fixture); + + (await client.GetAsync(new Uri(MeUrl, UriKind.Relative))).EnsureSuccessStatusCode(); + + await using (var scope = fixture.CreateScope()) + { + var database = scope.ServiceProvider.GetRequiredService(); + + await database.Users + .Where(u => u.Subject == enrollment.Subject) + .ExecuteUpdateAsync(setters => setters.SetProperty( + u => u.LastSeenAtUtc, TimeProvider.System.GetUtcNow().AddHours(-2))); + } + + var response = await EnrollAsync(client, enrollment.Build()); + + response.KeyGeneration.ShouldBe(1); + } + [Fact] public async Task Enroll_AppendsToTheKeyLogAndTheEntryReproducesItsOwnHash() { diff --git a/tests/DodoSSH.Api.Tests/SyncEndpointTests.cs b/tests/DodoSSH.Api.Tests/SyncEndpointTests.cs index aecc404..4c6e9ef 100644 --- a/tests/DodoSSH.Api.Tests/SyncEndpointTests.cs +++ b/tests/DodoSSH.Api.Tests/SyncEndpointTests.cs @@ -276,6 +276,52 @@ public sealed class SyncEndpointTests(ApiFixture fixture) second.Changes.Count.ShouldBe(1); } + /// + /// An item edited several times is pulled once, at the revision its ciphertext is sealed at. + /// + /// + /// The table holds only the newest payload, and its AAD binds the newest version. Hydrating each older + /// log entry with it paired revision 1 or 2 with a payload sealed at 3, which no client can open — and a + /// client that could not open it parked its own pending edit as undecryptable. + /// + [Fact] + public async Task Pull_OfAnItemEditedSinceItsEarlierEntries_SendsOnlyItsCurrentRevision() + { + var (subject, vaultId) = await SeedUserWithVaultAsync(); + var client = fixture.CreateClientFor(subject); + + var create = NewCreateBatch(); + var entityId = create.Operations[0].EntityId; + await client.PostContractAsync(PushUrl(vaultId), create); + + await client.PostContractAsync(PushUrl(vaultId), new SyncPushRequest( + [ + NewOperation(entityId, expectedVersion: 1, envelope: [2, 2, 2]), + ])); + + await client.PostContractAsync(PushUrl(vaultId), new SyncPushRequest( + [ + NewOperation(entityId, expectedVersion: 2, envelope: [3, 3, 3]), + ])); + + var pulled = await (await client.PostContractAsync( + PullUrl(vaultId), + new SyncPullRequest(null, null, null))).Content.ReadContractAsync(); + + pulled.ShouldNotBeNull(); + + var change = pulled.Changes.ShouldHaveSingleItem(); + change.Version.ShouldBe(3); + change.Payload!.Envelope.ShouldBe([3, 3, 3]); + + // And the cursor is past all three entries, so the next pull does not offer the old ones again. + var next = await (await client.PostContractAsync( + PullUrl(vaultId), + new SyncPullRequest(pulled.NextCursor, null, null))).Content.ReadContractAsync(); + + next!.Changes.ShouldBeEmpty(); + } + [Fact] public async Task Pull_WithACursorFromAnotherVault_Is400() { diff --git a/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs b/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs index 6b12f1d..1bcf22a 100644 --- a/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs +++ b/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs @@ -368,6 +368,42 @@ public sealed class TeamEndpointTests(ApiFixture fixture) .ShouldNotContain(row => row.TeamId == team.TeamId, "the team existed to carry that vault"); } + /// + /// The archive endpoint refuses an admin, and deleting a team's last vault archives the team too — so + /// without the same rule there, an admin could archive the team out from under its owner one vault at a + /// time, revoking the owner's membership on the way. The delete is theirs to make; the team is not. + /// + [Fact] + public async Task AnAdminDeletingTheLastVault_DeletesItButLeavesTheTeamToItsOwner() + { + var owner = await EnrolledClientAsync("vault-delete-admin-owner", "vdaowner@example.com"); + var admin = await EnrolledClientAsync("vault-delete-admin-admin", "vdaadmin@example.com"); + + var team = await CreateTeamAsync(owner, "Owned"); + var vaultId = await CreateVaultAsync(owner, team.TeamId); + var entry = await LookupAsync(owner, "vdaadmin@example.com"); + + await AddMemberAsync(owner, team.TeamId, entry.UserId, TeamMemberRole.Admin); + + await owner.PostContractAsync( + $"/api/v1/vaults/{vaultId}/grants", GrantRequest(entry, generation: 1)); + + (await DeleteAsync(admin, VaultUrl(vaultId))).StatusCode.ShouldBe(HttpStatusCode.NoContent); + + (await ReadAsync(owner, MeUrl)).Vaults + .ShouldNotContain(vault => vault.VaultId == vaultId); + + (await ReadAsync>(owner, TeamsUrl)) + .ShouldContain(row => row.TeamId == team.TeamId, "only the owner archives a team"); + + var ownerMe = await ReadAsync(owner, MeUrl); + + (await ReadAsync>(owner, MembersUrl(team.TeamId))) + .Where(row => row.UserId == ownerMe.UserId) + .ShouldHaveSingleItem() + .Role.ShouldBe(TeamMemberRole.Owner); + } + /// /// A team carrying several vaults keeps its own life when one of them goes, which is the same line the /// rename draws — and the other vault has to survive, which is the assertion that a deletion aimed at