fix: handle access denied errors properly

config/packages/security.yaml

@@ -17,9 +17,10 @@ security:
                         roles: [ ROLE_MAILJET_WEBHOOK ]
 
     role_hierarchy:
+        ROLE_ADMIN:
+            - ROLE_GROUPS_ADMIN
         ROLE_GROUPS_ADMIN:
             - ROLE_GROUPS_MANAGER
-            -
     firewalls:
         dev:
             pattern: ^/(_(profiler|wdt)|css|images|js)/

src/EventListener/AccessDeniedListener.php

@@ -4,9 +4,12 @@ declare(strict_types=1);
 
 namespace App\EventListener;
 
+use App\Htmx\HxRedirectResponse;
+use App\Security\DefaultRouteResolver;
 use Psr\Log\LoggerInterface;
 use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpKernel\Event\ExceptionEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
@@ -19,12 +22,17 @@ use Symfony\Component\Security\Core\Exception\AccessDeniedException;
  * anonymous users, or "Access denied" for authenticated users. Clears target
  * path for authenticated denials to prevent redirect loops. OAuth2 routes are
  * logged but allowed to pass through without modification.
+ *
+ * Authenticated users are redirected to their default route, where the flash
+ * message is shown. Anonymous users keep falling through to the firewall entry
+ * point, which sends them to the login form and preserves the target path.
  */
 class AccessDeniedListener implements EventSubscriberInterface
 {
     public function __construct(
         private readonly Security $security,
         private readonly LoggerInterface $authLogger,
+        private readonly DefaultRouteResolver $defaultRouteResolver,
     ) {
     }
 
@@ -58,16 +66,33 @@ class AccessDeniedListener implements EventSubscriberInterface
         /** @var Session $session */
         $session = $request->getSession();
 
-        if (null === $this->security->getUser()) {
-            $session->getFlashBag()->add('info', 'Bitte melde dich an.');
-        } else {
-            $session->getFlashBag()->add('error', 'Zugriff verweigert');
-            // Unset potentially set target path to avoid access denied errors
-            $session->remove('_security.main.target_path');
-        }
-
         $this->authLogger->warning('Access denied', [
             'uri' => $request->getRequestUri(),
         ]);
+
+        if (null === $this->security->getUser()) {
+            $session->getFlashBag()->add('info', 'Bitte melde dich an.');
+
+            return;
+        }
+
+        $session->getFlashBag()->add('error', 'Zugriff verweigert');
+        // Unset potentially set target path to avoid access denied errors
+        $session->remove('_security.main.target_path');
+
+        $url = $this->defaultRouteResolver->resolveUrl();
+
+        // The default route denied access itself, redirecting there would loop
+        if ($url === $request->getPathInfo()) {
+            return;
+        }
+
+        $event->setResponse(
+            $request->headers->has('HX-Request')
+                ? new HxRedirectResponse($url)
+                : new RedirectResponse($url)
+        );
+        // HxRedirectResponse carries a 200; without this the kernel turns it into a 500
+        $event->allowCustomResponseCode();
     }
 }

src/Security/BpnAuthenticator.php

@@ -53,6 +53,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
         private readonly Crypt $crypt,
         private readonly ProfileCompletenessChecker $completenessChecker,
         private readonly LoggerInterface $authLogger,
+        private readonly DefaultRouteResolver $defaultRouteResolver,
     ) {
     }
 
@@ -174,7 +175,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
         ]);
 
         $targetPath = $this->getTargetPath($request->getSession(), $firewallName)
-            ?? $this->urlGenerator->generate('app_account');
+            ?? $this->defaultRouteResolver->resolveUrl();
 
         // When redirecting to admin from an HTMX request, force a full page navigation
         // to avoid layout issues between frontend (hx-boost) and EasyAdmin

src/Security/DefaultRouteResolver.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Security;
+
+use App\Security\Voter\AdministrativeAccessVoter;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
+
+/**
+ * Resolves the route a user belongs to when no specific destination is known.
+ *
+ * Used after login (when no target path was stored) and when access to a page is
+ * denied, so users land somewhere they are actually allowed to be instead of on an
+ * error page.
+ */
+class DefaultRouteResolver
+{
+    public function __construct(
+        private readonly AuthorizationCheckerInterface $authorizationChecker,
+        private readonly UrlGeneratorInterface $urlGenerator,
+    ) {
+    }
+
+    public function resolveRoute(): string
+    {
+        if ($this->authorizationChecker->isGranted(AdministrativeAccessVoter::ADMINISTRATIVE_ACCESS)) {
+            return 'app_admin_dashboard';
+        }
+
+        return 'app_account';
+    }
+
+    public function resolveUrl(): string
+    {
+        return $this->urlGenerator->generate($this->resolveRoute());
+    }
+}

templates/account/index.html.twig

@@ -1,6 +1,7 @@
 {% extends 'layout.html.twig' %}
 
 {% block content %}
+    {% include '_partials/_flashes.html.twig' %}
     <div class="px-4 lg:px-8 py-8 lg:py-16">
         <h1 class="text-white uppercase pb-4">
             Mein<br>Account

tests/EventListener/AccessDeniedListenerTest.php

@@ -0,0 +1,130 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\EventListener;
+
+use App\Entity\User;
+use App\EventListener\AccessDeniedListener;
+use App\Htmx\HxRedirectResponse;
+use App\Security\DefaultRouteResolver;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
+use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
+use Symfony\Component\HttpKernel\Event\ExceptionEvent;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\Security\Core\Exception\AccessDeniedException;
+use Symfony\Component\Security\Core\User\UserInterface;
+
+class AccessDeniedListenerTest extends TestCase
+{
+    public function testAuthenticatedUserIsRedirectedToTheirDefaultRoute(): void
+    {
+        $request = $this->request('/admin/user');
+        $event = $this->dispatch($request, new User('[email protected]'));
+
+        $response = $event->getResponse();
+        self::assertInstanceOf(RedirectResponse::class, $response);
+        self::assertSame('/admin/dashboard', $response->getTargetUrl());
+        self::assertSame(['Zugriff verweigert'], $this->session($request)->getFlashBag()->get('error'));
+    }
+
+    public function testTargetPathIsClearedToAvoidBeingSentBackToTheDeniedPage(): void
+    {
+        $request = $this->request('/admin/user');
+        $this->session($request)->set('_security.main.target_path', '/admin/user');
+
+        $this->dispatch($request, new User('[email protected]'));
+
+        self::assertFalse($this->session($request)->has('_security.main.target_path'));
+    }
+
+    public function testHtmxRequestsGetAFullPageRedirect(): void
+    {
+        $request = $this->request('/admin/user', ['HTTP_HX-Request' => 'true']);
+        $event = $this->dispatch($request, new User('[email protected]'));
+
+        $response = $event->getResponse();
+        self::assertInstanceOf(HxRedirectResponse::class, $response);
+        self::assertSame('/admin/dashboard', $response->headers->get('HX-Redirect'));
+        // Without this the kernel replaces the 200 of an HxRedirectResponse with a 500
+        self::assertTrue($event->isAllowingCustomResponseCode());
+    }
+
+    public function testDenialOnTheDefaultRouteItselfDoesNotRedirect(): void
+    {
+        $request = $this->request('/admin/dashboard');
+        $event = $this->dispatch($request, new User('[email protected]'));
+
+        self::assertFalse($event->hasResponse());
+    }
+
+    public function testAnonymousUsersFallThroughToTheEntryPoint(): void
+    {
+        $request = $this->request('/admin/user');
+        $event = $this->dispatch($request, null);
+
+        self::assertFalse($event->hasResponse());
+        self::assertSame(['Bitte melde dich an.'], $this->session($request)->getFlashBag()->get('info'));
+    }
+
+    public function testOauth2RoutesAreLeftUntouched(): void
+    {
+        $request = $this->request('/authorize');
+        $request->attributes->set('_route', 'oauth2_authorize');
+
+        $event = $this->dispatch($request, new User('[email protected]'));
+
+        self::assertFalse($event->hasResponse());
+        self::assertSame([], $this->session($request)->getFlashBag()->all());
+    }
+
+    /**
+     * @param array<string, string> $server
+     */
+    private function request(string $uri, array $server = []): Request
+    {
+        $request = Request::create($uri, server: $server);
+        $request->setSession(new Session(new MockArraySessionStorage()));
+
+        return $request;
+    }
+
+    private function session(Request $request): Session
+    {
+        /** @var Session $session */
+        $session = $request->getSession();
+
+        return $session;
+    }
+
+    private function dispatch(Request $request, ?UserInterface $user): ExceptionEvent
+    {
+        $security = $this->createMock(Security::class);
+        $security->method('getUser')->willReturn($user);
+
+        $resolver = $this->createMock(DefaultRouteResolver::class);
+        $resolver->method('resolveUrl')->willReturn('/admin/dashboard');
+
+        $listener = new AccessDeniedListener(
+            $security,
+            $this->createMock(LoggerInterface::class),
+            $resolver,
+        );
+
+        $event = new ExceptionEvent(
+            $this->createMock(HttpKernelInterface::class),
+            $request,
+            HttpKernelInterface::MAIN_REQUEST,
+            new AccessDeniedException(),
+        );
+
+        $listener->onKernelException($event);
+
+        return $event;
+    }
+}

tests/Security/BpnAuthenticatorTest.php

@@ -10,6 +10,7 @@ use App\BusProNet\Model\PersonalData;
 use App\Entity\User;
 use App\Security\BpnAuthenticator;
 use App\Security\Crypt;
+use App\Security\DefaultRouteResolver;
 use App\Security\Role;
 use App\Service\ProfileCompletenessChecker;
 use Doctrine\ORM\EntityManagerInterface;
@@ -143,6 +144,7 @@ class BpnAuthenticatorTest extends TestCase
             $crypt,
             $completenessChecker,
             $this->createMock(LoggerInterface::class),
+            $this->createMock(DefaultRouteResolver::class),
         );
     }
 

tests/Security/DefaultRouteResolverTest.php

@@ -0,0 +1,51 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Security;
+
+use App\Security\DefaultRouteResolver;
+use App\Security\Voter\AdministrativeAccessVoter;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
+
+class DefaultRouteResolverTest extends TestCase
+{
+    public function testStaffLandsInTheAdminArea(): void
+    {
+        self::assertSame('app_admin_dashboard', $this->resolver(true)->resolveRoute());
+    }
+
+    public function testEverybodyElseLandsOnTheirAccount(): void
+    {
+        self::assertSame('app_account', $this->resolver(false)->resolveRoute());
+    }
+
+    public function testUrlIsGeneratedForTheResolvedRoute(): void
+    {
+        self::assertSame('/admin/dashboard', $this->resolver(true)->resolveUrl());
+    }
+
+    private function resolver(bool $hasAdministrativeAccess): DefaultRouteResolver
+    {
+        $authorizationChecker = $this->createMock(AuthorizationCheckerInterface::class);
+        $authorizationChecker
+            ->method('isGranted')
+            ->with(AdministrativeAccessVoter::ADMINISTRATIVE_ACCESS)
+            ->willReturn($hasAdministrativeAccess)
+        ;
+
+        $urlGenerator = $this->createMock(UrlGeneratorInterface::class);
+        $urlGenerator
+            ->method('generate')
+            ->willReturnCallback(static fn (string $route): string => match ($route) {
+                'app_admin_dashboard' => '/admin/dashboard',
+                'app_account' => '/account',
+                default => self::fail(sprintf('Unexpected route "%s"', $route)),
+            })
+        ;
+
+        return new DefaultRouteResolver($authorizationChecker, $urlGenerator);
+    }
+}