From 2e0a56073647a32b8980d0da7642ccf2f0b45f9f Mon Sep 17 00:00:00 2001 From: martinydeAI Date: Fri, 19 Jun 2026 09:16:46 +0200 Subject: [PATCH 1/3] feat(security): reject pending and blocked users at login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `App\Security\AccountStatusChecker` implementing Symfony Security's `UserCheckerInterface`. The pre-auth hook rejects any `User` whose `status` is not `Approved`, throwing a `CustomUserMessageAccountStatusException` with a localised key: `account.pending` for users still awaiting approval and `account.blocked` for users an admin has revoked. Translations land in a new `translations/security.da.yaml` (the `security` domain the login template already renders error messages from). Wired on the `main` firewall via `security.yaml`'s `user_checker:` key. Identity state stays orthogonal to authorisation per ADR 006 — the voter from PR #87 reads roles + email, the checker reads status; neither overlaps with the other. Sequenced as PR 3 of the User management milestone plan. Stacked on PR 1 (#86) since it reads `User::$status` which doesn't exist on `develop` yet — labelled `do-not-merge` until #86 lands. Closes #63. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 10 +++ config/packages/security.yaml | 1 + src/Security/AccountStatusChecker.php | 66 ++++++++++++++++++ .../Controller/SecurityControllerTest.php | 59 ++++++++++++++++ .../Security/AccountStatusCheckerTest.php | 69 +++++++++++++++++++ translations/security.da.yaml | 10 +++ 6 files changed, 215 insertions(+) create mode 100644 src/Security/AccountStatusChecker.php create mode 100644 tests/Unit/Security/AccountStatusCheckerTest.php create mode 100644 translations/security.da.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index dfc670b..bf952a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `App\Security\AccountStatusChecker` implementing + `UserCheckerInterface` — gates the login flow so a `User` with + `status = Pending` or `status = Blocked` is rejected before the + password is verified. Throws + `CustomUserMessageAccountStatusException` with the localised + translation keys `account.pending` and `account.blocked` (rendered + in the `security` domain — see `translations/security.da.yaml`). + Wired on the `main` firewall via `security.yaml`'s `user_checker:` + key + ([#63](https://github.com/itk-dev/ai-lib/issues/63)). - `User.name` (display name) and `User.status` (lifecycle enum: `pending | approved | blocked`) per ADR 006, plus the `App\Enum\UserStatus` PHP enum. `UserManager::createUser()` now diff --git a/config/packages/security.yaml b/config/packages/security.yaml index bad06f3..ec318ec 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -19,6 +19,7 @@ security: main: lazy: true provider: app_user_provider + user_checker: App\Security\AccountStatusChecker form_login: login_path: app_login check_path: app_login diff --git a/src/Security/AccountStatusChecker.php b/src/Security/AccountStatusChecker.php new file mode 100644 index 0000000..15dd522 --- /dev/null +++ b/src/Security/AccountStatusChecker.php @@ -0,0 +1,66 @@ +getStatus()) { + UserStatus::Pending => throw new CustomUserMessageAccountStatusException('account.pending'), + UserStatus::Blocked => throw new CustomUserMessageAccountStatusException('account.blocked'), + UserStatus::Approved => null, + }; + } + + /** + * Post-auth hook required by the interface; no checks needed here. + * + * @param UserInterface $user the user that just authenticated successfully + * @param TokenInterface|null $token unused; Symfony 8 added the slot for hooks that need it + */ + public function checkPostAuth(UserInterface $user, ?TokenInterface $token = null): void + { + } +} diff --git a/tests/Integration/Controller/SecurityControllerTest.php b/tests/Integration/Controller/SecurityControllerTest.php index 9db7eba..fdcbde8 100644 --- a/tests/Integration/Controller/SecurityControllerTest.php +++ b/tests/Integration/Controller/SecurityControllerTest.php @@ -6,6 +6,8 @@ use App\Controller\SecurityController; use App\Entity\User; +use App\Enum\UserStatus; +use App\Security\UserManager; use Symfony\Bundle\FrameworkBundle\KernelBrowser; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; @@ -100,4 +102,61 @@ public function testLogoutClearsTheSession(): void $this->client->getContainer()->get('security.token_storage')->getToken(), ); } + + public function testPendingUserCannotLogIn(): void + { + $this->client->getContainer()->get(UserManager::class)->createUser( + 'carol@example.test', + 'Carol', + 'password', + status: UserStatus::Pending, + ); + + $crawler = $this->client->request('GET', '/login'); + $form = $crawler->filter('form')->form(); + $form['_username'] = 'carol@example.test'; + $form['_password'] = 'password'; + $this->client->submit($form); + + self::assertResponseRedirects('/login'); + $crawler = $this->client->followRedirect(); + self::assertResponseIsSuccessful(); + + // Localised pending message is rendered on the form. + self::assertStringContainsString( + 'venter på godkendelse', + $crawler->filter('body')->text(), + ); + self::assertNull( + $this->client->getContainer()->get('security.token_storage')->getToken(), + ); + } + + public function testBlockedUserCannotLogIn(): void + { + $this->client->getContainer()->get(UserManager::class)->createUser( + 'dora@example.test', + 'Dora', + 'password', + status: UserStatus::Blocked, + ); + + $crawler = $this->client->request('GET', '/login'); + $form = $crawler->filter('form')->form(); + $form['_username'] = 'dora@example.test'; + $form['_password'] = 'password'; + $this->client->submit($form); + + self::assertResponseRedirects('/login'); + $crawler = $this->client->followRedirect(); + self::assertResponseIsSuccessful(); + + self::assertStringContainsString( + 'spærret', + $crawler->filter('body')->text(), + ); + self::assertNull( + $this->client->getContainer()->get('security.token_storage')->getToken(), + ); + } } diff --git a/tests/Unit/Security/AccountStatusCheckerTest.php b/tests/Unit/Security/AccountStatusCheckerTest.php new file mode 100644 index 0000000..e8394a1 --- /dev/null +++ b/tests/Unit/Security/AccountStatusCheckerTest.php @@ -0,0 +1,69 @@ +setName('Alice') + ->setStatus(UserStatus::Approved); + + (new AccountStatusChecker())->checkPreAuth($user); + + // No exception thrown is the assertion; explicit to keep PHPUnit happy. + self::assertTrue(true); + } + + public function testPendingUserIsRejectedWithLocalisedMessage(): void + { + $user = (new User()) + ->setName('Pending') + ->setStatus(UserStatus::Pending); + + $this->expectException(CustomUserMessageAccountStatusException::class); + $this->expectExceptionMessage('account.pending'); + + (new AccountStatusChecker())->checkPreAuth($user); + } + + public function testBlockedUserIsRejectedWithLocalisedMessage(): void + { + $user = (new User()) + ->setName('Blocked') + ->setStatus(UserStatus::Blocked); + + $this->expectException(CustomUserMessageAccountStatusException::class); + $this->expectExceptionMessage('account.blocked'); + + (new AccountStatusChecker())->checkPreAuth($user); + } + + public function testForeignUserImplementationsAreIgnored(): void + { + $foreignUser = $this->createMock(UserInterface::class); + + (new AccountStatusChecker())->checkPreAuth($foreignUser); + + self::assertTrue(true); + } + + public function testCheckPostAuthIsANoOp(): void + { + $user = (new User())->setStatus(UserStatus::Approved); + + (new AccountStatusChecker())->checkPostAuth($user); + + self::assertTrue(true); + } +} diff --git a/translations/security.da.yaml b/translations/security.da.yaml new file mode 100644 index 0000000..3d87661 --- /dev/null +++ b/translations/security.da.yaml @@ -0,0 +1,10 @@ +# Danish strings rendered in the `security` translation domain. +# +# Symfony Security's CustomUserMessageAccountStatusException carries a +# `messageKey` that the login template renders as +# `error.messageKey|trans(error.messageData, 'security')`. The keys +# below match the messageKeys thrown by App\Security\AccountStatusChecker. + +account: + pending: "Din konto venter på godkendelse fra en administrator." + blocked: "Din konto er spærret. Kontakt en administrator for at få den åbnet igen." From a795476cd70b12c2f6164d7751fa0c1437defe1f Mon Sep 17 00:00:00 2001 From: Martin Yde Granath Date: Fri, 19 Jun 2026 09:58:39 +0200 Subject: [PATCH 2/3] Refine comment regarding blocked user roles Clarify comment about 'Blocked' user roles in AccountStatusChecker. --- src/Security/AccountStatusChecker.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Security/AccountStatusChecker.php b/src/Security/AccountStatusChecker.php index 15dd522..1710f76 100644 --- a/src/Security/AccountStatusChecker.php +++ b/src/Security/AccountStatusChecker.php @@ -21,8 +21,7 @@ * {@see CustomUserMessageAccountStatusException} halts the flow and * surfaces the (localised) translation key on the login form. * - * Identity state (this checker) stays orthogonal to authorisation - * (roles, voters) per ADR 006. A `Blocked` user retains the roles + * A `Blocked` user retains the roles * they had before being blocked — they just can't sign in to exercise * them. */ From 66509e0ff1164728f8f20e867e58e4bba630abca Mon Sep 17 00:00:00 2001 From: martinydeAI Date: Fri, 19 Jun 2026 12:22:37 +0200 Subject: [PATCH 3/3] chore(tests): add one-line intent comments to account-status tests Per review on PR #88 - apply the test-comment convention to the new AccountStatusCheckerTest and the two new login-rejection methods in SecurityControllerTest. Each `test...` method now opens with a single-line "Tests ...", "Ensures ...", or "Verifies ..." comment naming what it asserts. Pure documentation change - no test logic touched. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Integration/Controller/SecurityControllerTest.php | 2 ++ tests/Unit/Security/AccountStatusCheckerTest.php | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/tests/Integration/Controller/SecurityControllerTest.php b/tests/Integration/Controller/SecurityControllerTest.php index fdcbde8..5dbf35b 100644 --- a/tests/Integration/Controller/SecurityControllerTest.php +++ b/tests/Integration/Controller/SecurityControllerTest.php @@ -103,6 +103,7 @@ public function testLogoutClearsTheSession(): void ); } + // Tests that a Pending user is rejected at login with the localised pending message. public function testPendingUserCannotLogIn(): void { $this->client->getContainer()->get(UserManager::class)->createUser( @@ -132,6 +133,7 @@ public function testPendingUserCannotLogIn(): void ); } + // Tests that a Blocked user is rejected at login with the localised blocked message. public function testBlockedUserCannotLogIn(): void { $this->client->getContainer()->get(UserManager::class)->createUser( diff --git a/tests/Unit/Security/AccountStatusCheckerTest.php b/tests/Unit/Security/AccountStatusCheckerTest.php index e8394a1..38924b1 100644 --- a/tests/Unit/Security/AccountStatusCheckerTest.php +++ b/tests/Unit/Security/AccountStatusCheckerTest.php @@ -13,6 +13,7 @@ final class AccountStatusCheckerTest extends TestCase { + // Tests that an Approved user passes the pre-auth hook without raising. public function testApprovedUserPassesPreAuth(): void { $user = (new User()) @@ -25,6 +26,7 @@ public function testApprovedUserPassesPreAuth(): void self::assertTrue(true); } + // Ensures a Pending user is rejected with the 'account.pending' message key. public function testPendingUserIsRejectedWithLocalisedMessage(): void { $user = (new User()) @@ -37,6 +39,7 @@ public function testPendingUserIsRejectedWithLocalisedMessage(): void (new AccountStatusChecker())->checkPreAuth($user); } + // Ensures a Blocked user is rejected with the 'account.blocked' message key. public function testBlockedUserIsRejectedWithLocalisedMessage(): void { $user = (new User()) @@ -49,6 +52,7 @@ public function testBlockedUserIsRejectedWithLocalisedMessage(): void (new AccountStatusChecker())->checkPreAuth($user); } + // Verifies non-App User implementations fall through to the password checker. public function testForeignUserImplementationsAreIgnored(): void { $foreignUser = $this->createMock(UserInterface::class); @@ -58,6 +62,7 @@ public function testForeignUserImplementationsAreIgnored(): void self::assertTrue(true); } + // Tests that checkPostAuth does nothing (required by the interface). public function testCheckPostAuthIsANoOp(): void { $user = (new User())->setStatus(UserStatus::Approved);