diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 06dd701..236c24c 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -915,6 +915,25 @@ add a bucket — then come back. **Failure means:** the screen has gone back to copying the vault's lists once at unlock. Covered by `TheTransfersScreen_FollowsTheVaultsHostList`; this is the version of it with a real picker in front of it. +### 7.12 A key made now can be bound in the editor that is already open + +The same check one screen over, and the ordinary way round: start editing a host, type a port into it, then go +to KEYS and add a key — or a password, or a tag — and come back to HOSTS. Repeat it with a group's editor open +instead of a host's. + +**Pass:** the editor is still open with the port still in it, and the new key is in the AUTHENTICATION picker +straight away. Choosing it and saving binds the host. A new tag is a chip among the others, unworn. + +Then the other half, which matters more: with a host that already authenticates with a key, open its editor +and add a *second* key. **Pass:** the picker still reads the first one, and saving leaves the host bound to it. + +**Failure means:** an editor that has to be cancelled and reopened before it can see what was just made — the +pickers went back to being snapshots taken when the editor opened, which was defensible only while the two +screens refused each other. See `VaultViewModel.RefreshOpenEditors`, and +`AKeyAddedWithTheHostEditorOpen_AppearsInItsAuthenticationPicker` for the same thing asserted against view +models. A picker that *moves* instead — landing on the key that just arrived, or back on "Password (ask each +time)" — is the worse failure of the two: it rebinds a host as a side effect of somebody else's sync. + --- ## Phase 8 — Adding and removing on the phone's host list diff --git a/src/DodoSSH.Client.Shell/ViewModels/SnippetsViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/SnippetsViewModel.cs index 7b5d3a2..c972a4d 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/SnippetsViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/SnippetsViewModel.cs @@ -72,6 +72,11 @@ internal sealed partial class SnippetsViewModel : ObservableObject vault.Snippets.CollectionChanged += OnSnippetsChanged; + // The editor's vault picker is a snapshot of this list — see BuildEditorVaultChoices — and a vault + // created on the Teams screen with a half-typed snippet open behind it would otherwise not be + // offered until the editor was closed and opened again. + vault.TargetVaults.CollectionChanged += OnTargetVaultsChanged; + Rebuild(); } @@ -577,6 +582,21 @@ internal sealed partial class SnippetsViewModel : ObservableObject private void OnSnippetsChanged(object? sender, NotifyCollectionChangedEventArgs e) => Rebuild(); + /// Refills the open editor's vault picker, landing back on the vault it was already writing to. + /// + /// Only while the editor is open, because that is the only picker built from a copy of the list — the move + /// panel's is built when it opens and folds away with the selection it was opened about. Restored by the + /// latched rather than by what the control shows, so a vault arriving mid-edit + /// cannot move a half-typed snippet: the refill is invisible except for the entry it adds. + /// + private void OnTargetVaultsChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (IsEditing) + { + BuildEditorVaultChoices(editorVaultId); + } + } + private void Rebuild() { // Captured and restored around the refill, for the reason the host sidebar's rebuild is written the diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index 741e39d..45dd95e 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -2629,10 +2629,19 @@ internal sealed partial class VaultViewModel( /// What the authentication picker offers: a typed password, then every key, then every credential. /// /// - /// Rebuilt when the editor opens rather than kept in step with the two lists. A background sync could pull - /// a new key while a host is being edited, and having the picker's contents change under the user mid-edit - /// is worse than the list being a minute stale — only one editor may be open at a time, so the only way to - /// add a key or a credential is to close this one anyway. + /// + /// Filled when the editor opens and kept in step with the two lists from then on, by + /// . It used to be the snapshot alone, on the grounds that only one editor + /// could be open at a time and so the only way to add a key was to close this one — which stopped being + /// true when was split from . A + /// host editor now sits open on the Hosts screen while a key is added on the Keychain screen, and a + /// picker that did not notice left the user cancelling an editor to see the key they had just made. + /// + /// + /// What the old note was protecting against is real, and is why the refill restores the selection by id + /// rather than rebuilding from the stored host: entries appear and disappear under the user, but what + /// they have chosen does not move. + /// /// internal ObservableCollection EditorAuthenticationChoices { get; } = []; @@ -3529,9 +3538,86 @@ internal sealed partial class VaultViewModel( // After all four lists, because the table is a projection of three of them. RebuildVaultItems(); + // Last of the rebuilds, because every picker an open editor holds is drawn from one of the lists + // above and would otherwise be showing the vault as it was when that editor opened. + RefreshOpenEditors(); + await LoadConflictsAsync(cancellationToken).ConfigureAwait(true); } + /// + /// Refills the pickers of whichever editor is open, so a key or a vault that has just arrived shows in it. + /// + /// + /// + /// The pickers are snapshots — see — and taking one when the + /// editor opens was correct while nothing could change a list without closing it first. Nothing about + /// that holds any more: the host editor lives on the Hosts screen and the keychain's editors live on the + /// Vault screen, so a key, a credential, a tag, a group or a whole vault can be added with a host editor + /// standing open behind it. Every one of them then failed to appear in the picker that exists to offer + /// it, and the only way to see it was to abandon the edit and start again. + /// + /// + /// Called from the one reload rather than from each of the twenty-odd places that write to the vault, + /// which is what makes a sync count as well as a save: a key pulled from another machine reaches the + /// open editor by the same path a key typed here does. + /// + /// + /// Selections are carried across by id, and every typed field is left alone. The refill has to be + /// invisible to somebody halfway through a form — an editor that reset its own bindings because a + /// background sync landed would be a worse bug than the stale list it fixes. So each picker is rebuilt + /// and then put back onto what it was already showing, including the placeholder entries that stand for + /// a binding whose target has gone: it is the same restore-by-id the open path does, from the editor's + /// current selection rather than from the stored item, because for as long as the editor is open the two + /// deliberately differ. + /// + /// + private void RefreshOpenEditors() + { + if (IsEditing) + { + // Read before anything is cleared. Rebuilding a bound collection makes the control null its own + // selection and write that back, so by the time the last picker is refilled these properties no + // longer say what the user chose. + var authentication = EditorSelectedAuthentication; + var groupId = EditorSelectedGroup?.EntityId; + + BuildTagChoices(); + + // The same order the two commands that open this editor use: the vault picker first because a + // group belongs to one vault, then the groups, then the bindings — which offer "inherit from + // group" only where the group picker has landed on something. + BuildEditorVaultChoices(editingHostVaultId); + + // The selection as it stands, not filtered through GroupInEditingVault: a group that has gone + // keeps its placeholder here for the reason it does on the open path, so that a sync arriving + // mid-edit cannot unfile the host when the form is saved. + BuildGroupChoices(groupId); + + BuildAuthenticationChoices( + authentication?.Kind == AuthenticationKind.SshKey ? authentication.EntityId : null, + authentication?.Kind == AuthenticationKind.Credential ? authentication.EntityId : null, + asksForPassword: authentication?.Kind == AuthenticationKind.Typed, + grouped: EditorSelectedGroup?.EntityId is not null); + } + + if (IsEditingGroup) + { + var authentication = GroupEditorSelectedAuthentication; + var parentId = GroupEditorSelectedParent?.EntityId; + + BuildGroupEditorVaultChoices(GroupEditorVaultId); + + // Guid.Empty while creating, which is what EditingGroupId being null means and is the same + // stand-in ClearGroupEditor uses: a group that does not exist yet cannot be its own parent. + BuildGroupParentChoices(EditingGroupId ?? Guid.Empty, parentId); + + BuildGroupAuthenticationChoices( + authentication?.Kind == AuthenticationKind.SshKey ? authentication.EntityId : null, + authentication?.Kind == AuthenticationKind.Credential ? authentication.EntityId : null); + } + } + /// Redraws every list from the vault, without saying anything about it. /// /// For the two things that change which vaults exist or which are drawn without going through this type diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 0240b51..0d79c42 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -2494,6 +2494,130 @@ public sealed class ShellFlowTests : IAsyncLifetime vault.Status.ShouldContain("host"); } + // ---- Keeping an open editor's pickers in step with the vault ---- + + /// + /// The other side of the split guard, and the bug it left behind. The host editor and the keychain's + /// editors are on different screens and refuse each other no longer, so a key is very often added + /// because the host in front of the user needs one — with that host's editor still standing on the + /// Hosts screen. The picker was a snapshot taken when the editor opened, so the key never appeared in it + /// and the only way to reach it was to abandon the edit and start again. + /// + [Fact] + public async Task AKeyAddedWithTheHostEditorOpen_AppearsInItsAuthenticationPicker() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddHostAsync(vault, "prod-db"); + vault.SelectedHost = vault.Hosts[0]; + vault.EditSelectedHostCommand.Execute(null); + vault.EditorPort = 2244; + + await AddKeyAsync(vault, "deploy"); + + vault.IsEditing.ShouldBeTrue("adding a key must not close the host editor"); + vault.EditorPort.ShouldBe(2244, "nor discard what has been typed into it"); + + var offered = vault.EditorAuthenticationChoices.Single( + choice => string.Equals(choice.Label, "deploy", StringComparison.Ordinal)); + + offered.Kind.ShouldBe(AuthenticationKind.SshKey); + offered.EntityId.ShouldBe(vault.Keys[0].EntityId); + + // A real entry rather than a label: choosing it and saving is what the user came here to do. + vault.EditorSelectedAuthentication = offered; + await vault.SaveHostCommand.ExecuteAsync(null); + + vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBe(vault.Keys[0].EntityId); + vault.Hosts[0].Host.Port.ShouldBe(2244); + } + + /// + /// The counterweight, and the reason the refill restores each picker by id rather than reloading the + /// stored host: a list that grows under somebody halfway through a form must not move what they had + /// already chosen in it, and the save that follows must not rebind the host to something nobody picked. + /// + [Fact] + public async Task AnItemArrivingWhileTheHostEditorIsOpen_LeavesItsSelectionWhereItWas() + { + var vault = await ReadyToConnectAsync(); + await AddKeyAsync(vault, "deploy"); + + var keyId = vault.Keys.ShouldHaveSingleItem().EntityId; + + await BindKeyAsync(vault, vault.Hosts[0], keyId); + + vault.SelectedHost = vault.Hosts[0]; + vault.EditSelectedHostCommand.Execute(null); + + await AddCredentialAsync(vault, "pg-primary"); + + vault.EditorAuthenticationChoices.ShouldContain( + choice => choice.Kind == AuthenticationKind.Credential); + + vault.EditorSelectedAuthentication.ShouldNotBeNull().EntityId + .ShouldBe(keyId, "a credential appearing must not unbind the host from its key"); + + await vault.SaveHostCommand.ExecuteAsync(null); + + vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBe(keyId); + vault.Hosts[0].Host.CredentialId.ShouldBeNull(); + } + + /// + /// Tags reach the same editor by a different route — the keychain screen rather than the box under the + /// chips — and a chip that only appeared on the next open would send the user round the same detour. + /// + [Fact] + public async Task ATagAddedWithTheHostEditorOpen_AppearsAmongItsChips() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddHostAsync(vault, "prod-db"); + vault.SelectedHost = vault.Hosts[0]; + vault.EditSelectedHostCommand.Execute(null); + + await AddTagAsync(vault, "production"); + + vault.HasTagChoices.ShouldBeTrue(); + + var chip = vault.EditorTagChoices.ShouldHaveSingleItem(); + chip.Label.ShouldBe("production"); + chip.IsWorn.ShouldBeFalse("appearing is not the same as being put on"); + } + + /// + /// The group editor shares the drawer with the host editor and its own picker was the same snapshot, so + /// the same detour applied to the default binding a whole group of hosts inherits. + /// + [Fact] + public async Task AKeyAddedWithTheGroupEditorOpen_AppearsInItsDefaultBindingPicker() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddGroupAsync(vault, "platform"); + + vault.SelectedGroup = vault.Groups.ShouldHaveSingleItem(); + vault.EditGroupCommand.Execute(null); + vault.GroupEditorDefaultPort = 2222; + + await AddKeyAsync(vault, "deploy"); + + vault.IsEditingGroup.ShouldBeTrue("adding a key must not close the group editor"); + vault.GroupEditorDefaultPort.ShouldBe(2222, "nor discard what has been typed into it"); + + vault.GroupEditorSelectedAuthentication = vault.GroupEditorAuthenticationChoices.Single( + choice => string.Equals(choice.Label, "deploy", StringComparison.Ordinal)); + + await vault.SaveGroupCommand.ExecuteAsync(null); + + vault.Groups.ShouldHaveSingleItem().Group.DefaultSshKeyId.ShouldBe(vault.Keys[0].EntityId); + vault.Groups[0].Group.DefaultPort.ShouldBe(2222); + } + // ---- Binding a key to a host ---- [Fact] diff --git a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs index 4ef94d0..815928b 100644 --- a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs @@ -319,6 +319,95 @@ public sealed class VaultSharingTests : IAsyncLifetime shell.Vault.HasVaultChoice.ShouldBeTrue(); } + /// + /// + /// And it reaches a host editor that was already open, which is very nearly the only way a vault gets made: + /// somebody starts adding a host, realises it belongs to the team rather than to them, and goes to make + /// somewhere to put it. The editor's picker is its own — see VaultViewModel.EditorVaultChoices — and + /// it was filled when the editor opened, so the vault they had just made for this host was the one place + /// they could not file it without throwing the form away. + /// + /// + /// The second half is what stops the fix being worse than the bug: the refill adds the entry and leaves the + /// selection alone, so a vault appearing cannot move a half-typed host into it. + /// + /// + [Fact] + public async Task AVaultCreatedWithAHostEditorOpen_IsOfferedAsSomewhereToFileThatHost() + { + await UnlockedAsync(); + + var vault = shell.Vault!; + + vault.NewHostCommand.Execute(null); + vault.EditorLabel = "prod-db"; + vault.EditorHostname = "db.internal"; + + await CreateVaultAsync(shell.Vaults, "Platform secrets"); + + var vaultId = shell.Vaults.SelectedVault!.VaultId; + + vault.IsEditing.ShouldBeTrue("making a vault must not close the host editor"); + vault.EditorLabel.ShouldBe("prod-db", "nor discard what has been typed into it"); + + vault.ShowsEditorVaultChoice.ShouldBeTrue( + "a second writable vault is what makes the picker worth drawing at all"); + + vault.EditorVaultChoices.Select(choice => choice.VaultId).ShouldContain(vaultId); + + vault.EditorSelectedVault.ShouldNotBeNull().IsPersonal + .ShouldBeTrue("the refill must not move the host into the vault that has just appeared"); + + // Choosing it, on the other hand, files the host there — which is what the picker was for. + vault.EditorSelectedVault = vault.EditorVaultChoices.Single( + choice => choice.VaultId == vaultId); + + await vault.SaveHostCommand.ExecuteAsync(null); + + vault.Hosts.ShouldHaveSingleItem().VaultId.ShouldBe(vaultId, vault.Status); + } + + /// + /// The snippet editor's picker is a copy of the same list — see + /// SnippetsViewModel.BuildEditorVaultChoices — and went stale in exactly the same way. It watches the + /// list rather than the reload, because this screen has always been a wrapper over the vault's collections + /// and has no reload of its own to hang off. + /// + [Fact] + public async Task AVaultCreatedWithASnippetEditorOpen_IsOfferedAsSomewhereToFileThatSnippet() + { + await UnlockedAsync(); + + var vault = shell.Vault!; + var snippets = SnippetsOver(vault); + + snippets.NewCommand.Execute(null); + snippets.EditorLabel = "restart the api"; + snippets.EditorCommand = "sudo systemctl restart dodossh-api"; + + await CreateVaultAsync(shell.Vaults, "Platform secrets"); + + var vaultId = shell.Vaults.SelectedVault!.VaultId; + + snippets.IsEditing.ShouldBeTrue("making a vault must not close the snippet editor"); + snippets.EditorCommand.ShouldBe( + "sudo systemctl restart dodossh-api", + "nor discard what has been typed into it"); + + snippets.ShowsEditorVaultChoice.ShouldBeTrue("there are two vaults to choose between now"); + snippets.EditorVaultChoices.Select(choice => choice.VaultId).ShouldContain(vaultId); + + snippets.EditorSelectedVault.ShouldNotBeNull().IsPersonal + .ShouldBeTrue("the refill must not move the snippet into the vault that has just appeared"); + + snippets.EditorSelectedVault = snippets.EditorVaultChoices.Single( + choice => choice.VaultId == vaultId); + + await snippets.SaveCommand.ExecuteAsync(null); + + Snippet(snippets, "restart the api").VaultId.ShouldBe(vaultId, snippets.Status); + } + /// /// Making a vault makes exactly one membership list, and this is the assertion that the two-step create /// has not started leaking them: the screen no longer offers to make one on its own, so a second one