Page MenuHomePhabricator

Authorization checks have $action parameter, but accept a user right
Open, Needs TriagePublic

Description

Problem
Title::userCan(), Title::getUserPermissionsErrors(), and User::isAlowed() accept an $action parameter, which is the result of Action::getRestriction() not Action::getName(). Therefore, the parameter name / documentation implies that it accepts the name of an action, when in reality it accepts a user right.

Solution
Change the parameter name (and documentation) to be $right or $restriction or $permission to better indicate what the method(s) accept.

Event Timeline

dbarratt created this task.Dec 19 2018, 9:29 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 19 2018, 9:29 PM
Tgr added a subscriber: Tgr.Dec 22 2018, 12:09 AM

Are they? They seem to be used for user rights pretty consistently. Calling them actions in the documentation is a bit confusing, although it makes sense that most user rights would have a corresponding "action" in a sense, whether that is handled via an Action subclass or not. Also, some action names double as user rights; I'm not sure Title::userCan() handles any parameters that are not rights, though.

Action is more or less a controller in the MCV terminology, it definitely should not be used for passing representations of actions / user rights around. Also creating a new class for every user right would be really onerous.

Action is more or less a controller in the MCV terminology, it definitely should not be used for passing representations of actions / user rights around. Also creating a new class for every user right would be really onerous.

Perhaps we need a store of actions and metadata about those actions? (i.e. does this action require the user to be unblocked)?

Tgr added a comment.Jan 2 2019, 11:29 PM

Not exactly a store, because that metadata is always expected to be static, but some kind of handler or configuration, yeah. But that's not a different topic from having an action model or value object, which is really what this task talks about. I don't see what problem that would solve, or if there is really a problem to solve here, though. To the extent we have overlapping but different concepts for actions and permissions and it's unclear which should be used in a given situation, creating an ActionValue object which can hold either of those things wouldn't really help. And it would make the code less readable as most permission checks are hard-coded.

Do you remember any specific instances of passing Action::getName() into a permission check? That seems wrong, maybe it's just a coding error somewhere, or a dirty shortcut. Many actions have a corresponding permission, but passing an arbitrary action like, say,credits into a permission check won't have sane results.

dbarratt added a comment.EditedJan 3 2019, 2:48 PM

so... let's give a hypothetical...
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.

Now 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).

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).

Tgr added a comment.Jan 4 2019, 1:10 AM

That's T212341: Action authorization has multiple code paths to different results, no? The descrption of this task is talking about conflation between action names and permission names, your example does not seem to have anything to do with that.

AHH! You're right, sorry. I can't talk about conflating things without conflating things. 😉

So I looked at Action::checkCanExecute() and Title::getUserPermissionsErrors() accepts the result of Action::getRestriction() not Action::getName() (although, many times, they are the same).

So I think the parameter names / documentation should be updated to reflect what it's accepting, which is a right, not an action.

dbarratt renamed this task from User rights and actions on titles are conflated to Authorization checks have $action parameter, but accept a user right.Jan 4 2019, 8:55 PM
dbarratt updated the task description. (Show Details)
dbarratt updated the task description. (Show Details)Jan 11 2019, 4:46 PM