[go: nahoru, domu]

Page MenuHomePhabricator

Make UserIdentity objects aware of which wiki they belong to
Closed, ResolvedPublic

Description

To allow service objects for safe cross-wiki operations, we need t make all entity objects aware of which wiki they belong to. UserIdentity is so far lacking this information.

Similar to what we do with PageIdentity (and soon with RevisionRecord), UserIdentity should get the following methods:

public function getWikiId: string|false;
public function assertWiki( string|false $wikiId );

And methods that return IDs that are bound to specific databases need to assert the wikiId:

public function getUserId( $wikiId = self::LOCAL ): int;
public function getActorId( $wikiId = self::LOCAL ): int;

For backwards compatibility, calls getActorId() with no $wikiId provided should not yet fail if the user belongs to another wiki, but merely trigger a deprecation warning. The getId() emthod should be deprecated, and should emit deprecation warnings when called on an instance that belongs to another wiki.

Steps

  • add wiki ID to UserIdentity
  • make getId(), getUserId() and getActorId() trigger a warning if $wikiId is LOCAL but the object belongs to another wiki. If $wikiId is provided but mismatches, throw.
  • after ensuring the above deprecations do not cause problems, make getUserId() and getActorId() always throw if $wikiId mismatches, and remove getId().

Side notes:

  • getId() should at some point be renamed to getUserId() for clarity. If we do it now, getUserId() could immediately throw on mismatching $wikiId.
  • It should probably not be possible for a UserIdentity to have a user ID but no actor ID. If a user ID is given, an actor ID must also be given, I think. There may however be existing tests that don't follow that rule.

Event Timeline

Do you think it would be valuable to define an interface (name TBD as always, I'm terrible in naming)

interface WikiAwareObject {
  public function getWikiId() : string;
}

and make UserIdentity and other usages extend it? I can't really imagine a ton of usages for the new interface, but you can for example create a common trait for all the services that allow cross-wiki object manipulation like

trait LocalWikiAssert {
    public function assertLocalWiki( WikiAwareObject $object );
}

or even have convenience methods in, say, LoadBalancerFactory like

public function getLoadBalancerFor( WikiAwareObject $entity );

Not crazy fancy, but looks kinda neat

eprodromou moved this task from Inbox to Tech Planning Review on the Platform Engineering board.
eprodromou subscribed.

Worth having some review of the code design.

Change 658751 had a related patch set uploaded (by Cicalese; owner: Cicalese):
[mediawiki/core@master] Make UserIdentity objects aware of which wiki they belong to.

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

daniel renamed this task from Make User objects aware of which wiki they belong to to Make UserIdentity objects aware of which wiki they belong to.Jan 29 2021, 2:17 PM
daniel updated the task description. (Show Details)
daniel raised the priority of this task from Medium to High.Jan 29 2021, 2:31 PM

Change 658751 merged by jenkins-bot:
[mediawiki/core@master] Make UserIdentity objects aware of which wiki they belong to.

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

Deprecation warnings are being emitted now, we need to wait until starting to throw.

Besides T298223, there have been no deprecation warnings like for the last 9 months. I think it should be fine to make the methods throw now.

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

[mediawiki/core@master] Throw for mismatched wiki in UserIdentity::getId

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

Change 787916 merged by jenkins-bot:

[mediawiki/core@master] Throw for mismatched wiki in UserIdentity::getId

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

It seems like nothing is left to do here.