From ccaef5d36af452d31543ef05a2d95f825ab31695 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Wed, 9 Sep 2026 00:30:53 +0200 Subject: [PATCH] fix(subgroups): read the admin list off the group's data, not off a flag no creator ever sets "Only an admin of this group can make a subgroup", said to the admin who had just made the group. The guard was reading the wrong thing. **`Participant.adminAt` is written in exactly one place**: `MarmotInboundManager.processGroupMembershipChanges`, reached only from `NostrDao` on a `GroupEventResult.CommitProcessed` -- an *arriving* commit. The rows a creator makes come from `getOrCreateChatRoom` and `addMembers`, neither of which sets it. So on the device that created a room, every participant reads as a non-admin, including the creator, until some other member sends a commit it processes. A freshly made `#admins` room has had no commits at all, so a group whose epoch-0 context names three admins has none of them flagged on the one device certain to be one. **The guard now reads `MarmotGroupData.adminPubkeys`** off the room's own MLS state. That is the thing the column is a cache of: MIP-01 stamps it into the epoch-0 group context, every member gets it in their Welcome, and `processGroupMembershipChanges` reads exactly this when it writes the flag. Reading it directly cannot drift from what the group agreed and is right on both sides from the first moment. A room whose group data cannot be read refuses rather than falling back to the column. The compiler caught a second bug while this was being written: the parent's admin list was named `adminPublicKeys`, shadowing the parameter of the same name that holds the *subgroup's* picked admins -- and the ceremony room is derived from that parameter. It is now `parentAdminPublicKeys`, with a comment saying why the two must never be confused. **The column is caught up as well, because the UI still labels members with it.** `MarmotGroupCreation` stamps `adminAt` on the rows of the members the room was created with as admins -- the same list baked into `MarmotGroupData` a few lines above, so nothing new is asserted. Flags only: `processGroupMembershipChanges` also removes participants missing from the MLS tree, and running the whole reconciliation would soft-delete the rows of anyone `addMembers` could not reach, turning a partial invite into a partial membership. Widening a flag is safe; narrowing membership on a best-effort step is not. Swallowed on failure like `adopt` beside it: a missing flag costs a button not being offered, not correctness. **The old fixture was testing nothing.** It built the parent as a NIP-17 room with MLS state bolted on, and `ChatRoom.deriveChatRoomId` returns a 33-byte compressed key (66 hex) while `MarmotGroupData.nostrGroupId` takes 32 -- so `toExtension()` produced an extension `currentMarmotData()` read back as **null**, silently, taking the admin list with it. Every guard test was passing on that null. The parent is now built as what a real one is: the `#admins` room derived from the group's key, with `adminAt` deliberately left null on every row, which is the state the guard has to work in. Four new tests. Two in `SubgroupManagerJvmTest`: an admin is admitted with every `adminAt` still null, and a room with no MLS state has no admin list to consult. Two in a new `MarmotGroupCreationJvmTest`: the creator is flagged an admin of the room they just made, and the room can read its own group data back -- the second asserting `nostrGroupId` is the 64-hex derived id, since the wrong length there fails by returning null rather than by throwing. 397 common tests, 712 jvm tests, `m3Audit` meets every budget. Co-Authored-By: Claude Opus 5 --- .../compose/managers/MarmotGroupCreation.kt | 51 +++++++ .../compose/managers/SubgroupManager.kt | 35 ++++- .../managers/MarmotGroupCreationJvmTest.kt | 135 ++++++++++++++++++ .../managers/SubgroupManagerJvmTest.kt | 113 ++++++++++++--- 4 files changed, 310 insertions(+), 24 deletions(-) create mode 100644 composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/MarmotGroupCreationJvmTest.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt index ca00f401..c450c0f9 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt @@ -11,6 +11,7 @@ import kotlinx.coroutines.awaitAll import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first +import kotlin.time.Clock import kotlinx.coroutines.withTimeoutOrNull import press.mantra.compose.database.MantraDatabase import press.mantra.compose.database.model.MarmotKeyPackage @@ -180,12 +181,62 @@ object MarmotGroupCreation { logger.w("$groupId created without ${notAdded.size} member(s): $notAdded") } + stampAdmins(database, groupId, adminPublicKeys) + return Outcome.Created( room = chatRepository.getChatRoomByIdentifier(groupId) ?: localChatRoom, notAdded = notAdded ) } + /** + * Writes `adminAt` on the rows of the members this room was created with as + * admins. + * + * The creating device never learns this any other way, which is a bug older + * than subgroups and easy to miss. `Participant.adminAt` is written in exactly + * one place -- `MarmotInboundManager.processGroupMembershipChanges`, on an + * arriving commit -- while the rows a creator makes come from + * `getOrCreateChatRoom` and `addMembers`, neither of which sets it. So on the + * device that made the room, every participant reads as a non-admin until some + * *other* member sends a commit it processes: a room whose epoch-0 context + * names three admins had none of them flagged, on the one device certain to be + * one. + * + * This is the same list already baked into `MarmotGroupData` a few lines + * above, so nothing new is being asserted -- the row is being caught up with + * the group context it was created from. `processGroupMembershipChanges` + * reconciles it against the MLS tree from then on and will agree. + * + * Only flags are written. That function also *removes* participants missing + * from the tree, and running the whole reconciliation here would soft-delete + * the rows of anyone `addMembers` could not reach -- turning a partial invite + * into a partial membership. Widening a flag is safe; narrowing membership on + * a best-effort step is not. + * + * Swallowed on failure, like `adopt` above it: the room is made and usable, + * and what a missing flag costs is a button not being offered rather than + * anything being wrong. + */ + private suspend fun stampAdmins( + database: MantraDatabase, + groupId: String, + adminPublicKeys: Set + ) { + runCatching { + val room = database.chatRoomDao().findChatRoomById(groupId) ?: return + + val stamped = room.localParticipants + .map { it.participant } + .filter { it.participantPublicKey in adminPublicKeys && it.adminAt == null } + .map { it.copy(adminAt = Clock.System.now()) } + + if (stamped.isNotEmpty()) database.participantDao().upsert(stamped) + }.onFailure { + logger.e("Created $groupId but could not flag its admins", it) + } + } + /** * Every peer's published key package, or null where there is none. * 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 68ee831b..d3664934 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt @@ -114,13 +114,36 @@ object SubgroupManager { // 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 + // + // Read off the room's own `MarmotGroupData`, not off `Participant.adminAt`. + // That column is written in exactly one place -- `processGroupMembershipChanges`, + // on an arriving commit -- so on the device that *created* a room it is + // null for everybody, including the creator, until some other member + // sends a commit. A freshly made room has had no commits, so the flag + // says nobody administers a group whose epoch-0 context names three + // admins. + // + // The group data is the thing that column is a cache of: MIP-01 stamps + // `admin_pubkeys` into the epoch-0 group context, every member gets it in + // their Welcome, and `processGroupMembershipChanges` reads exactly this + // when it writes the flag. Reading it directly cannot drift from what the + // group agreed, and is right on both sides from the first moment. + // Named for the parent, because `adminPublicKeys` in this function is the + // *subgroup's* picked admins and the two must never be confused -- the + // ceremony room is derived from the second, and deriving it from the + // first would silently make a different room. + val parentAdminPublicKeys = runCatching { + parentRoom.chatRoom.toMlsGroup()?.currentMarmotData()?.adminPubkeys + }.getOrNull() + + if (parentAdminPublicKeys == null) { + // No MLS state, or state this build cannot read. A NIP-17 room has no + // admin list to consult and no group context to put a subgroup's + // certificate in, which is the same reason the button is hidden there. + return "A subgroup can only be made from a group that has admins." } - if (!isAdmin) { + + if (!parentAdminPublicKeys.contains(coordinatorPublicKey)) { return "Only an admin of this group can make a subgroup." } diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/MarmotGroupCreationJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/MarmotGroupCreationJvmTest.kt new file mode 100644 index 00000000..071aea08 --- /dev/null +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/MarmotGroupCreationJvmTest.kt @@ -0,0 +1,135 @@ +package press.mantra.compose.managers + +import androidx.room3.Room +import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.IO +import kotlinx.coroutines.runBlocking +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertIs +import kotlin.test.assertNotNull +import kotlin.test.assertTrue +import press.mantra.compose.database.MantraDatabase +import press.mantra.compose.database.builder.getRoomDatabase +import press.mantra.compose.database.model.NostrEvent +import press.mantra.compose.database.model.Profile +import press.mantra.compose.database.repository.DatabaseChatRepository +import press.mantra.compose.extensions.toHex + +/** + * What the device that *creates* a room knows about it afterwards. + * + * `Participant.adminAt` is written in exactly one place -- + * `MarmotInboundManager.processGroupMembershipChanges`, on an arriving commit -- + * while the rows a creator makes come from `getOrCreateChatRoom` and + * `addMembers`, neither of which sets it. So a room whose epoch-0 context names + * its admins had none of them flagged on the one device certain to be one, until + * some *other* member sent a commit. + * + * It read as a permissions bug: "only an admin of this group can make a + * subgroup", said to the admin who had just made the group. + * + * The guard now reads `MarmotGroupData` instead, which is right on both sides + * from epoch 0 -- but the column is still what the UI labels members with, so it + * is caught up here too. + */ +class MarmotGroupCreationJvmTest { + + private val db: MantraDatabase = getRoomDatabase( + Room.inMemoryDatabaseBuilder() + ) + + @AfterTest + fun closeDb() = db.close() + + private val keyPair = KeyPair() + private val user = keyPair.pubKey.toHexKey() + + private val repository = DatabaseChatRepository(db, CoroutineScope(Dispatchers.IO)) + + private val groupId = SharedKeyDerivation.marmotGroupId( + "02" + "ab".repeat(32) + ) + + @Test + fun `the creator is flagged an admin of the room they just created`(): Unit = runBlocking { + seedProfile(user) + + val outcome = MarmotGroupCreation.create( + database = db, + chatRepository = repository, + groupId = groupId, + name = "Ekklesia (#admins)", + purpose = "Admins of Ekklesia.", + // A group of one, so nothing has to be invited and no key package has + // to be found. The flag is the whole of what is being tested. + adminPublicKeys = setOf(user), + userPublicKey = user, + keyPair = keyPair, + ) + + assertIs(outcome) + + val room = assertNotNull(db.chatRoomDao().findChatRoomById(groupId)) + val creator = assertNotNull( + room.localParticipants.firstOrNull { it.participant.participantPublicKey == user } + ) + + assertNotNull( + creator.participant.adminAt, + "the member who created the room is one of its admins in its own epoch-0 " + + "context, and nothing else will ever tell this device so", + ) + } + + @Test + fun `the room's own group data names the admins it was created with`(): Unit = runBlocking { + seedProfile(user) + + MarmotGroupCreation.create( + database = db, + chatRepository = repository, + groupId = groupId, + name = "Ekklesia (#admins)", + purpose = "Admins of Ekklesia.", + adminPublicKeys = setOf(user), + userPublicKey = user, + keyPair = keyPair, + ) + + val room = assertNotNull(db.chatRoomDao().findChatRoomById(groupId)) + val data = assertNotNull( + room.chatRoom.toMlsGroup()?.currentMarmotData(), + "a room created here has to be able to read its own group data back", + ) + + // The source the guard reads, and the one that is right from epoch 0 on + // every device. `nostrGroupId` has to be the 32-byte derived id: a + // 33-byte value there produces an extension that reads back as null, + // silently, taking the admin list with it. + assertEquals(groupId, data.nostrGroupId) + assertEquals(64, groupId.length) + assertTrue(data.adminPubkeys.contains(user)) + } + + private suspend fun seedProfile(publicKey: String) { + val nostrEventId = publicKey.take(63) + "f" + db.nostrEventDao().upsert( + NostrEvent( + id = nostrEventId, + pubKey = publicKey, + kind = 0, + tags = emptyArray(), + content = "{}", + sig = "0".repeat(128), + ) + ) + db.profileDao().upsert( + Profile(publicKey = publicKey, userName = "member", nostrEventId = nostrEventId) + ) + } +} 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 961a55ed..d4b781c6 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt @@ -1,6 +1,10 @@ package press.mantra.compose.managers import androidx.room3.Room +import com.vitorpamplona.quartz.marmot.mip01Groups.MarmotGroupData +import com.vitorpamplona.quartz.marmot.mls.crypto.Ed25519 +import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup +import press.mantra.compose.nostr.Relays import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray import com.vitorpamplona.quartz.nip01Core.core.toHexKey @@ -31,6 +35,7 @@ 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.Participant import press.mantra.compose.database.model.Profile import press.mantra.compose.database.model.intermdiate.LocalChatRoom import press.mantra.compose.extensions.toHex @@ -271,6 +276,43 @@ class SubgroupManagerJvmTest { assertNotNull(refusal(parent, setOf(alice, bob))) } + @Test + fun `an admin is read off the group's data, not off a flag no creator ever sets`(): Unit = + runBlocking { + // The bug this replaced. `Participant.adminAt` is written in exactly + // one place -- `processGroupMembershipChanges`, on an arriving commit + // -- and a device that created a room has processed none, so every + // row reads as a non-admin including the creator's. A guard on that + // column refused the one person certain to be an admin. + // + // MarmotGroupData is right from epoch 0 on both sides, so this passes + // with every adminAt still null. + val parent = parentWith(listOf(alice, bob, carol)) + + assertTrue( + parent.localParticipants.all { it.participant.adminAt == null }, + "the fixture must not set the flag, or it is not testing the fix", + ) + assertNull(refusal(parent, setOf(alice, bob))) + } + + @Test + fun `a group with no MLS state has no admin list to consult`(): Unit = runBlocking { + // A NIP-17 room: everybody is an equal member and there is no group + // context to put a certificate in. The button is hidden there for the + // same reason, and this is the guard behind it. + (listOf(alice, bob, carol) + user).forEach { seedProfile(it) } + val nip17 = assertNotNull( + db.nostrNip17Dao().createNip17ChatRoom( + userPublicKey = user, + participantPublicKeys = listOf(alice, bob, carol), + subject = "Ekklesia", + ) + ) + + assertNotNull(refusal(nip17, setOf(alice, bob))) + } + @Test fun `a subgroup of fewer than three admins is refused`(): Unit = runBlocking { val parent = parentWith(listOf(alice, bob, carol)) @@ -446,6 +488,21 @@ class SubgroupManagerJvmTest { * and [isAdmin] are here so the tests that are *about* those two can turn * each off on its own. */ + /** + * A parent this device could actually make a subgroup of. + * + * Built as what a real parent *is* -- the `#admins` room derived from the + * group's own key -- rather than as a NIP-17 room with MLS state bolted on. + * That is not fussiness: `ChatRoom.deriveChatRoomId` returns a 33-byte + * compressed key (66 hex) and `MarmotGroupData.nostrGroupId` takes 32 bytes, + * so a NIP-17 id put in that field produces an extension `currentMarmotData()` + * reads back as **null**, silently. The fixture that did it tested nothing but + * that null. + * + * [canSign] and [isAdmin] are switchable so the tests about those two can turn + * each off on its own; both are conditions `refuseCeremonyRoom` checks before + * it looks at the picking at all. + */ private suspend fun parentWith( members: List, canSign: Boolean = true, @@ -453,21 +510,52 @@ class SubgroupManagerJvmTest { ): LocalChatRoom { (members + user).forEach { seedProfile(it) } - val room = assertNotNull( - db.nostrNip17Dao().createNip17ChatRoom( + val admins = if (isAdmin) members + user else members + + val metadata = MarmotGroupData( + nostrGroupId = parentRoomId, + name = "Ekklesia", + description = SharedKeyDerivation.describe("Admins of Ekklesia."), + adminPubkeys = admins, + relays = Relays.DefaultDMRelayList.map { it.url } + ) + val mlsGroup = MlsGroup.create( + KeyPair().pubKey, + Ed25519.generateKeyPair().privateKey, + listOf(metadata.toExtension()) + ) + + db.chatRoomDao().upsert( + ChatRoom( + id = parentRoomId, userPublicKey = user, - participantPublicKeys = members, subject = "Ekklesia", + description = metadata.description, + mlsGroupState = mlsGroup.saveState().encodeTls().toHex(), ) ) + // Deliberately with `adminAt` left null on every one of them. That column + // is only ever written by `processGroupMembershipChanges`, on an arriving + // commit, so on a device that made the room it is null for everybody -- + // which is the state the guard has to work in. + db.participantDao().upsert( + (members + user).map { + Participant( + participantPublicKey = it, + chatRoomId = parentRoomId, + relayHint = null, + ) + } + ) + 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. + // `completedKey` rederives each key-holding ceremony's room and + // matches, which is exactly how a real admin room resolves its key. db.dkgSessionDao().upsert( DkgSession( id = "parent-ceremony", - chatRoomId = room.chatRoom.id, + chatRoomId = parentRoomId, coordinatorPublicKey = user, userPublicKey = user, threshold = 2, @@ -482,18 +570,7 @@ class SubgroupManagerJvmTest { ) } - 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)) + return assertNotNull(db.chatRoomDao().findChatRoomById(parentRoomId)) } private suspend fun seedProfile(publicKey: String) {