Fix prod-mode asserts, middleware exception routing, redirect GET guard (#330)

* Replace assert() in AuthorizationService with explicit exception throws

The three assert() calls in performCheck(), getCanHandler() and
getScopeHandler() are compiled out when PHP runs with zend.assertions=-1,
the standard production setting. Under that configuration a missing policy
method silently falls through to the callable invocation and throws a
generic Error instead of MissingMethodException, and an invalid policy
return value flows on and causes a downstream type error.

Convert the three checks to explicit if/throw so the documented exceptions
fire in both development and production.

* Route policy exceptions through unauthorizedHandler in RequestAuthorizationMiddleware

The call to AuthorizationService::canResult() sat outside the try/catch
block, so any Authorization\Exception\Exception thrown from inside a
RequestPolicy (e.g. MissingMethodException, a custom MissingIdentityException
raised by canAccess(), or another policy-level failure) bypassed the
configured unauthorizedHandler and bubbled out of the middleware unhandled.

Move the canResult() call inside the try block to match the symmetry of
AuthorizationMiddleware, so all policy-level exceptions are routed through
the handler. Add a regression test using the Suppress handler.

* Honor GET-only guard in CakeRedirectHandler::getUrl()

The parent RedirectHandler::getUrl() guards the redirect query-param
appendage with a GET method check, so POST/PUT/DELETE/PATCH unauthorized
responses do not receive a useless `?redirect=` payload that clients cannot
follow. The Cake-flavoured override dropped that guard and appended the
query param for every method.

Mirror the parent behavior and add a data-provider test covering all
common non-GET methods.

* Add regression test for invalid return type from policy method

Covers the explicit-throw branch added in AuthorizationService::performCheck()
when a policy method returns a value that is neither bool nor a
ResultInterface. Adds a canInvalidReturnType() helper to ArticlePolicy
returning a plain string and asserts the documented exception fires.

Author: dereuromark

src/AuthorizationService.php

@@ -94,13 +94,12 @@ protected function performCheck(
         $handler = $this->getCanHandler($policy, $action);
         $result = $handler($user, $resource, ...$optionalArgs);
 
-        assert(
-            is_bool($result) || $result instanceof ResultInterface,
-            new Exception(sprintf(
+        if (!is_bool($result) && !$result instanceof ResultInterface) {
+            throw new Exception(sprintf(
                 'Authorization check method must return `%s` or `bool`.',
                 ResultInterface::class,
-            )),
-        );
+            ));
+        }
 
         return $result;
     }
@@ -138,10 +137,9 @@ protected function getCanHandler(mixed $policy, string $action): Closure
     {
         $method = 'can' . ucfirst($action);
 
-        assert(
-            method_exists($policy, $method) || method_exists($policy, '__call'),
-            new MissingMethodException([$method, $action, $policy::class]),
-        );
+        if (!method_exists($policy, $method) && !method_exists($policy, '__call')) {
+            throw new MissingMethodException([$method, $action, $policy::class]);
+        }
 
         /** @phpstan-ignore callable.nonCallable */
         return [$policy, $method](...);
@@ -159,10 +157,9 @@ protected function getScopeHandler(mixed $policy, string $action): Closure
     {
         $method = 'scope' . ucfirst($action);
 
-        assert(
-            method_exists($policy, $method) || method_exists($policy, '__call'),
-            new MissingMethodException([$method, $action, $policy::class]),
-        );
+        if (!method_exists($policy, $method) && !method_exists($policy, '__call')) {
+            throw new MissingMethodException([$method, $action, $policy::class]);
+        }
 
         /** @phpstan-ignore callable.nonCallable */
         return [$policy, $method](...);

src/Middleware/RequestAuthorizationMiddleware.php

@@ -98,8 +98,8 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
         $service = $this->getServiceFromRequest($request);
         $identity = $request->getAttribute($this->getConfig('identityAttribute'));
 
-        $result = $service->canResult($identity, $this->getConfig('method'), $request);
         try {
+            $result = $service->canResult($identity, $this->getConfig('method'), $request);
             if (!$result->getStatus()) {
                 throw new ForbiddenException($result, [$this->getConfig('method'), $request->getRequestTarget()]);
             }

src/Middleware/UnauthorizedHandler/CakeRedirectHandler.php

@@ -68,7 +68,7 @@ public function __construct()
     protected function getUrl(ServerRequestInterface $request, array $options): string
     {
         $url = $options['url'];
-        if ($options['queryParam'] !== null) {
+        if ($options['queryParam'] !== null && $request->getMethod() === 'GET') {
             $uri = $request->getUri();
             $redirect = $uri->getPath();
             if ($uri->getQuery()) {

tests/TestCase/AuthorizationServiceTest.php

@@ -17,6 +17,7 @@
 namespace Authorization\Test\TestCase;
 
 use Authorization\AuthorizationService;
+use Authorization\Exception\Exception;
 use Authorization\IdentityDecorator;
 use Authorization\IdentityInterface;
 use Authorization\Policy\BeforePolicyInterface;
@@ -539,4 +540,22 @@ public function testMissingMethod(): void
 
         $service->can($user, 'disable', $entity);
     }
+
+    public function testCanThrowsWhenPolicyReturnsInvalidType(): void
+    {
+        $resolver = new MapResolver([
+            Article::class => ArticlePolicy::class,
+        ]);
+        $service = new AuthorizationService($resolver);
+        $user = new IdentityDecorator($service, [
+            'role' => 'admin',
+        ]);
+
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage(
+            'Authorization check method must return `Authorization\Policy\ResultInterface` or `bool`.',
+        );
+
+        $service->can($user, 'invalidReturnType', new Article());
+    }
 }

tests/TestCase/Middleware/RequestAuthorizationMiddlewareTest.php

@@ -146,4 +146,32 @@ public function testUnauthorizedHandlerSuppress(): void
 
         $this->assertSame(200, $result->getStatusCode());
     }
+
+    public function testPolicyExceptionRoutedThroughUnauthorizedHandler(): void
+    {
+        $request = (new ServerRequest([
+                'url' => '/articles/index',
+            ]))
+            ->withParam('action', 'index')
+            ->withParam('controller', 'Articles');
+
+        $handler = new TestRequestHandler();
+
+        $resolver = new MapResolver([
+            ServerRequest::class => new RequestPolicy(),
+        ]);
+
+        $authService = new AuthorizationService($resolver);
+        $request = $request->withAttribute('authorization', $authService);
+
+        // `doesNotExist` triggers MissingMethodException inside canResult();
+        // the middleware should route it through the configured handler instead of letting it bubble.
+        $middleware = new RequestAuthorizationMiddleware([
+            'method' => 'doesNotExist',
+            'unauthorizedHandler' => 'Suppress',
+        ]);
+        $result = $middleware->process($request, $handler);
+
+        $this->assertSame(200, $result->getStatusCode());
+    }
 }

tests/TestCase/Middleware/UnauthorizedHandler/CakeRedirectHandlerTest.php

@@ -22,6 +22,7 @@
 use Cake\Http\ServerRequestFactory;
 use Cake\Routing\Router;
 use Cake\TestSuite\TestCase;
+use PHPUnit\Framework\Attributes\DataProvider;
 
 class CakeRedirectHandlerTest extends TestCase
 {
@@ -150,4 +151,39 @@ public function testHandleRedirectWithBasePath(): void
             $response->getHeaderLine('Location'),
         );
     }
+
+    public static function httpMethodProvider(): array
+    {
+        return [
+            ['POST'],
+            ['PUT'],
+            ['DELETE'],
+            ['PATCH'],
+            ['OPTIONS'],
+            ['HEAD'],
+        ];
+    }
+
+    #[DataProvider('httpMethodProvider')]
+    public function testHandleRedirectionIgnoreNonIdempotentMethods(string $method): void
+    {
+        $handler = new CakeRedirectHandler();
+
+        $exception = new Exception();
+        $request = ServerRequestFactory::fromGlobals(
+            [
+                'REQUEST_METHOD' => $method,
+                'REQUEST_URI' => '/admin/dashboard',
+            ],
+        );
+
+        $response = $handler->handle($exception, $request, [
+            'exceptions' => [
+                Exception::class,
+            ],
+        ]);
+
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertSame('/login', $response->getHeaderLine('Location'));
+    }
 }

tests/test_app/TestApp/Policy/ArticlePolicy.php

@@ -141,4 +141,13 @@ public function canWithInjectedService($user, Article $article)
     {
         return $this->service->serviceLogic();
     }
+
+    /**
+     * Returns an invalid type (not bool, not ResultInterface) to exercise the
+     * defensive type guard in AuthorizationService::performCheck().
+     */
+    public function canInvalidReturnType($user, Article $article): string
+    {
+        return 'not a bool nor a Result';
+    }
 }