Page MenuHomePhabricator

Update UserOptions classes to prevent temporary users having access to preferences
Closed, ResolvedPublic5 Estimated Story Points

Description

The following classes should treat temporary users the same as anonymous users (i.e. users who can't save custom preferences):

  • UserOptionsManager
  • StaticUserOptionsLookup
  • DefaultOptionsLookup
  • UserOptionsUpdateJob

Wherever these do something different based on whether a user is anonymous or registered, they may need updating (including comments).

Tests and comments should also be updated.

Event Timeline

Change 901632 had a related patch set uploaded (by AGueyte; author: AGueyte):

[mediawiki/core@master] WIP: Update UserOptions classes to prevent temporary users having access to preferences

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

A few comments:

->isNamed() isn't easily accessible and not for type UserIdentity
->isNamed() isn't in the UserNameUtils.php class

DefaultOptionsLookup.php
Can't load UserNameUtils service as it's looping

UserOptionsManager::saveOptionsInternal()
Blocks the submission and creation of a Temporary user if we treat temp user like Anon

Screenshot 2023-03-29 at 1.34.17 PM.png (626×2 px, 194 KB)

Screenshot 2023-03-29 at 12.42.11 PM.png (526×2 px, 93 KB)

A few comments:

->isNamed() isn't easily accessible and not for type UserIdentity
->isNamed() isn't in the UserNameUtils.php class

There's a related discussion here: T328799 - looks like this was intentional and your approach on the patch is correct.

DefaultOptionsLookup.php
Can't load UserNameUtils service as it's looping

Did we solve this? I've lost track of the patch sets!

UserOptionsManager::saveOptionsInternal()
Blocks the submission and creation of a Temporary user if we treat temp user like Anon

Screenshot 2023-03-29 at 1.34.17 PM.png (626×2 px, 194 KB)

Screenshot 2023-03-29 at 12.42.11 PM.png (526×2 px, 93 KB)

I think this would be solved by updating User functions to only call UserOptionsManager::saveOptions and ::saveOptionsInternal if the user is named. The calls are made here:

A few comments:

->isNamed() isn't easily accessible and not for type UserIdentity
->isNamed() isn't in the UserNameUtils.php class

There's a related discussion here: T328799 - looks like this was intentional and your approach on the patch is correct.

Therefore, we can only verify an isNamed() user by checking whether they are Registered but not Temp via UserNameUtils.
I've attempted to access those functions from MediaWikiServices and UserNameUtils and couldn't get anything done.

DefaultOptionsLookup.php
Can't load UserNameUtils service as it's looping

Did we solve this? I've lost track of the patch sets!

No! I can't think of a workaround. I need UserNameUtils to verify if the user is Temp but cannot inject the UserNameUtils service as this one needs DefaultOptionsLookup to operate. It's looping and rise an error.

UserOptionsManager::saveOptionsInternal()
Blocks the submission and creation of a Temporary user if we treat temp user like Anon

Screenshot 2023-03-29 at 1.34.17 PM.png (626×2 px, 194 KB)

Screenshot 2023-03-29 at 12.42.11 PM.png (526×2 px, 93 KB)

I think this would be solved by updating User functions to only call UserOptionsManager::saveOptions and ::saveOptionsInternal if the user is named. The calls are made here:

I had a look at some of the docs in the codebase as well as the patch that introduced DefaultOptionsLookup: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/565462

It looks like it is meant for use in an installer context before there is access to the database, and the function that calls isRegistered needs to assert that the user is not in the database, as this would indicate that it's being called from an unintended context, i.e. there's a bug:

/**
 * Checks if the DefaultOptionsLookup is usable as an instance of UserOptionsLookup.
 * It only makes sense in an installer context when UserOptionsManager cannot be yet instantiated
 * as the database is not available. Thus, this can only be called for an anon user,
 * calling under different circumstances indicates a bug.
 * @param UserIdentity $user
 * @param string $fname
 */
private function verifyUsable( UserIdentity $user, string $fname ) {
	Assert::precondition( !$user->isRegistered(), "$fname called on a registered user " );
}

In that case, it seems we don't need to update this after all, since it shouldn't be called on a temporary user either.

So multiple problems here:

UserOptionsManager.php

Creates an error.

  1. MediaWiki\Tests\User\TempUser\TempUserCreatorTest::testCreate

Undefined index: anon

/workspace/src/includes/user/UserOptionsManager.php:442

StaticUserOptionsLookup.php

Extends UserOptionsLookup
Difficulties with getting UserNameUtils service from ServiceWiring and issues test files and accessing Mediawiki services too early.
I could really user user_is_temp from UserIdentity as discussed in T333223

The situation is if I prevent temp and anon users from having their internal Options saved, the error from saveOptionsInternal is called:

throw new InvalidArgumentException( __METHOD__ . ' was called on anon or user' );

If I let a future temp user (who has an ID and an assigned temp user name but has not been created yet), I get

" 1) MediaWiki\Tests\User\TempUser\TempUserCreatorTest::testCreate
 Undefined index: anon
 /workspace/src/includes/user/UserOptionsManager.php:442"

Because of getCacheKey()

if ( !$user->isRegistered() || $isTempUser ) {
    return 'anon';
} else {
    return "u:{$user->getId()}";
}

Does saveOptionsInternal() really need to throw an InvalidArgumentException for temp users?

Change 901632 merged by jenkins-bot:

[mediawiki/core@master] Update UserOptions classes to prevent temporary users having access to preferences

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

Tchanders added a subscriber: dom_walden.

It's difficult to know what to test here. Before I merged the patch I checked that user creation still worked for temp and registered users, and that changing preferences still worked for registered users. @dom_walden any ideas?

AGueyte changed the point value for this task from 3 to 5.May 4 2023, 4:46 PM

@AGueyte Editing a page as an anonymous user on dewiki beta I see an exception in the logs:

MediaWiki\User\UserOptionsManager::saveOptionsInternal was called on anon or temporary user
from /srv/mediawiki/php-master/includes/user/UserOptionsManager.php(426)
#0 /srv/mediawiki/php-master/includes/user/UserOptionsManager.php(399): MediaWiki\User\UserOptionsManager->saveOptionsInternal(User, Wikimedia\Rdbms\DBConnRef)
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/Hooks.php(326): MediaWiki\User\UserOptionsManager->saveOptions(User)
#2 [internal function]: MediaWiki\Extension\VisualEditor\Hooks::MediaWiki\Extension\VisualEditor\{closure}()
#3 /srv/mediawiki/php-master/includes/deferred/MWCallableUpdate.php(38): call_user_func(Closure)
#4 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(473): MWCallableUpdate->doUpdate()
#5 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(398): DeferredUpdates::attemptUpdate(MWCallableUpdate, Wikimedia\Rdbms\LBFactoryMulti)
#6 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(213): DeferredUpdates::run(MWCallableUpdate, Wikimedia\Rdbms\LBFactoryMulti, Monolog\Logger, BufferingStatsdDataFactory, MediaWiki\JobQueue\JobQueueGroupFactory, string)
#7 /srv/mediawiki/php-master/includes/deferred/DeferredUpdatesScope.php(267): DeferredUpdates::{closure}(MWCallableUpdate, integer)
#8 /srv/mediawiki/php-master/includes/deferred/DeferredUpdatesScope.php(196): DeferredUpdatesScope->processStageQueue(integer, integer, Closure)
#9 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(234): DeferredUpdatesScope->processUpdates(integer, Closure)
#10 /srv/mediawiki/php-master/includes/MediaWiki.php(1125): DeferredUpdates::doUpdates()
#11 /srv/mediawiki/php-master/includes/MediaWiki.php(856): MediaWiki->restInPeace()
#12 /srv/mediawiki/php-master/includes/MediaWiki.php(604): MediaWiki->doPostOutputShutdown()
#13 /srv/mediawiki/php-master/index.php(50): MediaWiki->run()
#14 /srv/mediawiki/php-master/index.php(46): wfIndexMain()
#15 /srv/mediawiki/w/index.php(3): require(string)
#16 {main}

@AGueyte I find there are certain user options which are preserved as an anonymous user which are not as a temporary user.

For example, the dialog that pops up when you first edit which allows you to choose the VisualEditor or Wikitext editor (below). As an anonymous user, this only shows once. As a temporary user, it shows each time you refresh the page.

ve_wikitext_switcher_dialog.png (758×1 px, 110 KB)

@AGueyte Editing a page as an anonymous user on dewiki beta I see an exception in the logs:

MediaWiki\User\UserOptionsManager::saveOptionsInternal was called on anon or temporary user
from /srv/mediawiki/php-master/includes/user/UserOptionsManager.php(426)
#0 /srv/mediawiki/php-master/includes/user/UserOptionsManager.php(399): MediaWiki\User\UserOptionsManager->saveOptionsInternal(User, Wikimedia\Rdbms\DBConnRef)
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/Hooks.php(326): MediaWiki\User\UserOptionsManager->saveOptions(User)
#2 [internal function]: MediaWiki\Extension\VisualEditor\Hooks::MediaWiki\Extension\VisualEditor\{closure}()
#3 /srv/mediawiki/php-master/includes/deferred/MWCallableUpdate.php(38): call_user_func(Closure)
#4 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(473): MWCallableUpdate->doUpdate()
#5 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(398): DeferredUpdates::attemptUpdate(MWCallableUpdate, Wikimedia\Rdbms\LBFactoryMulti)
#6 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(213): DeferredUpdates::run(MWCallableUpdate, Wikimedia\Rdbms\LBFactoryMulti, Monolog\Logger, BufferingStatsdDataFactory, MediaWiki\JobQueue\JobQueueGroupFactory, string)
#7 /srv/mediawiki/php-master/includes/deferred/DeferredUpdatesScope.php(267): DeferredUpdates::{closure}(MWCallableUpdate, integer)
#8 /srv/mediawiki/php-master/includes/deferred/DeferredUpdatesScope.php(196): DeferredUpdatesScope->processStageQueue(integer, integer, Closure)
#9 /srv/mediawiki/php-master/includes/deferred/DeferredUpdates.php(234): DeferredUpdatesScope->processUpdates(integer, Closure)
#10 /srv/mediawiki/php-master/includes/MediaWiki.php(1125): DeferredUpdates::doUpdates()
#11 /srv/mediawiki/php-master/includes/MediaWiki.php(856): MediaWiki->restInPeace()
#12 /srv/mediawiki/php-master/includes/MediaWiki.php(604): MediaWiki->doPostOutputShutdown()
#13 /srv/mediawiki/php-master/index.php(50): MediaWiki->run()
#14 /srv/mediawiki/php-master/index.php(46): wfIndexMain()
#15 /srv/mediawiki/w/index.php(3): require(string)
#16 {main}

That seems like an issue for T332435: Update VisualEditor for IP masking and should be fixed with this patch.

It's difficult to know what to test here. Before I merged the patch I checked that user creation still worked for temp and registered users, and that changing preferences still worked for registered users. @dom_walden any ideas?

Not many.

I think you identified the major risk in T335971. I haven't tested many extensions yet.

I see that if you go to Special:Preferences as a temporary user you are redirected to the login page (same with anonymous users). Same with Special:ChangeEmail and Special:Mute. Also, attempting to change user preferences via the API returns notloggedin code.

I was concerned what features that set user preferences in the background would do. I tested the wikieditor to see that it wasn't trying to set user preferences via the API, which it wasn't. But, I haven't tested this very thoroughly and there are probably many other places to test. This was where I noticed the issue in T332415#8828849.

Keeping my testing to MediaWiki core for now, I tested different core actions as a temporary and anonymous user (edit, preview, viewing diffs, create account).

In case of regressions, I tested Special:Preferences as a logged in user on dewiki, enwiki and commonswiki beta.

I kept an eye on beta's logstash for any exceptions. Only saw T332415#8828832.

I have been attempting to trigger UserOptionsUpdateJob, but I don't think I have been successful yet. I did a codesearch and tried sending a message to my mentor on the Newcomer Homepage. But I think it only triggers the job if the request is a GET. Not sure how to do that.

@AGueyte Editing a page as an anonymous user on dewiki beta I see an exception in the logs:
(…)

That seems like an issue for T332435: Update VisualEditor for IP masking and should be fixed with this patch.

Now merged, feel free to retry your tests.

Side note: maybe we're missing a basic integration test here (with Selenium, edit a page as a logged-out user using VisualEditor). I'll add a note to T296187: Selenium Tests for VisualEditor.

@AGueyte Editing a page as an anonymous user on dewiki beta I see an exception in the logs:
(…)

That seems like an issue for T332435: Update VisualEditor for IP masking and should be fixed with this patch.

Now merged, feel free to retry your tests.

The exception is still occuring on dewiki. I checked that the version of VisualEditor is up-to-date. Latest logstash https://beta-logs.wmcloud.org/app/discover#/doc/5f0c9be0-0b6f-11ec-9cde-3f4490e09a26/logstash-deploy-1-7.0.0-1-2023.05.09?id=wipD_4cBbwAE2eebPR0j

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

[mediawiki/extensions/VisualEditor@master] Fix one more place where we tried to set preferences for temp users

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

Change 917987 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Fix one more place where we tried to set preferences for temp users

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

@dom_walden The VisualEditor issues should be really resolved now, thanks for spotting that.

@dom_walden The VisualEditor issues should be really resolved now, thanks for spotting that.

Yes, thank you, I don't see any exceptions anymore.

@AGueyte I find there are certain user options which are preserved as an anonymous user which are not as a temporary user.

For example, the dialog that pops up when you first edit which allows you to choose the VisualEditor or Wikitext editor (below). As an anonymous user, this only shows once. As a temporary user, it shows each time you refresh the page.

ve_wikitext_switcher_dialog.png (758×1 px, 110 KB)

I believe this is because the preference is saved as a cookie, and that the Editing Team are working on this for temporary accounts. Is that right @matmarex ?

I believe this is because the preference is saved as a cookie, and that the Editing Team are working on this for temporary accounts. Is that right @matmarex ?

Yes, and this should be resolved now by the same patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/913188 that was mentioned above.

I believe this is because the preference is saved as a cookie, and that the Editing Team are working on this for temporary accounts. Is that right @matmarex ?

Yes, and this should be resolved now by the same patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/913188 that was mentioned above.

Oh, I assumed that was just a fix for T332415#8828832.

I have checked that temporary users only see the dialog once (unless they clear their local storage). This happened for VisualEditor, WikiEditor and 2017 Wikitext editor.

I have nothing to add to this since T332415#8829911, so I am going to move this to Done.

Change 931929 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] Document that saving preferences for temporary users now throws an error

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

Change 931929 merged by jenkins-bot:

[mediawiki/core@master] Document that saving preferences for temporary users now throws an error

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