DodoTech-Public/DodoSSH
Public Access
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Make Connections the place a connection is made, and put the keychain away

Browse Source 
Four changes to the phone, and the last one needed the connect path taking
apart.

**The bottom bar is three entries.** The keychain moved onto the hub, which is
now SETTINGS with a gear rather than MORE with a hamburger. A bottom bar is for
the places a session moves between, and keys, credentials and tags are managed
occasionally and then left alone — which is the shape of everything already
behind that hub. With the keychain on it, "more" stopped being a description of
what is there. `ShellScreen.Vault` joining `IsMoreSurface` is the whole of the
change: the tab that lights, the header that stands down and the back gesture's
first case all read that one property, which is why the switch mirrors it by
construction rather than by a second list. The keychain screen grew the header
every hub screen has, because the shell's own is not above it any more and
without one there would be no back arrow and nothing saying what the list is.

The desktop keeps its Keychain rail entry. A rail with nine slots has room, so
this is the second thing the two heads arrange deliberately differently, after
the hub itself.

**Terminal became Connections**, and the word does more work than a rename
usually does — see below. The enum member stays `ShellSurface.Terminal`, for
the reason the tab was never called Vault: the surface is a terminal, and the
word a user reads is the product's.

**The + puts the software keyboard away.** It sits above a terminal somebody is
typing into, so the sheet it raises was arriving underneath a keyboard covering
the half of the screen the sheet is on — and worse, laid out into the strip
left above it, since the keyboard's inset shortens everything this head draws.
Avalonia cannot do this and it is worth knowing why: `TopLevel.InputPane`
reports the keyboard and offers nothing that closes one, because the framework's
model is that it belongs to whatever has focus — and this keyboard was raised by
the `WebView`'s own text input, by a native view Avalonia's focus manager never
owned. Clearing Avalonia's focus leaves it exactly where it is. So
`Platform/SoftKeyboard.cs` asks `InputMethodManager`, off the decor view's
window token, and every step of it is allowed to be absent.

**With nothing open, Connections is a connect screen rather than an empty
state.** A box taking `user@host` or `user@host:port`, a password, and the
machines most recently connected to underneath. The box is the only path in this
product to a machine the keychain has never heard of, which is a real case it
had no answer for: an address somebody was handed five minutes ago. A typed
password and nothing else — offering the keychain's keys would be a second
binding resolution beside `TryBuildAuthentication`, and the argument against a
second one is written there at length. Nothing typed is saved, and the screen
says so: a machine worth keeping belongs on HOSTS, where it can carry a key, a
group's defaults and a name.

The recents come out of the vault's own connection log rather than a list kept
in this process, so they survive a restart and arrive on a new phone with the
keychain. Deduplicated by address, because this is a list of places and not of
events, and capped at six so the box stays above the keyboard. Emptied when the
vault is — they are decrypted entries naming where somebody works, and a lock
that left them on screen would be a list still readable after every key that
decrypted it was zeroed. Tapping one leads to whichever of two things it is: a
keychain host goes to that host's connect bar, where its key, its password box
and its refusals already live, and an address goes back into the box, without
the password, whose absence is the point of that path rather than a gap in it.

**The connect path was shaped like `HostRowViewModel` all the way down.** The
log entry, the identification, the failure record and the retry all took a row.
They take a four-field `ConnectionTarget` now, so a connection to an address
shares the ladder of refusals, the host-key question and the tab's lifecycle
rather than growing a second copy of them. `ConnectionRecorder.Record` and
`Identify` have always taken a nullable host id, so the log could already hold a
connection with no item behind it.

One behavioural change falls out of that and it is the one to know about:
**trusting a host key now retries the attempt that raised the question** instead
of re-running whichever host is selected. That was correct while a selected host
was the only way to connect; with a manual target it would dial a different
machine, or refuse with "choose a host first" over a key the user has just
agreed to trust. The test selects a host first, so a regression cannot pass by
connecting to the wrong thing successfully.

`LogsViewModel.ReloadAsync` split so the connections half can be read alone.
Reading the keychain's activity for a screen that offers neither would double
the decryption on the list that was already the expensive one.

Twelve tests: the parse grammar as a theory over seven refusals, the dialled
request, the retry, and both branches of tapping a recent row. The recents rows
are built by hand rather than connected-and-closed — what those tests are about
is which branch a row takes, and driving it through the recorder's queue would
test the recorder, which `DodoSSH.Client.Session.Tests` already does. What needs
a device is phases 11.6 to 11.9 of `docs/manual-checks.md`.
This commit is contained in:
jaap-jan
2026-08-03 15:26:47 +02:00
parent a2f0d4813a
commit f5ffd1983d
15 changed files with 1028 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs
+216
View File
@@ -1,4 +1,5 @@
using DodoSSH.Client.Auth;
using DodoSSH.Client.Domain;
using DodoSSH.Client.Import;
using DodoSSH.Client.Session;
// FakeDeviceKeyStore is compiled into this assembly from a source link and keeps its original namespace;
@@ -7,6 +8,7 @@ using DodoSSH.Client.Session.Tests;
using DodoSSH.Client.Shell.ViewModels;
using DodoSSH.Client.Ssh;
using DodoSSH.Client.Storage;
using DodoSSH.Client.Sync;
using DodoSSH.Client.Terminal;
using DodoSSH.Crypto;
@@ -687,6 +689,220 @@ public sealed class ShellFlowTests : IAsyncLifetime
shell.Screen.ShouldBe(ShellScreen.Vault);
}
/// <remarks>
/// The one connection this application makes to a machine that is not in the keychain. What is worth
/// pinning is that it is dialled exactly as typed and nothing is inferred — the account, the address and
/// the port all come out of the one box.
/// </remarks>
[Fact]
public async Task AManualTarget_IsDialledExactlyAsItWasTyped()
{
var vault = await ReadyToConnectAsync();
await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
vault.ManualTarget = " deploy@build.internal:2222 ";
vault.ManualPassword = "hunter2";
await vault.ConnectManuallyCommand.ExecuteAsync(null);
var request = ssh.Requests.ShouldHaveSingleItem();
request.Host.ShouldBe("build.internal");
request.Port.ShouldBe(2222);
request.Username.ShouldBe("deploy");
request.Credential.ShouldBeOfType<SshPasswordCredential>().Password.ShouldBe("hunter2");
shell.IsTerminalShowing.ShouldBeTrue();
shell.Tabs.ShouldHaveSingleItem().Label.ShouldBe("deploy@build.internal");
}
[Fact]
public async Task AManualTargetWithNoPort_TakesTwentyTwo()
{
var vault = await ReadyToConnectAsync();
await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
vault.ManualTarget = "root@box";
vault.ManualPassword = "hunter2";
await vault.ConnectManuallyCommand.ExecuteAsync(null);
ssh.Requests.ShouldHaveSingleItem().Port.ShouldBe(22);
}
/// <remarks>
/// <c>ssh</c> would fall back to this machine's own account name. A phone's is the Android user, which
/// is never a login on anything, so the guess would fail at the remote as "authentication failed"
/// rather than here as a sentence about the box that was typed into.
/// </remarks>
[Theory]
[InlineData("", "Type a machine")]
[InlineData("build.internal", "Say who to log in as")]
[InlineData("deploy@", "Say who to log in as")]
[InlineData("@build.internal", "Say who to log in as")]
[InlineData("deploy@build.internal:70000", "between 1 and 65535")]
[InlineData("deploy@build.internal:ssh", "between 1 and 65535")]
[InlineData("deploy@[fe80::1]", "bracketed IPv6")]
public async Task AManualTargetThatCannotBeRead_IsRefusedBeforeAnythingIsDialled(
string typed,
string because)
{
var vault = await ReadyToConnectAsync();
await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
vault.ManualTarget = typed;
vault.ManualPassword = "hunter2";
await vault.ConnectManuallyCommand.ExecuteAsync(null);
vault.ManualStatus.ShouldContain(because);
ssh.Requests.ShouldBeEmpty();
shell.Tabs.ShouldBeEmpty("a refusal is not an attempt, so there is no tab to explain it");
}
/// <remarks>
/// A password is the only thing this path can authenticate with, so an empty one is refused here rather
/// than sent. Offering the keychain's keys would be a second binding resolution beside the connect
/// path's own, which is the thing <c>TryBuildAuthentication</c> exists to be the only copy of.
/// </remarks>
[Fact]
public async Task AManualTargetWithNoPassword_SaysSoRatherThanDiallingWithoutOne()
{
var vault = await ReadyToConnectAsync();
await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
vault.ManualTarget = "root@box";
await vault.ConnectManuallyCommand.ExecuteAsync(null);
vault.ManualStatus.ShouldContain("password");
ssh.Requests.ShouldBeEmpty();
}
/// <remarks>
/// <b>The retry used to re-run whichever host was selected.</b> That was right while a selected host was
/// the only way to connect; with a manual target it would answer "do you trust this key" by dialling a
/// different machine — or by refusing with "choose a host first" over a key the user has just agreed to
/// trust. A host is deliberately selected here, so a retry that ignored the attempt would connect and
/// the assertion would still catch it.
/// </remarks>
[Fact]
public async Task TrustingAHostKey_RetriesTheAttemptThatRaisedItRatherThanTheSelectedHost()
{
var vault = await ReadyToConnectAsync();
await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
vault.SelectedHost.ShouldNotBeNull();
ssh.Failure = new SshHostKeyUnknownException(
new HostKeyPresentation("build.internal", 2222, "ssh-ed25519", "SHA256:unknown"));
vault.ManualTarget = "deploy@build.internal:2222";
vault.ManualPassword = "hunter2";
await vault.ConnectManuallyCommand.ExecuteAsync(null);
vault.HasPendingHostKey.ShouldBeTrue();
ssh.Failure = null;
await vault.TrustHostKeyCommand.ExecuteAsync(null);
ssh.Requests.Count.ShouldBe(2);
ssh.Requests[1].Host.ShouldBe("build.internal", "the retry is the attempt that asked the question");
ssh.Requests[1].Port.ShouldBe(2222);
ssh.Requests[1].Username.ShouldBe("deploy");
}
/// <remarks>
/// A recent row is one of two different things, and tapping it has to lead to whichever one it is. The
/// keychain half goes to the host's own connect bar rather than connecting from here, because that bar
/// is where its key, its password box and its refusals already live.
/// </remarks>
[Fact]
public async Task ARecentConnectionNamingAKeychainHost_OpensThatHostOnTheHostsScreen()
{
var vault = await ReadyToConnectAsync();
var host = vault.Hosts[0];
vault.SelectedHost = null;
shell.ShowScreenCommand.Execute(ShellScreen.Preferences);
shell.ConnectToRecentCommand.Execute(Recent("prod-db", "root@prod-db:22", host.EntityId));
shell.IsHostsShowing.ShouldBeTrue();
vault.SelectedHost.ShouldBe(host);
vault.ManualTarget.ShouldBeEmpty("a keychain host is not dialled out of the manual box");
}
/// <remarks>
/// The other half. The log stored what was actually dialled, which is the grammar the manual box takes,
/// so it goes straight back in — without the password, which was never stored and whose absence is the
/// point of that path rather than a gap in it.
/// </remarks>
[Fact]
public async Task ARecentConnectionWithNoKeychainItem_GoesBackIntoTheManualBox()
{
var vault = await ReadyToConnectAsync();
// Deliberately somewhere else first, so "it did not navigate" is an assertion rather than the
// screen the application happens to open on.
shell.ShowTerminalCommand.Execute(null);
shell.ConnectToRecentCommand.Execute(
Recent("deploy@build.internal", "deploy@build.internal:2222", hostId: null));
vault.ManualTarget.ShouldBe("deploy@build.internal:2222");
vault.ManualPassword.ShouldBeEmpty();
shell.IsTerminalSurface.ShouldBeTrue("the box being filled in is on this surface");
}
/// <remarks>
/// A host deleted since it was connected to. The machine is still there and the keychain no longer knows
/// about it, so the address is the honest answer rather than a tap that does nothing.
/// </remarks>
[Fact]
public async Task ARecentConnectionNamingAHostThatHasGone_FallsBackToTheAddress()
{
var vault = await ReadyToConnectAsync();
shell.ShowTerminalCommand.Execute(null);
shell.ConnectToRecentCommand.Execute(
Recent("prod-db", "root@prod-db:22", Guid.CreateVersion7()));
vault.ManualTarget.ShouldBe("root@prod-db:22");
shell.IsTerminalSurface.ShouldBeTrue();
}
/// <summary>One row of the connection log, built by hand.</summary>
/// <remarks>
/// Built rather than connected-and-closed, because what these three tests are about is which of the two
/// branches a row takes — and driving that through a real connection, a real close and the recorder's own
/// queue would test the recorder instead, which <c>DodoSSH.Client.Session.Tests</c> already does.
/// </remarks>
private static ConnectionLogRowViewModel Recent(string label, string address, Guid? hostId) =>
new(
new VaultItem<ConnectionLogSecret>(
Guid.CreateVersion7(),
new ConnectionLogSecret
{
HostLabel = label,
Address = address,
HostId = hostId,
StartedAt = DateTimeOffset.UnixEpoch,
DeviceName = "a phone",
},
Version: 1,
HasUnsyncedChanges: false,
IsBlocked: false,
IsReadOnly: false),
isLive: false);
/// <remarks>
/// The phone's connect menu is drawn over the terminal's own rectangle, so it obeys the rule the palette
/// does: whatever covers the renderer collapses it instead. The surface stays, because the bar the menu