diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt index 36fc86045d..a83066cf05 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnGroupPolicy.kt @@ -30,9 +30,12 @@ import com.vitorpamplona.quartz.mls.group.MlsExporterLabel import com.vitorpamplona.quartz.mls.group.MlsGroup import com.vitorpamplona.quartz.mls.group.MlsGroupPolicy import com.vitorpamplona.quartz.mls.group.PendingProposal +import com.vitorpamplona.quartz.mls.messages.Proposal import com.vitorpamplona.quartz.mls.tree.Capabilities import com.vitorpamplona.quartz.mls.tree.Credential import com.vitorpamplona.quartz.mls.tree.Extension +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.core.toHexKey /** * cordn's profile, as an [MlsGroupPolicy]. @@ -49,20 +52,25 @@ import com.vitorpamplona.quartz.mls.tree.Extension * ) * ``` * - * ## Why there is no authorization hook + * ## Admin authorization * - * cordn has no equivalent of Marmot's MIP-03. `spec/01.md` §5.3 defines - * admins as *presentation* metadata — who the application shows a settings - * button to — and neither the spec nor the reference coordinator restricts who - * may commit. Empty `admin_pubkeys` is egalitarian mode, a deliberate and - * permanent choice, not a bootstrap window. + * `spec/01.md` §5.3 leaves the meaning of `admin_pubkeys` to the application + * and says only that an EMPTY list is egalitarian mode. The reference client + * fills that in, and because MLS has no server that can police membership, + * it does so on both sides of every commit: it refuses to build an add, a + * remove or a metadata change unless the local member is an admin, and it + * rejects an inbound commit carrying one of those from a member who is not. * - * So [authorizeCommit] is left at the default. That is a real statement about - * cordn, not an omission: **any member of a cordn group can commit anything - * MLS itself permits, including removing other members.** A UI that presents a - * cordn group's admin list as an access-control boundary would be lying. If - * `spec/01.md` later gives admins enforcement teeth, that rule belongs here, - * where an admin change can be checked against the post-commit extensions. + * [authorizeCommit] implements exactly that rule, and the engine calls it in + * both directions, so one check covers both. Matching it is not cosmetic — + * accepting a commit the rest of the group rejects forks the epoch, and MLS + * does not recover from a fork. Being *stricter* than the reference would fork + * it the other way, which is why the gate is the reference's three proposal + * types and nothing else: an Update, a SelfRemove or a PSK from any member + * stays allowed, so nobody can be trapped in a group they may not leave. + * + * Egalitarian mode is a permanent choice, not a bootstrap window: an empty + * list means every member administers, so the gate opens rather than closes. */ object CordnGroupPolicy : MlsGroupPolicy { /** @@ -169,15 +177,88 @@ object CordnGroupPolicy : MlsGroupPolicy { ) /** - * Unused today; kept so the shape of a future rule is obvious. + * Refuses a commit that adds, removes or rewrites metadata on behalf of a + * member `admin_pubkeys` does not name. See the class KDoc for why this + * matches the reference client exactly rather than approximately. * - * If cordn ever gives `admin_pubkeys` enforcement teeth, the check belongs - * here — [group] carries the pre-commit extensions and [proposals] the - * replacement, which is exactly what deciding an admin change needs. + * [committerLeafIndex] is the committer, which is who the reference checks + * for a commit. A proposal one member sent by reference and an admin then + * committed is therefore allowed — on both implementations, the admin who + * committed it is the one answering for it. */ override fun authorizeCommit( group: GroupView, proposals: List, committerLeafIndex: Int, - ) = Unit + ) { + if (proposals.isEmpty()) return + + if (adminIdentitiesIn(group.extensions).isEmpty()) return + if (proposals.none { it.proposal.needsAdmin() }) return + + check(isAdminLeaf(group, committerLeafIndex)) { + "cordn: only admin_pubkeys may add, remove or rewrite group metadata; leaf " + + "$committerLeafIndex is not an admin" + } + } + + /** + * The admin set [extensions] names, or empty for egalitarian. + * + * A metadata extension too malformed to decode reads as egalitarian rather + * than taking the group down with it — the same call Marmot's policy makes + * about its own components. It is not a way in: installing metadata takes a + * GroupContextExtensions commit, which this gate already covers, so nobody + * outside the admin set can put a broken extension there in the first + * place. A group whose metadata was malformed from creation was never + * administrable by anyone. + */ + fun adminIdentitiesIn(extensions: List): Set = + runCatching { CordnGroupMetadata.fromExtensions(extensions)?.adminPubkeys } + .getOrNull() + .orEmpty() + .toSet() + + /** True if the account [pubKey] may add, remove or rewrite metadata here. */ + fun isAdmin( + group: GroupView, + pubKey: HexKey, + ): Boolean { + val admins = adminIdentitiesIn(group.extensions) + return admins.isEmpty() || pubKey in admins + } + + /** + * True if the member at [leafIndex] may. + * + * The comparison happens in credential-bytes space, not account space, and + * deliberately: [GroupView.memberIdentityHex] hexes whatever the credential + * holds, and a cordn credential holds the account key as its 64 ASCII hex + * characters (see [CordnCredential]), so that accessor returns 128 + * characters which are never a pubkey. Marmot stores raw bytes and can + * compare directly; comparing an account pubkey against this without + * converting would match nothing and reject every commit in a group that + * names admins. Mapping the admin list forwards rather than decoding the + * leaf backwards keeps untrusted bytes out of the decoder entirely. + */ + fun isAdminLeaf( + group: GroupView, + leafIndex: Int, + ): Boolean { + val admins = adminIdentitiesIn(group.extensions) + if (admins.isEmpty()) return true + + val credentialHex = group.memberIdentityHex(leafIndex) ?: return false + return credentialHex in admins.mapTo(mutableSetOf()) { it.encodeToByteArray().toHexKey() } + } + + /** True if the local member may. */ + fun isLocalAdmin(group: GroupView): Boolean = isAdminLeaf(group, group.myLeafIndex) + + /** + * The three proposal types the reference client gates, and only those: + * `add`, `remove` and `group_context_extensions`, matching its + * `addMember` / `removeMember` / `updateGroupMetadata`. + */ + private fun Proposal.needsAdmin(): Boolean = this is Proposal.Add || this is Proposal.Remove || this is Proposal.GroupContextExtensions } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt new file mode 100644 index 0000000000..5d830daf33 --- /dev/null +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/cordn/groups/CordnAdminPolicyTest.kt @@ -0,0 +1,135 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.cordn.groups + +import com.vitorpamplona.quartz.cordn.spec01GroupMetadata.CordnGroupMetadata +import com.vitorpamplona.quartz.mls.group.MlsGroup +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * That `admin_pubkeys` actually gates commits, the way the reference client + * gates its `addMember` / `removeMember` / `updateGroupMetadata`. + * + * The engine calls [CordnGroupPolicy.authorizeCommit] before building a local + * commit AND before applying an inbound one, so these tests cover both + * directions at once: a commit this policy refuses to build is the same commit + * it refuses to apply from someone else. That matters more than the local error + * — applying a commit the rest of the group rejects forks the epoch. + */ +class CordnAdminPolicyTest { + private val alice = "a1".repeat(32) + private val bob = "b2".repeat(32) + + private fun identity(pubKeyHex: String) = CordnCredential.of(pubKeyHex).identity + + /** A group created by [creator] whose metadata names [admins]. */ + private fun group( + creator: String, + admins: List, + ): MlsGroup = + MlsGroup.create( + identity = identity(creator), + policy = CordnGroupPolicy, + initialExtensions = listOf(CordnGroupMetadata(name = "Admin test", adminPubkeys = admins).toExtension()), + ) + + @Test + fun anEmptyAdminListLetsAnyMemberRewriteMetadata() { + val egalitarian = group(creator = alice, admins = emptyList()) + val before = egalitarian.epoch + + // spec/01.md §5.3: empty means egalitarian, permanently — not a + // bootstrap window that later closes. + egalitarian.proposeGroupContextExtensions(egalitarian.extensions) + egalitarian.commit() + + assertEquals(before + 1, egalitarian.epoch) + assertTrue(CordnGroupPolicy.isLocalAdmin(egalitarian.view())) + } + + @Test + fun aNonAdminCannotRewriteMetadata() { + val group = group(creator = alice, admins = listOf(bob)) + assertFalse(CordnGroupPolicy.isLocalAdmin(group.view()), "alice must not be an admin or this proves nothing") + + group.proposeGroupContextExtensions(group.extensions) + assertFailsWith { group.commit() } + } + + @Test + fun anAdminCan() { + val group = group(creator = alice, admins = listOf(alice)) + assertTrue(CordnGroupPolicy.isLocalAdmin(group.view())) + val before = group.epoch + + group.proposeGroupContextExtensions(group.extensions) + group.commit() + + assertEquals(before + 1, group.epoch) + } + + /** + * The regression that matters most: a cordn credential stores the account + * key as 64 ASCII hex characters, so hexing the credential bytes yields 128 + * characters. An admin check that compared an account pubkey against that + * would match nothing — and since the gate only fires when a commit needs + * an admin, the failure would look like "admins can never commit" rather + * than like a decoding bug. + */ + @Test + fun theAdminListIsComparedInTheCredentialsOwnEncoding() { + val group = group(creator = alice, admins = listOf(alice)) + + val credentialHex = group.view().let { it.memberIdentityHex(it.myLeafIndex) } + assertEquals(128, credentialHex?.length, "a cordn credential hexes to twice the pubkey length") + assertTrue(credentialHex != alice, "if these were equal this test would be vacuous") + + assertTrue(group.view().let { CordnGroupPolicy.isAdminLeaf(it, it.myLeafIndex) }) + } + + @Test + fun anUpdateIsNotAnAdminAction() { + val group = group(creator = alice, admins = listOf(bob)) + val before = group.epoch + + // Only add, remove and group_context_extensions are gated. Keeping the + // rest open is what stops a non-admin being trapped in a group whose + // keys they may never rotate. + group.proposeSigningKeyRotation() + group.commit() + + assertEquals(before + 1, group.epoch) + } + + @Test + fun anEmptyCommitIsNotAnAdminAction() { + val group = group(creator = alice, admins = listOf(bob)) + val before = group.epoch + + group.commit() + + assertEquals(before + 1, group.epoch) + } +}