feat: add user authentication (#2)#59
Merged
Merged
Conversation
Wires up the application's first persistent identity and the login flow it gates. Installs Doctrine ORM + migrations + Symfony Security + dev-only MakerBundle and DoctrineFixturesBundle; switches doctrine.yaml to MariaDB and sets DATABASE_URL to the in-stack mariadb service. App surface: App\Entity\User (email, hashed password, roles), UserRepository with PasswordUpgraderInterface, App\Security\UserManager service that owns persistence + hashing, App\Controller\SecurityController exposing /login and /logout (form_login + declarative logout in security.yaml), Twig login form under templates/security/, and Danish translation keys under security.login.*. Two console commands sit on UserManager: app:user:create and app:user:change-password. App\DataFixtures\UserFixtures seeds alice@example.test and bob@example.test (password `password`) for local dev. Tests: 32 cases, 70 assertions, 100% coverage. UserManager unit-tested via KernelTestCase + a schema-reset trait; UserRepository's upgradePassword covered for the happy path and the foreign-user rejection; commands exercised through CommandTester; SecurityController's full /login + /logout + failed-login flow exercised through WebTestCase. Tests share tests/Support/ResetsDatabaseSchemaTrait which drops + recreates the db_test schema from ORM metadata in setUp. Docs: README gains a "Creating the first user" subsection covering migration, fixture load, and the two console commands. CHANGELOG entry under [Unreleased] / Added references #2. Closes #2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PHPUnit's test environment uses Symfony's dbname_suffix to talk to a separate test database (db_test, optionally db_test_paratest_N). The itkdev/mariadb image only grants MYSQL_USER on MYSQL_DATABASE, so the test runner hits 'Access denied for user db@% to database db_test' on a fresh container. Mount .docker/mariadb/init/ as /docker-entrypoint-initdb.d/ so the included GRANT runs on first container initialisation. The wildcard is escaped as db\_test% so it only matches db_test... not unrelated names like dbXtest. Local devs with an already-initialised mariadb volume can either recreate the container (task compose -- down -v && task site-install) or apply the grant once manually. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds task db-prepare-test which re-applies the GRANT init SQL and ensures db_test exists. test and test-coverage depend on it so any local dev whose mariadb data volume predates the new /docker-entrypoint-initdb.d mount can run tests without first recreating the container. The grant is idempotent and only touches privileges — no data is read or written. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The earlier init script only granted privileges on db_test* but never created the database. CI runs vendor/bin/phpunit directly (not task test), so the db-prepare-test target couldn't backfill the CREATE either, and the Tests workflow failed with 'Unknown database db_test'. Folds CREATE DATABASE IF NOT EXISTS db_test into the init SQL so both fresh CI containers and the local db-prepare-test target reach the same state. The doctrine:database:create call in db-prepare-test is now redundant and removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds doctrine:migrations:migrate as the final step of site-install so a fresh check-out's database schema lands automatically. Drops the manual 'Apply the database schema' snippet from the README's user-creation section since the schema is now in place by the time anyone reaches it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 'Creating the first user' section doesn't need to remind readers that site-install ran migrations — that's documented in the section above. Removes the explanatory paragraph and leaves just the commands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@symfony enables phpdoc_align with align=vertical, which padded @param and @return so columns lined up and pushed descriptions onto extra wrapped lines. Override to align=left and rewrite the verbose blurbs in the user-auth code so each tag fits on one line. Net 46 lines removed across UserManager, the two console commands, SecurityController, UserFixtures, and DevTemplateMarkerNodeVisitor (the last one fell out of the same cs-fixer pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
martinyde
reviewed
Jun 11, 2026
martinyde
left a comment
martinyde
left a comment
Contributor
There was a problem hiding this comment.
This is a scraped user entity with login functionality, commands related to user management and a few fixtures for good measure. The user entity is not complete with this PR and we have not yet implemented any restrictions based on user session or role. I have added some comments to pinpoint my uncertainties in regards to our usual user/login implementations.
This was referenced Jun 12, 2026
tuj
requested changes
Jun 15, 2026
tuj
requested changes
Jun 15, 2026
Resolves CHANGELOG conflict: develop dropped the Changed section (PR #57 consolidated entries under Added), so the auth bullet is moved to Added alongside develop's existing entries.
Address review feedback on PR #59: replace the hand-rolled mariadb init SQL (which created db_test and granted the db user on db_test%) with bin/console doctrine:database:create --env=test, the standard Symfony approach. The db user has grants scoped to the db database only, so .env.test overrides DATABASE_URL to connect as root against an explicit db_test. The when@test doctrine block (which appended the _test suffix) is removed — the URL now names the test database directly, so dev/test separation cannot be silently broken by a misread DATABASE_URL. - Delete .docker/mariadb/init/01-grant-test-databases.sql and the docker-compose volume mount that wired it into the container. - Replace task db-prepare-test with the Symfony console command (idempotent via --if-not-exists). - Add the same database-create step to the Tests CI workflow.
Address PR #59 review feedback: tuj noted that os2display has adopted Doctrine's Schema tool API for migrations so they stay portable across any database Doctrine supports, enforced via a NoAddSqlInMigration PHPStan rule. Mirror that convention here. The previous body called $this->addSql() with a MariaDB-specific CREATE TABLE string. The new body uses $schema->createTable() with typed columns, the primary key, the unique index, and the utf8mb4 table option so the resulting DDL is byte-identical on MariaDB while remaining portable. Verified by dropping db_test, running doctrine:migrations:migrate, then doctrine:migrations:diff to confirm no schema delta vs the User entity metadata.
Address PR #59 review feedback: tuj flagged that the Usage line in UserChangePasswordCommand duplicates information already carried by the AsCommand attribute and addArgument descriptions. Strip the same line from UserCreateCommand for consistency. The README's "Creating the first user" section keeps its `task console -- app:user:...` examples — those are human-facing setup docs, not the per-class documentation tuj objected to.
Address PR #59 review feedback: tuj noted the override should live upstream in itk-dev/devops_itkdev-docker so the style aligns across projects, not as a per-project delta. Restore .php-cs-fixer.dist.php to match the template byte-for-byte. Follow-up needed: existing PHPDocs are left-aligned (from commit 46e417c) but the @symfony preset's default phpdoc_align is `vertical`. The CI PHP CS check will fail until either an upstream PR lands the rule or the local files are reflowed via task coding-standards-php-apply.
Address PR #59 review item 6: tuj pointed at the Symfony docs section on resetting the database between tests, plus the economics phpunit.xml.dist split. Adopt both. - composer require --dev dama/doctrine-test-bundle:^8.6 for transactional per-test isolation. The bundle wraps every test in a DBAL transaction and rolls back at tearDown, so per-test schema rebuilds are no longer needed. - Two PHPUnit testsuites: unit (tests/Unit, 11 cases, no kernel) and integration (tests/Integration, 21 cases, full kernel + DB). - tests/bootstrap.php builds the schema once from ORM metadata via Doctrine SchemaTool. Guarded by TESTS_SKIP_SCHEMA=1 so task test-unit can run against a torn-down database. - Remove ResetsDatabaseSchemaTrait and its 6 setUp() invocations. Empty bootstrap state plus DAMA rollback gives the same isolation guarantee at no per-test DDL cost. - New Taskfile targets: test-unit and test-integration. The existing task test continues to run both suites in one PHPUnit invocation, so the 100% coverage gate is unchanged. Verified locally: task test (32/32), task test-unit against a dropped db_test (11/11), task test-integration (21/21), task test twice back-to-back, task test-coverage (100%). PHP CS check fails only on the 4 files carried over from commit 7ef32d3 (phpdoc_align revert); the moved/edited files all pass.
Address PR #59 review item 7: yepzdk asked that the button, input, and label in templates/security/login.html.twig become reusable Twig components so styling and behaviour live in one place. Done via the existing anonymous-component pattern this project already uses for Eyebrow, Box, Nav/Link, etc. - templates/components/Form/Label.html.twig wraps the label text + span + slotted content. Takes `for` and `text` props. - templates/components/Form/Input.html.twig accepts id, name, type (default text), value, autocomplete, required, autofocus. The Tailwind class set is encapsulated in the component. - templates/components/Form/Button.html.twig accepts type (default submit), variant (default primary), size (default md). The variant and size maps currently hold one entry each — the API is set up so secondary/danger variants and sm/lg sizes can be added in one place. - templates/security/login.html.twig now uses these three components instead of inlining the markup. Verified: task test (32/32, including SecurityControllerTest which asserts input[name="_username"], input[name="_password"], and a submit button via the DOM crawler) and task test-coverage (100%). task coding-standards-twig-check passes.
Add tests/bootstrap_integration.php that builds the schema from ORM
metadata and calls UserFixtures::load() once before any integration
test runs. DAMA wraps each test in a DBAL transaction, so the
bootstrap-loaded baseline (alice + bob with password "password")
persists across the run while per-test mutations stay isolated.
Trim tests/bootstrap.php back to dotenv-only — it remains the default
bootstrap referenced from phpunit.dist.xml and is used by
task test-unit (which doesn't need a database). The TESTS_SKIP_SCHEMA
env-var guard is gone; the bootstraps now differ by file, not by env.
Taskfile and CI:
- task test, task test-integration, task test-coverage all pass
--bootstrap=tests/bootstrap_integration.php.
- task test-unit uses the XML default (minimal bootstrap).
- .github/workflows/tests.yaml mirrors the coverage invocation.
Tests adapted to the new baseline:
- SecurityControllerTest drops its setUp createUser; login tests use
alice's fixture password ("password") instead of "secret".
- UserManagerTest: tests that previously created alice from scratch
now use charlie@example.test. testCreateUserRejectsDuplicateEmail
relies on alice already being in the baseline. testChangePassword
variants read alice from the repository instead of creating her.
- UserRepositoryTest reads baseline alice.
- UserChangePasswordCommandTest reads baseline alice.
- UserCreateCommandTest uses charlie@example.test for the success
case and the baseline alice for the duplicate-email case.
- UserFixturesTest (integration) is reframed as a baseline-presence
assertion since the bootstrap has already loaded the fixtures.
Coverage gate: bootstrap-time execution of UserFixtures::load() is not
attributed to coverage, so a focused unit test under
tests/Unit/DataFixtures/UserFixturesTest.php invokes load() with the
real UserManager + mocked collaborators (UserManager is final and
can't be PHPUnit-mocked directly). 100% gate stays green.
Verified locally: task test (33/33, 74 assertions), task test-unit
against a dropped db_test (12/12, no DB contact), task test-integration
from scratch (21/21), task test twice back-to-back, task test-coverage
(100%).
Trim the docblock to one short paragraph: name what the test does (re-invoke load() for coverage) and the one constraint that shapes the implementation (UserManager is final, so collaborators are mocked instead of the manager itself).
yepzdk
previously approved these changes
Jun 16, 2026
yepzdk
left a comment
yepzdk
left a comment
Contributor
There was a problem hiding this comment.
This looks like a good take on a first base.
We can pick up any follow-ups that might surface, in new issues.
tuj
requested changes
Jun 16, 2026
tuj
previously approved these changes
Jun 16, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #2 end-to-end: the application's first persistent identity, the form-login flow that gates it, the supporting console tooling, and the tests that hold all of it at 100 % coverage.
Backend
symfony/orm-pack(Doctrine ORM + migrations + bundle),symfony/security-bundle, plussymfony/maker-bundleanddoctrine/doctrine-fixtures-bundleas dev-only deps.docker-compose.ymladditions and the generatedcompose.override.yaml, dropped the Postgres-specific bits fromconfig/packages/doctrine.yaml, and setDATABASE_URLin.envto point at the in-stackmariadbservice. Granted thedbuser privileges ondb_test*so PHPUnit's test DB works.App\Entity\User— email, hashed password, roles. Repository implementsPasswordUpgraderInterfacefor transparent rehashing.App\Security\UserManager— single seam owning persistence + hashing. Throws\DomainExceptionon duplicate email / missing user and\InvalidArgumentExceptionon empty password. Per project conventions the controller and commands stay thin and delegate here.security.yaml—app_user_provider(entity provider onemail),form_login+ declarativelogouton themainfirewall,/loginand/logoutpublicly accessible.Version20260611124347.phpcreates theusertable (id, email unique, roles JSON, password). Migration applied locally and against the test DB.Login flow
App\Controller\SecurityController—/loginrenders the form using Symfony Security'sAuthenticationUtils;/logoutis intercepted by the firewall.templates/security/login.html.twig— extendsbase.html.twig, uses the<twig:Eyebrow>component, CSRF token, and translation keys.security.login.*added totranslations/messages.da.yaml.Tooling
app:user:create <email> <password>— creates a new user viaUserManager.app:user:change-password <email> <password>— rotates an existing user's password.App\DataFixtures\UserFixtures— seedsalice@example.testandbob@example.test, both with passwordpassword, viaUserManager(same code path the commands use).Tests — 32 cases, 70 assertions, 100 % coverage
tests/Support/ResetsDatabaseSchemaTrait— drops and recreates the schema from ORM metadata insetUp. Shared by every DB-touching test class.tests/Security/UserManagerTest— happy paths and four exception branches.tests/Repository/UserRepositoryTest— coversupgradePasswordfor the supported and unsupported cases.tests/Command/UserCreateCommandTestandUserChangePasswordCommandTest— CommandTester wrappers, success + failure paths.tests/DataFixtures/UserFixturesTest— verifies the fixture loads both users.tests/Controller/SecurityControllerTest— full HTTP flow: page renders, successful login redirects to/with token populated, failed login redirects back with no token,/logoutclears the session. Also unit-tests the logout method's defensive throw.Docs
README.md— new Creating the first user subsection covering migration, fixture load, and the two console commands.CHANGELOG.md—[Unreleased] / Addedentry referencing User login #2.Out of scope
ROLE_USER(the framework's implicit guarantee is enough for now).active,domainManager,namefields from feat: add User.name field (rescoped per ADR 006) #45 — they'll land in a follow-up PR with their own migration (per the answer to the scope question on this PR's setup).Local verification
task site-install task console -- doctrine:migrations:migrate -n task console -- doctrine:fixtures:load -n # Visit /login, sign in as alice@example.test / password task test-coverageTest plan
task test-coverage→ 100 %, 32 tests, 70 assertions.task coding-standards-check→ green across PHP, Twig, YAML, JS, CSS, Markdown, Composer./loginrenders, accepts credentials, redirects to/on success./logoutclears the session and redirects to/.app:user:create+app:user:change-passwordsucceed and surface domain errors.Closes #2
🤖 Generated with Claude Code