From d0e6051b5ce8a1fa6089d0761ab91e28484f2f2d Mon Sep 17 00:00:00 2001
From: =?utf8?q?Gerg=C5=91=20Tisza?= <gtisza@wikimedia.org>
Date: Sun, 29 May 2016 21:14:28 +0000
Subject: [PATCH] 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.

Also make CreationReasonAuthenticationRequest non-required so that
the list of required form fields is more in sync with that of
pre-AuthManager code.

Bug: T85853
Change-Id: I9d33bd22295758cc532a260b1848616b41d94f12
---
 includes/auth/AuthenticationRequest.php       | 23 ++++++++++++++++++-
 .../CreationReasonAuthenticationRequest.php   |  2 ++
 .../auth/AuthenticationRequestTest.php        | 19 ++++++++-------
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/includes/auth/AuthenticationRequest.php b/includes/auth/AuthenticationRequest.php
index 3c19b87f17..f314849dec 100644
--- a/includes/auth/AuthenticationRequest.php
+++ b/includes/auth/AuthenticationRequest.php
@@ -281,6 +281,21 @@ abstract class AuthenticationRequest {
 	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 @@ abstract class AuthenticationRequest {
 			}
 
 			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/includes/auth/CreationReasonAuthenticationRequest.php b/includes/auth/CreationReasonAuthenticationRequest.php
index 1711aec974..146470ed8f 100644
--- a/includes/auth/CreationReasonAuthenticationRequest.php
+++ b/includes/auth/CreationReasonAuthenticationRequest.php
@@ -10,6 +10,8 @@ class CreationReasonAuthenticationRequest extends AuthenticationRequest {
 	/** @var string Account creation reason (only used when creating for someone else) */
 	public $reason;
 
+	public $required = self::OPTIONAL;
+
 	public function getFieldInfo() {
 		return [
 			'reason' => [
diff --git a/tests/phpunit/includes/auth/AuthenticationRequestTest.php b/tests/phpunit/includes/auth/AuthenticationRequestTest.php
index 84a0ea6dbe..cac031cf06 100644
--- a/tests/phpunit/includes/auth/AuthenticationRequestTest.php
+++ b/tests/phpunit/includes/auth/AuthenticationRequestTest.php
@@ -243,14 +243,6 @@ class AuthenticationRequestTest extends \MediaWikiTestCase {
 
 		$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 @@ class AuthenticationRequestTest extends \MediaWikiTestCase {
 		$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 );
 	}
 
 	/**
-- 
2.20.1