Page MenuHomePhabricator

Fix or remove capability to override user rights for the current request
Closed, ResolvedPublic

Description

Sometimes extensions modified User::$mRights to add pseudo-rights for the current request, mainly so that an account without the bot right can perform some special action with a bot flag. This was a dirty hack, but there wasn't really any non-dirty alternative. In Wikimedia production this is used (optionally) for mass user lockout (Special:MultiLock in CentralAuth); other extensions relying on it include GraphViz, UserGroups and ArticleFeedbackv5. (code search)
rMWdd6b94024c53: Re-apply: Factors out permissions check from User into PermissionManager service removes this ability; trying to write User::$mRights (outside of unit tests) now throws an exception. Either PermissionManager should provide an alternative way to achieve this, or Special:MultiLock should inject the bot flag in some other way, or the functionality should be removed cleanly from CentralAuth. (Preferably the other extensions too but that's up to their own maintainers.)

Details

Related Gerrit Patches:
mediawiki/extensions/CentralAuth : wmf/1.34.0-wmf.14[1.34.0-wmf.14] Fix SpecialMultiLock markasbot option (2nd attempt)
mediawiki/extensions/CentralAuth : wmf/1.34.0-wmf.14[1.34.0-wmf.14] Fix SpecialMultiLock markasbot option
mediawiki/core : masterAdd mechanism for temporary user rights
mediawiki/extensions/CentralAuth : wmf/1.34.0-wmf.13[1.34.0-wmf.13] Fix SpecialMultiLock markasbot option
mediawiki/extensions/CentralAuth : masterFix SpecialMultiLock markasbot option
mediawiki/core : wmf/1.34.0-wmf.14[1.34.0-wmf.14] Add mechanism for temporary user rights
mediawiki/core : wmf/1.34.0-wmf.13[1.34.0-wmf.13] Add mechanism for temporary user rights

Event Timeline

Tgr created this task.Jul 11 2019, 1:24 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 11 2019, 1:24 PM

Are you suggesting to remove Special:MultiLock?

Isarra added a subscriber: Isarra.Jul 11 2019, 2:53 PM

If we did have a more sensible way to achieve this sort of thing, it might provide a useful way around changecontentmodel right restrictions for extensions just creating new pages with different content content models (massmessage lists, collaboration hubs), without us needing to totally redo how we handle content model changes in general...

Not sure if that's really the right approach with that either, but there might also be other use cases to consider with regards to adding an alternative here as well.

Tgr added a comment.Jul 11 2019, 2:56 PM

Are you suggesting to remove Special:MultiLock?

Only the "mark as bot" option of Special:MultiLock, if I understand the code correctly.

I mean potential use cases, where the extensions/whatever aren't doing this because it's too hacky, but might actually want to do something more sane if the option were available.

Are you suggesting to remove Special:MultiLock?

Only the "mark as bot" option of Special:MultiLock, if I understand the code correctly.

May I please please ask to stop the removal of this function (the bot flag one) before a solution is found? MultiLock is the most heavily used tool for stewards at this moment. Flooding off recent changes is annoying and prevents patrolling the wiki. At this time we're dealing with a spambot attack. That means our locking activity has doubled in the last days. I don't think keeping recent changes unreadable for our users is okay. Thanks for your consideration.

Tgr added a comment.Jul 11 2019, 3:50 PM

May I please please ask to stop the removal of this function (the bot flag one) before a solution is found?

To be clear, this is already broken and will throw an exception if you try using it now. The change that broke it (a thorough refactoring of permission handling) is "too big to fail", I don't think it would be a good idea to revert it over this.

MassMessage uses the UserGetRights hook for this purpose (https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/MassMessage/+/master/includes/MassMessageHooks.php#134). Extensions could register a temporary hook and force reloading the user or something?

revi added a subscriber: revi.Jul 11 2019, 4:02 PM

If it is too big to fail (as in revert), can we get the functionality back in any possible way? It is also too big to fail for our workflow.

I hope a solution can be found then. This puts a significant burden in our job now with having us to go with the bot flag on-and-off dance if we want to avoid recent changes pollution, which in turn will cause pollution of the user rights logs for pointless user rights changes. I am between sad and frustrated that this has happened without finding an alternative first.

revi awarded a token.Jul 11 2019, 4:52 PM

Change 522148 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Add mechanism for temporary user rights

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

Change 522152 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/CentralAuth@master] Fix SpecialMultiLock markasbot option

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

Melos added a subscriber: Melos.Jul 11 2019, 6:16 PM
daniel added a subscriber: daniel.

Tagging CPT for review.

Tgr triaged this task as Unbreak Now! priority.Jul 16 2019, 1:59 PM

Making this UBN per steward input above.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJul 16 2019, 1:59 PM

Change 522148 merged by jenkins-bot:
[mediawiki/core@master] Add mechanism for temporary user rights

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

Change 523840 had a related patch set uploaded (by Tim Starling; owner: Gergő Tisza):
[mediawiki/core@wmf/1.34.0-wmf.13] [1.34.0-wmf.13] Add mechanism for temporary user rights

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

Change 523841 had a related patch set uploaded (by Tim Starling; owner: Gergő Tisza):
[mediawiki/core@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Add mechanism for temporary user rights

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

Change 523840 merged by jenkins-bot:
[mediawiki/core@wmf/1.34.0-wmf.13] [1.34.0-wmf.13] Add mechanism for temporary user rights

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

Change 523841 merged by jenkins-bot:
[mediawiki/core@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Add mechanism for temporary user rights

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

Change 522152 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Fix SpecialMultiLock markasbot option

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

Change 523843 had a related patch set uploaded (by Tim Starling; owner: Gergő Tisza):
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.13] [1.34.0-wmf.13] Fix SpecialMultiLock markasbot option

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

Change 523844 had a related patch set uploaded (by Tim Starling; owner: Gergő Tisza):
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Fix SpecialMultiLock markasbot option

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

Change 523843 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.13] [1.34.0-wmf.13] Fix SpecialMultiLock markasbot option

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

Change 523844 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Fix SpecialMultiLock markasbot option

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

Mentioned in SAL (#wikimedia-operations) [2019-07-17T03:42:53Z] <tstarling@deploy1001> Synchronized php-1.34.0-wmf.13/extensions/CentralAuth/includes/specials/SpecialMultiLock.php: T227772 (duration: 00m 56s)

Mentioned in SAL (#wikimedia-operations) [2019-07-17T03:46:16Z] <tstarling@deploy1001> Synchronized php-1.34.0-wmf.14/extensions/CentralAuth/includes/specials/SpecialMultiLock.php: T227772 (duration: 00m 54s)

tstarling lowered the priority of this task from Unbreak Now! to High.Jul 17 2019, 3:49 AM
tstarling added a subscriber: tstarling.

Fix for Special:MultiLock deployed, and I tested it in production.

Tgr closed this task as Resolved.Jul 17 2019, 8:48 AM
Tgr claimed this task.

Thanks Tim!

Filed T228248: Replace broken User::$mRights access in GraphViz and T228249: Replace broken User::$mRights access in ArticleFeedbackv5 (UserGroups has no Phab project).

Per T228025: Translate sandbox signup is broken extensions that use UserGetRights for temporary user rights might also be affected (although that bug might be specific to using it during registration). Filed T228252: Use PermissionManager::addTemporaryUserRights() in MassMessage just in case.

The bigger deal (not related to writing User::$mRights, I just noticed it while working on that ) is that user rights are now cached by user ID instead of per User object, so the cache will get polluted whenever there are significantly different User objects with the same ID. There are two cases I can think of:

tstarling raised the priority of this task from High to Unbreak Now!.Jul 18 2019, 9:31 PM

For the record, the patch landed in master after the 1.34-wmf.14 branch cut.

Yesterday, a backport to 1.34-wmf.14 was created and merged, but not successfully deployed. I found the repo in a dirty state today, and reverted in Gerrit so the git branch matches what's in production.

tstarling lowered the priority of this task from Unbreak Now! to High.Jul 18 2019, 9:38 PM

Lowering priority again since .14 has been rolled back, so the fix is still fully live.

Jdforrester-WMF raised the priority of this task from High to Unbreak Now!.Jul 18 2019, 9:39 PM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Train blockers are always UBN.

This comment was removed by Agusbou2015.

@Agusbou2015: You have been asked several times not to add unhelpful comments. Please read and understand the previous comments if you would like to stay active on Wikimedia Phabricator. Thank you for your understanding.

Change 524702 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Fix SpecialMultiLock markasbot option (2nd attempt)

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

Change 524702 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@wmf/1.34.0-wmf.14] [1.34.0-wmf.14] Fix SpecialMultiLock markasbot option (2nd attempt)

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

Mentioned in SAL (#wikimedia-operations) [2019-07-22T04:17:00Z] <tstarling@deploy1001> Synchronized php-1.34.0-wmf.14/extensions/CentralAuth/includes/specials/SpecialMultiLock.php: fix UBN bug T227772 (duration: 00m 56s)

tstarling closed this task as Resolved.Jul 22 2019, 4:17 AM

Actually the fix was not deployed on metawiki for the last three days, metawiki was running .14 before and after the train rollback. But it should be fixed now.