Page MenuHomePhabricator

Refactor SpecialPage execution checks to use Authority
Open, Needs TriagePublic

Description

Currently SpecialPage has stable to override methods related to permission checks:

  • SpecialPage::userCanExecute - User is passed in, but should probably always be the context user for the special page, it doesn't make too much sense to check for some other user. In practice SpecialPage's context user seem to be always provided, except SpecialPageFactory, where theoretically another user can be passed, but AFAIK never is.
  • SpecialPage::checkPermissions- a wrapper over "userCanExecute" which throws an exception instead of returning false.

Additionally, FormSpecialPage defines a FormSpecialPage::checkExecutePermissions, another wrapper over SpecialPage::checkPermissions which is called from FormSpecialPage::execute, and can be overridden to make more checks.

The logic behind all these methods escapes me, there's no documentation on which methods to override under which circumstances, plus when permission check method is called, it doesn't know the context, so has to apply the most restrictive RIGOR level possible, which results in reading block status from DB_PRIMARY.

Since for Authority we need to make backwards-incompatible changes anyway, so it might be a good time to clean up the interface. I propose to replace this with 3 methods, following Authority. Names TBD.

/**
  * Check whether the context user has appropriate to execute the special page if it's restricted.
  * @stable to override
  * @return bool
  */
public function isExecutionAllowed(): bool;

/**
  * Check whether the context user can probably execute the special page if they submit the form. 
  * This method offers a fast, lightweight check, and may produce false positives.
  * It is intended for determining which UI elements should be offered to the user.
  * @stable to override
  * @throws PermissionException
  * @return void
  */
protected function probablyCanExecute(): void;

/**
  * Check whether the context user definitely has appropriate rights to execute the special page.
  * Should be used immediately before performing the special page action.
  * @stable to override
  * @throws PermissionException
  * @return void
  */
protected function authorizeExecution(): void;

With PermissionException being a new exception, a wrapper over the PermissionStatus (derived from StatusValue), and a restriction that has been checked. Thus PermissionException can be converted into ErrorPageError derivative really easily for backwards compatibility.

Event Timeline

userCanExecute() predates RequestContext. It's fine to use the context user now that that concept exists.

One thing I regretted very soon after introducing SpecialPage was that subclasses are responsible for making sure various execution stages are done, including setHeaders() and checkPermissions(), because that's all in SpecialPage::execute() which we encourage subclasses to completely replace. I would prefer it if permissions were checked by default. Most restricted special pages just specify a user right when they call the parent constructor, and I think that should just work without developers having to remember to call something from execute(). But that's a bigger b/c break than what you're proposing here.

FormSpecialPage got it right. I think everything should be more like FormSpecialPage, with its execute() method that's actually called and does useful things, and its copious stages and hook points.

In general, the common things should be easy. I would lean more towards declarative rather than procedural overrides where possible.

Looking at the things that override userCanExecute(), the needs that drive it are:

  • Need multiple rights instead of just one right (SpecialInvestigateBlock)
  • A hackish config check, presumably with a confusing error message if the special page is disabled by configuration. displayRestrictionError() shows a message saying that the user should be in a particular group, which is not great if the user is already in that group. (SpecialUpload, SpecialCollabPad, SpecialEditGrowthConfig, UserCredentialsPage)
  • Implementing random chunks of logic conceptually similar to user authorization. (SpecialPasswordReset, SpecialSulRenameWarning, SpecialMWOAuthConsumerRegistration, SpecialClaimMentee)
  • No apparent special needs, just duplicating SpecialPage for no reason (SpecialCreateAccount, SpecialUndelete)

Basically, I'm skeptical that anything really deserves to call itself authorization if it's doing more than checking user rights. Even PasswordReset::isAllowed(), with its suggestive method name, has a lot of random stuff mixed in, including config. How useful is it to throw a PermissionsError exception, which will put an <h1> across the top of the page saying "Permission error", when the thing that is preventing execution is not really a permission?

So here's what I'm thinking:

  • Make the $restriction parameter to SpecialPage::__construct() accept an array
  • Introduce isEnabledByConfig(). Let the subclass return a message somehow if it is disabled.
  • A generic overridable method, with a suggestive name, to attract the random chunks of authorization-like controller logic.
  • A new overridable method to replace subclass execute() functions. Empty by default and aspirationally tending towards being abstract.
  • SpecialPage::execute() would aspirationally tend towards being final rather than being abstract and would call all these new overridable methods.

We could still have isExecutionAllowed() and probablyCanExecute() as you suggest. I would just want the number of subclasses overriding those functions to be approximately zero. The subclasses should just declare data for the base class to use to make these decisions, and we should discourage subclasses from shoehorning every error message into the concept of authorization.

I think that in big class hierarchies, it often helps to split methods that are stable to call from methods that are stable to override. That way, the base class can get in between the caller and the subclass, giving it a place to implement global policy and migration logic. If you have authorizeExecution() with complex logic in SpecialPage but also stable to override, you limit your ability to change the contract in the future or to implement backwards compatibility. That's why I tend towards overridable methods that are near-empty or abstract.

Change 695689 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] WIP: Provide a convinient way of disabling special page based on config

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

Removing inactive assignee (Platform Engineering: Please unassign tasks of previous team members.)

So here's what I'm thinking:

  • Make the $restriction parameter to SpecialPage::__construct() accept an array
  • Introduce isEnabledByConfig(). Let the subclass return a message somehow if it is disabled.
  • A generic overridable method, with a suggestive name, to attract the random chunks of authorization-like controller logic.
  • A new overridable method to replace subclass execute() functions. Empty by default and aspirationally tending towards being abstract.
  • SpecialPage::execute() would aspirationally tend towards being final rather than being abstract and would call all these new overridable methods.

Makes sense to me. execute() could also handly checking doesWrites()/checkReadOnly(). I ended up looking at this task after being confused by the presence of beforeExecute/afterExecute and the current execute() code comment about execute() saying "it will be made abstract in a future version".

Change #695689 abandoned by Hashar:

[mediawiki/core@master] WIP: Provide a convinient way of disabling special page based on config

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

Change #695689 restored by Thcipriani:

[mediawiki/core@master] WIP: Provide a convinient way of disabling special page based on config

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

Change #695689 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] WIP: Provide a convinient way of disabling special page based on config

Reason:

Proof-of-concept patch by a former WMF employee. I think it is unlikely to be merged.

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