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())) + } +}