Files
DodoSSH/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs
T
jaap-jan a2c56de1f2 Read the selected team once per reload, so its owner is listed once
Creating a team drew its owner twice. ReloadAsync rebuilds the team list and
then reselects, and the selection handler answers that assignment by starting
its own read of the members, invitations and vaults — while ReloadAsync is
awaiting a read of exactly the same thing. Both clear the collections up front
and both append when their round trip answers, so everything below the team list
was drawn twice. On a team nobody has been added to yet, whose only member is its
owner, that read as the owner being in the team twice.

The rows are records, so the reselect only raises a change when something about
the team has actually moved — which is every reload that follows a change:
creating a team, adding a member, renaming one. A plain refresh looked fine, and
the screen doubled precisely after the acts people come to this screen to
perform.

isReselecting suppresses the handler for the length of the assignment rather
than deduplicating rows afterwards, because only one of the two reads is
awaited. A command that reloads and then reads Members has to be looking at the
reload's own read and not at a fire-and-forget one that may not have answered.

The generation counter is the other half, and it is a different bug with the
same cause: selecting a second team before the first has answered leaves two
reads in flight against the same collections and nothing decides which wins, so
team A's members could land in the list under team B's name. A superseded read
now drops its answer instead of appending it.

Why nothing caught this. The fake server answers from memory, so every read
completes before the next begins and the appends can never interleave — the
duplicate needs a round trip to hold two reads open at once. FakeVaultServer
grows a MemberReadGate for that, and the new test holds a read open and counts
the reads in flight, which is the only moment a second one is distinguishable
from the first. It fails against the old behaviour with two. 237 tests pass in
the app suite and 83 in the layout suite.
2026-08-03 16:26:52 +02:00

610 lines
23 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.Contracts;
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");
}
/// <remarks>
/// The screen's answer to "who can actually open this", which until now it could not give at all —
/// the endpoint existed and nothing called it. Asserted after a share rather than before, because
/// an empty list proves nothing about whether the call was made.
/// </remarks>
[Fact]
public async Task SelectingATeamVault_ListsWhoHoldsAKeyToIt()
{
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.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.SelectedVault = teams.Vaults[0];
await teams.ShareVaultCommand.ExecuteAsync(null);
// Selecting the vault again is what drives the read; the share above happened after the
// previous selection had already loaded an empty list.
teams.SelectedVault = null;
teams.SelectedVault = teams.Vaults[0];
var holder = teams.Grants.ShouldHaveSingleItem();
holder.UserId.ShouldBe(colleague);
holder.IsLive.ShouldBeTrue(teams.Status);
holder.State.ShouldBe("holds a key");
}
/// <remarks>
/// A role change is authorization only. The status line has to say so, because the obvious reading
/// of "demoted to viewer" is that they can no longer read the vault — and they still can, with the
/// key they were already wrapped. Withdrawing that is a separate act.
/// </remarks>
[Fact]
public async Task ChangingAMembersRole_SaysItDoesNotTakeBackTheKeyTheyHold()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateTeamAsync(teams, "Platform", "platform");
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
await teams.ChangeRoleCommand.ExecuteAsync(TeamMemberRole.Admin);
teams.Members.Single(member => member.UserId == colleague).Role.ShouldBe("ADMIN");
teams.Status.ShouldContain("does not withdraw a vault key");
}
/// <remarks>
/// The owner's role is the one that cannot be changed this way, and the interface has to refuse it
/// itself rather than letting the server do it: a button that produced a server error would be
/// reporting a rule the screen already knew.
/// </remarks>
[Fact]
public async Task MakingSomebodyOwnerThroughTheRolePicker_IsRefusedAndPointsAtHandingOver()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateTeamAsync(teams, "Platform", "platform");
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
await teams.ChangeRoleCommand.ExecuteAsync(TeamMemberRole.Owner);
teams.Members.Single(member => member.UserId == colleague).Role.ShouldBe("MEMBER");
teams.Status.ShouldContain("HAND OVER");
}
/// <remarks>
/// <para>
/// Both halves, because a transfer that only promoted the recipient would leave the team owned
/// twice and a test asserting one role would pass anyway. That is the exact failure the server uses
/// a single transaction to make impossible, so the client test asserts the same pair.
/// </para>
/// <para>
/// It also goes through the armed confirmation rather than calling the command directly, since
/// arming and confirming are where the target id is carried — and carrying it on the selection
/// instead is how a confirmation ends up applied to whatever was clicked last.
/// </para>
/// </remarks>
[Fact]
public async Task HandingOverATeam_MakesThemTheOwnerAndTheCallerAnAdmin()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateTeamAsync(teams, "Platform", "platform");
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.TransferOwnershipCommand.Execute(null);
teams.IsConfirming.ShouldBeTrue("the hand-over has to be answered, not just pressed");
teams.ShowsTeamActions.ShouldBeFalse("the buttons that armed it are replaced, not left live");
await teams.ConfirmActionCommand.ExecuteAsync(null);
teams.Members.Single(member => member.UserId == colleague).Role.ShouldBe("OWNER");
teams.Members.Single(member => member.IsSelf).Role.ShouldBe("ADMIN");
teams.IsConfirming.ShouldBeFalse();
}
/// <remarks>
/// Archiving is refused while the team owns a vault, and the refusal has to reach the screen. The
/// failure this guards is the quiet one: a client that swallowed the 409 and reloaded would show a
/// team that is still there with no explanation of why nothing happened.
/// </remarks>
[Fact]
public async Task ArchivingATeamThatOwnsAVault_IsRefusedAndSaysWhy()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
await teams.CreateVaultCommand.ExecuteAsync(null);
teams.ArchiveTeamCommand.Execute(null);
await teams.ConfirmActionCommand.ExecuteAsync(null);
teams.Teams.ShouldContain(team => team.Slug == "platform");
teams.Status.ShouldContain("holding a key");
}
/// <remarks>
/// An empty team can go, and this is the only operation on the screen that removes something from
/// everybody's list at once.
/// </remarks>
[Fact]
public async Task ArchivingAnEmptyTeam_RemovesIt()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
teams.ArchiveTeamCommand.Execute(null);
await teams.ConfirmActionCommand.ExecuteAsync(null);
teams.Teams.ShouldNotContain(team => team.Slug == "platform");
teams.Status.ShouldContain("Archived");
}
/// <remarks>
/// Renaming leaves the slug alone, and the status line says so unprompted — somebody who assumed
/// otherwise would find out from a URL much later, which is the worst moment to find out.
/// </remarks>
[Fact]
public async Task RenamingATeam_LeavesItsSlugAlone()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
teams.RenameTeamCommand.Execute(null);
teams.EditTeamName = "Platform Engineering";
await teams.SaveTeamCommand.ExecuteAsync(null);
var team = teams.Teams.ShouldHaveSingleItem();
team.Name.ShouldBe("Platform Engineering");
team.Slug.ShouldBe("platform");
teams.Status.ShouldContain("slug is still 'platform'");
}
/// <remarks>
/// <para>
/// The address the directory does not know used to be a dead end — the screen said they had to sign
/// in first and stopped. It invites them instead, from the same button, because which of the two
/// applies is a fact about the server's account table rather than about what the user is doing.
/// </para>
/// <para>
/// The status assertion is the point of the test. Nothing is sent, and an interface that said
/// "invited" without saying that would leave somebody waiting for an email that is never coming.
/// </para>
/// </remarks>
[Fact]
public async Task AddingAnAddressWithNoAccount_InvitesItAndSaysNothingWasSent()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
teams.InviteEmail = "newcomer@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.Members.ShouldHaveSingleItem("nobody has joined — they have only been invited");
var invitation = teams.Invitations.ShouldHaveSingleItem();
invitation.Email.ShouldBe("newcomer@example.com");
invitation.IsPending.ShouldBeTrue();
invitation.State.ShouldContain("Nothing was sent");
teams.Status.ShouldContain("cannot send mail");
}
/// <remarks>
/// A withdrawn invitation stays on the list saying it was withdrawn, rather than vanishing. One that
/// disappeared would read as never having been sent, which is the same thing the screen looks like
/// before anybody does anything.
/// </remarks>
[Fact]
public async Task WithdrawingAnInvitation_LeavesItListedAsWithdrawn()
{
await UnlockedAsync();
var teams = shell.Teams;
await CreateTeamAsync(teams, "Platform", "platform");
teams.InviteEmail = "newcomer@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedInvitation = teams.Invitations.ShouldHaveSingleItem();
await teams.RevokeInvitationCommand.ExecuteAsync(null);
teams.Invitations.ShouldHaveSingleItem().State.ShouldBe("withdrawn");
teams.Status.ShouldContain("Withdrew the invitation");
}
/// <remarks>
/// <para>
/// A reload rebuilds the team list and reselects, so a reload that changed the selection — creating
/// the first team is exactly that — used to leave two reads of the same team in flight: the one the
/// reload awaits, and one the selection handler started on its own. Both clear the member list and
/// then both append to it, so every member was drawn twice. On a team nobody has been added to yet,
/// whose only member is its owner, that read as the owner being in the team twice.
/// </para>
/// <para>
/// Counted rather than inferred from the list, and the gate is why: against a fake that answers from
/// memory each read finishes before the next begins, so the duplicate never appears and the bug
/// survives the test. Holding the read open is what makes this behave like a server.
/// </para>
/// </remarks>
[Fact]
public async Task CreatingATeam_ReadsItsMembersOnce()
{
await UnlockedAsync();
var teams = shell.Teams;
await teams.LoadAsync(Token);
teams.NewTeamCommand.Execute(null);
teams.NewTeamName = "Platform";
teams.NewTeamSlug = "platform";
var gate = new TaskCompletionSource();
server.MemberReadGate = gate;
var create = teams.CreateTeamCommand.ExecuteAsync(null);
// Asserted while the read is still in flight: that is the only moment at which a second read
// started by the selection handler is distinguishable from the reload's own.
server.MemberReads.ShouldBe(1, "a reload reads the selected team's members once");
gate.SetResult();
await create;
teams.Members.ShouldHaveSingleItem().Role.ShouldBe("OWNER");
}
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);
}
}