Merge "AuthManager: do not rewrite PRIMARY_REQUIRED to REQUIRED"

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 ),
                ];