CentralAuthIdLookup: Enforce that UserIdentity objects belong to the right wiki in isAttached()
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	matmarex
	Sep 5 2024, 5:09 PM

Project Tags

Referenced Files

None

Subscribers

Description

CentralAuthIdLookup::isAttached() takes a $wikiId parameter, but it also takes a UserIdentity object that has a wiki ID associated with it. We should enforce that the two wiki IDs match, to avoid programming errors. However, the last time we tried it, it caused problems with the GlobalUserPage extension (T374122).

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Do not use full User objects for cross-wiki stuff in GlobalUserPage	mediawiki/extensions/GlobalUserPage	master	+42 -5
	CentralAuthIdLookup: Assert that user belongs to the right wiki	mediawiki/extensions/CentralAuth	master	+4 -0

Customize query in gerrit

Related Objects

Mentioned Here: T374122: "Expected MediaWiki\User\User to belong to 'metawiki', but it belongs to the local wiki" accessing any beta site while logged in

Event Timeline

matmarex created this task.Sep 5 2024, 5:09 PM

Restricted Application added a project: MediaWiki-Platform-Team. · View Herald TranscriptSep 5 2024, 5:09 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

matmarex added a subscriber: Zabe.Sep 5 2024, 5:09 PM

Change #1071006 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/CentralAuth@master] CentralAuthIdLookup: Assert that user belongs to the right wiki

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

Change #1070970 had a related patch set uploaded (by Bartosz Dziewoński; author: Zabe):

[mediawiki/extensions/GlobalUserPage@master] Do not use full User objects for cross-wiki stuff in GlobalUserPage

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

I'm not planning to work on this right now, it's free for taking if anyone wants to look into it. Current status is that the proposed GlobalUserPage change seems to work, but its CI isn't configured in a way that would allow the tests to pass with the change.

matmarex edited projects, added: Patch-Needs-Improvement, Technical-Debt; removed: Patch-For-Review.Sep 5 2024, 5:16 PM

• DAlangi_WMF moved this task from Inbox, needs triage to DO NOT USE (was: Not planned) on the MediaWiki-Platform-Team board.Sep 6 2024, 1:22 PM

JTweed-WMF edited projects, added: MediaWiki-Platform-Team (Roadmap); removed: MediaWiki-Platform-Team.Jan 22 2025, 3:45 PM

JTweed-WMF moved this task from Now => Move to Inbox to Parking Lot on the MediaWiki-Platform-Team (Roadmap) board.

OWresch-WMF moved this task from Roadmap to Not planned / Patches welcome on the MediaWiki-Platform-Team board.Jan 20 2026, 11:17 AM

OWresch-WMF edited projects, added: MediaWiki-Platform-Team; removed: MediaWiki-Platform-Team (Roadmap).

How about this:

we create a UserNameIdentity interface, which is UserIdentity without the user ID (and existence checks / isRegistered; unlike UserIdentity)
UserIdentity extends UserNameIdentity
UserNameIdentityValue implements UserNameIdentity
UserNameIdentity gets an isIP method, which is what isAnonymous should have been in a sane world (that needs UserNameUtils so we'd need a factory method or a slight DI violation to create UserNameIdentityValue objects, which is not ideal but not tragic either, User/UserIdentity need a factory as well)
CentralIdLookup interfaces are relaxed to accept UserNameIdentity in place of UserIdentity
wiki gets enforced like proposed here

Change #1071006 abandoned by Bartosz Dziewoński: