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.Jan 12 2020, 7:21 PM
DMacks added a subscriber: DMacks.Jan 15 2020, 6:33 AM

Note on the screen shot from Cyphoidbomb's failure above, this is referencing a group that doesn't even exist on enwiki "Editors".

Masumrezarock100 added a comment.EditedFeb 2 2020, 9:22 PM

A bug like this isn't so surprising since FlaggedRevs is buggy and has no maintainer currently But not sure why it is affecting Translate extension.

Zache added a comment.Feb 3 2020, 1:14 AM
Masumrezarock100 wrote:

Note on the screen shot from Cyphoidbomb's failure above, this is referencing a group that doesn't even exist on enwiki "Editors".

A bug like this isn't so surprising since FlaggedRevs is buggy and has no maintainer currently But not sure why it is affecting Translate extension.

This can be still explained with a configuration race condition. At some time times, FlaggedRevs site-specific configurations arent loaded so it is running with defaults. (ie. there is default user groups like "editors" etc)

Problem likely started when FR config was moved to an extension function in summer 2019. And also as catrope noticed it maybe not just to be Flagged revs configuration problem. So it is just more visible with a current configuration where the config is inside extension function.

QEDK added a comment.Feb 3 2020, 8:06 AM

@DannyS712 could you file a gerrit change to add the right to administrators. This is mildly annoying to say the least.

Tgr added a comment.Feb 4 2020, 4:12 AM

And also as catrope noticed it maybe not just to be Flagged revs configuration problem.

That's T236104: Cache of wmf-config/InitialiseSettings often 1 step behind. Maybe has a related cause but definitely not the same issue.

QEDK added a subscriber: Nemo_bis.Feb 5 2020, 12:33 PM

@Aklapper @Nemo_bis any timeline on when this will be fixed?

MBH added a comment.Feb 5 2020, 12:35 PM

If "Problem likely started when FR config was moved to an extension function in summer 2019", why not just roll back this change?

In T234743#5851888, @Ankit-Maity wrote:

@Aklapper @Nemo_bis any timeline on when this will be fixed?

@Ankit-Maity: Hi, I'm not sure why you are asking us specifically or why you think we could answer that. All that is known to anybody is written in this public task...

QEDK added a comment.EditedFeb 5 2020, 1:01 PM
In T234743#5851888, @Ankit-Maity wrote:

@Aklapper @Nemo_bis any timeline on when this will be fixed?

@Ankit-Maity: Hi, I'm not sure why you are asking us specifically or why you think we could answer that. All that is known to anybody is written in this public task...

Okay, maybe. But, from what I can see there is no timeline mentioned anywhere. I'll take it that means there is no timeline on this and/or no one is working on fixing this atm?

Edit: Nemo is a member of the project and you are added as a default subscriber for phab tasks, I don't know who else I can ask.

In T234743#5851941, @Ankit-Maity wrote:

from what I can see there is no timeline mentioned anywhere. I'll take it that means there is no timeline on this and/or no one is working on fixing this atm?

Correct; see https://www.mediawiki.org/wiki/Bug_management/Development_prioritization for some more background.

I have OS rights on en.wiki but couldn't revert using pending changes. Ok, I understand priorities, but still.

I have OS rights on en.wiki but couldn't revert using pending changes. Ok, I understand priorities, but still.

Not sure how Oversight right is related to pending changes.

It's not, I just mean that it's ridiculous to have OS and CU and not be able to do pending changes. I've been given the pending changes rights since posting here, it will be interesting to see if that makes a difference. On the other hand, I was able to do pc yesterday.

@DougWeller, I'm a bit lost in your comment. On enwiki at least, accepting pending changes is part of the (review) permission, this is included in the "administrators" (sysop) group and the "pending changes reviewers" (reviewer) group. I can't see what this has to do in the least with oversight and even further from what it would have to do with checkuser. It is very certainly possible to be a member of both the CU and OS group and not have access to "review" just as you wouldn't have access to many other functions.

You can't have OS without being a sysop. I apologise for confusing you. As
you say, I should obviously have access to pc review and have always had
it, until I didn't.

I believe what Doug Weller is trying to say is that enwiki oversighters are all admins and admins on enwiki hold the "review" permission. And yet sometimes there is a permission error for oversighters. Which is a bug as it shouldn't happen to users who have the "review" permission via their usergroup.

Zache added a comment.Feb 19 2020, 9:11 AM

I believe what Doug Weller is trying to say is that enwiki oversighters are all admins and admins on enwiki hold the "review" permission.

Confirmed, there is setting $wgGroupPermissions['sysop']['review'] = true; in enwiki's settings at flaggedrevs.php

OK, so I think it is confirmed that this has nothing at all to do with OS, which you absolutely can be a member of without being a member of sysop and has no nested review permissions; so this is still the original problem, that sometimes people with review permissions (such as reviwers and sysops on enwiki) are intermittently failing.

ToBeFree added a subscriber: ToBeFree.EditedWed, Mar 18, 9:38 PM

This sometimes happens to me too. Either the review interface doesn't appear, or I get an error message about a lack of permission if it appears and I use it. This time, at "Tyler1" on enwiki. A pending changes reviewer has kindly reviewed the sysop edits, but shouldn't have had to. I wonder if the bug can be circumvented by adding a sysop to the pending changes reviewer group.

I've just added myself to the group and the interface appears. The bug can be circumvented by adding a sysop to the pending changes reviewer group.

Huji added a comment.Mon, Apr 6, 6:32 PM

I, and at least two other folks on fawiki, are experiencing this bug too. On fawiki, we don't have the "circumventing" workaround explained above. Is there anyway I/we could help with resolving this?

QEDK added a comment.Mon, Apr 6, 7:00 PM

I, and at least two other folks on fawiki, are experiencing this bug too. On fawiki, we don't have the "circumventing" workaround explained above. Is there anyway I/we could help with resolving this?

Since FlaggedRevs is not being maintained (and no one is working on this and no one is sure why it's happening), the fastest way would be to make a pendingchanges usergroup on fawiki.

Huji added a comment.Tue, Apr 7, 12:42 PM

I, and at least two other folks on fawiki, are experiencing this bug too. On fawiki, we don't have the "circumventing" workaround explained above. Is there anyway I/we could help with resolving this?

Since FlaggedRevs is not being maintained (and no one is working on this and no one is sure why it's happening), the fastest way would be to make a pendingchanges usergroup on fawiki.

Can you point me to the WMF setting file in which a similar group is defined for another wiki?

QEDK added a comment.Tue, Apr 7, 1:34 PM

I, and at least two other folks on fawiki, are experiencing this bug too. On fawiki, we don't have the "circumventing" workaround explained above. Is there anyway I/we could help with resolving this?

Since FlaggedRevs is not being maintained (and no one is working on this and no one is sure why it's happening), the fastest way would be to make a pendingchanges usergroup on fawiki.

Can you point me to the WMF setting file in which a similar group is defined for another wiki?

See https://gerrit.wikimedia.org/r/plugins/gitiles/operations/mediawiki-config/+/master/wmf-config/flaggedrevs.php line 252 for the enwiki config, you also have to readd reviewer in fawiki.

Can you point me to the WMF setting file in which a similar group is defined for another wiki?

Similar group already exists in fawiki and it's the same 'patroller' group that you mentioned in the other ticket. Enwiki just decided to name the group 'pendingchanges reviewer' maybe because patroller is used for something else there. The relevant internal rights are: review and autoreview which allow reviewing others and automatic self-reviewing. Both groups have them common besides other ancillary ones not related to FR extension. The settings are defined in /wmf-config/flaggedrevs.php configuration file

Huji added a comment.Tue, Apr 7, 1:49 PM

Can you point me to the WMF setting file in which a similar group is defined for another wiki?

Similar group already exists in fawiki and it's the same 'patroller' group that you mentioned in the other ticket. Enwiki just decided to name the group 'pendingchanges reviewer' maybe because patroller is used for something else there. The relevant internal rights are: review and autoreview which allow reviewing others and automatic self-reviewing. Both groups have them common besides other ancillary ones not related to FR extension. The settings are defined in /wmf-config/flaggedrevs.php configuration file

If we have similar groups and (as far as I can see) similar settings, then why does the "workaround" not work for fawiki?

QEDK added a comment.Tue, Apr 7, 3:35 PM

Can you point me to the WMF setting file in which a similar group is defined for another wiki?

Similar group already exists in fawiki and it's the same 'patroller' group that you mentioned in the other ticket. Enwiki just decided to name the group 'pendingchanges reviewer' maybe because patroller is used for something else there. The relevant internal rights are: review and autoreview which allow reviewing others and automatic self-reviewing. Both groups have them common besides other ancillary ones not related to FR extension. The settings are defined in /wmf-config/flaggedrevs.php configuration file

If we have similar groups and (as far as I can see) similar settings, then why does the "workaround" not work for fawiki?

It's not the same, @Ammarpad . reviewer is the default group used by FlaggedRevs, fawiki unsets it and instead assigns reviewer rights to patroller. If you're not using the default reviewer group, the workaround does not work, even if you give identical rights to another group, and no way to currently explain this behaviour.

Ammarpad added a comment.EditedTue, Apr 7, 4:19 PM

I am talking about the internal review and autoreview rights that the groups share in common. I did not mention 'reviewer' group in my comment. The rights I mentioned are what the default group also have (in addition to others), so unless flaggedrevs.php is not being loaded or loaded very late they should behave the same.

Huji added a comment.Tue, Apr 7, 5:34 PM

The similarities of their names (review and reviewer) confused me. Thanks for clarifying.