From ab8a6ec10d1f9b7dd2c6c023c190803643576e68 Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:53:00 -0400 Subject: [PATCH] feat(ui): warn that the data-export scope reads the whole account Matches the server-side API key changes in 1.3.5. Data export read reaches the full account export, every member, journal, board message, poll and setting, no matter how narrow the other scopes on the key are. Picking scopes one resource at a time makes that easy to miss: the row reads like just another resource. Selecting it now shows an inline warning in the create sheet, and creating such a key asks for confirmation first. The confirmation is skipped when the key already grants read on every readable resource, since there is nothing surprising left to warn about. The "already reads everything" rule is a pure function with tests rather than a condition buried in the composable. It is wrong in both directions in quiet ways: too eager and people learn to click through the warning, too lax and a key that reads the whole account goes out without anyone being told. Write-only resources are excluded from it, since they grant no read at all and counting them would mean the check never passed. Nothing to port from the same release's clipboard fix: that was a browser secure-context problem, and this screen's copy button makes no success claim to be wrong about. --- .../lupine/sheaf/ui/apikeys/ApiKeysScreen.kt | 59 +++++++++++++++- .../sheaf/ui/apikeys/ApiKeysViewModel.kt | 16 +++++ .../sheaf/ui/apikeys/GrantsEveryReadTest.kt | 68 +++++++++++++++++++ 3 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/ui/apikeys/GrantsEveryReadTest.kt diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysScreen.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysScreen.kt index 64072ee..8ca99ad 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysScreen.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysScreen.kt @@ -23,6 +23,7 @@ import androidx.hilt.navigation.compose.hiltViewModel import systems.lupine.sheaf.ui.components.ErrorBanner import systems.lupine.sheaf.ui.components.SectionHeader import systems.lupine.sheaf.ui.components.SheafTopAppBar +import systems.lupine.sheaf.ui.theme.LocalWarningColors @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -205,6 +206,14 @@ private fun CreateApiKeySheet( val computedScopes = remember(levels, adminLevel, isAdmin) { scopesFromLevels(levels, isAdmin, adminLevel) } + // The export endpoint hands back the whole account, so export:read is a + // read of everything however narrow the rest of the picks are. That is + // easy to miss when choosing scopes one resource at a time, so say it + // inline, and confirm before creating. + val exportSelected = (levels["export"] ?: ApiScopeLevel.NONE) != ApiScopeLevel.NONE + // Only surprising when the key does not already read everything anyway. + val grantsEveryRead = remember(levels) { grantsEveryRead(levels) } + var confirmExport by remember { mutableStateOf(false) } ModalBottomSheet( onDismissRequest = onDismiss, @@ -279,8 +288,29 @@ private fun CreateApiKeySheet( fontFamily = FontFamily.Monospace, ) + if (exportSelected) { + val warning = LocalWarningColors.current + Surface( + color = warning.container, + contentColor = warning.onContainer, + shape = MaterialTheme.shapes.small, + modifier = Modifier.fillMaxWidth(), + ) { + Text( + "Data export read lets this key download a full account " + + "export: everything in your account, not just the scopes " + + "selected above.", + style = MaterialTheme.typography.bodySmall, + modifier = Modifier.padding(12.dp), + ) + } + } + Button( - onClick = { onCreate(name, computedScopes, null) }, + onClick = { + if (exportSelected && !grantsEveryRead) confirmExport = true + else onCreate(name, computedScopes, null) + }, enabled = !isCreating && name.isNotBlank() && computedScopes.isNotEmpty(), modifier = Modifier.fillMaxWidth().height(48.dp), ) { @@ -289,6 +319,33 @@ private fun CreateApiKeySheet( } } } + + if (confirmExport) { + AlertDialog( + onDismissRequest = { confirmExport = false }, + title = { Text("This key can read everything") }, + text = { + Text( + "Data export read lets this key download a full account export: " + + "every member, journal, board message, poll, and setting in your " + + "account, not just the scopes you selected. Only hand it to " + + "something you would trust with all of it.", + ) + }, + confirmButton = { + TextButton( + enabled = !isCreating, + onClick = { + confirmExport = false + onCreate(name, computedScopes, null) + }, + ) { Text("Create key anyway") } + }, + dismissButton = { + TextButton(onClick = { confirmExport = false }) { Text("Cancel") } + }, + ) + } } @OptIn(ExperimentalMaterial3Api::class) diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysViewModel.kt index 552b4e8..b3081fa 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/apikeys/ApiKeysViewModel.kt @@ -63,6 +63,22 @@ val ALL_SCOPE_RESOURCES: List = SCOPE_GROUPS.flatMap { (_, rs) enum class ApiScopeLevel { NONE, READ, WRITE, DELETE } +/** + * True when the chosen levels already grant read on every readable resource, + * so adding data export reveals nothing the key could not already reach. + * + * Used to decide whether picking data export deserves a warning: it grants a + * read of the entire account regardless of how narrow the other picks are, + * which is easy to miss when choosing scopes one resource at a time. + * + * Write-only resources (data import) are excluded. They grant no read at all, + * so counting them would mean this never returned true and the warning would + * fire even on a key that genuinely reads everything. + */ +fun grantsEveryRead(levels: Map): Boolean = + ALL_SCOPE_RESOURCES.filterNot { it.writeOnly } + .all { (levels[it.key] ?: ApiScopeLevel.NONE) != ApiScopeLevel.NONE } + // Project the per-resource level map down to the scope strings the backend // expects. Write implies read (server-side), so we only emit the higher // level. Delete level emits both write and delete. diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/ui/apikeys/GrantsEveryReadTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/apikeys/GrantsEveryReadTest.kt new file mode 100644 index 0000000..5e20ac2 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/apikeys/GrantsEveryReadTest.kt @@ -0,0 +1,68 @@ +package systems.lupine.sheaf.ui.apikeys + +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Decides whether picking data export deserves a warning. Getting it wrong in + * either direction is bad in a quiet way: too eager and people learn to click + * through the warning, too lax and a key that reads the whole account is handed + * out without anyone being told. + */ +class GrantsEveryReadTest { + + private val readable = ALL_SCOPE_RESOURCES.filterNot { it.writeOnly } + + private fun allReadable(level: ApiScopeLevel = ApiScopeLevel.READ) = + readable.associate { it.key to level } + + @Test fun `nothing picked does not count as reading everything`() { + assertFalse(grantsEveryRead(emptyMap())) + } + + @Test fun `export alone does not count`() { + // The case the warning exists for: one narrow-looking pick that in fact + // reaches the whole account. + assertFalse(grantsEveryRead(mapOf("export" to ApiScopeLevel.READ))) + } + + @Test fun `read on every readable resource counts`() { + assertTrue(grantsEveryRead(allReadable())) + } + + @Test fun `higher levels also count as read`() { + // Write and delete imply read server-side, so they satisfy this too. + assertTrue(grantsEveryRead(allReadable(ApiScopeLevel.WRITE))) + assertTrue(grantsEveryRead(allReadable(ApiScopeLevel.DELETE))) + } + + @Test fun `one missing readable resource is enough to warn`() { + readable.forEach { held -> + val levels = allReadable() - held.key + assertFalse( + grantsEveryRead(levels), + "withholding ${held.key} should still warrant the warning", + ) + } + } + + @Test fun `an explicit none is the same as absent`() { + val levels = allReadable() + ("members" to ApiScopeLevel.NONE) + assertFalse(grantsEveryRead(levels)) + } + + @Test fun `write-only resources are not required`() { + // Data import grants no read, so demanding it would mean a key that + // genuinely reads everything still got warned at. + val writeOnly = ALL_SCOPE_RESOURCES.filter { it.writeOnly } + assertTrue(writeOnly.isNotEmpty(), "expected at least one write-only resource") + writeOnly.forEach { r -> + assertTrue( + r.key !in allReadable().keys, + "${r.key} is write-only and should not be in the readable set", + ) + } + assertTrue(grantsEveryRead(allReadable())) + } +}