call google oauth success
This commit is contained in:
@@ -1,8 +1,11 @@
|
||||
# AGENTS.md
|
||||
|
||||
Single-package Pyramid 2.0 blog app (`ctp_blogr`). Python 3.14, SQLite, Jinja2, WTForms,
|
||||
SQLAlchemy 2.1, `pyramid_tm` transactions. UI strings, comments, and flash messages are
|
||||
**French**; match that when editing views/templates.
|
||||
SQLAlchemy 2.1, `pyramid_tm` transactions. Sign-in is Google OAuth only: Authlib
|
||||
protocol/crypto primitives plus `joserfc` and `urllib.request` for HTTP. There are no
|
||||
passwords anywhere — no password column, no password form fields, no local credential
|
||||
path. UI strings, comments, and flash messages are **French**; match that when editing
|
||||
views/templates.
|
||||
|
||||
## Environment
|
||||
|
||||
@@ -18,14 +21,19 @@ Don't invent one; don't claim you ran one.
|
||||
## Commands
|
||||
|
||||
```bash
|
||||
python -m pytest # 2 tests, all in ctp_blogr/tests.py
|
||||
ENV_GOOGLE_CLIENT_ID=... ENV_GOOGLE_CLIENT_SECRET=... pserve development.ini # waitress on localhost:9280 + debugtoolbar
|
||||
ENV_GOOGLE_CLIENT_ID=... ENV_GOOGLE_CLIENT_SECRET=... pserve production.ini # binds *:9280, url_scheme https
|
||||
ENV_GOOGLE_CLIENT_ID=... ENV_GOOGLE_CLIENT_SECRET=... pshell development.ini # preloaded: request, tm, dbsession, models
|
||||
python -m pytest # 34 tests, all offline (2 in tests.py + 32 in test_oauth.py)
|
||||
python -m pytest ctp_blogr/tests.py::TestMyViewSuccessCondition # single test/class
|
||||
pserve development.ini # waitress on localhost:9280 + debugtoolbar
|
||||
pserve production.ini # binds *:9280, url_scheme https
|
||||
pshell development.ini # preloaded: request, tm, dbsession, models
|
||||
python -c "from pyramid.paster import get_app; get_app('development.ini')" # boot check
|
||||
ENV_GOOGLE_CLIENT_ID=... ENV_GOOGLE_CLIENT_SECRET=... python -c "from pyramid.paster import get_app; get_app('development.ini')" # boot check
|
||||
```
|
||||
|
||||
- `pserve`, `pshell`, and the boot check all refuse to start without both env vars:
|
||||
`__init__.py` raises `ConfigurationError` with a French message naming the missing ones.
|
||||
PasteDeploy `%(ENV_...)s` interpolation does **not** work (`plaster_pastedeploy` 1.0.1
|
||||
ships no `os.environ` support), so the app reads `os.environ` itself; an `.ini` value,
|
||||
if set, wins over the environment.
|
||||
- `pytest.ini` sets `testpaths = ctp_blogr` and `python_files = test*.py`, so a new test
|
||||
file must be named `test*.py` inside the package to be collected.
|
||||
- **`coverage` is broken**: `.coveragerc` still says `source = pyramid_blogr` (old package
|
||||
@@ -37,14 +45,67 @@ python -c "from pyramid.paster import get_app; get_app('development.ini')" # b
|
||||
|
||||
## Architecture / wiring
|
||||
|
||||
- `ctp_blogr/__init__.py::main` is the PasteDeploy app factory. It hardcodes the session
|
||||
secret and auth-ticket secret inline — rotating them logs everyone out.
|
||||
- Auth: `AuthTktAuthenticationPolicy` + `ACLAuthorizationPolicy` with
|
||||
- `ctp_blogr/__init__.py::main` is the PasteDeploy app factory. Auth is Pyramid's
|
||||
`SessionAuthenticationPolicy` + `ACLAuthorizationPolicy` with
|
||||
`root_factory='ctp_blogr.security.RootFactory'`. The whole ACL is 3 lines in `security.py`:
|
||||
`view` → `Authenticated`, `manage` → `group:administrators`, then `DENY_ALL`.
|
||||
- **Admin is not a DB flag.** `groupfinder` (`services/user.py`) hardcodes
|
||||
`userid == 'admin'` → `['group:administrators']`. Any other user gets `[]`, i.e. logged in
|
||||
but no manage rights. Only `/users` requires `manage`.
|
||||
- The session factory is explicit: 85-char secret, `timeout=36000` (10 h; the
|
||||
`SignedCookieSessionFactory` default is 1200 s / 20 min), `httponly=True` always,
|
||||
`secure=True` only when `session.cookie_secure = true` (set in production.ini).
|
||||
- Identity is the **email**, stored as `request.authenticated_userid` (set via `remember()`
|
||||
in the OAuth callback).
|
||||
- **Admin is not a DB flag.** `services/user.py::make_groupfinder(admin_emails)` returns the
|
||||
auth callback built from the `auth.admin_emails` setting in the `.ini` files. It must
|
||||
**not** query the DB: `pyramid_tm` resolves `request.authenticated_userid` inside
|
||||
`manager.begin()`, so a DB query there would run per request inside the transaction and
|
||||
any DB error would 500 every route including `/login`. Falsy userid → `None`, admin
|
||||
email → `['group:administrators']`, anything else → `[]` (logged in, no manage rights).
|
||||
Callers test `'group:administrators' in request.effective_principals` (views and
|
||||
templates). `/users` and `/user_edit` require `manage`; `blog_edit`, `tag_edit`, and
|
||||
`/tags` require only `view`.
|
||||
- `services/google_oauth.py::GoogleOAuth` is registered as an **attribute**,
|
||||
`config.registry.google_oauth` (read as `request.registry.google_oauth`). Pyramid's
|
||||
`Registry` subclasses `dict` but has no `__getattr__`, so a `registry['google_oauth']`
|
||||
dict-key assignment would be invisible to the views. It resolves
|
||||
`authorization_endpoint` / `token_endpoint` / `jwks_uri` / `issuer` from the Google
|
||||
discovery document, cached, plus a cached JWKS (`CACHE_TTL = 3600`, counted by
|
||||
`jwks_fetches`). Authlib has no Pyramid integration, so the app owns `state` / PKCE /
|
||||
`nonce` itself; `authorize_redirect` / `save_authorize_data` are not used. Authlib is
|
||||
used only for protocol primitives (`generate_token`, `create_s256_code_challenge`) and
|
||||
the 3 HTTP calls go through `urllib.request` — do **not** add `requests`
|
||||
(`authlib.integrations.requests_client` hard-imports it). `id_token` is verified with
|
||||
`joserfc`, not `authlib.jose`. Scope is `openid email` with no `hd` parameter (the
|
||||
second account is a personal Gmail address, so an `hd` lock would reject it).
|
||||
- Routes added: `login_google` = `/login/google`,
|
||||
`login_google_callback` = `/login/google/callback`.
|
||||
- The callback pops `state` / `nonce` / `code_verifier` from the session **before** the
|
||||
token exchange, and sets `request.environ['retry.attempts'] = attempt + 1` to declare
|
||||
itself the last attempt. Both are required: a `pyramid_retry` replay restarts from the
|
||||
original request cookie (which still carries `oauth_state`), which would re-send an
|
||||
already-consumed authorization code and fail `invalid_grant`.
|
||||
- Sign-in is gated on the `users` table (allowlist). No row is ever auto-created. Unknown
|
||||
email sets `request.response.status_int = 403` explicitly and renders
|
||||
`templates/denied.jinja2`, without raising `HTTPForbidden`. New template
|
||||
`denied.jinja2` renders the refusal in French.
|
||||
- The callback validates three ID-token claims before touching the allowlist, and each
|
||||
one is pinned by a test that fails if the guard is removed:
|
||||
- a missing `code` param is refused **before** any network call (`fetch_calls == 0`);
|
||||
- `email_verified` must be exactly `True`. The identity *is* the email, so accepting
|
||||
an unverified address would let an unverified Google account impersonate an
|
||||
allowlisted one. Google returns this claim; don't "simplify" the check away;
|
||||
- `sub` must be non-empty. Without it the matched row would get `google_sub = ''`,
|
||||
which both collides with the `UNIQUE` constraint and leaves the record rebindable
|
||||
by any other account presenting the same address.
|
||||
- `views/default.py::denied` is registered with `@view_config(context=HTTPForbidden, ...)`
|
||||
and sets a real 403. Unauthorized requests now return **HTTP 403 with the denied
|
||||
page** — assert 403 in tests, and don't assume 200 means "the page worked" when
|
||||
smoke-testing protected routes (`/users`, `/tags`, `/blog_edit/{id}`, `/tag_edit/{id}`,
|
||||
`/user_edit/{name}`).
|
||||
- `user_edit` is `permission='manage'` (any logged-in user could previously open or create
|
||||
fiches). The create-user branch (`name='0'`) is gone; unknown names flash
|
||||
"Utilisateur non trouvé" and redirect. `UserEditForm` (renamed from `UserCreateForm`)
|
||||
has only `id` + `name`, and the view deletes `form.name` (not `form.id`) before
|
||||
`populate_obj`, since the email is the identity.
|
||||
- Views bind to routes via `@view_config(route_name=...)`; routes are declared centrally in
|
||||
`routes.py`. Adding a URL means editing both.
|
||||
- `models/__init__.py` includes `pyramid_tm` + `pyramid_retry` and reifies `request.dbsession`.
|
||||
@@ -52,8 +113,8 @@ python -c "from pyramid.paster import get_app; get_app('development.ini')" # b
|
||||
breaks commit/abort behaviour.
|
||||
- Write path: view → `request.dbsession.add(...)` → implicit commit by `pyramid_tm`. Do **not**
|
||||
call `commit()` yourself.
|
||||
- Query layer lives in `services/` (`BlogRecordService`, `UserService`) — all classmethods
|
||||
taking `request`. Keep DB access out of views.
|
||||
- Query layer lives in `services/` (`BlogRecordService`, `UserService`, plus the Google
|
||||
collaborator) — all classmethods taking `request`. Keep DB access out of views.
|
||||
- Read filtering is duplicated per-method: anonymous users only ever see `status == 'publié'`
|
||||
(`by_criteria`, `by_tag`, `get_last_created`). `by_id` does **not** filter — any post is
|
||||
readable by direct URL. Preserve that asymmetry or you change access behaviour.
|
||||
@@ -63,22 +124,31 @@ python -c "from pyramid.paster import get_app; get_app('development.ini')" # b
|
||||
(relative to the package), but `default.py::user_edit` uses the asset spec
|
||||
`'ctp_blogr:templates/user_edit.jinja2'`. Both work; don't "normalize" one without
|
||||
checking the jinja2 search path.
|
||||
- `login.jinja2` has a single "Se connecter avec Google" control and no password markup;
|
||||
`layout.jinja2` shows the Utilisateurs nav item only for admins; the password-change nav
|
||||
item and the "Nouvel utilisateur" button are gone. `users.jinja2` is NULL-safe for
|
||||
`last_logged` (renders "jamais"); `user_edit.jinja2` shows the email readonly.
|
||||
- `forms.py` contains dead forms from another app (`HistoForm`, `AllocationForm`, `ActifForm`,
|
||||
`Actif2Form`) matching orphaned DB tables `histo`, `allocation`, `actifs`. Ignore them.
|
||||
|
||||
## Database
|
||||
|
||||
- **`ctp_blogr.sqlite` is committed and is the real dev database** (~99 posts, 8 tags, users
|
||||
`admin` and `phuoc`). Both `development.ini` and `production.ini` point at it via
|
||||
- **`ctp_blogr.sqlite` is committed and is the real dev database** (~99 posts, 8 tags; users
|
||||
are now `phuoc@caotek.fr` (admin, via the `.ini` setting) and `ctphuoc@gmail.com`
|
||||
(authenticated, no manage)). Both `development.ini` and `production.ini` point at it via
|
||||
`%(here)s`. `.gitignore` lists `*.sqlite`, but the file is already tracked, so the ignore
|
||||
rule does nothing — every write view's `pyramid_tm` commit is a real diff to review.
|
||||
- `users`: `password` dropped, `google_sub` added (nullable, UNIQUE), `name` still NOT NULL
|
||||
+ UNIQUE and now stores the email. `entries.creator` / `entries.editor` are `Unicode(255)`.
|
||||
- The DB contains an `alembic_version` table (rev `b8f8216d72c7`) but there is **no
|
||||
`alembic.ini` and no `migrations/` directory in this repo**. Alembic is installed in the
|
||||
venv, yet nothing here is wired to it. So the schema cannot be regenerated.
|
||||
venv, yet nothing here is wired to it. The OAuth schema changes were hand-applied SQL,
|
||||
so the schema cannot be regenerated.
|
||||
- **The DB file is the source of truth for schema.** Changing a model means: apply the
|
||||
equivalent `ALTER TABLE` to `ctp_blogr.sqlite` by hand, and tell the user migrations are
|
||||
missing rather than pretending `Base.metadata.create_all()` covers them. `create_all()` is
|
||||
only used by the in-memory test fixture (`tests.py::BaseTest.init_database`).
|
||||
only used by the in-memory test fixtures (`tests.py::BaseTest.init_database`,
|
||||
`test_oauth.py::demarrer`).
|
||||
- `Tags` is defined in `models/blog_record.py` but is **not** imported in `models/__init__.py`
|
||||
(only `User` and `BlogRecord` are, before `configure_mappers()`). It still works only because
|
||||
importing `blog_record` defines the class as a side effect. Import it explicitly if you touch
|
||||
@@ -86,9 +156,6 @@ python -c "from pyramid.paster import get_app; get_app('development.ini')" # b
|
||||
|
||||
## Known-broken (verified, do not re-diagnose)
|
||||
|
||||
- `views/default.py:142` calls bare `delete(request, user.id)` — `delete` is neither imported
|
||||
nor defined. Submitting the delete button on `/user_edit` raises `NameError`. The intended
|
||||
call is `UserService.delete(request, user.id)`.
|
||||
- `views/default.py:18` — `home` hardcodes `BlogRecordService.by_id(request, 95)` and then reads
|
||||
`entry.title` / `entry.body` with no `None` check. The home page raises `AttributeError` on any
|
||||
database where id 95 doesn't exist (e.g. a freshly created one). `home.jinja2:18` hardcodes the
|
||||
@@ -98,20 +165,35 @@ python -c "from pyramid.paster import get_app; get_app('development.ini')" # b
|
||||
- `routes.py:5` — `'/blog/{id:\d+}/{slug}'` is a non-raw string, so Python 3.12+ emits
|
||||
`SyntaxWarning: "\d" is an invalid escape sequence`. Prefix with `r` if you edit it.
|
||||
- `models/meta.py` uses `sqlalchemy.ext.declarative.declarative_base` (`MovedIn20Warning`).
|
||||
- **Unauthorized requests return HTTP 200, not 403** (verified with and without
|
||||
`pyramid_debugtoolbar`). `forbidden_view_config` renders `login.jinja2` and the status stays
|
||||
200. Don't write tests asserting 403, and don't assume 200 means "the page worked" — check
|
||||
for the login form when smoke-testing protected routes (`/users`, `/tags`, `/blog_edit/{id}`,
|
||||
`/tag_edit/{id}`, `/user_edit/{name}`).
|
||||
- Draft leak in `by_id` (no status filter) and the `LIKE` search leak in `by_criteria`
|
||||
(anonymous search matches drafts/privates, only the display is filtered elsewhere) are
|
||||
pre-existing and deliberately not fixed by the OAuth work.
|
||||
- Any logged-in user (including the non-admin account) can edit and delete ANY post,
|
||||
because `security.py` grants `view` to every authenticated user and `blog_edit` /
|
||||
`tag_edit` only require `view`. Pre-existing, deliberately unchanged (README documents an
|
||||
"admin ou redacteur" role). Pinned by
|
||||
`test_DEFAUT_connu_un_utilisateur_non_administrateur_peut_modifier_un_billet`.
|
||||
- `UserService.delete` has no FK cascade: deleting a user leaves their email stranded in
|
||||
`entries.creator` / `entries.editor`.
|
||||
- Pre-existing DB defect, not caused by the OAuth work: `PRAGMA foreign_key_check` aborts
|
||||
with "foreign key mismatch" because `fk_allocation_classe_actifs` references
|
||||
`actifs.classe`, which is neither UNIQUE nor a PK.
|
||||
- No CSRF protection on the pre-existing POST views, and `logout` is a state-mutating GET.
|
||||
Accepted risk, not fixed. Pyramid 2.1's cookie session exposes `new_csrf_token()` /
|
||||
`get_csrf_token()` if someone wants to add it.
|
||||
- Identity lives entirely in the signed client-side cookie (no server-side session
|
||||
backend). Payload is ~250 bytes against Pyramid's hard 4064-byte limit, so the only
|
||||
growth vector is unbounded flash accumulation.
|
||||
|
||||
## Testing
|
||||
|
||||
`ctp_blogr/tests.py` has only two tests and they are duplicates that both assert the same
|
||||
success case (`test_failing_view` is a copy-paste of `test_passing_view`). There is no coverage
|
||||
of auth, forms, or the DB. `BaseTest` sets up `sqlite:///:memory:` with
|
||||
`Base.metadata.create_all()` and wraps the session in `transaction.manager`, aborting in
|
||||
teardown. `WebTest` is installed and is the right tool for new view tests — see
|
||||
`pyramid.testing.DummyRequest` usage there for the lighter alternative.
|
||||
`ctp_blogr/tests.py` still has only its two original duplicate tests (both assert the same
|
||||
success case; `test_failing_view` is a copy-paste of `test_passing_view`). New
|
||||
`ctp_blogr/test_oauth.py` adds 32 tests on a real WSGI app over `sqlite:///:memory:` with
|
||||
the Google collaborator faked and sockets blocked by an autouse fixture — 34 passing in
|
||||
total, all fully offline. Tests named `DEFAUT_*` pin known defects on purpose (observed
|
||||
behaviour, not desired). `BaseTest` wraps its session in `transaction.manager`, aborting in
|
||||
teardown. `WebTest` is installed; `test_oauth.py` shows the pattern for new view tests.
|
||||
|
||||
## Conventions
|
||||
|
||||
@@ -122,10 +204,14 @@ teardown. `WebTest` is installed and is the right tool for new view tests — se
|
||||
- Form submission is detected by string key presence: `if 'form.submitted' in request.params`,
|
||||
plus `'form.update_created'` and `'form.deleted'` for the extra buttons. Buttons must set
|
||||
`name="form.submitted"` etc.
|
||||
- `del form.id` before `form.populate_obj(entry)` is a deliberate guard against overwriting the
|
||||
primary key. Keep it.
|
||||
- `del form.id` (blog/tag forms) / `del form.name` (`user_edit`) before `populate_obj` is a
|
||||
deliberate guard against overwriting the primary key / identity. Keep it.
|
||||
- `/blog_bytag/{tag}/{retour}` — `retour` is a **route name** (`request.route_url(retour)`), not a
|
||||
URL. Callers pass e.g. `retour='home'`.
|
||||
- One commit, currently on `master`. `main` and `origin/*` point at the same commit, so branch
|
||||
choice is not settled — ask before creating branches or pushing to the self-hosted
|
||||
`https://git.caotek.fr/phuoc/ctp_blogr.git`. Commit only when asked.
|
||||
- Currently on branch `feat/google-oauth`, uncommitted, nothing pushed (`master` tracks
|
||||
`origin/master` at the same commit, so branch choice is still not settled). A verified
|
||||
pre-migration DB backup exists outside the tree at
|
||||
`/var/folders/cm/x5cx1bfd749gxd1x_zt6bvp40000gn/T/opencode/ctp_blogr-backup-20261005-202240.sqlite`
|
||||
(`PRAGMA integrity_check` returns `ok`); restore with `cp` over `ctp_blogr.sqlite`.
|
||||
Ask before creating branches, committing, or pushing to the self-hosted
|
||||
`https://git.caotek.fr/phuoc/ctp_blogr.git`. Commit only when asked.
|
||||
|
||||
Reference in New Issue
Block a user