From e9cea2ccbcef30714a46b7b52b0f1c7822d8ed54 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Tue, 4 Aug 2026 15:34:40 +0200 Subject: [PATCH] Let a shared vault arrive, a bucket be found, and a vault be deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things a user reported, one of which was a real bug and one of which was not the bug it looked like. **A vault shared with somebody never reached their machine.** The grant was correct at both ends: the sharing client verified the recipient's key against the key log and wrapped every generation to it, the server stored it, and /me would have returned it. Nothing asked. VaultSession.RefreshVaultsAsync — the method whose own summary says it is "called after a share and on a periodic pass" — had no caller anywhere in the application, so the vault list was whatever the last browser sign-in cached. A restart did not help: an offline unlock reads that same cache. The vault appeared only if the recipient happened to sign in through the browser again, which is why this looked like sharing being broken rather than like a list that was never re-read. So every synchronisation pass now re-reads it, before it syncs. SyncOnceAsync takes the whole server rather than its sync half for that reason, and the order matters: a vault admitted by the refresh is one that same pass then pulls, where the other order would show a newly shared vault as an empty one until the minute after. The shell is told only when the set actually changed — it rebuilds the tab strip's vault menu from the session's list, and doing that on every quiet pass would rebuild a menu once a minute for nothing. The test needed the fake server to be able to do something no test here had needed before: hand this account a vault it did not make. ShareVaultWithMe wraps a real key to the encryption key this account enrolled, so the keyring opens it exactly as it opens a real colleague's — a helper that filled the field with bytes would let a vault appear in the list and never prove it could be read. **Adding an S3 bucket on the desktop works, and could not be found.** The report was that it is not possible; driving the real XAML headlessly says otherwise — Keychain, + BUCKET, and the editor saves. What is true is that S3 is where somebody goes looking, and from there SELECT BUCKET opened a combo box with nothing in it and no sentence anywhere saying that a bucket is a keychain item. From where the user was standing that is indistinguishable from an application with no way to add one. The empty state now says what a bucket is and offers a button that lands on the keychain with the editor already open — navigating to the screen and leaving + BUCKET to be found among five buttons would be most of the same problem. The phone gets the sentence and no button: its keychain screen reads and deletes and edits nothing, so there is no editor to send anybody to, and naming the machine that has one beats an empty control that reads as a screen still loading. The keychain screen's layout test grew the two categories it never covered. Tags and buckets arrived after it was written, and the header strip it measures is one that has overflowed twice before. **A vault can now be deleted.** DELETE /api/v1/vaults/{id}, gated on Admin — the line the rename already drew, for a stronger version of its reason, since this takes the vault from everybody in it at once. The row is soft-deleted and every grant to it withdrawn in one write; VaultAccessService filters on the stamp at both ends, so from that moment the vault is absent from every member's /me and every call naming it answers 404. Their clients notice on the pass described above. The team behind it is archived when it owned nothing else, which is the mirror of renaming it: a vault made from the vaults screen gets a team named after it that nobody was ever shown, and leaving that behind would leave a membership list no screen has a row for. That is a second call rather than one transaction — archiving is TeamService's, it refuses while a team owns vaults, and it can only tell that this one no longer does once the deletion is committed. A crash between the two leaves an empty team: invisible, archivable afterwards, harmless, and a better failure than a vault that could not be deleted because tidying up after it did not work. Two refusals worth stating. The personal vault cannot be deleted at either end: it is created by enrollment, everything filed nowhere else lives in it, and no call would make another. And the items are kept — ciphertext behind a vault nothing will resolve, so deleting them buys no confidentiality while destroying what an operator undoing a mistake would need. The client drops the key from the keyring and the row from the cache rather than waiting for a refresh, so the list is right immediately; the items stay, as they stay for a vault whose grant was withdrawn, because a copy is on every other member's machine too and removing these rows would be the client pretending to a reach it does not have. The confirmation says that out loud before it is answered. It is the one sentence this screen must not leave implied: deletion is no more retroactive than revocation is. See ADR 0001. Desktop only, deliberately. The Android vaults screen offers no rename and no hand-over either, so adding delete alone there would be the one destructive vault operation on a screen with no other. Three places asserted that a vault can never be deleted — TeamService's refusal message, the TeamNotEmpty problem code, and ADR 0009 — and each now names the route instead. --- README.md | 19 +- docs/adr/0009-team-access-model.md | 10 ++ src/DodoSSH.Api/Features/Teams/TeamLog.cs | 13 ++ src/DodoSSH.Api/Features/Teams/TeamService.cs | 15 +- .../Features/Teams/VaultGrantEndpoints.cs | 90 ++++++++++ .../Features/Teams/VaultGrantService.cs | 80 +++++++++ src/DodoSSH.Api/Setup/EndpointRegistration.cs | 1 + .../Views/FilesScreen.axaml | 19 +- src/DodoSSH.Client.Api/DodoSshApiClient.cs | 24 +++ .../Views/TransfersScreen.axaml | 33 +++- .../Views/VaultsScreen.axaml | 8 + src/DodoSSH.Client.Session/VaultSharing.cs | 44 +++++ .../ViewModels/MainWindowViewModel.cs | 44 ++++- .../ViewModels/TransfersViewModel.cs | 77 +++++++- .../ViewModels/VaultViewModel.cs | 66 ++++++- .../ViewModels/VaultsViewModel.cs | 165 ++++++++++++++++-- src/DodoSSH.Client.Storage/VaultStore.cs | 38 ++++ src/DodoSSH.Client.Sync/VaultKeyring.cs | 34 ++++ src/DodoSSH.Contracts/ProblemCodes.cs | 8 +- .../EndpointInventoryTests.cs | 5 + tests/DodoSSH.Api.Tests/TeamEndpointTests.cs | 126 +++++++++++++ .../ScreenLayoutTests.cs | 17 ++ .../StubTeamServer.cs | 4 + .../FakeVaultServer.Teams.cs | 120 +++++++++++++ .../ShellFlowTests.cs | 44 +++++ .../VaultSharingTests.cs | 118 +++++++++++++ 26 files changed, 1181 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 5aaa367..bac43c1 100644 --- a/README.md +++ b/README.md @@ -307,10 +307,12 @@ usually staying in it. Six limits, stated rather than discovered: -- **A vault cannot be deleted.** Nothing in this product removes one, and the server refuses to archive the - membership list behind a vault that still exists — a shared vault is readable *because* of membership, so - archiving it would take the vault away from everybody holding a key, including you, quietly and all at - once. The screen says so where you would otherwise go looking for the button. +- **Deleting a vault does not reach a machine that has already synced it.** An admin or the owner can + delete a shared vault: it leaves everybody's list at once, every key to it is withdrawn, and the + membership list behind it is archived when it existed to carry that vault alone. What it cannot do is + take back the copy a colleague pulled yesterday — the same limit revocation has, for the same reason — + and the question you are asked before it happens says so. Your personal vault is refused: everything + filed nowhere else lives in it, and there is no way to make another. - **Removing a member is not retroactive.** It revokes their grants, rotates every vault behind that membership list your machine can open, and hands each new key to the members who are left — so nothing written from that point on is readable to them. Everything they already pulled is still on their machine. @@ -622,8 +624,8 @@ keychain plus a terminal — and the spike that gates all of it. membership to permissions, so a viewer may pull and may not push; the desktop client reads and syncs every vault it holds a key for, and a real VAULTS screen replaces the placeholder. The screen is vault-shaped rather than team-shaped: naming a vault makes the membership list that carries it, so the - team is behind the vault rather than a thing anybody has to create first. A vault can be renamed and - handed to another member; a member row carries when that account was last here; and an address with no + team is behind the vault rather than a thing anybody has to create first. A vault can be renamed, deleted + and handed to another member; a member row carries when that account was last here; and an address with no account on this deployment can be invited, joining the moment somebody signs in with it. See [Sharing a vault](#sharing-a-vault) for the one distinction the whole design rests on, and the limits worth knowing before you rely on it; the reasoning is in @@ -639,6 +641,11 @@ keychain plus a terminal — and the spike that gates all of it. the rotation is re-sealed as it is pushed, so nothing reaches the server under a superseded key at all. See [ADR 0010](docs/adr/0010-vault-key-rotation.md). + **A vault shared with you arrives on the next synchronisation pass**, within the minute, with no sign-in + and nothing to press. There is no push channel, so each pass asks the server which vaults this account can + reach before syncing the ones it already knows — which is also how a vault that has been deleted, or one + whose grant was withdrawn, stops being listed. + **Ownership transfer is here, and it is one write rather than two.** The member you name becomes owner and you become an admin, in a single transaction — because ownership is sole, so promoting first leaves the team owned twice and demoting first leaves it owned by nobody, and there is nobody left with the diff --git a/docs/adr/0009-team-access-model.md b/docs/adr/0009-team-access-model.md index 3e4b44a..3c1a827 100644 --- a/docs/adr/0009-team-access-model.md +++ b/docs/adr/0009-team-access-model.md @@ -145,6 +145,16 @@ Three consequences of the change belong here: still appear is a create whose second call failed; cancelling that form archives it, which is a deliberate exception to this client's rule against tidying up on the user's behalf, made because nothing else can reach it. +- **A vault can be deleted**, which is the operation this ADR's first version said the product did not + have: `DELETE /api/v1/vaults/{id}` requires `PermissionFlags.Admin`, the same line the rename draws and + for a stronger reason — it takes the vault from everybody in it at once. It soft-deletes the row, + withdraws every grant to it, and archives the owning team when that team was made to carry this vault + alone, so deleting the last vault of a one-vault team does not leave a membership list no screen has a + row for. The personal vault is refused: it is created by enrollment, everything filed nowhere else lives + in it, and no call would make another. The items are kept — ciphertext behind a vault nothing resolves — + because deleting them buys no confidentiality and destroys what an operator would need to undo a + mistake. **It reaches no machine that has already synced the vault**, which is the same limit revocation + has and for the same reason; see ADR 0001. The client says so before asking for the confirmation. - **A vault can be renamed**, which it could not before: `PUT /api/v1/vaults/{id}` requires `PermissionFlags.Admin` — the line `UpdateTeamEndpoint` already draws, because a name is what everybody in the vault sees it called rather than part of its contents. It renames the owning team with it when diff --git a/src/DodoSSH.Api/Features/Teams/TeamLog.cs b/src/DodoSSH.Api/Features/Teams/TeamLog.cs index 076dee0..d577a73 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamLog.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamLog.cs @@ -60,6 +60,19 @@ internal static partial class TeamLog Message = "Renamed vault {VaultId} of team {TeamId}.")] internal static partial void VaultRenamed(ILogger logger, Guid vaultId, Guid? teamId); + /// + /// A warning rather than information, and the count is why: this is the one operation that takes a + /// vault away from everybody at once, and how many keys were withdrawn with it is the number an + /// operator asked about it will want. No name, for the reason the rename gives. + /// + [LoggerMessage( + EventId = 2116, + Level = LogLevel.Warning, + Message = "Deleted vault {VaultId} of team {TeamId}, by {ActorId}; withdrew {GrantCount} key " + + "grant(s). Its items are kept and unreadable; copies already synced elsewhere are unaffected.")] + internal static partial void VaultDeleted( + ILogger logger, Guid vaultId, Guid teamId, Guid actorId, int grantCount); + [LoggerMessage( EventId = 2106, Level = LogLevel.Information, diff --git a/src/DodoSSH.Api/Features/Teams/TeamService.cs b/src/DodoSSH.Api/Features/Teams/TeamService.cs index 78fe647..737d786 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamService.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamService.cs @@ -218,10 +218,15 @@ internal sealed class TeamService( /// The vault check is the whole of this operation's safety and it refuses rather than /// cascades. Archiving a team hides it from every member's list at once, and a team vault /// resolves through membership — so archiving one that still owned vaults would take those vaults - /// away from people who hold keys to them, silently, including the caller. Nothing in this product - /// deletes a vault, so there is no sequence of calls that turns this refusal into a success today. - /// That is stated plainly rather than worked around, for the reason the SFTP layer refuses a - /// recursive delete: a refusal is visible and a quiet removal is not. + /// away from people who hold keys to them, silently, including the caller. The way out is to delete + /// those vaults first — VaultGrantService.DeleteVaultAsync, which asks its own question and + /// withdraws every key — and then archive what is left. A refusal that names a route is worth more + /// than a cascade, for the reason the SFTP layer refuses a recursive delete: a refusal is visible and + /// a quiet removal is not. + /// + /// + /// Deleting the last vault of a team made to carry it archives that team on the way past, so + /// the ordinary case never reaches this refusal at all. See DeleteVaultEndpoint. /// /// /// Memberships are archived with the team, in one transaction, because a live membership pointing @@ -246,7 +251,7 @@ internal sealed class TeamService( throw new TeamNotEmptyException( string.Create( CultureInfo.InvariantCulture, - $"This team still owns {vaultCount} vault(s), and archiving it would take them away from everybody holding a key — including you. There is no way to delete a vault in this product yet, so a team with vaults cannot be archived.")); + $"This team still owns {vaultCount} vault(s), and archiving it would take them away from everybody holding a key — including you. Delete those vaults first, which asks about each one and withdraws every key to it.")); } var now = clock.GetUtcNow(); diff --git a/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs b/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs index b62730c..44e32f3 100644 --- a/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs +++ b/src/DodoSSH.Api/Features/Teams/VaultGrantEndpoints.cs @@ -122,6 +122,96 @@ internal sealed class RenameVaultEndpoint( } } +/// Deletes a vault. +/// +/// +/// Admin, like the rename above and for a stronger version of the same reason: this takes the vault away +/// from everybody in it at once, so it belongs to whoever administers the membership list rather than to +/// anybody who may write to its contents. +/// +/// +/// Authenticated rather than Enrolled, again like the rename. Deleting a vault touches no key material — +/// it withdraws grants rather than wrapping anything — so requiring a published identity key would refuse +/// an admin for a reason that has nothing to do with what they are asking. +/// +/// +/// The team behind it is archived in a second call, and the two are deliberately not one +/// transaction. Archiving is TeamService's, it refuses while a team owns vaults, and it can +/// only tell that this one no longer does once the deletion is committed. A crash between the two leaves +/// an empty team — invisible on every screen, archivable afterwards by the endpoint that exists for it, +/// and harmless — which is a better failure than a vault that could not be deleted because tidying up its +/// team failed. +/// +/// +internal sealed class DeleteVaultEndpoint( + ICurrentUserContext currentUser, + IVaultAccessService vaultAccess, + VaultGrantService grants, + TeamService teams) + : EndpointWithoutRequest> +{ + /// + public override void Configure() + { + Delete("/api/v1/vaults/{vaultId:guid}"); + + Policies(Auth.AuthenticatedPolicy); + + Description(b => b + .WithName("DeleteVault") + .WithSummary("Deletes a vault and withdraws every key to it.") + .WithTags("Vaults")); + } + + /// + public override async Task> ExecuteAsync( + CancellationToken ct) + { + var user = await currentUser.GetOrProvisionAsync(ct).ConfigureAwait(false); + var access = await vaultAccess + .ResolveAsync(user.Id, Route("vaultId"), ct) + .ConfigureAwait(false); + + if (!access.Granted || !access.Permissions.HasFlag(PermissionFlags.Read)) + { + // Indistinguishable from a vault that never existed, and from one somebody else deleted a + // moment ago — which is what makes a repeat of this call safe to send. + return TypedResults.NotFound(); + } + + if (!access.Permissions.HasFlag(PermissionFlags.Admin)) + { + return Problems.Coded( + StatusCodes.Status403Forbidden, + ProblemCodes.Forbidden, + "Only an admin or the owner of the team that owns this vault can delete it."); + } + + try + { + var orphaned = await grants.DeleteVaultAsync(user, access.Vault!, ct).ConfigureAwait(false); + + if (orphaned is not null) + { + await teams.ArchiveAsync(user, orphaned, ct).ConfigureAwait(false); + } + + return TypedResults.NoContent(); + } + catch (VaultGrantInvalidException exception) + { + return Problems.Coded( + StatusCodes.Status400BadRequest, ProblemCodes.InvalidVaultGrant, exception.Message); + } + catch (TeamNotEmptyException) + { + // The team gained a vault between the delete and the archive. The deletion stands and is what + // was asked for; the team staying is not a failure the caller can or should act on. + return TypedResults.NoContent(); + } + } +} + /// Wraps this vault's key to another member. /// /// The one call in this API whose body the server can neither produce nor check. It stores a sealed diff --git a/src/DodoSSH.Api/Features/Teams/VaultGrantService.cs b/src/DodoSSH.Api/Features/Teams/VaultGrantService.cs index 3f2b67e..277d8b1 100644 --- a/src/DodoSSH.Api/Features/Teams/VaultGrantService.cs +++ b/src/DodoSSH.Api/Features/Teams/VaultGrantService.cs @@ -216,6 +216,86 @@ internal sealed class VaultGrantService( RekeyRequired: vault.RekeyRequired); } + /// + /// Deletes a vault, and the team behind it where that team exists to carry this vault alone. + /// + /// + /// The team that has been left owning nothing by this, or null where there is none. The caller + /// archives it; see for why that is a second step. + /// + /// + /// + /// Soft, like every other removal here. The row is stamped rather than dropped, so an operator + /// can see that a vault existed and what became of it — and so the foreign keys from its items and its + /// grants stay valid. VaultAccessService filters on the stamp at both ends, so from the moment + /// this commits the vault is absent from every member's /me and every call naming it answers 404. + /// + /// + /// The items are deliberately left alone. They are ciphertext behind a vault nothing will now + /// resolve, so deleting them buys no confidentiality — and it would destroy the one thing an operator + /// restoring a vault deleted by mistake would need. What this is not is a promise about other people's + /// machines: a member who synced yesterday still holds their copy, exactly as ADR 0001 says about + /// revocation, and the deletion message says so. + /// + /// + /// Every grant is withdrawn in the same write. Nothing reads them once the vault is gone, but a + /// live grant on a deleted vault is a row that says somebody holds a key to something that no longer + /// exists — and the grant list is the thing an operator reads to answer "who could open this". + /// + /// + internal async Task DeleteVaultAsync( + UserAccount actor, + Vault vault, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(vault); + + // A personal vault is where everything filed nowhere else lives, and it is created by enrollment + // rather than by anybody choosing to make it. Deleting one would leave an enrolled account with a + // key, no vault, and no way to make the vault it is supposed to have. + if (vault.OwnerKind != VaultOwnerKind.Team || vault.TeamId is not { } teamId) + { + throw new VaultGrantInvalidException( + "A personal vault cannot be deleted. It is where everything filed nowhere else lives, and " + + "the account has no way to make another. Move what you want to keep into a shared vault " + + "and delete that instead."); + } + + var now = clock.GetUtcNow(); + + var grants = await database.VaultKeyGrants + .Where(g => g.VaultId == vault.Id && g.RevokedAtUtc == null) + .ToListAsync(cancellationToken) + .ConfigureAwait(false); + + foreach (var grant in grants) + { + grant.State = GrantState.Revoked; + grant.RevokedAtUtc = now; + } + + vault.DeletedAtUtc = now; + vault.UpdatedAtUtc = now; + + await database.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + + TeamLog.VaultDeleted(logger, vault.Id, teamId, actor.Id, grants.Count); + + // Read after the save, so "owns nothing else" is asked of a database that already knows this one + // is gone. The team is returned rather than archived here for the reason the rename gives about + // renaming it: a team carrying several vaults has a name and a membership list somebody chose, and + // one made to carry this vault alone is a thing its creator was never shown. + var alone = !await database.Vaults + .AnyAsync(other => other.TeamId == teamId && other.DeletedAtUtc == null, cancellationToken) + .ConfigureAwait(false); + + return alone + ? await database.Teams + .SingleOrDefaultAsync(t => t.Id == teamId && t.DeletedAtUtc == null, cancellationToken) + .ConfigureAwait(false) + : null; + } + /// Lists who can open a vault. /// /// One row per holder, not one per grant. A rotated vault holds several grants per member — one per diff --git a/src/DodoSSH.Api/Setup/EndpointRegistration.cs b/src/DodoSSH.Api/Setup/EndpointRegistration.cs index 7ddbd54..bb9d545 100644 --- a/src/DodoSSH.Api/Setup/EndpointRegistration.cs +++ b/src/DodoSSH.Api/Setup/EndpointRegistration.cs @@ -57,6 +57,7 @@ internal static class EndpointRegistration typeof(RevokeTeamInvitationEndpoint), typeof(CreateTeamVaultEndpoint), typeof(RenameVaultEndpoint), + typeof(DeleteVaultEndpoint), typeof(ListVaultGrantsEndpoint), typeof(IssueVaultGrantEndpoint), typeof(RevokeVaultGrantEndpoint), diff --git a/src/DodoSSH.Client.Android/Views/FilesScreen.axaml b/src/DodoSSH.Client.Android/Views/FilesScreen.axaml index 514a71d..bf43a61 100644 --- a/src/DodoSSH.Client.Android/Views/FilesScreen.axaml +++ b/src/DodoSSH.Client.Android/Views/FilesScreen.axaml @@ -85,7 +85,7 @@ - @@ -98,6 +98,16 @@ + + +