From cc3d32fcbbb9c43ecbaebe5cb696130ff0efd9db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Wed, 23 Sep 2026 13:46:18 +0200 Subject: [PATCH] feat: gate oauth2 authorization on per-client roles --- config/packages/league_oauth2_server.yaml | 3 + migrations/Version20260923120000.php | 60 ++++++ src/Entity/OAuth2Client.php | 70 +++++++ .../AuthorizationCodeListener.php | 100 ++++++++- templates/security/oauth2_denied.html.twig | 23 +++ .../AuthorizationCodeListenerTest.php | 191 ++++++++++++++++++ 6 files changed, 436 insertions(+), 11 deletions(-) create mode 100644 migrations/Version20260923120000.php create mode 100644 src/Entity/OAuth2Client.php create mode 100644 templates/security/oauth2_denied.html.twig create mode 100644 tests/EventListener/AuthorizationCodeListenerTest.php diff --git a/config/packages/league_oauth2_server.yaml b/config/packages/league_oauth2_server.yaml index 6842142..334f033 100644 --- a/config/packages/league_oauth2_server.yaml +++ b/config/packages/league_oauth2_server.yaml @@ -19,6 +19,9 @@ league_oauth2_server: doctrine: null client: allow_plaintext_secrets: false + # App\Entity\OAuth2Client adds the required_roles column that + # App\EventListener\AuthorizationCodeListener gates authorization on. + classname: App\Entity\OAuth2Client when@test: league_oauth2_server: diff --git a/migrations/Version20260923120000.php b/migrations/Version20260923120000.php new file mode 100644 index 0000000..ecb8b68 --- /dev/null +++ b/migrations/Version20260923120000.php @@ -0,0 +1,60 @@ +addSql("ALTER TABLE oauth2_client ADD required_roles JSON DEFAULT NULL COMMENT '(DC2Type:json)'"); + $this->addSql("UPDATE oauth2_client SET required_roles = '[]'"); + $this->addSql("ALTER TABLE oauth2_client MODIFY required_roles JSON NOT NULL COMMENT '(DC2Type:json)'"); + + // mirrors ephub's App\Security\Role::ELIGIBLE + $this->addSql('UPDATE oauth2_client SET required_roles = :roles WHERE name = :name', [ + 'roles' => json_encode(['ROLE_EMPLOYEE', 'ROLE_ADMIN'], JSON_THROW_ON_ERROR), + 'name' => 'EPHub', + ]); + + // mirrors myep-team's App\Security\MyEpAuthenticator::ELIGIBLE_ROLES + $this->addSql('UPDATE oauth2_client SET required_roles = :roles WHERE name = :name', [ + 'roles' => json_encode(['ROLE_TEAM_ADMIN', 'ROLE_TEAMER', 'ROLE_MANAGER', 'ROLE_HOUSE_MANAGER'], JSON_THROW_ON_ERROR), + 'name' => 'MyEPTeam', + ]); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE oauth2_client DROP required_roles'); + } +} diff --git a/src/Entity/OAuth2Client.php b/src/Entity/OAuth2Client.php new file mode 100644 index 0000000..af9aaa8 --- /dev/null +++ b/src/Entity/OAuth2Client.php @@ -0,0 +1,70 @@ + + */ + #[ORM\Column(type: 'json')] + private array $requiredRoles = []; + + /** + * @return list + */ + public function getRequiredRoles(): array + { + return $this->requiredRoles; + } + + /** + * @param list $requiredRoles + */ + public function setRequiredRoles(array $requiredRoles): self + { + $this->requiredRoles = $requiredRoles; + + return $this; + } +} diff --git a/src/EventListener/AuthorizationCodeListener.php b/src/EventListener/AuthorizationCodeListener.php index ef6b30d..a0aa44b 100644 --- a/src/EventListener/AuthorizationCodeListener.php +++ b/src/EventListener/AuthorizationCodeListener.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace App\EventListener; +use App\Entity\OAuth2Client; use App\Entity\User; +use App\Security\Role; use League\Bundle\OAuth2ServerBundle\Event\AuthorizationRequestResolveEvent; use League\Bundle\OAuth2ServerBundle\OAuth2Events; use Psr\Log\LoggerInterface; @@ -14,14 +16,21 @@ use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Twig\Environment; /** * Handles OAuth2 authorization code grant flow. * - * Automatically approves authorization requests for authenticated users without - * consent prompts. Redirects anonymous users to login. For external OAuth2 - * requests (flagged via session), logs out the user after authorization to - * prevent session hijacking. + * Redirects anonymous users to login, then decides whether the account may authorize the + * client that asked. Consent is deliberately not part of that decision: every client is + * one of our own applications, so there is nothing for the user to weigh up. What the user + * cannot be asked, though, is whether they are entitled to the application at all, and + * that is what this listener answers — a client declares the roles it requires in + * OAuth2Client::$requiredRoles and an account holding none of them is refused here rather + * than handed a token the client will reject for reasons it cannot explain. + * + * For external OAuth2 requests (flagged via session), logs out the user after a successful + * authorization to prevent session hijacking. */ #[AsEventListener(event: OAuth2Events::AUTHORIZATION_REQUEST_RESOLVE, method: 'onAuthorizationRequestResolve')] class AuthorizationCodeListener @@ -31,6 +40,7 @@ class AuthorizationCodeListener private readonly RequestStack $requestStack, private readonly Security $security, private readonly LoggerInterface $authenticationLogger, + private readonly Environment $twig, ) { } @@ -53,13 +63,81 @@ class AuthorizationCodeListener ); $event->setResponse($response); $this->authenticationLogger->info('Authorization request without session'); - } else { - $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); - // in case this authorization request has been flagged as external in login controller, - // immediately logout the current user (see App\Controller\Core\Security\LoginController). - if (true === $request->getSession()->get('_oauth2', false)) { - $this->security->logout(false); - } + + return; + } + + $client = $event->getClient(); + + // the event is typed to the bundle's ClientInterface, which knows nothing about the + // column below: anything that is not our entity is a client we cannot have configured + if (false === $client instanceof OAuth2Client) { + $this->deny($event, $user, $client->getIdentifier(), [], 'unexpected client class'); + + return; + } + + $required = $client->getRequiredRoles(); + + // An empty requirement is not "no requirement": it is the state a client row starts + // in, so it means nobody has said who may use this client yet. + if ([] === $required) { + $this->deny($event, $user, $client->getName(), $required, 'client has no roles configured'); + + return; + } + + // effectiveOnly(), never getRoles(): a *_PENDING marker is a CRM nomination that + // grants nothing until an administrator approves it, and matching the raw role set + // would let a nomination open a staff application on its own. + if ([] === array_intersect($required, Role::effectiveOnly($user->getRoles()))) { + $this->deny($event, $user, $client->getName(), $required, 'insufficient roles'); + + return; + } + + $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); + + // in case this authorization request has been flagged as external in login controller, + // immediately logout the current user (see App\Controller\SecurityController). + if (true === $request->getSession()->get('_oauth2', false)) { + $this->security->logout(false); } } + + /** + * Refuses the request and explains it on our own page. + * + * The resolution is left at its AUTHORIZATION_DENIED default; setting a response is what + * keeps the user here instead of bouncing them back to a client that would only be able + * to report that no authorization code arrived. The session is deliberately left intact + * — the user has to still be signed in to read the page and navigate away from it. + * + * Logged with the client's requirement but never with the user's roles: the requirement + * is configuration, and it is what makes a misspelled entry diagnosable from the log, + * while the account's roles are personal data and the auth channel is database-backed. + * + * @param list $required + */ + private function deny( + AuthorizationRequestResolveEvent $event, + User $user, + string $client, + array $required, + string $reason, + ): void { + $this->authenticationLogger->info('Authorization request denied', [ + 'client' => $client, + 'user_id' => $user->getId(), + 'required_roles' => $required, + 'reason' => $reason, + ]); + + $event->setResponse(new Response( + // oauth2: true drops hx-boost in base.html.twig -- this page is rendered + // mid-redirect-chain, where boosting breaks the navigation + $this->twig->render('security/oauth2_denied.html.twig', ['client' => $client, 'oauth2' => true]), + Response::HTTP_FORBIDDEN + )); + } } diff --git a/templates/security/oauth2_denied.html.twig b/templates/security/oauth2_denied.html.twig new file mode 100644 index 0000000..49cd6d7 --- /dev/null +++ b/templates/security/oauth2_denied.html.twig @@ -0,0 +1,23 @@ +{% extends 'layout.html.twig' %} + +{% block title %}Kein Zugriff{% endblock %} + +{% block content %} +
+

+ Kein Zugriff +

+

+ Du bist bei MyE&P angemeldet, aber dein Konto ist nicht für + {{ client }} freigeschaltet. +

+

+ Das liegt an den Berechtigungen deines Kontos, nicht an deinem Passwort – + ein erneuter Login ändert daran nichts. Wenn du hier Zugriff brauchst, wende + dich bitte an dein E&P-Team. +

+ + Zurück zu MyE&P + +
+{% endblock %} diff --git a/tests/EventListener/AuthorizationCodeListenerTest.php b/tests/EventListener/AuthorizationCodeListenerTest.php new file mode 100644 index 0000000..60c8b4e --- /dev/null +++ b/tests/EventListener/AuthorizationCodeListenerTest.php @@ -0,0 +1,191 @@ +dispatch(null, $this->client(['ROLE_EMPLOYEE'])); + + $response = $event->getResponse(); + self::assertInstanceOf(RedirectResponse::class, $response); + self::assertSame(Response::HTTP_TEMPORARY_REDIRECT, $response->getStatusCode()); + self::assertFalse($event->getAuthorizationResolution()); + } + + public function testAccountHoldingOneOfTheRequiredRolesIsApproved(): void + { + $event = $this->dispatch( + $this->user([Role::EMPLOYEE]), + $this->client([Role::EMPLOYEE, Role::ADMIN]) + ); + + self::assertTrue($event->getAuthorizationResolution()); + self::assertNull($event->getResponse()); + } + + public function testAccountHoldingNoneOfTheRequiredRolesIsDenied(): void + { + $event = $this->dispatch( + $this->user([Role::CUSTOMER]), + $this->client([Role::EMPLOYEE, Role::ADMIN]) + ); + + self::assertFalse($event->getAuthorizationResolution()); + self::assertSame(Response::HTTP_FORBIDDEN, $event->getResponse()?->getStatusCode()); + } + + /** + * A CRM nomination grants nothing until an administrator approves it, so it must not + * open a client either. + */ + public function testPendingMarkerDoesNotSatisfyTheRequirement(): void + { + $event = $this->dispatch( + $this->user([Role::ADMIN.Role::PENDING_SUFFIX]), + $this->client([Role::ADMIN]) + ); + + self::assertFalse($event->getAuthorizationResolution()); + } + + /** + * The column starts empty, so "nobody configured this client" and "this client needs + * no roles" are the same value. Only one of the two readings is safe. + */ + public function testClientWithoutConfiguredRolesIsDenied(): void + { + $event = $this->dispatch($this->user([Role::ADMIN]), $this->client([])); + + self::assertFalse($event->getAuthorizationResolution()); + self::assertSame(Response::HTTP_FORBIDDEN, $event->getResponse()?->getStatusCode()); + } + + public function testClientOfAnotherImplementationIsDenied(): void + { + $foreign = $this->createStub(ClientInterface::class); + $foreign->method('getIdentifier')->willReturn('f3f10f8807e191c127d74cc9a247bf45'); + + $event = $this->dispatch($this->user([Role::ADMIN]), $foreign); + + self::assertFalse($event->getAuthorizationResolution()); + self::assertSame(Response::HTTP_FORBIDDEN, $event->getResponse()?->getStatusCode()); + } + + public function testExternalRequestLogsTheUserOutAfterApproval(): void + { + $security = $this->createMock(Security::class); + $security->method('getUser')->willReturn($this->user([Role::EMPLOYEE])); + $security->expects(self::once())->method('logout')->with(false); + + $this->dispatch( + $this->user([Role::EMPLOYEE]), + $this->client([Role::EMPLOYEE]), + external: true, + security: $security + ); + } + + /** + * A denied user has to stay signed in: they need a session to read the explanation and + * navigate away from it. + */ + public function testExternalRequestDoesNotLogTheUserOutAfterDenial(): void + { + $security = $this->createMock(Security::class); + $security->method('getUser')->willReturn($this->user([Role::CUSTOMER])); + $security->expects(self::never())->method('logout'); + + $this->dispatch( + $this->user([Role::CUSTOMER]), + $this->client([Role::EMPLOYEE]), + external: true, + security: $security + ); + } + + /** + * @param list $requiredRoles + */ + private function client(array $requiredRoles): OAuth2Client + { + return (new OAuth2Client('EPHub', 'f3f10f8807e191c127d74cc9a247bf45', 'secret')) + ->setRequiredRoles($requiredRoles); + } + + /** + * @param list $roles + */ + private function user(array $roles): User + { + return (new User('someone@example.org'))->setRoles($roles); + } + + private function dispatch( + ?UserInterface $user, + ClientInterface $client, + bool $external = false, + ?Security $security = null, + ): AuthorizationRequestResolveEvent { + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + $session->set('_oauth2', $external); + $request->setSession($session); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + if (null === $security) { + $security = $this->createStub(Security::class); + $security->method('getUser')->willReturn($user); + } + + $urlGenerator = $this->createStub(UrlGeneratorInterface::class); + $urlGenerator->method('generate')->willReturn('/'); + + $twig = $this->createStub(Environment::class); + $twig->method('render')->willReturn(''); + + $listener = new AuthorizationCodeListener( + $urlGenerator, + $requestStack, + $security, + $this->createStub(LoggerInterface::class), + $twig, + ); + + $event = new AuthorizationRequestResolveEvent( + $this->createStub(AuthorizationRequestInterface::class), + [], + $client, + $user ?? new User('someone@example.org'), + ); + + $listener->onAuthorizationRequestResolve($event); + + return $event; + } +}