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.
Description
Details
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Open | None | T277511 Drop hard-deprecated User methods once they've been hard-deprecated in at least once release | |||
Open | None | T274211 Hard-deprecate soft-deprecated User methods | |||
Resolved | • Pchelolo | T234921 Factor group membership management out of User class | |||
Open | None | T275148 Prepare User group methods for hard deprecation | |||
Open | None | T255309 Remove UserRightsProxy and replace its usages with UserGroupManager | |||
Open | Umherirrender | T337590 Assert correct wiki for the user identity in UserGroupManager for cross-wiki group changes/reading |
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
Change 714584 merged by jenkins-bot:
[mediawiki/core@master] Make UserRightsProxy implement UserIdentity
Change 762064 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):
[mediawiki/core@master] Formally deprecate UserRightsProxy
Change 762064 merged by jenkins-bot:
[mediawiki/core@master] Formally deprecate UserRightsProxy
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?
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!
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.
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
Change 787917 had a related patch set uploaded (by Simetrical; author: Simetrical):
[mediawiki/extensions/CentralAuth@master] Try converting special page to UserIdentity
Change 787920 had a related patch set uploaded (by Simetrical; author: Simetrical):
[mediawiki/extensions/Echo@master] Don't mention deprecated UserRightsProxy
Change 787920 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Don't mention deprecated UserRightsProxy
Change 787917 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Remove mentions of deprecated UserRightsProxy
Change 890510 had a related patch set uploaded (by Umherirrender; author: Umherirrender):
[mediawiki/core@master] [DNM] PoC: Create a UserIdentityCacheUpdater service
Change 890510 merged by jenkins-bot:
[mediawiki/core@master] user: Move UserRightsProxy::invalidateCache to UserFactory
Change 914900 had a related patch set uploaded (by Umherirrender; author: Umherirrender):
[mediawiki/core@master] user: Assert wikiId in UserGroupManager
Change 914900 merged by jenkins-bot:
[mediawiki/core@master] user: Assert wikiId in UserGroupManager
Change 923706 had a related patch set uploaded (by Umherirrender; author: Umherirrender):
[mediawiki/core@master] specials: Use cross-wiki aware UserIdentityLookup on Special:UserRights