Page MenuHomePhabricator

Create a PermissionManager service
Closed, ResolvedPublic

Description

Currently, permission checks are implemented in the Title and User classes. Factoring it out removes baggage from these classes, and helps with resolving the circular dependency between User and Title.

Rough interface draft:

  • userCan( UserIdentity, Title, $permission ); // was Title::userCan* andisNamespaceProtected. Could use PageIdentity instead of Title, once we have this.
  • isBlockedFrom( UserIdentity, Title, $permission ); // was User::isBlockedFrom. Could use PageIdentity instead of Title, once we have this.

Title::userCan* and User::isBlockedFrom should be deprecated. For the deprecation period, they can delegate to the new service via the global service locator.

Details

Related Gerrit Patches:

Related Objects

Event Timeline

daniel created this task.Nov 5 2018, 7:35 PM
Tgr added a subscriber: Tgr.Nov 19 2018, 9:38 PM

Maybe a good time to also fix T180888: All permission checks should be able to return a custom error message?

Assuming that one of the goals is to avoid hidden dependencies on global state, the interface seems a bit optimistic about what a permission check will depend on. The session will have to be included (for Session::getAllowedUserRights()) and also the request ( for the IP, but possibly also things like whether it is cross-origin) - I think the current logic just assumes these always match the global context, so that can be done initially, but but's far from optional.

There's also User::isAllowed() which is probably used more often than it should be, but there are always going to be permission checks which do not relate to a page. And User::isEveryoneAllowed(). And User::getBlock()/User::getBlockedStatus() (fun fact: $user->getBlock()->preventsEdit( $title ) is not the same as $user->isBlockedFrom( $title, 'edit' )). And User::isLocallyBlockedProxy() although that can probably be deprecated as a public method. And User::getGlobalBlock()/User::isBlockedGlobally() (which is not checked by User::getBlock() because that would be too simple). And User::isLocked() (not sure if this counts as permission and it does not involve titles, but exposing it in the permission manager interface would at least reduce the number of security bugs where people forget it exists).

Also Title::userCan() has a $rigor parameter, which is not pretty, but I doubt you can just drop it.

Also there is the whole protection/restriction system, which is implemented completely independently, but conceptually very much belongs to permission management.

Tgr added a comment.Dec 22 2018, 7:43 PM

See also T212311: Blocks should be an implementation detail of an abstract authorization system and T212341: Action authorization has multiple code paths to different results (and my comment there) if there's appetite to do a bigger refactoring of the authz system instead of piecemeal changes.

Tgr added a comment.Dec 27 2018, 8:10 AM

Another thing that would be nice to figure out if there's an authz refactoring is T212639: Separate could do / can do / is doing permission checks in MediaWiki.

tosfos added a subscriber: tosfos.Mar 4 2019, 3:28 PM
tosfos reassigned this task from daniel to Vedmaka.Mar 4 2019, 3:32 PM
daniel added a comment.EditedMar 6 2019, 12:53 PM

Another thing that would be nice to figure out if there's an authz refactoring is T212639: Separate could do / can do / is doing permission checks in MediaWiki.

This could perhaps be done by adding an optional $mode parameter to userCan(): userCan( UserIdentity, Title, $permission, $mode ); Possibly modes would be "could"/"can"/"do" as you suggest, or "quick"/"full"/"final", which more fits with the model used by Title now. We'd want constants of course, not strings. The default mode would be "can" resp "full": no corners cut, and no side effects triggered.

Change 495832 had a related patch set uploaded (by Vedmaka Wakalaka; owner: Vedmaka Wakalaka):
[mediawiki/core@master] PermissionManager service draft

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

CCicalese_WMF triaged this task as Normal priority.Mar 15 2019, 5:41 PM

Change 495832 had a related patch set uploaded (by Vedmaka Wakalaka; owner: Vedmaka Wakalaka):
[mediawiki/core@master] PermissionManager service draft

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

Tchanders added a subscriber: Tchanders.EditedApr 3 2019, 11:49 AM

AHT are discussing implementing a BlockManager: T219441

Restricted Application added a subscriber: MGChecker. · View Herald TranscriptApr 3 2019, 9:09 PM

Change 495832 merged by jenkins-bot:
[mediawiki/core@master] Introduce PermissionManager service

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

daniel added a comment.EditedApr 10 2019, 7:00 PM

I don't see how - the stack trace points to Title->checkPermissionHooks(string, User, array, string, boolean). This method is removed by the patch that introduces PermissionManager, I94479b44afb3. So the error must have happened before PermissionManager was introduced. In fact, the ticket says that "Users have encountered this since at least php-1.33.0-wmf.21", so long before PermissionManager.

But the fix for whatever is going wrong will probably now involve PermissionManager, since the relevant code moved there.

T220623 seems related.

Probably not; per Timo, those issues have been in production since before this code was written, much less merged.

Since the related change was merged, do you think we should track this task as Resolved?

@Vedmaka: This task has subtasks which are still open.

@Vedmaka: This task has subtasks which are still open.

I think they are intended to be parent tasks. :)