From da7462e41fc5b8740b9b0dd10f89bd3991686656 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Thu, 30 Jul 2026 17:08:59 +0200 Subject: [PATCH] Show one kind of vault item at a time, and let the vault hold passwords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Outstanding items #8 and #9, in one commit rather than two. They are separable as work and were built in that order, but not as a diff: the section enum has three members, the one-editor guard has three arms, and the picker offers keys and credentials from the same list. Reconstructing an #8-only state would mean hand-writing an intermediate version of VaultViewModel that never existed and that no test has ever run. One honest commit beats two invented ones. --- #8, the type selector --- The column showed two lists and two editors stacked in 340 pixels, and only just: the key list needed a MaxHeight and had to hide itself whenever its editor opened, both to stop the host list above it pushing the buttons off the bottom edge. Credentials would not have fitted at all. It now shows one kind at a time, chosen by a selector at the top, and both workarounds are gone because a section owns the whole column. Three departures from the plan, each with a reason found while building it. The selector is plain Buttons and a parameterised command, not a TabControl, a TabStrip or a ListBox. All three of those hold the selection themselves, so a click moves the highlight before the view model can refuse it — and this column does refuse, while an editor is open. A selector lit on a section the column is not showing is worse than the refusal it would be hiding. Buttons carry no state and cannot disagree with the vault. The one-editor-at-a-time rule survives with its justification replaced. That rule was a workaround for the sizing problem above, and sections dissolved it: the editors are in different sections and only one section is ever laid out. BothEditorsAtOnce_DoNotFit_WhichIsWhyTheRuleExists is now BothEditorsOpen_NowFit_BecauseOnlyOneSectionIsLaidOut — the same test, inverted, because its own comment said that if it ever started passing the rule had become unnecessary. It has. The rule stays for a better reason: an open key editor holds a pasted private key in a bound string, and letting the column move on would leave key material in a form nobody can see, with nothing on screen to say it is there. A sizing hack became a rule about not hiding a secret from the person holding it. KeyEditorIsInTheWay and HostEditorIsInTheWay are one AnEditorIsInTheWay, called by the section switch and by every editor-opening command. And releasing the keyboard from the terminal has never worked. MainWindow takes Win32 focus off the WebView's child window and then calls Focus() on VaultColumn.KeyboardTarget — and a ListBox is not focusable by default in Avalonia, which leaves focus to its items. So the call returned false, the window ended up with nothing focused, and the keystrokes went nowhere: exactly the state that method's own comment says its second half exists to prevent. Found by writing the test to assert focus was taken rather than that the right control was named — the cheap assertion was already passing. Fixed with Focusable="True" on every list. --- #9, credentials --- Credentials have synced since they were added and could not be created. They can now, and the sync layer needed no change at all: fourth item type, same result, which is the item-kind seam working as intended. One picker for all three ways a host authenticates, which is what makes the illegal combination unrepresentable rather than merely invalid. SshKeyChoice became AuthenticationChoice carrying an AuthenticationKind, and BuildHost reads both SshKeyId and CredentialId off that single selection, so a host naming a key and a credential — which HostSecret.TryValidate refuses — cannot be expressed. Two pickers would have expressed it and then rejected it at save time. The kind travels with the id in three places and none is padding: Missing takes it, the placeholder lookup matches on kind as well as id, and Bound(kind) returns null unless the selection is that kind. Drop any one and a dangling credential comes back as a dangling key, which saves as a key binding to an id no key has. A credential's username had to reach the SSH request, not just its password. TryBuildCredential returned only the secret and the connect path read the username off the host, so a stored credential would have gone out under the wrong account — wrong in a way a server only reports as "authentication failed". It is now TryBuildAuthentication returning a (Username, Credential) pair. The no-username refusal moved, and had to. It ran before anything looked at the binding, which made a credential's username unreachable in the one case it is most useful: a host somebody never filled a username in for. It is now the last thing every branch agrees on, so such a host is perfectly usable through a credential that carries one, and a host with neither still refuses and now says where to put one. --- What the measurements cost --- Ten mutations, all caught. Two are worth naming. Removing a section's IsVisible is caught by OnlyOneSectionIsOnScreenAtOnce and by nothing else: two visible sections overlap in the row they share rather than clip, so every fit test still passes while the column shows one list through another. Defaulting the credential selection to the first row is caught by ReloadingKeepsACredentialSelectionButNeverInventsOne, and the property is a safety one rather than tidiness — Delete acts on the selection, so a list that picked a row on every background sync would aim a one-click password deletion at something nobody chose. The key list has the same property, and its comment cited a method that has not existed for some time; both now name the delete command they actually protect. One test of mine could not fail, and the mutation pass is what found it. AHostBoundToACredential_SendsItsPasswordAndItsUsername gave the credential and the host the same username, so it passed whichever one the code read. An override is only tested when the two values differ. Two shipped statements went false and were corrected rather than left: the class remark saying passwords were "not yet" in the vault, and the terminal column's "Keys are in the vault; passwords are not yet." That column's hint is now a tooltip on the password box rather than a sentence in the row, which was measured the hard way — by looking. At the window's 820px minimum the column gets 480, and a 220px box plus Connect plus any sentence does not fit; the row has shipped clipped for as long as it has had a hint in it. That strip is the one part of the window nothing can measure, because MainWindow cannot be laid out headlessly at all. Extracting it into its own control, as the vault column was extracted for exactly this reason, is what would fix that, and is not done here. 911 tests green, 30 of them new. Zero warnings, dotnet format clean. Seen by a person, which is how the two defects above were found. Still open from that pass: unlocking with the device key raises its consent dialog and then never returns, while registering one works — the difference is which thread the CNG call lands on, and diagnosing it properly is its own change. --- .../ViewModels/VaultViewModel.cs | 834 +++++++++++++++--- src/DodoSSH.Client.App/Views/MainWindow.axaml | 37 +- .../Views/VaultColumn.axaml | 467 ++++++---- .../Views/VaultColumn.axaml.cs | 26 +- .../VaultColumnLayoutTests.cs | 253 +++++- .../ShellFlowTests.cs | 559 +++++++++++- 6 files changed, 1835 insertions(+), 341 deletions(-) diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index 4e24d03..947ccf9 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -1,4 +1,5 @@ using System.Collections.ObjectModel; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Text; using CommunityToolkit.Mvvm.ComponentModel; @@ -38,38 +39,98 @@ internal sealed class HostRowViewModel(VaultItem host) /// How this host authenticates, in one word. /// - /// Worth a word in the list because the two behave differently at the moment of connecting: one needs - /// the password box filled in and the other does not, and a user staring at an empty password box on a - /// key-authenticated host has no other way to know it is not needed. + /// Worth a word in the list because the three behave differently at the moment of connecting: only one of + /// them needs the password box filled in, and a user staring at an empty password box on a + /// key-authenticated host has no other way to know it is not needed. "password" is the typed kind, which + /// is why the stored kind is "credential" rather than a second sort of password. /// - internal string Authentication => host.Secret.SshKeyId is null ? "password" : "key"; + internal string Authentication => host.Secret switch + { + { CredentialId: not null } => "credential", + { SshKeyId: not null } => "key", + _ => "password", + }; /// A short marker for the row, so the list says what it knows without a tooltip. internal string Badge => ItemBadge.For(host.IsBlocked, host.IsReadOnly, host.HasUnsyncedChanges); } -/// An entry in the host editor's key picker. -/// The key's item id, or null for password authentication. -/// What to show. -/// -/// A sentinel entry rather than a nullable selection, because a ComboBox with nothing selected and a -/// ComboBox meaning "no key" look identical and are not the same thing — the first is a host whose binding -/// has not been decided, the second is a decision. -/// -internal sealed record SshKeyChoice(Guid? EntityId, string Label) +/// What a host can authenticate with. +internal enum AuthenticationKind { - /// The "use a password" entry, always first. - internal static SshKeyChoice None { get; } = new(null, "Password (no key)"); + /// Typed at the moment of connecting, and never stored. + Typed, + + /// An SSH key in this vault. + SshKey, + + /// A username and password in this vault. + Credential, +} + +/// An entry in the host editor's authentication picker. +/// Which of the three ways this entry means. +/// The bound item's id, or null for a typed password. +/// What to show. +/// +/// What kind of thing the label names, shown beside it. Empty for the typed-password entry, which is not a +/// thing in the vault. +/// +/// +/// +/// One picker for all three, not two pickers. A host authenticates with a key or a stored +/// credential or a typed password, never two of them — HostSecret.TryValidate refuses a host +/// naming both. Two controls would express the illegal state and then reject it at save time; one control +/// cannot express it at all. That is the same reason this is a sentinel entry rather than a nullable +/// selection: a ComboBox with nothing selected and a ComboBox meaning "type a password" look identical and +/// are not the same thing — the first is a host whose binding has not been decided, the second is a decision. +/// +/// +/// The qualifier is load-bearing rather than decoration. Keys and credentials are named by the user and often +/// named the same thing — a key called deploy and the deploy account's password are the ordinary case — +/// so a list of bare labels would offer two indistinguishable entries that authenticate completely +/// differently. +/// +/// +internal sealed record AuthenticationChoice( + AuthenticationKind Kind, + Guid? EntityId, + string Label, + string Qualifier) +{ + /// The "type it each time" entry, always first. + /// + /// Named for what it costs rather than for what it lacks. "Password (no key)" described the old two-way + /// choice from the key's side; with credentials in the same list the distinction a user needs is between a + /// password this vault knows and one they will be asked for. + /// + internal static AuthenticationChoice Typed { get; } = + new(AuthenticationKind.Typed, null, "Password (ask each time)", string.Empty); + + /// An SSH key that is in the vault. + internal static AuthenticationChoice ForKey(Guid entityId, string label) => + new(AuthenticationKind.SshKey, entityId, label, "SSH key"); + + /// A credential that is in the vault. + internal static AuthenticationChoice ForCredential(Guid entityId, string label) => + new(AuthenticationKind.Credential, entityId, label, "credential"); /// - /// A stand-in for a key the host names and the vault no longer has. + /// A stand-in for something the host names and the vault no longer has. /// /// /// Kept in the list, and kept selected, so that opening a host to change its port does not silently - /// convert it to password authentication on save. The id is preserved; only the label admits the + /// convert it to a typed password on save. The id and the kind are both preserved — the kind because + /// dropping it would rebind a dangling credential as a dangling key — and only the label admits the /// problem. /// - internal static SshKeyChoice Missing(Guid entityId) => new(entityId, "(a key that is no longer here)"); + internal static AuthenticationChoice Missing(AuthenticationKind kind, Guid entityId) => new( + kind, + entityId, + kind is AuthenticationKind.Credential + ? "(a credential that is no longer here)" + : "(a key that is no longer here)", + kind is AuthenticationKind.Credential ? "credential" : "SSH key"); } /// One SSH key, as a row in the list. @@ -112,6 +173,45 @@ internal sealed class SshKeyRowViewModel(VaultItem key) internal string Badge => ItemBadge.For(key.IsBlocked, key.IsReadOnly, key.HasUnsyncedChanges); } +/// One stored credential, as a row in the list. +/// +/// +/// The same arrangement as , for the same two reasons: the decrypted secret +/// travels with the row so opening the editor or connecting needs no second decryption, and +/// nothing here exposes the password to the view. is what the editor and the +/// connect path read; what the XAML binds is a label, a description and a badge. Not a security boundary — +/// the same object holds the password either way — but it means no template, tooltip or accessibility surface +/// can render a password by being pointed at the obvious property. +/// +/// +internal sealed class CredentialRowViewModel(VaultItem credential) +{ + internal Guid EntityId => credential.EntityId; + + internal CredentialSecret Credential => credential.Secret; + + internal string Label => credential.Secret.Label; + + /// What the list shows under the name: the account, never the password. + /// + /// The username is the whole reason a credential is a separate item rather than two fields on a host, so + /// it is what the row has to show. Absent means "whatever the host says", which is a different statement + /// from a blank one and is written out rather than left as an empty line. + /// + internal string Description => credential.Secret.Username is { } username + ? username + : "uses each host's own username"; + + internal bool HasUnsyncedChanges => credential.HasUnsyncedChanges; + + internal bool IsBlocked => credential.IsBlocked; + + internal bool IsReadOnly => credential.IsReadOnly; + + internal string Badge => + ItemBadge.For(credential.IsBlocked, credential.IsReadOnly, credential.HasUnsyncedChanges); +} + /// The one-word marker a row shows for its sync state. /// /// Shared by both row types rather than written twice, because the three states mean the same thing for @@ -157,6 +257,29 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) internal bool HasDetail => notice.Fields.Count > 0; } +/// +/// Which kind of item the vault column is showing. +/// +/// +/// One at a time, chosen by a selector at the top of the column. The alternative was every kind stacked in +/// one scrolling column, which is what the column did with two of them and would not survive a third: two +/// lists and two editors already only just fit at the window's minimum height, and credentials are coming. +/// A section is also the unit the column's own layout is measured in — see +/// DodoSSH.Client.App.Layout.Tests, which lays out one of these at a time because that is all a user +/// can ever see at once. +/// +internal enum VaultSection +{ + /// The hosts to connect to, and the column's opening state. + Hosts, + + /// The SSH keys those hosts authenticate with. + Keys, + + /// The usernames and passwords they authenticate with instead. + Credentials, +} + /// /// An open vault: the host list, the editor, syncing, and connecting a terminal. /// @@ -173,18 +296,18 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// A background pass is deliberately quieter than the button: see . /// /// -/// Keys and host key trust are in the vault; passwords are not yet. An SSH key is a synced item, so -/// it is stored once and available on every machine, and so is a known host key — approving a fingerprint -/// here approves it on every device and survives a restart. Credentials are synced as well, but nothing in -/// this interface can create one, so password authentication still asks each time. That is a real M1 -/// limitation rather than a design choice, and the interface says so rather than implying the vault holds -/// more than it does. +/// Everything a connection needs is in the vault. Keys, passwords and host key trust are all synced +/// items, so each is stored once and available on every machine — approving a fingerprint here approves it on +/// every device and survives a restart. A typed password is what is left when a host is bound to nothing, and +/// that is now a choice rather than the only option this interface offered. /// /// -/// A key belongs to a host. Each host names the key it authenticates with, or none, and that choice -/// is a field in its encrypted payload — so it follows the host to every machine rather than being made -/// again per connection. The cost is a payload schema version, paid only by hosts that actually bind a key: -/// see HostSecretCodec.CurrentSchemaVersion. +/// A key or a password belongs to a host. Each host names the one thing it authenticates with, or +/// nothing, and that choice is a field in its encrypted payload — so it follows the host to every machine +/// rather than being made again per connection. The two are mutually exclusive and the editor offers them +/// through a single picker, which is what makes the illegal combination unrepresentable rather than merely +/// invalid. The cost is a payload schema version, paid only by hosts that actually bind something: see +/// HostSecretCodec.CurrentSchemaVersion. /// /// internal sealed partial class VaultViewModel( @@ -224,6 +347,9 @@ internal sealed partial class VaultViewModel( /// The SSH keys to show, unpushed local state included. internal ObservableCollection Keys { get; } = []; + /// The stored credentials to show, unpushed local state included. + internal ObservableCollection Credentials { get; } = []; + /// Whatever the merge had to override and the user has not acknowledged. internal ObservableCollection Conflicts { get; } = []; @@ -236,6 +362,9 @@ internal sealed partial class VaultViewModel( [ObservableProperty] private SshKeyRowViewModel? selectedKey; + [ObservableProperty] + private CredentialRowViewModel? selectedCredential; + [ObservableProperty] private string status = string.Empty; @@ -248,6 +377,27 @@ internal sealed partial class VaultViewModel( [ObservableProperty] private bool isBusy; + // ---- Which kind of item is showing ---- + + /// + /// Settable, and the markup deliberately does not bind a selector's selection to it. A selection binding + /// would move before could refuse, leaving a selector highlighting a section + /// the column is not showing; two plain buttons and a command carry no state of their own and cannot + /// disagree with this. Tests set it directly, which is the same thing the command does once it has + /// decided. + /// + [ObservableProperty] + private VaultSection section; + + /// Whether the hosts section is the one showing. + internal bool ShowsHosts => Section is VaultSection.Hosts; + + /// + internal bool ShowsKeys => Section is VaultSection.Keys; + + /// + internal bool ShowsCredentials => Section is VaultSection.Credentials; + // ---- The editor ---- [ObservableProperty] @@ -272,18 +422,18 @@ internal sealed partial class VaultViewModel( private bool editorRelayEnabled; /// - /// What the key picker offers: password, then every key in the vault. + /// 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 key list. 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 — the two editors cannot be open at once, so - /// the only way to add a key is to close this one anyway. + /// 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. /// - internal ObservableCollection EditorKeyChoices { get; } = []; + internal ObservableCollection EditorAuthenticationChoices { get; } = []; [ObservableProperty] - private SshKeyChoice? editorSelectedKey; + private AuthenticationChoice? editorSelectedAuthentication; /// The item being edited, or null when creating. private Guid? editingEntityId; @@ -330,17 +480,74 @@ internal sealed partial class VaultViewModel( /// The key being edited, or null when creating. private Guid? editingKeyId; + // ---- The credential editor ---- + // A third set, on the same reasoning as the second: three editors holding unrelated fields, and sharing + // them would mean a half-typed key reappearing inside a credential. + + [ObservableProperty] + private bool isEditingCredential; + + [ObservableProperty] + private string credentialEditorLabel = string.Empty; + + /// + /// Optional, and what makes a credential worth being its own item: one account on twenty machines is + /// rotated in one place. Blank means "use each host's own username" — see CredentialSecret.Username, + /// which normalises the two spellings of that to one. + /// + [ObservableProperty] + private string credentialEditorUsername = string.Empty; + + /// + /// Holds a password for as long as the editor is open, and clears it — + /// the same bargain, and the same limits, as the private key box. See CredentialSecret. + /// + [ObservableProperty] + private string credentialEditorPassword = string.Empty; + + [ObservableProperty] + private string credentialEditorNotes = string.Empty; + + /// The credential being edited, or null when creating. + private Guid? editingCredentialId; + // ---- Connecting ---- /// - /// Typed per connection because nothing in this interface can create a vault credential yet — not because - /// the vault cannot hold one. Never persisted. + /// Typed per connection, never persisted, and now only reached by a host bound to nothing. It stays because + /// not every password is worth storing — a one-off on a machine somebody will never open again, or one + /// they would rather this vault did not hold — and because a credential has to be created before it can be + /// bound, which means the first connection to a new host happens through this box. /// [ObservableProperty] private string connectPassword = string.Empty; - /// Whether the selected host authenticates with a key, so the password box can say so. - internal bool SelectedHostUsesAKey => SelectedHost?.Host.SshKeyId is not null; + /// + /// Whether the selected host will want something typed into the password box. + /// + /// + /// True with nothing selected, which is deliberate: the box is the resting state of that corner of the + /// window, and an empty terminal column with no password box in it reads as a column that is still + /// loading. + /// + internal bool SelectedHostAsksForAPassword => + SelectedHost is null or { Host: { SshKeyId: null, CredentialId: null } }; + + /// + /// What the terminal column says in place of the password box, or nothing when the box is showing. + /// + /// + /// A sentence rather than a hidden box on its own, because "nothing needs typing" and "something needs + /// typing and the box has not appeared yet" look identical, and only one of them is fine. Which of the two + /// bindings is doing it matters to the reader: a stored password can be wrong and re-typed here if this + /// said nothing, and a key cannot. + /// + internal string SelectedHostAuthenticationNote => SelectedHost?.Host switch + { + { CredentialId: not null } => "This host uses a password stored in your vault.", + { SshKeyId: not null } => "This host authenticates with its SSH key.", + _ => string.Empty, + }; [ObservableProperty] private HostKeyPresentation? pendingHostKey; @@ -371,12 +578,36 @@ internal sealed partial class VaultViewModel( { await ReloadAsync(cancellationToken).ConfigureAwait(true); - Status = (Hosts.Count, Keys.Count) switch + // Built from whatever is there rather than enumerated per combination. Two item types were four cases; + // three would be eight, and the fourth kind the selector will eventually grow — pinned host keys — + // would be sixteen. + var counted = new List(3); + + if (Hosts.Count > 0) + { + counted.Add($"{Hosts.Count} host(s)"); + } + + if (Keys.Count > 0) + { + counted.Add($"{Keys.Count} key(s)"); + } + + if (Credentials.Count > 0) + { + counted.Add($"{Credentials.Count} credential(s)"); + } + + var contents = string.Join(", ", counted); + + // "No hosts yet" survives as its own case, because it is the one nudge this line gives: a vault holding + // keys and credentials and no hosts is set up but unused, and "2 key(s) in Personal." would read as + // though everything were in order. + Status = (Hosts.Count, counted.Count) switch { (0, 0) => "No hosts yet. Add one.", - (0, var keys) => $"No hosts yet, and {keys} key(s) in {VaultName}.", - (var hosts, 0) => $"{hosts} host(s) in {VaultName}.", - var (hosts, keys) => $"{hosts} host(s) and {keys} key(s) in {VaultName}.", + (0, _) => $"No hosts yet, and {contents} in {VaultName}.", + _ => $"{contents} in {VaultName}.", }; } @@ -394,6 +625,7 @@ internal sealed partial class VaultViewModel( var unreadable = await ReloadHostsAsync(cancellationToken).ConfigureAwait(true); unreadable += await ReloadKeysAsync(cancellationToken).ConfigureAwait(true); + unreadable += await ReloadCredentialsAsync(cancellationToken).ConfigureAwait(true); UnreadableItems = unreadable; PendingChanges = await session.PendingChangeCountAsync(cancellationToken).ConfigureAwait(true); @@ -426,9 +658,9 @@ internal sealed partial class VaultViewModel( /// How many keys would not decrypt. /// - /// Unlike the host list, the selection is not defaulted to the first row. A key selection is - /// what authenticates with, and quietly selecting one on load would - /// mean a connection made with a key the user never chose. + /// Unlike the host list, the selection is not defaulted to the first row: it is what + /// acts on, and a list that picked a row on every background sync would aim + /// that button at a key nobody chose. /// private async Task ReloadKeysAsync(CancellationToken cancellationToken) { @@ -450,6 +682,34 @@ internal sealed partial class VaultViewModel( return listing.Unreadable; } + /// How many credentials would not decrypt. + /// + /// An existing selection survives a reload and a reload never invents one, which is the same pair of rules + /// as the key list and matters more here. acts on the selection, so a + /// list that fell back to its first row would put a one-click deletion of somebody's password behind a + /// button they never aimed. + /// + private async Task ReloadCredentialsAsync(CancellationToken cancellationToken) + { + var listing = await session.Credentials + .ListAsync(session.ActiveVaultId, cancellationToken) + .ConfigureAwait(true); + + var selectedId = SelectedCredential?.EntityId; + + Credentials.Clear(); + + foreach (var credential in listing.Items + .OrderBy(credential => credential.Secret.Label, StringComparer.CurrentCulture)) + { + Credentials.Add(new CredentialRowViewModel(credential)); + } + + SelectedCredential = Credentials.FirstOrDefault(row => row.EntityId == selectedId); + + return listing.Unreadable; + } + /// Runs a synchronisation pass, if there is a server to talk to. [RelayCommand] private async Task SyncAsync(CancellationToken cancellationToken) @@ -596,15 +856,44 @@ internal sealed partial class VaultViewModel( } } - /// Starts a new host. + /// Shows one kind of item, if nothing is being edited. + /// + /// Takes the section rather than there being one command per kind, so a third kind is an enum member and + /// a button and nothing else. + /// [RelayCommand] - private void NewHost() + private void ShowSection(VaultSection target) { - if (KeyEditorIsInTheWay()) + // Before the editor check, not after. Asking for the section already showing is not a request to leave + // an editor, and refusing it would scold somebody for clicking where they already are. + if (target == Section) { return; } + if (AnEditorIsInTheWay()) + { + return; + } + + Section = target; + + // Cleared rather than set to the section's name. The status line is shared with the account bar and + // carries the result of the last thing that happened; "Hosts." would push a sync report or a save + // confirmation off it to say something the selector is already showing. + Status = string.Empty; + } + + /// Starts a new host. + [RelayCommand] + private void NewHost() + { + if (AnEditorIsInTheWay()) + { + return; + } + + Section = VaultSection.Hosts; editingEntityId = null; EditorLabel = string.Empty; EditorHostname = string.Empty; @@ -612,7 +901,7 @@ internal sealed partial class VaultViewModel( EditorUsername = string.Empty; EditorNotes = string.Empty; EditorRelayEnabled = false; - BuildKeyChoices(boundKeyId: null); + BuildAuthenticationChoices(boundKeyId: null, boundCredentialId: null); IsEditing = true; Status = "Adding a host."; } @@ -621,7 +910,7 @@ internal sealed partial class VaultViewModel( [RelayCommand] private void EditSelectedHost() { - if (SelectedHost is not { } row || KeyEditorIsInTheWay()) + if (SelectedHost is not { } row || AnEditorIsInTheWay()) { return; } @@ -634,6 +923,7 @@ internal sealed partial class VaultViewModel( return; } + Section = VaultSection.Hosts; editingEntityId = row.EntityId; EditorLabel = row.Host.Label; EditorHostname = row.Host.Hostname; @@ -641,7 +931,7 @@ internal sealed partial class VaultViewModel( EditorUsername = row.Host.Username ?? string.Empty; EditorNotes = row.Host.Notes ?? string.Empty; EditorRelayEnabled = row.Host.RelayEnabled; - BuildKeyChoices(row.Host.SshKeyId); + BuildAuthenticationChoices(row.Host.SshKeyId, row.Host.CredentialId); IsEditing = true; Status = $"Editing {row.Label}."; } @@ -730,11 +1020,12 @@ internal sealed partial class VaultViewModel( [RelayCommand] private void NewKey() { - if (HostEditorIsInTheWay()) + if (AnEditorIsInTheWay()) { return; } + Section = VaultSection.Keys; editingKeyId = null; ClearKeyEditor(); IsEditingKey = true; @@ -749,7 +1040,7 @@ internal sealed partial class VaultViewModel( [RelayCommand] private void EditSelectedKey() { - if (SelectedKey is not { } row || HostEditorIsInTheWay()) + if (SelectedKey is not { } row || AnEditorIsInTheWay()) { return; } @@ -760,6 +1051,7 @@ internal sealed partial class VaultViewModel( return; } + Section = VaultSection.Keys; editingKeyId = row.EntityId; KeyEditorLabel = row.Key.Label; KeyEditorPrivateKey = row.Key.PrivateKeyPem; @@ -849,6 +1141,131 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } + /// Starts a new credential. + [RelayCommand] + private void NewCredential() + { + if (AnEditorIsInTheWay()) + { + return; + } + + Section = VaultSection.Credentials; + editingCredentialId = null; + ClearCredentialEditor(); + IsEditingCredential = true; + Status = "Adding a credential."; + } + + /// Opens the selected credential for editing. + /// + /// The password is loaded into the editor, as the private key is and for the same reason: the codec has no + /// notion of a partial update, so saving re-encodes every field. + /// + [RelayCommand] + private void EditSelectedCredential() + { + if (SelectedCredential is not { } row || AnEditorIsInTheWay()) + { + return; + } + + if (row.IsReadOnly) + { + Status = "This credential was written by a newer version of DodoSSH. Update before editing it."; + return; + } + + Section = VaultSection.Credentials; + editingCredentialId = row.EntityId; + CredentialEditorLabel = row.Credential.Label; + CredentialEditorUsername = row.Credential.Username ?? string.Empty; + CredentialEditorPassword = row.Credential.Password; + CredentialEditorNotes = row.Credential.Notes ?? string.Empty; + IsEditingCredential = true; + Status = $"Editing {row.Label}."; + } + + /// Abandons the credential editor, clearing the password out of it. + [RelayCommand] + private void CancelCredentialEdit() + { + IsEditingCredential = false; + editingCredentialId = null; + ClearCredentialEditor(); + Status = string.Empty; + } + + /// Stores the credential editor's contents, encrypted, and queues it for the server. + [RelayCommand] + private async Task SaveCredentialAsync(CancellationToken cancellationToken) + { + var credential = BuildCredential(); + + if (!credential.TryValidate(out var reason)) + { + Status = reason; + return; + } + + await RunAsync( + "Saving…", + async () => + { + if (editingCredentialId is { } entityId) + { + await session.Credentials + .UpdateAsync(session.ActiveVaultId, entityId, credential, cancellationToken) + .ConfigureAwait(true); + } + else + { + editingCredentialId = await session.Credentials + .CreateAsync(session.ActiveVaultId, credential, cancellationToken) + .ConfigureAwait(true); + } + + IsEditingCredential = false; + ClearCredentialEditor(); + + await ReloadAsync(cancellationToken).ConfigureAwait(true); + + SelectedCredential = Credentials + .FirstOrDefault(row => row.EntityId == editingCredentialId); + editingCredentialId = null; + + Status = connection() is null + ? $"Saved '{credential.Label}'. It will sync when you are online." + : $"Saved '{credential.Label}'."; + }).ConfigureAwait(true); + + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + + /// Queues a tombstone for the selected credential. + [RelayCommand] + private async Task DeleteCredentialAsync(CancellationToken cancellationToken) + { + if (SelectedCredential is not { } row) + { + return; + } + + await RunAsync( + "Deleting…", + async () => + { + await session.Credentials + .DeleteAsync(session.ActiveVaultId, row.EntityId, cancellationToken) + .ConfigureAwait(true); + + await ReloadAsync(cancellationToken).ConfigureAwait(true); + Status = $"Deleted '{row.Label}'."; + }).ConfigureAwait(true); + + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + /// Opens a terminal on the selected host. [RelayCommand] private async Task ConnectAsync(CancellationToken cancellationToken) @@ -859,19 +1276,12 @@ internal sealed partial class VaultViewModel( return; } - if (string.IsNullOrEmpty(row.Host.Username)) + // Refused rather than quietly falling back to the password box. A host set up for key-only access + // that silently starts offering a password is the failure worth ruling out — the user asked for one + // thing and got another, and the host is the last place that would say so. + if (!TryBuildAuthentication(row.Host, out var authentication, out var refusal)) { - Status = "This host has no username. Edit it and add one."; - return; - } - - if (TryBuildCredential(row.Host) is not { } credential) - { - // Refused rather than quietly falling back to the password box. A host set up for key-only - // access that silently starts offering a password is the failure worth ruling out — the user - // asked for one thing and got another, and the host is the last place that would say so. - Status = $"'{row.Label}' authenticates with an SSH key that is not in this vault any more. " - + "Edit the host to choose another key, or set it back to a password."; + Status = refusal; return; } @@ -880,7 +1290,7 @@ internal sealed partial class VaultViewModel( await RunAsync( $"Connecting to {row.Label}…", - () => OpenSessionAsync(row, credential, cancellationToken)).ConfigureAwait(true); + () => OpenSessionAsync(row, authentication, cancellationToken)).ConfigureAwait(true); } /// @@ -1047,7 +1457,7 @@ internal sealed partial class VaultViewModel( /// private async Task OpenSessionAsync( HostRowViewModel row, - SshCredential credential, + HostAuthentication authentication, CancellationToken cancellationToken) { try @@ -1057,8 +1467,8 @@ internal sealed partial class VaultViewModel( var request = new SshConnectionRequest( row.Host.Hostname, row.Host.Port, - row.Host.Username!, - credential); + authentication.Username, + authentication.Credential); await workspace .OpenSessionAsync(request, TerminalSize.Default, cancellationToken) @@ -1094,7 +1504,20 @@ internal sealed partial class VaultViewModel( } /// - /// How this host authenticates, or null when it names a key the vault does not have. + /// Everything the SSH stack needs to authenticate as somebody on a host. + /// + /// The account to log in as, after any credential has had its say. + /// What proves it. + /// + /// The two travel together because a credential can change both. Returning only the secret and reading the + /// username off the host separately is what the connect path used to do, and it would have sent a stored + /// credential's password under the host's username — which is the one combination that is wrong in a way + /// the server reports as "authentication failed". + /// + private sealed record HostAuthentication(string Username, SshCredential Credential); + + /// + /// Works out how a host authenticates, or says why it cannot. /// /// /// @@ -1105,25 +1528,101 @@ internal sealed partial class VaultViewModel( /// check, because SshKeySecret.Passphrase cannot hold an empty string. /// /// - /// Null is a refusal, not a fallback, and the caller must treat it as one. A dangling reference means a - /// key was deleted on another machine — plausible, and no reason to start sending a password to a host - /// somebody deliberately set up not to accept one. + /// False is a refusal, not a fallback, and the caller must treat it as one. A dangling reference means the + /// key or credential was deleted on another machine — plausible, and no reason to start sending a typed + /// password to a host somebody deliberately set up not to accept one. + /// + /// + /// The credential branch comes first because the two bindings are mutually exclusive and a host carrying + /// both is already invalid; reading the credential first means a host that somehow acquired both is + /// answered by the more specific of the two rather than by whichever the code happened to check. /// /// - private SshCredential? TryBuildCredential(HostSecret host) + private bool TryBuildAuthentication( + HostSecret host, + [NotNullWhen(true)] out HostAuthentication? authentication, + [NotNullWhen(false)] out string? reason) { - if (host.SshKeyId is not { } keyId) + if (host.CredentialId is { } credentialId) { - return new SshPasswordCredential(ConnectPassword); + if (Credentials.FirstOrDefault(row => row.EntityId == credentialId) is not { } credential) + { + return Refuse( + $"'{host.Label}' authenticates with a credential that is not in this vault any more. " + + "Edit the host to choose another one, or set it back to a typed password.", + out authentication, + out reason); + } + + // The credential's username wins where it has one, which is the whole reason it can carry one: one + // account on twenty machines is described once. Falling back to the host's covers the ordinary + // case of a shared password used under each machine's own account. + return Complete( + credential.Credential.Username ?? host.Username, + new SshPasswordCredential(credential.Credential.Password), + out authentication, + out reason); } - if (Keys.FirstOrDefault(row => row.EntityId == keyId) is not { } key) + if (host.SshKeyId is { } keyId) { - return null; + if (Keys.FirstOrDefault(row => row.EntityId == keyId) is not { } key) + { + return Refuse( + $"'{host.Label}' authenticates with an SSH key that is not in this vault any more. " + + "Edit the host to choose another key, or set it back to a password.", + out authentication, + out reason); + } + + return Complete( + host.Username, + new SshPrivateKeyCredential( + Encoding.UTF8.GetBytes(key.Key.PrivateKeyPem), key.Key.Passphrase), + out authentication, + out reason); } - return new SshPrivateKeyCredential( - Encoding.UTF8.GetBytes(key.Key.PrivateKeyPem), key.Key.Passphrase); + return Complete( + host.Username, new SshPasswordCredential(ConnectPassword), out authentication, out reason); + } + + /// + /// The last thing every branch has to agree on: there is somebody to log in as. + /// + /// + /// Checked here rather than at the top of because the answer depends + /// on which branch was taken — a host with no username of its own is perfectly usable through a credential + /// that carries one, and refusing it up front would have made the credential's most useful property + /// unreachable. + /// + private static bool Complete( + string? username, + SshCredential credential, + out HostAuthentication? authentication, + [NotNullWhen(false)] out string? reason) + { + if (string.IsNullOrEmpty(username)) + { + return Refuse( + "This host has no username. Edit it and add one, or bind it to a credential that carries one.", + out authentication, + out reason); + } + + authentication = new HostAuthentication(username, credential); + reason = null; + return true; + } + + private static bool Refuse( + string reason, + out HostAuthentication? authentication, + out string? refusal) + { + authentication = null; + refusal = reason; + return false; } private HostSecret BuildHost() => @@ -1136,41 +1635,95 @@ internal sealed partial class VaultViewModel( Notes = string.IsNullOrWhiteSpace(EditorNotes) ? null : EditorNotes, RelayEnabled = EditorRelayEnabled, - // Whatever the picker holds, including the id of a key that has gone missing. Reading it from - // the picker rather than carrying the original through is what lets a binding be removed at all, - // and preserving a missing id is what stops an unrelated edit removing one by accident. - SshKeyId = EditorSelectedKey?.EntityId, + // Both read off the one picker, including the id of something that has gone missing. Reading them + // from the picker rather than carrying the originals through is what lets a binding be removed at + // all, and preserving a missing id is what stops an unrelated edit removing one by accident. One + // control means the two can never both be set: mutual exclusion by construction, rather than + // HostSecret.TryValidate catching it after the fact. + SshKeyId = Bound(AuthenticationKind.SshKey), + CredentialId = Bound(AuthenticationKind.Credential), }; + /// The picker's selection, if it names something of this kind. + private Guid? Bound(AuthenticationKind kind) => + EditorSelectedAuthentication is { } choice && choice.Kind == kind ? choice.EntityId : null; + /// - /// Fills the key picker, keeping whatever the host is currently bound to selectable. + /// Fills the authentication picker, keeping whatever the host is currently bound to selectable. /// - /// The key the host names, or null for password authentication. + /// The key the host names, if any. + /// The credential the host names, if any. /// - /// A bound key that is no longer in the vault gets a placeholder entry rather than being dropped. Without - /// one the picker would open on "Password (no key)", and someone editing the host's port would convert it - /// to password authentication by saving — which is the quiet version of the failure the connect path - /// refuses outright. + /// A binding whose target is no longer in the vault gets a placeholder entry rather than being dropped. + /// Without one the picker would open on "Password (ask each time)", and someone editing the host's port + /// would convert it to a typed password by saving — which is the quiet version of the failure the connect + /// path refuses outright. /// - private void BuildKeyChoices(Guid? boundKeyId) + private void BuildAuthenticationChoices(Guid? boundKeyId, Guid? boundCredentialId) { - EditorKeyChoices.Clear(); - EditorKeyChoices.Add(SshKeyChoice.None); + EditorAuthenticationChoices.Clear(); + EditorAuthenticationChoices.Add(AuthenticationChoice.Typed); foreach (var key in Keys) { - EditorKeyChoices.Add(new SshKeyChoice(key.EntityId, key.Label)); + EditorAuthenticationChoices.Add(AuthenticationChoice.ForKey(key.EntityId, key.Label)); } - if (boundKeyId is { } bound && EditorKeyChoices.All(choice => choice.EntityId != bound)) + foreach (var credential in Credentials) { - EditorKeyChoices.Add(SshKeyChoice.Missing(bound)); + EditorAuthenticationChoices.Add( + AuthenticationChoice.ForCredential(credential.EntityId, credential.Label)); } - EditorSelectedKey = EditorKeyChoices.FirstOrDefault(choice => choice.EntityId == boundKeyId) - ?? SshKeyChoice.None; + // At most one of the two is set on a valid host, so at most one placeholder is ever added. + AddMissing(AuthenticationKind.SshKey, boundKeyId); + AddMissing(AuthenticationKind.Credential, boundCredentialId); + + EditorSelectedAuthentication = Selected(boundKeyId, boundCredentialId); } + private void AddMissing(AuthenticationKind kind, Guid? boundId) + { + if (boundId is { } bound + && !EditorAuthenticationChoices.Any(choice => choice.Kind == kind && choice.EntityId == bound)) + { + EditorAuthenticationChoices.Add(AuthenticationChoice.Missing(kind, bound)); + } + } + + /// + /// Matched on the kind as well as the id. Ids are v7 GUIDs and a collision is not the worry — selecting the + /// right row for the wrong reason is, because a lookup by id alone would compile, pass, and silently pick a + /// key when the host named a credential the day the two ever shared an id. + /// + private AuthenticationChoice Selected(Guid? boundKeyId, Guid? boundCredentialId) => + (boundKeyId, boundCredentialId) switch + { + ({ } key, _) => Find(AuthenticationKind.SshKey, key), + (_, { } credential) => Find(AuthenticationKind.Credential, credential), + _ => AuthenticationChoice.Typed, + }; + + private AuthenticationChoice Find(AuthenticationKind kind, Guid entityId) => + EditorAuthenticationChoices + .FirstOrDefault(choice => choice.Kind == kind && choice.EntityId == entityId) + ?? AuthenticationChoice.Typed; + + private CredentialSecret BuildCredential() => + new() + { + Label = CredentialEditorLabel.Trim(), + + // Not trimmed and not emptied, exactly as a key's passphrase is not: leading or trailing spaces + // are legitimate in a password, and CredentialSecret refuses an empty one on its own. + Password = CredentialEditorPassword, + + // Trimmed, unlike the password. A username with a trailing space is a different account name to + // sshd, and it is never the one somebody meant. + Username = CredentialEditorUsername.Trim(), + Notes = string.IsNullOrWhiteSpace(CredentialEditorNotes) ? null : CredentialEditorNotes, + }; + /// /// The private key is not trimmed. Its armour is whitespace-significant and a client that tidied it up /// would eventually tidy a format it did not fully understand — the same reason @@ -1191,45 +1744,49 @@ internal sealed partial class VaultViewModel( }; /// - /// Whether the key editor has to be dealt with before another one can open. + /// Whether an open editor has to be dealt with before the column does anything else. /// /// /// - /// Only one editor open at a time, and this is a layout constraint rather than a style rule. Both - /// editors sit in the same 340-pixel column as Auto rows, and their desired heights together - /// exceed the column at the window's minimum height — so opening both pushes the lower one's Save and - /// Cancel past the bottom edge, where they cannot be clicked. That is the same failure this window has - /// already shipped once, when the setup screens rendered sliced with their buttons unreachable, and it is - /// the failure that nothing in this repository can catch: no test loads a .axaml. Making it a - /// state rule instead of a sizing hope is what makes it testable at all. + /// One editor open at a time, and the reason has changed. It used to be a layout constraint: + /// both editors sat in the same 340-pixel column as Auto rows and their desired heights together + /// exceeded it, so opening both pushed the lower one's Save and Cancel past the bottom edge. Sections + /// dissolved that — the two editors are now in different sections and only one section is ever laid out, + /// so two open editors no longer clip anything. That is measured, not assumed: + /// BothEditorsOpen_NowFit_BecauseOnlyOneSectionIsLaidOut is the same test that used to prove the + /// opposite. + /// + /// + /// The rule stays for a better reason. The key editor holds a pasted private key in a bound string for + /// as long as it is open, and only CancelKeyEdit lets go of it. Letting the column move on with + /// that editor open would leave key material in a form nobody can see, with nothing on screen to say it + /// is there — so what was a workaround for a sizing problem is now a rule about not hiding a private key + /// from the person holding it. /// /// /// Refused rather than resolved by closing the other editor, because closing it would silently discard /// what was typed there — and in the key editor that is a pasted private key the user may have nowhere /// else. One sentence and one click is the cheaper of the two. /// + /// + /// One check rather than the pair this replaced. Each of those asked about the other editor, + /// which only made sense while the two lists shared a column; the question a selector asks is whether + /// anything is open at all, and every caller wants that same answer. + /// /// - private bool KeyEditorIsInTheWay() + private bool AnEditorIsInTheWay() { - if (!IsEditingKey) + // Names the editor that is actually open, because "finish what you are editing" is useless advice + // in a column that shows one section: the thing to go back to may not be on screen. + Status = (IsEditing, IsEditingKey, IsEditingCredential) switch { - return false; - } + (_, true, _) => "Finish or cancel the SSH key you are editing first.", + (_, _, true) => "Finish or cancel the credential you are editing first.", + (true, _, _) => "Finish or cancel the host you are editing first.", + _ => Status, + }; - Status = "Finish or cancel the SSH key you are editing first."; - return true; - } - - /// - private bool HostEditorIsInTheWay() - { - if (!IsEditing) - { - return false; - } - - Status = "Finish or cancel the host you are editing first."; - return true; + return IsEditing || IsEditingKey || IsEditingCredential; } private void ClearKeyEditor() @@ -1241,6 +1798,14 @@ internal sealed partial class VaultViewModel( KeyEditorNotes = string.Empty; } + private void ClearCredentialEditor() + { + CredentialEditorLabel = string.Empty; + CredentialEditorUsername = string.Empty; + CredentialEditorPassword = string.Empty; + CredentialEditorNotes = string.Empty; + } + private async Task LoadConflictsAsync(CancellationToken cancellationToken) { var notices = await session.ReadConflictsAsync(cancellationToken).ConfigureAwait(true); @@ -1329,8 +1894,23 @@ internal sealed partial class VaultViewModel( } } - partial void OnSelectedHostChanged(HostRowViewModel? value) => - OnPropertyChanged(nameof(SelectedHostUsesAKey)); + partial void OnSelectedHostChanged(HostRowViewModel? value) + { + OnPropertyChanged(nameof(SelectedHostAsksForAPassword)); + OnPropertyChanged(nameof(SelectedHostAuthenticationNote)); + } + + /// + /// Both, on every change. A selector that highlights the showing section and a column that shows the + /// selected one are the same fact read from two directions, and raising only the one that became true + /// would leave the other button lit. + /// + partial void OnSectionChanged(VaultSection value) + { + OnPropertyChanged(nameof(ShowsHosts)); + OnPropertyChanged(nameof(ShowsKeys)); + OnPropertyChanged(nameof(ShowsCredentials)); + } /// /// The editing id is set before this flips in every path that opens the editor, and cleared after it diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index b6dd128..5aad3ef 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -90,28 +90,35 @@ - + IsVisible="{Binding Vault.SelectedHostAsksForAPassword}" + ToolTip.Tip="Typed each time and never stored. To stop typing it, add a password under Passwords and bind this host to it in the host's own editor." /> - + IsVisible="{Binding !Vault.SelectedHostAsksForAPassword}" /> /// + /// /// Exposed as a property rather than left for the window to find by name, because the name is now inside - /// this control's template and the window cannot see it. Which is the better arrangement anyway: when - /// this column shows one list at a time, "the list the keyboard belongs to" is a question only the column + /// this control's template and the window cannot see it. Which is the better arrangement anyway: the + /// column shows one list at a time, so "the list the keyboard belongs to" is a question only the column /// can answer, and answering it here means the window never has to know how many lists there are. + /// + /// + /// It has to be the list that is on screen, not a fixed one. Focus() on a collapsed control is + /// measurably a no-op and is not replayed when the control is revealed, so returning the host list while + /// the keys section is showing would swallow the keyboard: the terminal would let go and nothing would + /// take it. + /// + /// + /// Read from the view model rather than from the controls' own IsVisible, because that is the + /// direction the truth flows — the section is the state and the visibility is a binding to it. Asking the + /// controls would answer the same question one indirection later, and would answer it wrongly for the + /// moment between a section change and the next layout pass. + /// /// - internal IInputElement KeyboardTarget => HostList; + internal IInputElement KeyboardTarget => DataContext switch + { + VaultViewModel { ShowsKeys: true } => KeyList, + VaultViewModel { ShowsCredentials: true } => CredentialList, + _ => HostList, + }; } diff --git a/tests/DodoSSH.Client.App.Layout.Tests/VaultColumnLayoutTests.cs b/tests/DodoSSH.Client.App.Layout.Tests/VaultColumnLayoutTests.cs index c557f95..0b8d074 100644 --- a/tests/DodoSSH.Client.App.Layout.Tests/VaultColumnLayoutTests.cs +++ b/tests/DodoSSH.Client.App.Layout.Tests/VaultColumnLayoutTests.cs @@ -16,10 +16,16 @@ namespace DodoSSH.Client.App.Layout.Tests; /// /// /// -/// The column is 340 pixels wide and holds a list and an editor per item type, and the only thing keeping it -/// from clipping its own Save button at the window's minimum height is a state rule — one editor open at a -/// time. That rule was added on the strength of an argument, not a measurement, and this suite is the -/// measurement. +/// The column is 340 pixels wide and holds a list and an editor per item type, of which it shows one type at a +/// time. This suite is the measurement behind that arrangement: the column used to stack both types and keep +/// itself from clipping its own Save button with a state rule — one editor open at a time — and that rule was +/// added on the strength of an argument. The argument was right about the stacked column and is now moot, +/// which is a thing this suite found rather than assumed. See +/// . +/// +/// +/// One test per section, and one per section with its editor open, because that is the full set of shapes a +/// user can put this column into. A third section will add two more. /// /// /// A real VaultViewModel over a real unlocked vault, rather than a stand-in. Compiled bindings resolve @@ -87,13 +93,13 @@ public sealed class VaultColumnLayoutTests : IAsyncLifetime } [Fact] - public async Task TheColumnFitsWithNoEditorOpen() + public async Task TheHostsSectionFitsWithNoEditorOpen() { await MeasureAsync(faults => faults.ShouldBeEmpty()); } [Fact] - public async Task TheColumnFitsWithTheHostEditorOpen() + public async Task TheHostsSectionFitsWithItsEditorOpen() { vault.NewHostCommand.Execute(null); vault.IsEditing.ShouldBeTrue(); @@ -102,12 +108,23 @@ public sealed class VaultColumnLayoutTests : IAsyncLifetime } [Fact] - public async Task TheColumnFitsWithTheKeyEditorOpen() + public async Task TheKeysSectionFitsWithNoEditorOpen() { - // The tall one: a private key needs a real text area, and this is the editor the MaxHeight on the key - // list exists to make room for. + vault.ShowSectionCommand.Execute(VaultSection.Keys); + vault.ShowsKeys.ShouldBeTrue(); + + await MeasureAsync(faults => faults.ShouldBeEmpty()); + } + + [Fact] + public async Task TheKeysSectionFitsWithItsEditorOpen() + { + // The tall one: a private key needs a real text area, and this editor is what the key list used to + // hide itself and cap its own height for. Both workarounds are gone, so this measurement is now the + // only thing saying they were not needed. vault.NewKeyCommand.Execute(null); vault.IsEditingKey.ShouldBeTrue(); + vault.ShowsKeys.ShouldBeTrue("opening an editor has to bring its own section into view"); vault.KeyEditorPrivateKey = string.Join( '\n', @@ -117,35 +134,205 @@ public sealed class VaultColumnLayoutTests : IAsyncLifetime } [Fact] - public async Task BothEditorsAtOnce_DoNotFit_WhichIsWhyTheRuleExists() + public async Task TheCredentialsSectionFitsWithNoEditorOpen() { - // The justification for KeyEditorIsInTheWay/HostEditorIsInTheWay, turned from an argument in a comment - // into a number. The flags are set directly because the commands refuse this on purpose — the point is - // to measure what the refusal is protecting. + vault.ShowSectionCommand.Execute(VaultSection.Credentials); + vault.ShowsCredentials.ShouldBeTrue(); + + await MeasureAsync(faults => faults.ShouldBeEmpty()); + } + + [Fact] + public async Task TheCredentialsSectionFitsWithItsEditorOpen() + { + vault.NewCredentialCommand.Execute(null); + vault.IsEditingCredential.ShouldBeTrue(); + vault.ShowsCredentials.ShouldBeTrue("opening an editor has to bring its own section into view"); + + await MeasureAsync(faults => faults.ShouldBeEmpty()); + } + + /// + /// The host editor is the one a third item type made taller: its authentication picker is now a ComboBox + /// with a two-line-capable item template, and the section it sits in is the only one holding a + /// NumericUpDown, a CheckBox and two paragraphs of hint text. Measured with the picker + /// populated, because an empty ComboBox is shorter than one showing a qualifier beside a label. + /// + [Fact] + public async Task TheHostEditorFitsWithTheAuthenticationPickerFull() + { + vault.SelectedHost = vault.Hosts[0]; + vault.EditSelectedHostCommand.Execute(null); + + vault.EditorAuthenticationChoices.Count + .ShouldBeGreaterThan(1, "the picker has to be populated for this to measure anything"); + + vault.EditorSelectedAuthentication = vault.EditorAuthenticationChoices + .First(choice => choice.Kind is AuthenticationKind.Credential); + + await MeasureAsync(faults => faults.ShouldBeEmpty()); + } + + [Fact] + public async Task BothEditorsOpen_NowFit_BecauseOnlyOneSectionIsLaidOut() + { + // This test used to assert the opposite, and its own comment said that if it ever started passing the + // rule it justified had become unnecessary. That has happened, and this is the record of it: the two + // editors are in different sections now and only one section is laid out, so the sizing argument for + // one-editor-at-a-time is dead. // - // If this test ever starts passing, the rule has become unnecessary and the comments claiming it is - // load-bearing have become false. That is a finding, not a flake: read it as "the column has room - // now", check what changed, and delete the rule rather than this test. + // The rule itself is not, and AnEditorIsInTheWay says why — an open key editor holds a pasted private + // key, and moving on would leave it in a form nobody can see. That is a state rule with a state + // reason, so it belongs in the shell's tests and not here. This suite's job was the sizing claim, and + // the honest thing to do with a measurement that has flipped is to keep measuring it. vault.IsEditing = true; vault.IsEditingKey = true; - await MeasureAsync(faults => faults.ShouldNotBeEmpty( - "one editor at a time is a workaround for a column that cannot hold two")); + await MeasureAsync(faults => faults.ShouldBeEmpty( + "one section at a time means two open editors are never laid out together")); + + vault.Section = VaultSection.Keys; + + await MeasureAsync(faults => faults.ShouldBeEmpty( + "and the same holds from the other side, where the taller editor is the visible one")); + } + + /// + /// + /// The one thing a wrong answer here breaks is unrecoverable from the keyboard: MainWindow takes the + /// keyboard off the terminal's native child window first and then focuses this target, so a target that + /// cannot take focus leaves the user with no focused element and no way back except the mouse. + /// + /// + /// Which is why this asserts that focus was taken rather than that the right control was named. + /// Naming is the cheap half and it was already right; taking it was not — a ListBox is not focusable + /// by default, so this call returned false against the column as it stood and the shipped release-the- + /// keyboard path did nothing. Two ways to fail, and only the assertion that runs the call sees both: a + /// control in the section that is not showing is collapsed, and Focus() on a collapsed control is a + /// no-op that is not replayed when it is revealed. + /// + /// + [Fact] + public async Task TheKeyboardTargetIsTheListThatIsOnScreenAndItTakesFocus() + { + await OnTheColumnAsync((column, _) => + { + column.KeyboardTarget.ShouldBeSameAs(column.HostList); + column.KeyboardTarget.Focus().ShouldBeTrue("the hosts section is showing"); + }); + + vault.ShowSectionCommand.Execute(VaultSection.Keys); + + await OnTheColumnAsync((column, _) => + { + column.KeyboardTarget.ShouldBeSameAs(column.KeyList); + column.KeyboardTarget.Focus().ShouldBeTrue("the keys section is showing"); + }); + + vault.ShowSectionCommand.Execute(VaultSection.Credentials); + + await OnTheColumnAsync((column, _) => + { + column.KeyboardTarget.ShouldBeSameAs(column.CredentialList); + column.KeyboardTarget.Focus().ShouldBeTrue("the credentials section is showing"); + }); + } + + /// + /// The same call in the state the section rule allows: an editor open, its own list still on screen behind + /// it. The key list used to collapse itself whenever its editor opened, so a target that followed the + /// section would have been a no-op in exactly the state a user is most likely to leave the terminal in. + /// + [Fact] + public async Task TheKeyboardTargetStillTakesFocusWithAnEditorOpen() + { + vault.NewKeyCommand.Execute(null); + + await OnTheColumnAsync((column, _) => + { + column.KeyList.IsEffectivelyVisible.ShouldBeTrue(); + column.KeyboardTarget.Focus().ShouldBeTrue(); + }); + } + + /// + /// The claim the whole arrangement rests on, and the one nothing else here would notice breaking: two + /// sections left visible at once would overlap in the row they share rather than clip, so every fit test + /// above would still pass while the column showed one list through another. + /// + [Fact] + public async Task OnlyOneSectionIsOnScreenAtOnce() + { + await AssertOnlyVisibleAsync(VaultSection.Hosts); + await AssertOnlyVisibleAsync(VaultSection.Keys); + await AssertOnlyVisibleAsync(VaultSection.Credentials); + } + + /// Shows one section and checks that it is the only one a user can see. + private async Task AssertOnlyVisibleAsync(VaultSection section) + { + vault.Section = section; + + await OnTheColumnAsync((column, _) => + { + var lists = new Dictionary + { + [VaultSection.Hosts] = column.HostList, + [VaultSection.Keys] = column.KeyList, + [VaultSection.Credentials] = column.CredentialList, + }; + + foreach (var (owner, list) in lists) + { + list.IsEffectivelyVisible.ShouldBe( + owner == section, + $"{owner} showing while {section} is selected"); + } + }); + } + + /// + /// The selector is the only way to reach a section, so a click that lands on nothing is a column with one + /// half of it walled off. Its buttons are covered by every fit test above — the harness treats a + /// as interactive — but that only proves they are inside the window. This proves they + /// are the size a pointer can find, which a zero-height row of buttons in a collapsed border would not be. + /// + [Fact] + public async Task TheSelectorIsBigEnoughToClick() + { + await OnTheColumnAsync((column, _) => + { + var buttons = column.SectionSelector.Children.OfType