mantra/mantra-kmp
1
0
Fork 0
Code Issues 2 Pull Requests Actions 1 Packages Projects Releases 1 Wiki Activity

fix(subgroups): read the admin list off the group's data, not off a flag no creator ever sets

Browse Source 
"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 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-09-09 00:30:53 +02:00
parent 0180425904
commit ccaef5d36a
4 changed files with 310 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt
View File

@@ -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<HexKey>
) {
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.
*

35
composeApp/src/commonMain/kotlin/press/mantra/compose/managers/SubgroupManager.kt
View File

@@ -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."
}

135
composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/MarmotGroupCreationJvmTest.kt Normal file
View File

@@ -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<MantraDatabase>()
)
@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<MarmotGroupCreation.Outcome.Created>(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)
)
}
}

113
composeApp/src/jvmTest/kotlin/press/mantra/compose/managers/SubgroupManagerJvmTest.kt
View File

@@ -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<String>,
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) {