diff --git a/core/Middleware/ApiErrorMiddleware.php b/core/Middleware/ApiErrorMiddleware.php new file mode 100644 index 0000000000000..605b8270879ab --- /dev/null +++ b/core/Middleware/ApiErrorMiddleware.php @@ -0,0 +1,53 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program 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 Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\Core\Middleware; + +use Exception; +use OC\AppFramework\Exceptions\ParameterMissingException; +use OC\Http\ApiResponse; +use OCP\AppFramework\Http; +use OCP\AppFramework\Middleware; +use OCP\IRequest; + +class ApiErrorMiddleware extends Middleware { + private IRequest $request; + + public function __construct(IRequest $request) { + $this->request = $request; + } + + public function afterException($controller, $methodName, Exception $exception) { + $sendsJson = $this->request->getHeader('Content-Type') === 'application/json'; + $acceptsJson = $this->request->getHeader('Accept') === 'application/json'; + if (($sendsJson || $acceptsJson) && $exception instanceof ParameterMissingException) { + return ApiResponse::fail( + ['error' => 'Required parameter ' . $exception->getParameterName() . ' missing'], + Http::STATUS_UNPROCESSABLE_ENTITY, + ); + } + return parent::afterException($controller, $methodName, $exception); + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 90e7fe51fa74a..23d8c3ab0655a 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -678,6 +678,7 @@ 'OC\\AppFramework\\Bootstrap\\ServiceFactoryRegistration' => $baseDir . '/lib/private/AppFramework/Bootstrap/ServiceFactoryRegistration.php', 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => $baseDir . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => $baseDir . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', + 'OC\\AppFramework\\Exceptions\\ParameterMissingException' => $baseDir . '/lib/private/AppFramework/Exceptions/ParameterMissingException.php', 'OC\\AppFramework\\Http' => $baseDir . '/lib/private/AppFramework/Http.php', 'OC\\AppFramework\\Http\\Dispatcher' => $baseDir . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => $baseDir . '/lib/private/AppFramework/Http/Output.php', @@ -1021,6 +1022,7 @@ 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => $baseDir . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => $baseDir . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => $baseDir . '/core/Listener/BeforeTemplateRenderedListener.php', + 'OC\\Core\\Middleware\\ApiErrorMiddleware' => $baseDir . '/core/Middleware/ApiErrorMiddleware.php', 'OC\\Core\\Middleware\\TwoFactorMiddleware' => $baseDir . '/core/Middleware/TwoFactorMiddleware.php', 'OC\\Core\\Migrations\\Version13000Date20170705121758' => $baseDir . '/core/Migrations/Version13000Date20170705121758.php', 'OC\\Core\\Migrations\\Version13000Date20170718121200' => $baseDir . '/core/Migrations/Version13000Date20170718121200.php', @@ -1290,6 +1292,7 @@ 'OC\\Hooks\\Emitter' => $baseDir . '/lib/private/Hooks/Emitter.php', 'OC\\Hooks\\EmitterTrait' => $baseDir . '/lib/private/Hooks/EmitterTrait.php', 'OC\\Hooks\\PublicEmitter' => $baseDir . '/lib/private/Hooks/PublicEmitter.php', + 'OC\\Http\\ApiResponse' => $baseDir . '/lib/private/Http/ApiResponse.php', 'OC\\Http\\Client\\Client' => $baseDir . '/lib/private/Http/Client/Client.php', 'OC\\Http\\Client\\ClientService' => $baseDir . '/lib/private/Http/Client/ClientService.php', 'OC\\Http\\Client\\DnsPinMiddleware' => $baseDir . '/lib/private/Http/Client/DnsPinMiddleware.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 8cef29efc39bc..8de9ea6ab0263 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -711,6 +711,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Bootstrap\\ServiceFactoryRegistration' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Bootstrap/ServiceFactoryRegistration.php', 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => __DIR__ . '/../../..' . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', + 'OC\\AppFramework\\Exceptions\\ParameterMissingException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Exceptions/ParameterMissingException.php', 'OC\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http.php', 'OC\\AppFramework\\Http\\Dispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Output.php', @@ -1054,6 +1055,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => __DIR__ . '/../../..' . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => __DIR__ . '/../../..' . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => __DIR__ . '/../../..' . '/core/Listener/BeforeTemplateRenderedListener.php', + 'OC\\Core\\Middleware\\ApiErrorMiddleware' => __DIR__ . '/../../..' . '/core/Middleware/ApiErrorMiddleware.php', 'OC\\Core\\Middleware\\TwoFactorMiddleware' => __DIR__ . '/../../..' . '/core/Middleware/TwoFactorMiddleware.php', 'OC\\Core\\Migrations\\Version13000Date20170705121758' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170705121758.php', 'OC\\Core\\Migrations\\Version13000Date20170718121200' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170718121200.php', @@ -1323,6 +1325,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Hooks\\Emitter' => __DIR__ . '/../../..' . '/lib/private/Hooks/Emitter.php', 'OC\\Hooks\\EmitterTrait' => __DIR__ . '/../../..' . '/lib/private/Hooks/EmitterTrait.php', 'OC\\Hooks\\PublicEmitter' => __DIR__ . '/../../..' . '/lib/private/Hooks/PublicEmitter.php', + 'OC\\Http\\ApiResponse' => __DIR__ . '/../../..' . '/lib/private/Http/ApiResponse.php', 'OC\\Http\\Client\\Client' => __DIR__ . '/../../..' . '/lib/private/Http/Client/Client.php', 'OC\\Http\\Client\\ClientService' => __DIR__ . '/../../..' . '/lib/private/Http/Client/ClientService.php', 'OC\\Http\\Client\\DnsPinMiddleware' => __DIR__ . '/../../..' . '/lib/private/Http/Client/DnsPinMiddleware.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 298b9f000e36e..bea877294ce4e 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -300,6 +300,9 @@ public function __construct($appName, $urlParams = [], ServerContainer $server = $c->get(OC\Security\RateLimiting\Limiter::class) ) ); + $dispatcher->registerMiddleware( + $c->get(\OC\Core\Middleware\ApiErrorMiddleware::class) + ); $dispatcher->registerMiddleware( new OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware( $c->get(IRequest::class), diff --git a/lib/private/AppFramework/Exceptions/ParameterMissingException.php b/lib/private/AppFramework/Exceptions/ParameterMissingException.php new file mode 100644 index 0000000000000..11d9e6e21784a --- /dev/null +++ b/lib/private/AppFramework/Exceptions/ParameterMissingException.php @@ -0,0 +1,41 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program 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 Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\AppFramework\Exceptions; + +use Exception; + +class ParameterMissingException extends Exception { + private string $parameterName; + + public function __construct(string $controllerName, string $methodName, string $parameterName) { + parent::__construct("Parameter $parameterName missing for $controllerName::$methodName"); + $this->parameterName = $parameterName; + } + + public function getParameterName(): string { + return $this->parameterName; + } +} diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index c1a203a7165f8..0faa7cbfab64b 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -32,6 +32,7 @@ */ namespace OC\AppFramework\Http; +use OC\AppFramework\Exceptions\ParameterMissingException; use OC\AppFramework\Http; use OC\AppFramework\Middleware\MiddlewareDispatcher; use OC\AppFramework\Utility\ControllerMethodReflector; @@ -43,6 +44,9 @@ use OCP\IConfig; use OCP\IRequest; use Psr\Log\LoggerInterface; +use ReflectionMethod; +use ReflectionNamedType; +use function get_class; /** * Class to dispatch the request to the middleware dispatcher @@ -192,20 +196,31 @@ public function dispatch(Controller $controller, string $methodName): array { */ private function executeController(Controller $controller, string $methodName): Response { $arguments = []; + $reflection = new ReflectionMethod($controller, $methodName); // valid types that will be casted $types = ['int', 'integer', 'bool', 'boolean', 'float']; - foreach ($this->reflector->getParameters() as $param => $default) { + foreach ($reflection->getParameters() as $param) { + $paramName = $param->name; // try to get the parameter from the request object and cast - // it to the type annotated in the @param annotation - $value = $this->request->getParam($param, $default); - $type = $this->reflector->getType($param); + // it to the type annotated in the @paramName annotation + $defaultValue = null; + if ($param->isDefaultValueAvailable()) { + $defaultValue = $param->getDefaultValue(); + } + $value = $this->request->getParam($paramName, $defaultValue); + $type = $param->getType(); + if ($type instanceof ReflectionNamedType) { + $typeName = $type->getName(); + } else { + $typeName = $this->reflector->getType($paramName); + } // if this is submitted using GET or a POST form, 'false' should be // converted to false - if (($type === 'bool' || $type === 'boolean') && + if (($typeName === 'bool' || $typeName === 'boolean') && $value === 'false' && ( $this->request->method === 'GET' || @@ -214,8 +229,12 @@ private function executeController(Controller $controller, string $methodName): ) ) { $value = false; - } elseif ($value !== null && \in_array($type, $types, true)) { - settype($value, $type); + } elseif ($value !== null && \in_array($typeName, $types, true)) { + settype($value, $typeName); + } + + if ($value === null && !$param->allowsNull()) { + throw new ParameterMissingException(get_class($controller), $methodName, $paramName); } $arguments[] = $value; diff --git a/lib/private/Http/ApiResponse.php b/lib/private/Http/ApiResponse.php new file mode 100644 index 0000000000000..928bbcdfc9b04 --- /dev/null +++ b/lib/private/Http/ApiResponse.php @@ -0,0 +1,75 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program 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 Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\Http; + +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\JSONResponse; + +/** + * JSend-link API response + * + * @see https://github.com/omniti-labs/jsend + */ +class ApiResponse extends JSONResponse { + public static function success($data = null, int $statusCode = Http::STATUS_OK): self { + return new self( + [ + 'status' => 'success', + 'data' => $data, + ], + $statusCode + ); + } + + public static function fail($data = null, int $statusCode = Http::STATUS_BAD_REQUEST): self { + return new self( + [ + 'status' => 'fail', + 'data' => $data, + ], + $statusCode + ); + } + + public static function error(string $message, + int $statusCode = Http::STATUS_INTERNAL_SERVER_ERROR, + int $code = null, + $data = null): self { + return new self( + [ + 'status' => 'error', + 'message' => $message, + 'code' => $code, + 'data' => $data, + ], + $statusCode + ); + } + + private function __construct($data = [], $statusCode = Http::STATUS_OK) { + parent::__construct($data, $statusCode); + } +} diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index 8f591f26e58a6..4c9ff7dd2d318 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -62,6 +62,9 @@ public function exec($int, $bool, $test = 4, $test2 = 1) { return [$int, $bool, $test, $test2]; } + public function execTyped(int $int, bool $bool, string $str, ?int $opt = 42): array { + return [$int, $bool, $str, $opt]; + } /** * @param int $int @@ -370,7 +373,35 @@ public function testControllerParametersInjectedDefaultOverwritten() { $this->assertEquals('[3,true,4,7]', $response[3]); } + public function testTypedControllerParameters(): void { + $this->request = new Request( + [ + 'post' => [ + 'int' => '3', + 'bool' => 'false', + 'str' => '7', + ], + 'method' => 'POST', + ], + $this->createMock(IRequestId::class), + $this->createMock(IConfig::class) + ); + $this->dispatcher = new Dispatcher( + $this->http, $this->middlewareDispatcher, $this->reflector, + $this->request, + $this->config, + \OC::$server->getDatabaseConnection(), + $this->logger, + $this->eventLogger + ); + $controller = new TestController('app', $this->request); + + // reflector is supposed to be called once + $this->dispatcherPassthrough(); + $response = $this->dispatcher->dispatch($controller, 'execTyped'); + $this->assertEquals('[3,true,"7",42]', $response[3]); + } public function testResponseTransformedByUrlFormat() { $this->request = new Request(