feat: harden the myep OAuth2 login

tests/Command/LockUserCommandTest.php

@@ -0,0 +1,88 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Command;
+
+use App\Entity\User;
+use Doctrine\ORM\EntityManagerInterface;
+use Symfony\Bundle\FrameworkBundle\Console\Application;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use Symfony\Component\Console\Tester\CommandTester;
+
+/**
+ * The only way to set the flag the UserChecker reads.
+ */
+class LockUserCommandTest extends KernelTestCase
+{
+    private EntityManagerInterface $em;
+    private CommandTester $command;
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+        $this->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' => '[email protected]', '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' => '[email protected]', '--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' => ' [email protected] ']);
+
+        $this->command->assertCommandIsSuccessful();
+        self::assertTrue($user->isDisabled());
+    }
+
+    public function testAnUnknownAddressFails(): void
+    {
+        self::assertSame(1, $this->command->execute(['email' => '[email protected]']));
+        self::assertStringContainsString('No account', $this->command->getDisplay());
+    }
+
+    private function user(): User
+    {
+        $user = new User('[email protected]', 'Anna');
+        $user->setRoles(['ROLE_EMPLOYEE']);
+        $this->em->persist($user);
+        $this->em->flush();
+
+        return $user;
+    }
+}

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

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();
 

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('[email protected]', 'Anna Berger');
+        $existing->setSub('4711');
+        $existing->setRoles([Role::EMPLOYEE]);
+        $this->em->persist($existing);
+        $this->em->flush();
+
+        $user = $this->authenticate([
+            'sub' => '4711',
+            'email' => '[email protected]',
+            'roles' => ['ROLE_EMPLOYEE'],
+        ]);
+
+        self::assertSame($existing->getId(), $user->getId());
+        self::assertSame('[email protected]', $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('[email protected]', 'Anna Berger');
+        $existing->setSub('4711');
+        $existing->setRoles([Role::EMPLOYEE]);
+        $this->em->persist($existing);
+        $this->em->flush();
+
+        $user = $this->authenticate(['sub' => '4712', 'email' => '[email protected]', '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('[email protected]', 'Anna Berger');
+        $existing->setRoles([Role::EMPLOYEE]);
+        $this->em->persist($existing);
+        $this->em->flush();
+
+        $user = $this->authenticate(['sub' => '4711', 'email' => '[email protected]', '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('[email protected]', 'Anna Berger');
+        $existing->setRoles([Role::EMPLOYEE]);
+        $this->em->persist($existing);
+        $this->em->flush();
+
+        $user = $this->authenticate(['email' => '  [email protected] ', 'roles' => ['ROLE_EMPLOYEE']]);
+
+        self::assertSame($existing->getId(), $user->getId());
+        self::assertSame('[email protected]', $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('[email protected]', 'Anna Berger');
+        $existing->setSub('4711');
+        $existing->setRoles([Role::EMPLOYEE]);
+        $this->em->persist($existing);
+        $this->em->flush();
+
+        try {
+            $this->authenticate(['sub' => '4712', 'email' => '[email protected]', '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('[email protected]', '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' => '[email protected]',
+            '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<string, mixed> $userinfo
      */

tests/Security/OAuth2/EpClientTest.php

@@ -0,0 +1,148 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Security\OAuth2;
+
+use App\Security\OAuth2\AuthorizationDeniedException;
+use App\Security\OAuth2\AuthorizationRequestException;
+use App\Security\OAuth2\EpClient;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+
+/**
+ * The two legs of the authorization code flow that do not need myep to answer.
+ *
+ * Everything asserted here happens before a single HTTP request goes out: what the browser is
+ * sent to myep with, and which callbacks are refused without an exchange being attempted.
+ */
+class EpClientTest extends TestCase
+{
+    public function testTheAuthorizationUrlCarriesAnS256Challenge(): void
+    {
+        $request = $this->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<string, string> $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;
+    }
+}

tests/Security/UserCheckerTest.php

@@ -0,0 +1,82 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Security;
+
+use App\Entity\User;
+use App\Security\UserChecker;
+use App\Service\AuditLogger;
+use Monolog\Handler\TestHandler;
+use Monolog\Logger;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Security\Core\Exception\CustomUserMessageAccountStatusException;
+
+/**
+ * The local lockout, which is the one access decision myep has no say in.
+ */
+class UserCheckerTest extends TestCase
+{
+    private TestHandler $auditRecords;
+
+    protected function setUp(): void
+    {
+        $this->auditRecords = new TestHandler();
+    }
+
+    public function testAnOrdinaryAccountPasses(): void
+    {
+        $this->expectNotToPerformAssertions();
+
+        $this->checker()->checkPreAuth(new User('[email protected]', 'Anna'));
+    }
+
+    public function testALockedOutAccountIsRefusedWithItsReason(): void
+    {
+        $user = new User('[email protected]', '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('[email protected]', '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('[email protected]', '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('[email protected]', $records[0]->context['account']);
+    }
+
+    private function checker(): UserChecker
+    {
+        return new UserChecker(new AuditLogger(new Logger('audit', [$this->auditRecords])));
+    }
+}