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