Gergő Tisza has uploaded a new change for review.

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

Change subject: Fix required field calculation in AuthenticationRequest
......................................................................

Fix required field calculation in AuthenticationRequest

Instead of only flagging fields which are required by a request
needed by all primairy providers, it should be enough if all
requests needed by some primary provider require that field.

Bug: T85853
Change-Id: I9d33bd22295758cc532a260b1848616b41d94f12
---
M includes/auth/AuthenticationRequest.php
M tests/phpunit/includes/auth/AuthenticationRequestTest.php
2 files changed, 33 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core 
refs/changes/78/291678/1

diff --git a/includes/auth/AuthenticationRequest.php 
b/includes/auth/AuthenticationRequest.php
index 3c19b87..f314849 100644
--- a/includes/auth/AuthenticationRequest.php
+++ b/includes/auth/AuthenticationRequest.php
@@ -281,6 +281,21 @@
        public static function mergeFieldInfo( array $reqs ) {
                $merged = [];
 
+               // fields that are required by some primary providers but not 
others are not actually required
+               $primaryRequests = array_filter( $reqs, function ( $req ) {
+                       return $req->required === 
AuthenticationRequest::PRIMARY_REQUIRED;
+               } );
+               $sharedRequiredPrimaryFields = array_reduce( $primaryRequests, 
function ( $shared, $req ) {
+                       $required = array_keys( array_filter( 
$req->getFieldInfo(), function ( $options ) {
+                               return empty( $options['optional'] );
+                       } ) );
+                       if ( $shared === null ) {
+                               return $required;
+                       } else {
+                               return array_intersect( $shared, $required );
+                       }
+               }, null );
+
                foreach ( $reqs as $req ) {
                        $info = $req->getFieldInfo();
                        if ( !$info ) {
@@ -288,8 +303,14 @@
                        }
 
                        foreach ( $info as $name => $options ) {
-                               if ( $req->required !== self::REQUIRED ) {
+                               if (
                                        // If the request isn't required, its 
fields aren't required either.
+                                       $req->required === self::OPTIONAL
+                                       // If there is a primary not requiring 
this field, no matter how many others do,
+                                       // authentication can proceed without 
it.
+                                       || $req->required === 
self::PRIMARY_REQUIRED
+                                               && !in_array( $name, 
$sharedRequiredPrimaryFields, true )
+                               ) {
                                        $options['optional'] = true;
                                } else {
                                        $options['optional'] = !empty( 
$options['optional'] );
diff --git a/tests/phpunit/includes/auth/AuthenticationRequestTest.php 
b/tests/phpunit/includes/auth/AuthenticationRequestTest.php
index 84a0ea6..cac031c 100644
--- a/tests/phpunit/includes/auth/AuthenticationRequestTest.php
+++ b/tests/phpunit/includes/auth/AuthenticationRequestTest.php
@@ -243,14 +243,6 @@
 
                $req1->required = AuthenticationRequest::PRIMARY_REQUIRED;
 
-               $fields = AuthenticationRequest::mergeFieldInfo( [ $req1 ] );
-               $expect = $req1->getFieldInfo();
-               foreach ( $expect as $name => &$options ) {
-                       $options['optional'] = true;
-               }
-               unset( $options );
-               $this->assertEquals( $expect, $fields );
-
                $fields = AuthenticationRequest::mergeFieldInfo( [ $req1, $req2 
] );
                $expect += $req2->getFieldInfo();
                $expect['string1']['optional'] = false;
@@ -258,6 +250,17 @@
                $expect['select']['optional'] = false;
                $expect['select']['options']['bar'] = $msg;
                $this->assertEquals( $expect, $fields );
+
+               $req2->required = AuthenticationRequest::PRIMARY_REQUIRED;
+
+               $fields = AuthenticationRequest::mergeFieldInfo( [ $req1, $req2 
] );
+               $expect = $req1->getFieldInfo() + $req2->getFieldInfo();
+               $expect['string1']['optional'] = false;
+               $expect['string2']['optional'] = true;
+               $expect['string3']['optional'] = true;
+               $expect['select']['optional'] = false;
+               $expect['select']['options']['bar'] = $msg;
+               $this->assertEquals( $expect, $fields );
        }
 
        /**

-- 
To view, visit https://gerrit.wikimedia.org/r/291678
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9d33bd22295758cc532a260b1848616b41d94f12
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Gergő Tisza <gti...@wikimedia.org>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to