diff --git a/public/lib/classes/component.php b/public/lib/classes/component.php index 2294048fa57..41c52f95dc0 100644 --- a/public/lib/classes/component.php +++ b/public/lib/classes/component.php @@ -548,8 +548,7 @@ class component { // Always keep moodle_exception in place. $keyclasses = [ \core\exception\moodle_exception::class, - \core\navigation\navbar::class, - \core\navigation\navigation_node::class, + \core\router\middleware\api_validation_middleware::class, ]; foreach ($keyclasses as $classname) { if (!array_key_exists($classname, $cache['classmap'])) { diff --git a/public/lib/classes/router.php b/public/lib/classes/router.php index b5c927e9eef..d194cf89bd7 100644 --- a/public/lib/classes/router.php +++ b/public/lib/classes/router.php @@ -16,6 +16,10 @@ namespace core; +use core\exception\not_found_exception; +use core\exception\response_aware_exception; +use core\router\error_renderer; +use core\router\middleware\api_validation_middleware; use core\router\middleware\cors_middleware; use core\router\middleware\error_handling_middleware; use core\router\middleware\moodle_api_authentication_middleware; @@ -206,18 +210,25 @@ class router { $this->app->getCallableResolver(), $this->app->getResponseFactory(), displayErrorDetails: $displayerrordetails, - logErrors: true, - logErrorDetails: true, + logErrors: false, + logErrorDetails: false, ); // Set a custom error handler for the HttpNotFoundException and HttpForbiddenException. // We route these to a custom error handler to ensure that the error is displayed with a feedback form. + $errorhandler = new router\error_handler( + $this->app->getCallableResolver(), + $this->app->getResponseFactory(), + ); + $errorhandler->registerErrorRenderer('text/html', error_renderer::class); $errormiddleware->setErrorHandler( [ + response_aware_exception::class, HttpNotFoundException::class, HttpForbiddenException::class, ], - new router\error_handler($this->app), + $errorhandler, + true, ); $errormiddleware->getDefaultErrorHandler()->registerErrorRenderer('text/html', router\error_renderer::class); @@ -252,7 +263,7 @@ class router { // Add a Middleware to set the CORS headers for all REST Responses. ->add(di::get(cors_middleware::class)) ->add(di::get(moodle_api_authentication_middleware::class)) - ->add(di::get(validation_middleware::class)); + ->add(di::get(api_validation_middleware::class)); } /** diff --git a/public/lib/classes/router/error_handler.php b/public/lib/classes/router/error_handler.php index db80b3fb033..f72d0059d41 100644 --- a/public/lib/classes/router/error_handler.php +++ b/public/lib/classes/router/error_handler.php @@ -16,34 +16,19 @@ namespace core\router; +use core\exception\response_aware_exception; +use core\router\response\exception_response; use Psr\Http\Message\ServerRequestInterface; -use Slim\App; use Slim\Handlers\ErrorHandler; /** - * An Eerror Handler implementation for Moodle which is aware of the REST API. + * An Error Handler implementation for Moodle which is aware of the REST API. * * @package core * @copyright Andrew Lyons * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class error_handler extends ErrorHandler { - /** - * Construct a new Error Handler. - * - * @param \Slim\App $app - */ - public function __construct( - App $app, - ) { - parent::__construct( - $app->getCallableResolver(), - $app->getResponseFactory(), - ); - - $this->registerErrorRenderer('text/html', error_renderer::class); - } - #[\Override] protected function determineContentType(ServerRequestInterface $request): ?string { // For anything hitting /rest/api/v2 we will default to JSON. @@ -55,4 +40,18 @@ class error_handler extends ErrorHandler { // Fall back to the default behaviour of using the Accept header. return parent::determineContentType($request); } + + #[\Override] + protected function determineStatusCode(): int { + $exception = $this->exception; + + if ($exception instanceof response_aware_exception) { + $responseclassname = $exception->get_response_classname(); + if (is_subclass_of($responseclassname, exception_response::class)) { + return $responseclassname::get_exception_status_code(); + } + } + + return parent::determineStatusCode(); + } } diff --git a/public/lib/classes/router/middleware/api_validation_middleware.php b/public/lib/classes/router/middleware/api_validation_middleware.php new file mode 100644 index 00000000000..f9582d96d46 --- /dev/null +++ b/public/lib/classes/router/middleware/api_validation_middleware.php @@ -0,0 +1,49 @@ +. + +namespace core\router\middleware; + +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequestInterface; +use Psr\Http\Server\RequestHandlerInterface; + +/** + * Middleware to handle validation of API requests and responses based on the route data. + * + * @package core + * @copyright Andrew Lyons + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class api_validation_middleware extends validation_middleware { + #[\Override] + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { + try { + $request = $this->requestvalidator->validate_request($request); + } catch (\Exception $e) { + return $this->responsehandler->get_response_from_exception($request, $e); + } + + $response = $handler->handle($request); + + try { + $this->responsevalidator->validate_response($request, $response); + } catch (\Exception $e) { + return $this->responsehandler->get_response_from_exception($request, $e); + } + + return $response; + } +} diff --git a/public/lib/classes/router/middleware/validation_middleware.php b/public/lib/classes/router/middleware/validation_middleware.php index 05330873a01..4c7034bf9ff 100644 --- a/public/lib/classes/router/middleware/validation_middleware.php +++ b/public/lib/classes/router/middleware/validation_middleware.php @@ -53,27 +53,9 @@ class validation_middleware implements MiddlewareInterface { #[\Override] public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - global $USER; - - try { - $request = $this->requestvalidator->validate_request($request); - } catch (\Exception $e) { - $response = $this->responsehandler->get_response_from_exception($request, $e); - // Throw 'page not found' exception for non-admins. - // This hides stacktrace and errorcodes in detailed payload responses. - if (!is_siteadmin($USER->id) && $response->getStatusCode() == 404) { - return \core\router\util::throw_page_not_found($request, $response, $response->getReasonPhrase()); - } - return $response; - } - + $request = $this->requestvalidator->validate_request($request); $response = $handler->handle($request); - - try { - $this->responsevalidator->validate_response($request, $response); - } catch (\Exception $e) { - return $this->responsehandler->get_response_from_exception($request, $e); - } + $this->responsevalidator->validate_response($request, $response); return $response; } diff --git a/public/lib/classes/router/util.php b/public/lib/classes/router/util.php index 590160bd2fe..85dc9453af3 100644 --- a/public/lib/classes/router/util.php +++ b/public/lib/classes/router/util.php @@ -97,6 +97,39 @@ class util { return self::redirect($response, $url); } + /** + * Throw a specialized HTTP exception based on the response status code. + * + * @param ServerRequestInterface $request + * @param ResponseInterface $response + * @param string|null $message + * @param \Throwable|null $previous + */ + public static function throw_specialized_exception( + ServerRequestInterface $request, + ResponseInterface $response, + ?string $message = null, + ?\Throwable $previous = null, + ): void { + $exceptionclass = match ($response->getStatusCode()) { + 400 => \Slim\Exception\HttpBadRequestException::class, + 401 => \Slim\Exception\HttpUnauthorizedException::class, + 403 => \Slim\Exception\HttpForbiddenException::class, + 404 => \Slim\Exception\HttpNotFoundException::class, + 405 => \Slim\Exception\HttpMethodNotAllowedException::class, + 410 => \Slim\Exception\HttpGoneException::class, + 429 => \Slim\Exception\HttpTooManyRequestsException::class, + 500 => \Slim\Exception\HttpInternalServerErrorException::class, + 501 => \Slim\Exception\HttpNotImplementedException::class, + + default => null, + }; + + if ($exceptionclass !== null) { + throw new $exceptionclass($request, $message, $previous); + } + } + /** * Generate a Page Not Found result. * @@ -113,7 +146,7 @@ class util { ?string $message = null, ?\Throwable $previous = null, ): ResponseInterface { - throw new \Slim\Exception\HttpNotFoundException($request, $message, $previous); + self::throw_specialized_exception($request, $response->withStatus(404), $message, $previous); } /** diff --git a/public/lib/tests/router/middleware/api_validation_middleware_test.php b/public/lib/tests/router/middleware/api_validation_middleware_test.php new file mode 100644 index 00000000000..f59ab7130bb --- /dev/null +++ b/public/lib/tests/router/middleware/api_validation_middleware_test.php @@ -0,0 +1,117 @@ +. + +namespace core\router\middleware; + +use core\di; +use core\router\request_validator; +use core\router\response_handler; +use core\router\response_validator; +use GuzzleHttp\Psr7\Response; +use GuzzleHttp\Psr7\ServerRequest; +use Psr\Http\Server\RequestHandlerInterface; + +/** + * Tests for the API validation middleware. + * + * @package core + * @category test + * @copyright 2026 Laurent David + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +#[\PHPUnit\Framework\Attributes\CoversClass(api_validation_middleware::class)] +final class api_validation_middleware_test extends \advanced_testcase { + /** + * If request validation fails, API middleware returns an error response. + */ + public function test_process_fails_request_validation_returns_error_response(): void { + $request = new ServerRequest('GET', '/test'); + $errorresponse = (new Response())->withStatus(400); + + // Mock the request validator to throw an exception. + $requestvalidator = $this->getMockBuilder(request_validator::class)->getMock(); + $requestvalidator->expects($this->once()) + ->method('validate_request') + ->with($request) + ->willThrowException(new \Exception('Invalid request')); + + // If the request fails validation, it will not be passed to next Middleware. + $handler = $this->getMockBuilder(RequestHandlerInterface::class)->getMock(); + $handler->expects($this->never())->method('handle'); + + // It will return an error response. + $responsehandler = $this->getMockBuilder(response_handler::class) + ->disableOriginalConstructor() + ->getMock(); + $responsehandler->expects($this->once()) + ->method('get_response_from_exception') + ->with($request, $this->isInstanceOf(\Exception::class)) + ->willReturn($errorresponse); + + di::set(request_validator::class, $requestvalidator); + di::set(response_handler::class, $responsehandler); + + $middleware = di::get(api_validation_middleware::class); + $this->assertSame($errorresponse, $middleware->process($request, $handler)); + } + + /** + * If response validation fails, API middleware returns an error response. + */ + public function test_process_fails_response_validation_returns_error_response(): void { + $request = new ServerRequest('GET', '/test'); + $response = new Response(); + $errorresponse = (new Response())->withStatus(500); + + // Mock the request validator to pass validation. + $requestvalidator = $this->getMockBuilder(request_validator::class)->getMock(); + $requestvalidator->expects($this->once()) + ->method('validate_request') + ->with($request) + ->willReturnArgument(0); + + // The request will be passed to next Middleware. + $handler = $this->getMockBuilder(RequestHandlerInterface::class)->getMock(); + $handler->expects($this->once()) + ->method('handle') + ->with($request) + ->willReturn($response); + + // Mock the response validator to throw an exception. + $responsevalidator = $this->getMockBuilder(response_validator::class)->getMock(); + $responsevalidator->expects($this->once()) + ->method('validate_response') + ->with($request, $response) + ->willThrowException(new \Exception('Invalid response')); + + // It will return an error response. + $responsehandler = $this->getMockBuilder(response_handler::class) + ->disableOriginalConstructor() + ->getMock(); + $responsehandler->expects($this->once()) + ->method('get_response_from_exception') + ->with($request, $this->isInstanceOf(\Exception::class)) + ->willReturn($errorresponse); + + di::set(request_validator::class, $requestvalidator); + di::set(response_validator::class, $responsevalidator); + di::set(response_handler::class, $responsehandler); + + // Execute the middleware. + $middleware = di::get(api_validation_middleware::class); + $this->assertSame($errorresponse, $middleware->process($request, $handler)); + } +} diff --git a/public/lib/tests/router/middleware/error_handling_middleware_test.php b/public/lib/tests/router/middleware/error_handling_middleware_test.php index fcaab22b9d2..3f3c1ca76d5 100644 --- a/public/lib/tests/router/middleware/error_handling_middleware_test.php +++ b/public/lib/tests/router/middleware/error_handling_middleware_test.php @@ -55,7 +55,7 @@ final class error_handling_middleware_test extends route_testcase { } /** - * When no errors, the error handle is not called. + * When errors occur, the error handler is called. */ public function test_error_handling(): void { $responsehandler = $this->getMockBuilder(response_handler::class) diff --git a/public/lib/tests/router/middleware/validation_middleware_test.php b/public/lib/tests/router/middleware/validation_middleware_test.php index 07cdf9ef805..e912a03c073 100644 --- a/public/lib/tests/router/middleware/validation_middleware_test.php +++ b/public/lib/tests/router/middleware/validation_middleware_test.php @@ -45,7 +45,7 @@ final class validation_middleware_test extends \advanced_testcase { $requestvalidator->expects($this->once()) ->method('validate_request') ->with($request) - ->willThrowException(new \Exception('Invalid request')); + ->willThrowException(new \Slim\Exception\HttpException($request, 'Invalid request')); // If the request fails validation, it will not be passed to next Middleware. $handler = $this->getMockBuilder(RequestHandlerInterface::class)->getMock(); @@ -60,8 +60,10 @@ final class validation_middleware_test extends \advanced_testcase { // Execute the middleware. $middleware = di::get(validation_middleware::class); - $returns = $middleware->process($request, $handler); - $this->assertInstanceOf(ResponseInterface::class, $returns); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Invalid request'); + $middleware->process($request, $handler); } /** @@ -97,9 +99,10 @@ final class validation_middleware_test extends \advanced_testcase { // Execute the middleware. $middleware = di::get(validation_middleware::class); - $returns = $middleware->process($request, $handler); - $this->assertInstanceOf(ResponseInterface::class, $returns); - $this->assertNotEquals($response, $returns); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Invalid response'); + $middleware->process($request, $handler); } /** diff --git a/public/lib/tests/router_test.php b/public/lib/tests/router_test.php index d7e8f089c0f..d417887aa64 100644 --- a/public/lib/tests/router_test.php +++ b/public/lib/tests/router_test.php @@ -17,6 +17,8 @@ namespace core; use core\tests\router\route_testcase; +use GuzzleHttp\Psr7\ServerRequest; +use Psr\Http\Message\ResponseInterface; use Slim\App; /** @@ -25,9 +27,9 @@ use Slim\App; * @package core * @copyright Andrew Lyons * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @covers \core\router - * @covers \core\router\response_handler */ +#[\PHPUnit\Framework\Attributes\CoversClass(\core\router::class)] +#[\PHPUnit\Framework\Attributes\CoversClass(\core\router\response_handler::class)] final class router_test extends route_testcase { public function test_get_app(): void { $router = $this->get_router('/example'); @@ -88,9 +90,8 @@ final class router_test extends route_testcase { $this->assertEquals('/example', $router->basepath); } - /** - * @dataProvider basepath_provider - */ + + #[\PHPUnit\Framework\Attributes\DataProvider('basepath_provider')] public function test_basepath( string $wwwroot, string $expected, @@ -105,11 +106,19 @@ final class router_test extends route_testcase { $this->assertEquals($expected, $router->basepath); } - public static function basepath_provider(): \Iterator { + /** + * Data provider for test_basepath. + * + * @return \Generator + */ + public static function basepath_provider(): \Generator { yield 'Domain' => ['http://example.com', '/r.php']; yield 'Subdirectory' => ['http://example.com/moodle', '/moodle/r.php']; } + /** + * Test that the basepath is correctly guessed when accessed via r.php. + */ public function test_basepath_guessed_rphp(): void { $wwwroot = new \moodle_url('/r.php'); $_SERVER['SCRIPT_FILENAME'] = 'r.php'; @@ -151,11 +160,9 @@ final class router_test extends route_testcase { /** * Data provider for test_basepath_guessed_rphp_configuration_provided. * - * @return \Generator, mixed, void> + * @return \Generator */ - public static function router_configured_basepath_provider(): \Iterator { - global $CFG; - + public static function router_configured_basepath_provider(): \Generator { yield 'Root domain, Not configured, accessed via r.php' => [ 'http://example.com', null, @@ -229,4 +236,51 @@ final class router_test extends route_testcase { '/moodle/r.php', ]; } + + /** + * Test the expected error codes of various exception types. + * + * @param \Throwable $exception The exception to throw. + * @param int $expectedstatus The expected HTTP status code. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('exception_provider')] + public function test_error_codes_correct( + \Throwable $exception, + int $expectedstatus, + ): void { + $app = $this->get_app(); + + $app->map(['GET'], '/test', fn ($request, $response) => throw $exception); + // Handle the request. + $request = new ServerRequest('GET', '/test'); + $returns = $app->handle($request); + $this->assertInstanceOf(ResponseInterface::class, $returns); + $this->assertEquals($expectedstatus, $returns->getStatusCode()); + } + + /** + * Data provider for testing error handling. + * + * @return \Generator + */ + public static function exception_provider(): \Generator { + yield 'Generic Exception' => [new \Exception('Test'), 500]; + yield 'Moodle not_found_exception' => [ + new \core\exception\not_found_exception('test', 'thing'), + 404, + ]; + yield 'Not Found Exception' => [new \Slim\Exception\HttpNotFoundException(new ServerRequest('GET', '/test')), 404]; + yield 'Method Not Allowed Exception' => [ + new \Slim\Exception\HttpMethodNotAllowedException( + new ServerRequest('POST', '/test'), + 'GET', + ), + 405, + ]; + + yield 'Moodle exception not implementing response_aware_exception_interface' => [ + new \core\exception\moodle_exception('test', 'thing'), + 500, + ]; + } }