Page MenuHomePhabricator

Remove UserRightsProxy and replace its usages with UserGroupManager
Open, Needs TriagePublic

Description

The UserRightsProxy was used to get/set groups for the users in foreign wikis. We need to remove all usages of this class, since now the correct way of accessing groups for users in the foreign wikis is to use UserGroupManagerFactory to create a manager pointing to the correct domain.

Event Timeline

Note: In deployed code, it is only used in core, meaning that it should be possible to hard deprecate entirely in 1.35 if we moved fast

The crazy part there is that the only place it's returned from is UserrightPage::fetchUser which is overridden by SpecialGlobalGroupMembership from CentralAuth, that can return CentralAuthGroupMembershipProxy. So it's a bit trickier then it sounds..

It may be useful to make UserRIghtsProxy implement UserIdentity. This way, we can just start passing $user to UserGroupManager without worrying about whether it's a User or a UserRightsProxy. Then we can make the constructor of UserrightsProxy emit deprecation warnings.

Change 714584 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/core@master] Make UserRightsProxy implement UserIdentity

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

Change 714584 merged by jenkins-bot:

[mediawiki/core@master] Make UserRightsProxy implement UserIdentity

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

Vlad.shapik added a subscriber: Vlad.shapik.

Change 762064 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Formally deprecate UserRightsProxy

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

Change 762064 merged by jenkins-bot:

[mediawiki/core@master] Formally deprecate UserRightsProxy

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

Something interesting came out while grepping usages: T301642. That needs to be fixed somehow. Making it not use UserrightsPage at all would be nice.

What should be used for invalidateCache? Should UserRightsManager have something that can do that, or should it go elsewhere?

What should be used for invalidateCache? Should UserRightsManager have something that can do that, or should it go elsewhere?

UserRightsProxy is only there for changing user rights cross-wiki. There is UserGroupManager::clearCache(), which cleans cached group memberships for some user identity. I don't think more is needed since changing the user rights cross-wiki doesn't actually touch the user table.

It changes the user table for the other wiki, right? Surely that's why UserRightsProxy::invalidateCache exists to begin with?

I'll try to give my two cents, but @Zabe and @Urbanecm probably have a better understanding of the intended and current semantics.

So here's my current take:

  • historically, SpecialGlobalGroupMembership used to extend UserrightsPage. It used to override fetchUser, and returned a CentralAuthGroupMembershipProxy instead of the UserrightsProxy that the base class would return. CentralAuthGroupMembershipProxy was made to be duck-type compatible with UserrightsProxy, and everything relied on that fact.
  • Zabe fixed this recently, in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/738574. Now, SpecialGlobalGroupMembership is independent of UserrightsPage, and CentralAuthGroupMembershipProxy is independent of UserrightsProxy. That should make this task here a whole lot simpler!
  • This means that UserrightsPage no longer needs to support cross-wiki functionality at all. (Is that correct? Please let me know if I am mistaking here!)
  • Methods like saveSettings() and invalidateCache() can become private methods in UserrightsPage.

One more thing for context, because I saw it discussed elsewhere and now I can't find it: Title and WikiPage are guaranteed to be local. Making them support non-local pages would break that guarantee and violate LSP!

  • This means that UserrightsPage no longer needs to support cross-wiki functionality at all. (Is that correct? Please let me know if I am mistaking here!)

Core still needs to support the raw interwiki rights syntax (Username@dbnamewiki).

In T255309#7876308, @Majavah wrote:

Core still needs to support the raw interwiki rights syntax (Username@dbnamewiki).

Darn. Ok then.

I think it should be still possible to do the following:

  • create a getUserIdentity() method, and use it instead of fetchUser().
  • change all code in UserrightsPage that currently operates on a User or UserrightsProxy to operate on a UserIdentity. Move all logic from UserrightsProxy into UserrightsPage. Make sure to use the correct DB connection (and the correct UserGroupManager) per UserIdentity::getWikiId().
  • deprecate fetchUser, and make UserrightsProxy a stub. It can have the UserrightsPage injected, and delegate all calls back to UserrightsPage (the relevant methods on UserrightsPage should be tagged @internal).

Note the following code in the constructor of UserrightsdPage:

		// TODO don't hard code false, use interwiki domains. See T14518
		$this->userGroupManager = ( $userGroupManagerFactory ?? $services->getUserGroupManagerFactory() )
			->getUserGroupManager( false );

My suggestion above should fix that, I hope.

FTR, I made an attempt at removing usages of UserRightsProxy: r762507. I remember having a few doubts as to how to proceed, though. One of those questions was whether it would be better/easier to do something like r762118 (ChangeUserGroups command) first, or the other way around.

Simetrical renamed this task from Remove UserRightsProxy and replace it's usages with UserGroupManager to Remove UserRightsProxy and replace its usages with UserGroupManager.Apr 26 2022, 3:03 PM

So a basic question: if I have the ID of a non-local wiki, how do I find out the corresponding database domain? This is necessary to implement UserRightsProxy::invalidateCache as a method of UserrightsPage.

So a basic question: if I have the ID of a non-local wiki, how do I find out the corresponding database domain? This is necessary to implement UserRightsProxy::invalidateCache as a method of UserrightsPage.

For now we can assume that the wiki ID *is* the database domain. Or at least we can assume that the LoadBalancer will treat it as an alias.

Change 786318 had a related patch set uploaded (by Simetrical; author: Simetrical):

[mediawiki/core@master] Remove UserRightsProxy

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

Change 787917 had a related patch set uploaded (by Simetrical; author: Simetrical):

[mediawiki/extensions/CentralAuth@master] Try converting special page to UserIdentity

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

Change 787920 had a related patch set uploaded (by Simetrical; author: Simetrical):

[mediawiki/extensions/Echo@master] Don't mention deprecated UserRightsProxy

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

Change 787920 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Don't mention deprecated UserRightsProxy

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

Change 787917 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Remove mentions of deprecated UserRightsProxy

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

Change 890510 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] [DNM] PoC: Create a UserIdentityCacheUpdater service

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

Change 890510 merged by jenkins-bot:

[mediawiki/core@master] user: Move UserRightsProxy::invalidateCache to UserFactory

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

Change 914900 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] user: Assert wikiId in UserGroupManager

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

Change 914900 merged by jenkins-bot:

[mediawiki/core@master] user: Assert wikiId in UserGroupManager

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

Change 923706 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] specials: Use cross-wiki aware UserIdentityLookup on Special:UserRights

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