Page MenuHomePhabricator

User::getRights() applies session rights restrictions to non-session users
Open, Needs TriagePublic


The Session can filter the list of rights allowed to the session-user, which is used to implement grants in OAuth and BotPasswords.

But since all User objects fall back to $wgRequest, that means they all also fall back to the request Session and therefore the OAuth or BotPasswords rights restrictions apply to users it's not intended to.

Normally this probably makes little difference, since we seldom check the rights of users other than the session user. But it can be seen easily enough by using the Action API action=query&list=users&usprop=rights.

Ideally we'd only have User objects created by newFromSession() tied to the WebRequest, or at least to its Session. A potential complication is whether there's any code that loads the session-user via some method other than User::newFromSession() and checks that User object's rights.

Event Timeline

Anomie created this task.Mar 19 2019, 1:56 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2019, 1:56 PM
Tgr added a subscriber: Tgr.Mar 19 2019, 4:49 PM

User::getBlockedStatus has some ugly checks to see whether the User object whose status has been queried is the same as the global user (to determine whether the IP from $wgRequest can be used to check for IP blocks).

Looks like that check was first added in rSVN56325, fixing a similar bug where it was using the request's IP when checking blocks for all User objects rather than only the session user. No indication as to whether they compared user names (versus mFrom === 'session' or the like) there for a specific reason or just because that seemed like the safest way to not maybe break something else.

Tgr added a comment.Jul 14 2019, 3:45 PM

User::getRights() has been refactored into PermissionManager::getUserPermissions() which has a comment saying FIXME: $user->getRequest().. need to be replaced with something else. So presumably in the long term a different mechanism will be needed to signal whether the permission check should take session restrictions into account. Possibly T212639: Separate could do / can do / is doing permission checks in MediaWiki.

I agree that building that distinction into the new PermissionManager's interface would be beneficial.

Tgr added a comment.Jul 17 2019, 8:53 AM

Note that PermissionManager now has an internal cache so it's impossible for User objects representing the same User to have different rights, e.g. in case one is a session user and the other is used in some different context. (See T227772#5340242.) Although there's probably no real use case for that anyway.

Err, this task is the real use case for that. But as mentioned the use case here could be satisfied by more explicit requests for "rights for the user in this session" versus "rights for this user generically".

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:01 PM
Restricted Application added a project: Platform Engineering. · View Herald TranscriptOct 16 2020, 5:01 PM