diff --git a/config/packages/security.yaml b/config/packages/security.yaml index f3e2152..ada0f1a 100644 --- a/config/packages/security.yaml +++ b/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)/ diff --git a/src/EventListener/AccessDeniedListener.php b/src/EventListener/AccessDeniedListener.php index 968af31..096416c 100644 --- a/src/EventListener/AccessDeniedListener.php +++ b/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(); } } diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index 8d673bf..c355638 100644 --- a/src/Security/BpnAuthenticator.php +++ b/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 diff --git a/src/Security/DefaultRouteResolver.php b/src/Security/DefaultRouteResolver.php new file mode 100644 index 0000000..ceb4c9a --- /dev/null +++ b/src/Security/DefaultRouteResolver.php @@ -0,0 +1,39 @@ +authorizationChecker->isGranted(AdministrativeAccessVoter::ADMINISTRATIVE_ACCESS)) { + return 'app_admin_dashboard'; + } + + return 'app_account'; + } + + public function resolveUrl(): string + { + return $this->urlGenerator->generate($this->resolveRoute()); + } +} diff --git a/templates/account/index.html.twig b/templates/account/index.html.twig index 193d4d0..d3e8753 100644 --- a/templates/account/index.html.twig +++ b/templates/account/index.html.twig @@ -1,6 +1,7 @@ {% extends 'layout.html.twig' %} {% block content %} + {% include '_partials/_flashes.html.twig' %}

Mein
Account diff --git a/tests/EventListener/AccessDeniedListenerTest.php b/tests/EventListener/AccessDeniedListenerTest.php new file mode 100644 index 0000000..d3888a1 --- /dev/null +++ b/tests/EventListener/AccessDeniedListenerTest.php @@ -0,0 +1,130 @@ +request('/admin/user'); + $event = $this->dispatch($request, new User('admin@example.org')); + + $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('admin@example.org')); + + 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('admin@example.org')); + + $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('admin@example.org')); + + 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('admin@example.org')); + + self::assertFalse($event->hasResponse()); + self::assertSame([], $this->session($request)->getFlashBag()->all()); + } + + /** + * @param array $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; + } +} diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 55638c5..adc003e 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/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), ); } diff --git a/tests/Security/DefaultRouteResolverTest.php b/tests/Security/DefaultRouteResolverTest.php new file mode 100644 index 0000000..cbdd97c --- /dev/null +++ b/tests/Security/DefaultRouteResolverTest.php @@ -0,0 +1,51 @@ +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); + } +}