From db521e557482c2bf955447439dc34242b7a3e201 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 1 Jun 2016 11:58:44 -0400 Subject: [PATCH] AuthManager: Ensure neededRequests have action and username set properly They were coming out as null instead, which screws up when requests are changing their fields based on the action. Change-Id: Ic8caf57ebad35c3eb17d45f9d96c6de5b559a83a --- includes/auth/AuthManager.php | 31 +++++++++++++++---- .../phpunit/includes/auth/AuthManagerTest.php | 13 ++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php index 69f51b899f..0a4bd68230 100644 --- a/includes/auth/AuthManager.php +++ b/includes/auth/AuthManager.php @@ -442,6 +442,7 @@ class AuthManager implements LoggerAwareInterface { case AuthenticationResponse::REDIRECT; case AuthenticationResponse::UI; $this->logger->debug( "Primary login with $id returned $res->status" ); + $this->fillRequests( $res->neededRequests, self::ACTION_LOGIN, $guessUserName ); $state['primary'] = $id; $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::authnState', $state ); @@ -504,6 +505,7 @@ class AuthManager implements LoggerAwareInterface { case AuthenticationResponse::REDIRECT; case AuthenticationResponse::UI; $this->logger->debug( "Primary login with $id returned $res->status" ); + $this->fillRequests( $res->neededRequests, self::ACTION_LOGIN, $guessUserName ); $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::authnState', $state ); return $res; @@ -556,6 +558,7 @@ class AuthManager implements LoggerAwareInterface { ); $ret->neededRequests[] = $ret->createRequest; } + $this->fillRequests( $ret->neededRequests, self::ACTION_LOGIN, null ); $session->setSecret( 'AuthManager::authnState', [ 'reqs' => [], // Will be filled in later 'primary' => null, @@ -625,6 +628,7 @@ class AuthManager implements LoggerAwareInterface { case AuthenticationResponse::REDIRECT; case AuthenticationResponse::UI; $this->logger->debug( "Secondary login with $id returned " . $res->status ); + $this->fillRequests( $res->neededRequests, self::ACTION_LOGIN, $user->getName() ); $state['secondary'][$id] = false; $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::authnState', $state ); @@ -1259,6 +1263,7 @@ class AuthManager implements LoggerAwareInterface { 'user' => $user->getName(), 'creator' => $creator->getName(), ] ); + $this->fillRequests( $res->neededRequests, self::ACTION_CREATE, null ); $state['primary'] = $id; $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::accountCreationState', $state ); @@ -1321,6 +1326,7 @@ class AuthManager implements LoggerAwareInterface { 'user' => $user->getName(), 'creator' => $creator->getName(), ] ); + $this->fillRequests( $res->neededRequests, self::ACTION_CREATE, null ); $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::accountCreationState', $state ); return $res; @@ -1416,6 +1422,7 @@ class AuthManager implements LoggerAwareInterface { 'user' => $user->getName(), 'creator' => $creator->getName(), ] ); + $this->fillRequests( $res->neededRequests, self::ACTION_CREATE, null ); $state['secondary'][$id] = false; $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::accountCreationState', $state ); @@ -1784,6 +1791,7 @@ class AuthManager implements LoggerAwareInterface { $this->logger->debug( __METHOD__ . ": Account linking $res->status by $id", [ 'user' => $user->getName(), ] ); + $this->fillRequests( $res->neededRequests, self::ACTION_LINK, $user->getName() ); $state['primary'] = $id; $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::accountLinkState', $state ); @@ -1886,6 +1894,7 @@ class AuthManager implements LoggerAwareInterface { $this->logger->debug( __METHOD__ . ": Account linking $res->status by $id", [ 'user' => $user->getName(), ] ); + $this->fillRequests( $res->neededRequests, self::ACTION_LINK, $user->getName() ); $state['continueRequests'] = $res->neededRequests; $session->setSecret( 'AuthManager::accountLinkState', $state ); return $res; @@ -2047,12 +2056,7 @@ class AuthManager implements LoggerAwareInterface { } // Fill in reqs data - foreach ( $reqs as $req ) { - $req->action = $providerAction; - if ( $req->username === null ) { - $req->username = $options['username']; - } - } + $this->fillRequests( $reqs, $providerAction, $options['username'] ); // For self::ACTION_CHANGE, filter out any that something else *doesn't* allow changing if ( $providerAction === self::ACTION_CHANGE || $providerAction === self::ACTION_REMOVE ) { @@ -2064,6 +2068,21 @@ class AuthManager implements LoggerAwareInterface { return array_values( $reqs ); } + /** + * Set values in an array of requests + * @param AuthenticationRequest[] &$reqs + * @param string $action + * @param string|null $username + */ + private function fillRequests( array &$reqs, $action, $username ) { + foreach ( $reqs as $req ) { + $req->action = $action; + if ( $req->username === null ) { + $req->username = $username; + } + } + } + /** * Determine whether a username exists * @param string $username diff --git a/tests/phpunit/includes/auth/AuthManagerTest.php b/tests/phpunit/includes/auth/AuthManagerTest.php index e681be0957..82608b0c48 100644 --- a/tests/phpunit/includes/auth/AuthManagerTest.php +++ b/tests/phpunit/includes/auth/AuthManagerTest.php @@ -1059,6 +1059,10 @@ class AuthManagerTest extends \MediaWikiTestCase { } else { $this->assertNotNull( $session->getSecret( 'AuthManager::authnState' ), "Response $i, session state" ); + foreach ( $ret->neededRequests as $neededReq ) { + $this->assertEquals( AuthManager::ACTION_LOGIN, $neededReq->action, + "Response $i, neededRequest action" ); + } $this->assertEquals( $ret->neededRequests, $this->manager->getAuthenticationRequests( AuthManager::ACTION_LOGIN_CONTINUE ), @@ -1114,6 +1118,7 @@ class AuthManagerTest extends \MediaWikiTestCase { $restartResponse2->createRequest = new CreateFromLoginAuthenticationRequest( null, [ $req->getUniqueId() => $req ] ); + $restartResponse2->createRequest->action = AuthManager::ACTION_LOGIN; $restartResponse2->neededRequests = [ $rememberReq, $restartResponse2->createRequest ]; return [ @@ -2102,6 +2107,10 @@ class AuthManagerTest extends \MediaWikiTestCase { $this->request->getSession()->getSecret( 'AuthManager::accountCreationState' ), "Response $i, session state" ); + foreach ( $ret->neededRequests as $neededReq ) { + $this->assertEquals( AuthManager::ACTION_CREATE, $neededReq->action, + "Response $i, neededRequest action" ); + } $this->assertEquals( $ret->neededRequests, $this->manager->getAuthenticationRequests( AuthManager::ACTION_CREATE_CONTINUE ), @@ -3525,6 +3534,10 @@ class AuthManagerTest extends \MediaWikiTestCase { $this->request->getSession()->getSecret( 'AuthManager::accountLinkState' ), "Response $i, session state" ); + foreach ( $ret->neededRequests as $neededReq ) { + $this->assertEquals( AuthManager::ACTION_LINK, $neededReq->action, + "Response $i, neededRequest action" ); + } $this->assertEquals( $ret->neededRequests, $this->manager->getAuthenticationRequests( AuthManager::ACTION_LINK_CONTINUE ), -- 2.20.1