laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-06 11:48:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(cordn): say who could not be added, not their pubkey

Browse Source 
Every admin failure on the group info screen was rendered as `e.message`.
Those messages are written for a log — they name pubkeys and gids in full,
which is exactly what you want when reading one and never what you want in a
chat. So somebody who searched by name, saw a face and tapped add was told
"the coordinator holds no KeyPackage for 74ee1b23…" and left to work out which
of those hex characters was the person.

That is the same thing the group info refactor spent a commit removing from
this page, and it slipped through because it arrives from a commons exception
rather than a string resource.

`CordnGroupException` gains a `Reason`, so the screen can branch on a value
rather than search for substrings in English that is free to be reworded. Four
reasons carry wording of their own — no key package, a key package belonging to
somebody else, no Welcome, not a member — and the default stays OTHER so every
other throw site keeps compiling and keeps showing its message.

The screen adds three more cases the enum cannot cover: self-removal, which
is an IllegalArgumentException from a `require`; a coordinator that does not
answer, which is the commonest failure here and says nothing useful in its own
words; and anything else, which still falls back to the message rather than to
silence. The display name comes from the row that was tapped, because the
exception only ever knows the pubkey.

Every failure is also logged in full now. The banner is for the person; logcat
is for whoever has to find out why.

Verified on the tablet against a freshly generated key that belongs to nobody
and has published nothing: "npub1wnhpkgc7…js6ksmev has not published a key
package to this coordinator, so they cannot be added yet. Ask them to open
cordn on this coordinator first." Membership stayed at 2 — `invite` throws
before it commits anything.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vitor Pamplona
2026-09-25 14:42:44 -04:00
co-authored by Claude Opus 5
parent f26a5c858e
commit 649d4ab459
4 changed files with 176 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt
+66 -10
View File
@@ -20,6 +20,8 @@
*/
package com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.cordnGroup
import android.content.Context
import android.util.Log
import androidx.compose.animation.AnimatedVisibility
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Arrangement
@@ -62,6 +64,7 @@ import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.commons.cordn.CordnGroupException
import com.vitorpamplona.amethyst.commons.cordn.GroupExposure
import com.vitorpamplona.amethyst.commons.cordn.ui.CordnExposureCard
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
@@ -90,6 +93,7 @@ import com.vitorpamplona.amethyst.ui.stringRes
import com.vitorpamplona.quartz.cordn.spec00Coordinator.JoinRequest
import com.vitorpamplona.quartz.cordn.spec01GroupMetadata.CordnGroupMetadata
import com.vitorpamplona.quartz.nip01Core.core.HexKey
import kotlinx.coroutines.TimeoutCancellationException
import kotlinx.coroutines.launch
/**
@@ -224,7 +228,12 @@ private fun CordnGroupInfo(
* roster, and a rename stayed the old name. The runtime's own methods pair
* each commit with that refresh.
*/
fun runAdmin(block: suspend (CordnRuntime) -> Unit) {
val context = LocalContext.current
fun runAdmin(
who: String? = null,
block: suspend (CordnRuntime) -> Unit,
) {
val target = runtime
if (target == null || manager == null) {
adminError = noSession
@@ -236,7 +245,11 @@ private fun CordnGroupInfo(
try {
block(target)
} catch (e: Exception) {
adminError = e.message ?: failed
// Not e.message. Those are written for a log and name pubkeys
// and gids in full, so the screen that had just shown a face
// answered with 64 hex characters.
Log.w("CordnGroupInfo", "admin action failed in ${room.gid}: ${e.message}", e)
adminError = adminFailureText(context, e, who, failed)
} finally {
busy = false
}
@@ -359,7 +372,7 @@ private fun CordnGroupInfo(
onInvite = { user ->
memberSearch = ""
userSuggestions.reset()
runAdmin { it.invite(coordinatorPubKey, room.gid, user.pubkeyHex) }
runAdmin(user.toBestDisplayName()) { it.invite(coordinatorPubKey, room.gid, user.pubkeyHex) }
},
)
}
@@ -395,6 +408,8 @@ private fun CordnGroupInfo(
}
removing?.let { target ->
val removingName = observeUserNameByHex(target, accountViewModel)
// A plain confirm rather than the shared quick-action dialog: that
// one offers "don't ask again", which for an irreversible removal
// would be a setting nobody should be nudged into.
@@ -402,17 +417,12 @@ private fun CordnGroupInfo(
onDismissRequest = { removing = null },
title = { Text(stringRes(R.string.cordn_info_remove_confirm_title)) },
text = {
Text(
stringRes(
R.string.cordn_info_remove_confirm_body,
observeUserNameByHex(target, accountViewModel),
),
)
Text(stringRes(R.string.cordn_info_remove_confirm_body, removingName))
},
confirmButton = {
TextButton(onClick = {
removing = null
runAdmin { it.removeMember(coordinatorPubKey, room.gid, target) }
runAdmin(removingName) { it.removeMember(coordinatorPubKey, room.gid, target) }
}) {
Text(
text = stringRes(R.string.cordn_info_remove_member),
@@ -567,6 +577,52 @@ private fun CordnAddMember(
}
}
/**
* What to tell somebody when an admin action failed.
*
* `CordnGroupException` carries a [CordnGroupException.Reason] precisely so this
* can be a `when` rather than a search for substrings in English. [who] is the
* display name already on screen; the exception only knows the pubkey, and a
* person who just tapped a face should not be answered with 64 hex characters.
*
* Anything with no better wording falls back to the message, which is still
* better than silence — and the caller has logged the throwable in full either
* way.
*/
private fun adminFailureText(
context: Context,
e: Throwable,
who: String?,
fallback: String,
): String {
val name = who ?: stringRes(context, R.string.cordn_admin_someone)
return when (e) {
is CordnGroupException ->
when (e.reason) {
CordnGroupException.Reason.NO_KEY_PACKAGE -> stringRes(context, R.string.cordn_admin_no_key_package, name)
CordnGroupException.Reason.WRONG_KEY_PACKAGE_OWNER -> stringRes(context, R.string.cordn_admin_wrong_key_package, name)
CordnGroupException.Reason.NO_WELCOME -> stringRes(context, R.string.cordn_admin_no_welcome, name)
CordnGroupException.Reason.NOT_A_MEMBER -> stringRes(context, R.string.cordn_admin_not_a_member, name)
CordnGroupException.Reason.OTHER -> e.message ?: fallback
}
// The UI hides Remove against yourself, so reaching this means the
// roster and the button disagreed rather than that anyone tried.
is IllegalArgumentException ->
if (e.message?.contains("self-removal") == true) {
stringRes(context, R.string.cordn_admin_no_self_remove)
} else {
e.message ?: fallback
}
// A coordinator that does not answer is the single commonest failure
// here and says nothing useful in its own words.
is TimeoutCancellationException -> stringRes(context, R.string.cordn_admin_coordinator_silent)
else -> e.message ?: fallback
}
}
/**
* The three values that only matter when something is wrong.
*
amethyst/src/main/res/values/strings.xml
+7
View File
@@ -396,6 +396,13 @@
<string name="cordn_info_add_member">Add someone</string>
<string name="cordn_info_add_member_placeholder">Name, npub or name@domain</string>
<string name="cordn_info_add_member_note">They can only be added if they published a key package to this coordinator.</string>
<string name="cordn_admin_someone">That person</string>
<string name="cordn_admin_no_key_package">%1$s has not published a key package to this coordinator, so they cannot be added yet. Ask them to open cordn on this coordinator first.</string>
<string name="cordn_admin_wrong_key_package">This coordinator served a key package that belongs to someone else, so %1$s was not added. Nothing was changed.</string>
<string name="cordn_admin_no_welcome">%1$s could not be given a way into the group, so the add was abandoned.</string>
<string name="cordn_admin_not_a_member">%1$s is not in this group.</string>
<string name="cordn_admin_no_self_remove">You cannot remove yourself from a cordn group.</string>
<string name="cordn_admin_coordinator_silent">The coordinator did not answer, so nothing was changed. Try again in a moment.</string>
<string name="cordn_info_add_member_none">Nobody found by that name.</string>
<string name="cordn_info_admin_badge">Admin</string>
<string name="cordn_invitations_members_more">+%1$d more</string>
commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupManager.kt
+42 -6
View File
@@ -235,14 +235,20 @@ class CordnGroupManager(
// theirs would spend a package they meant for someone else.
val taken =
call { coordinator.takeKeyPackage(keyPackageRef ?: targetPubKey) }
?: throw CordnGroupException("the coordinator holds no KeyPackage for $targetPubKey")
?: throw CordnGroupException(
"the coordinator holds no KeyPackage for $targetPubKey",
CordnGroupException.Reason.NO_KEY_PACKAGE,
)
val verified = KeyPackagePublication.verify(taken.publicationEvent)
if (verified.pubKey != targetPubKey) {
throw CordnGroupException("the KeyPackage served for $targetPubKey belongs to ${verified.pubKey}")
throw CordnGroupException(
"the KeyPackage served for $targetPubKey belongs to ${verified.pubKey}",
CordnGroupException.Reason.WRONG_KEY_PACKAGE_OWNER,
)
}
val result = group.addMember(verified.bytes)
val welcome = result.welcomeBytes ?: throw CordnGroupException("adding a member produced no Welcome")
val welcome = result.welcomeBytes ?: throw CordnGroupException("adding a member produced no Welcome", CordnGroupException.Reason.NO_WELCOME)
// framedCommitBytes, not commitBytes: the latter is the bare RFC 9420
// Commit struct with no MLSMessage around it, which no receiver can
@@ -297,7 +303,10 @@ class CordnGroupManager(
.entries
.firstOrNull { it.value == targetPubKey }
?.key
?: throw CordnGroupException("$targetPubKey is not a member of $gid")
?: throw CordnGroupException(
"$targetPubKey is not a member of $gid",
CordnGroupException.Reason.NOT_A_MEMBER,
)
val result = group.removeMember(leafIndex)
val posted =
@@ -946,10 +955,37 @@ class CordnGroupManager(
}
}
/** Something went wrong that is this manager's to explain, not MLS's. */
/**
* Something went wrong that is this manager's to explain, not MLS's.
*
* [reason] exists so a screen can say what happened in its own words. The
* [message] is written for a log — it names pubkeys and gids in full, which is
* what you want when reading one and never what you want in a chat — so a UI
* that rendered `e.message` showed a person who had just tapped a face a
* 64-character hex string. Branching on the reason lets it name the person
* instead, without matching on English that is free to change.
*/
class CordnGroupException(
message: String,
) : IllegalStateException(message)
val reason: Reason = Reason.OTHER,
) : IllegalStateException(message) {
enum class Reason {
/** Nobody has published a KeyPackage for this person to this coordinator. */
NO_KEY_PACKAGE,
/** The coordinator served a KeyPackage belonging to somebody else. */
WRONG_KEY_PACKAGE_OWNER,
/** The add produced no Welcome, so the invitee could never open the group. */
NO_WELCOME,
/** The person named is not in this group. */
NOT_A_MEMBER,
/** Anything with no better wording than the message itself. */
OTHER,
}
}
/**
* A Welcome that has been opened but not joined.
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupExceptionReasonTest.kt
+61
View File
@@ -0,0 +1,61 @@
/*
* 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.amethyst.commons.cordn
import kotlin.test.Test
import kotlin.test.assertEquals
/**
* The reason a screen branches on.
*
* `CordnGroupException`'s message is written for a log: it names pubkeys and
* gids in full, which is what you want when reading one and never what you
* want in a chat. A UI that rendered it answered somebody who had just tapped
* a face with 64 hex characters. [CordnGroupException.Reason] is how the
* screen says it in its own words instead, so it has to survive as a value
* rather than as English anyone is free to reword.
*/
class CordnGroupExceptionReasonTest {
@Test
fun `the reason travels with the exception`() {
val e = CordnGroupException("the coordinator holds no KeyPackage for abc", CordnGroupException.Reason.NO_KEY_PACKAGE)
assertEquals(CordnGroupException.Reason.NO_KEY_PACKAGE, e.reason)
}
@Test
fun `an exception raised without one is OTHER, not a crash`() {
// Most throw sites have no better wording than their own message, and
// must keep compiling and keep rendering that message.
val e = CordnGroupException("something else went wrong")
assertEquals(CordnGroupException.Reason.OTHER, e.reason)
}
@Test
fun `the message is left alone for the log`() {
// The reason is additive. Whatever a maintainer reads in logcat must
// still be the precise thing, pubkey and all.
val e = CordnGroupException("abc is not a member of gid-1", CordnGroupException.Reason.NOT_A_MEMBER)
assertEquals("abc is not a member of gid-1", e.message)
}
}