From 808a9a7fc19ba5c7cf133c6c0641ca08fc509ada Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Thu, 6 Aug 2026 12:08:15 +0200 Subject: [PATCH] Open a new host in the vault of the group it is being made in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit + NEW HOST decided two defaults separately and let them contradict each other. The group came from the screen — the selected card, or failing that the group whose contents are showing — and the vault came from the keychain screen's standing "new items go to" preference. Inside a group belonging to any other vault the two disagreed, and the group is what lost: GroupInEditingVault drops a group the editor's vault has not got, on the sound reasoning that a host filed under an id its readers cannot resolve looks unfiled to everybody but the person who wrote it. So pressing the button while standing inside a team's PLATFORM opened a form filed under nothing, bound for the personal vault, with no sentence anywhere saying either thing had happened. The vault now follows the group. A group lives in exactly one vault, so a host that is to land in that group has to be sealed in that vault too — which is the rule + NEW GROUP has followed for a parent since the cards became a tree, and the comment there claiming this as a deliberate difference from the host's editor is the one the code has now caught up with. The filter stays, because there is one case left for it: the group's vault may be one this session can read and not write, a team vault this account is a viewer of. TargetVaults is the readable-and-writable set and is what decides here, so a viewer keeps the standing preference and loses the group with it, rather than opening an editor aimed at a save that cannot happen. Both directions are tested, since one alone would not say which default wins: standing in a shared vault's group, the editor opens on that vault with the group selected and the host saves there; and with the preference pointed at the shared vault while a personal-vault group is open, the group beats the picker somebody set once. --- .../ViewModels/VaultViewModel.cs | 53 +++++++++---- .../VaultSharingTests.cs | 74 +++++++++++++++++++ 2 files changed, 113 insertions(+), 14 deletions(-) diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index 4bdedbc..741e39d 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -6174,10 +6174,10 @@ internal sealed partial class VaultViewModel( editingEntityId = null; - // The keychain screen's picker is the default rather than the answer: the editor has a picker of its - // own from here on, and moving that one is what decides where this host lands. See - // EditorVaultChoices. - editingHostVaultId = TargetVaultId; + // Read before the vault is decided, because it is what decides it. + var inherited = GroupTarget; + + editingHostVaultId = VaultForANewHostUnder(inherited); EditorLabel = string.Empty; EditorHostname = string.Empty; @@ -6199,11 +6199,11 @@ internal sealed partial class VaultViewModel( // that the group whose contents are showing. Adding three machines to the group somebody has just // made is the ordinary case, and since the grid holds one level at a time the alternative is worse // than a default nobody chose: a host created inside a group and filed under none would vanish from - // the screen it was created on. Only when that group is in the vault this host is going into, - // though — the grid draws the active vault's groups, and inheriting one into a shared vault would - // file the host under something nobody else in it can resolve. Before the authentication picker, - // because whether there is a group to inherit from decides whether that one offers to. - BuildGroupChoices(GroupInEditingVault(GroupTarget?.EntityId)); + // the screen it was created on. Still filtered by the vault being written to, which is that group's + // own wherever this session can write there: what the filter is left holding is the case where it + // cannot. Before the authentication picker, because whether there is a group to inherit from decides + // whether that one offers to. + BuildGroupChoices(GroupInEditingVault(inherited?.EntityId)); BuildAuthenticationChoices( boundKeyId: null, @@ -6214,6 +6214,32 @@ internal sealed partial class VaultViewModel( Status = "Adding a host."; } + /// Which vault a host started from the grid is sealed in. + /// The group the editor is about to open on, if there is one. + /// + /// + /// The keychain screen's picker is the default rather than the answer: the editor has a picker of its own + /// from here on, and moving that one is what decides where the host lands. See + /// . + /// + /// + /// A group to inherit beats it, because a group lives in exactly one vault and a host that is to land in + /// that group has to be sealed in that vault too. Deciding the two separately made them contradict each + /// other: the group was dropped by and + NEW HOST inside PLATFORM + /// opened a form filed under nothing, bound for somewhere else. It is the rule + /// already follows for a parent. + /// + /// + /// Only where this session can write to that vault, which is what is asked + /// here: a viewer of a shared vault keeps the standing preference and loses the group with it, rather + /// than opening an editor aimed at a save that cannot happen. + /// + /// + private Guid VaultForANewHostUnder(HostGroupRowViewModel? inherited) => + inherited is { } group && TargetVaults.Any(choice => choice.VaultId == group.VaultId) + ? group.VaultId + : TargetVaultId; + /// Opens the selected host for editing. [RelayCommand] private void EditSelectedHost() @@ -6623,11 +6649,10 @@ internal sealed partial class VaultViewModel( /// while the open group is the screen everybody can see they are on. /// /// - /// And in the open group's vault, which is where this differs from a second - /// time. A host opens on the standing "new items go to" preference and takes the open group only if - /// that group happens to be in the same vault; a group made inside another group is in that group's - /// vault by construction, because a parent in a second vault is a level half the readers cannot resolve. - /// Defaulting to the preference instead would answer "+ NEW GROUP inside PLATFORM" with a group + /// And in the open group's vault, which now does too and for the same + /// reason: a parent — or a group — in a second vault is a level half the readers cannot resolve, so the + /// thing being made goes where the thing it is going inside already is. Defaulting to the standing + /// "new items go to" preference instead would answer "+ NEW GROUP inside PLATFORM" with a group /// somewhere else and no parent — a form that silently dropped the one thing the button said. /// /// diff --git a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs index aefdf73..4ef94d0 100644 --- a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs @@ -1619,6 +1619,80 @@ public sealed class VaultSharingTests : IAsyncLifetime .ShouldBeNull("the host is left where it was rather than filed under an unresolvable group"); } + /// + /// + /// The same rule as the drag above, arrived at from the other end: a group lives in exactly one vault, so + /// + NEW HOST inside one has to open on that vault as well as on that group. The two defaults were + /// decided separately — the group came from the screen, the vault from the standing "new items go to" + /// preference — so pressing the button inside a shared vault's group opened a form bound for the personal + /// vault, with the group silently dropped by the picker that keeps the two in step. + /// + /// + /// The second half is the same test read backwards, and it is what says which of the two wins. The + /// preference is an answer for a host being made from nowhere in particular; the group somebody is + /// standing inside is a better one, and it is the one the button was pressed in. + /// + /// + [Fact] + public async Task ANewHostInsideAGroup_IsMadeInThatGroupsVaultRatherThanTheStandingPreference() + { + await UnlockedAsync(); + + var vaults = shell.Vaults; + + await CreateVaultAsync(vaults, "Platform secrets"); + + var vault = shell.Vault!; + var sharedVaultId = vaults.SelectedVault!.VaultId; + + await vault.LoadAsync(Token); + + // In the personal vault, which is where a new group goes with nothing open and the preference + // untouched. It is the one the second half of this test stands in. + await AddGroupAsync(vault, "staging", under: null); + + var personalVaultId = Named(vault, "staging").VaultId; + + personalVaultId.ShouldNotBe(sharedVaultId, "this test is meaningless with one vault"); + + vault.NewGroupCommand.Execute(null); + + vault.GroupEditorSelectedVault = + vault.GroupEditorVaultChoices.Single(choice => choice.VaultId == sharedVaultId); + + vault.GroupEditorLabel = "production"; + await vault.SaveGroupCommand.ExecuteAsync(null); + + vault.OpenGroupCommand.Execute(Named(vault, "production")); + + vault.NewHostCommand.Execute(null); + + vault.EditorSelectedVault.ShouldNotBeNull(vault.Status).VaultId + .ShouldBe(sharedVaultId, "the group the screen is standing in is in the shared vault"); + vault.EditorSelectedGroup.ShouldNotBeNull().Label + .ShouldBe("production", "and the group survives, having a vault it can be resolved in"); + + vault.EditorLabel = "prod-db"; + vault.EditorHostname = "db.internal"; + await vault.SaveHostCommand.ExecuteAsync(null); + + var host = vault.Hosts.ShouldHaveSingleItem(); + + host.VaultId.ShouldBe(sharedVaultId, vault.Status); + host.Host.GroupId.ShouldBe(Named(vault, "production").EntityId); + + // And backwards: the preference names the shared vault and the open group is in the personal one. + vault.SelectedTargetVault = vault.TargetVaults.Single(choice => choice.VaultId == sharedVaultId); + + vault.OpenGroupCommand.Execute(Named(vault, "staging")); + + vault.NewHostCommand.Execute(null); + + vault.EditorSelectedVault.ShouldNotBeNull(vault.Status).VaultId + .ShouldBe(personalVaultId, "the group somebody is standing in beats the picker they set once"); + vault.EditorSelectedGroup.ShouldNotBeNull().Label.ShouldBe("staging"); + } + /// /// The mirror image of the host test above, and it goes the other way on purpose. A host filed into a /// shared vault has to stay there, because hosts are read across every readable vault and so come back;