diff --git a/README.md b/README.md index d32c930..aebc7e9 100644 --- a/README.md +++ b/README.md @@ -132,8 +132,9 @@ dotnet run --project src/DodoSSH.Client.App In the app, enter `http://localhost:5233` as the server. Your browser opens for sign-in — the realm ships `alice` / `alice` — then choose a vault passphrase and **write down the recovery code**, which cannot be -skipped and cannot be recovered from the server. You can then add a host and open a shell on it. Keycloak's -admin console is at `http://localhost:18080` (`admin` / `admin`). +skipped and cannot be recovered from the server. You can then add a host and open a shell on it — double-click +it in the sidebar, or select it and press **CONNECT**, which is the same command with the password box beside +it. Keycloak's admin console is at `http://localhost:18080` (`admin` / `admin`). You can also add an SSH key, which is stored in the vault like a host and synced the same way: paste the private key, then edit a host and pick that key from its **key** dropdown. From then on that host @@ -146,6 +147,14 @@ the next sync. If a server is legitimately rebuilt and offers a new key, the con with no way to continue from the warning — edit the host and choose **Forget host key**, which is deliberately somewhere you have to go on purpose. +**Deleting asks first, and the question is worth reading.** DELETE on a host, an SSH key or a stored password +puts a question where the buttons were, and what it says is counted rather than generic: how many hosts +authenticate with the key about to go — they refuse to connect afterwards rather than falling back to a typed +password — whether a terminal is open on the host about to go, and whether this machine can push the deletion +yet or is queuing it. There is no undo, which is the other thing it says. Withdrawing host key trust is the +deliberate exception: it costs one fingerprint check on the next connection, and the dangerous button there is +the one that *adds* trust. + **Signing in once is enough.** The refresh token is kept in the local cache, sealed under the vault's own key, so a later launch resumes the session itself and no browser opens — and because it is sealed under that key, resuming can only happen *after* the vault is unlocked. A machine that unlocks with no network keeps @@ -186,6 +195,9 @@ beside its destination and is renamed into place at the end, so an interrupted t mistaken for a finished one — which matters most for what people actually use this for, which is copying a build artefact onto a server and then running it. A destination that already exists is refused outright rather than overwritten; the remote pane has **DELETE** and **MKDIR** so that refusal is not a dead end. +DELETE asks first and names the full path, and it carries the strongest warning in the application on +purpose: everything else DodoSSH deletes is a tombstone against a copy the server still holds, and a file on +somebody's host is bytes with nothing behind them. **RESUME** on a stopped transfer carries on from what the part file already holds. Resume works within a run of the application and not across a restart, and that limit is deliberate: nothing diff --git a/src/DodoSSH.Client.App/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.App/ViewModels/TransfersViewModel.cs index d3a2bbf..6be72ec 100644 --- a/src/DodoSSH.Client.App/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/TransfersViewModel.cs @@ -174,6 +174,40 @@ internal sealed partial class TransferRowViewModel(TransferSnapshot snapshot) : } } +/// +/// Something on the host that has been asked about and not yet agreed to. +/// +/// +/// +/// The one deletion in this application that nothing can walk back. A vault item is a tombstone against a +/// copy the server still holds until the pass lands; a file on somebody's host is bytes, and this screen +/// has no wastebasket to put them in. +/// +/// +/// It carries the full path rather than only the name, because the name is the half that does not identify +/// anything: config in the directory that was showing a moment ago and config in the one +/// showing now look identical in a confirmation, and only one of them is the file somebody meant. +/// +/// +/// What the row was called. +/// Where it is, which is what the question actually promises to delete. +/// Whether it is a directory, which the host treats differently. +internal sealed record RemoteDeletionRequest(string Name, string FullPath, bool IsDirectory) +{ + /// The question, naming the kind because the two behave differently. + internal string Question => IsDirectory + ? $"Delete the directory '{Name}' on the host?" + : $"Delete '{Name}' on the host?"; + + /// What it costs, which is everything: there is no copy here and no undo there. + internal string Consequence => IsDirectory + ? "It is removed on the host itself. The host refuses a directory that still has anything in it, so " + + "this either removes an empty one or fails — and if it goes, it is gone: nothing here keeps a " + + "copy and there is no undo." + : "It is removed on the host itself. Nothing here keeps a copy, the folder on this machine is not " + + "touched, and there is no undo."; +} + /// /// The transfers screen: a host, two directory panes, and the queue between them. /// @@ -284,6 +318,20 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo internal bool HasRemoteEntries => RemoteEntries.Count > 0; + /// The deletion on the host that has been asked about, or null when none has. + [ObservableProperty] + private RemoteDeletionRequest? pendingRemoteDeletion; + + internal bool IsConfirmingRemoteDeletion => PendingRemoteDeletion is not null; + + /// Whether the pane's DELETE is live. + /// + /// Off while its own question is up, so a second press cannot arm a second one behind the card — and + /// disabled rather than hidden, because this button sits in a row of three and a gap where it was would + /// move UP and REFRESH out from under the pointer. + /// + internal bool CanDeleteRemote => IsConnected && !IsConfirmingRemoteDeletion; + // ---- The local pane ---- [ObservableProperty] @@ -672,15 +720,16 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo } /// - /// Deletes the chosen remote file, or an empty directory. + /// Asks whether the chosen remote file, or empty directory, should go. /// /// - /// Not recursive, and the refusal comes from the server rather than from a check here — see - /// ISftpSession.DeleteAsync. It is offered because the queue refuses to overwrite: without a way - /// to remove what is in the way, "that file is already there" would be a dead end. + /// Deleting on the host is offered because the queue refuses to overwrite: without a way to remove what + /// is in the way, "that file is already there" would be a dead end. It is asked about first because of + /// what it is — the only thing this application destroys that neither the server nor this machine has a + /// copy of. /// [RelayCommand] - private async Task DeleteRemoteAsync(CancellationToken cancellationToken) + private void DeleteRemote() { if (SelectedRemoteEntry is not { } row) { @@ -688,18 +737,44 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo return; } + PendingRemoteDeletion = new RemoteDeletionRequest(row.Name, row.FullPath, !row.IsFile); + } + + /// + /// Deletes what was agreed to. + /// + /// + /// Not recursive, and the refusal comes from the server rather than from a check here — see + /// ISftpSession.DeleteAsync. It acts on the path the question named rather than on the selection, + /// which is what makes the question a promise: nothing between asking and answering can point it + /// somewhere else. + /// + [RelayCommand] + private async Task ConfirmDeleteRemoteAsync(CancellationToken cancellationToken) + { + if (PendingRemoteDeletion is not { } request) + { + return; + } + + PendingRemoteDeletion = null; + await RunAsync( - $"Deleting {row.Name}…", + $"Deleting {request.Name}…", async () => { - await RequireSession().DeleteAsync(row.FullPath, cancellationToken).ConfigureAwait(true); + await RequireSession().DeleteAsync(request.FullPath, cancellationToken).ConfigureAwait(true); await ListRemoteAsync(RemotePath, cancellationToken).ConfigureAwait(true); - Status = $"Deleted {row.Name}."; + Status = $"Deleted {request.Name}."; }).ConfigureAwait(true); } + /// Thinks better of it. + [RelayCommand] + private void CancelDeleteRemote() => PendingRemoteDeletion = null; + /// public async ValueTask DisposeAsync() { @@ -919,11 +994,28 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo { OnPropertyChanged(nameof(CanDownload)); OnPropertyChanged(nameof(CanUpload)); + OnPropertyChanged(nameof(CanDeleteRemote)); } - partial void OnSelectedRemoteEntryChanged(RemoteEntryRowViewModel? value) => + /// + /// Any change to the selection takes the question away, which is stricter than the vault's rule and can + /// afford to be: this list is refilled only by a navigation or a refresh somebody asked for, so there is + /// no background pass to pull a card out from under a reader. Listing and disconnecting both null the + /// selection, so this one hook covers all three ways the answer could stop being about what was asked. + /// + partial void OnSelectedRemoteEntryChanged(RemoteEntryRowViewModel? value) + { OnPropertyChanged(nameof(CanDownload)); + PendingRemoteDeletion = null; + } + + partial void OnPendingRemoteDeletionChanged(RemoteDeletionRequest? value) + { + OnPropertyChanged(nameof(IsConfirmingRemoteDeletion)); + OnPropertyChanged(nameof(CanDeleteRemote)); + } + partial void OnSelectedLocalEntryChanged(LocalEntryRowViewModel? value) => OnPropertyChanged(nameof(CanUpload)); diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index 45abb07..e4846be 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -422,6 +422,55 @@ internal sealed record VaultItemRowViewModel( internal bool HasBadge => Badge.Length > 0; } +/// Which list a deletion that has been asked for is aimed at. +internal enum DeletionTarget +{ + /// A host, from the sidebar beside the terminal. + Host, + + /// An SSH key, from the vault screen. + Key, + + /// A stored password, from the vault screen. + Credential, +} + +/// +/// A deletion that has been asked for and not yet agreed to. +/// +/// +/// +/// A state rather than a dialog, on the same reasoning as the sign-out confirmation — see +/// MainWindowViewModel.IsConfirmingSignOut. What makes it worth having at all is that the sentences +/// below are computed: how many hosts authenticate with the key about to go, whether a terminal is +/// open on the host about to go, and whether this machine can push the tombstone yet. A confirmation that +/// only said "are you sure?" would be a click to train people out of. +/// +/// +/// It carries the item's id rather than pointing at the selection, so that whatever moves the selection +/// between the question and the answer — a background sync, a filter, a click in the list — cannot turn an +/// agreement about one item into the deletion of another. +/// +/// +/// Which list to delete from. +/// The item the question is about. +/// The question itself, naming the item. +/// Where it goes, and how far. +/// +/// What is riding on this particular item — hosts that authenticate with it, a terminal open on it — or +/// empty when nothing is. The line that changes the answer, as opposed to the one every deletion shares. +/// +internal sealed record DeletionRequest( + DeletionTarget Target, + Guid EntityId, + string Question, + string Consequence, + string Usage) +{ + /// Whether anything depends on the item, which is the line worth reading twice. + internal bool HasUsage => Usage.Length > 0; +} + /// /// Gets this machine online, if it can be. /// @@ -857,6 +906,32 @@ internal sealed partial class VaultViewModel( /// The credential being edited, or null when creating. private Guid? editingCredentialId; + // ---- Deleting ---- + + /// The deletion that has been asked for, or null when nothing has been. + /// + /// One at a time, and one for all three kinds. Two armed deletions cannot be told apart by a user + /// looking at two cards, and this application only ever has one selected item per screen to aim a + /// question at. + /// + [ObservableProperty] + private DeletionRequest? pendingDeletion; + + internal bool IsConfirmingDeletion => PendingDeletion is not null; + + /// Whether the sidebar's row of host buttons is showing. + /// + /// Its own property because the markup cannot express !IsEditing && !IsConfirmingDeletion, + /// and because both halves are the same rule: the question about deleting a host takes the place of the + /// buttons that asked it, so that DELETE cannot be pressed a second time while its own confirmation is + /// on screen. + /// + internal bool ShowsHostActions => !IsEditing && !IsConfirmingDeletion; + + /// Whether the vault screen's Edit and Delete are showing. + /// + internal bool ShowsItemActions => SelectedItemIsEditable && !IsConfirmingDeletion; + // ---- Connecting ---- /// @@ -1068,7 +1143,7 @@ internal sealed partial class VaultViewModel( /// How many keys would not decrypt. /// /// 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 + /// aims at, and a list that picked a row on every background sync would point /// that button at a key nobody chose. /// private async Task ReloadKeysAsync(CancellationToken cancellationToken) @@ -1094,9 +1169,9 @@ internal sealed partial class VaultViewModel( /// 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. + /// as the key list and matters more here. reads the selection, so a list + /// that fell back to its first row would point the deletion — and the question in front of it — at a + /// password nobody chose. /// private async Task ReloadCredentialsAsync(CancellationToken cancellationToken) { @@ -1470,26 +1545,23 @@ internal sealed partial class VaultViewModel( } } - /// Deletes whatever the selected row is. + /// Asks about deleting whatever the selected row is. /// /// Pins are not deleted from here even though they can be. Withdrawing trust applies to an endpoint /// rather than to a row — every pin for the address goes — and calling that "delete" beside two buttons /// that remove exactly one item would misdescribe it. It has its own button, named for what it does. /// [RelayCommand] - private async Task DeleteSelectedItemAsync() + private void DeleteSelectedItem() { switch (SelectedVaultItem?.Kind) { - // Null rather than a token, and deliberately: a [RelayCommand] over a method whose only - // parameter is a CancellationToken generates ExecuteAsync(object? parameter) that ignores the - // argument and supplies a token from its own source. Passing one would read as plumbing. case VaultItemKind.Key: - await DeleteKeyCommand.ExecuteAsync(null).ConfigureAwait(true); + DeleteKeyCommand.Execute(null); break; case VaultItemKind.Credential: - await DeleteCredentialCommand.ExecuteAsync(null).ConfigureAwait(true); + DeleteCredentialCommand.Execute(null); break; default: @@ -1555,15 +1627,42 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } - /// Queues a tombstone for the selected host. + /// Asks whether the selected host should go. + /// + /// A terminal already open on the host is disclosed rather than prevented, because deleting a host does + /// not close one — a session outlives the row that opened it, exactly as it outlives a lock. Somebody + /// deleting a machine they are still working on should know that is what they have done. + /// [RelayCommand] - private async Task DeleteHostAsync(CancellationToken cancellationToken) + private void DeleteHost() { if (SelectedHost is not { } row) { return; } + PendingDeletion = new DeletionRequest( + DeletionTarget.Host, + row.EntityId, + $"Delete the host '{row.Label}'?", + HowFarADeletionGoes("The host and everything saved about it"), + row.IsConnected + ? "A terminal is open on this host. It stays open — deleting the host does not close it, and " + + "nothing will reopen it afterwards." + : string.Empty); + } + + /// Queues a tombstone for the host that was agreed to. + private async Task DeleteHostNowAsync(Guid entityId, CancellationToken cancellationToken) + { + if (Hosts.FirstOrDefault(row => row.EntityId == entityId) is not { } row) + { + // Gone between the question and the answer — a sync that pulled somebody else's deletion is the + // realistic way. Saying so beats a silent no-op under a card that has just been agreed to. + Status = "That host is no longer here, so nothing was deleted."; + return; + } + await RunAsync( "Deleting…", async () => @@ -1682,15 +1781,39 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } - /// Queues a tombstone for the selected key. + /// Asks whether the selected key should go. + /// + /// The private key is the thing this vault holds that is least likely to exist anywhere else, which is + /// why the question says so. What it does not say is that the key is gone from the machines it was + /// installed on: deleting it here removes this vault's copy, and the authorized_keys file on a + /// server is not something this application has ever written to. + /// [RelayCommand] - private async Task DeleteKeyAsync(CancellationToken cancellationToken) + private void DeleteKey() { if (SelectedKey is not { } row) { return; } + PendingDeletion = new DeletionRequest( + DeletionTarget.Key, + row.EntityId, + $"Delete the SSH key '{row.Label}'?", + HowFarADeletionGoes("The private key, its passphrase and everything saved with them") + + " If this key is not on disk anywhere else, this is the only copy.", + HostsBoundTo(host => host.SshKeyId, row.EntityId)); + } + + /// Queues a tombstone for the key that was agreed to. + private async Task DeleteKeyNowAsync(Guid entityId, CancellationToken cancellationToken) + { + if (Keys.FirstOrDefault(row => row.EntityId == entityId) is not { } row) + { + Status = "That key is no longer here, so nothing was deleted."; + return; + } + await RunAsync( "Deleting…", async () => @@ -1807,15 +1930,32 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } - /// Queues a tombstone for the selected credential. + /// Asks whether the selected credential should go. [RelayCommand] - private async Task DeleteCredentialAsync(CancellationToken cancellationToken) + private void DeleteCredential() { if (SelectedCredential is not { } row) { return; } + PendingDeletion = new DeletionRequest( + DeletionTarget.Credential, + row.EntityId, + $"Delete the password '{row.Label}'?", + HowFarADeletionGoes("The password and the account saved with it"), + HostsBoundTo(host => host.CredentialId, row.EntityId)); + } + + /// Queues a tombstone for the credential that was agreed to. + private async Task DeleteCredentialNowAsync(Guid entityId, CancellationToken cancellationToken) + { + if (Credentials.FirstOrDefault(row => row.EntityId == entityId) is not { } row) + { + Status = "That password is no longer here, so nothing was deleted."; + return; + } + await RunAsync( "Deleting…", async () => @@ -1831,6 +1971,92 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } + /// Carries out the deletion that was asked about. + /// + /// Disarmed before the work rather than after it, so that the card goes the moment it is answered and a + /// second press during a slow round trip has nothing left to agree to. + /// + [RelayCommand] + private async Task ConfirmDeleteAsync(CancellationToken cancellationToken) + { + if (PendingDeletion is not { } request) + { + return; + } + + PendingDeletion = null; + + switch (request.Target) + { + case DeletionTarget.Host: + await DeleteHostNowAsync(request.EntityId, cancellationToken).ConfigureAwait(true); + break; + + case DeletionTarget.Key: + await DeleteKeyNowAsync(request.EntityId, cancellationToken).ConfigureAwait(true); + break; + + case DeletionTarget.Credential: + await DeleteCredentialNowAsync(request.EntityId, cancellationToken).ConfigureAwait(true); + break; + + default: + break; + } + } + + /// Thinks better of it. + [RelayCommand] + private void CancelDelete() => PendingDeletion = null; + + /// + /// Where a deleted item goes, and how far. + /// + /// + /// The offline branch is the same distinction saving makes, and it matters more here: a tombstone that + /// has not been pushed is a deletion the other machines have not heard about, and somebody deleting a + /// credential because it leaked should be told which of those two they have just done. + /// + private string HowFarADeletionGoes(string what) => connection() is null + ? $"{what} goes from this machine now, and from your other machines once this one is online again. " + + "There is no undo." + : $"{what} goes from this machine now, and from your other machines at the next synchronisation. " + + "There is no undo."; + + /// + /// What the hosts that authenticate with an item would be left with, or nothing when none do. + /// + /// + /// Counted rather than warned about in general terms. The number is the difference between a sentence + /// somebody reads and one they click past, and what happens next is worth stating exactly: a host bound + /// to something the vault no longer has is refused at connect time rather than quietly falling back to a + /// typed password — see . + /// + private string HostsBoundTo(Func binding, Guid entityId) + { + var bound = Hosts + .Where(row => binding(row.Host) == entityId) + .Select(row => row.Label) + .ToArray(); + + if (bound.Length == 0) + { + return string.Empty; + } + + // Three names and a count past that, because this is read in a 244-pixel column and a vault with + // twenty hosts on one key would otherwise put a paragraph of names where a warning should be. + var named = bound.Length <= 3 + ? string.Join(", ", bound) + : $"{string.Join(", ", bound.Take(3))} and {bound.Length - 3} more"; + + return bound.Length == 1 + ? $"{named} authenticates with it, and will refuse to connect rather than fall back to a typed " + + "password." + : $"{bound.Length} hosts authenticate with it — {named} — and will refuse to connect rather than " + + "fall back to a typed password."; + } + /// /// Withdraws trust from the selected pin's endpoint. /// @@ -2564,6 +2790,33 @@ internal sealed partial class VaultViewModel( { OnPropertyChanged(nameof(SelectedHostAsksForAPassword)); OnPropertyChanged(nameof(SelectedHostAuthenticationNote)); + + DisarmIfAimedElsewhere(DeletionTarget.Host, value?.EntityId); + } + + partial void OnPendingDeletionChanged(DeletionRequest? value) + { + OnPropertyChanged(nameof(IsConfirmingDeletion)); + OnPropertyChanged(nameof(ShowsHostActions)); + OnPropertyChanged(nameof(ShowsItemActions)); + } + + /// + /// Takes the question away when the selection it was asked about has moved on. + /// + /// + /// Compared by entity id rather than by row, and that is the whole point of the method. A reload + /// replaces every row object in the list, so a background pass a minute after the question would + /// otherwise take the card away from under somebody still reading it — while a click onto a different + /// item, which is the case that actually needs handling, leaves an armed deletion pointing at something + /// nobody is looking at any more. + /// + private void DisarmIfAimedElsewhere(DeletionTarget target, Guid? entityId) + { + if (PendingDeletion is { } request && request.Target == target && request.EntityId != entityId) + { + PendingDeletion = null; + } } /// @@ -2651,6 +2904,11 @@ internal sealed partial class VaultViewModel( OnPropertyChanged(nameof(SelectedItemIsEditable)); OnPropertyChanged(nameof(SelectedItemIsPin)); OnPropertyChanged(nameof(SelectedDetailHeading)); + OnPropertyChanged(nameof(ShowsItemActions)); + + // Both kinds this table can delete, because one selection covers both lists. + DisarmIfAimedElsewhere(DeletionTarget.Key, value?.EntityId); + DisarmIfAimedElsewhere(DeletionTarget.Credential, value?.EntityId); switch (value?.Kind) { @@ -2701,8 +2959,34 @@ internal sealed partial class VaultViewModel( /// flips back in every path that closes one, so this notification always observes the pair in a /// consistent state. /// - partial void OnIsEditingChanged(bool value) => + partial void OnIsEditingChanged(bool value) + { OnPropertyChanged(nameof(CanForgetHostKey)); + OnPropertyChanged(nameof(ShowsHostActions)); + + DisarmOnceAnEditorIsOpen(value); + } + + partial void OnIsEditingKeyChanged(bool value) => DisarmOnceAnEditorIsOpen(value); + + partial void OnIsEditingCredentialChanged(bool value) => DisarmOnceAnEditorIsOpen(value); + + /// + /// Takes the question away when an editor opens over the pane it was asked in. + /// + /// + /// The sidebar's confirmation replaces the buttons that could open the host editor, so that half cannot + /// happen; the vault screen's Add buttons stay on screen beside the detail pane, so that half can. One + /// rule for both, rather than a guard on the three commands that would have to be remembered by the + /// fourth. + /// + private void DisarmOnceAnEditorIsOpen(bool opened) + { + if (opened) + { + PendingDeletion = null; + } + } partial void OnPendingHostKeyChanged(HostKeyPresentation? value) => OnPropertyChanged(nameof(HasPendingHostKey)); diff --git a/src/DodoSSH.Client.App/Views/ConfirmDeleteCard.axaml b/src/DodoSSH.Client.App/Views/ConfirmDeleteCard.axaml new file mode 100644 index 0000000..8b40903 --- /dev/null +++ b/src/DodoSSH.Client.App/Views/ConfirmDeleteCard.axaml @@ -0,0 +1,52 @@ + + + + + + + + + + + + + + + + +