before configuring OAuth authentification
This commit is contained in:
@@ -0,0 +1,743 @@
|
||||
# google-oauth-migration - Work Plan
|
||||
|
||||
## TL;DR (For humans)
|
||||
|
||||
**What you'll get:** Password login is gone. Visitors sign in with a Google account, and only the
|
||||
two email addresses you already have in the database are allowed in. Signing in as
|
||||
phuoc@caotek.fr keeps admin powers; signing in as ctphuoc@gmail.com gets you in without them.
|
||||
|
||||
**Why this approach:** The app keeps its own list of permitted emails and checks it once, at
|
||||
sign-in, so Google's "test user" loophole cannot be used to walk in. Authorization no longer
|
||||
decides who is an admin by comparing a name against the word "admin"; it reads a configured list
|
||||
instead. Sign-in state is protected with Google's recommended anti-hijacking checks.
|
||||
|
||||
**What it will NOT do:** It will not create accounts for strangers, ask Google for any extra
|
||||
information about you, add a server-side session system, or introduce a database migration tool
|
||||
that the project does not have.
|
||||
|
||||
**Effort:** Large
|
||||
**Risk:** High - it replaces the entire sign-in mechanism and rebuilds a database table inside a
|
||||
database file that is tracked in version control.
|
||||
**Decisions to sanity-check:** Your admin rights move from a hardcoded name to a list you edit in
|
||||
a config file; the sign-in screen timeout is lengthened so slow Google sign-ins are not thrown
|
||||
away; a broken edit-user page and a crash on the user list are fixed along the way because they
|
||||
sit in the code being changed.
|
||||
|
||||
Your next move: approve, or run a high-accuracy review. Full execution detail follows below.
|
||||
|
||||
---
|
||||
|
||||
> TL;DR (machine): Large/high-risk Pyramid 2.1 auth swap to Google OIDC via Authlib, DB allowlist
|
||||
> gate, destructive SQLite users-table rebuild, 14 todos in 5 waves + F1-F4.
|
||||
|
||||
## Scope
|
||||
### Must have
|
||||
- **Google OAuth 2.0 / OIDC sign-in replacing password login entirely.** Login becomes a redirect
|
||||
to Google; there is no local credential path left in the codebase.
|
||||
- **Authlib 1.8.0 as the only new library**, driven by Google's OpenID discovery document at
|
||||
`https://accounts.google.com/.well-known/openid-configuration` so `authorize_url`,
|
||||
`access_token_url` and `jwks_uri` are never hand-written. It is a requests-based client
|
||||
(`authlib.integrations.requests_client.OAuth2Session`); Authlib ships integrations for Flask,
|
||||
Django and Starlette but **none for Pyramid**, which is why the app owns its own `state` / PKCE /
|
||||
`nonce` handling. **The app fetches the discovery document itself** and passes the resolved
|
||||
`authorization_endpoint`, `token_endpoint` and `jwks_uri` into the client: passing
|
||||
`server_metadata_url=` to `OAuth2Session` only stores inert metadata, because
|
||||
`load_server_metadata` exists solely on the Starlette-style `sync_app` / `async_app` clients.
|
||||
The document is cached alongside the JWKS rather than refetched per sign-in.
|
||||
- **A named, registry-registered collaborator for the Google exchange** - not ad-hoc monkeypatching
|
||||
in tests. It exposes `create_authorization_url(...)` and `fetch_claims(...)`, and it **fetches and
|
||||
caches the JWKS** itself, because the bare client does not: `CodeIDToken` + `jwt.decode` require
|
||||
the app to pull keys from `jwks_uri`, and an unbounded refetch on every sign-in is a latency and
|
||||
rate-limit problem. Cache TTL is a decision of this plan, not left implicit.
|
||||
- **App-owned `state`, PKCE and `nonce` handling.** `state` is generated, persisted in the session
|
||||
and **consumed (deleted) before the token exchange is attempted**; `code_verifier` is generated
|
||||
by the app with `generate_token(48)` because `create_authorization_url` returns only
|
||||
`(uri, state)` and never hands the verifier back; `nonce` is generated, persisted, compared
|
||||
against the `id_token` claim, then deleted.
|
||||
- **`nonce` is REQUIRED** by Google's OIDC request parameters.
|
||||
- **PKCE with `code_challenge_method=S256`**, the `code_verifier` persisted between the two hops.
|
||||
- **A single, non-replayable code exchange.** `pyramid_tm` tags exceptions as retryable, so a
|
||||
SQLite "database is locked" transient error can make `pyramid_retry` re-run the callback and
|
||||
re-exchange an already-used authorization code, which fails with `invalid_grant` and shows the
|
||||
user a broken login. The callback must be non-retryable and `fetch_claims` must be called **at
|
||||
most once** per request even with retries enabled.
|
||||
- **Auth policy swap to Pyramid's built-in `SessionAuthenticationPolicy` + `SessionAuthenticationHelper`**,
|
||||
replacing `AuthTktAuthenticationPolicy`. Pyramid 2.1 ships both with the same three-method
|
||||
interface as `AuthTktCookieHelper`, so `remember()`/`forget()` call sites keep working and
|
||||
`security.py` is untouched. The hardcoded auth-ticket secret is removed.
|
||||
- **The session factory is explicitly reconfigured - it is a first-class part of this change, not
|
||||
an afterthought.** `SignedCookieSessionFactory('hZug2zPt7hT2MZ')` defaults to `timeout=1200`
|
||||
(20 minutes) while the outgoing `AuthTktAuthenticationPolicy` used `36000` (10 hours). A Google
|
||||
consent screen, a Google sign-in and a second factor routinely exceed 20 minutes, so the OAuth
|
||||
values would expire mid-flow. The factory must be constructed with an explicit, longer `timeout`,
|
||||
a secret of at least 64 characters for `hashalg='sha512'` (it is currently 16), and
|
||||
`httponly=True` in both environments plus `secure=True` under production settings. Today the
|
||||
cookie carries `secure=False`, `httponly=False` even though `production.ini` declares
|
||||
`url_scheme = https`; once the cookie *is* the identity, third-party CDN scripts in
|
||||
`layout.jinja2:14-15,106-107` can read it via XSS.
|
||||
- **No session-id rotation is attempted, because the API does not exist.** `request.session.renew()`
|
||||
is **not** part of Pyramid 2.1, and neither are the `SessionRenew` / `SessionInvalidate`
|
||||
exceptions. The only primitive is `invalidate()`, which is literally `self.clear()` carrying the
|
||||
upstream comment `# XXX probably needs to unset cookie`. There is no server-side session id, so
|
||||
classic session fixation is structurally impossible; the real requirement is to **delete the
|
||||
`state` / `code_verifier` / `nonce` keys after the callback** so they do not survive client-side.
|
||||
- **`remember()` stores the user's email** as `authenticated_userid`, which keeps all **15**
|
||||
existing `request.authenticated_userid` call sites working - 7 in Python
|
||||
(`services/blog_record.py` x3, `views/blog.py` x2, `views/default.py` x2) and 8 in templates
|
||||
(`layout.jinja2` x4, `blog.jinja2` x2, `home.jinja2` x1, `user_edit.jinja2` x1) - including
|
||||
`layout.jinja2:46` (routes `/user_edit/{name}` by that value) and `views/blog.py:64,75` (already
|
||||
write it into `entries.creator` / `entries.editor`).
|
||||
- **A database allowlist as the mandatory access control, checked exactly once, in the callback.**
|
||||
An email that does not match a pre-provisioned `users` row is rejected and no row is created.
|
||||
This is required, not defensive: Google's own production-readiness guidance states the
|
||||
consent-screen test-user allowlist is bypassed when an app requests only basic identity scopes.
|
||||
- **The allowlist check must NOT become the group lookup.** `pyramid_tm` defaults
|
||||
`tm.annotate_user` to `True` and then calls `request.authenticated_userid` *inside*
|
||||
`manager.begin()`. A database-backed groupfinder therefore runs a SELECT on **every single
|
||||
request, inside the transaction**, and any database error turns every route - including `/login`
|
||||
and the OAuth callback itself - into a 500. This was reproduced against the real app.
|
||||
- **Admin rights become configured data, not a literal and not a database lookup.** All 4 sites
|
||||
comparing to the literal `'admin'` are replaced by `'group:administrators' in
|
||||
request.effective_principals`: `services/user.py:26`, `views/default.py:93`,
|
||||
`templates/layout.jinja2:43`, `templates/user_edit.jinja2:45`. The mapping from email to admin
|
||||
comes from an `auth.admin_emails` list in settings, so it survives a session loss, takes effect
|
||||
immediately on edit, and costs no query. Templates cannot call a groupfinder, which is why the
|
||||
`effective_principals` check is the correct template-side expression.
|
||||
- **One-shot binding of the two existing accounts, with no first-login claim window:**
|
||||
`admin` (id 1) -> `phuoc@caotek.fr` (retains `manage`); `phuoc` (id 2) -> `ctphuoc@gmail.com`
|
||||
(authenticated, no `manage`). Two distinct people, so each row maps to exactly one identity. The
|
||||
Google `sub` is stored on first successful sign-in and matched first on later sign-ins, because
|
||||
`sub` is Google's stable identifier and email is mutable.
|
||||
- **Removal of the password surface:** the `users.password` column, the `passlib` dependency, the
|
||||
top-level `import passlib` in `models/user.py:11` (**removing the dependency without removing
|
||||
that import breaks the entire app at import time**), `User.verify_password` / `set_password`,
|
||||
the `password` / `confirm` WTForms fields and their length check at `views/default.py:116-117`,
|
||||
the non-admin self-service password path, and the admin "Nouvel utilisateur" create-user flow at
|
||||
`views/default.py:98-102,119-131`.
|
||||
- **Every template that renders a removed field is fixed in the same change.** Jinja2's default
|
||||
`Undefined` raises on attribute access, so `user_edit.jinja2:27-28,36-37` will raise
|
||||
`UndefinedError` the moment the password fields disappear.
|
||||
- **`last_logged` gets an explicit writer and a NULL-safe renderer.**
|
||||
`User.verify_password` (`models/user.py:29`) is currently the **only** code in the app that writes
|
||||
it, so deleting that method deletes the only writer - and `templates/users.jinja2:27` calls
|
||||
`entry.last_logged.strftime(...)` with no NULL guard, which was verified to raise
|
||||
`UndefinedError`. The callback must set `last_logged`, and `/users` must survive a NULL.
|
||||
- **The pre-existing `delete(...)` `NameError` at `views/default.py:142` is fixed in this change.**
|
||||
It sits inside `user_edit`, which this migration edits regardless (lines 93, 98-102, 116-117,
|
||||
119-131, 127, 137). `UserService.delete` already exists at `services/user.py:18`. The trap: if
|
||||
the `'admin'` literal in `user_edit.jinja2:45` is swapped for the `effective_principals` check,
|
||||
the delete button **still renders for admins and the `NameError` survives the migration**.
|
||||
- **Hand-written schema change against the committed `ctp_blogr.sqlite`**, with an explicit warning
|
||||
that this repo has no `alembic.ini` and no `migrations/` directory, so the schema cannot be
|
||||
regenerated. A SQLite **table rebuild** (not a plain `ALTER TABLE`) is required because
|
||||
`users.password` is `nullable=False`. `entries.creator` / `entries.editor` are `Unicode(50)` and
|
||||
are written straight from `request.authenticated_userid` with no length guard, so they are
|
||||
widened in the same rebuild.
|
||||
- **A branch decision and a documented restore path for `ctp_blogr.sqlite` are prerequisites, not
|
||||
afterthoughts.** There is a single commit on `master` with `main` and `origin/*` at the same
|
||||
commit, and this migration performs a destructive rebuild on a tracked file.
|
||||
- **`scope=openid email` only.** `profile` is not requested (no display name or avatar is used).
|
||||
**`hd` is deliberately omitted**: it means "a Workspace account on this domain", and
|
||||
`ctphuoc@gmail.com` is a personal Gmail account, so setting `hd=caotek.fr` would lock the second
|
||||
user out. The database allowlist is the control that actually works.
|
||||
- **A brand-new WebTest harness.** The existing `BaseTest` builds a `testing.DummyRequest`, not a
|
||||
WSGI app, so WebTest cannot drive it - and `BaseTest.init_database()` is **defined but never
|
||||
called** by either existing test, so no table is ever created. "WebTest on the existing fixture"
|
||||
is impossible; the plan therefore builds its own WSGI app on `sqlite:///:memory:` with the Google
|
||||
collaborator stubbed and `pyramid_retry` disabled, covering the happy path and every failure path.
|
||||
- **All user-visible strings, comments and flash messages stay in French**, matching the app.
|
||||
|
||||
### Must NOT have (guardrails, anti-slop, scope boundaries)
|
||||
- **No implicit sign-up.** A Google identity that is not already in `users` is refused; it is
|
||||
never auto-created. This closes the self-registration hole that a naive OAuth migration opens.
|
||||
- **No `profile` scope, no avatar, no display name, no refresh token, no `access_type=offline`,
|
||||
no `hd` parameter, and no call to any Google API.** Sign-in only.
|
||||
- **No Google-side logout or token revocation.** Logout is local session invalidation only.
|
||||
- **No database-backed groupfinder and no `tm.annotate_user` change.** The login allowlist is a DB
|
||||
read performed once in the callback; the group mapping is configuration. Silencing `annotate_user`
|
||||
to dodge the tween problem would break commit/abort semantics and is forbidden.
|
||||
- **No `pyramid_csrf` package and no retrofitting CSRF onto the existing write views.** Pyramid
|
||||
2.1's cookie session already provides `new_csrf_token()` / `get_csrf_token()` under `_csrft_`,
|
||||
so no dependency is needed - but the token is a single long-lived per-session value with no
|
||||
automatic comparison helper, and wiring it into every pre-existing POST view is an unrelated
|
||||
refactor. `state` is the control for the callback itself (a single-use GET bound to the
|
||||
pre-authentication session). The pre-existing absence of CSRF, and `logout` being a
|
||||
state-mutating GET, are recorded as accepted risks, not silently fixed and not silently ignored.
|
||||
- **No server-side session backend.** No `pyramid_beaker`. Measured, not assumed: identity plus
|
||||
`state` + `code_verifier` + `nonce` serializes to **324 bytes**; with ten French flash messages,
|
||||
662. Pyramid raises a hard `ValueError` at **4064 bytes**, leaving ~3.7 KB of headroom. The only
|
||||
growth vector is unbounded flash accumulation, which is bounded by capping flash length, not by
|
||||
adding a backend.
|
||||
- **No migration tooling.** Do not introduce Alembic, do not create a `migrations/` directory,
|
||||
and do not pretend `Base.metadata.create_all()` covers the production schema.
|
||||
- **No use of coverage as an acceptance gate.** `.coveragerc` still says
|
||||
`source = pyramid_blogr` (the old package name) so coverage collects nothing; the number would
|
||||
be fiction. Fix the name only if asked.
|
||||
- **No unrelated refactors beyond the lines this migration must already touch.** Explicitly
|
||||
**out of scope**: the hardcoded post id `95` in `home` and `home.jinja2:18`, the legacy
|
||||
`Query.get()` call in `services/blog_record.py`, the non-raw string at `routes.py:5`, the legacy
|
||||
`declarative_base` in `models/meta.py`, the missing `initialize_db.py` behind the declared
|
||||
`initialize_ctp_blogr_db` console script, and the dead `HistoForm` / `AllocationForm` /
|
||||
`ActifForm` / `Actif2Form` forms matching orphaned tables. The pre-existing draft-leak in
|
||||
`by_id` (no status filter) and the `LIKE` search leak are **recorded, not fixed**: they are
|
||||
unchanged by this migration, and widening the user population is what makes them worth a
|
||||
separate decision.
|
||||
- **No invented linting, formatting, type-checking, CI or task runner.** None exists and this
|
||||
migration does not create any.
|
||||
- **No reduction to an MVP or phase 1.** This is the complete migration, one plan.
|
||||
|
||||
## Verification strategy
|
||||
> Zero human intervention for everything the agent can execute. The single exception is F3's real
|
||||
> Google sign-in, which needs credentials only the user can issue - see the prerequisite below.
|
||||
> Todos 1-14 all run fully offline against a stubbed Google collaborator.
|
||||
>
|
||||
> **User prerequisite, required before F3 only (not before Wave 1):** provision a Google Cloud
|
||||
> OAuth 2.0 client, add the Web application redirect URI
|
||||
> `http://localhost:9280/login/google/callback`, and set `ENV_GOOGLE_CLIENT_ID` /
|
||||
> `ENV_GOOGLE_CLIENT_SECRET`. Todos 1-14 do not need these; F3 cannot pass without them.
|
||||
- Test decision: **tests-after**, WebTest + pytest, on `sqlite:///:memory:` via a **newly built**
|
||||
WSGI test app (the existing `BaseTest` cannot be reused - see Scope). The Google collaborator and
|
||||
the JWKS fetch are injected behind the named seam from todo 7 so no test performs network I/O.
|
||||
`pyramid_retry` is disabled in tests, because single-shot side effects such as consuming `state`
|
||||
are otherwise non-deterministic.
|
||||
- **Never assert a bare 200 in this app.** Unauthorized requests return **200 with the login page**,
|
||||
not 403, because a `forbidden_view_config` swallows `HTTPForbidden`. Every protected-route
|
||||
assertion checks status *and* body content.
|
||||
- Per-todo happy AND failure QA, each writing evidence to
|
||||
`<attemptDir>/task-<N>-google-oauth-migration.<ext>`.
|
||||
- Structural self-check before handoff: every implementation row is column-zero
|
||||
`- [ ] N. <title>` and every final-verifier row is column-zero `- [ ] F<number>. <title>`,
|
||||
both inside their intended section, with no prose bullet standing in for a task.
|
||||
|
||||
### Acceptance criteria (transcribed from the gap analysis, grouped)
|
||||
**Environment / boot**
|
||||
1. `python -c "from pyramid.paster import get_app; get_app('development.ini')"` exits 0, and
|
||||
`python -c "import authlib, joserfc; print(authlib.__version__)"` succeeds.
|
||||
2. `python -m pytest -q` reports the exact new count, `0 failed`, and **no** `no such table` error
|
||||
(which proves the new harness actually creates the schema).
|
||||
|
||||
**Session policy**
|
||||
3. The post-login cookie, read on the **next** request, contains `auth.userid` and **no** key
|
||||
matching `state|code_verifier|nonce`.
|
||||
4. A session older than the configured `timeout` is rejected, asserted with a monkeypatched clock
|
||||
rather than a sleep.
|
||||
5. The factory is constructed with `secure=True` under production settings and `httponly=True` in
|
||||
both; the configured `timeout` is asserted to exceed the previous 20-minute default and its
|
||||
value is written down.
|
||||
|
||||
**Callback**
|
||||
6. Unknown email -> assert the **actual** status (403), that the body contains the French rejection
|
||||
message, and that it contains **no** `<input type="password">`.
|
||||
7. `state` mismatch, missing `state`, and replayed `state` each fail closed, call the token endpoint
|
||||
**zero** times, and leave no session identity.
|
||||
8. `nonce` mismatch is rejected; assert the JWT `nonce` claim is compared against the stored value
|
||||
(the stub returns a mismatching claim).
|
||||
9. `?error=access_denied` -> French flash, no DB write, no session identity.
|
||||
10. Retry safety: with `retry.attempts=3`, a transient error during the callback does **not**
|
||||
produce a second exchange (assert collaborator call count == 1).
|
||||
|
||||
**Access control**
|
||||
11. Authenticated as `ctphuoc@gmail.com`: `GET /user_edit/phuoc@caotek.fr` is refused,
|
||||
`GET /user_edit/0` is refused, `GET /users` does **not** contain `users_list`.
|
||||
12. Same user: `POST /blog_edit/<id>` with `form.submitted` does **not** modify the post, and with
|
||||
`form.deleted` the row is still present.
|
||||
13. `users.name` is restricted to the two allowlisted emails, and
|
||||
`groupfinder('ghost@nowhere.test', request)` returns `None`.
|
||||
|
||||
**Templates / removals**
|
||||
14. `GET /users` returns 200 with a row whose `last_logged` is NULL in the database (no `strftime`
|
||||
`UndefinedError`).
|
||||
15. `GET /user_edit/<email>` returns 200 and the HTML contains **no** `name="password"`.
|
||||
16. `grep -c "route_url('user_edit', name='0')" ctp_blogr/templates/users.jinja2` == 0 and
|
||||
`GET /user_edit/0` is unreachable.
|
||||
17. `grep -rn passlib ctp_blogr/` returns nothing **and** the app still boots (the import-order
|
||||
trap).
|
||||
18. `grep -rn "'admin'" ctp_blogr/` returns no authorization comparison.
|
||||
|
||||
**Schema**
|
||||
19. After the rebuild, `PRAGMA table_info(users)` shows **no** `password` column and `name` still
|
||||
`NOT NULL` + `UNIQUE`; `SELECT COUNT(*) FROM users` == 2 with the correct emails;
|
||||
`PRAGMA foreign_key_check` is clean; the `alembic_version` row is still present and untouched.
|
||||
20. `PRAGMA table_info(entries)` shows `creator`/`editor` widened.
|
||||
|
||||
**Regression**
|
||||
21. `python -m pytest ctp_blogr/tests.py` still passes, or the two legacy duplicate tests are
|
||||
deliberately deleted as part of the harness replacement.
|
||||
22. An explicit smoke pass over all 15 routes asserting **content**, never a bare 200.
|
||||
|
||||
## Execution strategy
|
||||
### Parallel execution waves
|
||||
> Waves are deliberately smaller than the 5-8 target. The gap analysis proved that the session
|
||||
> factory, the auth policy and the group mapping are **one coupled change** (a 20-minute default
|
||||
> timeout plus a database-backed callback inside the transaction tween), and that the destructive
|
||||
> table rebuild must be isolated. Splitting those further would produce intermediate states that
|
||||
> cannot boot.
|
||||
|
||||
- **Wave 1 - prerequisites and foundation (no interdependencies):** dependency manifest and
|
||||
`authlib` install; branch decision plus database backup and restore path; session factory
|
||||
hardening; the `settings` surface for the client id/secret and `auth.admin_emails`.
|
||||
- **Wave 2 - auth core (coupled, land together):** the `SessionAuthenticationPolicy` swap with the
|
||||
settings-based groupfinder; the destructive schema rebuild on the agreed branch; the named
|
||||
Google collaborator with JWKS caching.
|
||||
- **Wave 3 - the flow (depends on Wave 2):** the authorize redirect view; the callback view with
|
||||
state consumption, nonce comparison and a single non-retryable exchange; the allowlist gate with
|
||||
the explicit 403, `remember()`, the `last_logged` write and OAuth-key cleanup.
|
||||
- **Wave 4 - authorization gaps and surface removal (must land in the same change set as Wave 3):**
|
||||
the `/user_edit` permission and ownership fix together with the `delete(...)` `NameError`; the
|
||||
four literal `'admin'` sites; removal of the password surface and the template fixes it forces.
|
||||
- **Wave 5 - tests (depends on all):** the new WSGI harness and the full suite.
|
||||
- **Wave 6 - final verification wave** (F1-F4), all four in parallel, all must APPROVE.
|
||||
|
||||
### Dependency matrix
|
||||
| Todo | Depends on | Blocks | Can parallelize with |
|
||||
| --- | --- | --- | --- |
|
||||
| 1. Dependency manifest + `authlib` install | - | 7, 13 | 2, 3, 4 |
|
||||
| 2. Branch decision + DB backup/restore path | - | 6 | 1, 3, 4 |
|
||||
| 3. Session factory hardening (timeout, secret, httponly, secure) | - | 5, 8, 9 | 1, 2, 4 |
|
||||
| 4. `settings` surface: client id/secret + `auth.admin_emails` | - | 5, 7, 8 | 1, 2, 3 |
|
||||
| 5. `SessionAuthenticationPolicy` + settings-based groupfinder | 3, 4 | 10, 11, 12 | 6, 7 |
|
||||
| 6. Schema rebuild: drop `password`, add `google_sub`, widen creator/editor | 2 | 9, 10, 14 | 5, 7 |
|
||||
| 7. Google collaborator seam + JWKS cache | 1, 4 | 8 | 5, 6 |
|
||||
| 8. Authorize redirect + callback views | 3, 7 | 9, 13 | - |
|
||||
| 9. Allowlist gate + explicit 403 + `last_logged` + key cleanup | 5, 6, 8 | 13 | - |
|
||||
| 10. `/user_edit` authorization + `delete(...)` `NameError` | 5, 6, 9 | 13 | - |
|
||||
| 11. Replace the 4 literal `'admin'` sites | 5, 9 | 13 | 10 |
|
||||
| 12. Remove the password surface + template fixes | 6, 10, 11 | 13 | - |
|
||||
| 13. New WSGI test harness + full suite | 1-12 | F1-F4 | - |
|
||||
| 14. `AGENTS.md` accuracy update | 13 | F1-F4 | - |
|
||||
|
||||
## Todos
|
||||
> Implementation + Test = ONE todo. Never separate.
|
||||
<!-- APPEND TASK BATCHES BELOW THIS LINE WITH edit/apply_patch - never rewrite the headers above. -->
|
||||
|
||||
- [ ] 1. Declare and install `authlib` without corrupting the shared venv
|
||||
What to do / Must NOT do: Add `authlib` to `install_requires` in `setup.py` (which pulls in
|
||||
`joserfc`). Install it into the parent venv at `/Users/phuoc/pyramid2`. MUST NOT casually
|
||||
`pip install` into that venv - `AGENTS.md` records that it is shared and still carries a stale
|
||||
editable install of a `wiki_test` project whose source is gone. MUST NOT add any other new
|
||||
dependency, and MUST NOT introduce `pyramid_csrf` or `pyramid_beaker`.
|
||||
Parallelization: Wave 1 | Blocked by: - | Blocks: 7, 13
|
||||
References (executor has NO interview context - be exhaustive): `setup.py` (install_requires,
|
||||
currently declares the dead `initialize_ctp_blogr_db` console script whose target
|
||||
`ctp_blogr/scripts/initialize_db.py` does not exist); `AGENTS.md` "Environment" section; venv root
|
||||
is the **parent** directory, `python` is already on PATH.
|
||||
Acceptance criteria (agent-executable):
|
||||
`python -c "import authlib, joserfc; print(authlib.__version__)"` exits 0 and prints 1.8.0;
|
||||
`python -c "from pyramid.paster import get_app; get_app('development.ini')"` exits 0;
|
||||
`git diff setup.py` shows exactly one added dependency line.
|
||||
QA scenarios (name the exact tool + invocation): happy - `python -c "import authlib, joserfc"`
|
||||
succeeds after install. failure - `pip install authlib==0.0.0` is NOT run; instead assert
|
||||
`grep -c "authlib" setup.py` == 1 and that `pyramid_csrf`/`pyramid_beaker` are absent.
|
||||
Evidence: `<attemptDir>/task-1-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(deps): add authlib for Google OAuth`, commit only on explicit request.
|
||||
|
||||
- [ ] 2. Decide the branch and establish a restore path for the tracked database
|
||||
What to do / Must NOT do: Surface the branch decision to the user and get an explicit answer
|
||||
before any schema work. `ctp_blogr.sqlite` is **tracked in git** (the `*.sqlite` line in
|
||||
`.gitignore` does nothing for an already-tracked file), there is a single commit on `master`, and
|
||||
`main` and `origin/*` point at it. Produce a dated backup of the database file outside the repo
|
||||
and record the exact restore command. MUST NOT create a branch, commit, or push without the user's
|
||||
answer. MUST NOT modify the schema in this todo.
|
||||
Parallelization: Wave 1 | Blocked by: - | Blocks: 6
|
||||
References (executor has NO interview context - be exhaustive): `AGENTS.md` "Database" and
|
||||
"Conventions" sections; `development.ini` and `production.ini` both point at the file via
|
||||
`%(here)s`; the file contains `alembic_version` rev `b8f8216d72c7`, ~99 posts and 8 tags.
|
||||
Acceptance criteria (agent-executable): a backup file exists outside the working tree and
|
||||
`sqlite3 <backup> "PRAGMA integrity_check"` returns `ok`; the restore command is written into the
|
||||
plan's commit strategy; `git status --porcelain ctp_blogr.sqlite` is still clean at the end of
|
||||
this todo.
|
||||
QA scenarios: happy - backup created and verified with `PRAGMA integrity_check`. failure - confirm
|
||||
`git checkout -- ctp_blogr.sqlite` would restore the pre-migration state, and record that the
|
||||
working tree is untouched so far.
|
||||
Evidence: `<attemptDir>/task-2-google-oauth-migration.<ext>`
|
||||
Commit: N - documentation only, no commit.
|
||||
|
||||
- [ ] 3. Harden and reconfigure the session factory
|
||||
What to do / Must NOT do: Replace the bare `SignedCookieSessionFactory('hZug2zPt7hT2MZ')` with an
|
||||
explicit construction: a secret of at least 64 characters for `hashalg='sha512'` (currently 16),
|
||||
an explicit `timeout` long enough to survive a Google consent screen plus sign-in plus 2FA (the
|
||||
default is 1200s / 20 min against the outgoing policy's 36000s / 10 h), `httponly=True` in both
|
||||
environments, and `secure=True` driven by settings so production gets it. MUST NOT add
|
||||
`pyramid_beaker`. MUST NOT call `request.session.renew()` - **that method does not exist in
|
||||
Pyramid 2.1**, and neither do `SessionRenew` / `SessionInvalidate`. MUST NOT leave the factory
|
||||
defaults in place, because the cookie becomes the identity.
|
||||
Parallelization: Wave 1 | Blocked by: - | Blocks: 5, 8, 9
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/__init__.py:13` (the
|
||||
factory) and `:15` (the hardcoded auth-ticket secret being removed); the installed factory source
|
||||
confirms `_timeout: 1200`, `_cookie_secure: False`, `_cookie_httponly: False`,
|
||||
`_cookie_samesite: 'Lax'`, `_cookie_on_exception: True`; `production.ini:29` declares
|
||||
`url_scheme = https` while the cookie still ships no `Secure` flag;
|
||||
`templates/layout.jinja2:14-15,106-107` load third-party CDN scripts, so `httponly=False` is a
|
||||
real XSS-to-identity-theft path once the cookie carries the identity. `SameSite=Lax` is already
|
||||
correct for a top-level GET callback - do not change it to `None`.
|
||||
Acceptance criteria (agent-executable): `grep -n "SignedCookieSessionFactory" ctp_blogr/__init__.py`
|
||||
shows named kwargs for `timeout`, `httponly`, `secure` and the secret; the secret is >= 64 chars
|
||||
(assert with a Python one-liner); `python -c "from pyramid.paster import get_app;
|
||||
get_app('development.ini')"` exits 0; a `Set-Cookie` from a test request carries `HttpOnly`, and
|
||||
carries `Secure` when production settings are loaded.
|
||||
QA scenarios: happy - boot check passes and the header assertions hold. failure - assert a
|
||||
monkeypatched clock past the configured `timeout` causes the identity to be rejected (criterion 4),
|
||||
with no `sleep` in the test.
|
||||
Evidence: `<attemptDir>/task-3-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `fix(auth): harden and configure the signed session cookie`.
|
||||
|
||||
- [ ] 4. Add the `settings` surface for the Google client and the admin email list
|
||||
What to do / Must NOT do: Read `google.client_id` and `google.client_secret` from settings via
|
||||
PasteDeploy environment interpolation (`%(ENV_GOOGLE_CLIENT_ID)s`,
|
||||
`%(ENV_GOOGLE_CLIENT_SECRET)s`) in both `development.ini` and `production.ini`, and add
|
||||
`auth.admin_emails` as a parsed list (`phuoc@caotek.fr`). Fail fast with a clear French message if
|
||||
the credentials are missing. MUST NOT commit any real credential, and MUST NOT invent a settings
|
||||
key that nothing reads.
|
||||
Parallelization: Wave 1 | Blocked by: - | Blocks: 5, 7, 8
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/__init__.py` `main()`
|
||||
(reads `settings`); `development.ini` and `production.ini`; `ctp_blogr/security.py` (the 3-line
|
||||
ACL whose `group:administrators` principal this list feeds).
|
||||
Acceptance criteria (agent-executable): `grep -n "auth.admin_emails" ctp_blogr/*.ini` shows both
|
||||
files; the groupfinder receives `['phuoc@caotek.fr']` from settings; starting the app with the
|
||||
env vars unset raises a clear configuration error naming the missing variable, and
|
||||
`git grep -n "client_secret" ` shows only the interpolation, never a literal value.
|
||||
QA scenarios: happy - app boots with env vars set. failure - app refuses to boot with a readable
|
||||
message when they are absent.
|
||||
Evidence: `<attemptDir>/task-4-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(auth): add Google OAuth and admin email settings`.
|
||||
|
||||
- [ ] 5. Swap `AuthTkt` for `SessionAuthenticationPolicy` with a settings-based groupfinder
|
||||
What to do / Must NOT do: Replace `AuthTktAuthenticationPolicy` (and its hardcoded secret) with
|
||||
Pyramid's `SessionAuthenticationPolicy` + `SessionAuthenticationHelper`, keeping
|
||||
`root_factory='ctp_blogr.security.RootFactory'` and `ACLAuthorizationPolicy` so `security.py` is
|
||||
untouched. Rewrite the `groupfinder` callback so it maps email -> group **purely from
|
||||
`auth.admin_emails` in settings**. MUST NOT query the database from the callback:
|
||||
`pyramid_tm` defaults `tm.annotate_user` to `True` and calls `request.authenticated_userid`
|
||||
inside `manager.begin()`, so a DB-backed groupfinder runs a SELECT on every request inside the
|
||||
transaction, and any DB error 500s every route including `/login` and the callback itself. MUST
|
||||
NOT "fix" this by setting `tm.annotate_user = false`. Unknown email must return `None`, not raise.
|
||||
Parallelization: Wave 2 | Blocked by: 3, 4 | Blocks: 10, 11, 12
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/__init__.py:15` (the
|
||||
policy block and secret); `ctp_blogr/services/user.py:26` (the current `userid == 'admin'`
|
||||
literal to remove); `ctp_blogr/security.py`; installed
|
||||
`pyramid_tm/__init__.py` `annotate_user = asbool(settings.get('tm.annotate_user', True))` and the
|
||||
`request.authenticated_userid` access inside `manager.begin()`;
|
||||
`models/__init__.py` sets `settings['tm.manager_hook'] = 'pyramid_tm.explicit_manager'`.
|
||||
Acceptance criteria (agent-executable): `groupfinder('phuoc@caotek.fr', request)` returns
|
||||
`['group:administrators']`; `groupfinder('ctphuoc@gmail.com', request)` returns `[]`;
|
||||
`groupfinder('ghost@nowhere.test', request)` returns `None`; no SQL is emitted during a
|
||||
groupfinder call (assert with an engine event listener); `grep -rn "'admin'" ctp_blogr/` returns
|
||||
nothing; the ACL file is byte-identical to its pre-change content.
|
||||
QA scenarios: happy - both known identities resolve to the expected principals and `/users` is
|
||||
reachable only for the admin. failure - `groupfinder` on an unknown email returns `None` and
|
||||
creates no row; a request with the database deliberately broken still serves `/login` (this is
|
||||
the B3 regression test).
|
||||
Evidence: `<attemptDir>/task-5-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `refactor(auth): swap AuthTkt for SessionAuthenticationPolicy`.
|
||||
|
||||
- [ ] 6. Rebuild the `users` table and widen `entries.creator` / `entries.editor`
|
||||
What to do / Must NOT do: Against the backed-up `ctp_blogr.sqlite`, rebuild `users` to drop the
|
||||
`password` column (it is `nullable=False`, so a plain `ALTER TABLE ... DROP COLUMN` is not the
|
||||
safe route), add a nullable unique `google_sub`, keep `id` / `name` (`name` stores the allowlisted
|
||||
email and stays `NOT NULL` + `UNIQUE`) and `last_logged`. Set row 1 `name` to
|
||||
`phuoc@caotek.fr` and row 2 to `ctphuoc@gmail.com`. Widen `entries.creator` and `entries.editor`
|
||||
from `Unicode(50)` to `Unicode(255)` in the same rebuild. Back up first (todo 2). MUST NOT
|
||||
introduce Alembic or a `migrations/` directory - none exists in this repo. MUST NOT touch the
|
||||
`alembic_version` row (rev `b8f8216d72c7`), even though no Alembic is wired up. MUST NOT drop or
|
||||
alter the `entries` rows themselves. Report clearly that migrations are missing.
|
||||
Parallelization: Wave 2 | Blocked by: 2 | Blocks: 9, 10, 14
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/models/user.py`
|
||||
(columns, `password nullable=False`); `ctp_blogr/models/blog_record.py` (`creator`/`editor`
|
||||
`Unicode(50)`); `ctp_blogr/models/__init__.py:8-9` (`Tags` is defined in `blog_record` but not
|
||||
imported here - import it explicitly if mapper config is touched); `ctp_blogr/views/blog.py:64,75`
|
||||
(the unguarded writes); `AGENTS.md` "Database" section (the DB file is the schema source of
|
||||
truth; `create_all()` is test-only).
|
||||
Acceptance criteria (agent-executable): `PRAGMA table_info(users)` shows no `password` column,
|
||||
`name` still `NOT NULL` + `UNIQUE`, and a `google_sub` column; `SELECT COUNT(*) FROM users` == 2
|
||||
with exactly `phuoc@caotek.fr` and `ctphuoc@gmail.com`; `PRAGMA table_info(entries)` shows
|
||||
`creator`/`editor` widened; `PRAGMA foreign_key_check` is clean;
|
||||
`SELECT * FROM alembic_version` still returns `b8f8216d72c7`;
|
||||
`SELECT COUNT(*) FROM entries` is unchanged from the pre-migration count;
|
||||
`python -m pytest -q` shows no `no such table`.
|
||||
QA scenarios: happy - all the assertions above pass against the real file. failure - run every
|
||||
assertion against the **backup** first to confirm the script is not silently a no-op, and confirm
|
||||
the restore path from todo 2 actually restores a working app.
|
||||
Evidence: `<attemptDir>/task-6-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(db): migrate users to Google identity and widen entry authors`. The
|
||||
database diff is a real tracked change and must be inspected before any commit.
|
||||
|
||||
- [ ] 7. Build the named Google OAuth collaborator with JWKS caching
|
||||
What to do / Must NOT do: Add a registry-registered service (e.g.
|
||||
`ctp_blogr/services/google_oauth.py`) wrapping `OAuth2Session` from
|
||||
`authlib.integrations.requests_client`, configured from the discovery document so
|
||||
`authorize_url` / `access_token_url` / `jwks_uri` are never hand-written. Expose exactly two
|
||||
operations: `create_authorization_url(...)` and `fetch_claims(code, code_verifier, ...)`. The
|
||||
service MUST fetch **and cache** the discovery document and the JWKS itself with a bounded TTL,
|
||||
because the bare client does neither: `CodeIDToken` plus key decoding needs the keys pulled from
|
||||
`jwks_uri` manually, and refetching on every sign-in is a latency and rate-limit problem. Use
|
||||
**`joserfc`** (Authlib's own forward dependency) to decode and verify the `id_token`, **not**
|
||||
`authlib.jose`, which is soft-deprecated in 1.8.0 and warns. Set `hd` nowhere.
|
||||
MUST NOT use `authorize_redirect` / `save_authorize_data` / `set_state_data` /
|
||||
`clear_state_data` - those are framework integrations that do not exist for Pyramid.
|
||||
Parallelization: Wave 2 | Blocked by: 1, 4 | Blocks: 8
|
||||
References (executor has NO interview context - be exhaustive): discovery URL
|
||||
`https://accounts.google.com/.well-known/openid-configuration`; Authlib entry point
|
||||
`authlib.integrations.requests_client.OAuth2Session`; `create_authorization_url` returns the tuple
|
||||
`(uri, state)` and does **not** return `code_verifier`, which the app generates with
|
||||
`generate_token(48)` and passes in itself; Google's OIDC reference
|
||||
(https://developers.google.com/identity/openid-connect/reference) requires `nonce`;
|
||||
`development.ini` / `production.ini` settings from todo 4.
|
||||
Acceptance criteria (agent-executable): the service is reachable from
|
||||
`request.registry.google_oauth`; `create_authorization_url` returns a `state` and an absolute
|
||||
`accounts.google.com` URI containing `code_challenge_method=S256`, a `code_challenge`, an `openid`
|
||||
scope and **no** `hd` parameter; two consecutive `fetch_claims` calls hit the JWKS endpoint once
|
||||
(assert with a counting stub); the cache honours its TTL under a monkeypatched clock.
|
||||
QA scenarios: happy - authorization URL is well-formed and JWKS is fetched once. failure - a JWKS
|
||||
fetch failure surfaces a French error and never reaches the token endpoint; a cached-key path still
|
||||
validates a token after the TTL expires.
|
||||
Evidence: `<attemptDir>/task-7-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(auth): add Google OAuth client with state, PKCE and nonce`.
|
||||
|
||||
- [ ] 8. Add the authorize redirect and the callback views
|
||||
What to do / Must NOT do: Add a `/login/google` route that generates `state`, `nonce` and
|
||||
`code_verifier`, stores all three in the session, and redirects. Add the `/login/google/callback`
|
||||
GET which: rejects `?error=access_denied` with a French flash and no DB write; consumes
|
||||
(deletes) `state` **before** attempting the exchange so a replay cannot reach the token endpoint;
|
||||
exchanges the code exactly once with the stored `code_verifier` and `redirect_uri`; compares the
|
||||
`id_token` `nonce` claim against the stored value; deletes `state` / `code_verifier` / `nonce`;
|
||||
and then defers to todo 9 for the allowlist gate and `remember()`. MUST NOT call
|
||||
`request.session.renew()` (it does not exist). MUST NOT let `pyramid_retry` replay the exchange:
|
||||
`pyramid_tm` marks exceptions retryable and a SQLite lock would re-run the callback into
|
||||
`invalid_grant`. Verify with a call-count assertion under `retry.attempts=3`.
|
||||
Parallelization: Wave 3 | Blocked by: 3, 7 | Blocks: 9, 13
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/routes.py` (add both
|
||||
routes; note `:5` is a non-raw string emitting `SyntaxWarning` on 3.12+ - prefix with `r` if you
|
||||
edit that file, the new routes are purely additive); `ctp_blogr/views/default.py:37-74` (the
|
||||
existing `login` / `logout` / `forget` pattern and its `permission` / `renderer` conventions);
|
||||
`development.ini` (`retry.attempts = 3`); installed `pyramid_tm` retryable-error tagging.
|
||||
Acceptance criteria (agent-executable): `state` mismatch, missing `state`, and replayed `state`
|
||||
each fail closed, call the collaborator **zero** times, and leave no session identity; `nonce`
|
||||
mismatch is rejected with a stubbed mismatching claim; `?error=access_denied` produces a French
|
||||
flash, no DB write and no session identity; with `retry.attempts=3` and an injected transient
|
||||
error the collaborator is called exactly once; after a successful callback no session key matches
|
||||
`state|code_verifier|nonce`.
|
||||
QA scenarios: happy - full redirect-URI round trip with a stubbed token endpoint yields verified
|
||||
claims. failure - the six rejection cases above, each asserting zero token-endpoint calls.
|
||||
Evidence: `<attemptDir>/task-8-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(auth): add authorize and callback views`.
|
||||
|
||||
- [ ] 9. Gate sign-in on the database allowlist, then remember the identity
|
||||
What to do / Must NOT do: In the callback, after claims are verified, match the identity against
|
||||
the `users` table: `google_sub` first, then the allowlisted email. A match updates `last_logged`
|
||||
(the **only** writer, since `verify_password` is being deleted) and stores `google_sub` on first
|
||||
sign-in, then calls `remember(email)`. **No match is refused** and no row is ever created. The
|
||||
refusal MUST set `request.response.status_int = 403` explicitly and render a French rejection
|
||||
template **without raising `HTTPForbidden`**, because `forbidden_view_config` on login
|
||||
(`views/default.py:38`) catches that exception and renders `login.jinja2` with status **200** -
|
||||
a rejected user would otherwise be shown the dead password form. Replace that
|
||||
`forbidden_view_config` with a real 403 view rather than inheriting it. MUST NOT auto-provision.
|
||||
MUST NOT query the DB for the group mapping (todo 5).
|
||||
Parallelization: Wave 3 | Blocked by: 5, 6, 8 | Blocks: 13
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/views/default.py:38`
|
||||
(the `forbidden_view_config` that swallows `HTTPForbidden`), `:53-58` (existing `remember()` /
|
||||
`forget()` call style), `:68-74` (logout); `ctp_blogr/models/user.py:29` (the `last_logged` write
|
||||
being removed with `verify_password`); `templates/users.jinja2:27` (`last_logged.strftime` with no
|
||||
NULL guard); Google's production-readiness note that the test-user allowlist is bypassed for
|
||||
basic scopes, which is why this DB gate is mandatory rather than defence-in-depth.
|
||||
Acceptance criteria (agent-executable): unknown email -> status 403, body contains the French
|
||||
rejection message, body contains no `<input type="password">`, and
|
||||
`SELECT COUNT(*) FROM users` is unchanged; `phuoc@caotek.fr` yields `auth.userid` equal to that
|
||||
email plus the `manage` principal and a non-NULL `last_logged`; `ctphuoc@gmail.com` yields a
|
||||
session **without** `manage`; a second sign-in matches on `google_sub`.
|
||||
QA scenarios: happy - both known identities sign in with the right principals. failure - unknown
|
||||
email refused at 403 with no row created; a tampered `sub`/email pair refused.
|
||||
Evidence: `<attemptDir>/task-9-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `feat(auth): gate sign-in on a database allowlist`.
|
||||
|
||||
- [ ] 10. Close the `/user_edit` authorization hole and fix the `delete(...)` `NameError`
|
||||
What to do / Must NOT do: `views/default.py:88` declares `permission='view'`, **not** `manage`,
|
||||
and the only identity check in the function is `== 'admin'` for the return URL. Any authenticated
|
||||
user can therefore open `/user_edit/<any-email>` and rewrite that row, or `/user_edit/0` to
|
||||
create one. Change the permission to `manage` and remove the create-user branch
|
||||
(`:98-102,119-131`) so `name='0'` is unreachable; if an admin-only allowlist editor is kept, it
|
||||
must require `manage` and must not accept a password. In the same change, fix `:142`
|
||||
`delete(request, user.id)` -> `UserService.delete(request, user.id)`; that method already exists
|
||||
at `services/user.py:18`. Record that it is a `filter().delete(synchronize_session=False)` with
|
||||
**no FK cascade** from `entries.creator` / `entries.editor`, so deleting a user strands PII in
|
||||
those columns. MUST NOT leave the `NameError` live: `user_edit.jinja2:45` gates the delete button
|
||||
on `request.authenticated_userid == 'admin'`, and swapping that literal for the
|
||||
`effective_principals` check keeps the button rendering for admins.
|
||||
Parallelization: Wave 4 | Blocked by: 5, 6, 9 | Blocks: 13
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/views/default.py:88`
|
||||
(permission), `:93` (`'admin'` literal), `:98-102` and `:119-131` (create-user branch),
|
||||
`:116-117` (`len(form.password.data) < 6`), `:127` and `:137` (`set_password`),
|
||||
`:142` (the `NameError`); `ctp_blogr/services/user.py:18` (`delete`);
|
||||
`ctp_blogr/templates/user_edit.jinja2:45` (delete-button gate);
|
||||
`ctp_blogr/templates/users.jinja2` (the `route_url('user_edit', name='0')` create link).
|
||||
Acceptance criteria (agent-executable): as `ctphuoc@gmail.com`,
|
||||
`GET /user_edit/phuoc@caotek.fr` and `GET /user_edit/0` are both refused and the response body
|
||||
does not contain the edit form; `grep -c "route_url('user_edit', name='0')"
|
||||
ctp_blogr/templates/users.jinja2` == 0; as admin, the delete button renders **and** submitting it
|
||||
deletes the row without raising; the other user row is untouched.
|
||||
QA scenarios: happy - admin edits and deletes correctly. failure - non-admin is refused on both
|
||||
URLs (content assertion, never a bare 200, because forbidden responses render the login page at
|
||||
200 in this app).
|
||||
Evidence: `<attemptDir>/task-10-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `fix(auth): require manage on user_edit and fix user deletion`.
|
||||
|
||||
- [ ] 11. Replace the four literal `'admin'` comparisons with `effective_principals`
|
||||
What to do / Must NOT do: Replace every authorization comparison against the literal `'admin'`
|
||||
with `'group:administrators' in request.effective_principals`: `services/user.py:26`,
|
||||
`views/default.py:93`, `templates/layout.jinja2:43`, `templates/user_edit.jinja2:45`. Templates
|
||||
cannot call a groupfinder, which is why `effective_principals` is the correct template-side
|
||||
expression. Audit **all 15** `request.authenticated_userid` sites (7 Python, 8 templates) so none
|
||||
is left comparing an identity to a name. MUST NOT leave any `== 'admin'` behind.
|
||||
Parallelization: Wave 4 | Blocked by: 5, 9 | Blocks: 13
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/services/user.py:26`;
|
||||
`ctp_blogr/views/default.py:93`; `ctp_blogr/templates/layout.jinja2:43` and `:46`;
|
||||
`ctp_blogr/templates/user_edit.jinja2:45`;
|
||||
`ctp_blogr/services/blog_record.py` x3, `ctp_blogr/views/blog.py:64,75`, x2,
|
||||
`ctp_blogr/views/default.py` x2, `templates/layout.jinja2` x4, `templates/blog.jinja2` x2,
|
||||
`templates/home.jinja2` x1, `templates/user_edit.jinja2` x1.
|
||||
Acceptance criteria (agent-executable): `grep -rn "'admin'" ctp_blogr/` returns no authorization
|
||||
comparison; as `phuoc@caotek.fr` the admin navigation item renders and `/users` is reachable; as
|
||||
`ctphuoc@gmail.com` it does not render and `/users` does not contain `users_list`.
|
||||
QA scenarios: happy - both identities see exactly the navigation they should. failure - the
|
||||
non-admin sees no admin-only control in any of the 15 audited sites.
|
||||
Evidence: `<attemptDir>/task-11-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `refactor(auth): replace literal admin checks with effective_principals`.
|
||||
|
||||
- [ ] 12. Remove the password surface and fix every template it breaks
|
||||
What to do / Must NOT do: Remove the `password` / `confirm` WTForms fields and the
|
||||
`len(form.password.data) < 6` check, `User.verify_password` / `set_password`, the top-level
|
||||
`import passlib` at `models/user.py:11`, and the `passlib` dependency. Fix every template that
|
||||
renders a removed field, because Jinja2's default `Undefined` raises on attribute access:
|
||||
`user_edit.jinja2:27-28,36-37`. Make `users.jinja2:27` NULL-safe for `last_logged`. Remove the
|
||||
non-admin self-service password path and the admin "Nouvel utilisateur" button. MUST NOT remove
|
||||
`import passlib` last - leaving it breaks the entire app at import time, since the app will not
|
||||
even boot. MUST NOT touch the dead `HistoForm` / `AllocationForm` / `ActifForm` / `Actif2Form`
|
||||
forms or their orphaned `histo` / `allocation` / `actifs` tables.
|
||||
Parallelization: Wave 4 | Blocked by: 6, 10, 11 | Blocks: 13
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/forms.py` (password /
|
||||
confirm fields; plus the dead forms to ignore);
|
||||
`ctp_blogr/models/user.py:11` (import) and `:29` (`last_logged` writer);
|
||||
`ctp_blogr/views/default.py:116-117,127,137,98-102,119-131`;
|
||||
`ctp_blogr/templates/user_edit.jinja2:27-28,36-37`;
|
||||
`ctp_blogr/templates/users.jinja2:27`;
|
||||
`ctp_blogr/templates/login.jinja2` (replaced by a single "Se connecter avec Google" control);
|
||||
`setup.py` (the `passlib` requirement).
|
||||
Acceptance criteria (agent-executable): `grep -rn passlib ctp_blogr/` returns nothing **and**
|
||||
`python -c "from pyramid.paster import get_app; get_app('development.ini')"` still exits 0 (the
|
||||
import-order trap); `GET /users` returns 200 for a row whose `last_logged` is NULL; `GET
|
||||
/user_edit/<email>` returns 200 with no `name="password"` in the HTML; `grep -rn "form.password"
|
||||
ctp_blogr/` returns nothing.
|
||||
QA scenarios: happy - `/users` and `/user_edit/<email>` render clean with no password markup.
|
||||
failure - the NULL-`last_logged` row renders 200; the app boots with `passlib` uninstalled from
|
||||
the source tree.
|
||||
Evidence: `<attemptDir>/task-12-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `refactor(auth): remove password forms and password storage`.
|
||||
|
||||
- [ ] 13. Build a new WSGI test harness and the full offline suite
|
||||
What to do / Must NOT do: Build a **new** WSGI test app - the existing `BaseTest` constructs a
|
||||
`testing.DummyRequest`, so WebTest cannot drive it, and `BaseTest.init_database()` is defined but
|
||||
**never called**, so no table is ever created. The harness must create a real router on
|
||||
`sqlite:///:memory:` with `Base.metadata.create_all()`, register the stubbed Google
|
||||
collaborator from todo 7, and disable `pyramid_retry`. Cover the happy path and every failure
|
||||
path in the Acceptance criteria above: unknown email, state mismatch / missing / replay, nonce
|
||||
mismatch, `error=access_denied`, retry-safety call count, the `/user_edit` and `/users`
|
||||
authorization refusals, `POST /blog_edit` not mutating for a non-admin, the NULL-`last_logged`
|
||||
render, and the two legacy duplicate tests. MUST NOT perform network I/O. MUST NOT assert a bare
|
||||
200 as proof of access. Decide explicitly whether the two legacy duplicate tests are kept or
|
||||
deliberately deleted.
|
||||
Parallelization: Wave 5 | Blocked by: 1-12 | Blocks: F1-F4
|
||||
References (executor has NO interview context - be exhaustive): `ctp_blogr/tests.py`
|
||||
(`BaseTest`, `DummyRequest`, the uncalled `init_database`, the two duplicate tests);
|
||||
`pyramid.testing.DummyRequest` for the lighter alternative; `ctp_blogr/models/__init__.py`;
|
||||
the "Testing" section of `AGENTS.md` (pytest.ini sets `testpaths = ctp_blogr` and
|
||||
`python_files = test*.py`, so a new file must be named `test*.py` inside the package).
|
||||
Acceptance criteria (agent-executable): `python -m pytest -q` exits 0 with `0 failed` and an
|
||||
exact reported count, no `no such table` errors, and no outbound socket (assert by blocking
|
||||
`socket.socket` in a fixture); criteria 1-22 from Verification all pass.
|
||||
QA scenarios: happy - the complete sign-in flow passes end to end offline. failure - each of the
|
||||
ten failure criteria fails closed, most importantly that a non-admin cannot reach
|
||||
`/user_edit/<other-email>` and that a replayed `state` never reaches the token endpoint.
|
||||
Evidence: `<attemptDir>/task-13-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `test(auth): cover the Google OAuth flow without network access`.
|
||||
|
||||
- [ ] 14. Update `AGENTS.md` so it still describes reality
|
||||
What to do / Must NOT do: Rewrite the "Architecture / wiring" section for the new policy and the
|
||||
settings-based admin list, and correct "Known-broken" for the items this migration resolves: the
|
||||
`delete(...)` `NameError`, the 200-not-403 forbidden behaviour and its `forbidden_view_config`
|
||||
cause, and the auth-ticket secret. Remove the now-false statement that admin is a hardcoded
|
||||
`userid == 'admin'`. MUST NOT invent linting, CI or a task runner, and MUST NOT claim any command
|
||||
was run that was not. Note that coverage is still broken and still must not be trusted.
|
||||
Parallelization: Wave 5 | Blocked by: 13 | Blocks: F1-F4
|
||||
References (executor has NO interview context - be exhaustive): `AGENTS.md` "Architecture /
|
||||
wiring", "Known-broken", "Conventions" and "Testing" sections.
|
||||
Acceptance criteria (agent-executable): `grep -n "AuthTkt" AGENTS.md` returns nothing;
|
||||
`grep -n "userid == 'admin'" AGENTS.md` returns nothing; every command listed in `AGENTS.md`
|
||||
"Commands" still runs as written.
|
||||
QA scenarios: happy - the documented wiring matches the code. failure - a reviewer greps each
|
||||
`AGENTS.md` claim and confirms it against the source.
|
||||
Evidence: `<attemptDir>/task-14-google-oauth-migration.<ext>`
|
||||
Commit: N - propose `docs: update AGENTS.md for the Google OAuth migration`.
|
||||
|
||||
## Final verification wave
|
||||
> Runs in parallel after ALL todos. ALL must APPROVE. Surface results and wait for the user's explicit okay before declaring complete.
|
||||
- [ ] F1. Plan compliance audit - confirm every todo's acceptance criteria and all 22 criteria in
|
||||
Verification were actually executed, with evidence files present.
|
||||
- [ ] F2. Code quality review - re-read the diff for the failure modes the gap analysis surfaced:
|
||||
retry replay, stale session values, the `passlib` import order, `UndefinedError` in templates, and
|
||||
any remaining literal identity comparison.
|
||||
- [ ] F3. Real manual QA - boot `pserve development.ini`, complete a real Google sign-in end to end
|
||||
in a browser, and smoke all 15 routes asserting **content** rather than bare 200. **Requires the
|
||||
Google Cloud OAuth client and registered redirect URI from the Verification prerequisite; if the
|
||||
user has not provisioned them, report F3 as BLOCKED-ON-USER rather than APPROVE or REJECT.**
|
||||
- [ ] F4. Scope fidelity - confirm nothing outside Scope changed: no `pyramid_csrf`, no
|
||||
`pyramid_beaker`, no Alembic, no `hd`, no offline access, no auto sign-up, no new tooling, and no
|
||||
change to the out-of-scope items (post id 95, `Query.get()`, `routes.py:5`, `declarative_base`,
|
||||
the dead forms, the draft-leak filters).
|
||||
|
||||
## Commit strategy
|
||||
The pre-existing history is two commits on `master` - `411ca8d "Inital commit since swicthing to
|
||||
python 3.14"` followed by `ea830da "opencode /init done"`; `main` and `origin/*` point at the same
|
||||
tip and the branch choice is **unsettled**. Per todo 2, an explicit branch decision is a
|
||||
prerequisite and no branch is created, no push is made, and nothing is committed unless the user
|
||||
asks - `AGENTS.md` states "Commit only when asked".
|
||||
|
||||
Suggested messages, matching the repo's terse existing style:
|
||||
- `feat(deps): add authlib for Google OAuth`
|
||||
- `fix(auth): harden and configure the signed session cookie`
|
||||
- `feat(auth): add Google OAuth and admin email settings`
|
||||
- `refactor(auth): swap AuthTkt for SessionAuthenticationPolicy`
|
||||
- `feat(db): migrate users to Google identity and widen entry authors`
|
||||
- `feat(auth): add Google OAuth client with state, PKCE and nonce`
|
||||
- `feat(auth): add authorize and callback views`
|
||||
- `feat(auth): gate sign-in on a database allowlist`
|
||||
- `fix(auth): require manage on user_edit and fix user deletion`
|
||||
- `refactor(auth): replace literal admin checks with effective_principals`
|
||||
- `refactor(auth): remove password forms and password storage`
|
||||
- `test(auth): cover the Google OAuth flow without network access`
|
||||
- `docs: update AGENTS.md for the Google OAuth migration`
|
||||
|
||||
**Restore path for `ctp_blogr.sqlite`** (established in todo 2, required before todo 6):
|
||||
`<backup path from todo 2>` restores the pre-migration database, and
|
||||
`git checkout -- ctp_blogr.sqlite` restores the committed version. The database diff is a real
|
||||
tracked change and must be inspected before any commit.
|
||||
|
||||
## Success criteria
|
||||
- Signing in with `phuoc@caotek.fr` yields a session whose `authenticated_userid` is that email
|
||||
and which carries the `group:administrators` principal; the `/users` page is reachable.
|
||||
- Signing in with `ctphuoc@gmail.com` yields a session WITHOUT `manage`; `/users` does not expose
|
||||
the user list.
|
||||
- Signing in with any other Google account produces an explicit **403** with a French message and
|
||||
**no** password input on the page, creates no database row, and makes no group lookup.
|
||||
- The password column, passlib, the password form fields, and the local login form are gone from
|
||||
the codebase; no route, template or form field still references them, and the app still boots.
|
||||
- No source line compares an identity to the literal `'admin'`; the admin mapping is the
|
||||
`auth.admin_emails` setting.
|
||||
- No session key matching `state|code_verifier|nonce` survives a completed sign-in.
|
||||
- A replayed or mismatched `state`, and a mismatched `nonce`, each reach the token endpoint zero
|
||||
times and leave no session identity.
|
||||
- `python -m pytest` passes with the new tests, no test performs network I/O, and no test asserts a
|
||||
bare 200 as proof of access.
|
||||
- `pserve development.ini` serves `/`, and `/login` presents a single "Se connecter avec Google"
|
||||
control instead of a credential form.
|
||||
- `AGENTS.md` is updated: its "Architecture / wiring" and "Known-broken" sections still describe
|
||||
reality after the migration (it currently documents the AuthTkt + groupfinder wiring, the
|
||||
200-not-403 behaviour, and the `delete(...)` NameError that this plan fixes).
|
||||
Reference in New Issue
Block a user