From b10c79f9ed297bb34faee9ef02674cb563ba3c6e Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 8 Sep 2026 23:50:05 +0200 Subject: [PATCH] feat(subgroups): the refusals that are about capability, not preference Phase 8 of docs/subgroups.md. Four of the seven refusals landed with the code they guard; these are the three that did not, and two of them are about what this device *can* do rather than what the user should pick. **No share of the parent's key.** `FrostSigningManager.proposeSigningBatch` throws outright for a device with no share, so without this the button crashes rather than declines. It is checked first because it is the one refusal a user cannot fix by picking differently -- every other message says "pick differently" and this one cannot. **Not an admin of the parent.** A non-admin proposing the group's signature is a proposal the admins have to decline by hand, which is worse than not offering it. Read off `Participant.adminAt`, which is the epoch-0 admin list MIP-01 carries and the same reading every side of the group makes. Both are checked in `refuseCeremonyRoom` as well as at the button that hides itself on `canAddSubgroup`, because a screen not drawing something is not a guard -- and a resumed flow reaches the manager without passing that screen at all. **A child already certified.** One certificate per child: a second is a `d`-tag replacement of the first rather than a second subgroup, and spending a quorum's attention to restate something they have already signed is worse than doing nothing. This is a `check` at propose time rather than a picker refusal, since the child's id does not exist until the ceremony finishes. It deliberately does not try to stop the race. Two coordinators can each propose a certificate for the same child, neither able to see the other's session before it completes; `certificateFor` folds those together because both say the same true thing. This only stops the case somebody can actually see. The blank-name `require` gets a test of its own for the same reason the name exists at all: without it the parent's admins would be approving a hash. Six tests in `SubgroupManagerJvmTest`, and the fixture had to grow to carry them. `parentWith` now gives the parent a completed ceremony and marks the coordinator an admin, with both switchable, because a fixture missing either tests only the first refusal -- which is how the two new checks were found to short-circuit the existing cases the moment they were added. The plan called for a pure `SubgroupGuardsTest` in commonTest. It is not one: every refusal here reads the database -- a key-holding session, an admin flag, a ceremony in the derived room -- so the tests live in jvmTest beside the manager's others rather than being reshaped into something pure that would test less. 397 common tests, 705 jvm tests, `m3Audit` meets every budget. Co-Authored-By: Claude Opus 5 --- .../compose/managers/SubgroupManager.kt | 32 ++++ .../managers/SubgroupManagerJvmTest.kt | 137 +++++++++++++++++- 2 files changed, 165 insertions(+), 4 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt index 2d68d577..68ee831b 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt @@ -101,6 +101,28 @@ object SubgroupManager { coordinatorPublicKey: HexKey ): String? { val admins = adminPublicKeys + coordinatorPublicKey + val parentChatRoomId = parentRoom.chatRoom.id + + // A subgroup is certified by the parent's key, so a device holding no + // share of it cannot open the session at all -- + // `FrostSigningManager.proposeSigningBatch` throws outright, and throwing + // at a button is not a UI. Checked before the size rules because it is the + // one refusal the user cannot fix by picking differently. + if (!FrostSigningManager.canSign(database, parentChatRoomId)) { + return "Only a member who holds a share of this group's key can make a subgroup." + } + + // A non-admin proposing the group's signature is a proposal the admins + // have to decline by hand, which is a worse outcome than not offering it. + // Read off the epoch-0 admin list MIP-01 carries, the same reading every + // side of the group makes. + val isAdmin = parentRoom.localParticipants.any { + it.participant.participantPublicKey == coordinatorPublicKey && + it.participant.adminAt != null + } + if (!isAdmin) { + return "Only an admin of this group can make a subgroup." + } if (admins.size < MINIMUM_ADMINS) { return "A subgroup needs at least $MINIMUM_ADMINS admins. Pick at least " + @@ -168,6 +190,16 @@ object SubgroupManager { require(name.isNotBlank()) { "A subgroup has to be called something" } + // One certificate per child. A second is a `d`-tag replacement of the + // first rather than a second subgroup, and spending a quorum's attention + // to restate something they have already signed is worse than doing + // nothing. Two coordinators racing still produce two -- neither can see + // the other's session before it completes -- and `certificateFor` folds + // those together; this only stops the case somebody can actually see. + check(certificateFor(database, subgroupChatRoomId, parentChatRoomId) == null) { + "$parentChatRoomId has already certified $subgroupChatRoomId" + } + logger.i("Asking $parentChatRoomId to certify $subgroupChatRoomId as its subgroup") return FrostSigningManager.proposeSigning( diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt index 332e5a26..961a55ed 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.runBlocking import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -27,6 +28,7 @@ import press.mantra.compose.database.MantraDatabase import press.mantra.compose.database.builder.getRoomDatabase import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.DkgSession +import press.mantra.compose.database.model.types.DkgRitualStage import press.mantra.compose.database.model.GroupSignedEvent import press.mantra.compose.database.model.NostrEvent import press.mantra.compose.database.model.Profile @@ -251,6 +253,24 @@ class SubgroupManagerJvmTest { // ---- the refusals ------------------------------------------------------ + @Test + fun `a member holding no share of the parent's key is refused`(): Unit = runBlocking { + // Not a permission rule: `proposeSigningBatch` throws outright for a + // device with no share, so without this the button would crash rather + // than decline. It is also the one refusal the user cannot fix by picking + // differently, which is why it is checked first. + val parent = parentWith(listOf(alice, bob, carol), canSign = false) + + assertNotNull(refusal(parent, setOf(alice, bob))) + } + + @Test + fun `a member who is not an admin of the parent is refused`(): Unit = runBlocking { + val parent = parentWith(listOf(alice, bob, carol), isAdmin = false) + + assertNotNull(refusal(parent, setOf(alice, bob))) + } + @Test fun `a subgroup of fewer than three admins is refused`(): Unit = runBlocking { val parent = parentWith(listOf(alice, bob, carol)) @@ -288,8 +308,8 @@ class SubgroupManagerJvmTest { // of the members. A second subgroup with the same admins would come back // with the first one's key and therefore the first one's room id. // - // Stood up the way `openCeremony` does, room first: the ceremony room is - // a real NIP-17 room and `DkgSession.chatRoomId` is a foreign key onto it. + // Room first, the way the picker stands one up: the ceremony room is a + // real NIP-17 room and `DkgSession.chatRoomId` is a foreign key onto it. val ceremonyRoom = assertNotNull( db.nostrNip17Dao().createNip17ChatRoom( userPublicKey = user, @@ -336,8 +356,70 @@ class SubgroupManagerJvmTest { ) } + @Test + fun `certifying a child the parent has already certified is refused`(): Unit = runBlocking { + seedRoom(parentRoomId) + record(certificate()) + + val parentRoom = assertNotNull(db.chatRoomDao().findChatRoomById(parentRoomId)) + + // One certificate per child: a second is a `d`-tag replacement rather + // than a second subgroup, and spending a quorum's attention to restate + // something they have already signed is worse than doing nothing. + // + // Two coordinators racing still produce two -- neither can see the + // other's session before it completes -- which is why `certificateFor` + // folds them. This only stops the case somebody can actually see. + assertFailsWith { + SubgroupManager.proposeBirthCertificate( + database = db, + parentRoom = parentRoom, + userPublicKey = user, + key = completedChildCeremony(), + adminPublicKeys = listOf(user, alice, bob), + name = "Translation team", + ) + } + } + + @Test + fun `an unnamed subgroup cannot be certified`(): Unit = runBlocking { + seedRoom(parentRoomId) + val parentRoom = assertNotNull(db.chatRoomDao().findChatRoomById(parentRoomId)) + + // The parent's admins would be approving a hash. The name is what makes + // the proposal legible, so it is required rather than defaulted. + assertFailsWith { + SubgroupManager.proposeBirthCertificate( + database = db, + parentRoom = parentRoom, + userPublicKey = user, + key = completedChildCeremony(), + adminPublicKeys = listOf(user, alice, bob), + name = " ", + ) + } + } + // ---- fixtures ---------------------------------------------------------- + /** A finished ceremony for the child, as far as these guards can tell. */ + private fun completedChildCeremony() = DkgSession( + id = "child-ceremony", + chatRoomId = parentRoomId, + coordinatorPublicKey = user, + userPublicKey = user, + threshold = 2, + participantCount = 3, + stage = DkgRitualStage.COMPLETE, + hostPublicKey = user, + round1Random = "aa".repeat(32), + round2AuxRandom = "bb".repeat(32), + thresholdPublicKey = childMaterial.thresholdPublicKey.value.toHex(), + secretShare = "dd".repeat(32), + ) + + private fun keyMaterial(secret: String): KeyMaterial = Frost.trustedDealerKeygen( thresholdSecretKey = PrivateKey(ByteVector32(secret)), nParticipants = participants, @@ -355,16 +437,63 @@ class SubgroupManagerJvmTest { coordinatorPublicKey = user ) - private suspend fun parentWith(members: List): LocalChatRoom { + /** + * A parent this device could actually make a subgroup of: an admin of it, and + * holding a share of its key. + * + * Both are conditions of `refuseCeremonyRoom` before it looks at the picking + * at all, so a fixture without them tests only the first refusal. [canSign] + * and [isAdmin] are here so the tests that are *about* those two can turn + * each off on its own. + */ + private suspend fun parentWith( + members: List, + canSign: Boolean = true, + isAdmin: Boolean = true, + ): LocalChatRoom { (members + user).forEach { seedProfile(it) } - return assertNotNull( + val room = assertNotNull( db.nostrNip17Dao().createNip17ChatRoom( userPublicKey = user, participantPublicKeys = members, subject = "Ekklesia", ) ) + + if (canSign) { + // `completedKey` falls back to the newest completed ceremony held in + // the room, which is what a NIP-17 room's key looks like from here. + db.dkgSessionDao().upsert( + DkgSession( + id = "parent-ceremony", + chatRoomId = room.chatRoom.id, + coordinatorPublicKey = user, + userPublicKey = user, + threshold = 2, + participantCount = members.size + 1, + stage = DkgRitualStage.COMPLETE, + hostPublicKey = user, + round1Random = "aa".repeat(32), + round2AuxRandom = "bb".repeat(32), + thresholdPublicKey = parentMaterial.thresholdPublicKey.value.toHex(), + secretShare = "cc".repeat(32), + ) + ) + } + + if (isAdmin) { + db.participantDao().upsert( + assertNotNull( + db.chatRoomDao().findChatRoomById(room.chatRoom.id) + ).localParticipants + .map { it.participant } + .filter { it.participantPublicKey == user } + .map { it.copy(adminAt = Instant.fromEpochSeconds(1_700_000_000)) } + ) + } + + return assertNotNull(db.chatRoomDao().findChatRoomById(room.chatRoom.id)) } private suspend fun seedProfile(publicKey: String) {