Subject | Repo | Branch | Lines +/- | |
---|---|---|---|---|
Restrict requested grants to ones the user has | mediawiki/extensions/OAuth | master | +38 -3 |
Details
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Open | None | T90925 General authentication improvements for MediaWiki | |||
Open | None | T86869 Support a nice sso experience with MediaWiki's OAuth | |||
Open | None | T75062 OAuth permission screen needs redesign for better usability and comprehension | |||
Open | None | T94478 Only show grants that the user has permission to use, to avoid confusion |
Event Timeline
Chris could correct me here, but I expect a requirement then is that the app wouldn't even be granted the grants they hadn't explicitly approved. Otherwise the app could request all sorts of admin rights which the non-admin user doesn't see, and is never asked to allow the app to have once the user passes RFA.
@Anomie I wasn't sure if there was a database of rights that had actually been granted, or if it was just a switch "this app was approved" and then it got all of the rights it requests.
If the latter then there are schema changes to make and I should unassign myself as a pedestrian database person at best.
oauth_accepted_consumer column oaac_grants holds the grants that were actually accepted.
Yep, what @Anomie said. As long as we're only storing the rights that they
see when they approve it, I'm ok with that.
Something else that came to mind: would you show a grant like "Delete pages, revisions, and log entries" (see here) that contains 'deletelogentry', 'deleterevision', 'delete', 'bigdelete', 'edit', 'minoredit', 'browsearchive', 'undelete', 'deletedhistory', and 'deletedtext'
- If the user only has 'edit' and 'minoredit'? (e.g. a normal user)
- If the user has all the relevant rights except 'bigdelete'? (e.g. a sysop who is not a steward)
@Anomie I guess the problem is that if the user ever, in the future, *gains* those rights, then there's no way to work out in the grant which of the rights were actually properly granted.
Maybe we should keep a table of *rights* granted instead of grants? I mean, it's going to be bigger, but more granular, and it would allow us to keep things simple for 99% of users.
For history, ux was very anti showing a list of rights, which is how we got
to the rights bundles.
The ux of showing a summary list of bundles, but storing the specific
rights, which will be a subset of the bundle and might not in the future be
the intersection of bundle rights and the users rights... That seems like a
hard thing to get right. But I would love to see us do it.
Could do. Although you might have to back-calculate the list of granted grants based on which rights the user has, which were granted, and what the consumer's grants actually ask for, and consider whether it should also show "Delete pages, etc" as granted for a user who only has 'edit' and 'minoredit' and did grant "Edit existing pages". Or you could store both the granted-grants and the granted-rights, and make sure nothing iterates over granted-grants and accesses the list of requested rights for each one (for authz purposes, anyway).
Another option would be to do something like this:
- $rightsThisUserHas = Rights this user already has.
- $grantsToAskAbout = All the grants where array_diff( $thisGrantRights, $rightsThisUserHas ) === array()
- $rightsAlreadyAskedAbout = The rights granted by grants in $grantsToAskAbout.
- For each remaining grant, add it to $grantsToAskAbout if array_diff( array_intersect( $thisGrantRights, $rightsThisUserHas ), $rightsToAskAbout ) !== array().
That should at least work for the "Delete pages, etc" case when the app also asks for "Edit existing pages". But we would have to hope that a sysop wouldn't change their decision about "Delete pages, etc" were they to become a steward and gain 'bigdelete'.
OK, so, your suggestion would be to show:
- Grants that the user has all the rights for
- Grants that the user has some of the rights for
I feel like there's a less confusing way to write that code, but set that aside.
How do we explain, automatically(ish), the different combinations? Design doesn't want individual rights shown to the user, fine, but that means that either we show the partially-allowed grants (in which case, the app gets new permissions, possibly without the user's knowledge, upon user promotions), or we don't (in which case, the app doesn't get as many permissions as the user has).
As annoying as it would be to app authors, I'm leaning towards the latter, because I don't like the idea of granting apps new rights automatically.
On the other hand, my suggestion wasn't about showing the user the rights - we can keep track, internally, of what rights were granted to an app, and the user doesn't need to see it. If we show one of the partial grants, we can mark the rights that the user had in that grant as allowed, and leave the rest out. Granting newly-acquired rights in an already-accepted grant package is a totally separate UX thing that I'm not sure how to handle, but I think we can postpone for now.
That sounds good to me. I also don't like that the user gets new rights automatically.
The decision to do it the current way was made before the /identify extension was added. Now App authors can query /identify and get the effective rights of the access token they have for the user. So it might be extra work for app authors, but they now have visibility into exactly what rights the user has after they authorize, so it shouldn't be too annoying for them.
Oh, OK, I don't feel too bad about it, then. :)
I'll set about doing that now, and it should be set sometime this week.
Change 204096 had a related patch set uploaded (by MarkTraceur):
Restrict requested grants to ones the user has
I feel this task discussion has lost its track. The original problem is a conflict between two objectives of the grant presentation UI: group rights so the user is not overwhelmed, and be granular so that the user is not asked about rights they don't even have. That won't be resolved by changing how grants are stored, or the conditions of granting a right; the only way to resolve it is to relax one of those objectives (or to ensure that rights bundles always align with user groups, which sounds like a nightmare given that both can change in time and different users can receive the same right in the same bundle from different user groups).
This could use some UX expertise. IMO the current interface is okay for most users (compare with a phone app permissions screen where you typically give out much scarier permissions and the description is much more obscure, yet no one seems disturbed by it), and for those who care, some sort of "more info" link to details of those rights should do the trick. Maybe add a consumer key parameter to Special:OAuth/grants to filter it to the items of the current grant request, and improve it so that it shows which rights the user actually has. So e.g. the grant screen for the hello world app would look like this:
If there are security reasons (or UX reasons distinct from the layout of the presentation screen) to store rights one-by-one, independent of the UI, that should be a separate task.
@Vibhabamba (from email conversation):
The user seems to be a reader and an editor, but which one is it primarily _targeted at?
In theory, there is a wide range of user personas: OAuth can be used for administrator power tools, for helpers that ease routine editing tasks for the average editor, for onboarding experiences for newbies, even for alternative reading interfaces. Developers use it for authentication in tools like Phabricator.
In practice, IMO the two user groups worth focusing on are the power users who have some advanced permissions and want to be sure which of them they are granting (e.g. an administrator might want to allow page deletion to a tool but at the same time not allow blocking users; a checkuser might want to be sure that the tools they authorize do not get access to the checkuser interface as that would have wide-ranging privacy consequences) and less-experienced editors who have no idea what OAuth even is and don't recognize most of the rights, and are interacting with OAuth as part of some sort of tutorial or onboarding environment (e.g. WikiGrok or an educational project; T69082 is an example of the experience of such a user).
Can you re-arrange the order within each section of the table where 1 is most general and n is most specific.
You mean rearrange the bullet points in the right-hand cells of the Special:OAuth/grants table? It's possible although it's not always easy to tell what's more specific.
This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!
For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)