Page MenuHomePhabricator

SpecialCentralAutoLogin calls User::saveSettings() on HTTP GET presend
Closed, ResolvedPublic

Description

This happens in /refreshCookies and /setCookies.

What is this trying to save about the user? Can it be done using DeferredUpdates?

Expectation (masterConns <= 0) by MediaWiki::main not met:
[connect to 10.64.16.31 (commonswiki)]
TransactionProfiler.php line 311 calls wfBacktrace()
TransactionProfiler.php line 146 calls TransactionProfiler->reportExpectationViolated()
LoadBalancer.php line 571 calls TransactionProfiler->recordConnection()
GlobalFunctions.php line 3122 calls LoadBalancer->getConnection()
User.php line 3817 calls wfGetDB()
SessionBackend.php line 639 calls User->saveSettings()
SessionBackend.php line 596 calls MediaWiki\Session\SessionBackend->save()
ScopedCallback.php line 74 calls Closure$MediaWiki\Session\SessionBackend::delaySave()
ScopedCallback.php line 54 calls ScopedCallback->__destruct()
SpecialCentralAutoLogin.php line 502 calls ScopedCallback::consume()
SpecialPage.php line 417 calls SpecialCentralAutoLogin->execute()
SpecialPageFactory.php line 572 calls SpecialPage->run()
MediaWiki.php line 282 calls SpecialPageFactory::executePath()
MediaWiki.php line 745 calls MediaWiki->performRequest()
MediaWiki.php line 519 calls MediaWiki->main()
index.php line 43 calls MediaWiki->run()
index.php line 3 calls include()

Event Timeline

aaron removed aaron as the assignee of this task.May 10 2016, 5:49 AM
Tgr added a subscriber: Anomie.

It logs the user in, which involves (if the user has set "remember me") storing the same user token in a cookie, in the session and in the DB, so that there is some proof of the login which survives expiration of data in the session backend.

How badly is this needed? It seems very messy to achieve: /setCookies is part of a sequence of redirects, and there is no reliable way to redirect POST requests, so it has to be a GET. It creates a session for the user; the session needs a token to be valid, so if the user did not have a token it needs to be set and saved. Creating the session cannot be delayed as it involves setting cookies and the user has been disconnected by the time DeferredUpdates is called.

So the only possibility is to set the user token in the session and cookies but delay saving it in the DB which seems like a rather fragile hack: anything trying to get the session from a different WebRequest object would break. If the user is just being autocreated, a not actually existing user would have to be set in the session, which would probably break even worse.

Note that session handling can trigger an autocreation on any kind of GET request, if the user has a valid central session but no local account yet. That results in the same problem (cookies need to be set early; we want to do the DB write late) except that we don't know anything about the code paths taken between persisting the session and DeferredUpdates. So I don't think anything could be done about that.

The specific backtrace here is saving the user_token field when it was initialized after being null, because checking the token is necessary for having a logged-in session. You could avoid it by making sure every user has the field set to a valid token (for existing users and on creation for all new users) instead of letting it be lazy-initialized. A deferred update would be liable to fail things during the current request, as Gergő already described.

As Gergő also noted, user auto-creation can happen on any request under the right circumstances. For example, someone has CentralAuth cookies for .wikipedia.org then visits foo.wikipedia.org where they don't have a local account yet. Trying to avoid that one would be particularly problematic if they visited something like index.php?action=edit, or are trying to use the API with centralauthtoken or OAuth to do a query using CORS. Deferring the creation would make the first response fail to have a logged-in user, which in the CA case would probably clear the CA cookies. That would be a different bug though.

Another thing for a different bug: Under AuthManager, consider what happens if the AuthenticationProvider redirects to a third-party service. Eventually the third-party service will send the user back to Special:UserLogin/return, but that might be a GET or the end of a chain of redirects (and we might not even be able to control that). Are you going to require a "Click this button to continue logging in" be inserted into the login process at that point?

The specific backtrace here is saving the user_token field when it was initialized after being null, because checking the token is necessary for having a logged-in session. You could avoid it by making sure every user has the field set to a valid token (for existing users and on creation for all new users) instead of letting it be lazy-initialized. A deferred update would be liable to fail things during the current request, as Gergő already described.

This makes sense.

As for account creation, that is already exempted from the logs (e.g. the "too hard" bin).

So this only affects users with user_token=''? The field is not nullable. Apparently only users created between March and June 2012 are affected by this:

mysql:wikiadmin@db1080 [enwiki]> select floor(user_id/1000000),count(*) from user where user_token='';
+------------------------+----------+
| floor(user_id/1000000) | count(*) |
+------------------------+----------+
|                     16 |   475413 |
+------------------------+----------+
1 row in set (13.51 sec)

mysql:wikiadmin@db1080 [enwiki]> select min(user_registration),max(user_registration),min(user_id),max(user_id) from user where user_id between 16000000 and 17000000 and user_token='';
+------------------------+------------------------+--------------+--------------+
| min(user_registration) | max(user_registration) | min(user_id) | max(user_id) |
+------------------------+------------------------+--------------+--------------+
| 20120322143714         | 20120615121417         |     16522112 |     16999995 |
+------------------------+------------------------+--------------+--------------+
1 row in set (0.54 sec)

So this only affects users with user_token=''? The field is not nullable. Apparently only users created between March and June 2012 are affected by this:

mysql:wikiadmin@db1080 [enwiki]> select floor(user_id/1000000),count(*) from user where user_token='';
+------------------------+----------+
| floor(user_id/1000000) | count(*) |
+------------------------+----------+
|                     16 |   475413 |
+------------------------+----------+
1 row in set (13.51 sec)

mysql:wikiadmin@db1080 [enwiki]> select min(user_registration),max(user_registration),min(user_id),max(user_id) from user where user_id between 16000000 and 17000000 and user_token='';
+------------------------+------------------------+--------------+--------------+
| min(user_registration) | max(user_registration) | min(user_id) | max(user_id) |
+------------------------+------------------------+--------------+--------------+
| 20120322143714         | 20120615121417         |     16522112 |     16999995 |
+------------------------+------------------------+--------------+--------------+
1 row in set (0.54 sec)

Is there some sort of migration script that could close this out then?

Looks like in just over 2 years, just shy of~9K accounts have been removed from that count

MariaDB [enwiki]> select floor(user_id/1000000),count(*) from user where user_token='';
+------------------------+----------+
| floor(user_id/1000000) | count(*) |
+------------------------+----------+
|                     16 |   466795 |
+------------------------+----------+
1 row in set (2 min 20.55 sec)

Can we repurpose resetUserTokens.php to do it? It has a provision for "null". Something similar for 'user_token' => '' seems trivial (either empty xor null). Then just run it everywhere (or, well, on older wikis)

		if ( $nullsOnly ) {
			// Have to build this by hand, because \ is escaped in helper functions
			$where = [ 'user_token = \'' . str_repeat( '\0', 32 ) . '\'' ];
		}

Was added in 4191b3d0b1b134610114c165487e30e6d82402f6 for T43586: On wikidata.org, api login returns wrong lgtoken (object replacement character) in response body; correct in HTTP header

Of course...

-- A pseudorandomly generated value that is stored in
-- a cookie when the "remember password" feature is
-- used (previously, a hash of the password was used, but
-- this was vulnerable to cookie-stealing attacks)
user_token binary(32) NOT NULL default '',

So obviously any "fixing" doesn't necessarily fix the default '' in the schema.. But as long as MW is populating it... Maybe we should be removing the default?

Krinkle assigned this task to aaron.
Krinkle subscribed.

This was fixed in 2015 by @aaron, which moved the user options change to post-send, which is good enough for the immediate restriction on pre-send GET master conns and Multi-DC.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/234158

Note that this is for the /refreshCookies route. The setCookies route nor any of the others in this special page as of 2015 or today make calls to saveSettings or saveOptions.

The above conversation about this code potentially being redundant sounds interesting. I suggest creating a new task for that.

Nevermind. The original trace is for the saveSettings call from SessionBackend->save(), which still exists indeed. However as per T91820 and the various multi-dc sync meetings, we're planning on routing login.wikimedia.org to primary always.

If someone thinks it is relatively easy to entirely remove the need for pre-send DB writes during CentralAuth GET requests (not just this particular one), that would definitely be interesting though.