Feature/8316 improve callback verification

datamodel.combodo-webhook-integration.xml

@@ -852,22 +852,9 @@
 			/** @var \DBObject $oTriggeringObject */
 			$oTriggeringObject = $aContextArgs['this->object()'];
 
-			// Check if callback is on the object itself
-			if(stripos($sCallbackFQCN, '$this->') !== false)
-			{
-				$sMethodName = str_ireplace('$this->', '', $sCallbackFQCN);
-				$payload = $oTriggeringObject->$sMethodName($aContextArgs, $oLog, $this);
-			}
-			// Otherwise, check if callback is callable as a static method
-			elseif(is_callable($sCallbackFQCN))
-			{
-				$payload = call_user_func($sCallbackFQCN, $oTriggeringObject, $aContextArgs, $oLog, $this);
-			}
-			// Otherwise, there is a problem
-			else
-			{
-				throw new Exception('Prepare payload callback is not callable ('.$sCallbackFQCN.')');
-			}
+			$oCallBack = new CallbackService($sCallbackFQCN);
+			$oCallBack->CheckCallbackSignature(get_class($oTriggeringObject), ['array', EventNotification::class, ActionWebhook::class]);
+			$oCallBack->Invoke($oTriggeringObject, [$aContextArgs, $oLog, $this]);
 		}
 
 		return $payload;

src/Core/Notification/Action/_ActionWebhook.php

@@ -6,11 +6,14 @@
 use ApplicationContext;
 use Combodo\iTop\Core\Notification\Action\Webhook\Exception\WebhookInvalidJsonValueException;
 use Combodo\iTop\Core\WebResponse;
+use Combodo\iTop\Service\CallbackService;
 use Combodo\iTop\Service\WebRequestSender;
+use DBObject;
 use EventWebhook;
 use Exception;
 use IssueLog;
 use MetaModel;
+use ReflectionException;
 use UserRights;
 use utils;
 
@@ -30,40 +33,29 @@ abstract class _ActionWebhook extends ActionNotification
 	 *
 	 * @throws \ArchivedObjectException
 	 * @throws \CoreException
+	 * @throws ReflectionException
+	 * @throws \Exception
 	 */
 	public static function ExecuteResponseHandler(WebResponse $oResponse, array $aParams)
 	{
 		// Retrieve objects from params
-		if (! array_key_exists('oTriggeringObject', $aParams) || ! array_key_exists('oActionWebhook', $aParams)) {
+		if (!array_key_exists('oTriggeringObject', $aParams) || !array_key_exists('oActionWebhook', $aParams)) {
 			IssueLog::Error('Missing parameters in response handler. Expecting at least oTriggeringObject and oActionWebhook', 'console', [
 				'oResponse' => $oResponse,
 				'aParams' => $aParams,
 			]);
 			throw new Exception('Missing parameters in response handler. See error log for details.');
 		}
-		/** @var \DBObject $oTriggeringObject */
-        $oTriggeringObject = is_object($aParams['oTriggeringObject']) ?
-                            $aParams['oTriggeringObject'] :
-                            MetaModel::GetObject($aParams['oTriggeringObject']['class'], $aParams['oTriggeringObject']['id'], true, true);
+		/** @var DBObject $oTriggeringObject */
+		$oTriggeringObject = is_object($aParams['oTriggeringObject']) ?
+			$aParams['oTriggeringObject'] :
+			MetaModel::GetObject($aParams['oTriggeringObject']['class'], $aParams['oTriggeringObject']['id'], true, true);
 		$oActionWebhook = MetaModel::GetObject($aParams['oActionWebhook']['class'], $aParams['oActionWebhook']['id'], true, true);
+        $sResponseCallback = $oActionWebhook->Get('process_response_callback');
 
-		// Check if callback is on the object itself
-		$sResponseCallback = $oActionWebhook->Get('process_response_callback');
-		if(stripos($sResponseCallback, '$this->') !== false)
-		{
-			$sMethodName = str_ireplace('$this->', '', $sResponseCallback);
-			$oTriggeringObject->$sMethodName($oResponse, $oActionWebhook);
-		}
-		// Otherwise, check if callback is callable as a static method
-		elseif(is_callable($sResponseCallback))
-		{
-			call_user_func($sResponseCallback, $oTriggeringObject, $oResponse, $oActionWebhook);
-		}
-		// Otherwise, there is a problem, we cannot call the callback
-		elseif(empty($sResponseCallback) === false)
-		{
-			throw new Exception('Process response callback is not callable ('.$sResponseCallback.')');
-		}
+        $oCallBack = new CallbackService($sResponseCallback);
+        $oCallBack->CheckCallbackSignature(get_class($oTriggeringObject), [WebResponse::class, \ActionWebhook::class]);
+        $oCallBack->Invoke($oTriggeringObject, [$oResponse, $oActionWebhook]);
 	}
 
 	/**

src/Service/CallbackService.php

@@ -0,0 +1,109 @@
+<?php
+
+namespace Combodo\iTop\Service;
+
+use DBObject;
+use Exception;
+use IssueLog;
+use ReflectionClass;
+use ReflectionException;
+use SecurityException;
+
+/**
+ * A service that will perform checks on the signature of a method.
+ * Usage:
+ * $callbackService = new CallbackService('ClassName::methodName');
+ * $callbackService->CheckCallbackSignature(get_class($oTriggeringObject), [DBObject::class, 'string', 'int']);
+ * $callbackService->Invoke($oTriggeringObject, ['stringValue', 42]);
+ *
+ */
+class CallbackService
+{
+	private string $sCallBackClassName;
+	private string $sCallBackMethodName;
+	private bool $bIsStatic;
+
+	/**
+	 * @throws Exception
+	 */
+	public function __construct(string $sCallBackDefinition)
+	{
+		if (stripos($sCallBackDefinition, '$this->') !== false) {
+			$this->sCallBackMethodName = str_ireplace('$this->', '', $sCallBackDefinition);
+			$this->bIsStatic = false;
+			$this->sCallBackClassName = ''; // we don't need it for non-static methods
+		} else {
+			if (!is_callable($sCallBackDefinition)) {
+				$sMessageError = "The callback '$sCallBackDefinition' is not valid.";
+				IssueLog::Error($sMessageError, 'console');
+				throw new Exception($sMessageError);
+			}
+			$iPos = strrpos($sCallBackDefinition, '::');
+			if ($iPos !== false && $iPos > 0) {
+				$this->sCallBackClassName = substr($sCallBackDefinition, 0, $iPos);
+				$this->sCallBackMethodName = substr($sCallBackDefinition, $iPos + 2);
+				$this->bIsStatic = true;
+			} else {
+				$sMessageError = "The callback '$sCallBackDefinition' is not a valid static method.";
+				IssueLog::Error($sMessageError, 'console');
+				throw new Exception($sMessageError);
+			}
+		}
+	}
+
+	public function IsStatic(): bool
+	{
+		return $this->bIsStatic;
+	}
+
+	/**
+	 * @throws ReflectionException
+	 * @throws SecurityException
+	 */
+	public function CheckCallbackSignature(string $sTriggeringObjectClass, array $aParamsType): void
+	{
+		$iParamCount = 0;
+		if ($this->bIsStatic) { // If the callback is static, we expect the first parameter to be the triggering object type
+			array_unshift($aParamsType, DBObject::class);
+		} else {
+			$this->sCallBackClassName = $sTriggeringObjectClass;
+		}
+		$oReflector = new ReflectionClass($this->sCallBackClassName);
+		$aCallbackParameters = $oReflector->getMethod($this->sCallBackMethodName)->getParameters();
+		$iRequiredNumberOfParams = count($aParamsType);
+		if (count($aCallbackParameters) !== $iRequiredNumberOfParams) {
+			$sErrorMessage = "The callback method '$this->sCallBackMethodName' of class '$this->sCallBackClassName' must have exactly $iRequiredNumberOfParams parameters.";
+			IssueLog::Error($sErrorMessage);
+			throw new SecurityException($sErrorMessage);
+		}
+		foreach ($aParamsType as $iParamOrder => $sParamType) {
+			$oParam = $aCallbackParameters[$iParamOrder];
+			if ($oParam->getType() === null) {
+				$sErrorMessage = "The callback method '$this->sCallBackMethodName' of class '$this->sCallBackClassName' must have type-hinted parameters, but parameter {$oParam->getName()} is not type-hinted.";
+				IssueLog::Error($sErrorMessage);
+				throw new SecurityException($sErrorMessage);
+			}
+			if ($oParam->getType()->getName() !== $sParamType) {
+				$sErrorMessage = "The callback method '$this->sCallBackMethodName' of class '$this->sCallBackClassName' must have a parameter of type '$sParamType', but it has {$oParam->getType()->getName()} instead.";
+				IssueLog::Error($sErrorMessage);
+				throw new SecurityException($sErrorMessage);
+			}
+		}
+	}
+
+	/**
+	 * @param $oTriggeringObject
+	 * @param array $aParams
+	 *
+	 * @return mixed
+	 */
+	public function Invoke($oTriggeringObject, array $aParams): mixed
+	{
+		if ($this->bIsStatic) {
+			return call_user_func_array([$this->sCallBackClassName, $this->sCallBackMethodName], array_merge([$oTriggeringObject], $aParams));
+
+		} else {
+			return call_user_func_array([$oTriggeringObject, $this->sCallBackMethodName], $aParams);
+		}
+	}
+}