Improve REST API error handling

- Specific route handlers don’t need to catch exceptions just to return them in the response. - In case of errors include a json with Mantis error code, message, and localized message for thrown exceptions. - Convert RestFault and throw, though in this case only include the message in json, no code or localized message. LegacyApiFaultException can be removed once all APIs throw proper exceptions instead of returning RestFault. Fixes #23714
mantisbt · Dec 30, 2017 · c0747fc · c0747fc
1 parent 61059a3
commit c0747fc
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 109 deletions.
diff --git a/api/rest/index.php b/api/rest/index.php
@@ -25,6 +25,7 @@
 
 # Bypass default Mantis headers
 $g_bypass_headers = true;
+$g_bypass_error_handler = true;
 
 require_once( __DIR__ . '/../../vendor/autoload.php' );
 require_once( __DIR__ . '/../../core.php' );
@@ -42,13 +43,36 @@
 # For example, this will disable logic like encoding dates with XSD meta-data.
 ApiObjectFactory::$soap = false;
 
-
 # Show SLIM detailed errors according to Mantis settings
 $t_config = array();
 if( ON == config_get_global( 'show_detailed_errors' ) ) {
 	$t_config['settings'] = array( 'displayErrorDetails' => true );
 }
+
 $t_container = new \Slim\Container( $t_config );
+$t_container['errorHandler'] = function( $p_container ) {
+	return function( $p_request, $p_response, $p_exception ) use ( $p_container ) {
+		$t_data = array(
+			'message' => $p_exception->getMessage(),
+		);
+
+		if( is_a( $p_exception, 'Mantis\Exceptions\MantisException' ) ) {
+			global $g_error_parameters;
+			$g_error_parameters =  $p_exception->getParams();
+			$t_data['code'] = $p_exception->getCode();
+			$t_data['localized'] = error_string( $p_exception->getCode() );
+
+			$t_result = ApiObjectFactory::faultFromException( $p_exception );
+			return $p_response->withStatus( $t_result->status_code, $t_result->fault_string )->withJson( $t_data );
+		}
+
+		if( is_a( $p_exception, 'Mantis\Exceptions\LegacyApiFaultException' ) ) {
+			return $p_response->withStatus( $p_exception->getCode(), $p_exception->getMessage() )->withJson( $t_data );
+		}
+
+		return $p_response->withStatus( HTTP_STATUS_INTERNAL_SERVER_ERROR, $p_exception->getMessage() )->withJson( $t_data );
+	};
+};
 
 $g_app = new \Slim\App( $t_container );
 

diff --git a/api/rest/restcore/issues_rest.php b/api/rest/restcore/issues_rest.php
@@ -65,13 +65,9 @@ function rest_issue_get( \Slim\Http\Request $p_request, \Slim\Http\Response $p_r
 
 		# Username and password below are ignored, since middleware already done the auth.
 		$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
+		ApiObjectFactory::throwIfFault( $t_issue );
 
-		if( ApiObjectFactory::isFault( $t_issue ) ) {
-			$t_result = null;
-			$p_response = $p_response->withStatus( $t_issue->status_code, $t_issue->fault_string );
-		} else {
-			$t_result = array( 'issues' => array( $t_issue ) );
-		}
+		$t_result = array( 'issues' => array( $t_issue ) );
 	} else {
 		$t_page_number = $p_request->getParam( 'page', 1 );
 		$t_page_size = $p_request->getParam( 'page_size', 50 );
@@ -125,9 +121,7 @@ function rest_issue_add( \Slim\Http\Request $p_request, \Slim\Http\Response $p_r
 	$t_issue = $p_request->getParsedBody();
 
 	$t_result = mc_issue_add( /* username */ '', /* password */ '', $t_issue );
-	if( ApiObjectFactory::isFault( $t_result ) ) {
-		return $p_response->withStatus( $t_result->status_code, $t_result->fault_string );
-	}
+	ApiObjectFactory::throwIfFault( $t_result );
 
 	$t_issue_id = $t_result;
 
@@ -148,17 +142,10 @@ function rest_issue_add( \Slim\Http\Request $p_request, \Slim\Http\Response $p_r
 function rest_issue_delete( \Slim\Http\Request $p_request, \Slim\Http\Response $p_response, array $p_args ) {
 	$t_issue_id = isset( $p_args['id'] ) ? $p_args['id'] : $p_request->getParam( 'id' );
 
-	$t_found = bug_exists( $t_issue_id );
-	if( $t_found ) {
-		$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
-		if( ApiObjectFactory::isFault( $t_issue ) ) {
-			return $p_response->withStatus( $t_issue->status_code, $t_issue->fault_string );
-		}
+	$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
+	ApiObjectFactory::throwIfFault( $t_issue );
 
-		$t_etag = mc_issue_hash( $t_issue_id, array( 'issues' => array( $t_issue ) ) );
-	} else {
-		$t_etag = mc_issue_hash( $t_issue_id, /* issue */ null );
-	}
+	$t_etag = mc_issue_hash( $t_issue_id, array( 'issues' => array( $t_issue ) ) );
 
 	if( $p_request->hasHeader( HEADER_IF_MATCH ) ) {
 		$t_match_etag = $p_request->getHeaderLine( HEADER_IF_MATCH );
@@ -168,22 +155,12 @@ function rest_issue_delete( \Slim\Http\Request $p_request, \Slim\Http\Response $
 		}
 	}
 
-	if( $t_found ) {
-		# Username and password below are ignored, since middleware already done the auth.
-		$t_result = mc_issue_delete( /* username */ '', /* password */ '', $t_issue_id );
-
-		if( ApiObjectFactory::isFault( $t_result ) ) {
-			return $p_response->withStatus( $t_result->status_code, $t_result->fault_string )
-				->withHeader( HEADER_ETAG, $t_etag );
-		}
-
-		$p_response = $p_response->withStatus( HTTP_STATUS_NO_CONTENT )
-			->withHeader( HEADER_ETAG, mc_issue_hash( $t_issue_id, null ) );
-	} else {
-		$p_response = $p_response->withStatus( HTTP_STATUS_NOT_FOUND, 'Issue not found' );
-	}
+	# Username and password below are ignored, since middleware already done the auth.
+	$t_result = mc_issue_delete( /* username */ '', /* password */ '', $t_issue_id );
+	ApiObjectFactory::throwIfFault( $t_result );
 
-	return $p_response;
+	return $p_response->withStatus( HTTP_STATUS_NO_CONTENT )
+		->withHeader( HEADER_ETAG, mc_issue_hash( $t_issue_id, null ) );
 }
 
 /**
@@ -215,9 +192,7 @@ function rest_issue_note_add( \Slim\Http\Request $p_request, \Slim\Http\Response
 	# TODO: support note attachments
 
 	$t_result = mc_issue_note_add( /* username */ '', /* password */ '', $t_issue_id, $t_note );
-	if( ApiObjectFactory::isFault( $t_result ) ) {
-		return $p_response->withStatus( $t_result->status_code, $t_result->fault_string );
-	}
+	ApiObjectFactory::throwIfFault( $t_result );
 
 	$t_note_id = $t_result;
 
@@ -246,9 +221,7 @@ function rest_issue_note_delete( \Slim\Http\Request $p_request, \Slim\Http\Respo
 	$t_issue_note_id = isset( $p_args['note_id'] ) ? $p_args['note_id'] : $p_request->getParam( 'note_id' );
 
 	$t_result = mc_issue_note_delete( '', '', $t_issue_note_id );
-	if( ApiObjectFactory::isFault( $t_result ) ) {
-		return $p_response->withStatus( $t_result->status_code, $t_result->fault_string );
-	}
+	ApiObjectFactory::throwIfFault( $t_result );
 
 	$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
 	return $p_response->withStatus( HTTP_STATUS_SUCCESS, 'Issue Note Deleted' )->
@@ -270,18 +243,10 @@ function rest_issue_update( \Slim\Http\Request $p_request, \Slim\Http\Response $
 		return $p_response->withStatus( HTTP_STATUS_BAD_REQUEST, $t_message );
 	}
 
-	$t_found = bug_exists( $t_issue_id );
-	if( $t_found ) {
-		$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
-		if( ApiObjectFactory::isFault( $t_issue ) ) {
-			return $p_response->withStatus( $t_issue->status_code, $t_issue->fault_string );
-		}
+	$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
+	ApiObjectFactory::throwIfFault( $t_issue );
 
-		$t_etag = mc_issue_hash( $t_issue_id, array( 'issues' => array( $t_issue ) ) );
-	} else {
-		$t_etag = mc_issue_hash( $t_issue_id, /* issue */ null );
-		$t_issue = null;
-	}
+	$t_etag = mc_issue_hash( $t_issue_id, array( 'issues' => array( $t_issue ) ) );
 
 	if( $p_request->hasHeader( HEADER_IF_MATCH ) ) {
 		$t_match_etag = $p_request->getHeaderLine( HEADER_IF_MATCH );
@@ -291,32 +256,24 @@ function rest_issue_update( \Slim\Http\Request $p_request, \Slim\Http\Response $
 		}
 	}
 
-	if( $t_found ) {
-		# Construct full issue from issue from db + patched info
-		$t_issue_patch = $p_request->getParsedBody();
-		if( isset( $t_issue_patch['id'] ) && $t_issue_patch['id'] != $t_issue_id ) {
-			return $p_response->withStatus( HTTP_STATUS_BAD_REQUEST, 'Issue id mismatch' );
-		}
+	# Construct full issue from issue from db + patched info
+	$t_issue_patch = $p_request->getParsedBody();
+	if( isset( $t_issue_patch['id'] ) && $t_issue_patch['id'] != $t_issue_id ) {
+		return $p_response->withStatus( HTTP_STATUS_BAD_REQUEST, 'Issue id mismatch' );
+	}
 
-		$t_issue = (object)array_merge( $t_issue, $t_issue_patch );
+	$t_issue = (object)array_merge( $t_issue, $t_issue_patch );
 
-		# Trigger the issue update
-		$t_result = mc_issue_update( /* username */ '', /* password */ '', $t_issue_id, $t_issue );
-		if( ApiObjectFactory::isFault( $t_result ) ) {
-			return $p_response->withStatus( $t_result->status_code, $t_result->fault_string );
-		}
+	# Trigger the issue update
+	$t_result = mc_issue_update( /* username */ '', /* password */ '', $t_issue_id, $t_issue );
+	ApiObjectFactory::throwIfFault( $t_result );
 
-		$t_updated_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
-		$t_result = array( 'issues' => array( $t_updated_issue ) );
+	$t_updated_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );
+	$t_result = array( 'issues' => array( $t_updated_issue ) );
 
-		$p_response = $p_response->withStatus( HTTP_STATUS_SUCCESS, "Issue with id $t_issue_id Updated" )
-			->withHeader( HEADER_ETAG, mc_issue_hash( $t_issue_id, $t_result ) )
-			->withJson( $t_result );
-	} else {
-		$p_response = $p_response->withStatus( HTTP_STATUS_NOT_FOUND, 'Issue not found' );
-	}
-
-	return $p_response;
+	return $p_response->withStatus( HTTP_STATUS_SUCCESS, "Issue with id $t_issue_id Updated" )
+		->withHeader( HEADER_ETAG, mc_issue_hash( $t_issue_id, $t_result ) )
+		->withJson( $t_result );
 }
 
 /**
@@ -332,15 +289,10 @@ function rest_issue_monitor_add( \Slim\Http\Request $p_request, \Slim\Http\Respo
 	$t_data = $p_request->getParsedBody();
 	$t_data['issue_id'] = $t_issue_id;
 
-	try {
-		$command = new MonitorAddCommand( $t_data );
-		$command->execute();
+	$command = new MonitorAddCommand( $t_data );
+	$command->execute();
 
-		$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );			
-	} catch ( Exception $e ) {
-		$t_result = ApiObjectFactory::faultFromException( $e );
-		return $p_response->withStatus( $t_result->status_code, $t_result->fault_string );
-	}
+	$t_issue = mc_issue_get( /* username */ '', /* password */ '', $t_issue_id );			
 
 	return $p_response->withStatus( HTTP_STATUS_CREATED, "Users are now monitoring issue $t_issue_id" )->
 		withJson( array( 'issues' => array( $t_issue ) ) );

diff --git a/api/soap/mantisconnect.php b/api/soap/mantisconnect.php
@@ -30,8 +30,10 @@
 # configuration defined in MantisBT.
 $t_mantis_dir = dirname( dirname( dirname( __FILE__ ) ) ) . DIRECTORY_SEPARATOR;
 
-# include Mantis files
+# Overrides for behaviors for core.php and its dependencies
 $g_bypass_headers = true;
+$g_bypass_error_handler = false;
+
 require_once( $t_mantis_dir . 'core.php' );
 
 /**

diff --git a/api/soap/mc_api.php b/api/soap/mc_api.php
@@ -24,7 +24,10 @@
  */
 
 # set up error_handler() as the new default error handling function
-set_error_handler( 'mc_error_handler' );
+global $g_bypass_error_handler;
+if( !$g_bypass_error_handler ) {
+	set_error_handler( 'mc_error_handler' );
+}
 
 /**
  * Webservice APIs
@@ -34,6 +37,8 @@
 
 require_api( 'api_token_api.php' );
 
+use Mantis\Exceptions\LegacyApiFaultException;
+
 /**
  * A class to capture a RestFault
  */
@@ -316,7 +321,7 @@ static function datetimeString($p_timestamp ) {
 
 	/**
 	 * Checks if an object is a SoapFault
-	 * @param mixed $p_maybe_fault Object to check whether a SOAP fault.
+	 * @param mixed $p_maybe_fault Object to check whether it is a SOAP/REST fault.
 	 * @return boolean
 	 */
 	static function isFault( $p_maybe_fault ) {
@@ -334,6 +339,19 @@ static function isFault( $p_maybe_fault ) {
 
 		return false;
 	}
+
+	/**
+	 * Throw if the provided parameter is a SoapFault or RestFault/
+	 *
+	 * @param mixed $p_maybe_fault Object to check whether it is a SOAP/REST fault.
+	 * @return void
+	 * @throws LogacyApiFaultException
+	 */
+	static function throwIfFault( $p_maybe_fault ) {
+		if( ApiObjectFactory::isFault( $p_maybe_fault ) ) {
+			throw new LegacyApiFaultException( $p_maybe_fault->fault_string, $p_maybe_fault->status_code );
+		}
+	}
 }
 
 /**

diff --git a/api/soap/mc_core.php b/api/soap/mc_core.php
@@ -23,28 +23,6 @@
  * @link http://www.mantisbt.org
  */
 
-/**
- * Unhandled exception handler
- *
- * @param Exception|Error $p_exception The exception to handle
- * @return void
- */
-function api_error_exception_handler( $p_exception ) {
-    echo $p_exception->getMessage(), "\n";
-
-    # echo stack without parameters to protect sensitive data
-    $t_count = 0;
-    foreach( $p_exception->getTrace() as $t_frame ) {
-        $t_file = isset( $t_frame['file'] ) ? $t_frame['file'] : '-';
-        $t_line = isset( $t_frame['line'] ) ? $t_frame['line'] : '-';
-        $t_func = isset( $t_frame['function'] ) ? $t_frame['function'] : '-';
-        echo "#$t_count $t_file($t_line): $t_func()\n";
-        $t_count++;
-    }
-}
-
-set_exception_handler( 'api_error_exception_handler' );
-
 # constants and configurations
 $t_current_dir = dirname( __FILE__ ) . '/';
 

diff --git a/core/error_api.php b/core/error_api.php
@@ -48,8 +48,11 @@
 # These can be disabled in config_inc.php, see $g_display_errors
 error_reporting( error_reporting() | E_USER_ERROR | E_USER_WARNING | E_USER_NOTICE | E_USER_DEPRECATED );
 
-set_error_handler( 'error_handler' );
-set_exception_handler( 'error_exception_handler' );
+global $g_bypass_error_handler;
+if( !$g_bypass_error_handler ) {
+	set_error_handler( 'error_handler' );
+	set_exception_handler( 'error_exception_handler' );
+}
 
 $g_exception = null;
 

diff --git a/core/exceptions/LegacyApiFaultException.php b/core/exceptions/LegacyApiFaultException.php
@@ -0,0 +1,39 @@
+<?php
+# MantisBT - A PHP based bugtracking system
+
+# MantisBT is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+#
+# MantisBT is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with MantisBT.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace Mantis\Exceptions;
+
+/**
+ * An exception that is triggered from a RestFault or SoapFault which is the legacy
+ * way to trigger API errors.  This exception class should be removed once all APIs
+ * dependend on exceptions rather than return codes being faults.
+ *
+ * Don't inherit from MantisException since we don't follow the conventions of having
+ * code correspond to Mantis error code, but code here is the http error code.
+ */
+class LegacyApiFaultException extends \Exception {
+    /**
+     * Constructor
+     *
+     * @param string $p_message The internal non-localized error message.
+     * @param integer $p_code The Mantis error code.
+     * @param Throwable $p_previous The inner exception.
+     * @return void
+     */
+	function __construct( $p_message, $p_code, Throwable $p_previous = null ) {
+		parent::__construct( $p_message, $p_code, $p_previous );
+	}
+}