Page MenuHomePhabricator

Confusing error for OAuth consumers with rollback but not edit grant
Open, Needs TriagePublic

Description

When an OAuth consumer with the “rollback” grant but without the “edit” grant attempts to roll back an edit, the following error ensues:

mwapi.errors.APIError: permissiondenied: The action you have requested is limited to users in one of the groups: *, [[Wikidata:Users|Users]].

This error is highly confusing; it took @Tgr and me a while to figure out that it’s because WikiPage::doRollback() checks for both “edit” and “rollback”, and the groups mentioned in the error message are those of “edit”, even though the requested action is “rollback”.

Possible solutions I can think of:

  • Continue to require both rights, but improve the error message and the documentation of the “rollback” grant on Special:OAuthConsumerRegistration/propose.
  • Continue to require both rights, but make it impossible to request a consumer with “rollback” and without “edit” grant.
  • Make the “rollback” grant include the “edit” right, or even all rights of the “edit” grant.
  • Change WikiPage::doRollback() to only require the “rollback” right.
  • Change WikiPage::doRollback() to only require the “edit” right on the user, but not necessarily on the consumer. (“rollback” continues to be required on both.)

As a tool author, I want my tool to be as little privileged as possible, so I prefer any solution that doesn’t give my consumer the “edit” right. No longer requiring “edit” at all for rollback sounds possibly risky for wikis where some user groups, for some reason, have the “rollback” right but not the “edit” right (I would consider this a configuration error – are there valid uses for this?), so the last solution mentioned is a sort of compromise to alleviate that, though I’m not sure if it’s easy to implement with the current permissions structure.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 3 2019, 12:38 PM
Anomie added a subscriber: Anomie.Jan 3 2019, 2:27 PM
  • Change WikiPage::doRollback() to only require the “rollback” right.

That would likely break things by allowing someone to rollback a protected page that they shouldn't have the ability to edit.

  • Change WikiPage::doRollback() to only require the “edit” right on the user, but not necessarily on the consumer. (“rollback” continues to be required on both.)

That would require a fundamental change to how MediaWiki's permissions checking works and how OAuth interacts with it, making the whole thing more complex.

There doesn't seem to be anything to do in the API itself here. Improvements to the error message here or changes in grants would have to be done in other code.

I note it's not just OAuth that could result in a permissions error message listing "wrong" groups like this. Use of $wgRevokePermissions, for example, could result in a user missing rights that their group memberships would otherwise give them.

Tgr added a comment.Jan 7 2019, 5:27 AM

Functionally this is mostly working as expected: we don't want to allow users to mess with pages they cannot edit, and we don't want to force permission handling logic to list all the potential things a user is not allowed to do (rollback, move, delete etc.) - most write operations should also check for edit rights when sensible.

OTOH it would also be reasonable to expect that one can grant permissions for various write operations without granting the right for editing (which is often more dangerous / easier to abuse). So the "check the edit right but don't require the edit grant" idea also makes sense functinally. But requiring both a right and a grant for that right for performing the action via OAuth or another grant-based mechanism is a very clear and intuitive security contract, if some perimssion checks would require both the right and the grant and some only the grant, that would make the permission system much harder to argua about. We don't want to make security properties unpredictable.

Maybe there should be a separate edit and change permission, most generic permission checks should interact with change to determine which pages the user has write access to, and all write actions including edits should require both their own permission and the change permission to be present; and change should be part of the basic grant (and would not allow any action on its own). I'm just not sure how the migration path would look - if we started to check for a new permission instead of edit on rollback, move etc. now, and by default you'd have that permission for most changes, legacy code would fail towards being more permissive, which is bad.


If we don't plan to do that, then adding the edit right to the rollback grant package does make sense; there is certainly no point in having a grant checkbox called "rollback" that does not allow you to do rollback.

As for improving the permission message, that's orthogonal to the rest of the problem, and seems pretty simple. When WikiPage::doRollback() checks for edit rights and gets some errors back, wrap them in a message saying "this action requires you to be able to edit the page, and you can't do that because of these things: ...".

As a side note, it's not nice that WikiPage::doRollback() does a bunch of permission checks instead of those being included in the permission check for rollback in the first place. That's the same kind of problem as T212341: Action authorization has multiple code paths to different results.

Splitting edit and change sounds like a good idea to me, but doesn’t seem likely to happen soon :)

As for improving the permission message, that's orthogonal to the rest of the problem, and seems pretty simple. When WikiPage::doRollback() checks for edit rights and gets some errors back, wrap them in a message saying "this action requires you to be able to edit the page, and you can't do that because of these things: ...".

I think it would also make sense in general to change the message like this:

mwapi.errors.APIError: permissiondenied: The action you have requested ('edit') is limited to users in one of the groups: *, [[Wikidata:Users|Users]].

But it looks like that might be difficult to implement, since the errors returned are just arrays of message arguments.

For now, I’ve updated the mw.o documentation and will request a new OAuth consumer for my tool, with the editpage grant as well. I can always request another, more limited consumer (or reuse the old one?) when this task is resolved.

Splitting edit and change sounds like a good idea to me, but doesn’t seem likely to happen soon :)

Filed as T213141: Separate edit and change permission in MediaWiki.