diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index bc345f1..afda117 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs @@ -3761,7 +3761,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp return; } - var tab = new TerminalTabViewModel(e.Label, e.Address); + var tab = new TerminalTabViewModel(e.Label, e.Address) { HostId = e.HostId }; attempts[e.AttemptId] = tab; AdoptTab(tab); @@ -3825,6 +3825,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp // dropping it would leave a shell running with nothing in the window naming it. var adopted = new TerminalTabViewModel(e.SessionId, e.Label, e.Address) { + HostId = e.HostId, StartedAt = clock.GetUtcNow(), Cipher = NullIfEmpty(e.Cipher), HostKeyAlgorithm = NullIfEmpty(e.HostKeyAlgorithm), @@ -4036,13 +4037,22 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp } /// - /// The host this tab was opened against, or null when the keychain has nothing under that name. + /// The host this tab was opened against, or null when the keychain no longer holds it. /// /// /// - /// Matched on the label, which is the same handle matches on and for - /// the same reason: a tab outlives the vault that opened it, so it cannot hold an entity id that would - /// still mean anything after a lock. + /// ◆ Matched on the keychain item's id, never on the name. Nothing stops two hosts being called + /// the same thing — a team's vault beside a personal one makes it ordinary, and "db" is what people call + /// their database server in both — and this is the lookup dials the + /// answer to. A name match would take a dropped session on one machine and quietly reopen a shell on + /// another, with the tab, the status bar and the address all still naming the first. The id is carried + /// from the attempt that opened the tab; see . + /// + /// + /// It outlives a lock, which is what the name match was here to survive. The id is the item's own + /// sync identity rather than a reference to a row object, so a vault that is locked and opened again + /// rebuilds its list around the same ids and a tab from before the lock finds its host in it. A host + /// renamed since the tab opened is found too. /// /// /// Null is the ordinary answer for two real cases, and both of them are why the reconnect below has to be @@ -4051,10 +4061,9 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// /// private HostRowViewModel? HostFor(TerminalTabViewModel? tab) => - tab is null || Vault is not { } vault + tab?.HostId is not { } hostId || Vault is not { } vault ? null - : vault.Hosts.FirstOrDefault( - row => string.Equals(row.Label, tab.Label, StringComparison.Ordinal)); + : vault.Hosts.FirstOrDefault(row => row.EntityId == hostId); /// /// Whether the selected tab has stopped and there is something to dial again. @@ -4382,10 +4391,15 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// /// /// - /// Matched on the label, which is what a tab was named after, because that is the only handle the two - /// lists share — a tab outlives the vault that opened it, so it cannot hold an entity id that would - /// still mean anything after a lock. Two hosts sharing a name would light both dots, which is a smaller - /// wrong than a dot that goes dark when the vault is reopened. + /// Matched on the keychain item's id, the same handle matches on and for the + /// reason given there: two hosts may share a name, and this list is read as "which machines am I on". + /// Lighting both dots for a name two machines answer to would say the user has a shell open on a host + /// they have never connected to — and it is the same match that decides whose pinned paths the sidebar + /// offers, which would then be another host's folders. + /// + /// + /// A tab with no item behind it — a machine reached by typing an address — lights nothing, which is + /// correct: there is no row here that it is a session on. /// /// /// is rebuilt here, on the same match, rather than from a subscription @@ -4408,14 +4422,11 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp foreach (var host in vault.Hosts) { - host.IsConnected = Tabs.Any( - tab => tab.IsLive && string.Equals(tab.Label, host.Label, StringComparison.Ordinal)); + host.IsConnected = Tabs.Any(tab => tab.IsLive && tab.HostId == host.EntityId); // The active tab's host, and only when it is actually connected: the sidebar is for a session // that is open, not for whichever machine's tab happens to be selected while it is still dialling. - if (host.IsConnected - && SelectedTab is { } selected - && string.Equals(host.Label, selected.Label, StringComparison.Ordinal)) + if (host.IsConnected && SelectedTab is { } selected && selected.HostId == host.EntityId) { connectedActiveHost = host; } @@ -4549,15 +4560,14 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// is drawn and not what it does. It shares 's plumbing — the same /// refusal, the same re-found row, the same password-sheet branch for a host /// that cannot be dialled unattended — through , and the host is found by - /// the same label match used to decide the row is there to click. + /// the same match used to decide the row is + /// there to click — which is the point: the pins on screen and the host they are opened on come from one + /// answer to "which machine is this tab", and two hosts of the same name cannot pull them apart. /// [RelayCommand] private async Task OpenPinnedPathAsync(string path) { - if (Vault is not { } vault - || SelectedTab is not { } tab - || vault.Hosts.FirstOrDefault( - host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host) + if (HostFor(SelectedTab) is not { } host) { return; } @@ -4638,10 +4648,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [RelayCommand] private void PinFolderFromSidebar() { - if (Vault is not { } vault - || SelectedTab is not { } tab - || vault.Hosts.FirstOrDefault( - host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host) + if (Vault is not { } vault || HostFor(SelectedTab) is not { } host) { return; } @@ -4674,10 +4681,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [RelayCommand] private async Task SelectFilesHostAsync(TerminalTabViewModel tab) { - if (Vault is not { } vault - || tab is null - || vault.Hosts.FirstOrDefault( - host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host) + if (HostFor(tab) is not { } host) { return; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs index c415559..b99c28e 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs @@ -229,6 +229,31 @@ internal sealed partial class TerminalTabViewModel : ObservableObject internal string Label { get; } + /// + /// The keychain item this tab was opened from, or null for a machine reached by typing an address. + /// + /// + /// + /// ◆ What the shell matches this tab back to a host row with, because the name will not do it. + /// Nothing stops two hosts being called the same thing, and a team's vault beside a personal one makes it + /// ordinary — so a lookup on can answer with a different machine that happens to + /// share the name. The reconnect under a stopped tab dials whatever that lookup finds, which is + /// the one place being approximately right is dialling a stranger. See + /// MainWindowViewModel.HostFor. + /// + /// + /// It survives a lock, which the row objects do not. This is the item's sync identity rather than + /// an index into the list on screen — see VaultItem<TSecret>.EntityId — so a vault that is + /// locked and opened again rebuilds every row around the same ids, and a tab that outlived the vault + /// finds its host again. A rename finds it too, which a name match by definition cannot. + /// + /// + /// Null is not missing data: it is the whole of what makes a typed-in address different, and it is why + /// such a tab is honestly refused a reconnect rather than offered one that would dial a namesake. + /// + /// + internal Guid? HostId { get; init; } + /// The account and endpoint, for the pane header and the status bar. internal string Address { get; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index d75c868..aa38bdc 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -947,6 +947,7 @@ internal static class ItemBadge /// /// The display name of the key or credential that authenticated, or null when a typed password did. /// +/// The keychain item this was dialled from, or null for a typed-in address. internal sealed class TerminalSessionEventArgs( Guid attemptId, uint sessionId, @@ -954,7 +955,8 @@ internal sealed class TerminalSessionEventArgs( string address, string cipher, string hostKeyAlgorithm, - string? identityLabel) : EventArgs + string? identityLabel, + Guid? hostId) : EventArgs { /// Which attempt this session came out of. /// @@ -971,19 +973,27 @@ internal sealed class TerminalSessionEventArgs( internal string HostKeyAlgorithm { get; } = hostKeyAlgorithm; internal string? IdentityLabel { get; } = identityLabel; + + /// + internal Guid? HostId { get; } = hostId; } /// A connection that has been asked for, and has not answered yet. /// Identifies this attempt for the whole of its life. /// The host's name, as the vault has it. /// Who this will be logged in as, and where. +/// The keychain item this is being dialled from, or null for a typed-in address. /// /// The vault says a connection has started before it says whether it worked, so that the shell can put a /// tab in the strip at the moment the user asks for one rather than however many seconds later a handshake /// takes. Everything a tab needs to name itself is here, because the name is a decrypted item and the shell /// has no vault to read it from. /// -internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, string address) : EventArgs +internal sealed class ConnectionAttemptEventArgs( + Guid attemptId, + string label, + string address, + Guid? hostId) : EventArgs { /// Identifies this attempt for the whole of its life. /// @@ -995,6 +1005,19 @@ internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, s internal string Label { get; } = label; internal string Address { get; } = address; + + /// + /// The keychain item this connection was made from, or null for a machine that is not in the keychain. + /// + /// + /// ◆ The name is not an identifier and this is. Two hosts may be called the same thing — nothing + /// stops it, and a team vault next to a personal one makes it ordinary — so a tab that remembered only + /// what it was called could be matched back to the wrong machine. That is not a cosmetic mix-up: the + /// reconnect below the tab dials whichever row the match found, so the one thing that must not be + /// guessed is which machine a reconnect goes to. Carried from here because a tab has no vault to ask — + /// see TerminalTabViewModel.HostId. + /// + internal Guid? HostId { get; } = hostId; } /// A connection that has got as far as a named step. @@ -11326,7 +11349,8 @@ internal sealed partial class VaultViewModel( var attempt = new ConnectionAttemptEventArgs( Guid.CreateVersion7(), target.Label, - Dialled(target, authentication)); + Dialled(target, authentication), + target.HostId); ConnectionStarting?.Invoke(this, attempt); Status = $"Connecting to {target.Label}…"; @@ -11784,7 +11808,8 @@ internal sealed partial class VaultViewModel( Dialled(target, authentication), facts?.Cipher ?? string.Empty, facts?.HostKeyAlgorithm ?? string.Empty, - authentication.IdentityLabel)); + authentication.IdentityLabel, + target.HostId)); // Last, and after the tab exists: keeping the password is a favour, and the session the user asked // for must not wait on a vault write to appear. diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 2973a77..564c804 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -2099,6 +2099,82 @@ public sealed partial class ShellFlowTests : IAsyncLifetime "the session that was replaced should have been released with its pane"); } + /// + /// + /// ◆ THE ONE THIS EXISTS FOR: a reconnect that dialled a different machine of the same name. Nothing in + /// this application stops two hosts being called the same thing — a team's vault beside a personal one + /// makes it ordinary, and "prod-db" is what a database server gets called in both — and a tab used to + /// find its host by that name. A dropped session on one machine then came back as a live shell on the + /// other, with the tab, the address and the status bar all still naming the first. + /// + /// + /// Asserted on the address actually dialled rather than on anything the tab says about itself. The two + /// rows agree on every string a tab carries, which is exactly why this went unnoticed: a test written + /// against the label would have passed on the day the wrong machine was being opened. + /// + /// + [Fact] + public async Task ReconnectingATabWhoseHostHasANamesake_DialsTheMachineTheTabWasOpenedOn() + { + var vault = await ReadyToConnectAsync(); + + await AddKeyAsync(vault, "deploy"); + + // A second host under the name the fixture's own already has, on a different machine. + await AddHostAsync(vault, "prod-db", "db-replica.internal"); + + // Both bound to the key, so neither is diverted to the password sheet: what this asserts is which of + // the two gets dialled, not whether either can be. Re-found between the two, because saving a host + // rebuilds the list. + await BindKeyAsync(vault, Machine(vault, "db.internal"), vault.Keys[0].EntityId); + await BindKeyAsync(vault, Machine(vault, "db-replica.internal"), vault.Keys[0].EntityId); + + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + await vault.ConnectToRowCommand.ExecuteAsync(Machine(vault, "db-replica.internal")); + + var tab = shell.Tabs.ShouldHaveSingleItem(); + ssh.Requests.ShouldHaveSingleItem().Host.ShouldBe("db-replica.internal"); + + ssh.Shells[0].Drop("An established connection was aborted by the server."); + await EventuallyAsync(() => !tab.IsLive, "the dropped shell should have ended the session"); + + shell.CanReconnectSelectedTab.ShouldBeTrue("the machine this tab was opened on is still there"); + + await shell.ReconnectTabCommand.ExecuteAsync(tab); + + tab.IsLive.ShouldBeTrue(tab.Status); + + ssh.Requests.Count.ShouldBe(2); + ssh.Requests[1].Host.ShouldBe( + "db-replica.internal", + "a reconnect dials the machine the tab was opened on, not the one that shares its name"); + } + + /// + /// The same mix-up as the test above, where it is read rather than dialled. A green dot is the host list + /// answering "am I on this machine", and the name two rows share is not an answer to it — a dot on the + /// machine nobody has a shell on claims a session that does not exist, and takes QUICK ACCESS's pins with + /// it, since the sidebar offers the pinned folders of whichever row that same match found. + /// + [Fact] + public async Task ConnectingToOneOfTwoHostsOfTheSameName_LightsThatOneAlone() + { + var vault = await ReadyToConnectAsync(); + + await AddHostAsync(vault, "prod-db", "db-replica.internal"); + + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + + vault.SelectedHost = Machine(vault, "db-replica.internal"); + await vault.ConnectCommand.ExecuteAsync(null); + + shell.Tabs.ShouldHaveSingleItem().IsLive.ShouldBeTrue(); + + Machine(vault, "db-replica.internal").IsConnected.ShouldBeTrue(); + Machine(vault, "db.internal").IsConnected.ShouldBeFalse( + "nobody has a shell on this machine; only its name was connected to"); + } + /// /// A connection that was refused is worth retrying for the same reason a dropped one is, and the /// commonest refusal — a machine that was not up yet — is the one most likely to succeed a minute later. @@ -10377,11 +10453,14 @@ public sealed partial class ShellFlowTests : IAsyncLifetime await vault.SaveHostCommand.ExecuteAsync(null); } - private static async Task AddHostAsync(VaultViewModel vault, string label) + private static async Task AddHostAsync( + VaultViewModel vault, + string label, + string hostname = "db.internal") { vault.NewHostCommand.Execute(null); vault.EditorLabel = label; - vault.EditorHostname = "db.internal"; + vault.EditorHostname = hostname; vault.EditorUsername = "deploy"; await vault.SaveHostCommand.ExecuteAsync(null); @@ -10405,6 +10484,14 @@ public sealed partial class ShellFlowTests : IAsyncLifetime private static HostRowViewModel Host(VaultViewModel vault, string label) => vault.Hosts.Single(row => string.Equals(row.Label, label, StringComparison.Ordinal)); + /// The host row for a given machine, for the tests whose rows deliberately share a name. + /// + /// cannot be asked when two rows are called the same thing, which is the whole + /// situation those tests set up. Re-found rather than held, because saving any host rebuilds the list. + /// + private static HostRowViewModel Machine(VaultViewModel vault, string hostname) => + vault.Hosts.Single(row => string.Equals(row.Host.Hostname, hostname, StringComparison.Ordinal)); + /// Deletes the selected host: the question, and then the answer to it. /// /// Both halves, because both are what deleting anything now takes — arming on its own changes nothing,