From d4de10750fbb59a4580703fe928aac586187d46f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Thu, 4 Aug 2016 20:47:57 +0000 Subject: [PATCH] AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED AuthManager::getAuthenticationRequests() changes AuthenticationRequest::$required from REQUIRED to PRIMARY_REQUIRED if the request is from a primary; it made an exception when all primary providers returned a given request. That exception is not particularly useful (AuthenticationRequest::mergeFieldInfo() used to rely on it to determine which fields are required, but since I9d33bd2 that's not really needed), and knowing which request is from a primary is useful for other means. This changes required field semantics in a corner case: when a primary provider returns two required requests, the previous behavior was to assume that they are both required; the new one is to treat them as alternatives (as if they were returned by two different providers). So when all primary providers return request X, and one of them returns Y in addition, the fields of X will not be marked required, while previously that would have been the case. Instead of overcomplicating the interface for something that is unlikely to come up in any real use case, add a new requirement to PrimaryAuthenticationProvider that it should not return multiple required requests. Bug: T141471 Change-Id: I1c1f44d4d6b66f77c876e3459fb97f03483db744 --- RELEASE-NOTES-1.28 | 3 +++ includes/auth/AuthManager.php | 23 +++++-------------- includes/auth/AuthenticationRequest.php | 3 ++- .../auth/PrimaryAuthenticationProvider.php | 8 +++++++ .../phpunit/includes/auth/AuthManagerTest.php | 8 +++---- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28 index 48fe4ff211..f6c3530316 100644 --- a/RELEASE-NOTES-1.28 +++ b/RELEASE-NOTES-1.28 @@ -108,6 +108,9 @@ changes to languages because of Phabricator reports. Use ...->stashFile()->getFileKey() instead. * "Public domain" was removed as a wiki license option from the installer, in favour of CC-0. +* AuthenticationRequest::$required is now changed from REQUIRED to PRIMARY_REQUIRED + on requests needed by primary providers even if all primaries need them. + Primary providers are discouraged from returning multiple REQUIRED requests. == Compatibility == diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php index 50e370e9e6..b8c536ebd1 100644 --- a/includes/auth/AuthManager.php +++ b/includes/auth/AuthManager.php @@ -2026,37 +2026,26 @@ class AuthManager implements LoggerAwareInterface { // Query them and merge results $reqs = []; - $allPrimaryRequired = null; foreach ( $providers as $provider ) { $isPrimary = $provider instanceof PrimaryAuthenticationProvider; - $thisRequired = []; foreach ( $provider->getAuthenticationRequests( $providerAction, $options ) as $req ) { $id = $req->getUniqueId(); - // If it's from a Primary, mark it as "primary-required" but - // track it for later. + // If a required request if from a Primary, mark it as "primary-required" instead if ( $isPrimary ) { if ( $req->required ) { - $thisRequired[$id] = true; $req->required = AuthenticationRequest::PRIMARY_REQUIRED; } } - if ( !isset( $reqs[$id] ) || $req->required === AuthenticationRequest::REQUIRED ) { + if ( + !isset( $reqs[$id] ) + || $req->required === AuthenticationRequest::REQUIRED + || $reqs[$id] === AuthenticationRequest::OPTIONAL + ) { $reqs[$id] = $req; } } - - // Track which requests are required by all primaries - if ( $isPrimary ) { - $allPrimaryRequired = $allPrimaryRequired === null - ? $thisRequired - : array_intersect_key( $allPrimaryRequired, $thisRequired ); - } - } - // Any requests that were required by all primaries are required. - foreach ( (array)$allPrimaryRequired as $id => $dummy ) { - $reqs[$id]->required = AuthenticationRequest::REQUIRED; } // AuthManager has its own req for some actions diff --git a/includes/auth/AuthenticationRequest.php b/includes/auth/AuthenticationRequest.php index ff4d52ed92..f6f949ec7c 100644 --- a/includes/auth/AuthenticationRequest.php +++ b/includes/auth/AuthenticationRequest.php @@ -43,7 +43,8 @@ abstract class AuthenticationRequest { const REQUIRED = 1; /** Indicates that the request is required by a primary authentication - * provdier, but other primary authentication providers do not require it. */ + * provdier. Since the user can choose which primary to authenticate with, + * the request might or might not end up being actually required. */ const PRIMARY_REQUIRED = 2; /** @var string|null The AuthManager::ACTION_* constant this request was diff --git a/includes/auth/PrimaryAuthenticationProvider.php b/includes/auth/PrimaryAuthenticationProvider.php index c44c8fc1c9..35f3287bd4 100644 --- a/includes/auth/PrimaryAuthenticationProvider.php +++ b/includes/auth/PrimaryAuthenticationProvider.php @@ -57,6 +57,14 @@ interface PrimaryAuthenticationProvider extends AuthenticationProvider { /** Provider cannot create or link to accounts */ const TYPE_NONE = 'none'; + /** + * {@inheritdoc} + * + * Of the requests returned by this method, exactly one should have + * {@link AuthenticationRequest::$required} set to REQUIRED. + */ + public function getAuthenticationRequests( $action, array $options ); + /** * Start an authentication flow * diff --git a/tests/phpunit/includes/auth/AuthManagerTest.php b/tests/phpunit/includes/auth/AuthManagerTest.php index 99b9029bc3..788d304295 100644 --- a/tests/phpunit/includes/auth/AuthManagerTest.php +++ b/tests/phpunit/includes/auth/AuthManagerTest.php @@ -3087,7 +3087,7 @@ class AuthManagerTest extends \MediaWikiTestCase { $actual = $this->manager->getAuthenticationRequests( AuthManager::ACTION_LOGIN ); $expected = [ $rememberReq, - $makeReq( "primary-shared", AuthenticationRequest::REQUIRED ), + $makeReq( "primary-shared", AuthenticationRequest::PRIMARY_REQUIRED ), $makeReq( "required", AuthenticationRequest::PRIMARY_REQUIRED ), $makeReq( "required2", AuthenticationRequest::PRIMARY_REQUIRED ), $makeReq( "optional", AuthenticationRequest::OPTIONAL ), @@ -3107,10 +3107,10 @@ class AuthManagerTest extends \MediaWikiTestCase { $actual = $this->manager->getAuthenticationRequests( AuthManager::ACTION_LOGIN ); $expected = [ $rememberReq, - $makeReq( "primary-shared", AuthenticationRequest::REQUIRED ), - $makeReq( "required", AuthenticationRequest::REQUIRED ), + $makeReq( "primary-shared", AuthenticationRequest::PRIMARY_REQUIRED ), + $makeReq( "required", AuthenticationRequest::PRIMARY_REQUIRED ), $makeReq( "optional", AuthenticationRequest::OPTIONAL ), - $makeReq( "foo", AuthenticationRequest::REQUIRED ), + $makeReq( "foo", AuthenticationRequest::PRIMARY_REQUIRED ), $makeReq( "bar", AuthenticationRequest::REQUIRED ), $makeReq( "baz", AuthenticationRequest::REQUIRED ), ]; -- 2.20.1