From 91419294b8cea4d23a49de0d476d810cc8b033e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Wed, 23 Sep 2026 15:18:25 +0200 Subject: [PATCH] feat: harden the myep OAuth2 login --- .env | 20 ++- .env.dev | 6 + .env.test | 2 + config/packages/framework.yaml | 25 ++- config/packages/security.yaml | 16 ++ config/reference.php | 2 +- migrations/Version20260923090000.php | 37 +++++ src/Command/LockUserCommand.php | 86 ++++++++++ src/Controller/Security/LogoutController.php | 5 +- src/Entity/User.php | 73 +++++++++ src/Repository/UserRepository.php | 8 + src/Security/EpAuthenticator.php | 99 ++++++++++-- .../OAuth2/AuthorizationDeniedException.php | 31 ++++ .../OAuth2/AuthorizationRequestException.php | 9 +- src/Security/OAuth2/EpClient.php | 64 +++++++- src/Security/Role.php | 7 + src/Security/UserChecker.php | 54 +++++++ src/Service/AuditLogger.php | 21 +++ templates/layout.html.twig | 15 +- tests/Command/LockUserCommandTest.php | 88 +++++++++++ tests/Controller/DashboardControllerTest.php | 3 +- tests/Controller/Security/LogoutAuditTest.php | 6 +- tests/Security/EpAuthenticatorTest.php | 141 +++++++++++++++++ tests/Security/OAuth2/EpClientTest.php | 148 ++++++++++++++++++ tests/Security/UserCheckerTest.php | 82 ++++++++++ 25 files changed, 1013 insertions(+), 35 deletions(-) create mode 100644 migrations/Version20260923090000.php create mode 100644 src/Command/LockUserCommand.php create mode 100644 src/Security/OAuth2/AuthorizationDeniedException.php create mode 100644 src/Security/UserChecker.php create mode 100644 tests/Command/LockUserCommandTest.php create mode 100644 tests/Security/OAuth2/EpClientTest.php create mode 100644 tests/Security/UserCheckerTest.php diff --git a/.env b/.env index b75e064..51dea81 100644 --- a/.env +++ b/.env @@ -18,6 +18,15 @@ APP_ENV=dev APP_SECRET= APP_SHARE_DIR=var/share + +# The Host headers this application answers to, as a regular expression -- the OAuth2 +# redirect_uri is generated from the incoming request, so an unchecked Host header is what myep +# is told to send the authorization code to. Empty means every host is accepted; set it per +# environment, e.g. SYMFONY_TRUSTED_HOSTS='^ephub\.example\.com$'. +SYMFONY_TRUSTED_HOSTS= +# Only behind a TLS-terminating proxy: the proxy's address, a comma-separated list, or +# "private_ranges". Without it the generated redirect_uri of such a deployment comes out http://. +SYMFONY_TRUSTED_PROXIES= ###< symfony/framework-bundle ### ###> symfony/routing ### @@ -56,12 +65,11 @@ MAILER_DSN=null://null # https://ephub.ddev.site/auth/check for local dev, plus the production host. Leave the id and # secret empty until then -- the login page detects it and says so. # -# `email` is the address the account actually signed in with, and it is what the local `user` -# table is keyed on. It is not myep's account identifier -- that is the opaque, stable `sub` -# claim, which myep's consumer guide asks consumers to match on. Keying on email here is -# deliberate, at the price of a myep-side email change reading as a new account, so such a -# change has to be applied to the `user` table too. Do NOT use profile.communication.email: it -# is the first contact address on the BusPro record, shared by every account of the same person. +# The local `user` table is keyed on the `sub` claim -- opaque, stable, and unique per myep +# account -- so an address change on myep updates the row instead of creating a second one. +# `email` is the address the session actually signed in with and is kept as a display and +# contact value. Do NOT use profile.communication.email: it is the first contact address on the +# BusPro record, shared by every account of the same person. EP_OAUTH2_CLIENT_ID= EP_OAUTH2_CLIENT_SECRET= EP_OAUTH2_URL_AUTHORIZE=https://myep.ddev.site/authorize diff --git a/.env.dev b/.env.dev index 170ebe7..a5084c0 100644 --- a/.env.dev +++ b/.env.dev @@ -1,4 +1,10 @@ ###> symfony/framework-bundle ### +# A throwaway for local development, committed on purpose so a fresh checkout boots. The dev +# environment is never served: .env.dev and .env.test are both excluded from the deploy rsync, +# and production takes APP_SECRET from its own uncommitted .env.local. APP_SECRET=44660e8fcd421376ebe0f7a7d55e179c ###< symfony/framework-bundle ### + +# ddev serves the application under this name; anything else is refused. +SYMFONY_TRUSTED_HOSTS='^ephub\.ddev\.site$' diff --git a/.env.test b/.env.test index 64bd111..3ae2840 100644 --- a/.env.test +++ b/.env.test @@ -1,3 +1,5 @@ # define your env variables for the test env here KERNEL_CLASS='App\Kernel' +# Fixed and committed so the test suite is reproducible, and never served: .env.test is excluded +# from the deploy rsync and production takes APP_SECRET from its own uncommitted .env.local. APP_SECRET='$ecretf0rt3st' diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 3c3eca0..8040139 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -2,8 +2,31 @@ framework: secret: '%env(APP_SECRET)%' + # The OAuth2 redirect_uri is generated from the incoming request, so the Host and the + # X-Forwarded-* headers decide where myep is told to send the authorization code. Both lists + # read the standard SYMFONY_* variables Symfony already defaults to, and are spelled out here + # because leaving them implicit is what made them easy to overlook. Set SYMFONY_TRUSTED_HOSTS + # per environment -- an empty value means every Host header is accepted. + trusted_hosts: '%env(default::SYMFONY_TRUSTED_HOSTS)%' + # Only ever needed behind a TLS-terminating proxy. Without it the generated redirect_uri of + # such a deployment comes out as http://, and the flow breaks rather than downgrading quietly. + trusted_proxies: '%env(default::SYMFONY_TRUSTED_PROXIES)%' + trusted_headers: ['x-forwarded-for', 'x-forwarded-host', 'x-forwarded-proto', 'x-forwarded-port', 'x-forwarded-prefix'] + # Note that the session will be started ONLY if you read or write from it. - session: true + # + # Everything here is pinned rather than left to the default, because a session cookie is the + # whole of this application's authentication once the OAuth2 flow is over. cookie_secure and + # cookie_samesite happen to match the defaults; save_path does not, and it is the one that + # matters in production -- deploy.php shares var/sessions between releases, and without this + # sessions land in PHP's default directory and every release logs everybody out. + session: + handler_id: session.handler.native_file + save_path: '%kernel.project_dir%/var/sessions/%kernel.environment%' + name: epsession + cookie_secure: true + cookie_samesite: lax + cookie_httponly: true form: csrf_protection: diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 4dcbd31..68c2074 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -1,4 +1,9 @@ security: + # Symfony's default, stated because it is a decision: the session id is regenerated on a + # successful login, so a session id an attacker planted beforehand is not the one that ends + # up authenticated. + session_fixation_strategy: migrate + # No password_hashers: there are no passwords here. Authentication is OAuth2 against myep, # which owns the credentials; ephub only ever sees a userinfo response. @@ -19,9 +24,20 @@ security: entry_point: App\Security\LoginEntryPoint custom_authenticators: - App\Security\EpAuthenticator + # The local lockout. Roles follow myep on every login and a deleted row is simply + # provisioned again, so this flag is the only thing here that myep cannot undo. + user_checker: App\Security\UserChecker logout: path: app_logout target: app_login + # Without this any third-party page can log somebody out by embedding the URL. + # The header link is a form POST for the same reason. + enable_csrf: true + # Local only: there is no RP-initiated logout against myep and no token + # revocation, so this ends the ephub session and nothing else. It matters less + # than it usually would -- myep destroys its own session immediately after an + # external authorization request, so there is normally no SSO session left to + # end -- and the access token is gone either way, never having been stored. access_control: # The login surface itself has to stay reachable, or the entry point redirects in a loop. diff --git a/config/reference.php b/config/reference.php index 4605e65..17bad17 100644 --- a/config/reference.php +++ b/config/reference.php @@ -1293,7 +1293,7 @@ use Symfony\Component\Config\Loader\ParamConfigurator as Param; * lifetime?: int|Param, // Default: 31536000 * path?: scalar|Param|null, // Default: "/" * domain?: scalar|Param|null, // Default: null - * secure?: true|false|"auto"|Param, // Default: "auto" + * secure?: true|false|"auto"|Param, // Default: true * httponly?: bool|Param, // Default: true * samesite?: null|"lax"|"strict"|"none"|Param, // Default: "lax" * always_remember_me?: bool|Param, // Default: false diff --git a/migrations/Version20260923090000.php b/migrations/Version20260923090000.php new file mode 100644 index 0000000..21d43a5 --- /dev/null +++ b/migrations/Version20260923090000.php @@ -0,0 +1,37 @@ +addSql('ALTER TABLE `user` ADD sub VARCHAR(64) DEFAULT NULL, ADD disabled_at DATETIME DEFAULT NULL, ADD disabled_reason VARCHAR(255) DEFAULT NULL'); + $this->addSql('CREATE UNIQUE INDEX uniq_user_sub ON `user` (sub)'); + + // Existing rows keep a NULL sub and are matched on their address once more, on their next + // login, which is where the column is backfilled. The unique index tolerates that: NULL + // is not equal to NULL in MySQL, so any number of rows may be waiting for their first + // login at once. + + // Addresses were stored as they arrived, and the new matching normalises before it + // compares -- a row left in mixed case would never be found again. + $this->addSql('UPDATE `user` SET email = LOWER(TRIM(email))'); + } + + public function down(Schema $schema): void + { + $this->addSql('DROP INDEX uniq_user_sub ON `user`'); + $this->addSql('ALTER TABLE `user` DROP sub, DROP disabled_at, DROP disabled_reason'); + } +} diff --git a/src/Command/LockUserCommand.php b/src/Command/LockUserCommand.php new file mode 100644 index 0000000..243026c --- /dev/null +++ b/src/Command/LockUserCommand.php @@ -0,0 +1,86 @@ +addArgument('email', InputArgument::REQUIRED, 'The address of the account') + ->addArgument('reason', InputArgument::OPTIONAL, 'Shown to the person on their next login attempt') + ->addOption('release', null, InputOption::VALUE_NONE, 'Lets the account sign in again'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + // Matched the way a login matches it, or an address typed in a different case would + // report "no account" for a row that is plainly there. + $email = mb_strtolower(trim((string) $input->getArgument('email'))); + $user = $this->users->findOneByEmail($email); + + if (!$user instanceof User) { + $io->error(\sprintf('No account for %s.', $email)); + + return Command::FAILURE; + } + + if ($input->getOption('release')) { + $user->enable(); + $this->em->flush(); + $this->audit->accountUnlocked($user); + + $io->success(\sprintf('%s may sign in again.', $email)); + + return Command::SUCCESS; + } + + $reason = $input->getArgument('reason'); + $reason = \is_string($reason) && '' !== trim($reason) ? trim($reason) : null; + + $user->disable($reason); + $this->em->flush(); + $this->audit->accountLocked($user, $reason); + + // The lock is applied at authentication, so a session opened before it survives until it + // expires. Said out loud rather than left to be discovered. + $io->success(\sprintf('%s is locked out. A session opened before now stays valid until it expires.', $email)); + + return Command::SUCCESS; + } +} diff --git a/src/Controller/Security/LogoutController.php b/src/Controller/Security/LogoutController.php index cc4d4e2..b17a24c 100644 --- a/src/Controller/Security/LogoutController.php +++ b/src/Controller/Security/LogoutController.php @@ -11,8 +11,11 @@ final class LogoutController extends AbstractController { /** * Never executed: the firewall's logout listener intercepts this route. + * + * POST only, and CSRF-protected on the firewall: a logout that a GET could trigger is one + * any third-party page can trigger by embedding the URL. */ - #[Route('/logout', name: 'app_logout', methods: ['GET'])] + #[Route('/logout', name: 'app_logout', methods: ['POST'])] public function index(): never { throw new \LogicException('Intercepted by the logout key on the main firewall.'); diff --git a/src/Entity/User.php b/src/Entity/User.php index 10af1d1..515194a 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -18,6 +18,7 @@ use Symfony\Component\Validator\Constraints as Assert; #[ORM\Entity(repositoryClass: UserRepository::class)] #[ORM\Table(name: '`user`')] #[ORM\UniqueConstraint(name: 'uniq_user_email', columns: ['email'])] +#[ORM\UniqueConstraint(name: 'uniq_user_sub', columns: ['sub'])] class User implements UserInterface { #[ORM\Id] @@ -25,6 +26,20 @@ class User implements UserInterface #[ORM\Column] private ?int $id = null; + /** + * myep's account identifier: opaque, stable, and the only claim that identifies an account. + * Nullable because rows provisioned before it was stored have none -- those are matched on + * their email once more and backfilled on that login, so the column empties out by itself. + */ + #[ORM\Column(length: 64, nullable: true)] + private ?string $sub = null; + + /** + * The address this session authenticated with, normalised to lower case. Distinct per myep + * account and kept up to date from the claim, but a display and contact value rather than + * the key -- one person can hold several myep accounts, each with its own address and its + * own roles. + */ #[ORM\Column(length: 180)] #[Assert\NotBlank] #[Assert\Email] @@ -42,12 +57,34 @@ class User implements UserInterface #[ORM\Column] private array $roles = []; + /** + * When the account was locked out here, or null while it may sign in. + * + * Local and deliberately not derived from anything myep says: it is the only lever this + * application has over its own access, and a login must not be able to clear it. + */ + #[ORM\Column(nullable: true)] + private ?\DateTimeImmutable $disabledAt = null; + + #[ORM\Column(length: 255, nullable: true)] + private ?string $disabledReason = null; + public function __construct(string $email, string $name) { $this->email = $email; $this->name = $name; } + public function getSub(): ?string + { + return $this->sub; + } + + public function setSub(?string $sub): void + { + $this->sub = $sub; + } + public function getId(): ?int { return $this->id; @@ -58,6 +95,11 @@ class User implements UserInterface return $this->email; } + public function setEmail(string $email): void + { + $this->email = $email; + } + public function getName(): string { return $this->name; @@ -89,6 +131,37 @@ class User implements UserInterface $this->roles = $roles; } + public function isDisabled(): bool + { + return null !== $this->disabledAt; + } + + public function getDisabledAt(): ?\DateTimeImmutable + { + return $this->disabledAt; + } + + public function getDisabledReason(): ?string + { + return $this->disabledReason; + } + + /** + * Re-locking an already locked account leaves the original timestamp alone: when the lockout + * started is the part worth keeping. + */ + public function disable(?string $reason = null): void + { + $this->disabledAt ??= new \DateTimeImmutable(); + $this->disabledReason = $reason; + } + + public function enable(): void + { + $this->disabledAt = null; + $this->disabledReason = null; + } + public function eraseCredentials(): void { } diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index ec1c4b6..24d1052 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -22,4 +22,12 @@ class UserRepository extends ServiceEntityRepository { return $this->findOneBy(['email' => $email]); } + + /** + * The account myep's `sub` claim names, if this application has met it before. + */ + public function findOneBySub(string $sub): ?User + { + return $this->findOneBy(['sub' => $sub]); + } } diff --git a/src/Security/EpAuthenticator.php b/src/Security/EpAuthenticator.php index cdd97ee..eb69d5b 100644 --- a/src/Security/EpAuthenticator.php +++ b/src/Security/EpAuthenticator.php @@ -6,6 +6,7 @@ namespace App\Security; use App\Entity\User; use App\Repository\UserRepository; +use App\Security\OAuth2\AuthorizationDeniedException; use App\Security\OAuth2\AuthorizationRequestException; use App\Security\OAuth2\EpClient; use App\Security\OAuth2\UserinfoUnavailableException; @@ -56,6 +57,17 @@ class EpAuthenticator extends AbstractAuthenticator { try { $accessToken = $this->client->fetchAccessToken($request); + } catch (AuthorizationDeniedException $e) { + // myep said why, so the person gets that instead of a generic failure. Caught before + // its parent: every other broken callback still falls into the branch below. + $this->logger->error('Login via myep refused by the authorization server', ['exception' => $e]); + $this->audit->signInRefused('access_denied' === $e->error ? 'authorization_denied' : 'authorization_error'); + + throw new CustomUserMessageAuthenticationException( + 'access_denied' === $e->error + ? 'Die Anmeldung über myep wurde abgebrochen.' + : 'Anmeldung über myep fehlgeschlagen.', + ); } catch (AuthorizationRequestException|IdentityProviderException $e) { $this->logger->error('Login via myep failed: no access token', ['exception' => $e]); // Audited here rather than from a failure event: this is the only point that still @@ -75,22 +87,61 @@ class EpAuthenticator extends AbstractAuthenticator throw new CustomUserMessageAuthenticationException('Anmeldung über myep fehlgeschlagen.'); } - // `email` is the address this session actually authenticated with, and it is what User - // is keyed on. It is not myep's account identifier -- that is the opaque, stable `sub` - // claim, which myep's consumer guide asks consumers to match on; keying on email here - // is deliberate, at the price of a myep-side email change reading as a new account. - // Never profile.communication.email: that is the first contact address on the BusPro - // record, shared by every account of the same person. myep patches this claim from the + // `email` is the address this session actually authenticated with -- a display and + // contact value, kept up to date but not the account key. Never + // profile.communication.email: that is the first contact address on the BusPro record, + // shared by every account of the same person. myep patches this claim from the // authenticated account and exports no fallback, so an absent one is a myep-side fault // rather than an ungranted scope -- refused either way. $email = $userinfo['email'] ?? null; - if (!\is_string($email) || '' === $email) { + if (!\is_string($email) || '' === trim($email)) { $this->logger->error('Login via myep failed: userinfo carried no email claim'); $this->audit->signInRefused('no_email_claim'); throw new CustomUserMessageAuthenticationException('Anmeldung über myep fehlgeschlagen.'); } + // Normalised before it is compared with or written to anything: the unique index is + // byte-exact, so Anna@ep-reisen.de and anna@ep-reisen.de would otherwise become two + // accounts for one person. Same as the sibling `myep-team` client does. + $email = mb_strtolower(trim($email)); + + // `sub` is what actually identifies the account: opaque, stable, and unique per myep + // account. One person can hold several -- BusPro accepts any address on their record as + // a login and each one is its own account with its own roles -- so the same human + // signing in privately and with their @ep-reisen.de address is two accounts by design, + // and an address change on myep must not read as a third. + $sub = $userinfo['sub'] ?? null; + $sub = \is_string($sub) && '' !== $sub ? $sub : null; + + $user = $this->findLocal($sub, $email); + + // The address belongs to a row that is a different myep account. Only reachable from a + // stale local address -- myep gives each account its own -- and provisioning would hit + // the unique index and fail as a 500. Refused instead, so the trail names the two rows + // and an administrator can correct the stale one. + if (null === $user && null !== $conflict = $this->users->findOneByEmail($email)) { + $this->logger->error('Login via myep refused: the email claim belongs to another account', [ + 'email' => $email, + 'sub' => $sub, + 'conflicting_user_id' => $conflict->getId(), + ]); + $this->audit->signInRefused('email_belongs_to_another_account', $email); + + throw new CustomUserMessageAuthenticationException('Anmeldung über myep fehlgeschlagen.'); + } + + // A locked-out account is returned untouched -- no name, no email, no roles, nothing + // written back -- so that myep cannot undo a local lockout by reporting the person as + // eligible again. The UserChecker on the firewall refuses the login and says why. + if (null !== $user && $user->isDisabled()) { + $this->logger->info('Login via myep refused: account disabled locally', ['email' => $email]); + + return new SelfValidatingPassport( + new UserBadge($user->getUserIdentifier(), static fn (): User => $user), + ); + } + $roles = Role::eligibleOnly($this->claimedRoles($userinfo)); if ([] === $roles) { $this->logger->info('Login via myep refused: no eligible role', ['email' => $email]); @@ -101,7 +152,7 @@ class EpAuthenticator extends AbstractAuthenticator ); } - $user = $this->createOrUpdate($email, $this->nameFrom($userinfo, $email), $roles); + $user = $this->createOrUpdate($user, $sub, $email, $this->nameFrom($userinfo, $email), $roles); // Resolved here rather than by the user provider: the account may have just been // created in this very request, and handing the provider an email would mean a second @@ -162,12 +213,31 @@ class EpAuthenticator extends AbstractAuthenticator return mb_substr($fallback, 0, 100); } + /** + * The account this login belongs to, if ephub has met it before. + * + * `sub` first. The email fallback is for rows provisioned before the column existed: they + * carry no `sub` yet, and matching them once more on their address is what lets the backfill + * in createOrUpdate() reach them. It can go once every account has signed in once -- by then + * every row carries a `sub`, and a row that does is deliberately not matched this way, + * because a shared address would mean it belongs to somebody else's account. + */ + private function findLocal(?string $sub, string $email): ?User + { + if (null !== $sub && null !== $user = $this->users->findOneBySub($sub)) { + return $user; + } + + $user = $this->users->findOneByEmail($email); + + return null !== $user && null === $user->getSub() ? $user : null; + } + /** * @param list $roles */ - private function createOrUpdate(string $email, string $name, array $roles): User + private function createOrUpdate(?User $user, ?string $sub, string $email, string $name, array $roles): User { - $user = $this->users->findOneBy(['email' => $email]); $isNew = null === $user; if (null === $user) { @@ -175,6 +245,15 @@ class EpAuthenticator extends AbstractAuthenticator $this->em->persist($user); } else { $user->setName($name); + // Follows myep: the address on the account is whichever one this session signed in + // with, and on a row matched by `sub` that may be a new one. + $user->setEmail($email); + } + + // Backfills a row that predates the column, and is a no-op on every later login. Never + // clears one: a userinfo response that dropped the claim must not un-key an account. + if (null !== $sub) { + $user->setSub($sub); } // Filtered the same way the claim was, so the comparison below is between two sets of diff --git a/src/Security/OAuth2/AuthorizationDeniedException.php b/src/Security/OAuth2/AuthorizationDeniedException.php new file mode 100644 index 0000000..66f9372 --- /dev/null +++ b/src/Security/OAuth2/AuthorizationDeniedException.php @@ -0,0 +1,31 @@ +error, + null === $this->errorDescription ? '' : ' ('.$this->errorDescription.')', + )); + } +} diff --git a/src/Security/OAuth2/AuthorizationRequestException.php b/src/Security/OAuth2/AuthorizationRequestException.php index 8cb7cd7..87bea15 100644 --- a/src/Security/OAuth2/AuthorizationRequestException.php +++ b/src/Security/OAuth2/AuthorizationRequestException.php @@ -5,9 +5,12 @@ declare(strict_types=1); namespace App\Security\OAuth2; /** - * The callback from myep was not a usable authorization response -- no code, or a state that - * does not match the one this session started with. + * The callback from myep was not a usable authorization response -- no code, a state that does + * not match the one this session started with, or a missing PKCE verifier. + * + * Not final: AuthorizationDeniedException is the one case that carries a reason worth telling + * the user, and every existing catch of this class is meant to keep covering it. */ -final class AuthorizationRequestException extends \RuntimeException +class AuthorizationRequestException extends \RuntimeException { } diff --git a/src/Security/OAuth2/EpClient.php b/src/Security/OAuth2/EpClient.php index 4c1bd2a..8c25dd2 100644 --- a/src/Security/OAuth2/EpClient.php +++ b/src/Security/OAuth2/EpClient.php @@ -29,6 +29,9 @@ class EpClient */ public function __construct( private readonly UrlGeneratorInterface $urlGenerator, + // The client secret rides in here. Marked sensitive so that it is redacted from stack + // traces rather than printed in full on an exception page or in a log. + #[\SensitiveParameter] array $options, ) { $this->config = $this->resolveConfig($options); @@ -44,14 +47,20 @@ class EpClient } /** - * The URL to send the browser to, having stored the state this session expects back. + * The URL to send the browser to, having stored the state and the PKCE verifier this session + * expects back. */ public function authorizationUrl(Request $request): string { $provider = $this->getProvider(); $url = $provider->getAuthorizationUrl(); - $request->getSession()->set('oauth2state', $provider->getState()); + $session = $request->getSession(); + $session->set('oauth2state', $provider->getState()); + // Generated by getAuthorizationUrl() together with the S256 challenge that just went out + // in the URL, and replayed on the token request. Held on the session rather than + // anywhere the browser can reach, which is the whole point of the exercise. + $session->set('oauth2pkce', $provider->getPkceCode()); return $url; } @@ -61,8 +70,10 @@ class EpClient * * The state check is the CSRF protection of the authorization code flow: without it an * attacker can hand a victim a callback URL carrying the attacker's code and have the - * victim's session bound to the attacker's identity. The stored state is consumed either - * way, so replaying the same callback cannot succeed twice. + * victim's session bound to the attacker's identity. PKCE binds the code a second time, to a + * secret that never left this session -- unlike the redirect URI, which is generated from + * the request's own Host header. The state and the verifier are consumed either way, so + * replaying the same callback cannot succeed twice. * * @throws AuthorizationRequestException */ @@ -70,18 +81,42 @@ class EpClient { $session = $request->getSession(); $expectedState = $session->get('oauth2state'); + $verifier = $session->get('oauth2pkce'); $session->remove('oauth2state'); + $session->remove('oauth2pkce'); + + // Before everything else: myep explained why it is not sending a code, and any later + // check would report the wrong cause. + $error = $request->query->get('error'); + if (\is_string($error) && '' !== $error) { + $description = $request->query->get('error_description'); + + throw new AuthorizationDeniedException($error, \is_string($description) && '' !== $description ? $description : null); + } $code = $request->query->get('code'); if (null === $code || '' === $code) { throw new AuthorizationRequestException('The callback from myep carried no authorization code.'); } - if (null === $expectedState || $request->query->get('state') !== $expectedState) { + // hash_equals rather than !==: the value is one the caller already knows, so the timing + // channel is worth nothing, but a comparison that guards a session is not the place to + // keep the habit of using the fast one. + if (!\is_string($expectedState) || !\is_string($state = $request->query->get('state')) || !hash_equals($expectedState, $state)) { throw new AuthorizationRequestException('The state parameter does not match the one this session started with.'); } - return $this->getProvider()->getAccessToken('authorization_code', ['code' => $code]); + // A challenge went out with the authorization request, so myep will reject an exchange + // without the verifier. Refused here instead, where the reason is still known -- the + // usual cause is a session that was replaced between the two legs of the flow. + if (!\is_string($verifier) || '' === $verifier) { + throw new AuthorizationRequestException('This session no longer holds the PKCE verifier the authorization request was made with.'); + } + + $provider = $this->getProvider(); + $provider->setPkceCode($verifier); + + return $provider->getAccessToken('authorization_code', ['code' => $code]); } /** @@ -110,12 +145,25 @@ class EpClient throw new UserinfoUnavailableException('The userinfo endpoint could not be reached.', previous: $e); } + $status = $response->getStatusCode(); + if ($status < 200 || $status >= 300) { + throw new UserinfoUnavailableException(\sprintf('The userinfo endpoint answered with HTTP %d.', $status)); + } + $userinfo = json_decode((string) $response->getBody(), true); if (!\is_array($userinfo)) { throw new UserinfoUnavailableException('The userinfo endpoint returned an unreadable payload.'); } + // myep's consumer guide warns that several of its endpoints answer `{"error": ..., + // "type": "api_error"}` with HTTP 200, so the status alone does not settle it. Without + // this an error body travels on and is refused further down for carrying no email + // claim, which names the wrong cause. + if (isset($userinfo['error'])) { + throw new UserinfoUnavailableException('The userinfo endpoint returned an error body.'); + } + return $userinfo; } @@ -131,6 +179,10 @@ class EpClient 'scopes' => $this->config['scopes'], // myep's server expects space-separated scopes, not the library's default comma. 'scopeSeparator' => ' ', + // The library does the whole round trip from here: getAuthorizationUrl() generates + // the verifier and sends the S256 challenge, getAccessToken() replays the verifier + // that authorizationUrl() put on the session. + 'pkceMethod' => AbstractProvider::PKCE_METHOD_S256, ]); } diff --git a/src/Security/Role.php b/src/Security/Role.php index 7548df4..169c1df 100644 --- a/src/Security/Role.php +++ b/src/Security/Role.php @@ -28,6 +28,13 @@ final class Role * at login rather than let in without access, so they get an explanation instead of an * empty screen they cannot act on. * + * **Keep this in step with `required_roles` on ephub's `oauth2_client` row on myep.** myep + * gates its own authorization endpoint on the same list and shows an explanation page + * instead of redirecting back here, so the two are one policy written down twice: widen one + * alone and somebody is either refused on myep with nothing this side can explain, or let + * through to an ephub that refuses them. The refusal below stays either way -- it is + * defence in depth now rather than the first line, and in practice it stops firing. + * * @var list */ public const ELIGIBLE = [ diff --git a/src/Security/UserChecker.php b/src/Security/UserChecker.php new file mode 100644 index 0000000..54e58d3 --- /dev/null +++ b/src/Security/UserChecker.php @@ -0,0 +1,54 @@ +isDisabled()) { + return; + } + + // The authenticator audits the refusals it can see; this one happens after it has handed + // the passport over, so it is recorded here. + $this->audit->signInRefused('account_disabled', $user->getEmail()); + + $reason = $user->getDisabledReason(); + + throw new CustomUserMessageAccountStatusException( + null === $reason || '' === $reason + ? 'Dieser Zugang wurde gesperrt. Bitte wende dich an die Administration.' + : 'Dieser Zugang wurde gesperrt: '.$reason, + ); + } + + public function checkPostAuth(UserInterface $user, ?TokenInterface $token = null): void + { + } +} diff --git a/src/Service/AuditLogger.php b/src/Service/AuditLogger.php index 02df964..33fb9b8 100644 --- a/src/Service/AuditLogger.php +++ b/src/Service/AuditLogger.php @@ -166,6 +166,27 @@ final readonly class AuditLogger ]); } + /** + * The local lockout, set and released from the console. myep is not involved and cannot undo + * it, which is exactly why it is worth a record: it is the only access decision this + * application makes on its own. The actor reads as "system" -- a console command has no + * token -- so the reason is what identifies the act. + */ + public function accountLocked(User $user, ?string $reason): void + { + $this->logger->info('User account locked.', [ + 'account' => $user->getEmail(), + 'reason' => $reason, + ]); + } + + public function accountUnlocked(User $user): void + { + $this->logger->info('User account unlocked.', [ + 'account' => $user->getEmail(), + ]); + } + public function resourceCreated(Resource $resource): void { $this->logger->info('Resource created.', $this->resource($resource)); diff --git a/templates/layout.html.twig b/templates/layout.html.twig index 40c92a3..812f9cb 100644 --- a/templates/layout.html.twig +++ b/templates/layout.html.twig @@ -15,11 +15,16 @@ {% if app.user %}
- - {{ ux_icon('lucide:log-out', { class: 'h-5 w-5 shrink-0 text-gray-400' }) }} - - + {# A form rather than a link: the firewall's logout is CSRF-protected, so any + third-party page embedding the URL gets nowhere. #} +
+ + +
{% endif %} diff --git a/tests/Command/LockUserCommandTest.php b/tests/Command/LockUserCommandTest.php new file mode 100644 index 0000000..011b2f9 --- /dev/null +++ b/tests/Command/LockUserCommandTest.php @@ -0,0 +1,88 @@ +em = self::getContainer()->get(EntityManagerInterface::class); + $this->em->getConnection()->beginTransaction(); + $this->command = new CommandTester((new Application(self::$kernel))->find('app:user:lock')); + } + + protected function tearDown(): void + { + $this->em->getConnection()->rollBack(); + parent::tearDown(); + } + + public function testAnAccountIsLockedWithItsReason(): void + { + $user = $this->user(); + + $this->command->execute(['email' => 'anna@ep-reisen.de', 'reason' => 'Auf eigenen Wunsch']); + + $this->command->assertCommandIsSuccessful(); + self::assertTrue($user->isDisabled()); + self::assertSame('Auf eigenen Wunsch', $user->getDisabledReason()); + } + + public function testTheLockIsReleasedAgain(): void + { + $user = $this->user(); + $user->disable('Auf eigenen Wunsch'); + $this->em->flush(); + + $this->command->execute(['email' => 'anna@ep-reisen.de', '--release' => true]); + + $this->command->assertCommandIsSuccessful(); + self::assertFalse($user->isDisabled()); + self::assertNull($user->getDisabledReason()); + } + + /** + * Addresses are stored normalised, so a differently cased argument has to find the row + * rather than report that there is none. + */ + public function testTheAddressIsMatchedTheWayALoginMatchesIt(): void + { + $user = $this->user(); + + $this->command->execute(['email' => ' Anna@EP-Reisen.de ']); + + $this->command->assertCommandIsSuccessful(); + self::assertTrue($user->isDisabled()); + } + + public function testAnUnknownAddressFails(): void + { + self::assertSame(1, $this->command->execute(['email' => 'niemand@ep-reisen.de'])); + self::assertStringContainsString('No account', $this->command->getDisplay()); + } + + private function user(): User + { + $user = new User('anna@ep-reisen.de', 'Anna'); + $user->setRoles(['ROLE_EMPLOYEE']); + $this->em->persist($user); + $this->em->flush(); + + return $user; + } +} diff --git a/tests/Controller/DashboardControllerTest.php b/tests/Controller/DashboardControllerTest.php index 02dfa23..a231b2c 100644 --- a/tests/Controller/DashboardControllerTest.php +++ b/tests/Controller/DashboardControllerTest.php @@ -70,7 +70,8 @@ class DashboardControllerTest extends WebTestCase $crawler = $this->client->request('GET', '/'); self::assertCount(0, $crawler->filter('#mobilenav')); - self::assertCount(1, $crawler->filter('a[href="/logout"]')); + // The way out is a form, not a link: the firewall's logout is CSRF-protected. + self::assertCount(1, $crawler->filter('form[action="/logout"]')); } public function testATileCountsWhatIsStillFreeToday(): void diff --git a/tests/Controller/Security/LogoutAuditTest.php b/tests/Controller/Security/LogoutAuditTest.php index a95ce96..1fdf4e5 100644 --- a/tests/Controller/Security/LogoutAuditTest.php +++ b/tests/Controller/Security/LogoutAuditTest.php @@ -51,7 +51,11 @@ class LogoutAuditTest extends WebTestCase $this->em->flush(); $this->client->loginUser($user); - $this->client->request('GET', '/logout'); + + // Submitted from the page rather than requested directly: the firewall's logout is + // CSRF-protected, so the token in the header form is part of what is being pinned down. + $crawler = $this->client->request('GET', '/'); + $this->client->submit($crawler->filter('form[action$="/logout"]')->form()); self::assertResponseRedirects(); diff --git a/tests/Security/EpAuthenticatorTest.php b/tests/Security/EpAuthenticatorTest.php index dc544c7..8783c57 100644 --- a/tests/Security/EpAuthenticatorTest.php +++ b/tests/Security/EpAuthenticatorTest.php @@ -7,6 +7,7 @@ namespace App\Tests\Security; use App\Entity\User; use App\Repository\UserRepository; use App\Security\EpAuthenticator; +use App\Security\OAuth2\AuthorizationDeniedException; use App\Security\OAuth2\AuthorizationRequestException; use App\Security\OAuth2\EpClient; use App\Security\OAuth2\UserinfoUnavailableException; @@ -281,6 +282,146 @@ class EpAuthenticatorTest extends KernelTestCase self::assertFalse($authenticator->supports($dashboard)); } + /** + * `sub` is the account key. An address change on myep has to land on the existing row -- + * keying on the address would leave the old one orphaned with all its bookings and hand the + * person an empty account with no explanation. + */ + public function testAnAccountIsMatchedOnSubWhenTheEmailChanges(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Berger'); + $existing->setSub('4711'); + $existing->setRoles([Role::EMPLOYEE]); + $this->em->persist($existing); + $this->em->flush(); + + $user = $this->authenticate([ + 'sub' => '4711', + 'email' => 'anna.berger@ep-reisen.de', + 'roles' => ['ROLE_EMPLOYEE'], + ]); + + self::assertSame($existing->getId(), $user->getId()); + self::assertSame('anna.berger@ep-reisen.de', $user->getEmail()); + } + + /** + * Two myep accounts for one person -- the private address and the company one -- are + * separate by design, and hold different roles. + */ + public function testADifferentSubIsADifferentAccount(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Berger'); + $existing->setSub('4711'); + $existing->setRoles([Role::EMPLOYEE]); + $this->em->persist($existing); + $this->em->flush(); + + $user = $this->authenticate(['sub' => '4712', 'email' => 'anna@privat.de', 'roles' => ['ROLE_ADMIN']]); + + self::assertNotSame($existing->getId(), $user->getId()); + self::assertSame('4712', $user->getSub()); + } + + /** + * Rows provisioned before the column existed carry no sub. They are matched on their address + * once more, and the backfill is what retires that fallback. + */ + public function testARowFromBeforeTheSubColumnIsMatchedOnEmailAndBackfilled(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Berger'); + $existing->setRoles([Role::EMPLOYEE]); + $this->em->persist($existing); + $this->em->flush(); + + $user = $this->authenticate(['sub' => '4711', 'email' => 'anna@ep-reisen.de', 'roles' => ['ROLE_EMPLOYEE']]); + + self::assertSame($existing->getId(), $user->getId()); + self::assertSame('4711', $user->getSub()); + } + + /** + * The unique index is byte-exact, so an unnormalised claim would make a second account for + * somebody who already has one. + */ + public function testTheEmailClaimIsNormalisedBeforeItIsMatched(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Berger'); + $existing->setRoles([Role::EMPLOYEE]); + $this->em->persist($existing); + $this->em->flush(); + + $user = $this->authenticate(['email' => ' Anna@EP-Reisen.de ', 'roles' => ['ROLE_EMPLOYEE']]); + + self::assertSame($existing->getId(), $user->getId()); + self::assertSame('anna@ep-reisen.de', $user->getEmail()); + } + + /** + * Only reachable from a stale local address. Provisioning would hit the unique index and + * fail as a 500, so it is refused with a record naming both rows instead. + */ + public function testAnEmailHeldByAnotherAccountIsRefused(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Berger'); + $existing->setSub('4711'); + $existing->setRoles([Role::EMPLOYEE]); + $this->em->persist($existing); + $this->em->flush(); + + try { + $this->authenticate(['sub' => '4712', 'email' => 'anna@ep-reisen.de', 'roles' => ['ROLE_EMPLOYEE']]); + self::fail('Expected the login to be refused.'); + } catch (CustomUserMessageAuthenticationException) { + } + + self::assertSame('email_belongs_to_another_account', $this->audited('Sign-in refused.')->context['reason']); + } + + /** + * The point of the local lockout: myep still reports the person as eligible, and nothing it + * says may reach the row. The refusal itself is the UserChecker's, on the firewall. + */ + public function testALockedOutAccountIsReturnedUntouched(): void + { + $existing = new User('anna@ep-reisen.de', 'Anna Alt'); + $existing->setSub('4711'); + $existing->setRoles([Role::EMPLOYEE]); + $existing->disable('Auf eigenen Wunsch'); + $this->em->persist($existing); + $this->em->flush(); + + $user = $this->authenticate([ + 'sub' => '4711', + 'email' => 'anna@ep-reisen.de', + 'roles' => ['ROLE_EMPLOYEE', 'ROLE_ADMIN'], + 'profile' => ['first_name' => 'Anna', 'last_name' => 'Neu'], + ]); + + self::assertSame($existing->getId(), $user->getId()); + self::assertTrue($user->isDisabled()); + self::assertSame('Anna Alt', $user->getName()); + self::assertNotContains(Role::ADMIN, $user->getRoles()); + } + + /** + * myep explained why it is not sending a code. Reporting that as "carried no authorization + * code" would tell the person something untrue and lose the reason -- and it is what keeps + * myep on an explanation page of its own instead of the spec's redirect. + */ + public function testARefusedAuthorizationSaysSoRatherThanBlamingTheCallback(): void + { + try { + $this->authenticator(throwOnToken: new AuthorizationDeniedException('access_denied')) + ->authenticate(new Request()); + self::fail('Expected the login to be refused.'); + } catch (CustomUserMessageAuthenticationException $e) { + self::assertStringContainsString('abgebrochen', $e->getMessage()); + } + + self::assertSame('authorization_denied', $this->audited('Sign-in refused.')->context['reason']); + } + /** * @param array $userinfo */ diff --git a/tests/Security/OAuth2/EpClientTest.php b/tests/Security/OAuth2/EpClientTest.php new file mode 100644 index 0000000..7863ede --- /dev/null +++ b/tests/Security/OAuth2/EpClientTest.php @@ -0,0 +1,148 @@ +request(); + $url = $this->client()->authorizationUrl($request); + + parse_str((string) parse_url($url, \PHP_URL_QUERY), $query); + + self::assertSame('S256', $query['code_challenge_method'] ?? null); + self::assertNotEmpty($query['code_challenge'] ?? ''); + + // The verifier itself never leaves the session -- what went out is its hash. + $verifier = $request->getSession()->get('oauth2pkce'); + self::assertIsString($verifier); + self::assertSame( + rtrim(strtr(base64_encode(hash('sha256', $verifier, true)), '+/', '-_'), '='), + $query['code_challenge'], + ); + self::assertSame($query['state'] ?? null, $request->getSession()->get('oauth2state')); + } + + /** + * A callback that myep explained. Reporting it as a missing code would tell the person + * something untrue and lose the reason. + */ + public function testARefusedAuthorizationIsReportedAsSuch(): void + { + $request = $this->callbackRequest(['error' => 'access_denied', 'error_description' => 'Nope.']); + + try { + $this->client()->fetchAccessToken($request); + self::fail('Expected the callback to be refused.'); + } catch (AuthorizationDeniedException $e) { + self::assertSame('access_denied', $e->error); + self::assertSame('Nope.', $e->errorDescription); + } + } + + public function testAMismatchedStateIsRefused(): void + { + $this->expectException(AuthorizationRequestException::class); + $this->expectExceptionMessage('state parameter'); + + $this->client()->fetchAccessToken($this->callbackRequest(['code' => 'c', 'state' => 'somebody else\'s'])); + } + + /** + * The classic "no state in session means pass" bug. + */ + public function testACallbackIntoASessionThatStartedNoFlowIsRefused(): void + { + $this->expectException(AuthorizationRequestException::class); + + $request = $this->callbackRequest(['code' => 'c', 'state' => 'anything'], state: null); + + $this->client()->fetchAccessToken($request); + } + + public function testACallbackWithoutTheVerifierIsRefusedRatherThanExchanged(): void + { + $this->expectException(AuthorizationRequestException::class); + $this->expectExceptionMessage('PKCE verifier'); + + $this->client()->fetchAccessToken($this->callbackRequest(['code' => 'c', 'state' => 'st'], verifier: null)); + } + + /** + * Both are consumed whatever the outcome, so the same callback cannot be replayed. + */ + public function testTheStateAndTheVerifierAreConsumedEvenOnAFailedCallback(): void + { + $request = $this->callbackRequest(['state' => 'st']); + + try { + $this->client()->fetchAccessToken($request); + } catch (AuthorizationRequestException) { + } + + self::assertFalse($request->getSession()->has('oauth2state')); + self::assertFalse($request->getSession()->has('oauth2pkce')); + } + + private function client(): EpClient + { + $urls = $this->createStub(UrlGeneratorInterface::class); + $urls->method('generate')->willReturn('https://ephub.test/auth/check'); + + return new EpClient($urls, [ + 'client_id' => 'id', + 'client_secret' => 'secret', + 'url_authorize' => 'https://myep.test/authorize', + 'url_access_token' => 'https://myep.test/token', + 'url_userinfo' => 'https://myep.test/api/userinfo', + 'scopes' => ['email', 'roles'], + ]); + } + + private function request(): Request + { + $request = new Request(); + $request->setSession(new Session(new MockArraySessionStorage())); + + return $request; + } + + /** + * A callback as it arrives from myep, into a session that started the flow. + * + * @param array $query + */ + private function callbackRequest(array $query, ?string $state = 'st', ?string $verifier = 'verifier'): Request + { + $request = $this->request(); + $request->query->replace($query); + + if (null !== $state) { + $request->getSession()->set('oauth2state', $state); + } + + if (null !== $verifier) { + $request->getSession()->set('oauth2pkce', $verifier); + } + + return $request; + } +} diff --git a/tests/Security/UserCheckerTest.php b/tests/Security/UserCheckerTest.php new file mode 100644 index 0000000..4f6adb0 --- /dev/null +++ b/tests/Security/UserCheckerTest.php @@ -0,0 +1,82 @@ +auditRecords = new TestHandler(); + } + + public function testAnOrdinaryAccountPasses(): void + { + $this->expectNotToPerformAssertions(); + + $this->checker()->checkPreAuth(new User('anna@ep-reisen.de', 'Anna')); + } + + public function testALockedOutAccountIsRefusedWithItsReason(): void + { + $user = new User('anna@ep-reisen.de', 'Anna'); + $user->disable('Auf eigenen Wunsch'); + + $this->expectException(CustomUserMessageAccountStatusException::class); + $this->expectExceptionMessage('Auf eigenen Wunsch'); + + $this->checker()->checkPreAuth($user); + } + + public function testALockoutWithoutAReasonStillSaysWhatHappened(): void + { + $user = new User('anna@ep-reisen.de', 'Anna'); + $user->disable(); + + $this->expectException(CustomUserMessageAccountStatusException::class); + $this->expectExceptionMessage('gesperrt'); + + $this->checker()->checkPreAuth($user); + } + + /** + * The authenticator audits the refusals it can see; this one happens after it has handed the + * passport over, so the trail would otherwise show a sign-in that simply never completed. + */ + public function testTheRefusalIsAudited(): void + { + $user = new User('anna@ep-reisen.de', 'Anna'); + $user->disable('Auf eigenen Wunsch'); + + try { + $this->checker()->checkPreAuth($user); + } catch (CustomUserMessageAccountStatusException) { + } + + $records = $this->auditRecords->getRecords(); + + self::assertCount(1, $records); + self::assertSame('Sign-in refused.', $records[0]->message); + self::assertSame('account_disabled', $records[0]->context['reason']); + self::assertSame('anna@ep-reisen.de', $records[0]->context['account']); + } + + private function checker(): UserChecker + { + return new UserChecker(new AuditLogger(new Logger('audit', [$this->auditRecords]))); + } +}