Page MenuHomePhabricator

Action authorization has multiple code paths to different results
Open, Needs TriagePublic

Description

Problem
Actions check to see if the user has permission to perform the action when Action::show() is executed by executing Action::checkCanExecute()

Titles check to see if the user has permission to perform an action by calling Title::userCan() or Title::getUserPermissionsErrors().

These two code paths should return the same results, but they use different logic and therefor can often return different results. This leads to inconsistencies like T208862 where a title can report that a user cannot perform an action, but the action itself allows it.

Example
Let's say I have a HowlAction that has a name howl and a permission howl. The permission is set for all users, because, this is a free society and anyone can howl at the moon if they want too.

But in the HowlAction class, I have this in checkCanExecute()

$moon = new Solaris\MoonPhase();
return $moon->phase_name() === 'Full Moon';

This means that the user can only perform the action if the moon is full.

If someone calls Title::userCan('howl') or User::isAllowed('howl') they will always get true returned to them, but when the user actually goes to perform the action, they wont be able to do so (unless it happens to be a full moon).

This could also be a problem in reverse: No user has the howl right, but HowlAction::checkCanExecute() always returns true because howling is not allowed, but nothing is stopping you from doing it.

Solution
Given that the Action class contains the title, you could argue that Title::userCan ought to be deprecated and authorization should be checked from the Action class (i.e. Action::checkCanExecute()). Alternatively, perhaps it would be better to create a ActionPermission class that contains the authorization code for an action and can be used by both Title and by Action. This class could have sensible defaults (like using the permission, etc.) or it can be extended to perform it's own logic (see above).

Doing this would result in a consistent result (and code path) for both Title::userCan() and Action::checkCanExecute() which would, in this example. always return the same result (as it should).

Related

Event Timeline

dbarratt created this task.Dec 19 2018, 9:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 19 2018, 9:10 PM
dbarratt renamed this task from Action authorization has reverse code paths, but uses different logic to Action authorization has multiple code paths to different results.Dec 19 2018, 9:11 PM
dbarratt added a project: MediaWiki-General.
dbarratt added subscribers: aezell, dmaza, Mooeypoo, Tchanders.
dbarratt updated the task description. (Show Details)Dec 20 2018, 2:49 PM
Tgr added a subscriber: Tgr.Dec 21 2018, 11:54 PM

Action::checkCanExecute() calls Title::getUserPermissionsErrors(), so it's not so much that the code paths are different, but some of the checks (notably block status checks) are not considered permission checks. Which is reasonable from a UX perspective; when the user does not have permission to do things, the option to do them is not shown (e.g. there will be a "view source" tab instead of an "edit" tab). If the user has permission but is blocked, the option is shown and trying to do it will result in an error message. Arguably "should we show the user the option of doing this" (e.g. show edit link), "should we allow the user to view the form for doing this" (display edit form) and "should weallow the user to actually do this" (save an edit, including non-idempotent actions like rate limiting) are all permission checks, but different kinds of checks that should require different methods. Instead, everything is mashed together into a single method, and things that would require more granularity, like blocking and rate limiting, happen outside the permission system. It would be nice to fix that, but it will require a fair amount of refactoring.

Tgr added a comment.Jan 2 2019, 5:46 PM

Arguably "should we show the user the option of doing this" (e.g. show edit link), "should we allow the user to view the form for doing this" (display edit form) and "should weallow the user to actually do this" (save an edit, including non-idempotent actions like rate limiting) are all permission checks, but different kinds of checks that should require different methods.

Filed T212639: Separate could do / can do / is doing permission checks in MediaWiki about that a while ago.

dbarratt updated the task description. (Show Details)Jan 4 2019, 3:46 PM
dbarratt updated the task description. (Show Details)Jan 4 2019, 3:49 PM
dbarratt updated the task description. (Show Details)Jan 4 2019, 3:58 PM