Files
DodoSSH/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs
T
jaap-janandClaude Opus 5 16e0051e89
ci / build and test (push) Successful in 1m22s
ci / android head (push) Failing after 5s
ci / api image (push) Successful in 52s
Draw the tags that have been storable and invisible since the domain landed
`Tag` has been a full item kind for three commits — a table, a migration, a
codec, a merge, a cipher — and `HostSecret.TagIds` has merged per tag so two
people tagging one host both keep theirs. Nothing drew a chip. The tags a
client could store were ones nothing here could see.

Chips on host rows, both heads, from names resolved through the tag list rather
than ids: a tag that does not resolve is left out rather than drawn, because it
means the tag was deleted elsewhere or belongs to a vault this session cannot
read, and a host with one chip fewer is the honest answer where a host wearing
a GUID is not. The id stays on the host, so the chip comes back if the tag does.

The picker is chips that toggle, matching the chips on the row behind it. A
list of names to tick would make the user match an entry to a chip they can see
two inches away. The box under it creates a tag and puts it on straight away,
because that is when a tag is usually wanted — while tagging a host and finding
it does not exist yet. Unlike every other field in that editor it writes to the
keychain immediately, since a host can only name an id that exists; cancelling
therefore leaves the tag behind, which is honest rather than hidden. A name
that already exists is used rather than repeated: two tags called "staging" are
storable and must stay storable, because two people creating one offline is how
it happens, but typing it into a box beside a chip of the same name is a slip.

Renaming and deleting needed a home, or the picker fills with names nobody uses
and never empties. That home is a TAGS category on the keychain screen, where
every other item kind is managed — and renaming is the whole reason a tag is an
item rather than a string repeated inside twenty payloads: it is one write, and
no host is touched. The delete confirmation counts the hosts wearing it, which
is the difference between a tidy-up and losing a filter somebody relies on.

The desktop host editor now scrolls, and that is not a tidy-up. A picker's
height is a chip per tag in the keychain, wrapped, so somebody with fifteen
tags has an editor half again as tall as somebody with three; no fixed height
holds that, and trimming other fields to buy room only moves the failure to
whoever has sixteen. The layout suite caught it the moment its seeder grew tags
— which is why the seeder now creates ten rather than three, enough to drive
the pane onto its cap so the capped shape is what gets measured rather than one
no real keychain produces. The cost is named where it is paid: the harness
skips anything inside a ScrollViewer, so from here it certifies that pane fits
the column rather than that every field in it does.

Two smaller things fell out. Five buttons overflowed the keychain header by a
few pixels, so GENERATE lost the word KEY — its tooltip carries what the word
did. And TotalItemCount had been counting keys and credentials while ALL showed
four kinds; it counts all five now, because a number under a chip that
disagrees with the rows it opens is worse than no number.

An adversarial review of this change found two defects it had introduced, both
green against the full suite. NewTag filed into the "new items go to" picker
while the tag list only ever holds the active vault's — so with a team vault
selected a tag would be created, queued for push, reported as added, and then
invisible, with no row, no count, no picker entry and nothing able to rename or
delete it, because there is no active-vault switcher to go and find it with.
The comment on the host editor's own create path states that exact rule; this
was the one place that broke it, and NewObjectStore, whose list is likewise
active-vault-only, already ignored the picker. And the tag editor was the only
one of five that did not disarm a pending deletion when it opened, so arming a
key's deletion and then pressing + TAG left a live DELETE for an item the user
was no longer looking at, directly above the boxes they were typing into. Both
are fixed, both have a test, and the first was checked against the broken
version before being kept.

The same review caught a doc comment that had been inserted between
SnippetRowViewModel's summary and its declaration, silently taking it over.

Verified by the whole suite on a clean build: 1413 tests over nineteen
projects, none failing. Both heads build. The rectangles the layout suite
cannot reach are phase 9 of docs/manual-checks.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-03 12:42:43 +02:00

312 lines
12 KiB
C#

using DodoSSH.Client.Session;
// FakeDeviceKeyStore is compiled into this assembly from a source link and keeps its original namespace;
// see the csproj for why it is shared rather than reimplemented.
using DodoSSH.Client.Session.Tests;
using DodoSSH.Client.Shell.ViewModels;
using DodoSSH.Client.Ssh;
using DodoSSH.Client.Storage;
using DodoSSH.Client.Terminal;
using DodoSSH.Crypto;
namespace DodoSSH.Client.App.Tests;
/// <summary>
/// Teams, from the side that holds the keys: create one, add somebody, and wrap a vault key to them.
/// </summary>
/// <remarks>
/// <para>
/// The reason this suite exists rather than leaving teams to the server's own tests is that the
/// interesting half is not on the server. Adding a member is a row; <b>sharing is a decision the client
/// makes about whether to trust a public key the server just handed it</b>, and that decision is what
/// stands between an end-to-end encrypted vault and one the operator can read by answering a directory
/// lookup with a key of their own.
/// </para>
/// <para>
/// So the fake server keeps a real key log — chained with the same <c>KeyLogChain</c> the server uses —
/// and can be told to corrupt it. A test that only ever saw a well-formed log would be checking that
/// sharing works, not that verification does.
/// </para>
/// </remarks>
public sealed class TeamSharingTests : IAsyncLifetime
{
private const string Passphrase = "a sufficiently long passphrase";
private static readonly Argon2Profile CheapProfile =
Argon2Profile.FromStoredParameters(memoryKibibytes: 8 * 1024, passes: 1, parallelism: 1);
private readonly FakeVaultServer server = new();
private readonly FakeSshConnectionFactory ssh = new();
private string directory = null!;
private ClientCacheFactory caches = null!;
private TerminalWorkspace workspace = null!;
private VaultKnownHostStore knownHosts = null!;
private FakeDeviceKeyStore deviceKeys = null!;
private MainWindowViewModel shell = null!;
private static CancellationToken Token => TestContext.Current.CancellationToken;
/// <inheritdoc />
public ValueTask InitializeAsync()
{
directory = Path.Combine(Path.GetTempPath(), $"dodossh-teams-{Guid.CreateVersion7():N}");
var paths = new ClientPaths(directory);
caches = ClientCacheFactory.ForFile(paths.CacheFile);
knownHosts = new VaultKnownHostStore();
deviceKeys = new FakeDeviceKeyStore();
workspace = new TerminalWorkspace(
new InMemoryTerminalAssetProvider(
new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)),
ssh,
TimeProvider.System);
shell = new MainWindowViewModel(
paths,
caches,
workspace,
knownHosts,
deviceKeys,
(_, _) => Task.FromResult<IVaultServer>(server),
TimeProvider.System,
NSubstitute.Substitute.For<ISftpSessionFactory>(),
CheapProfile);
return ValueTask.CompletedTask;
}
/// <inheritdoc />
public async ValueTask DisposeAsync()
{
await shell.DisposeAsync();
knownHosts.Close();
await workspace.DisposeAsync();
caches.Dispose();
try
{
Directory.Delete(directory, recursive: true);
}
catch (IOException)
{
// A cache file the process has not finished releasing. The directory is under the temp path
// and named per run, so leaving it costs a few kilobytes and never collides.
}
}
/// <remarks>
/// The whole point of a team, in one test. Note what the status line says after the add and before
/// the share: adding somebody grants them nothing readable, and the interface has to say so rather
/// than let a user believe the credential is already with their colleague.
/// </remarks>
[Fact]
public async Task CreatingATeamAndSharingItsVault_WrapsTheKeyToTheOtherMember()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
teams.Vaults.Count.ShouldBe(1, teams.Status);
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.Members.Count.ShouldBe(2, teams.Status);
teams.Status.ShouldContain("cannot read anything yet");
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.SelectedVault = teams.Vaults[0];
await teams.ShareVaultCommand.ExecuteAsync(null);
var vaultId = teams.Vaults[0].VaultId;
server.IssuedGrants.ShouldContainKey((vaultId, colleague));
teams.Status.ShouldContain("Shared");
// The one thing verification cannot promise, said in the same breath as the success.
teams.Status.ShouldContain("fingerprint", Case.Insensitive);
}
/// <remarks>
/// <para>
/// The test this whole design exists for. A server that wants to read a team's vault only has to
/// answer one directory lookup with a key it holds the private half of — so the client reads the
/// append-only key log, verifies its chain, and refuses to wrap anything unless the key it was
/// offered is in there unchanged.
/// </para>
/// <para>
/// Nothing may be sent. A refusal that still issued the grant, or that issued it on a retry, would be
/// worse than no check at all, because the interface would have said it was verified.
/// </para>
/// </remarks>
[Fact]
public async Task ATamperedKeyLog_StopsTheShareRatherThanWarningAboutIt()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("mallory@example.com", "Mallory Example");
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
teams.InviteEmail = "mallory@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.SelectedVault = teams.Vaults[0];
server.CorruptKeyLog = true;
await teams.ShareVaultCommand.ExecuteAsync(null);
server.IssuedGrants.ShouldBeEmpty();
teams.Status.ShouldContain("Did not share");
teams.Status.ShouldContain("key log");
}
/// <remarks>
/// A vault created here is usable here, without a relock. The key was generated in this process, so
/// making the user lock and unlock to reach the vault they just made would be asking them to work
/// around bookkeeping.
/// </remarks>
[Fact]
public async Task ATeamVaultCreatedHere_IsImmediatelyReadableAndWritable()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
var vaultId = teams.Vaults[0].VaultId;
var session = shell.Vault!.Session;
session.ReadableVaults.Select(vault => vault.VaultId).ShouldContain(vaultId);
// And it is offered as somewhere to file a new item, which is what makes it worth having.
await shell.Vault.LoadAsync(Token);
shell.Vault.TargetVaults.Select(choice => choice.VaultId).ShouldContain(vaultId);
shell.Vault.HasVaultChoice.ShouldBeTrue();
}
/// <remarks>
/// Filing into a team vault has to be chosen and has to stick. The bug this guards is the obvious
/// one: an editor that read the picker at save time rather than at open time, so changing the picker
/// with a half-typed host on screen would move it.
/// </remarks>
[Fact]
public async Task AHostFiledIntoATeamVault_StaysThere()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
var vault = shell.Vault!;
var teamVaultId = teams.Vaults[0].VaultId;
await vault.LoadAsync(Token);
vault.SelectedTargetVault =
vault.TargetVaults.Single(choice => choice.VaultId == teamVaultId);
vault.NewHostCommand.Execute(null);
vault.EditorLabel = "prod-db";
vault.EditorHostname = "db.internal";
vault.EditorUsername = "deploy";
// Moved back after the editor opened. The host must still land in the team's vault.
vault.SelectedTargetVault =
vault.TargetVaults.First(choice => choice.VaultId != teamVaultId);
await vault.SaveHostCommand.ExecuteAsync(null);
var row = vault.Hosts.Single(
host => string.Equals(host.Label, "prod-db", StringComparison.Ordinal));
row.VaultId.ShouldBe(teamVaultId);
}
/// <remarks>
/// The mirror image of the host test above, and it goes the other way on purpose. A host filed into a
/// team vault has to stay there, because hosts are read across every readable vault and so come back.
/// Tags are not — the editable list is the active vault's alone, like groups and buckets — so a tag
/// filed anywhere else would be created, pushed, reported as added and then invisible, with nothing on
/// the keychain screen able to rename or delete it and no active-vault switcher to go and find it with.
/// </remarks>
[Fact]
public async Task ATagIgnoresTheTargetPicker_BecauseItsListOnlyEverShowsOneVault()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
var teamVaultId = teams.Vaults[0].VaultId;
var vault = shell.Vault!;
await vault.LoadAsync(Token);
vault.HasVaultChoice.ShouldBeTrue("this test is meaningless with one vault");
vault.SelectedTargetVault = vault.TargetVaults.Single(
choice => choice.VaultId == teamVaultId);
vault.NewTagCommand.Execute(null);
vault.TagEditorLabel = "eu-west-1";
await vault.SaveTagCommand.ExecuteAsync(null);
vault.Tags.ShouldHaveSingleItem().Label
.ShouldBe("eu-west-1", "a tag that is not in the list is a tag nothing can reach");
}
private async Task CreateTeamAsync(TeamsViewModel teams, string name, string slug)
{
await teams.LoadAsync(Token);
teams.NewTeamCommand.Execute(null);
teams.NewTeamName = name;
teams.NewTeamSlug = slug;
await teams.CreateTeamCommand.ExecuteAsync(null);
teams.SelectedTeam.ShouldNotBeNull(teams.Status);
}
/// <remarks>
/// The whole path rather than a shortcut into the unlocked state, because sharing needs an identity
/// key that was really enrolled: the fake server publishes it into its key log during enrollment, and
/// that entry is what the client verifies its own directory answer against.
/// </remarks>
private async Task UnlockedAsync()
{
await shell.StartAsync(Token);
await shell.SignInCommand.ExecuteAsync(null);
shell.Passphrase = Passphrase;
shell.ConfirmPassphrase = Passphrase;
await shell.EnrollCommand.ExecuteAsync(null);
shell.RecoveryCodeWrittenDown = true;
shell.ConfirmRecoveryCodeCommand.Execute(null);
shell.Passphrase = Passphrase;
await shell.UnlockCommand.ExecuteAsync(null);
shell.State.ShouldBe(ShellState.Unlocked, shell.StatusMessage);
}
}