Page MenuHomePhabricator

Allow bot credentials to be scoped to edit specific pages only
Closed, ResolvedPublicFeature

Description

Feature summary (what you would like to be able to do and where):

  • Allow bot passwords or owner-only OAuth consumers to be scoped to edit specific pages only.

Use case(s) (list the steps that you performed to discover that problem, and describe the actual underlying problem which you want to solve. Do not describe only a solution):

  • To automate deployments of gadgets on-wiki, an interface-admin bot is required. Users who are not interface-admins may have merge access to the git repository where the gadget is developed, and thus also have access to the CD configuration. As these users are entrusted to manage the gadget, they should be able to trigger deployments. However, they should not be able to hijack the CD configuration and trigger edits to other sensitive pages like MediaWiki:Common.js.

Benefits (why should this be implemented?):

  • This would enable setting up CD pipelines for gadgets, while keeping the attack surface to a minimum.

(Suggested by @taavi in a Discord discussion. cc: @Soda @Novem_Linguae)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Ci/cd configuration can be separated. For example I have a Jenkins installation of which configuration is not accessible to anyone. Jenkins pools GitHub and runs Wikiploy. That Wikiploy script can be in a separate repo if I need to. There is no problem in that.

I think a more interesting thing would be to have an account with access to only my own gadget. Like for example say I just want to push my gadgets to enwiki and for whatever reason people don't want me to change more popular and default gadgets.

This could be implemented as follows:

  • Modify PermissionManager userHasRight() to take an optional third parameter which is the page id.
  • Add getAllowedPages() in PermissionManager and invoke it in userHasRight() if page id is passed. This function would call
    • getAllowedPages() in Session, which calls
      • getAllowedPages() in SessionBackend, which in turn calls
        • getAllowedPages() in SessionProvider, which is overridden by the provider implementation that checks if page id is allowed per the provider metadata
          • getAllowedPages() in BotPasswordSessionProvider
          • getAllowedPages() in SessionProvider in extensions/OAuth
  • Modify callers of userHasRight() to pass the page id for page-specific actions like edit, move, delete, and undelete.
  • Add bp_pages blob field to bot_passwords table (similar to bp_grants which is an array saved as blob). Modify SpecialBotPasswords, BotPassword and BotPasswordStore classes to handle the UI and db persistence.
  • Modify BotPasswordSessionProvider newSessionForRequest() to persist the pageids in session metadata.

Please let me know if I'm missing something.

I think it would be preferable to add such a feature to the grants system and not specifically bot passwords (a feature mainly intended for B/C and for non-OAuth wikis).

The MWRestrictions class has intentionally been written in an extensible manner so it could include arbitrary other data beyond IPs, without the need for a schema change. It has to fit into a blob (64K) though, which might be a problem depending on the intended use case / usage pattern. But usage patterns which require many pages would complicate things in other ways as well (you couldn't store the page list in memory so you'd have to do the deny-list check as a DB query within the permission check) so I think that's a reasonable limitation.


I don't think it makes much sense to add a page to userHasRight(), that's basically userCan(). Having two "can this action be applied to this page?" checks that give different results would be very confusing. It would be nice to replace usages of title-independent permission checks with title-dependent ones wherever possible, although one should be mindful of performance - title-dependent checks involve checking vastly more things. But the relevant checks for a bot editing a gadget page are already page-dependent, so I think what's really needed here is expose the page to the session - alongside Session::getAllowedUserRights(), have something like Session::userCan(), that's called from PermissionManager::checkPermissionHooks() or some similar place.

(I'll note that the reason such a change is at all possible is that SessionProvider is an abstract class wrapping an interface, and reusers are required to extend the abstract class, not implement the interface, a pattern I wish MediaWiki used more.)


I am a bit skeptical about this use case in general. If it's a default-enabled gadget, there doesn't seem to be much benefit to such a restriction, while it adds code complexity and maintenance burden. Otherwise, it's essentially an ask to differentiate lower-risk gadgets in terms of editing permissions, and a manual page list seems like an awkward way of doing that. Maybe there should just be a different user right for sitewide and non-sitewide gadgets.

In T349957#9293278, @Tgr wrote:

I am a bit skeptical about this use case in general. If it's a default-enabled gadget, there doesn't seem to be much benefit to such a restriction, while it adds code complexity and maintenance burden. Otherwise, it's essentially an ask to differentiate lower-risk gadgets in terms of editing permissions, and a manual page list seems like an awkward way of doing that. Maybe there should just be a different user right for sitewide and non-sitewide gadgets.

I would argue that this feature would be beneficial even outside this specific usecases, since it would allow bot operators in general to follow the principle of least privilege. (for example, a bot editing a few report pages could be given edit access to those specific pages, so even in the unlikely scenario that the bot infrastructure is compromised (or some invalid formatting/api change triggers a bug), the only pages they can disrupt will be the specific pages they will have access to)

Thanks for the inputs @Tgr. I missed userCan(). MWRestrictions however seems to checked only at session creation, rather than at action level.

Yeah, it would still be the session provider's responsibility to pass the information, by storing the page list (or the serialized MWRestrictions object) as session metadata, or maybe storing the bot password ID. MWRestrictions is just a mechanism for storing the information. It would expand its scope a bit, it was imagined as a set of restrictions about the request (not the action), so separate storage would be maybe a little cleaner, but not so much cleaner as to be worth all the schema changes, IMO.

Okay. So I implemented the backend updates by including the serialized MWRestrictions in session metadata. The tricky part however seems to be the UI. HTMLRestrictionsField extends HTMLTextAreaField, and is also marked stable to extend (although there is no external use except for OAuth). I suppose we would change it to extend HTMLFormElement instead, but is there a precedence for an HTMLFormElement to actually represent two inputs?

HTMLRestrictionsField extends HTMLTextAreaField, and is also marked stable to extend (although there is no external use except for OAuth).

I think that was a mistake and should be reverted. It doesn't really make sense to extend this class - it's not a fundamental component, it's a composite field meant for a specific purpose.

I suppose we would change it to extend HTMLFormElement instead, but is there a precedence for an HTMLFormElement to actually represent two inputs?

Depends on how you define "input". On the model level, it's a single input which returns a single MWRestrictions object. On the UI level, multiple input elements in a single HTMLFormField in a is not that uncommon, e.g. checkmatrix, selectandother/selectorother, autocompleteselect.

Change 971572 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] Unmark HTMLRestrictionsField as stable to extend

https://gerrit.wikimedia.org/r/971572

Change 971573 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] Refactor HTMLRestrictionsField to allow more restrictions to be added

https://gerrit.wikimedia.org/r/971573

Change 971574 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] Allow setting page restrictions on BotPassword grants

https://gerrit.wikimedia.org/r/971574

Change 971575 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/extensions/OAuth@master] Allow setting page restrictions on OAuth access grants

https://gerrit.wikimedia.org/r/971575

Change 971572 merged by jenkins-bot:

[mediawiki/core@master] Unmark HTMLRestrictionsField as stable to extend

https://gerrit.wikimedia.org/r/971572

Change 971573 merged by jenkins-bot:

[mediawiki/core@master] Refactor HTMLRestrictionsField to allow more restrictions to be added

https://gerrit.wikimedia.org/r/971573

Change 971574 merged by jenkins-bot:

[mediawiki/core@master] Allow setting page restrictions on BotPassword grants

https://gerrit.wikimedia.org/r/971574

Change 971575 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Allow setting page restrictions on OAuth access grants

https://gerrit.wikimedia.org/r/971575

Re: Tech News - What wording and link(s) would you suggest as the content?
Please suggest 1-3 sentences with 1-2 links (perhaps a documentation page with a screenshot?), or add directly within the next ~27 hours (after which the edition will be frozen for translation). Thanks!