Improve test coverage for ApiBase.php

One bug fixed: a timestamp of '00' or similar would get interpreted as
'now' by mistake instead of Unix timestamp 0, without throwing the
warning for using 0 instead of 'now'.  This is because it called
wfTimestamp() once to parse the input date, got a Unix timestamp of 0
back, and then tried passing that 0 back to wfTimestamp again to
reformat as a wiki date, but it got reinterpreted as 'now'.

Also fixed parameters with type "user" to validate usernames more
correctly.  This might be risky, though, if I missed any valid
usernames, or if API clients were for some reason relying on passing in
invalid usernames.  If we don't actually want to do this, we should add
a comment explaining why we're allowing any title without a fragment
rather than validating properly.

Still lots more work to do here.

Change-Id: I56b4290263df8698efdbddda71a7eabd9e303abc

includes/WebRequest.php

@@ -432,7 +432,7 @@ class WebRequest {
 	 * selected by a drop-down menu). For freeform input, see getText().
 	 *
 	 * @param string $name
-	 * @param string $default Optional default (or null)
+	 * @param string|null $default Optional default (or null)
 	 * @return string|null
 	 */
 	public function getVal( $name, $default = null ) {

includes/api/ApiBase.php

@@ -1129,8 +1129,8 @@ abstract class ApiBase extends ContextSource {
 				) {
 					$type = array_merge( $type, $paramSettings[self::PARAM_EXTRA_NAMESPACES] );
 				}
-				// By default, namespace parameters allow ALL_DEFAULT_STRING to be used to specify
+				// Namespace parameters allow ALL_DEFAULT_STRING to be used to
-				// all namespaces.
+				// specify all namespaces irrespective of PARAM_ALL.
 				$allowAll = true;
 			}
 			if ( isset( $value ) && $type == 'submodule' ) {
@@ -1436,22 +1436,15 @@ abstract class ApiBase extends ContextSource {
 				return $value;
 			}
 
-			if ( is_array( $allowedValues ) ) {
+			$values = array_map( function ( $v ) {
-				$values = array_map( function ( $v ) {
+				return '<kbd>' . wfEscapeWikiText( $v ) . '</kbd>';
-					return '<kbd>' . wfEscapeWikiText( $v ) . '</kbd>';
+			}, $allowedValues );
-				}, $allowedValues );
+			$this->dieWithError( [
-				$this->dieWithError( [
+				'apierror-multival-only-one-of',
-					'apierror-multival-only-one-of',
+				$valueName,
-					$valueName,
+				Message::listParam( $values ),
-					Message::listParam( $values ),
+				count( $values ),
-					count( $values ),
+			], "multival_$valueName" );
-				], "multival_$valueName" );
-			} else {
-				$this->dieWithError( [
-					'apierror-multival-only-one',
-					$valueName,
-				], "multival_$valueName" );
-			}
 		}
 
 		if ( is_array( $allowedValues ) ) {
@@ -1537,7 +1530,7 @@ abstract class ApiBase extends ContextSource {
 	}
 
 	/**
-	 * Validate and normalize of parameters of type 'timestamp'
+	 * Validate and normalize parameters of type 'timestamp'
 	 * @param string $value Parameter value
 	 * @param string $encParamName Parameter name
 	 * @return string Validated and normalized parameter
@@ -1559,15 +1552,15 @@ abstract class ApiBase extends ContextSource {
 			return wfTimestamp( TS_MW );
 		}
 
-		$unixTimestamp = wfTimestamp( TS_UNIX, $value );
+		$timestamp = wfTimestamp( TS_MW, $value );
-		if ( $unixTimestamp === false ) {
+		if ( $timestamp === false ) {
 			$this->dieWithError(
 				[ 'apierror-badtimestamp', $encParamName, wfEscapeWikiText( $value ) ],
 				"badtimestamp_{$encParamName}"
 			);
 		}
 
-		return wfTimestamp( TS_MW, $unixTimestamp );
+		return $timestamp;
 	}
 
 	/**
@@ -1609,7 +1602,7 @@ abstract class ApiBase extends ContextSource {
 	}
 
 	/**
-	 * Validate and normalize of parameters of type 'user'
+	 * Validate and normalize parameters of type 'user'
 	 * @param string $value Parameter value
 	 * @param string $encParamName Parameter name
 	 * @return string Validated and normalized parameter
@@ -1619,15 +1612,32 @@ abstract class ApiBase extends ContextSource {
 			return $value;
 		}
 
-		$title = Title::makeTitleSafe( NS_USER, $value );
+		$titleObj = Title::makeTitleSafe( NS_USER, $value );
-		if ( $title === null || $title->hasFragment() ) {
+
+		if ( $titleObj ) {
+			$value = $titleObj->getText();
+		}
+
+		if (
+			!User::isValidUserName( $value ) &&
+			// We allow ranges as well, for blocks.
+			!IP::isIPAddress( $value ) &&
+			// See comment for User::isIP.  We don't just call that function
+			// here because it also returns true for things like
+			// 300.300.300.300 that are neither valid usernames nor valid IP
+			// addresses.
+			!preg_match(
+				'/^' . RE_IP_BYTE . '\.' . RE_IP_BYTE . '\.' . RE_IP_BYTE . '\.xxx$/',
+				$value
+			)
+		) {
 			$this->dieWithError(
 				[ 'apierror-baduser', $encParamName, wfEscapeWikiText( $value ) ],
 				"baduser_{$encParamName}"
 			);
 		}
 
-		return $title->getText();
+		return $value;
 	}
 
 	/**@}*/

includes/api/ApiMain.php

@@ -1720,8 +1720,8 @@ class ApiMain extends ApiBase {
 	/**
 	 * Get a request value, and register the fact that it was used, for logging.
 	 * @param string $name
-	 * @param mixed $default
+	 * @param string|null $default
-	 * @return mixed
+	 * @return string|null
 	 */
 	public function getVal( $name, $default = null ) {
 		$this->mParamsUsed[$name] = true;

includes/api/i18n/en.json

@@ -1736,7 +1736,6 @@
 	"apierror-missingtitle-byname": "The page $1 doesn't exist.",
 	"apierror-moduledisabled": "The <kbd>$1</kbd> module has been disabled.",
 	"apierror-multival-only-one-of": "{{PLURAL:$3|Only|Only one of}} $2 is allowed for parameter <var>$1</var>.",
-	"apierror-multival-only-one": "Only one value is allowed for parameter <var>$1</var>.",
 	"apierror-multpages": "<var>$1</var> may only be used with a single page.",
 	"apierror-mustbeloggedin-changeauth": "You must be logged in to change authentication data.",
 	"apierror-mustbeloggedin-generic": "You must be logged in.",

includes/api/i18n/qqq.json

@@ -1625,7 +1625,6 @@
 	"apierror-missingtitle-byname": "{{doc-apierror}}",
 	"apierror-moduledisabled": "{{doc-apierror}}\n\nParameters:\n* $1 - Name of the module.",
 	"apierror-multival-only-one-of": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.\n* $2 - Possible values for the parameter.\n* $3 - Number of values.",
-	"apierror-multival-only-one": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.",
 	"apierror-multpages": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name",
 	"apierror-mustbeloggedin-changeauth": "{{doc-apierror}}",
 	"apierror-mustbeloggedin-generic": "{{doc-apierror}}",