Page MenuHomePhabricator

AuthenticationProvider::postAuthentication called unexpectedly after autologin
Closed, ResolvedPublic

Description

When user account creation returns a passing response with $loginRequest set (which is AuthManager's way of logging in freshly registered users) and that request is passed in to beginAuthentication, all provider methods will be skipped, except postAuthentication. That can result in unexpected behavior, e.g. if the provider sets something up in testForAuthentication and expects it to be there in postAuthentication (that's the source of the throttler data not found for {user} logspam). Either the phpdoc should warn about this or there should be an extra argument to postAuthentication which tells that setup was skipped.

Event Timeline

Tgr created this task.Aug 4 2016, 11:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 4 2016, 11:20 PM
Tgr added a comment.Aug 4 2016, 11:20 PM

By the way, is it intentional that only preauth providers have postAuthentication? It feels asymmetric.

Either the phpdoc should warn about this

That's what I'd suggest.

By the way, is it intentional that only preauth providers have postAuthentication? It feels asymmetric.

Err, Primary and Secondary providers to have it, and it's called on all three. Why did you think they didn't?

Tgr added a comment.Aug 15 2016, 10:53 PM

I probably meant testForAuthentication.

Here is a Venn diagram of the methods:

array (
  'pre-only' => 
  array (
    9 => 'testForAccountLink',
    10 => 'testForAuthentication',
  ),
  'pri-only' => 
  array (
    0 => 'accountCreationType',
    2 => 'beginPrimaryAccountCreation',
    3 => 'beginPrimaryAccountLink',
    4 => 'beginPrimaryAuthentication',
    5 => 'continuePrimaryAccountCreation',
    6 => 'continuePrimaryAccountLink',
    7 => 'continuePrimaryAuthentication',
    8 => 'finishAccountCreation',
    17 => 'providerNormalizeUsername',
    23 => 'testUserCanAuthenticate',
    24 => 'testUserExists',
  ),
  'sec-only' => 
  array (
    1 => 'beginSecondaryAccountCreation',
    2 => 'beginSecondaryAuthentication',
    3 => 'continueSecondaryAccountCreation',
    4 => 'continueSecondaryAuthentication',
  ),
  'pre/pri' => 
  array (
    3 => 'postAccountLink',
  ),
  'pri/sec' => 
  array (
    0 => 'autoCreatedAccount',
    9 => 'providerAllowsAuthenticationDataChange',
    10 => 'providerAllowsPropertyChange',
    11 => 'providerChangeAuthenticationData',
    12 => 'providerRevokeAccessForUser',
  ),
  'pre/sec' => 
  array (
  ),
  'all' => 
  array (
    5 => 'getAuthenticationRequests',
    6 => 'getUniqueId',
    7 => 'postAccountCreation',
    8 => 'postAuthentication',
    13 => 'setConfig',
    14 => 'setLogger',
    15 => 'setManager',
    16 => 'testForAccountCreation',
    17 => 'testUserForCreation',
  ),
)

The two pre-only ones feel strange (esp. with testForAccountCreation available to all types). Also that testUserCanAuthenticate is primary-only and testUserForCreation is available to all, although they are conceptually similar.

testForAccountCreation is needed so primaries/secondaries can reject the attempt before some primary creates the account. That's not needed for testForAuthentication since the primary or secondary can just fail it in begin(Primary|Secondary)Authentication; the only hole there is that a primary can't prevent some earlier primary from authenticating first, but that seems like an unlikely situation versus using a pre-auth or secondary to do the needed check.

Secondaries have nothing to do with linking, so that's why they don't get testForAccountLink. Primaries aren't likely to need to fail other primaries' linking attempts, so that's why they don't get it. We could always add it when we find something that really does need it.

testUserCanAuthenticate is intended to back User::addSystemUser, returning true if any primary could pass authentication for the name. Since pre-auth and secondaries can't actually authenticate a user (only prevent authentication), they don't have anything to contribute. testUserForCreation, on the other hand, is intended to check whether anything prevents the creation of that user, so every provider needs to be asked.

Change 307051 had a related patch set uploaded (by Gergő Tisza):
Expand SessionManager / AuthManager documentation

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

Change 307051 merged by jenkins-bot:
Expand SessionManager / AuthManager documentation

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

Tgr closed this task as Resolved.Aug 30 2016, 7:17 PM
Tgr claimed this task.