| Tchanders | |
| Mar 17 2023, 4:37 PM |
| F36976512: ve_wikitext_switcher_dialog.png | |
| May 5 2023, 8:08 AM |
| F36934648: Screenshot 2023-03-29 at 12.42.11 PM.png | |
| Mar 30 2023, 8:47 PM |
| F36934649: Screenshot 2023-03-29 at 1.34.17 PM.png | |
| Mar 30 2023, 8:47 PM |
The following classes should treat temporary users the same as anonymous users (i.e. users who can't save custom preferences):
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.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Restricted Task | |||||
| Resolved | kostajh | T294511 2021 Security Team wikireplicas audit | |||
| Declined | None | T284948 Raw IPs of logged-out users disclosed in wiki-replicas | |||
| Resolved | Niharika | T324492 Temporary accounts - MVP | |||
| Resolved | Madalina | T326816 [Epic] Update features for temporary accounts | |||
| Resolved | Madalina | T331637 Update features in MediaWiki core for IP Masking | |||
| Resolved | Tchanders | T330815 Disallow preference setting by temporary users | |||
| Resolved | • AGueyte | T332415 Update UserOptions classes to prevent temporary users having access to preferences |
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
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
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
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:
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 loopingDid 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
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.
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
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 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.
That seems like an issue for T332435: Update VisualEditor for IP masking and should be fixed with this patch.
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.
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.
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
Change 917987 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Fix one more place where we tried to set preferences for temp users
@dom_walden The VisualEditor issues should be really resolved now, thanks for spotting that.
@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.
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
Change 931929 merged by jenkins-bot:
[mediawiki/core@master] Document that saving preferences for temporary users now throws an error