diff --git a/.omo/drafts/google-oauth-migration.md b/.omo/drafts/google-oauth-migration.md new file mode 100644 index 0000000..6a9f8d0 --- /dev/null +++ b/.omo/drafts/google-oauth-migration.md @@ -0,0 +1,443 @@ +--- +slug: google-oauth-migration +intent: clear +review_required: false +classification: architecture +status: drafting +pending-action: write .omo/plans/google-oauth-migration.md +approach: +--- + +# Draft: google-oauth-migration + +## Components (topology ledger) + + +| id | outcome | status | evidence path | +|----|---------|--------|---------------| +| C1 | Google OAuth client (lib + registration + settings) wired into the app factory | active | `ctp_blogr/__init__.py:9-27` | +| C2 | Login/logout views replaced by authorize-redirect + callback | active | `ctp_blogr/views/default.py:37-76` | +| C3 | User identity model migrated from password hash to Google `sub` | active | `ctp_blogr/models/user.py:14-34`, `ctp_blogr.sqlite` `users` table | +| C4 | ACL/groupfinder admin mapping survives the identity change | active | `ctp_blogr/services/user.py:22-32`, `ctp_blogr/security.py:7-14` | +| C5 | Login UI + password forms retired | active | `ctp_blogr/templates/login.jinja2`, `ctp_blogr/forms.py:28-33` | +| C6 | Test strategy for auth without network calls to Google | active | `ctp_blogr/tests.py` | + +## External findings - Google policy (developers.google.com, read 2026-10-05) +- **DECISIVE FINDING: Google's consent-screen test-user allowlist does NOT restrict access + for this app.** Google's own production-readiness table states, for Publishing Status + `Testing` / External: "Only users explicitly added to the test user allowlist can access the + app (limited to a hard cap of 100 test users). **Exception: If the app only requests basic + identity scopes (`openid`, `email`, `profile`), any user can access without being on the + allowlist.**" -> With exactly the scopes this blog needs, the allowlist is bypassed. The + restriction MUST be enforced in application code. This is now a verified fact, not a + precaution, and it is the reason the access-control question is unavoidable. + (Corroborated by a StackOverflow report of non-test-users successfully completing the flow.) +- `scope`: "The list must include the `openid` value and then include the `profile` value, the + `email` value, or both." Google states a basic request should be `openid email`. So + `profile` is only needed for display name / picture -> **use `openid email` only**; requesting + `profile` adds PII for no functional gain in this app. +- `nonce` is listed as **(Required)** in Google's OIDC authentication-request parameters: + "A random value generated by your app that enables replay protection." The plan MUST generate + a nonce on the authorize step and validate it against the `id_token` claim on callback. +- `response_type=code` for web server applications; the implicit flow (`token`, `id_token`, + `token id_token`) is described as "strongly discouraged" and "forbidden in OAuth 2.1". + OAuth 2.1 guidance: "PKCE ... Must be used to secure the authorization code flow." +- PKCE for a confidential web-server client: not documented as mandatory by Google for this + flow, but recommended by the wider ecosystem ("confidential clients use PKCE in addition to + their client secret"). `code_challenge_methods_supported` is exposed via discovery. + -> Plan SHOULD use PKCE; it is cheap and is the OAuth 2.1 direction. +- `redirect_uri` "must exactly match one of the authorized redirect URIs for the OAuth 2.0 + client, which you configured in your client's Cloud Console Clients page." +- `client_secret` is still expected for a confidential server-side client. +- Refresh tokens: for an app requesting only `openid`/`email`/`profile`, the 7-day Testing-mode + refresh-token expiry does NOT apply. This app needs no refresh token at all for sign-in. +- `hd` is a supported authorization parameter (appears in Google's own example: + `...&authuser=0&hd=example.com&prompt=consent`) -> the mechanism for a Google Workspace + domain restriction, if that is the chosen access model. +- `email` is only present "if you included the email scope"; `email_verified` accompanies it. + +## Library facts (PyPI API, queried 2026-10-05) +| package | version | requires-python | license | latest release | +|---|---|---|---|---| +| authlib | 1.8.0 | >=3.10 | BSD-3-Clause | 2026-08-30 | +| google-auth | 2.59.1 | >=3.10 | Apache 2.0 | 2026-09-30 | +| google-auth-oauthlib | 1.5.0 | >=3.10 | Apache 2.0 | 2026-09-29 | +| oauthlib | 4.0.0 | >=3.9 | BSD-3-Clause | 2026-09-28 | +| pyjwt | 2.15.1 | >=3.9 | - | 2026-09-28 | +| requests-oauthlib | 2.0.0 | >=3.4 | ISC | 2024-03-22 | +| pyramid_oauthlib | 1.0.0 | >=3.7 | - | 2022-09-17 | +| pyramid-google-auth | HTTP 404 | - | - | NOT ON PyPI | +- Every serious candidate requires Python >=3.9/3.10, so all are compatible with this repo's 3.14. +- `pyramid-google-auth` is **no longer on PyPI at all** -> definitively ruled out. +- `pyramid_oauthlib` last released 2022-09-17 (4 years stale) -> treat as unmaintained. +- `requests-oauthlib` last released 2024-03-22 (2+ years) -> aging. +- `authlib`, `google-auth*`, `oauthlib`, `pyjwt` were all released within the last ~6 weeks of + 2026-09-28/30 -> all actively maintained. +- Authlib additionally pulls in `joserfc` for JWT handling. + +## Open assumptions (announced defaults) + + +| assumption | adopted default | rationale | reversible? | +|---|---|---|---| +| Session store for `state` + PKCE verifier + nonce | keep the existing `SignedCookieSessionFactory`; no `pyramid_beaker` | the three values total well under 200 bytes against a ~4KB cookie budget, and the cookie is already the app's flash-message store; adding a server-side session backend is a second dependency in an already-shared venv. Note the cookie is signed but NOT encrypted, so these values are readable by the client - acceptable for a nonce/state/verifier, which are single-use and short-lived | yes | +| Password login | removed entirely, no fallback | the request says "remplacer"; a dual path doubles the auth surface and contradicts the goal | yes | +| `profile` scope | not requested | this app shows no display name or avatar; `openid email` is the documented minimum | yes | +| Refresh token / `access_type=offline` | not requested | sign-in only; no Google API calls are ever made | yes | +| Explicit `logout` revocation | local session logout only | see question; the app holds no Google-side token to revoke | yes | +| Tidy known bug in touched code | leave the `views/default.py:142` `delete(...)` `NameError` alone unless it sits on a line the migration must edit anyway (bugfix rule: no refactor while migrating) | avoids scope creep | yes | + +## Findings (cited - path:lines) +- Auth is `AuthTktAuthenticationPolicy(hashalg='sha512', timeout=36000)` with `groupfinder` + callback, secrets hardcoded in the app factory - `ctp_blogr/__init__.py:13-21`. +- Session is `SignedCookieSessionFactory('hZug2zPt7hT2MZ')` (signed, NOT encrypted, ~4KB) - + `ctp_blogr/__init__.py:13`. +- ACL is 3 lines: `view` -> `Authenticated`, `manage` -> `group:administrators`, `DENY_ALL` - + `ctp_blogr/security.py:9-12`. +- Admin is NOT a DB flag: `groupfinder` hardcodes `userid == 'admin'` - + `ctp_blogr/services/user.py:26-27`. With OAuth the userid becomes a Google subject/email, so + this mapping MUST change or nobody keeps `manage`. +- Passwords are passlib `custom_app_context` hashes; `verify_password` auto-upgrades cleartext - + `ctp_blogr/models/user.py:21-30`. `password` is `nullable=False` (`users.password`) - dropping + the column in SQLite requires a table rebuild, not `ALTER TABLE DROP COLUMN` on old versions. +- `UserCreateForm` has `password` + `confirm` and enforces a 6-char minimum in the view - + `ctp_blogr/forms.py:28-33`, `ctp_blogr/views/default.py:116-118`. +- Non-admin users are pointed at `/user_edit` as "Modifier le mot de passe" - + `ctp_blogr/templates/layout.jinja2:43-49`. That whole self-service flow dies with OAuth. +- `UserService.delete` exists; `views/default.py:142` calls an undefined bare `delete(...)` + (known NameError, already recorded in AGENTS.md). +- Committed dev DB: `ctp_blogr.sqlite`, users `admin` (id 1) and `phuoc` (id 2), 99 entries. + No `alembic.ini` / `migrations/` in repo though `alembic_version` row exists + (rev `b8f8216d72c7`) -> schema changes are hand-written `ALTER TABLE` (AGENTS.md "Database"). +- `pyramid_tm` with `tm.manager_hook = 'pyramid_tm.explicit_manager'` commits at the end of a + successful request - `ctp_blogr/models/__init__.py:61-64`. A user row created inside the OAuth + callback GET is committed by that implicit commit; there is no explicit `commit()` anywhere. +- `pyramid_retry` with `retry.attempts = 3` is included - `ctp_blogr/models/__init__.py:67`, + `development.ini:19`. Retrying the OAuth callback is a hazard to pin down. +- No linter/type-checker/CI/pre-commit exists. Only `python -m pytest` (2 tests, both trivial). +- `coverage` is broken (`.coveragerc` `source = pyramid_blogr`). +- **`request.authenticated_userid` is load-bearing in 11 places across 6 files**, and it is + more than a boolean: + - 4 comparisons against the literal `'admin'` -> `services/user.py:26`, + `views/default.py:93`, `templates/layout.jinja2:43`, `templates/user_edit.jinja2:45`. + With OAuth the userid stops being `'admin'`, so ALL FOUR break at once. + - it is WRITTEN TO THE DATABASE: `views/blog.py:64` (`entry.creator`), + `views/blog.py:75` (`entry.editor`), both `Unicode(50)` columns. 99 existing rows already + hold `'admin'` / `'phuoc'`, so post-migration the byline has mixed provenance. + - it is USED AS A URL SEGMENT: `templates/layout.jinja2:46` + `request.route_url('user_edit', name=request.authenticated_userid)`. An email-based + identity contains `@` and `.` and must survive Pyramid's route_url encoding. + - it is the ANONIMITY GATE: `services/blog_record.py:16,30,39` test + `request.authenticated_userid == None` to decide the published-only filter. If the new + session identity is ever not populated, this fails OPEN (site looks merely "empty"), + and no existing test would catch it. +- **Open-access regression risk (highest severity finding).** Today only the two + pre-provisioned rows can log in. After OAuth, ANY Google account holder can authenticate. + `templates/blog.jinja2:7` renders "[ Modifier ]" for ANY authenticated user, and + `routes.py:13` `user_edit` / `views/default.py:79` `users` are the only guarded pages. + So without an explicit allowlist or Workspace-domain restriction, the migration would + expose post editing to the public internet. This must be an explicit owner-decision. +- `templates/login.jinja2:8-20` posts `username`, `password`, `came_from` with + `name="form.submitted"`; the whole form is replaced by a redirect-to-Google button. +- `templates/user_edit.jinja2:26-38` renders the password/confirm block; the whole + non-admin self-service path ("Modifier le mot de passe") dies with OAuth. +- `templates/users.jinja2:7,23` wires admin user creation through + `request.route_url('user_edit', name='0')` and `name=entry.name`; under OAuth accounts are + provisioned at first login, so "Nouvel utilisateur" has no meaning. +- `templates/users.jinja2:27` calls `entry.last_logged.strftime(...)` with no NULL guard; the + model default is a Python-side `default=`, so any row inserted by raw SQL (i.e. by the + migration) and not through the ORM can render as a 500. +- **No OAuth-capable library is installed in the shared venv** - no `authlib`, + `google-auth`, `oauthlib`, `pyjwt`, `jose`, or `pyramid_beaker`. The plan necessarily adds a + `pip install` into a venv that is shared with a second project (AGENTS.md "Environment"). +- `ctp_blogr/views/__init__.py` and `ctp_blogr/services/__init__.py` are both empty (0 bytes). + +## External findings - Authlib API (from official Authlib docs, read 2026-10-05) +- **OpenID Connect discovery is supported and is the clean registration path.** Authlib can + take `server_metadata_url` and derive `authorize_url`, `access_token_url` and `jwks_uri` + automatically: + ```python + oauth.register('google', client_id=..., client_secret=..., + server_metadata_url='https://accounts.google.com/.well-known/openid-configuration', + client_kwargs={'scope': 'openid email profile'}) + ``` + Without a discovery endpoint the developer MUST supply `authorize_url`, `access_token_url` + and `jwks_uri` by hand. -> use discovery. +- **CRITICAL RISK: Authlib's high-level API is Flask/Django-only; Pyramid is NOT integrated.** + `authorize_redirect(request, redirect_uri=None, **kwargs)` and the whole state mechanism + live in `authlib/integrations/base_client/framework_integration.py` and + `authlib/integrations/django_client/apps.py`. `set_state_data` / + `clear_state_data` / `save_authorize_data` store state under a `_state_{name}_{state}` key + in a `session` object with an `exp` field. There is no Pyramid framework integration, so + **state generation, storage, and validation are the application's own responsibility.** + The framework-agnostic Authlib entry point is the lower-level `requests_client.OAuth2Session`. + => the plan must hand-write the state plumbing that Flask/Django users get for free, and + this is the single strongest argument for the hand-rolled `requests` + `pyjwt` option. +- `authorize_redirect` internally does `create_authorization_url(...)` then + `save_authorize_data(request, redirect_uri=redirect_uri, **rv)`; the returned `rv` is where + `url`, `state` and the PKCE `code_verifier` live. Without a framework integration the app + must persist those values itself (this app's signed-cookie session is the only store). +- PKCE: RFC 7636 `code_challenge` is enabled by registering `code_challenge_method` in + `client_kwargs`. +- Token response: `token = oauth.google.authorize_access_token()`; Authlib parses the + `id_token` and exposes claims as `token['userinfo']` so the userinfo endpoint need not be + called. Docs say framework integrations do this automatically "if configurations are + correct" - the Pyramid path must be verified against the `requests_client` API before the + plan relies on `token['userinfo']`, with a documented fallback to decoding the `id_token`. +- Raw ID-token validation (independent of the above) is + `authlib.oidc.core.CodeIDToken` + `joserfc.jwt.decode(id_token, keys)` where keys are fetched + from `https://www.googleapis.com/oauth2/v3/certs`, then `claims.validate()`. + -> Authlib pulls in `joserfc` for JWT; relevant to the dependency-weight trade-off. +- `authorize_access_token()` is shown taking no argument in the Flask example because + `redirect_uri` was persisted in the state data. In Pyramid the app must pass `redirect_uri` + explicitly and it must byte-match the value sent to the authorize step. +- **Authlib Loginpass (pre-configured social providers incl. Google) is Flask and Django + only** - it is not usable from Pyramid. Rules out the "use the batteries-included Google + connector" option. + +## Decisions (with rationale) +- Plan only; no product-code edits in this session (planner role). +- **Library: Authlib 1.8.0** (owner decision, 2026-10-05). Active (2026-08-30), Python >=3.10, + BSD-3. Use OIDC discovery (`server_metadata_url= + https://accounts.google.com/.well-known/openid-configuration`) so `authorize_url`, + `access_token_url` and `jwks_uri` are not hand-written. `pyramid-google-auth` is not on PyPI + and `pyramid_oauthlib` is 4 years stale, so both are ruled out. +- **Access control: database allowlist** (owner decision). The Google consent-screen test-user + allowlist provably does not apply to `openid email`, so the app must check, on callback, that + the authenticated email matches a pre-provisioned `users` row. This preserves today's + semantics exactly (only pre-provisioned accounts can sign in) and keeps `/users` as the + admin surface for granting and revoking access. +- **Existing accounts: one-shot email binding** (owner decision). `admin` (id 1) and `phuoc` + (id 2) are bound to their real Google addresses by a one-shot `UPDATE` in + `ctp_blogr.sqlite`. No first-login claim window is opened, so there is no period in which the + public can self-register. +- **VERIFIED LOCALLY: Pyramid already ships the auth policy we need, no new dependency.** + `pyramid.authentication.SessionAuthenticationPolicy(prefix='auth.', callback=...)` and + `SessionAuthenticationHelper` exist in Pyramid 2.1 and expose exactly the same three methods + as the current `AuthTktCookieHelper`: `remember`, `forget`, `authenticated_userid`. Therefore + - `AuthTktAuthenticationPolicy(callback=groupfinder)` -> `SessionAuthenticationPolicy(callback=...)`, + - the existing `remember(request, username)` / `forget(request)` calls in + `views/default.py:54,58,74` keep working unchanged, + - `Authenticated` still lands in `effective_principals`, so `security.py`'s ACL is untouched, + - and the hardcoded auth-ticket secret at `__init__.py:15` disappears. + Authlib is the only library added to the venv. +- **What `remember()` stores: the user's email.** Chosen because it is the identity that makes + the other 11 `authenticated_userid` call sites keep working: `layout.jinja2:46` routes by + `name=authenticated_userid`, and `views/blog.py:64,75` already write this value into the + `Unicode(50)` `entries.creator` / `entries.editor` columns (today holding `'admin'` / + `'phuoc'`). Cost, stated plainly: the signed cookie session is signed but NOT encrypted, so + the email is readable by the client. Acceptable for a public blog author's address; a numeric + `users.id` would avoid it but would break `layout.jinja2:46` and the byline columns. +- The 4 literal `'admin'` comparisons must be replaced by data, not by a new literal: + `services/user.py:26` (groupfinder), `views/default.py:93`, + `templates/layout.jinja2:43`, `templates/user_edit.jinja2:45`. +- `nonce` is REQUIRED by Google's OIDC spec -> generate on the authorize step, store in the + session, compare against the `id_token` claim on callback. +- PKCE included (`code_challenge_method=S256` + stored `code_verifier`) even though not + documented as mandatory for a confidential web-server client: cheap, and it is the direction + of OAuth 2.1, which Google itself links from its own guidance. + +## Scope IN + +## Scope OUT (Must NOT have) + +## Open questions +- ~~BLOCKING DATA INPUT~~ **RESOLVED 2026-10-05.** The two Google addresses, supplied by the + owner: `admin` (id 1) -> `phuoc@caotek.fr` (keeps the `manage` role); + `phuoc` (id 2) -> `ctphuoc@gmail.com` (authenticated, no `manage`). These are two DISTINCT + people, so each `users` row maps to exactly one Google identity and no merge is needed. +- `caotek.fr` MX records point at Google (`aspmx.l.google.com` pref 1 plus the canonical + `alt1/2/3` + `aspmx2/3` set) while NS and git are at OVH, so the domain is PROBABLY Google + Workspace. That would have made an `hd=caotek.fr` restriction viable, but it is not proven + (Google MX does not prove the accounts are Workspace accounts). The chosen DB allowlist is + strictly stronger and has no external dependency, so this is recorded as a valid alternative + that was considered and declined - not as a fallback. +- Where the OAuth client_id / client_secret live, given both `development.ini` and + `production.ini` are tracked in git. Not yet asked; will be defaulted in the plan to + PasteDeploy `%(ENV_GOOGLE_CLIENT_ID)s` interpolation with a documented fallback, so no + secret is ever committed. Flagged for veto at the gate. + +## Metis gap analysis - 2026-10-05 (empirical, verified against installed sources) +Metis read the installed Pyramid source and booted the real app. It reported 10 blockers and +12+ majors. Several invalidate parts of MY OWN approach. The following are corrections that +change the plan, not just the todos. + +### Corrections to my own design (must be folded in) +- **C1. The groupfinder must NOT query the database.** `pyramid_tm/__init__.py` sets + `annotate_user = asbool(settings.get('tm.annotate_user', True))` -> defaults **True**, and + lines ~136-139 call `request.authenticated_userid` INSIDE `manager.begin()`. With a DB-backed + groupfinder, EVERY request performs a DB SELECT inside the transaction block, and any DB error + 500s every route including `/login` and the OAuth callback itself. Metis reproduced the + traceback. **Revised design: the allowlist stays in the DB (it is the login gate, checked once + in the callback), but the GROUP mapping must be config-only, not DB.** `groupfinder` reads an + `auth.admin_emails` list from settings. This also makes "admin is data, not a literal" concrete + and keeps revocation immediate. `CallbackAuthenticationPolicy` calls the callback twice per + request (once in `authenticated_userid`, once in `effective_principals`), so a config-only + callback is essentially free. +- **C2. "Unknown email -> 403" is UNACHIEVABLE as I wrote it.** `forbidden_view_config` on + `login` (`views/default.py:38`) catches `HTTPForbidden` and renders `login.jinja2` with status + **200**. Verified on the real app: anonymous `/users` returns 200 plus the login form. So a + rejected OAuth user would be shown the dead password form at 200. **Revised: the callback must + set `request.response.status_int = 403` explicitly and render a French "acces refuse" template + WITHOUT raising `HTTPForbidden`, and the `forbidden_view_config` must be replaced rather than + inherited.** All acceptance criteria asserting 403 must be rewritten to assert on the explicit + status plus content, and the AGENTS.md "200 not 403" note must be updated. +- **C3. The session factory must be reconfigured, and I never mentioned it.** + `SignedCookieSessionFactory('hZug2zPt7hT2MZ')` at `__init__.py:13` has default `timeout=1200` + (20 min) against the current `AuthTktAuthenticationPolicy(timeout=36000)` (10 h). A Google + consent screen plus Google sign-in plus 2FA routinely exceeds 20 min, so `state`/`code_verifier`/ + `nonce` would expire mid-flow. The timeout must be raised explicitly. +- **C4. `request.session.renew()` DOES NOT EXIST in Pyramid 2.1**, and neither do + `SessionRenew` / `SessionInvalidate`. The only primitive is `invalidate()`, which is literally + `self.clear()` carrying the upstream comment `# XXX probably needs to unset cookie`. My brief + asked the wrong question. There is no server-side session id to rotate, so classic session + fixation is structurally impossible; the real requirement is to DELETE the `state`/ + `code_verifier`/`nonce` keys after the callback so the values do not survive client-side. +- **C5. My test strategy does not work as written.** `BaseTest` in `ctp_blogr/tests.py` builds a + `testing.DummyRequest`, not a WSGI app, so WebTest cannot drive it. Worse, + `BaseTest.init_database()` is DEFINED BUT NEVER CALLED by either existing test, so no table is + ever created. "WebTest on :memory: using the existing BaseTest fixture" is impossible. The plan + needs its own test-app harness that builds a real WSGI app with a stubbed OAuth client and an + in-memory engine. +- **C6. `create_authorization_url` returns `(uri, state)` and does NOT return `code_verifier`.** + The app must generate it with `generate_token(48)`, pass it in, and save it itself. My brief + implied Authlib hands these back. + +### Blockers Metis raised that are not yet in the todos +- **B1. `authlib` is not installed and not in `setup.py`**, and it drags in `joserfc`. Installing + into the shared parent venv is exactly what AGENTS.md warns against. Needs an explicit + dependency-manifest + install step with the shared-venv risk called out. +- **B5. The OAuth callback CAN be retried.** `pyramid_tm` tags exceptions as retryable via + `transaction.Transaction.isRetryableError`; SQLite "database is locked" is a transient error, + so `pyramid_retry` re-runs the callback, re-exchanging the same authorization code -> + `invalid_grant` -> the user sees a failed login. Code-exchange idempotency must be addressed, + and the callback should be made non-retryable. +- **B6. `user_edit` has `permission='view'`, NOT `manage`, and has NO ownership check.** The only + `authenticated_userid` comparison in the whole function is `== 'admin'` for the return URL. + Any authenticated allowlisted user can open `/user_edit/` and rewrite that row, or + `/user_edit/0` to create one. Live today with 2 users; becomes email enumeration plus + horizontal privilege once the allowlist grows. +- **B7 + M8. `verify_password` (`models/user.py:29`) is the ONLY writer of `last_logged`.** + Removing it removes the only writer, and `templates/users.jinja2:27` calls + `entry.last_logged.strftime(...)` with no NULL guard -> verified `UndefinedError`. The callback + must set `last_logged` explicitly, or `/users` 500s. +- **B8. `user_edit.jinja2:27-28,36-37` will `UndefinedError`** once the WTForms `password`/ + `confirm` fields are removed, because Jinja2's default `Undefined` raises on attribute access. + Verified. +- **B10. `passlib` is imported at top level in `models/user.py:11`.** Removing the dependency + without removing that import breaks the whole app at import time. +- **M2. There is zero CSRF protection today** (no `csrf` string anywhere in the repo), and the + callback adds none. Pyramid 2.1 DOES ship `session.new_csrf_token()` / `get_csrf_token()` + (`_csrft_`, 40 hex chars) on the cookie session, so no `pyramid_csrf` package is needed - but + nothing wires it automatically; each view must render and compare manually. `state` is + sufficient for the callback specifically (a GET, single-use, bound to the pre-auth session). + `logout` is a GET that mutates session state, so it is login-CSRF-able. +- **M3. The read filter fails CLOSED** (`authenticated_userid == None` -> anonymous -> only + `status == 'publiƩ'`), which is the safe direction. But `by_id` has no filter at all, and + `blog_search`/`by_criteria` leaks draft titles and bodies to any authenticated user via `LIKE`. +- **M4. Cookie size is NOT a blocker** - a correction to my own framing. Measured: identity + + state + code_verifier + nonce serializes to **324 bytes**; with 10 French flash messages, 662. + Pyramid raises a hard `ValueError` at **4064 bytes**, so headroom is 3740. The only growth + vector is unbounded flash accumulation. +- **M5. `set_on_exception=True` + `pyramid_retry`** means the session `Set-Cookie` is emitted even + for error responses, and a retried request can emit several. +- **M6. The cookie is not hardened for an identity-bearing value**: `secure=False`, `httponly=False`, + and the secret is 16 characters while `hashalg='sha512'` wants at least 64. + `production.ini:29` declares `url_scheme = https` yet the cookie still carries no `Secure` flag, + and `httponly=False` plus third-party jQuery/Bootstrap CDNs in `layout.jinja2:14-15,106-107` + means any XSS reads the identity straight out of the cookie. +- **M7. `entries.creator` / `entries.editor` are `Unicode(50)`** and are written directly from + `request.authenticated_userid` at `views/blog.py:64,75` with no length guard. +- **M9. Admin-as-data had no defined read path.** Templates cannot call the groupfinder; the + fix is `'group:administrators' in request.effective_principals` at all four sites. Where the + flag lives was undecided; C1 settles it as settings, not a new column. +- **M12. The test seam must be named**, and tests must disable `pyramid_retry` or single-shot side + effects such as state-clearing become non-deterministic. + +### Correction to a claim I made in the brief +- Authlib 1.8 does NOT ship integrations for Flask and Django only - there is also + `authlib.integrations.starlette_client`. The conclusion that there is **no Pyramid integration** + still holds, which is the part that matters. + +### Second pass - M13, the minors that actually bite, and the 22 acceptance criteria +- **M13. The `NameError` at `views/default.py:142` is FREE to fix**, because `user_edit` must be + edited anyway: line 93 (`'admin'` literal), 116-117 (`len(form.password.data) < 6`), 127 and + 137 (`set_password`), 98-102 and 119-131 (create-user branch). `UserService.delete` already + exists at `services/user.py:18`. **But the trap:** `user_edit.jinja2:45` gates the delete + button on `request.authenticated_userid == 'admin'`; if that literal is swapped for the + `effective_principals` check from M9, **the button still renders for admins and the + `NameError` survives the migration**. Fix line 142 in the same change or ship a 500. + **Second-order:** `UserService.delete` is + `query.filter(User.id == id).delete(synchronize_session=False)` with **no FK cascade** from + `entries.creator`/`entries.editor`, so deleting a user orphans those strings. With emails as + the identity, that is now **PII stranded in an orphaned column** and it collides directly with + Decision 3's accepted cost. +- **`hd` is a TRAP - do NOT set `hd=caotek.fr`.** It is a *hosted-domain* hint meaning "a + Workspace account on this domain", and `ctphuoc@gmail.com` is a **personal Gmail account**. + Setting it locks out the second user and contradicts Decision 5. The Workspace-admin-blocks-the- + app-app-wide observation is correct, and it is exactly why `hd` is not a control here. Omit it. +- **The call-site count in my brief was wrong: it is 15, not 11.** 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). Plan for 15. +- **JWKS keys must be fetched AND CACHED by the app.** With the bare Authlib client there is no + automatic fetch: `CodeIDToken` + `jwt.decode(id_token, keys)` needs the keys pulled from + `jwks_uri` ourselves. An unbounded refetch per login is a latency and rate-limit problem, so the + cache and its TTL have to be decided, not left implicit. +- **`request.session.get_csrf_token()` is a lame token**: one long-lived random value per session + under key `_csrft_`, not per-request, and there is **no automatic comparison helper** - every + view must compare by hand. It also writes to the session on first read. +- **Cookie secret is 16 characters against `hashalg='sha512'`**, which wants at least 64. Since + the cookie now *is* the identity, that secret is load-bearing and must be lengthened. +- **Branch and rollback are UNSETTLED and are a prerequisite, not a detail.** There is one commit + on `master` with `main` and `origin/*` at the same commit, and this migration performs a + **destructive table rebuild on a committed database file**. An explicit branch decision plus a + documented restore path for `ctp_blogr.sqlite` is required *before* touching it. Do not commit + unless asked. +- Passlib hashes stay in git history forever even after the destructive rebuild discards them + from the working tree - belongs in the accepted-costs note. +- `.coveragerc` still says `source = pyramid_blogr`, so coverage collects nothing. **Coverage must + not be used as an acceptance gate** for this migration; the number would be fiction. +- Harmless: `logout` has no permission and `forget()` on an empty session is a no-op. + +### The three items Metis left undecided - now settled +1. **"Admin is data"** -> resolved by C1: `groupfinder` reads `auth.admin_emails` from **settings**, + never the DB, because B3 forbids a DB-backed callback inside the `pyramid_tm` tween. The + database keeps only the login allowlist. +2. **"The allowlist is the primary control"** -> yes, confirmed: the DB allowlist gates sign-in + once, in the callback. It is not a per-request check, so it cannot be a DB callback. +3. **"403 on unknown email"** -> achievable only as an explicit + `request.response.status_int = 403` plus a French rejection template, **without raising + `HTTPForbidden`** (which the `forbidden_view_config` on login would swallow into a 200). + +Metis supplied **22 numbered acceptance criteria**, grouped by boot, session policy, callback, +access control, templates/removals, schema and regression. They are transcribed into the plan's +Verification section rather than restated here, together with its 6-phase sequencing. + +## Approval gate +status: approved +approved-on: 2026-10-05 +approval-evidence: >- + The owner answered the single blocking data input (the two Google email addresses) after the + brief was presented. Per the gate mechanics, "answering the open ambiguities" constitutes + approval to write the plan. Authorization covers writing `.omo/plans/google-oauth-migration.md` + ONLY - it is not authorization to implement. +approach: >- + Swap AuthTktAuthenticationPolicy for Pyramid's built-in SessionAuthenticationPolicy (same + three-method helper interface, so no new auth dependency and the hardcoded auth-ticket + secret disappears). Add Authlib 1.8.0 as the only new library, registered for Google via + OIDC discovery, with OIDC state + PKCE (S256) + nonce generation, storage and validation + written by the app because Authlib has no Pyramid integration. Login becomes a redirect to + Google; a new callback route exchanges the code, validates the id_token, checks the email + against a database allowlist, provisions nothing implicitly, and remembers the identity in + the session. Replace the 4 literal 'admin' comparisons with a data-driven role. Remove the + password column, the passlib dependency, the password/confirm form fields, the self-service + password-change page, and the admin "create user" flow, since accounts become + pre-provisioned by email. Hand-write the ALTER TABLEs against ctp_blogr.sqlite and warn that + no migrations exist. Add WebTest coverage that never touches the network. +pending-action: write .omo/plans/google-oauth-migration.md +test-strategy: >- + tests-after, agent-executed. WebTest drives the routes against a `sqlite:///:memory:` engine + built by the existing `BaseTest` fixture. The Google token exchange and JWKS fetch are + injected behind a small seam so no test performs network I/O. Per-todo QA covers both the + happy path and the failure paths that matter (unknown email -> 403, missing/mismatched + state, missing/mismatched nonce, `error=access_denied`). Announced as a default, not asked. diff --git a/.omo/plans/google-oauth-migration.md b/.omo/plans/google-oauth-migration.md new file mode 100644 index 0000000..2b3f2b1 --- /dev/null +++ b/.omo/plans/google-oauth-migration.md @@ -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 + `/task--google-oauth-migration.`. +- Structural self-check before handoff: every implementation row is column-zero + `- [ ] N. ` 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). \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef31b5205ffeymj770ZKgYuoTc.json b/.omo/run-continuation/ses_ef31b5205ffeymj770ZKgYuoTc.json new file mode 100644 index 0000000..9f69924 --- /dev/null +++ b/.omo/run-continuation/ses_ef31b5205ffeymj770ZKgYuoTc.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef31b5205ffeymj770ZKgYuoTc", + "updatedAt": "2026-10-05T16:31:35.122Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:31:35.122Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef3219eb7ffePWI4IWUtfFaPlU.json b/.omo/run-continuation/ses_ef3219eb7ffePWI4IWUtfFaPlU.json new file mode 100644 index 0000000..81608ca --- /dev/null +++ b/.omo/run-continuation/ses_ef3219eb7ffePWI4IWUtfFaPlU.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef3219eb7ffePWI4IWUtfFaPlU", + "updatedAt": "2026-10-05T16:21:03.078Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:21:03.078Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef321a66affe36l5vcF1NcjeSd.json b/.omo/run-continuation/ses_ef321a66affe36l5vcF1NcjeSd.json new file mode 100644 index 0000000..2d35c81 --- /dev/null +++ b/.omo/run-continuation/ses_ef321a66affe36l5vcF1NcjeSd.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef321a66affe36l5vcF1NcjeSd", + "updatedAt": "2026-10-05T16:21:02.917Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:21:02.917Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef32981bfffeFV05lbZn2tbpAi.json b/.omo/run-continuation/ses_ef32981bfffeFV05lbZn2tbpAi.json new file mode 100644 index 0000000..7cd6b5a --- /dev/null +++ b/.omo/run-continuation/ses_ef32981bfffeFV05lbZn2tbpAi.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef32981bfffeFV05lbZn2tbpAi", + "updatedAt": "2026-10-05T16:12:26.203Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:12:26.203Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef329bb8bffegs5WbAj1bICg36.json b/.omo/run-continuation/ses_ef329bb8bffegs5WbAj1bICg36.json new file mode 100644 index 0000000..904a58e --- /dev/null +++ b/.omo/run-continuation/ses_ef329bb8bffegs5WbAj1bICg36.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef329bb8bffegs5WbAj1bICg36", + "updatedAt": "2026-10-05T16:12:11.540Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:12:11.540Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef329bbbbffe2WXsJlnJaJuNcT.json b/.omo/run-continuation/ses_ef329bbbbffe2WXsJlnJaJuNcT.json new file mode 100644 index 0000000..fb524f8 --- /dev/null +++ b/.omo/run-continuation/ses_ef329bbbbffe2WXsJlnJaJuNcT.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef329bbbbffe2WXsJlnJaJuNcT", + "updatedAt": "2026-10-05T16:12:11.409Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:12:11.409Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef32a3e79ffe3QmNXbLnmZFEnx.json b/.omo/run-continuation/ses_ef32a3e79ffe3QmNXbLnmZFEnx.json new file mode 100644 index 0000000..cb3acda --- /dev/null +++ b/.omo/run-continuation/ses_ef32a3e79ffe3QmNXbLnmZFEnx.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef32a3e79ffe3QmNXbLnmZFEnx", + "updatedAt": "2026-10-05T16:20:34.354Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:20:34.354Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef34f552dffeM282xuKssUUXKU.json b/.omo/run-continuation/ses_ef34f552dffeM282xuKssUUXKU.json new file mode 100644 index 0000000..2983383 --- /dev/null +++ b/.omo/run-continuation/ses_ef34f552dffeM282xuKssUUXKU.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef34f552dffeM282xuKssUUXKU", + "updatedAt": "2026-10-05T15:50:55.988Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T15:50:55.988Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef35b96afffeXrpuxFC0wrkThx.json b/.omo/run-continuation/ses_ef35b96afffeXrpuxFC0wrkThx.json new file mode 100644 index 0000000..db95ba8 --- /dev/null +++ b/.omo/run-continuation/ses_ef35b96afffeXrpuxFC0wrkThx.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef35b96afffeXrpuxFC0wrkThx", + "updatedAt": "2026-10-05T15:17:44.049Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T15:17:44.049Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef35b9e91ffeI6ETz99g3s2EtG.json b/.omo/run-continuation/ses_ef35b9e91ffeI6ETz99g3s2EtG.json new file mode 100644 index 0000000..f32001a --- /dev/null +++ b/.omo/run-continuation/ses_ef35b9e91ffeI6ETz99g3s2EtG.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef35b9e91ffeI6ETz99g3s2EtG", + "updatedAt": "2026-10-05T15:17:43.882Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T15:17:43.882Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef35bb608ffeKyWw3wwjhCC0Sh.json b/.omo/run-continuation/ses_ef35bb608ffeKyWw3wwjhCC0Sh.json new file mode 100644 index 0000000..dd7150b --- /dev/null +++ b/.omo/run-continuation/ses_ef35bb608ffeKyWw3wwjhCC0Sh.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef35bb608ffeKyWw3wwjhCC0Sh", + "updatedAt": "2026-10-05T15:18:49.679Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T15:18:49.679Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef35bd8c1ffe6WiC2nJ3gkUtPr.json b/.omo/run-continuation/ses_ef35bd8c1ffe6WiC2nJ3gkUtPr.json new file mode 100644 index 0000000..00ac51e --- /dev/null +++ b/.omo/run-continuation/ses_ef35bd8c1ffe6WiC2nJ3gkUtPr.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef35bd8c1ffe6WiC2nJ3gkUtPr", + "updatedAt": "2026-10-05T15:18:41.859Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T15:18:41.859Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef37a9a1affeH7gOLSk7lZSu03.json b/.omo/run-continuation/ses_ef37a9a1affeH7gOLSk7lZSu03.json new file mode 100644 index 0000000..f81d988 --- /dev/null +++ b/.omo/run-continuation/ses_ef37a9a1affeH7gOLSk7lZSu03.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef37a9a1affeH7gOLSk7lZSu03", + "updatedAt": "2026-10-05T16:33:15.784Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T16:33:15.784Z" + } + } +} \ No newline at end of file diff --git a/.omo/run-continuation/ses_ef39729efffeOKFGZ0HHraXRSX.json b/.omo/run-continuation/ses_ef39729efffeOKFGZ0HHraXRSX.json new file mode 100644 index 0000000..4abe6a3 --- /dev/null +++ b/.omo/run-continuation/ses_ef39729efffeOKFGZ0HHraXRSX.json @@ -0,0 +1,10 @@ +{ + "sessionID": "ses_ef39729efffeOKFGZ0HHraXRSX", + "updatedAt": "2026-10-05T14:26:21.040Z", + "sources": { + "background-task": { + "state": "idle", + "updatedAt": "2026-10-05T14:26:21.040Z" + } + } +} \ No newline at end of file