There are two possible solutions here:

• Move the application of ->getAllowedUserRights() to after the hook. This prevents hook functions from overriding even if they want to.
• Have CentralAuth apply ->getAllowedUserRights() itself. This means every other extension that adds rights has to do the same thing.

Both of these, code-wise, are simple. IMO, the first is probably the best. If some extension really needs the ability to override ->getAllowedUserRights(), we could always add a new hook specifically for that purpose that has the dangers clearly documented.

In T139670#2439600, @Anomie wrote:

There are two possible solutions here:

• Move the application of ->getAllowedUserRights() to after the hook. This prevents hook functions from overriding even if they want to.
• Have CentralAuth apply ->getAllowedUserRights() itself. This means every other extension that adds rights has to do the same thing.

Both of these, code-wise, are simple. IMO, the first is probably the best. If some extension really needs the ability to override ->getAllowedUserRights(), we could always add a new hook specifically for that purpose that has the dangers clearly documented.

SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights().patch1 KBDownload

+1 to patch. I agree that first option is better.

Also, for when this is eventually public, I tried to make a phpunit test, but I had trouble getting it the existing test to run locally, so not really tested:

In T139670#2460916, @Bawolff wrote:

Also, for when this is eventually public, I tried to make a phpunit test, but I had trouble getting it the existing test to run locally, so not really tested:

0001-Add-unit-test-I6392cf.patch1 KBDownload

That patch won't actually test it, since the test doesn't call $user->getRights() anywhere.

The best place for the test is probably in UserTest.php.

This will be deployed today.

22:03 dapatrick: Deployed patch for T139670 to wmf.12

Change 306132 had a related patch set uploaded (by Ejegg):
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306132 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306093 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306103 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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