From 92c29b8891c15c9e7f0cd5e821a819027898b68d Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 19 Oct 2015 14:30:40 +0200 Subject: [PATCH] Allow passing detailed permission errors data to API Using the new system introduced in 1c57794e371d74e1d88748de567a1c3358c3ad2e (see T47843). This change allows Title::getUserPermissionsErrors() to include MessageSpecifiers instead of string message keys in its return value. This doesn't seem to have any bad effects, and should work seamlessly as long as callers aren't trying to do anything stupid and just pass the value to PermissionsError or OutputPage::showPermissionsErrorPage() or wfMessage() or some such. If the callers *are* trying something stupid, nothing worse than duplicated or otherwise less-than-perfect error messages (in code which tries to handle some message keys specially) should happen. (I fixed wfMergeErrorArrays(), but who knows what else lurks in all this code.) Any problems should only affect new-style errors using MessageSpecifier, though. Since MessageSpecifiers tend to be stringable, we probably won't get fatals, but might get incorrect checks. Should we try to log this happening somehow? Goes with I42a0c5b0ea7e61088dd609b764dd7d1396c60cd5 in TitleBlacklist. Bug: T115258 Change-Id: I1334ba21a2862973a9d8ff5be2c9bec06a82698b --- RELEASE-NOTES-1.27 | 3 +++ docs/hooks.txt | 8 ++++++-- includes/GlobalFunctions.php | 7 ++++++- includes/Title.php | 12 +++++++++--- includes/api/ApiBase.php | 26 +++++++++++++++++++++----- includes/api/ApiUpload.php | 4 ++++ 6 files changed, 49 insertions(+), 11 deletions(-) diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27 index 4275921e6a..35fa88763c 100644 --- a/RELEASE-NOTES-1.27 +++ b/RELEASE-NOTES-1.27 @@ -64,6 +64,9 @@ production. setting in the relevant section of $wgLBFactoryConf. * User::newSystemUser() may be used to simplify the creation of passwordless "system" users for logged actions from scripts and extensions. +* Extensions can now return detailed error information via the API when + preventing user actions using 'getUserPermissionsErrors' and similar hooks + by using ApiMessage instances instead of strings for the $result value. ==== External libraries ==== diff --git a/docs/hooks.txt b/docs/hooks.txt index 427f35e773..017416fc61 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -1521,7 +1521,9 @@ $lang: Language that will be used to render the timestamp 'getUserPermissionsErrors': Add a permissions error when permissions errors are checked for. Use instead of userCan for most cases. Return false if the user can't do it, and populate $result with the reason in the form of -array( messagename, param1, param2, ... ). For consistency, error messages +array( messagename, param1, param2, ... ) or a MessageSpecifier instance (you +might want to use ApiMessage to provide machine-readable details for the API). +For consistency, error messages should be plain text with no special coloring, bolding, etc. to show that they're errors; presenting them properly to the user as errors is done by the caller. @@ -1534,7 +1536,9 @@ $result: User permissions error to add. If none, return true. called only if expensive checks are enabled. Add a permissions error when permissions errors are checked for. Return false if the user can't do it, and populate $result with the reason in the form of array( messagename, param1, -param2, ... ). For consistency, error messages should be plain text with no +param2, ... ) or a MessageSpecifier instance (you might want to use ApiMessage +to provide machine-readable details for the API). For consistency, error +messages should be plain text with no special coloring, bolding, etc. to show that they're errors; presenting them properly to the user as errors is done by the caller. $title: Title object being checked against diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 791bf33667..7c572078e5 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -308,10 +308,15 @@ function wfMergeErrorArrays( /*...*/ ) { $out = array(); foreach ( $args as $errors ) { foreach ( $errors as $params ) { + $originalParams = $params; + if ( $params[0] instanceof MessageSpecifier ) { + $msg = $params[0]; + $params = array_merge( array( $msg->getKey() ), $msg->getParams() ); + } # @todo FIXME: Sometimes get nested arrays for $params, # which leads to E_NOTICEs $spec = implode( "\t", $params ); - $out[$spec] = $params; + $out[$spec] = $originalParams; } } return array_values( $out ); diff --git a/includes/Title.php b/includes/Title.php index 9ada4f314d..13e7ffbe2d 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -1944,7 +1944,7 @@ class Title { * - secure : does cheap and expensive checks, using the master as needed * @param array $ignoreErrors Array of Strings Set this to a list of message keys * whose corresponding errors may be ignored. - * @return array Array of arguments to wfMessage to explain permissions problems. + * @return array Array of arrays of the arguments to wfMessage to explain permissions problems. */ public function getUserPermissionsErrors( $action, $user, $rigor = 'secure', $ignoreErrors = array() @@ -1953,9 +1953,12 @@ class Title { // Remove the errors being ignored. foreach ( $errors as $index => $error ) { - $error_key = is_array( $error ) ? $error[0] : $error; + $errKey = is_array( $error ) ? $error[0] : $error; - if ( in_array( $error_key, $ignoreErrors ) ) { + if ( in_array( $errKey, $ignoreErrors ) ) { + unset( $errors[$index] ); + } + if ( $errKey instanceof MessageSpecifier && in_array( $errKey->getKey(), $ignoreErrors ) ) { unset( $errors[$index] ); } } @@ -2054,6 +2057,9 @@ class Title { } elseif ( $result !== '' && is_string( $result ) ) { // A string representing a message-id $errors[] = array( $result ); + } elseif ( $result instanceof MessageSpecifier ) { + // A message specifier representing an error + $errors[] = array( $result ); } elseif ( $result === false ) { // a generic "We don't want them to do that" $errors[] = array( 'badaccess-group0' ); diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 1465543a48..8af08a5061 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1375,10 +1375,11 @@ abstract class ApiBase extends ContextSource { * * @since 1.23 * @param Status $status + * @param array|null &$extraData Set if extra data from IApiMessage is available (since 1.27) * @return array Array of code and error string * @throws MWException */ - public function getErrorFromStatus( $status ) { + public function getErrorFromStatus( $status, &$extraData = null ) { if ( $status->isGood() ) { throw new MWException( 'Successful status passed to ApiBase::dieStatus' ); } @@ -1397,7 +1398,12 @@ abstract class ApiBase extends ContextSource { // error messages. if ( $errors[0] instanceof Message ) { $msg = $errors[0]; - $code = $msg->getKey(); + if ( $msg instanceof IApiMessage ) { + $extraData = $msg->getApiData(); + $code = $msg->getApiCode(); + } else { + $code = $msg->getKey(); + } } else { $code = array_shift( $errors[0] ); $msg = wfMessage( $code, $errors[0] ); @@ -1418,8 +1424,9 @@ abstract class ApiBase extends ContextSource { * @throws UsageException always */ public function dieStatus( $status ) { - list( $code, $msg ) = $this->getErrorFromStatus( $status ); - $this->dieUsage( $msg, $code ); + $extraData = null; + list( $code, $msg ) = $this->getErrorFromStatus( $status, $extraData ); + $this->dieUsage( $msg, $code, 0, $extraData ); } // @codingStandardsIgnoreStart Allow long lines. Cannot split these. @@ -1952,7 +1959,8 @@ abstract class ApiBase extends ContextSource { $error = array( $error ); } $parsed = $this->parseMsg( $error ); - $this->dieUsage( $parsed['info'], $parsed['code'] ); + $extraData = isset( $parsed['data'] ) ? $parsed['data'] : null; + $this->dieUsage( $parsed['info'], $parsed['code'], 0, $extraData ); } /** @@ -2005,6 +2013,14 @@ abstract class ApiBase extends ContextSource { $key = array_shift( $error ); } + if ( $key instanceof IApiMessage ) { + return array( + 'code' => $key->getApiCode(), + 'info' => $key->inLanguage( 'en' )->useDatabase( false )->text(), + 'data' => $key->getApiData() + ); + } + if ( isset( self::$messageMap[$key] ) ) { return array( 'code' => wfMsgReplaceArgs( self::$messageMap[$key]['code'], $error ), diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 5e13e98f11..b456b0d9d8 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -363,6 +363,10 @@ class ApiUpload extends ApiBase { $data['invalidparameter'] = $parameter; $parsed = $this->parseMsg( $error ); + if ( isset( $parsed['data'] ) ) { + $data = array_merge( $data, $parsed['data'] ); + } + $this->dieUsage( $parsed['info'], $parsed['code'], 0, $data ); } -- 2.20.1