diff --git a/docs/adding-hosts-on-the-phone.md b/docs/adding-hosts-on-the-phone.md index 8c95db9..802856c 100644 --- a/docs/adding-hosts-on-the-phone.md +++ b/docs/adding-hosts-on-the-phone.md @@ -325,9 +325,14 @@ because-string used as prose. `ConflictStore.Record` throws `ArgumentOutOfRangeException` on a conflict for any of those three. Pre-existing, unrelated to any of this, and `Tag` is already in that switch. Worth a separate fix. -> **Still open.** All three are on `SyncEntityType` and on `CryptoSpec.AadResourceType`, and all three are -> still absent from `LocalCacheProtector.For` — so this outlived the phases that shipped the logs and the -> buckets, which is exactly the drift a note like this is meant to prevent. +> **Fixed 2026-08-04**, having outlived the phases that shipped the logs and the buckets — which is exactly +> the drift a note like this is meant to prevent, so it is worth saying what let it last. Of the three callers +> of `AadResourceTypes.For`, two reach it only when an item carries plaintext fields and none of these three +> does; the third, `ConflictStore.RecordAsync`, calls it unconditionally but is reached only by a real merge +> conflict, which every existing test raised against a `Host`. The arms are in, and two tests now hold them +> there: `CacheStoreTests.AConflict_CanBeRecordedForEveryKindOfItem` records one per kind, and +> `AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping` fails on the *next* item type added +> without one, by name rather than by a list kept by hand. ## Prose that becomes false diff --git a/src/DodoSSH.Client.Storage/LocalCacheProtector.cs b/src/DodoSSH.Client.Storage/LocalCacheProtector.cs index d300c71..d9352a0 100644 --- a/src/DodoSSH.Client.Storage/LocalCacheProtector.cs +++ b/src/DodoSSH.Client.Storage/LocalCacheProtector.cs @@ -98,10 +98,27 @@ public sealed class LocalCacheProtector : IDisposable /// Maps a syncable entity type onto the resource type its AAD binds. /// /// +/// /// A switch rather than a cast, even though the two enums happen to be adjacent. They are not the /// same list: also covers users, devices and vaults, so the /// numbers do not line up, and a cast would bind an item's ciphertext to the wrong resource type /// without failing anywhere a test would notice. +/// +/// +/// It has to name every syncable type, and the throw is not a safety net. +/// ConflictStore.RecordAsync calls this unconditionally, so a type with no arm here is a type +/// whose conflicts cannot be recorded — and the conflict log is the only reason the merge is +/// allowed to pick a winner at all. Three types went two phases without one: the two logs and the +/// buckets were added to both enums and to the reconciler registry, and this switch was not touched, +/// so a merge over any of them turned a recorded conflict into an +/// . The two other callers hid it — +/// ItemStore and OutboxStore only reach this when an item carries plaintext fields, and +/// none of those three does. +/// +/// +/// AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping is what says so now, and it +/// is a name comparison rather than a list to keep by hand. +/// /// internal static class AadResourceTypes { @@ -117,6 +134,14 @@ internal static class AadResourceTypes SyncEntityType.Snippet => CryptoSpec.AadResourceType.Snippet, SyncEntityType.PortForward => CryptoSpec.AadResourceType.PortForward, SyncEntityType.KnownHostKey => CryptoSpec.AadResourceType.KnownHostKey, + + // Appended in the order the enums grew, which is why these three are not beside their + // neighbours by number. See docs/crypto.md §4.3 on why 14–16 are not one-behind their wire + // counterparts the way the arms above are. + SyncEntityType.ConnectionLogEntry => CryptoSpec.AadResourceType.ConnectionLogEntry, + SyncEntityType.ActivityLogEntry => CryptoSpec.AadResourceType.ActivityLogEntry, + SyncEntityType.ObjectStore => CryptoSpec.AadResourceType.ObjectStore, + _ => throw new ArgumentOutOfRangeException( nameof(entityType), entityType, "No AAD resource type is defined for this entity type."), }; diff --git a/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs b/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs index 90353a2..2360613 100644 --- a/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs +++ b/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs @@ -394,6 +394,49 @@ public sealed class CacheStoreTests : IAsyncLifetime all.Detail.ShouldBe(new byte[] { 1, 2, 3 }); } + /// + /// A conflict can be recorded against any kind of item, not only the kinds that were here first. + /// + /// + /// + /// Every other test in this section uses , and that is how three item + /// kinds shipped with no way to record a conflict at all: the two logs and the buckets were added to both + /// enums, to the reconciler registry and to the cipher pinning, while AadResourceTypes.For — which + /// ConflictStore.RecordAsync calls unconditionally — kept throwing for them. The two other callers + /// of that mapping only reach it when an item carries plaintext fields, which none of the three does, so + /// nothing else so much as touched the gap. + /// + /// + /// A theory over the types rather than one more Host case, because the failure was never about + /// conflicts and always about which types the layer below had been taught. Recording is asserted through + /// a read-back rather than by "it did not throw": an arm returning the wrong resource type would seal + /// under one AAD and open under another, which is a null detail rather than an exception. + /// + /// + [Theory] + [InlineData(SyncEntityType.Host)] + [InlineData(SyncEntityType.HostGroup)] + [InlineData(SyncEntityType.Snippet)] + [InlineData(SyncEntityType.ConnectionLogEntry)] + [InlineData(SyncEntityType.ActivityLogEntry)] + [InlineData(SyncEntityType.ObjectStore)] + public async Task AConflict_CanBeRecordedForEveryKindOfItem(SyncEntityType entityType) + { + var detail = System.Text.Encoding.UTF8.GetBytes($$"""{"kind":"{{entityType}}"}"""); + + var id = await harness.Conflicts.RecordAsync( + VaultId, entityType, Guid.CreateVersion7(), ConflictKind.FieldOverridden, detail, Token); + + var listed = (await harness.Conflicts.ListAsync(VaultId, false, Token)).ShouldHaveSingleItem(); + + listed.Id.ShouldBe(id); + listed.EntityType.ShouldBe(entityType); + listed.Detail.ShouldBe( + detail, + "an empty detail here means the record was sealed under one resource type and opened under " + + "another, which ListAsync reports as nothing rather than as a failure"); + } + [Fact] public async Task AnUnacknowledgedConflict_CannotBeDiscarded() { diff --git a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs index 28c86de..a51df87 100644 --- a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs @@ -1,5 +1,6 @@ using System.Security.Cryptography; using DodoSSH.Client.Domain; +using DodoSSH.Client.Storage; using DodoSSH.Client.Sync; using DodoSSH.Contracts; using DodoSSH.Crypto; @@ -151,6 +152,63 @@ public sealed class AadResourceTypeTests .ShouldBe(ItemKinds.SyncedTypes, ignoreOrder: true); } + /// + /// The same pairing, made a second time in the storage layer, and every type must be in it. + /// + /// + /// + /// AadResourceTypes.For is the cache's copy of the table above: the ciphers seal an item for the + /// server, and this seals the two things the local cache holds in the clear — a relay host's + /// address, and the values a merge overrode. A type missing from it throws rather than mis-seals, which + /// sounds like the safe failure and is not: ConflictStore.RecordAsync calls it unconditionally, so + /// the exception lands on the path that records what a merge discarded. + /// + /// + /// This is written after finding three types missing from it — ConnectionLogEntry, + /// ActivityLogEntry and ObjectStore went two shipping phases without an arm, because the + /// only unconditional caller is one a test suite reaches solely by causing a real merge conflict. Asserted + /// over the whole wire enum rather than over ItemKinds.SyncedTypes, which is the stronger claim and + /// the one the switch actually makes: the two reserved association types have arms too. + /// + /// + [Fact] + public void EverySyncableType_HasAnArmInTheStorageMapping() + { + var missing = new List(); + + foreach (var wire in Enum.GetValues()) + { + if (wire == SyncEntityType.Unspecified) + { + continue; + } + + CryptoSpec.AadResourceType resource; + + try + { + resource = AadResourceTypes.For(wire); + } + catch (ArgumentOutOfRangeException) + { + missing.Add(wire); + continue; + } + + // Same name, as the cipher table demands — a wrong-but-present arm is the failure this half + // would otherwise wave through. + Enum.GetName(resource).ShouldBe( + Enum.GetName(wire), + $"AadResourceTypes.For({wire}) returns {resource}, which binds this type's cache records " + + "to another type's resource."); + } + + missing.ShouldBeEmpty( + "every syncable type needs an arm in AadResourceTypes.For, or a conflict recorded against one " + + "of these throws instead of being written — and the conflict log is what justifies the merge " + + "picking a winner."); + } + private static EncryptedPayload SealSample( SyncEntityType wire, byte[] vaultKey,