feat(ui): warn that the data-export scope reads the whole account - #61
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Android side of the API key changes in server 1.3.5.
The problem
export:readreaches the full account export - every member, journal, board message, poll and setting - regardless of how narrow the other scopes on the key are. In a picker that lists resources one per row, "Data export: Read" reads like just another resource, so it's easy to hand out a key that reads everything while believing it reads one thing.What's added
Inline warning in the create sheet as soon as data export is selected, in the theme's warning colours:
A confirmation before the key is actually created:
The confirmation is skipped when the key already grants read on every readable resource, since there's nothing surprising left to say. Matches web's behaviour.
The rule is tested, not buried
"Already reads everything" is extracted as a pure
grantsEveryRead(levels)with 7 tests, rather than living as a condition inside the composable. It's 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 unannounced.The subtle part is that write-only resources are excluded. Data import grants no read at all, so counting it would mean the check never passed and the warning would fire even on a key that genuinely does read everything. There's a test pinning that.
Not ported
The same release fixed web's "Copy" button claiming success when the browser clipboard was unavailable outside a secure context. That has no Android counterpart: no secure-context restriction, and this screen's copy button makes no success claim to be wrong about. I checked rather than assumed.
Testing
:app:assemblePlayRelease+:app:testPlayReleaseUnitTestgreen. Device checklist: