MBS-13057 / MBS-13058: Improve the application/revoke-access page #2934
Conversation
This file contains 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
|
@brainzbot, retest this please |
yvanzo
requested changes
Jul 22, 2023
This also renames "Index" to "ApplicationList" for consistency with other folders where there's a List component to list multiple X and an Index component to show data about each specific X. We do not yet have this for applications, but might eventually want it.
Right now you can enter whatever integers in the revoke-access URL and it will try to revoke your token for that application and scope. Obviously, that's unlikely to exist, and PSQL fails, causing an ISE. Given we already have the check_granted_token method which takes the same parameters, we can check whether there's something to delete before we render the form. I chose a basic 404 error with custom message rather than a NotFound page because the only way a user should get here anyway is if they edit the URL by hand or reopen a tab revoking an already revoked token.
Right now once the user clicks through to the revoke-access page, there's no longer any info printed about what access they are revoking. The user is expected to click blindly to revoke access to whatever it is and might end up revoking the wrong thing if they misclicked in the applications page. This just lists the application name and its permissions when asking for confirmation for the revoking.
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.
MBS-13057 / MBS-13058
Problem
Right now once the user clicks through to the
revoke-accesspage, there's no longer any info printed about what access they are revoking.The user is expected to click blindly to revoke access to whatever it is and might end up revoking the wrong thing if they misclicked in the applications page.
Additionally, the user can modify the URL to end up trying to revoke something completely different (which is more likely to ISE than not since it will ISE unless they guess an existing application ID and scope for their tokens).
Solution
This just lists the application name and its permissions when asking for confirmation for the revoking.
For the made-up URLs ISE, given we already have the
check_granted_tokenmethod which takes the same parameters, we can check whether there's something to delete before we render the form. I chose a basic 404 error with custom message rather than aNotFoundpage because the only way a user should get here anyway is if they edit the URL by hand so it's probably not worth a better error really.Testing
Manually, making sure the page 404s when it should and displays the app/permission data when it shouldn't (before/after for that below).
Notes
The first commit renames the files (and one component) under
/applicationsbecause as one of the first sets of files converted to React they didn't match the way we usually do this elsewhere nowadays.