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

diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index 48fe4ff..f6c3530 100644 (file)
--- 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 50e370e..b8c536e 100644 (file)
--- 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 ff4d52e..f6f949e 100644 (file)
--- 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 c44c8fc..35f3287 100644 (file)
--- 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 99b9029..788d304 100644 (file)
--- 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 ),
                ];