Page MenuHomePhabricator

User rights validation is sometimes malfunctioning (with FlaggedRevs only?)
Open, HighPublic

Description

There are multiple current instances where users

  • Cannot proceed with an action, because they are not perceived as having rights that they are indeed assigned
  • Can proceed with an action, despite not having the rights needed

Event Timeline

DannyS712 created this task.Oct 6 2019, 8:53 AM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptOct 6 2019, 8:53 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 triaged this task as High priority.Oct 6 2019, 9:02 AM
DannyS712 moved this task from Backlog to User rights on the MediaWiki-User-management board.
DannyS712 added subscribers: Tgr, TerraCodes, Zache.

This seems like a permission handling problem, it also results occasionally in users with autoreview right getting a permission denied error on Special:UnreviewedPages.

I believe that this is indeed the case, and is found more generally than in just flagged revs:

  • Rollback user rights checks are based on Title::getUserPermissionsErrors, which uses PermissionManager::getPermissionErrors with PermissionManager::RIGOR_SECURE
  • The rights for T234736 use User::isAllowed to check a single permission

I created this parent task to better track the general investigation into why this is. My instinct would be that https://gerrit.wikimedia.org/r/plugins/gitiles/operations/mediawiki-config/+/refs/heads/master/wmf-config/InitialiseSettings.php isn't being property parsed, and thus that rollback remains with admins only be default, etc.

Zache added a comment.Oct 6 2019, 1:13 PM

AFAIK all Flaproblems what i have seen can be explained with settings from wmf-config/flaggedrevs.php is not loaded.

Tgr added a comment.Oct 6 2019, 8:32 PM

As noted in T233561#5532950 the start of these issues roughly coincides with variant settings being introduced. Since that involves an extra config cache layer, maybe what's happening is that the behavior is different on cache hit and cache miss for some reason.

DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.Oct 6 2019, 8:42 PM

As noted in T233561#5532950 the start of these issues roughly coincides with variant settings being introduced. Since that involves an extra config cache layer, maybe what's happening is that the behavior is different on cache hit and cache miss for some reason.

Pinging @Jdforrester-WMF here, then.

As noted in T233561#5532950 the start of these issues roughly coincides with variant settings being introduced. Since that involves an extra config cache layer, maybe what's happening is that the behavior is different on cache hit and cache miss for some reason.

But… it didn't. The caching code (for /tmp on the appservers) has been moved around and changed format away from serialised PHP, but we've not added any new caching. It's definitely possible that the config output is wrong, but it seems very odd that it'd be unstable and flake between different outcomes (the entire point of the work is to reduce the amount of live code and so chances for inconsistencies).

Failure to load flaggedrevs.php seems very alarming, yes.

Would it help for debugging if we could replicate this in test2.wikipedia.org?

I think that adding unreviewedpages right to sysops in flaggedrevs.php for test2wiki should do the trick.

Jdforrester-WMF renamed this task from User rights validation is malfunctioning to User rights validation is sometimes malfunctioning (with FlaggedRevs only?).Oct 15 2019, 7:58 PM

Wildly speculating, but the switch to properly use extension registration in production for FlaggedRevs may make this better (or, at least, more consitent): https://gerrit.wikimedia.org/r/543180 / T87915 / T140852

Noting this here since I don't think protection itself has been mentioned (here or in T233561), but I've now twice run into permissiondenied errors when trying to change an article from pending changes to semiprotection (enwiki, ofc).

A similar issue happened to me today while deploying this change. After I synced this change, about half the appservers (correctly) believed that $wgGEHomepageSuggestedEditsRequiresOptIn was false on cswiki, and the other half believed it was true. I checked one appserver that was returning the wrong responses, and I found that its copy of InitialiseSettings.php was correct, but running var_dump($wgGEHomepageSuggestedEditsRequiresOptIn) from eval.php returned the wrong result. I re-ran scap sync-file wmf-config/InitialiseSettings.php and that fixed it. Unfortunately I did that pretty early on, before looking at the JSON files in the cache directory in /tmp, so I wasn't able to investigate further.

MBH added a subscriber: MBH.Nov 22 2019, 11:42 AM
Zache added a subscriber: Reedy.Dec 17 2019, 9:32 AM

Based on Manual:$wgExtensionFunctions: This variable is an array that stores functions to be called after most of MediaWiki initialization is complete. Note however that at this point the RequestContext is not yet fully set up, so attempting to use it (or equivalent globals such as $wgUser or $wgTitle) is liable to fail in odd ways. If you need to use the RequestContext, consider the BeforeInitialize and ApiBeforeMain hooks instead.

However, configuration in file wmf-config/flaggedrevs.php was wrapped to function which is called from wgExtensionFunctions hook in commit 606310e502abc426a9f0b1a6cffd31aeb3d1e1c5 at Jun 24 and if $wgDBname is randomly not set up then FR config running with defaults in those cases. This could be reason for T237191 too. (ping @Reedy}

Reedy added a comment.Dec 18 2019, 1:58 AM

Wildly speculating, but the switch to properly use extension registration in production for FlaggedRevs may make this better (or, at least, more consitent): https://gerrit.wikimedia.org/r/543180 / T87915 / T140852

Probably. Or making these things worse.

The problem presumably happens when things load with the default config, and don't get overridden to turn off the rights

As has been said before, extensions should have sane defaults. But how do you set sane defaults for an extension as complex as FlaggedRevs, which has N different modes that it can be configured

Then we have stuff like https://github.com/wikimedia/operations-mediawiki-config/blob/master/wmf-config/flaggedrevs.php#L642-L669

But if we look at other bugs under this one... Rights being assigned by the default FR config, and then being removed at an arbitary time during wmf-config running... And we know complex array configs don't work well with extension registration

Ideally we strip out, and/or turn off most of the config out of FlaggedRev's extension.json (and document?).. And then set the config in flaggedrevs.php... And then strip out most of that into IS anyway? Because most of it is the same as IS...

However, configuration in file wmf-config/flaggedrevs.php was wrapped to function which is called from wgExtensionFunctions hook in commit 606310e502abc426a9f0b1a6cffd31aeb3d1e1c5 at Jun 24 and if $wgDBname is randomly not set up then FR config running with defaults in those cases. This could be reason for T237191 too. (ping @Reedy}

$wgDBname is definitely set before flaggedrevs.php is even loaded

A similar issue happened to me today while deploying this change. After I synced this change, about half the appservers (correctly) believed that $wgGEHomepageSuggestedEditsRequiresOptIn was false on cswiki, and the other half believed it was true. I checked one appserver that was returning the wrong responses, and I found that its copy of InitialiseSettings.php was correct, but running var_dump($wgGEHomepageSuggestedEditsRequiresOptIn) from eval.php returned the wrong result. I re-ran scap sync-file wmf-config/InitialiseSettings.php and that fixed it. Unfortunately I did that pretty early on, before looking at the JSON files in the cache directory in /tmp, so I wasn't able to investigate further.

The fact Roan has seen this with non FR is slightly concerning, but maybe slightly a good thing - it's not "just" FR... If it is the problems in FR and wmf-config for FR being exacerbated by the config rewrite?

I am getting a permission error response if I try to mark pages for translation on mobile (mobile site, to be precise).

Steps to reproduce

  • Find a page that doesn't have their latest version marked for translation
  • Click mark for translation and switch to mobile domain (Example)
  • Try to mark it for translation

Expected results

  • It marks the page for translation and redirects me to Special:PageTranslation homepage just like it does on the desktop site

Actual results

I get an permission error which says
The action you have requested is limited to users in the group: Translation administrators. even though I am a translation administrator on Mediawiki.org

Note

I was able to reproduced this bug on both Beta Commons and Mediawiki.org where I have translation administrator rights.

Nabla added a subscriber: Nabla.Sun, Jan 12, 7:21 PM
DMacks added a subscriber: DMacks.Wed, Jan 15, 6:33 AM