From b7085f9c3647d193942f2d095c17943e0785c7dc Mon Sep 17 00:00:00 2001 From: martinydeAI Date: Fri, 12 Jun 2026 07:44:08 +0200 Subject: [PATCH 1/2] =?UTF-8?q?docs:=20add=20ADR=20004=20=E2=80=94=20user?= =?UTF-8?q?=20registration,=20approval,=20and=20account=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the decision to model identity state with a status enum (pending | approved | blocked) on the User entity rather than gating via roles. Roles are kept orthogonal — they answer what a signed-in user can do, not whether they may sign in. The supporting architecture covers an env-backed email-domain allow-list for self-signup, a Symfony UserCheckerInterface that rejects login for non-approved users, and an approval queue restricted to users with domainManager=true. Notes that #45's active boolean is superseded by the enum and should be updated when implemented. Status: Draft. Index updated. CHANGELOG entry under [Unreleased] / Added. Closes #60. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 5 + .../004-user-approval-and-account-state.md | 166 ++++++++++++++++++ docs/adr/README.md | 9 +- 3 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 docs/adr/004-user-approval-and-account-state.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 30aa2a5..9ba2746 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - PHPUnit test harness with 100% coverage gate enforced in CI via `rregeer/phpunit-coverage-check` ([#31](https://github.com/itk-dev/ai-lib/issues/31)). +- ADR 004 (Draft) — user registration, approval, and account-state + model: a `status` enum (`pending | approved | blocked`) on `User`, + an env-var allow-list of e-mail domains for self-signup, and a + `UserCheckerInterface` gating login + ([#60](https://github.com/itk-dev/ai-lib/issues/60)). ### Changed diff --git a/docs/adr/004-user-approval-and-account-state.md b/docs/adr/004-user-approval-and-account-state.md new file mode 100644 index 0000000..019f5d9 --- /dev/null +++ b/docs/adr/004-user-approval-and-account-state.md @@ -0,0 +1,166 @@ +# 004: User registration, approval, and account-state model + +| Field | Value | +| ------------------ | -------------------------------------------------- | +| **Created By** | Martin Yde Granath | +| **Date** | 2026-06-12 | +| **Decision Maker** | ITK Dev team | +| **Stakeholders** | ITK Dev developers, future maintainers of ai-lib | +| **Status** | Draft | + +## Context + +ai-lib serves Danish public-sector organisations. The intended +onboarding flow is: + +1. A representative from an approved organisation self-registers at + `/register` using their work e-mail. +2. The system only accepts the registration if the e-mail domain is on + a project-managed allow-list (kommune domains, ministries, etc.). +3. Even after a successful registration, the new account does **not** + get application access until an existing trusted user (a domain + manager) approves it from an admin queue. +4. An approved account can later be **blocked** without being deleted + (audit trail, possible un-block). + +Before we build any of that we need to settle one design question that +ripples through the rest of the work: **how is the "can this person +sign in" state modelled on the `User` entity?** + +The two candidate approaches the team weighed informally were: + +- **A — Identity-state field(s) on `User`.** A `status` enum + (`pending | approved | blocked`) or a pair of booleans (`approved`, + `active`) that's explicit and independent of authorisation. +- **B — Role-based gating.** Treat "no roles" as "no access" — a + pending or blocked user simply has no entries in the `roles` column. + +This ADR is scoped to that choice and the surrounding registration / +approval architecture. Tracked in +[#60](https://github.com/itk-dev/ai-lib/issues/60). + +### Drivers + +- **Functional:** + - Self-signup gated by an allow-list of e-mail domains. + - Distinct "pending" (never approved) and "blocked" (approved, then + revoked) states for the admin UX. + - A single, declarative place for "should this credential succeed?" + so the login flow stays auditable. +- **Non-functional:** + - Stay within Symfony Security idioms — don't fight the framework's + User abstraction or its `UserCheckerInterface` extension point. + - Keep authorisation (`roles`, voters) orthogonal from identity + state so changes to one don't accidentally weaken the other. + +### Options Considered + +1. **Status enum on `User` (`pending | approved | blocked`).** + - Pros: single source of truth for the identity lifecycle; + distinguishes "never approved" from "approved then revoked"; + trivial to query (`status = 'pending'` powers the approval + queue); ergonomic with Symfony's `UserCheckerInterface`. + - Cons: extending the lifecycle later means adding enum cases + (Doctrine + migration), not just toggling a flag. +2. **Two booleans (`approved` + `active`).** + - Pros: smaller migrations to introduce. + - Cons: two flags that mostly want to move in lockstep, with one + impossible / undefined state (`approved=false, active=true`) that + code must remember not to produce; #45's existing `active` field + would need to grow a partner. +3. **Role-based gating (`roles` empty → no access).** + - Pros: no schema change beyond what #12 already implies. + - Cons: Symfony's generated `User::getRoles()` returns + `array_unique([...$this->roles, 'ROLE_USER'])`, so "empty roles" + does **not** actually mean "no access" without removing that + guarantee and fighting framework idioms; can't distinguish + "pending" from "blocked"; conflates *what* a signed-in user can + do with *whether* they may sign in at all. + +## Decision + +Adopt **option 1 — a `status` enum** on the `User` entity: + +```php +enum UserStatus: string +{ + case Pending = 'pending'; + case Approved = 'approved'; + case Blocked = 'blocked'; +} +``` + +`User::getStatus(): UserStatus` becomes the single source of truth +for the identity lifecycle. Authorisation (`roles`, voters, +`domainManager`) remains orthogonal — those answer "what may a +signed-in user do", not "may this person sign in". + +### Registration + +- Anonymous endpoint `/register` accepts `email`, `password`, and + `name`. +- The submitted e-mail's domain must match an entry on an allow-list. + The list is sourced from a comma-separated env var + (`REGISTRATION_ALLOWED_EMAIL_DOMAINS=aarhus.dk,kk.dk,…`) for the + first cut, with a clear migration path to a dedicated `Domain` + entity if the list later needs CRUD admin tooling. +- A successful registration creates a `User` with + `status = UserStatus::Pending`. The user is shown a "waiting for + approval" page and cannot sign in. + +### Login gating + +- A `Symfony\Component\Security\Core\User\UserCheckerInterface` + implementation (`App\Security\AccountStatusChecker`) rejects login + for any user whose `status` is not `Approved`, with localised + messages for `Pending` vs. `Blocked`. +- Wired in `security.yaml` via `user_checker:` on the `main` firewall. + +### Approval queue + +- A route (e.g. `/admin/users/pending`) lists users with + `status = Pending`. Restricted to users with `domainManager = true` + via a voter or `IsGranted` expression. +- The approver can **approve** (`status = Approved`) or **reject** + (`status = Blocked` — preserves the row for audit; a separate + "delete" action can come later if needed). +- An already-approved user can later be blocked (`Approved` → + `Blocked`) from the same admin surface; un-blocking is just the + same action in reverse. + +### Implication for #45 + +The `active` boolean planned in +[#45](https://github.com/itk-dev/ai-lib/issues/45) is **superseded** +by the `status` enum. When #45's implementation lands, the User entity +ships with `name`, `domainManager`, and `status` — not `active`. #45 +should be updated to reflect this so the migration doesn't end up +needing immediate amendment. + +## Consequences + +### Positive + +- Identity state and authorisation stay orthogonal; reasoning about + either in isolation is easier. +- The approval queue is a one-line query (`WHERE status = 'pending'`); + audit and compliance questions ("show me all blocked accounts") + fall out of the same model. +- `UserCheckerInterface` is Symfony's documented hook for exactly this + — no custom event listeners or controller checks scattered around. +- The enum's three named cases read better in code, templates, and + the admin UI than two booleans would. + +### Negative / Trade-offs + +- Doctrine string-backed enums need a small `Type` mapping (or use + Symfony's built-in support); a tiny amount of extra setup compared + to a bare boolean column. +- Adding a fourth state later (e.g. `awaiting_email_verification`, + `expired`) means a migration to extend the enum domain. We accept + this — it's exactly the kind of change an ADR should make + deliberate. +- The env-var allow-list is the right starting point but will need to + graduate to a `Domain` entity if domains grow or need per-domain + metadata (e.g. a different approver per organisation). Tracked as a + follow-up only if it actually happens. diff --git a/docs/adr/README.md b/docs/adr/README.md index 8ae100d..489f940 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -19,7 +19,8 @@ See [adr.github.io](https://adr.github.io/) for background on the format. ## Index -| Number | Title | Status | Date | -| -------------------------------------------- | -------------------------------------- | -------- | ---------- | -| [001](001-tech-stack-docker-symfony.md) | Tech stack: Docker + Symfony | Accepted | 2026-06-08 | -| [002](002-project-license-mpl-2.md) | Project license: MPL-2.0 | Accepted | 2026-06-08 | +| Number | Title | Status | Date | +| --------------------------------------------- | ----------------------------------------------- | -------- | ---------- | +| [001](001-tech-stack-docker-symfony.md) | Tech stack: Docker + Symfony | Accepted | 2026-06-08 | +| [002](002-project-license-mpl-2.md) | Project license: MPL-2.0 | Accepted | 2026-06-08 | +| [004](004-user-approval-and-account-state.md) | User registration, approval, and account state | Draft | 2026-06-12 | From 903e28410493ac76075285edbcd37d09e024755f Mon Sep 17 00:00:00 2001 From: martinydeAI Date: Fri, 12 Jun 2026 08:28:28 +0200 Subject: [PATCH 2/2] docs(adr): domain manager is a role, not a flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds the domainManager: bool field originally proposed in #45 into the existing roles column as ROLE_DOMAIN_MANAGER. Promote / demote is toggling the role; scope (which domain a manager can act on) is derived from the manager's own email domain via a small voter. Site admin (ROLE_ADMIN) sits above it via role_hierarchy and short-circuits the domain-match check. The 'Implication for #45' note is updated: the entity ships with only 'name' on top of the auth fields from #2 — both active and domainManager are superseded. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../004-user-approval-and-account-state.md | 51 ++++++++++++++++--- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/docs/adr/004-user-approval-and-account-state.md b/docs/adr/004-user-approval-and-account-state.md index 019f5d9..68fa137 100644 --- a/docs/adr/004-user-approval-and-account-state.md +++ b/docs/adr/004-user-approval-and-account-state.md @@ -119,8 +119,9 @@ signed-in user do", not "may this person sign in". ### Approval queue - A route (e.g. `/admin/users/pending`) lists users with - `status = Pending`. Restricted to users with `domainManager = true` - via a voter or `IsGranted` expression. + `status = Pending`. Restricted to users with `ROLE_DOMAIN_MANAGER` + (or `ROLE_ADMIN` via role hierarchy) via the voter described in + "Domain manager — a role, not a flag" below. - The approver can **approve** (`status = Approved`) or **reject** (`status = Blocked` — preserves the row for audit; a separate "delete" action can come later if needed). @@ -128,14 +129,48 @@ signed-in user do", not "may this person sign in". `Blocked`) from the same admin surface; un-blocking is just the same action in reverse. +### Domain manager — a role, not a flag + +Following the same reasoning as the `status` decision above — +capability and identity stay orthogonal — "domain manager" is +modelled as the **role** `ROLE_DOMAIN_MANAGER` on the existing +`User.roles` column rather than a dedicated boolean field. + +- Promotion / demotion is just adding or removing the role from the + array. +- A user's **scope** (which domain they manage) is derived from the + part of their own e-mail address after the `@`. No separate + column. +- Authorisation flows through a small voter that combines: + 1. `is_granted('ROLE_DOMAIN_MANAGER')` on the acting user, and + 2. `emailDomain(currentUser) === emailDomain(targetUser)`. +- Site admin gets `ROLE_ADMIN`. Symfony's `role_hierarchy` is + configured so `ROLE_ADMIN` implies `ROLE_DOMAIN_MANAGER`, and the + voter short-circuits the domain-match check for admins so they can + manage users across all domains from the same screen. +- The user-management view is a single controller. The list query + is scoped: `ROLE_ADMIN` sees everyone; `ROLE_DOMAIN_MANAGER` sees + only users whose email domain matches their own. + +If we later need a user to manage a domain *other than* their own +email's, or an organisation that owns multiple e-mail domains, we +introduce a `Domain` entity and a relation. Defer until needed — +removing that complexity later would be the painful direction. + ### Implication for #45 -The `active` boolean planned in -[#45](https://github.com/itk-dev/ai-lib/issues/45) is **superseded** -by the `status` enum. When #45's implementation lands, the User entity -ships with `name`, `domainManager`, and `status` — not `active`. #45 -should be updated to reflect this so the migration doesn't end up -needing immediate amendment. +Both `active` and `domainManager` from +[#45](https://github.com/itk-dev/ai-lib/issues/45) are **superseded**: + +- `active` → replaced by the `status` enum. +- `domainManager` → replaced by the `ROLE_DOMAIN_MANAGER` role on the + existing `User.roles` column. + +When #45's implementation lands, the entity ships with **only `name`** +on top of the auth fields from +[#2](https://github.com/itk-dev/ai-lib/issues/2). #45 should be +updated to reflect this so the migration doesn't end up needing +immediate amendment. ## Consequences