diff --git a/CHANGELOG.md b/CHANGELOG.md index 25942cb..6c68b38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +## 0.5.0 + +- **Ownership transfers are retry-safe.** `transferOwnershipWithCharter` had + a publish-retry trap: a repeat call with the already-updated group minted a + self-link or a link not signed by the previous owner — permanently invalid + under `validateCharter`, which under a strict policy stopped every member's + manifest from decrypting. Both transfer variants are now idempotent no-ops + when the target already owns the group, and the charter builder validates + its own extended chain before returning (a retryable `StateError` instead + of a silently poisoned link). No wire/crypto change — the guards only + constrain what the builder emits. + ## 0.4.0 Adopter-portability release (the seams a consuming app needs to swap its own diff --git a/README.md b/README.md index a60e05f..6fcac98 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,7 @@ flutter pub get flutter test ``` -Expect `All tests passed!` — 72 tests across three files: +Expect `All tests passed!` — 75 tests across three files: - **`group_model_test.dart`** (17) — `Group`/`GroupMember` JSON round-trips, the manifest-vs-local-storage field split, the unknown-field passthrough diff --git a/lib/src/group_service.dart b/lib/src/group_service.dart index 634f928..e906445 100644 --- a/lib/src/group_service.dart +++ b/lib/src/group_service.dart @@ -168,6 +168,11 @@ class GroupService { /// previously an `assert`, which is stripped in release builds and so let /// production set `ownerUid` to an arbitrary string. static Group transferOwnership(Group group, String newOwnerUid) { + // Same retry idempotence as the charter variant: a repeat with the + // already-updated group is a no-op, keeping both transfer paths + // uniformly safe in publish-retry loops. (The failure mode here is much + // milder — no chain to poison — but uniformity is free.) + if (newOwnerUid == group.ownerUid) return group; if (group.memberByUid(newOwnerUid) == null) { throw ArgumentError.value( newOwnerUid, 'newOwnerUid', 'New owner must be a member of the group'); @@ -198,6 +203,15 @@ class GroupService { required String signingKeyDomain, bool allowUnchartedFallback = false, }) { + // Idempotent no-op when the target already owns the group. This is the + // RETRY trap: transfer A→B succeeds locally but the manifest publish + // fails; the retry calls this again with the already-updated group and + // would mint either a B→B self-link or a link not signed by the (now + // previous) owner — both permanently invalid under [validateCharter], + // which under a strict policy stops every member's manifest decrypting. + // A no-op makes publish-retry loops safe by construction. + if (newOwnerUid == group.ownerUid) return group; + final newMember = group.memberByUid(newOwnerUid); final charter = group.charter; final newEd = newMember?.edPubKeyB64; @@ -223,8 +237,20 @@ class GroupService { ts: nextCharterTimestamp( prevEntry, DateTime.now().millisecondsSinceEpoch), ); - return group - .copyWith(ownerUid: newOwnerUid, charter: [...charter, link]); + final extended = [...charter, link]; + // Never hand back a chain that can't validate: a poisoned charter is + // silent at mint time and permanent once published (e.g. the caller + // passed a group whose tip the [currentOwner] key no longer signs + // for). A throw here is retryable; a published bad link is not. + final check = validateCharter(sodium, extended, group.groupId); + if (!check.valid) { + throw StateError( + 'Refusing to mint an invalid charter link (${check.reason}): ' + 'the current owner/tip and the signer no longer agree — ' + 'reload the group state and retry.', + ); + } + return group.copyWith(ownerUid: newOwnerUid, charter: extended); } finally { signing.secretKey.dispose(); } diff --git a/pubspec.yaml b/pubspec.yaml index a8021ae..ec29033 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,6 +1,6 @@ name: groups description: "End-to-end-encrypted group membership, key rotation, manifests, and a signed ownership charter." -version: 0.4.0 +version: 0.5.0 publish_to: 'none' environment: diff --git a/test/group_service_test.dart b/test/group_service_test.dart index 1b8eb20..1b6146f 100644 --- a/test/group_service_test.dart +++ b/test/group_service_test.dart @@ -544,6 +544,99 @@ void main() { expect(r.height, 2); }); + test('a RETRY with the already-updated group is an idempotent no-op', () { + // The publish-retry trap: transfer A→B succeeds locally, the manifest + // publish fails, and the retry calls transfer again with the ALREADY + // updated group (owner=B, tip=B). Unguarded, that minted a B→B + // self-link or a link not signed by the previous owner — permanently + // invalid, which under a strict policy stops every member's manifest + // from decrypting. The retry must change nothing. + final owner = generateIdentity(sodium); + final next = generateIdentity(sodium); + var g = GroupService.createGroup( + sodium: sodium, + name: 'G', + identity: owner, + signingKeyDomain: signingDomain); + g = GroupService.addMember( + g, memberFor(next, edPubKeyB64: edKeyOf(next))); + final g2 = GroupService.transferOwnershipWithCharter( + sodium: sodium, + group: g, + currentOwner: owner, + newOwnerUid: next.uid, + signingKeyDomain: signingDomain, + ); + + // The retry, exactly as an app's publish-retry loop would issue it. + final retried = GroupService.transferOwnershipWithCharter( + sodium: sodium, + group: g2, + currentOwner: owner, // the retry still signs as the OLD owner + newOwnerUid: next.uid, + signingKeyDomain: signingDomain, + ); + expect(identical(retried, g2), isTrue, reason: 'no-op, not a new link'); + expect(retried.charter!.length, 2); + final r = validateCharter(sodium, retried.charter!, retried.groupId); + expect(r.valid, isTrue, reason: r.reason); + }); + + test('refuses to mint a link the validator would reject', () { + // A caller passes a group whose tip the signing identity no longer + // matches (stale local state after someone else's transfer): the + // built link would be signature-invalid. Minting it is silent and, + // once published, permanent — so the builder validates its own output + // and throws (retryable) instead. + final owner = generateIdentity(sodium); + final mid = generateIdentity(sodium); + final next = generateIdentity(sodium); + var g = GroupService.createGroup( + sodium: sodium, + name: 'G', + identity: owner, + signingKeyDomain: signingDomain); + g = GroupService.addMember(g, memberFor(mid, edPubKeyB64: edKeyOf(mid))); + g = GroupService.addMember( + g, memberFor(next, edPubKeyB64: edKeyOf(next))); + // Real transfer owner→mid: the tip now requires MID's signature. + final g2 = GroupService.transferOwnershipWithCharter( + sodium: sodium, + group: g, + currentOwner: owner, + newOwnerUid: mid.uid, + signingKeyDomain: signingDomain, + ); + // A further transfer signed by the ORIGINAL owner (stale state) must + // refuse rather than hand back a poisoned chain. + expect( + () => GroupService.transferOwnershipWithCharter( + sodium: sodium, + group: g2, + currentOwner: owner, // no longer the tip's signer + newOwnerUid: next.uid, + signingKeyDomain: signingDomain, + ), + throwsA(isA()), + ); + }); + + test('plain transferOwnership is retry-idempotent too', () { + final owner = generateIdentity(sodium); + final next = generateIdentity(sodium); + var g = GroupService.createGroup( + sodium: sodium, + name: 'G', + identity: owner, + signingKeyDomain: signingDomain); + g = GroupService.addMember(g, memberFor(next)); + final g2 = GroupService.transferOwnership(g, next.uid); + // The retry: already the owner → the same group back, no throw even + // if roster state changed underneath (uniform with the charter path). + expect(identical(GroupService.transferOwnership(g2, next.uid), g2), + isTrue); + }); + test('refuses to silently downgrade when the ed key is unknown', () { final owner = generateIdentity(sodium); final next = generateIdentity(sodium);