Let a shared vault arrive, a bucket be found, and a vault be deleted

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.
This commit is contained in:
2026-08-04 15:34:40 +02:00
parent a0568d4c35
commit e9cea2ccbc
26 changed files with 1181 additions and 41 deletions
+13 -6
View File
@@ -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
+10
View File
@@ -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
+13
View File
@@ -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);
/// <remarks>
/// 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.
/// </remarks>
[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,
+10 -5
View File
@@ -218,10 +218,15 @@ internal sealed class TeamService(
/// <b>The vault check is the whole of this operation's safety and it refuses rather than
/// cascades.</b> 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 — <c>VaultGrantService.DeleteVaultAsync</c>, 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.
/// </para>
/// <para>
/// Deleting the <em>last</em> 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 <c>DeleteVaultEndpoint</c>.
/// </para>
/// <para>
/// 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();
@@ -122,6 +122,96 @@ internal sealed class RenameVaultEndpoint(
}
}
/// <summary>Deletes a vault.</summary>
/// <remarks>
/// <para>
/// 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// <para>
/// <b>The team behind it is archived in a second call, and the two are deliberately not one
/// transaction.</b> Archiving is <c>TeamService</c>'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.
/// </para>
/// </remarks>
internal sealed class DeleteVaultEndpoint(
ICurrentUserContext currentUser,
IVaultAccessService vaultAccess,
VaultGrantService grants,
TeamService teams)
: EndpointWithoutRequest<Results<NoContent, NotFound, ProblemHttpResult>>
{
/// <inheritdoc />
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"));
}
/// <inheritdoc />
public override async Task<Results<NoContent, NotFound, ProblemHttpResult>> ExecuteAsync(
CancellationToken ct)
{
var user = await currentUser.GetOrProvisionAsync(ct).ConfigureAwait(false);
var access = await vaultAccess
.ResolveAsync(user.Id, Route<Guid>("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();
}
}
}
/// <summary>Wraps this vault's key to another member.</summary>
/// <remarks>
/// The one call in this API whose body the server can neither produce nor check. It stores a sealed
@@ -216,6 +216,86 @@ internal sealed class VaultGrantService(
RekeyRequired: vault.RekeyRequired);
}
/// <summary>
/// Deletes a vault, and the team behind it where that team exists to carry this vault alone.
/// </summary>
/// <returns>
/// The team that has been left owning nothing by this, or null where there is none. The caller
/// archives it; see <see cref="DeleteVaultEndpoint"/> for why that is a second step.
/// </returns>
/// <remarks>
/// <para>
/// <b>Soft, like every other removal here.</b> 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. <c>VaultAccessService</c> filters on the stamp at both ends, so from the moment
/// this commits the vault is absent from every member's <c>/me</c> and every call naming it answers 404.
/// </para>
/// <para>
/// <b>The items are deliberately left alone.</b> 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.
/// </para>
/// <para>
/// <b>Every grant is withdrawn in the same write.</b> 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".
/// </para>
/// </remarks>
internal async Task<Team?> 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;
}
/// <summary>Lists who can open a vault.</summary>
/// <remarks>
/// One row per holder, not one per grant. A rotated vault holds several grants per member — one per
@@ -57,6 +57,7 @@ internal static class EndpointRegistration
typeof(RevokeTeamInvitationEndpoint),
typeof(CreateTeamVaultEndpoint),
typeof(RenameVaultEndpoint),
typeof(DeleteVaultEndpoint),
typeof(ListVaultGrantsEndpoint),
typeof(IssueVaultGrantEndpoint),
typeof(RevokeVaultGrantEndpoint),
@@ -85,7 +85,7 @@
</ComboBox.ItemTemplate>
</ComboBox>
<ComboBox HorizontalAlignment="Stretch" MinHeight="44" IsVisible="{Binding ShowsBucketPicker}"
<ComboBox HorizontalAlignment="Stretch" MinHeight="44" IsVisible="{Binding ShowsBucketChoice}"
ItemsSource="{Binding Buckets}" SelectedItem="{Binding SelectedBucket}"
PlaceholderText="choose a bucket">
<ComboBox.ItemTemplate>
@@ -98,6 +98,16 @@
</ComboBox.ItemTemplate>
</ComboBox>
<!--
A picker with nothing in it says nothing, and here it cannot say much more: this head's keychain
screen reads and deletes and edits nothing, so there is no bucket editor to send anybody to. The
sentence is therefore the whole of what this can offer, and it names the machine that can — which
is better than an empty control that reads as a screen still loading. The desktop's counterpart
has a button, because over there the editor is two clicks away.
-->
<TextBlock Classes="detail" TextWrapping="Wrap" IsVisible="{Binding ShowsNoBuckets}"
Text="No buckets yet. A bucket is a keychain item — its name, keys and endpoint — and this phone can read the keychain but not add to it. Add one on the desktop and it syncs here." />
<!--
Its own box, deliberately not the hosts screen's. This is a separate authentication, so a password
typed to open a terminal has not been offered here — and quietly reusing it would make a one-time
@@ -106,8 +116,13 @@
<TextBox Classes="field secret" IsVisible="{Binding SelectedHostAsksForAPassword}"
Text="{Binding TypedPassword}" PlaceholderText="password" />
<!--
Gone rather than disabled when there is no bucket to open, because a disabled OPEN under an empty
picker is the same dead end with a grey button on it. The sentence above is what the card carries
in that state.
-->
<Button Classes="primary" Content="{Binding ConnectLabel}" Command="{Binding ConnectCommand}"
IsEnabled="{Binding !IsBusy}" />
IsVisible="{Binding !ShowsNoBuckets}" IsEnabled="{Binding !IsBusy}" />
</StackPanel>
</Border>
@@ -202,6 +202,24 @@ public interface IVaultGrantApi
UpdateVaultRequest request,
CancellationToken cancellationToken);
/// <summary>
/// Deletes a vault and withdraws every key to it.
/// </summary>
/// <returns>Whether there was a vault to delete.</returns>
/// <remarks>
/// <para>
/// False rather than an exception for a vault that is already gone, exactly as
/// <see cref="RevokeVaultGrantAsync"/> answers about a grant: a caller driving towards "this vault is
/// no longer there" has arrived, and two admins deleting the same vault must not leave the slower one
/// looking at an error about something that happened.
/// </para>
/// <para>
/// What it does not do is reach anybody's machine. A member who synced before this holds their copy
/// afterwards — see ADR 0001 on revocation — and the interface that offers this has to say so.
/// </para>
/// </remarks>
Task<bool> DeleteVaultAsync(Guid vaultId, CancellationToken cancellationToken);
/// <summary>Lists who holds a key to this vault.</summary>
Task<VaultGrantsResponse> ListVaultGrantsAsync(Guid vaultId, CancellationToken cancellationToken);
@@ -599,6 +617,12 @@ public sealed class DodoSshApiClient(HttpClient http, IAccessTokenProvider token
DodoSshJsonContext.Default.VaultSummary,
cancellationToken);
/// <inheritdoc />
public Task<bool> DeleteVaultAsync(Guid vaultId, CancellationToken cancellationToken) =>
DeleteAsync(
string.Create(CultureInfo.InvariantCulture, $"/api/v1/vaults/{vaultId}"),
cancellationToken);
/// <inheritdoc />
public Task<VaultGrantsResponse> ListVaultGrantsAsync(
Guid vaultId,
@@ -407,7 +407,15 @@
<TextBlock Classes="heading" FontSize="16" TextAlignment="Center"
Text="Connect to a host" IsVisible="{Binding ShowsHostPicker}" />
<TextBlock Classes="heading" FontSize="16" TextAlignment="Center"
Text="Open a bucket" IsVisible="{Binding ShowsBucketPicker}" />
Text="Open a bucket" IsVisible="{Binding ShowsBucketChoice}" />
<!--
A different heading rather than the same one over an empty picker, which is what this screen
used to do: SELECT BUCKET opened a combo box with nothing in it and no sentence anywhere saying
where a bucket comes from. From here that is indistinguishable from an application that cannot
add one — and this is the screen somebody looking for S3 arrives at.
-->
<TextBlock Classes="heading" FontSize="16" TextAlignment="Center"
Text="No buckets yet" IsVisible="{Binding ShowsNoBuckets}" />
<!--
The sentence, and the button under it. Both go away once the picker is up — by then they have
@@ -425,14 +433,26 @@
IsVisible="{Binding ShowsHostPicker}"
Text="Browsing a host's files opens its own SFTP connection, so the host records a second login — it is not the channel a terminal uses." />
<TextBlock Classes="hint" FontSize="12" TextAlignment="Center"
IsVisible="{Binding ShowsBucketPicker}"
IsVisible="{Binding ShowsBucketChoice}"
Text="A bucket has nothing to connect to: the keys and the endpoint are tested by the first listing, not by opening a session." />
<!--
Where a bucket comes from, said on the screen that needs one. It names the keychain rather
than only offering a button, because the button is a shortcut and the sentence is what makes
the bucket findable again next week without it.
-->
<TextBlock Classes="hint" FontSize="12" TextAlignment="Center"
IsVisible="{Binding ShowsNoBuckets}"
Text="A bucket is an item in your keychain — the bucket name, its keys and its endpoint, encrypted like everything else there. Add one and it appears here." />
<Button Classes="accent" HorizontalAlignment="Center" Content="SELECT HOST"
IsVisible="{Binding ShowsHostPicker}"
Command="{Binding BeginChoosingRemoteCommand}" />
<Button Classes="accent" HorizontalAlignment="Center" Content="SELECT BUCKET"
IsVisible="{Binding ShowsBucketPicker}"
IsVisible="{Binding ShowsBucketChoice}"
Command="{Binding BeginChoosingRemoteCommand}" />
<Button Classes="accent" HorizontalAlignment="Center" Content="+ BUCKET"
IsVisible="{Binding ShowsNoBuckets}"
Command="{Binding AddBucketCommand}"
ToolTip.Tip="Goes to the keychain with the bucket editor open." />
</StackPanel>
<!-- ==== The picker, once it has been asked for ==== -->
@@ -454,9 +474,14 @@
</ComboBox.ItemTemplate>
</ComboBox>
<!--
Gated on there being one, not merely on this being the S3 screen. An empty picker is the
thing that made this screen look like it could not add a bucket, and the invitation above
now sends somebody to the keychain instead of opening this.
-->
<ComboBox HorizontalAlignment="Stretch" ItemsSource="{Binding Buckets}"
SelectedItem="{Binding SelectedBucket}"
IsVisible="{Binding ShowsBucketPicker}"
IsVisible="{Binding ShowsBucketChoice}"
PlaceholderText="choose a bucket">
<ComboBox.ItemTemplate>
<DataTemplate x:DataType="vm:ObjectStoreRowViewModel">
@@ -127,6 +127,11 @@
The vault's own operations. RENAME is an admin's; handing it on is the owner's alone, and
that is the line the server draws as well — an admin the owner promoted must not be able to
take the vault from them.
DELETE is an admin's too, and it is last and red because it is the only one of the three that
cannot be undone. It is absent on the personal vault rather than disabled: there is no version
of this window in which that vault can go, so a greyed button would be an offer that never
becomes real. Its question is the card below, as every destructive answer on this screen is.
-->
<StackPanel Grid.Column="1" Orientation="Horizontal" Spacing="6"
IsVisible="{Binding ShowsVaultActions}">
@@ -136,6 +141,9 @@
<Button Classes="ghost" Content="HAND OVER" Command="{Binding HandOverCommand}"
IsEnabled="{Binding !IsBusy}" IsVisible="{Binding OwnsSelected}"
ToolTip.Tip="Hands this vault to the selected member. They become its owner and you become an admin; only the new owner can hand it on again." />
<Button Classes="danger" Content="DELETE" Command="{Binding DeleteVaultCommand}"
IsEnabled="{Binding !IsBusy}" IsVisible="{Binding CanDeleteSelected}"
ToolTip.Tip="Deletes this vault and withdraws everybody's key to it. It cannot reach a machine that has already synced it." />
</StackPanel>
</Grid>
</Border>
@@ -198,6 +198,50 @@ public sealed partial class VaultSession
return stored;
}
/// <summary>
/// Deletes a vault, here and on the server.
/// </summary>
/// <param name="api">The vault calls.</param>
/// <param name="vaultId">The vault to delete.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns>Whether there was a vault to delete.</returns>
/// <remarks>
/// <para>
/// The server first, and this machine only if it agreed. The other order would take a vault off this
/// screen and leave it on everybody else's, which is the one outcome worse than the deletion failing:
/// the person who pressed it is then the only one who believes it is gone.
/// </para>
/// <para>
/// <b>The key is dropped from the keyring, and the items are not.</b> Dropping the key is what makes
/// this machine unable to read what is left, which is the honest end state — the ciphertext is still
/// in this cache, as it is in every other member's, and pretending otherwise by deleting the rows
/// would be claiming a reach this product does not have. See ADR 0001. The rows go with the next
/// wipe of the cache; nothing reads them meanwhile, because every list is built from the vaults the
/// keyring can open.
/// </para>
/// </remarks>
public async Task<bool> DeleteVaultAsync(
IVaultGrantApi api,
Guid vaultId,
CancellationToken cancellationToken)
{
ObjectDisposedException.ThrowIf(disposed, this);
ArgumentNullException.ThrowIfNull(api);
if (!await api.DeleteVaultAsync(vaultId, cancellationToken).ConfigureAwait(false))
{
return false;
}
keyring.Forget(vaultId);
await Vault.RemoveAsync(vaultId, cancellationToken).ConfigureAwait(false);
Vaults = await Vault.ListAsync(cancellationToken).ConfigureAwait(false);
return true;
}
/// <summary>
/// Wraps a vault's key to another member, after verifying their published key.
/// </summary>
@@ -377,7 +377,10 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
// to register for a reason no message could explain.
this.deviceName = string.IsNullOrWhiteSpace(deviceName) ? Environment.MachineName : deviceName;
transfers = new TransfersViewModel(sftpSessions, clock);
// The fourth argument is the route out of the S3 screen's empty state. A bucket is made on the
// keychain screen, which is a different tab and two clicks away from somebody who has gone to S3
// to add one — so the screen that needs a bucket is given a way to reach the screen that makes one.
transfers = new TransfersViewModel(sftpSessions, clock, addBucket: ShowNewBucket);
// Built once, like the workspace it writes for, and given a vault only while one is open. It has to
// outlive every lock for the same reason the workspace does: a shell opened before a lock is still
@@ -1300,6 +1303,27 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
vaults.NewVaultCommand.Execute(null);
}
/// <summary>
/// Goes to the keychain with the bucket editor open.
/// </summary>
/// <remarks>
/// <para>
/// The same shape as <see cref="ShowNewVault"/> and for the same reason: the thing being made lives on
/// one screen, and the moment somebody wants it happens on another. Here the two are a whole tab apart —
/// S3 is where a bucket is used and the keychain is where its keys are kept — which is what made the S3
/// screen's empty picker read as an application that could not add one at all.
/// </para>
/// <para>
/// Silent when the vault is locked, which is a state the button behind this is not reachable in: the
/// screen it sits on is inside the unlocked half of the window.
/// </para>
/// </remarks>
private void ShowNewBucket()
{
ShowScreen(ShellScreen.Keychain);
Vault?.NewObjectStoreCommand.Execute(null);
}
// ---- The phone's connect menu ----
/// <summary>
@@ -2599,6 +2623,8 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
// "connecting…" for ever and leave the session it eventually opened with nothing in the window
// naming it, and so no way to close it. The subscription dies with the vault once the attempt
// resolves: the vault holds the handler, not the other way round.
oldValue.VaultsChanged -= OnVaultsAdmitted;
if (attempts.Count == 0)
{
oldValue.ConnectionStarting -= OnVaultConnectionStarting;
@@ -2614,6 +2640,12 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
newValue.SessionOpened += OnVaultSessionOpened;
newValue.PropertyChanged += OnVaultPropertyChanged;
// A vault somebody shared arrives on a synchronisation pass rather than through anything the
// user pressed here — so the menu that lists them is rebuilt from the event rather than at the
// end of a command, which is the one place a newly admitted vault has no command to be at the
// end of.
newValue.VaultsChanged += OnVaultsAdmitted;
// The host list is rebuilt from scratch on every synchronisation pass, and a rebuilt row starts
// disconnected — so without this the status dots go out once a minute underneath terminals that
// are still open. The rows belong to the vault and the connection state belongs to the shell,
@@ -2696,6 +2728,16 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
AdoptTab(tab);
}
/// <summary>
/// Redraws the vault menu after a synchronisation pass found a vault this account had not seen.
/// </summary>
/// <remarks>
/// The vault itself is already on every screen by the time this runs — the pass that admitted it
/// reloaded the lists — so this is only the one thing the vault view model has no way to reach: the
/// switches in the tab strip, which are built here from the session's vault list.
/// </remarks>
private void OnVaultsAdmitted(object? sender, EventArgs e) => RebuildVaultToggles();
/// <remarks>
/// The vault opens SSH sessions and this shell owns the strip they appear in, so this is the seam between
/// them and nothing more.
@@ -313,13 +313,24 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
/// removes the thread from the question rather than making the test guess it right.
/// </para>
/// </param>
/// <param name="addBucket">
/// Takes the user to where a bucket is made, or null in a head that has nowhere to take them.
/// <para>
/// A delegate rather than this screen making one itself, and it is the same seam the vaults screen uses
/// for the shell: a bucket is a keychain item, the editor that writes one belongs to the keychain screen,
/// and duplicating it here would be a second form writing the same secret. What this screen owns is the
/// knowledge that somebody standing on it with no buckets needs to be sent somewhere.
/// </para>
/// </param>
internal TransfersViewModel(
ISftpSessionFactory sftp,
TimeProvider clock,
Action<Action>? post = null)
Action<Action>? post = null,
Action? addBucket = null)
{
this.sftp = sftp;
this.post = post ?? (action => Dispatcher.UIThread.Post(action));
this.addBucket = addBucket;
// The supplier answers with whatever session is current at the moment a transfer starts, which is
// what lets a queue survive a disconnect and reconnect without every queued row failing.
@@ -332,6 +343,32 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
RemoteEntries.CollectionChanged += (_, _) => OnPropertyChanged(nameof(HasRemoteEntries));
LocalEntries.CollectionChanged += (_, _) => OnPropertyChanged(nameof(HasLocalEntries));
Transfers.CollectionChanged += (_, _) => OnPropertyChanged(nameof(HasTransfers));
// The fourth, and it follows a list this screen does not own: the buckets are refilled from the
// vault on every synchronisation pass, so the invitation these flags decide between has to change
// with them rather than at the moment somebody pressed something. That is the path that matters —
// adding the first bucket happens on another screen, and this one has to notice when it comes back.
Buckets.CollectionChanged += (_, _) => RaiseBucketState();
}
/// <summary>Where a bucket is made, or null in a head with nowhere to send anybody.</summary>
private readonly Action? addBucket;
/// <summary>Goes to the keychain with the bucket editor open.</summary>
/// <remarks>
/// The dead end this replaces was a real one: SELECT BUCKET opened a picker with nothing in it, and
/// nothing anywhere on the screen said that a bucket is made on the keychain screen. Somebody who had
/// come to S3 to add one had arrived at the place it is used and been shown no route to the place it is
/// created.
/// </remarks>
[RelayCommand]
private void AddBucket()
{
// Closed first, so returning here from the keychain does not find a picker still open over a list
// that has since gained the bucket it was empty of.
IsChoosingRemote = false;
addBucket?.Invoke();
}
/// <summary>The hosts that can be connected to, which is the vault's list.</summary>
@@ -352,6 +389,28 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
[ObservableProperty]
private ObjectStoreRowViewModel? selectedBucket;
/// <summary>
/// Whether there is a bucket to open at all.
/// </summary>
/// <remarks>
/// The question this screen used to answer by showing an empty combo box. A bucket is made on the
/// keychain screen and nowhere else, and S3 is where somebody goes looking for it — so a picker with
/// nothing in it and no sentence beside it was, from where the user was standing, an application with no
/// way to add a bucket. See <see cref="AddBucket"/>.
/// </remarks>
internal bool HasBuckets => Buckets.Count > 0;
/// <summary>Whether this screen is on S3 with a bucket to offer.</summary>
/// <remarks>
/// The two flags are resolved here rather than combined in markup, because a binding cannot say "and" —
/// and because the pair is one question with two answers, which is easier to keep straight in one place
/// than in four <c>IsVisible</c> expressions that have to stay each other's opposites.
/// </remarks>
internal bool ShowsBucketChoice => ShowsBucketPicker && HasBuckets;
/// <summary>Whether this screen is on S3 with nothing to open yet.</summary>
internal bool ShowsNoBuckets => ShowsBucketPicker && !HasBuckets;
/// <summary>
/// Which sort of remote the right-hand pane is about to open.
/// </summary>
@@ -1641,16 +1700,30 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
partial void OnRemoteChanged(RemoteKind value)
{
OnPropertyChanged(nameof(ShowsHostPicker));
OnPropertyChanged(nameof(ShowsBucketPicker));
OnPropertyChanged(nameof(ConnectLabel));
OnPropertyChanged(nameof(SelectedHostAsksForAPassword));
RaiseBucketState();
// Arriving at the other destination shows its invitation rather than a picker somebody left open on
// this one — and the picker it would have left open is the wrong one, since the two kinds have
// different lists behind them.
IsChoosingRemote = false;
}
/// <summary>Raises the three flags that answer "is there a bucket to open".</summary>
/// <remarks>
/// Together, because they are one fact read three ways and a caller that raised two of them would leave
/// an invitation and an empty state on screen at once.
/// </remarks>
private void RaiseBucketState()
{
OnPropertyChanged(nameof(ShowsBucketPicker));
OnPropertyChanged(nameof(HasBuckets));
OnPropertyChanged(nameof(ShowsBucketChoice));
OnPropertyChanged(nameof(ShowsNoBuckets));
}
partial void OnIsConnectedChanged(bool value)
{
OnPropertyChanged(nameof(CanDownload));
@@ -3845,7 +3845,7 @@ internal sealed partial class VaultViewModel(
return;
}
var report = await SyncOnceAsync(server.Sync, cancellationToken).ConfigureAwait(true);
var report = await SyncOnceAsync(server, cancellationToken).ConfigureAwait(true);
// Null means a background pass held the gate. Saying so beats reporting a sync that this
// press did not perform.
@@ -3935,7 +3935,7 @@ internal sealed partial class VaultViewModel(
return;
}
var report = await SyncOnceAsync(server.Sync, cancellationToken).ConfigureAwait(true);
var report = await SyncOnceAsync(server, cancellationToken).ConfigureAwait(true);
if (report is null)
{
@@ -3971,12 +3971,19 @@ internal sealed partial class VaultViewModel(
}
/// <remarks>
/// <para>
/// The gate is shared with the manual command, so a press and a tick can never overlap. Taken with a
/// zero timeout rather than awaited: a pass that arrives while another is running has nothing to add by
/// waiting for it, and queueing them would turn a slow server into a backlog of identical work.
/// </para>
/// <para>
/// Takes the whole server rather than its sync half, because the first thing a pass does is ask
/// <em>which vaults there are</em> — see <see cref="AdmitNewVaultsAsync"/>. A pass that only synced the
/// vaults it already knew could never discover one somebody had just shared.
/// </para>
/// </remarks>
private async Task<IReadOnlyList<VaultSyncReport>?> SyncOnceAsync(
ISyncApi api,
IVaultServer server,
CancellationToken cancellationToken)
{
if (!await syncGate.WaitAsync(0, cancellationToken).ConfigureAwait(true))
@@ -3986,10 +3993,14 @@ internal sealed partial class VaultViewModel(
try
{
// Before the sync, so a vault admitted here is one of the vaults that pass then pulls. The
// other order would show a newly shared vault as an empty one until the minute after.
await AdmitNewVaultsAsync(server.Account, cancellationToken).ConfigureAwait(true);
// Every vault this session can read, not only the one new items are filed into. A team's
// vault that never synced would show its hosts exactly once — at the unlock that first
// pulled it — and then quietly stop, which reads as the feature not working.
var report = await session.SyncAllAsync(api, cancellationToken).ConfigureAwait(true);
var report = await session.SyncAllAsync(server.Sync, cancellationToken).ConfigureAwait(true);
// Not unconditionally false, which it was while a pass was one vault and a failure was an
// exception. A failure is now a report — one unreachable team vault must not stop the others
@@ -4012,6 +4023,53 @@ internal sealed partial class VaultViewModel(
}
}
/// <summary>
/// Re-reads which vaults this account can reach, and opens any that have become readable.
/// </summary>
/// <remarks>
/// <para>
/// <b>This is the whole of how a shared vault arrives.</b> Sharing is two acts on two machines: the
/// person sharing wraps the vault key to the recipient, and the recipient's own client has to notice.
/// The recipient is handed nothing — there is no push channel — so without this the vault list stayed
/// exactly as it was cached at sign-in, and a vault shared with somebody appeared on their machine only
/// if they happened to sign in through the browser again. Everything else was already right, which is
/// why it looked like sharing was broken rather than like a list that was never re-read.
/// </para>
/// <para>
/// A failure is left to the caller, which treats it as the pass failing: the call is to the same server
/// the sync is about to use, so a refresh that cannot answer is not a state in which the sync would
/// have.
/// </para>
/// <para>
/// The shell is told only when the set actually changed. It rebuilds the tab strip's vault menu from
/// this list, and doing that on every quiet pass would rebuild a menu once a minute for nothing.
/// </para>
/// </remarks>
private async Task AdmitNewVaultsAsync(IAccountApi api, CancellationToken cancellationToken)
{
var before = session.Vaults.Count;
var admitted = await session.RefreshVaultsAsync(api, cancellationToken).ConfigureAwait(true);
if (admitted == 0 && session.Vaults.Count == before)
{
return;
}
VaultsChanged?.Invoke(this, EventArgs.Empty);
}
/// <summary>
/// Raised when a synchronisation pass found that the vaults this account can reach have changed.
/// </summary>
/// <remarks>
/// An event rather than a callback because the listener is the shell and the shell owns this object,
/// which is the same shape the connection events above use. What it is for is the tab strip's vault
/// menu: it is built from the session's vault list, so a vault admitted mid-session would otherwise be
/// on every screen and missing from the one control that can hide it.
/// </remarks>
internal event EventHandler? VaultsChanged;
/// <remarks>
/// <c>ConfigureAwait(true)</c> throughout, and that is load-bearing rather than habit: the loop is
/// started from the UI thread, so every continuation returns to it and the observable collections
@@ -233,16 +233,38 @@ internal sealed record VaultInvitationRowViewModel(TeamInvitationSummary Invitat
/// than read from the selection at confirm time — otherwise selecting a different row between arming
/// and confirming would apply the answer to something else.
/// </remarks>
/// <param name="Kind">Which operation the answer applies to.</param>
/// <param name="TeamId">The membership list the action is aimed at.</param>
/// <param name="MemberId">The member it is aimed at.</param>
/// <param name="MemberId">The member it is aimed at, or empty where it is not aimed at one.</param>
/// <param name="VaultId">The vault it is aimed at.</param>
/// <param name="Question">What is being asked.</param>
/// <param name="Consequence">What will actually happen, stated honestly.</param>
internal sealed record VaultActionRequest(
VaultActionKind Kind,
Guid TeamId,
Guid MemberId,
Guid VaultId,
string Question,
string Consequence);
/// <summary>Which destructive operation a confirmation is standing in front of.</summary>
/// <remarks>
/// Carried on the request rather than inferred from which fields are set. There were two of these the
/// moment deletion existed, and a confirm handler that guessed from a member id being empty would be one
/// refactor away from carrying out the wrong one of the two.
/// </remarks>
internal enum VaultActionKind
{
/// <summary>Not a legal value.</summary>
Unspecified = 0,
/// <summary>Hand the vault's membership list to somebody else.</summary>
HandOver = 1,
/// <summary>Delete the vault.</summary>
Delete = 2,
}
/// <summary>
/// The vaults screen: which vaults there are, who is in each, and who holds a key to it.
/// </summary>
@@ -414,6 +436,16 @@ internal sealed partial class VaultsViewModel(
/// <summary>Whether the selected vault is the personal one.</summary>
internal bool SelectedIsPersonal => SelectedVault?.IsPersonal == true;
/// <summary>
/// Whether the selected vault is one this account may delete.
/// </summary>
/// <remarks>
/// Narrower than <see cref="CanAdministerSelected"/> by exactly the personal vault, which cannot be
/// deleted by anybody: it is where everything filed nowhere else lives and nothing can make another.
/// The server refuses one too, so a button offered here would be a button that leads to a refusal.
/// </remarks>
internal bool CanDeleteSelected => SelectedVault is { IsPersonal: false, CanAdminister: true };
/// <summary>Whether there is anything to show beside the vault list.</summary>
internal bool HasSelection => SelectedVault is not null;
@@ -1230,13 +1262,70 @@ internal sealed partial class VaultsViewModel(
}
PendingAction = new VaultActionRequest(
VaultActionKind.HandOver,
teamId,
member.UserId,
vault.VaultId,
$"Hand '{vault.Name}' to {member.Name}?",
"They become its owner and you become an admin. You will not be able to take it back "
+ "yourself — only the new owner can hand it on. Your key to it is untouched.");
}
/// <summary>
/// Arms the deletion confirmation for the selected vault.
/// </summary>
/// <remarks>
/// <para>
/// The consequence is spelled out at length rather than summarised, and every sentence in it is one
/// somebody could otherwise be surprised by afterwards. The last is the one this product must never
/// leave implied: deleting a vault does not reach the machines it has already synced to. That is the
/// same limit revocation has, for the same reason, and it is written down in ADR 0001.
/// </para>
/// <para>
/// The personal vault is refused here as well as by the server. A button that reached a refusal would
/// be teaching somebody to try things and read errors, and the reason is a fact this screen knows.
/// </para>
/// </remarks>
[RelayCommand]
private void DeleteVault()
{
if (SelectedVault is not { } vault)
{
return;
}
if (vault.IsPersonal)
{
Status = "Your personal vault cannot be deleted. It is where everything filed nowhere else "
+ "lives, and there is no way to make another.";
return;
}
if (vault.TeamId is not { } teamId)
{
return;
}
var others = Members.Count(member => !member.IsSelf);
var shared = others > 0
? string.Create(
CultureInfo.CurrentCulture,
$" {others} other member(s) lose it at the same moment, without being asked.")
: string.Empty;
PendingAction = new VaultActionRequest(
VaultActionKind.Delete,
teamId,
Guid.Empty,
vault.VaultId,
$"Delete '{vault.Name}'?",
$"Everything in it goes: its hosts, keys, passwords, snippets and buckets stop being readable "
+ $"by anybody, including you, and nothing here can undo it.{shared} What it cannot do is "
+ "reach a machine that has already synced this vault — a copy pulled yesterday is still "
+ "there. Rotate the credentials that mattered.");
}
/// <summary>Cancels an armed action.</summary>
[RelayCommand]
private void CancelAction() => PendingAction = null;
@@ -1258,22 +1347,71 @@ internal sealed partial class VaultsViewModel(
PendingAction = null;
await RunAsync(async () =>
await RunAsync(() => request.Kind switch
{
await server.Teams
.TransferTeamOwnershipAsync(
request.TeamId,
new TransferTeamOwnershipRequest(request.MemberId),
cancellationToken)
.ConfigureAwait(true);
await ReloadAsync(cancellationToken).ConfigureAwait(true);
Status = "Handed over. You are an admin of this vault now, and only its new owner can hand "
+ "it on again.";
VaultActionKind.Delete => DeleteAsync(server, request, cancellationToken),
_ => HandOverAsync(server, request, cancellationToken),
}).ConfigureAwait(true);
}
private async Task HandOverAsync(
IVaultServer server,
VaultActionRequest request,
CancellationToken cancellationToken)
{
await server.Teams
.TransferTeamOwnershipAsync(
request.TeamId,
new TransferTeamOwnershipRequest(request.MemberId),
cancellationToken)
.ConfigureAwait(true);
await ReloadAsync(cancellationToken).ConfigureAwait(true);
Status = "Handed over. You are an admin of this vault now, and only its new owner can hand "
+ "it on again.";
}
/// <summary>
/// Deletes the vault the confirmation was armed for.
/// </summary>
/// <remarks>
/// <para>
/// The name is read before the call, because afterwards there is no row to read it from and the
/// sentence this ends with is about a vault that no longer exists.
/// </para>
/// <para>
/// The rest of the shell is told, as a rename tells it: the tab strip's vault menu, the file-this-into
/// picker and every host list are built from the session's vault list, and all of them are a vault out
/// of date the moment one goes.
/// </para>
/// </remarks>
private async Task DeleteAsync(
IVaultServer server,
VaultActionRequest request,
CancellationToken cancellationToken)
{
if (session() is not { } open)
{
Status = "Unlock your keychain first: deleting a vault gives up this machine's key to it.";
return;
}
var name = Vaults.FirstOrDefault(row => row.VaultId == request.VaultId)?.Name ?? "That vault";
var deleted = await open
.DeleteVaultAsync(server.Grants, request.VaultId, cancellationToken)
.ConfigureAwait(true);
await ReloadAsync(cancellationToken).ConfigureAwait(true);
await NotifyVaultsChangedAsync(cancellationToken).ConfigureAwait(true);
Status = deleted
? $"Deleted '{name}'. Everybody's key to it is withdrawn. What anybody had already synced is "
+ "still on their machine — rotate the credentials that mattered."
: $"'{name}' was already gone. Somebody else deleted it, or your access to it ended.";
}
/// <summary>
/// Removes somebody, revoking their grants and rotating the vaults they could read.
/// </summary>
@@ -1629,6 +1767,7 @@ internal sealed partial class VaultsViewModel(
OnPropertyChanged(nameof(OwnsSelected));
OnPropertyChanged(nameof(SelectedIsShared));
OnPropertyChanged(nameof(SelectedIsPersonal));
OnPropertyChanged(nameof(CanDeleteSelected));
OnPropertyChanged(nameof(HasInvitations));
OnPropertyChanged(nameof(SharedMembershipWarning));
OnPropertyChanged(nameof(HasSharedMembershipWarning));
+38
View File
@@ -146,6 +146,44 @@ public sealed class VaultStore(IDbContextFactory<ClientCacheContext> contexts, T
await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false);
}
/// <summary>
/// Removes one vault's row and its wraps, leaving the rest alone.
/// </summary>
/// <remarks>
/// <para>
/// For the vault this machine has just deleted, which is gone here before the server's next
/// <c>/me</c> stops mentioning it — the mirror image of <see cref="UpsertAsync"/>, and needed for the
/// same reason: a screen that went on listing a vault the user has just deleted, until the next
/// refresh, would look as though the deletion had not worked.
/// </para>
/// <para>
/// The wraps go with it, as they do in <see cref="ReplaceAllAsync"/> and for the same reason: they are
/// keys to something that no longer exists. Its <em>items</em> stay, exactly as they stay for a vault
/// whose grant was withdrawn — the ciphertext is on every other member's machine too, and deleting
/// these rows would be the client pretending to a reach it does not have.
/// </para>
/// </remarks>
public async Task RemoveAsync(Guid vaultId, CancellationToken cancellationToken)
{
var context = contexts.CreateDbContext();
await using var scope = context.ConfigureAwait(false);
var row = await context.Set<CachedVaultRow>()
.SingleOrDefaultAsync(r => r.VaultId == vaultId, cancellationToken)
.ConfigureAwait(false);
if (row is null)
{
return;
}
context.Remove(row);
await RemoveWrapsAsync(context, vaultId, cancellationToken).ConfigureAwait(false);
await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false);
}
/// <summary>Reads the vaults this machine has been asked to leave off the screens.</summary>
/// <remarks>
/// The ids alone: a caller wanting the names already has <see cref="ListAsync"/>, and a list that
+34
View File
@@ -228,6 +228,40 @@ public sealed class VaultKeyring : IDisposable
}
}
/// <summary>
/// Drops every key to one vault, zeroing them.
/// </summary>
/// <remarks>
/// <para>
/// For a vault that has been deleted, which is the one case where this session is not merely unable to
/// read a vault but has no vault to read. <see cref="MarkUnreadable"/> is the other shape and keeps the
/// keys deliberately — the items sealed under them are still there and still wanted — whereas here
/// there is nothing left for them to open and holding them would be keeping key material for something
/// that no longer exists.
/// </para>
/// <para>
/// It is also taken off the unopened list, which is what stops a deleted vault being reported for ever
/// as one this machine is waiting for a key to.
/// </para>
/// </remarks>
public void Forget(Guid vaultId)
{
ObjectDisposedException.ThrowIf(disposed, this);
generations.Remove(vaultId);
Unopened = [.. Unopened.Where(id => id != vaultId)];
if (!keys.Remove(vaultId, out var held))
{
return;
}
foreach (var key in held.Values)
{
CryptographicOperations.ZeroMemory(key);
}
}
/// <summary>
/// Borrows a vault's current key: the one new items are sealed under.
/// </summary>
+4 -4
View File
@@ -112,10 +112,10 @@ public static class ProblemCodes
/// A team cannot be archived while it still owns vaults.
/// </summary>
/// <remarks>
/// Its own code because the remedy is neither "fix what you typed" nor "pick another value": it is
/// to deal with the vaults first. Archiving anyway would hide vaults from every member including
/// the ones holding keys to them, and this product has no way to delete a vault, so the refusal is
/// the honest end of that road rather than a step on it.
/// Its own code because the remedy is neither "fix what you typed" nor "pick another value": it is to
/// deal with the vaults first, by deleting each of them — <c>DELETE /api/v1/vaults/{vaultId}</c> — which
/// asks about each one on the way. Archiving anyway would hide vaults from every member including the
/// ones holding keys to them, which is the same removal without anybody having been asked.
/// </remarks>
public const string TeamNotEmpty = "team-not-empty";
@@ -104,6 +104,11 @@ public sealed class EndpointInventoryTests(ApiFixture fixture)
// reason. Gated on Admin inside the handler, which this table cannot see.
"PUT /api/v1/vaults/{vaultId:guid} name=RenameVault tags=Vaults policies=Authenticated anon=False",
// Authenticated for the same reason as the rename it sits beside — deleting withdraws grants
// rather than wrapping anything, so it needs no key of the caller's — and gated on Admin inside
// the handler, which this table cannot see. It refuses a personal vault there too.
"DELETE /api/v1/vaults/{vaultId:guid} name=DeleteVault tags=Vaults policies=Authenticated anon=False",
"GET /api/v1/vaults/{vaultId:guid}/grants name=ListVaultGrants tags=Vaults policies=Enrolled anon=False",
"POST /api/v1/vaults/{vaultId:guid}/grants name=IssueVaultGrant tags=Vaults policies=Enrolled anon=False",
"DELETE /api/v1/vaults/{vaultId:guid}/grants/{userId:guid} name=RevokeVaultGrant tags=Vaults policies=Enrolled anon=False",
@@ -320,6 +320,132 @@ public sealed class TeamEndpointTests(ApiFixture fixture)
listed.Single(row => row.TeamId == team.TeamId).Name.ShouldBe("Platform Engineering");
}
/// <remarks>
/// <para>
/// Deleting a vault, and the three things it has to be true of at once: it leaves everybody's list,
/// every key to it is withdrawn, and the team made to carry it goes with it. The last is the mirror of
/// the rename above — that team was never shown to anybody, so leaving it behind would leave a
/// membership list nothing in the interface can reach or remove.
/// </para>
/// <para>
/// The member's list is checked as well as the owner's, because "gone" that is only true for the
/// person who pressed it is the failure worth catching.
/// </para>
/// </remarks>
[Fact]
public async Task DeletingAVault_TakesItFromEveryMemberAndArchivesTheTeamBehindIt()
{
var owner = await EnrolledClientAsync("vault-delete-owner", "vdowner@example.com");
var member = await EnrolledClientAsync("vault-delete-member", "vdmember@example.com");
var team = await CreateTeamAsync(owner, "Short lived");
var vaultId = await CreateVaultAsync(owner, team.TeamId);
var entry = await LookupAsync(owner, "vdmember@example.com");
await AddMemberAsync(owner, team.TeamId, entry.UserId, TeamMemberRole.Member);
await owner.PostContractAsync(
$"/api/v1/vaults/{vaultId}/grants", GrantRequest(entry, generation: 1));
(await ReadAsync<MeResponse>(member, MeUrl)).Vaults
.ShouldContain(vault => vault.VaultId == vaultId);
var response = await DeleteAsync(owner, VaultUrl(vaultId));
response.StatusCode.ShouldBe(HttpStatusCode.NoContent);
(await ReadAsync<MeResponse>(owner, MeUrl)).Vaults
.ShouldNotContain(vault => vault.VaultId == vaultId);
(await ReadAsync<MeResponse>(member, MeUrl)).Vaults
.ShouldNotContain(vault => vault.VaultId == vaultId);
// Gone the way a vault is gone, rather than merely unlisted: every call naming it answers 404.
(await GetAsync(owner, $"/api/v1/vaults/{vaultId}/grants"))
.StatusCode.ShouldBe(HttpStatusCode.NotFound);
(await ReadAsync<IReadOnlyList<TeamSummary>>(owner, TeamsUrl))
.ShouldNotContain(row => row.TeamId == team.TeamId, "the team existed to carry that vault");
}
/// <remarks>
/// 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
/// one did not take its neighbour.
/// </remarks>
[Fact]
public async Task DeletingOneOfSeveralVaults_LeavesTheTeamAndTheOtherVaultAlone()
{
var owner = await EnrolledClientAsync("vault-delete-shared-owner");
var team = await CreateTeamAsync(owner, "Platform Engineering");
var vaultId = await CreateVaultAsync(owner, team.TeamId);
var kept = await CreateVaultAsync(owner, team.TeamId);
(await DeleteAsync(owner, VaultUrl(vaultId))).StatusCode.ShouldBe(HttpStatusCode.NoContent);
var me = await ReadAsync<MeResponse>(owner, MeUrl);
me.Vaults.ShouldNotContain(vault => vault.VaultId == vaultId);
me.Vaults.ShouldContain(vault => vault.VaultId == kept);
(await ReadAsync<IReadOnlyList<TeamSummary>>(owner, TeamsUrl))
.ShouldContain(row => row.TeamId == team.TeamId);
}
/// <remarks>
/// The one vault nothing can delete. It is created by enrollment rather than by anybody choosing to
/// make it, everything filed nowhere else lives in it, and there is no call that would make another —
/// so deleting one would leave an enrolled account with a key and nowhere to put anything.
/// </remarks>
[Fact]
public async Task DeletingThePersonalVault_IsRefused()
{
var owner = await EnrolledClientAsync("vault-delete-personal");
var me = await ReadAsync<MeResponse>(owner, MeUrl);
var personal = me.Vaults.Single(vault => vault.IsPersonal);
var response = await DeleteAsync(owner, VaultUrl(personal.VaultId));
await ShouldBeProblemAsync(
response, HttpStatusCode.BadRequest, ProblemCodes.InvalidVaultGrant);
(await ReadAsync<MeResponse>(owner, MeUrl)).Vaults
.ShouldContain(vault => vault.VaultId == personal.VaultId);
}
/// <remarks>
/// Admin, like the rename, and for a stronger version of the same reason: this takes the vault from
/// everybody in it at once. A member is refused with 403 rather than 404 because the vault is visible
/// to them, so naming the reason leaks nothing — and an outsider still gets 404, which
/// <c>IVaultAccessService</c> requires.
/// </remarks>
[Fact]
public async Task APlainMember_CannotDeleteAVaultTheyCanWriteTo()
{
var owner = await EnrolledClientAsync("vault-delete-limits-owner", "vdlowner@example.com");
var member = await EnrolledClientAsync("vault-delete-limits-member", "vdlmember@example.com");
var outsider = await EnrolledClientAsync("vault-delete-limits-outsider");
var team = await CreateTeamAsync(owner, "Limits");
var vaultId = await CreateVaultAsync(owner, team.TeamId);
var entry = await LookupAsync(owner, "vdlmember@example.com");
await AddMemberAsync(owner, team.TeamId, entry.UserId, TeamMemberRole.Member);
await ShouldBeProblemAsync(
await DeleteAsync(member, VaultUrl(vaultId)),
HttpStatusCode.Forbidden,
ProblemCodes.Forbidden);
(await DeleteAsync(outsider, VaultUrl(vaultId))).StatusCode
.ShouldBe(HttpStatusCode.NotFound);
(await ReadAsync<MeResponse>(owner, MeUrl)).Vaults
.ShouldContain(vault => vault.VaultId == vaultId);
}
/// <remarks>
/// Admin rather than Write, and the line is the one the team rename draws: a name is what everybody in
/// the vault sees it called, so a member who may add hosts to it may not rename it out from under them.
@@ -552,6 +552,7 @@ public sealed class ScreenLayoutTests : IAsyncLifetime
foreach (var section in new[]
{
VaultSection.All, VaultSection.Keys, VaultSection.Credentials,
VaultSection.Tags, VaultSection.Buckets,
})
{
vault.Section = section;
@@ -603,6 +604,22 @@ public sealed class ScreenLayoutTests : IAsyncLifetime
await MeasureVaultAsync(faults => faults.ShouldBeEmpty());
}
/// <remarks>
/// The S3 screen before there is a bucket to open, which is the state every new account starts in and
/// the state whose contents changed: a heading, a paragraph and a button where an empty picker used to
/// be. The paragraph is the risk — it is what says a bucket is a keychain item — and it sits in the
/// 320-pixel invitation column with no scroll viewer above it.
/// </remarks>
[Fact]
public async Task TheS3ScreenFitsWithNoBucketsToOpen()
{
transfers.Remote = RemoteKind.Bucket;
transfers.ShowsNoBuckets.ShouldBeTrue("this vault has no buckets in it");
await MeasureTransfersAsync(faults => faults.ShouldBeEmpty("with nothing to open yet"));
}
/// <remarks>
/// Both drop highlights forced on at once, which is a state the screen never actually reaches — the
/// point is that an overlay covering a whole pane does not change the layout of anything beneath it.
@@ -249,6 +249,10 @@ internal sealed class StubTeamServer : IVaultServer, ITeamApi, IVaultGrantApi
UpdateVaultRequest request,
CancellationToken cancellationToken) => throw new NotSupportedException();
/// <inheritdoc />
public Task<bool> DeleteVaultAsync(Guid vaultId, CancellationToken cancellationToken) =>
throw new NotSupportedException();
/// <inheritdoc />
public Task IssueVaultGrantAsync(
Guid vaultId,
@@ -156,6 +156,89 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
return userId;
}
/// <summary>
/// Puts a vault somebody else made, and shared with this account, on the server.
/// </summary>
/// <remarks>
/// <para>
/// The other half of sharing, which no test can otherwise reach: every vault in this suite is one
/// this client made, and a vault this client made is one it already holds the key to. What arrives
/// on the machine somebody shared <em>with</em> is different — a vault that appears in <c>/me</c>
/// out of nowhere, with a key wrapped to this account by a client this one never spoke to.
/// </para>
/// <para>
/// The wrap is real, made against the encryption key this account enrolled, so the keyring opens it
/// exactly as it opens one from a real colleague. A helper that filled the field with bytes would
/// let a vault appear in the list and never prove it could be read.
/// </para>
/// </remarks>
/// <param name="name">What the vault is called.</param>
/// <param name="sharedBy">The account that made it, from <see cref="AddAccount"/>.</param>
/// <returns>The vault's id.</returns>
internal Guid ShareVaultWithMe(string name, Guid sharedBy)
{
if (statement is not { } enrolled)
{
throw new InvalidOperationException(
"Nothing can be wrapped to this account until it has enrolled a key.");
}
var teamId = Guid.CreateVersion7();
var vaultId = Guid.CreateVersion7();
var vaultKey = VaultKeys.Create();
var wrapped = VaultKeys.WrapTo(vaultKey, enrolled.EncryptionPublicKey, vaultId, 1);
teams.Add(new TeamSummary(
teamId,
name,
name.ToLowerInvariant().Replace(' ', '-'),
Description: null,
// A member rather than an owner: somebody else made this and this account was added to it,
// which is what decides whether the screen offers to rename or remove it.
TeamMemberRole.Member,
MemberCount: 2,
VaultCount: 1,
DateTimeOffset.UnixEpoch));
var sharer = accounts.Find(account => account.UserId == sharedBy);
members[teamId] =
[
Member(sharedBy, sharer.Email, sharer.DisplayName, TeamMemberRole.Owner),
Member(UserId, "alice@example.com", "Alice Example", TeamMemberRole.Member),
];
teamVaults[vaultId] = new VaultSummary(
vaultId,
name,
IsPersonal: false,
TeamId: teamId,
KeyGeneration: 1,
Permissions: 31,
wrapped,
RekeyRequired: false);
return vaultId;
}
/// <summary>One active, enrolled member, which is the only kind this helper makes.</summary>
private static TeamMemberSummary Member(
Guid userId,
string email,
string displayName,
TeamMemberRole role) =>
new(
userId,
email,
displayName,
role,
TeamMemberStatus.Active,
IsEnrolled: true,
DateTimeOffset.UnixEpoch,
DateTimeOffset.UnixEpoch);
/// <inheritdoc />
public Task<IReadOnlyList<TeamSummary>> ListTeamsAsync(CancellationToken cancellationToken) =>
Task.FromResult<IReadOnlyList<TeamSummary>>([.. teams]);
@@ -583,6 +666,43 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
return Task.FromResult(renamed);
}
/// <inheritdoc />
/// <remarks>
/// Every grant to the vault goes with it, as the real service withdraws them in the same write, and the
/// team behind it is archived when it owns nothing else — the second half of what the endpoint does.
/// A fake that kept either would let a test assert a deletion that had left the vault readable, or
/// leave the vaults screen listing a membership list with no vault under it.
/// </remarks>
public Task<bool> DeleteVaultAsync(Guid vaultId, CancellationToken cancellationToken)
{
if (personalVault is { } personal && personal.VaultId == vaultId)
{
throw new DodoSshApiException(
System.Net.HttpStatusCode.BadRequest,
ProblemCodes.InvalidVaultGrant,
"A personal vault cannot be deleted.");
}
if (!teamVaults.Remove(vaultId, out var vault))
{
return Task.FromResult(false);
}
foreach (var key in grants.Keys.Where(key => key.VaultId == vaultId).ToList())
{
grants.Remove(key);
}
if (vault.TeamId is { } teamId && !teamVaults.Values.Any(other => other.TeamId == teamId))
{
teams.RemoveAll(team => team.TeamId == teamId);
members.Remove(teamId);
invitations.Remove(teamId);
}
return Task.FromResult(true);
}
/// <inheritdoc />
public Task<IReadOnlyList<DirectoryEntry>> LookupByEmailAsync(
string email,
@@ -5586,6 +5586,50 @@ public sealed class ShellFlowTests : IAsyncLifetime
shell.Transfers.IsChoosingRemote.ShouldBeFalse("a form offering a choice between nothing");
}
/// <remarks>
/// <para>
/// The S3 screen with nothing in the keychain, which is where every new account starts and which used to
/// be a dead end: SELECT BUCKET opened a combo box with nothing in it, and nothing anywhere said that a
/// bucket is made on the keychain screen. Somebody standing here had arrived at the place a bucket is
/// used and been shown no route to the place one is created — which is indistinguishable from an
/// application that cannot add one at all.
/// </para>
/// <para>
/// The assertion that matters is the last pair: pressing the button lands on the keychain <em>with the
/// editor already open</em>. Navigating to the screen and leaving the user to find + BUCKET among five
/// buttons would be most of the same problem.
/// </para>
/// </remarks>
[Fact]
public async Task TheS3ScreenWithNoBuckets_SaysWhereOneIsMadeAndGoesThere()
{
var vault = await ReadyToConnectAsync();
shell.Transfers.Attach(vault, knownHosts);
shell.ShowFilesCommand.Execute(RemoteKind.Bucket);
shell.Transfers.ShowsNoBuckets.ShouldBeTrue("nothing has been added to the keychain");
shell.Transfers.ShowsBucketChoice.ShouldBeFalse("there is nothing to choose between");
shell.Transfers.AddBucketCommand.Execute(null);
shell.Screen.ShouldBe(ShellScreen.Keychain);
vault.IsEditingObjectStore.ShouldBeTrue("the route has to land on the editor, not near it");
vault.BucketEditorLabel = "Backups";
vault.BucketEditorBucket = "backups";
vault.BucketEditorAccessKeyId = "AKIAEXAMPLE";
vault.BucketEditorSecretAccessKey = "a-secret-access-key";
vault.BucketEditorRegion = "eu-west-1";
await vault.SaveObjectStoreCommand.ExecuteAsync(null);
shell.ShowFilesCommand.Execute(RemoteKind.Bucket);
shell.Transfers.ShowsNoBuckets.ShouldBeFalse(vault.Status);
shell.Transfers.ShowsBucketChoice.ShouldBeTrue("the bucket that was just made is the one to open");
}
private async Task UnlockedAsync()
{
await EnrolledAndConfirmedAsync();
@@ -345,6 +345,124 @@ public sealed class VaultSharingTests : IAsyncLifetime
vaults.Members.ShouldHaveSingleItem().Role.ShouldBe("OWNER");
}
/// <remarks>
/// <para>
/// Sharing from the receiving end, which is the half that has to happen on somebody else's machine and
/// the half that was missing. A vault wrapped to this account appears in <c>/me</c> and nowhere else —
/// there is no push channel — so a client that never re-read that list showed nothing, indefinitely,
/// while the server and the grant were both perfectly correct.
/// </para>
/// <para>
/// Readable rather than merely listed, because those are two different failures with the same symptom:
/// a row that cannot be opened is a vault whose key never arrived, and this asserts the wrap was taken
/// into the keyring. The switch is asserted too — it is built by the shell rather than by the vault, so
/// it is the one thing a pass could admit a vault without redrawing.
/// </para>
/// </remarks>
[Fact]
public async Task AVaultSomebodyElseShared_ArrivesOnTheNextSynchronisation()
{
await UnlockedAsync();
var colleague = server.AddAccount("bob@example.com", "Bob Example");
var vaultId = server.ShareVaultWithMe("Platform secrets", colleague);
var vault = shell.Vault.ShouldNotBeNull();
await vault.SyncCommand.ExecuteAsync(null);
vault.Session.ReadableVaults.ShouldContain(
row => row.VaultId == vaultId,
"a vault shared with this account arrives on a synchronisation pass, with its key");
shell.VaultToggles.ShouldContain(
toggle => toggle.VaultId == vaultId,
"the tab strip's vault menu is built by the shell and has to be told");
await shell.Vaults.LoadAsync(Token);
var row = shell.Vaults.Vaults.Single(vault => vault.VaultId == vaultId);
row.IsReadable.ShouldBeTrue(shell.Vaults.Status);
row.IsOwned.ShouldBeFalse("somebody else made this one");
}
/// <remarks>
/// <para>
/// Deleting a shared vault, which is an admin's operation and the only one on this screen that cannot
/// be undone. It has to take three things with it: the vault, everybody's key to it — including the
/// people it was shared with — and this machine's own copy of the row, so the list is right before the
/// next refresh rather than after it.
/// </para>
/// <para>
/// The status line is asserted for what it says about the limit rather than for its wording. A message
/// implying that deletion reaches a colleague's laptop would be the one dishonest sentence this screen
/// could print; see ADR 0001.
/// </para>
/// </remarks>
[Fact]
public async Task DeletingAVault_TakesItAndEverybodysKeyToIt()
{
await UnlockedAsync();
var vaults = shell.Vaults;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateVaultAsync(vaults, "Platform secrets");
var vaultId = vaults.SelectedVault!.VaultId;
vaults.InviteEmail = "bob@example.com";
await vaults.AddMemberCommand.ExecuteAsync(null);
server.IssuedGrants.ShouldContainKey((vaultId, colleague));
vaults.CanDeleteSelected.ShouldBeTrue("an admin may delete a shared vault");
vaults.DeleteVaultCommand.Execute(null);
var question = vaults.PendingAction.ShouldNotBeNull("deletion is never carried out unasked");
question.Consequence.ShouldContain(
"already synced", Case.Insensitive, "the one limit this must not leave implied");
await vaults.ConfirmActionCommand.ExecuteAsync(null);
vaults.Vaults.ShouldNotContain(row => row.VaultId == vaultId, vaults.Status);
server.IssuedGrants.ShouldNotContainKey((vaultId, colleague));
shell.Vault!.Session.Vaults.ShouldNotContain(
row => row.VaultId == vaultId,
"the machine that deleted it does not wait for a refresh to stop listing it");
shell.VaultToggles.ShouldNotContain(toggle => toggle.VaultId == vaultId);
}
/// <remarks>
/// The one vault deletion cannot reach, refused by the screen rather than by the server: everything
/// filed nowhere else lives in it and nothing can make another, so the button is not offered and the
/// command says why if something reaches it anyway.
/// </remarks>
[Fact]
public async Task ThePersonalVault_CannotBeDeleted()
{
await UnlockedAsync();
var vaults = shell.Vaults;
await vaults.LoadAsync(Token);
vaults.SelectedVault = vaults.Vaults.Single(vault => vault.IsPersonal);
vaults.CanDeleteSelected.ShouldBeFalse();
vaults.DeleteVaultCommand.Execute(null);
vaults.PendingAction.ShouldBeNull("nothing was armed");
vaults.Status.ShouldContain("cannot be deleted");
vaults.Vaults.ShouldContain(vault => vault.IsPersonal);
}
/// <remarks>
/// The personal vault is in the list, is marked as the one thing it is, and offers nothing to share:
/// the server refuses a grant on one outright, so a screen that let somebody try would be sending them