Skip to content

Commit 55641bd

Browse files
skypank-coderskypank-coder
authored
RFC: User Authentication and Online MyOpenCRE Mapping (#876)
* RFC: User Authentication and Online MyOpenCRE Mapping * docs(rfc): address CodeRabbit review — reconcile session contract, clarify google_sub, define mapping accuracy metric * docs: address review on user-auth RFC; move to docs/rfc - Reconcile session contract with existing login_required (google_id/name -> user_id) - Clarify google_sub == existing session google_id (OIDC sub claim) - Privacy note: clarify nothing is persisted to DB (session data already exists) - Define TODO 3 accuracy checkpoint as exact-match vs human-verified CRE links - Remove Author/Status/Related-issues metadata lines - Move RFC under docs/rfc/
1 parent 0e16c2e commit 55641bd

1 file changed

Lines changed: 302 additions & 0 deletions

File tree

‎docs/rfc/user-auth-myopencre.md‎

Lines changed: 302 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,302 @@
1+
# RFC: User Authentication and Personal MyOpenCRE Views
2+
3+
---
4+
5+
## Context
6+
7+
MyOpenCRE currently runs locally only. Users who want automated CRE mappings must run their own OpenCRE instance, which requires Docker, a local database, and significant technical setup. This RFC proposes making MyOpenCRE available online with user login, so any authenticated user can upload a standard and receive automated CRE mappings stored against their account.
8+
9+
**What MyOpenCRE currently does (existing endpoints):**
10+
- `GET /rest/v1/cre_csv` — downloads a CSV template of all existing CREs
11+
- `POST /rest/v1/cre_csv_import` — accepts a filled CSV and imports mappings into the local database
12+
13+
Both endpoints require no authentication and operate only on a locally-running instance. There is no way for a user to upload a standard and receive automated mappings without running OpenCRE themselves. This RFC adds that capability via authenticated endpoints under `/rest/v2/`.
14+
15+
The current codebase already has Google OAuth plumbing (`GOOGLE_CLIENT_ID`, `GOOGLE_CLIENT_SECRET`, `login_required` decorator, `LOGIN_ALLOWED_DOMAINS`). What does not exist is any persistent `User` model or user-scoped data. This RFC adds that persistence and wires it to a new MyOpenCRE mapping flow.
16+
17+
**Core constraints:**
18+
- No data duplication: user sections point to shared CRE nodes by ID; nothing in the CRE graph is copied
19+
- No per-user embedding storage: section text is encoded ephemerally at upload time; only the mapping result is persisted
20+
- All new authenticated endpoints live under `/rest/v2/`
21+
- rq is already present in the repo; this RFC reuses that infrastructure for mapping jobs
22+
23+
---
24+
25+
## Implementation order
26+
27+
1. **User identity first** — nothing else works without knowing who the user is
28+
2. **Schema and storage second** — rows need to exist before the mapping engine writes to them
29+
3. **Mapping execution third** — build and test the core logic before wiring it to HTTP
30+
4. **Upload endpoint fourth** — wire the mapping logic to a real HTTP request
31+
5. **Result retrieval fifth** — expose what the mapping engine produced
32+
33+
**Note:** TODO 3 (mapping engine) is written to be fully testable with fixture data before any real HTTP upload exists, so it can also be built and validated in parallel with TODOs 1–2 if preferred.
34+
35+
---
36+
37+
## TODO 1 — User identity persistence
38+
39+
**What to build:**
40+
A `User` SQLAlchemy model, three auth routes, and session binding.
41+
42+
**Code changes:**
43+
- Add `User` model to `application/database/db.py`:
44+
```
45+
users
46+
id UUID, primary key
47+
google_sub TEXT, NOT NULL, UNIQUE
48+
email TEXT, NOT NULL
49+
display_name TEXT
50+
created_at TIMESTAMP WITH TIME ZONE
51+
last_seen_at TIMESTAMP WITH TIME ZONE
52+
```
53+
`google_sub` is used as the identity anchor because it is immutable; email can change and would silently break identity if used instead. (`google_sub` is the same value currently stored in `session['google_id']`; renamed to `google_sub` to match Google's OIDC `sub` claim.)
54+
55+
- Add to `application/web/web_main.py`:
56+
- `GET /auth/login` → redirect to Google consent; store `next` URL in session
57+
- `GET /auth/callback` → look up or create `User` by `google_sub`; write only `user_id` to Flask session; redirect to `next`
58+
- `GET /auth/logout` → clear session; redirect to `/`
59+
- Update `login_required` decorator to redirect to `/auth/login?next=<current_url>`
60+
- Session stores only `user_id` (UUID). Display name looked up on demand.
61+
- Session duration: 30-day sliding window, configurable via `SESSION_LIFETIME_DAYS` env var
62+
63+
**Reconciling with existing code:** The current `login_required` decorator checks `session['google_id']` and `session['name']` and aborts with 401 when they are missing. TODO 1 migrates it to check `session['user_id']` instead, and to redirect to `/auth/login?next=<current_url>` on a missing session — replacing the 401 with a redirect into the login flow. Display name is no longer stored in the session; it is looked up from the `User` model on demand. Note that `google_sub` is the same immutable Google `sub` claim currently stored as `session['google_id']` — a rename for clarity, not a new identifier.
64+
65+
**Tests:**
66+
- New user → exactly one row in `users`, correct `google_sub`
67+
- Same user logs in again → no new row, `last_seen_at` updated
68+
- Session cookie contains only `user_id` after login
69+
- `/auth/logout` clears session; next request is anonymous
70+
- `login_required` returns 302 for anonymous, 200 for authenticated
71+
- `LOGIN_ALLOWED_DOMAINS` blocks unlisted domains when set
72+
- Concurrent login from 10 users produces no duplicate rows
73+
74+
**Checkpoint — pass criteria:**
75+
- Repeat login does not create a duplicate user row
76+
- Auth-protected endpoint resolves current user reliably
77+
- All tests above green on CI
78+
79+
**Failure / rollback:**
80+
- Forged or expired OAuth state → 400, no session set
81+
- Google returns `sub` but no email → create user, log warning, display UUID
82+
- DB failure during callback → 503, no session set
83+
84+
---
85+
86+
## TODO 2 — User-owned standard and section storage
87+
88+
**What to build:**
89+
Schema and DB access methods for user-owned standards and their sections.
90+
91+
**Code changes:**
92+
- Add two models to `application/database/db.py`:
93+
94+
```
95+
user_standards
96+
id UUID, primary key
97+
user_id UUID, NOT NULL, FK → users.id ON DELETE CASCADE
98+
name TEXT, NOT NULL
99+
created_at TIMESTAMP WITH TIME ZONE
100+
updated_at TIMESTAMP WITH TIME ZONE
101+
UNIQUE (user_id, name)
102+
103+
user_standard_sections
104+
id UUID, primary key
105+
standard_id UUID, NOT NULL, FK → user_standards.id ON DELETE CASCADE
106+
section_id TEXT, NOT NULL
107+
section_text TEXT, NOT NULL
108+
cre_db_id TEXT, nullable ← soft reference to shared CRE node
109+
confidence REAL, nullable
110+
status TEXT, NOT NULL ← PENDING | AUTO_MAPPED | NEEDS_REVIEW |
111+
UNMAPPABLE | USER_CONFIRMED | USER_REJECTED
112+
created_at TIMESTAMP WITH TIME ZONE
113+
updated_at TIMESTAMP WITH TIME ZONE
114+
INDEX (standard_id, status)
115+
INDEX (standard_id, cre_db_id)
116+
```
117+
118+
`cre_db_id` is a soft reference, not a foreign key. CRE nodes live in the graph layer; a hard FK would couple the two layers. Application validates at write time.
119+
120+
- Add to `Node_collection`:
121+
- `create_user_standard(user_id, name) → UserStandard`
122+
- `get_user_standards(user_id) → List[UserStandard]`
123+
- `upsert_user_standard_sections(standard_id, sections) → None`
124+
- `get_user_standard_sections(standard_id, user_id) → List[UserStandardSection]`
125+
- `confirm_section_mapping(section_id, cre_db_id, user_id) → None`
126+
- `reject_section_mapping(section_id, user_id) → None`
127+
128+
All methods that read or write user-owned data require `user_id` as a mandatory parameter — a call without `user_id` is a type error, not a runtime check.
129+
130+
**Tests:**
131+
- `create_user_standard` creates one row linked to correct `user_id`
132+
- `create_user_standard` with duplicate name raises unique constraint error
133+
- `get_user_standards` returns only rows for the requesting user
134+
- `upsert_user_standard_sections` creates rows with `status=PENDING`
135+
- `confirm_section_mapping` sets `USER_CONFIRMED` and stores `cre_db_id`
136+
- Two users uploading same-named standards produce separate rows — zero shared state
137+
138+
**Checkpoint — pass criteria:**
139+
- Two users uploading the same-named standard do not share rows
140+
- All six tests green on CI
141+
142+
**Failure / rollback:**
143+
- Upload with duplicate `standard_name` without `overwrite=true` → 409 Conflict
144+
- Upload with `overwrite=true` → delete existing sections, re-import, reset to PENDING
145+
- DB failure mid-upload → transaction rollback; no partial standard row
146+
147+
---
148+
149+
## TODO 3 — Automated mapping execution
150+
151+
**What to build:**
152+
A standalone mapping function and rq job that takes PENDING sections and produces `cre_db_id` + `confidence` + `status` per section.
153+
154+
**Code changes:**
155+
- Add `application/utils/mapping_job.py`:
156+
- `run_mapping(standard_id: str, database: Node_collection) → None`
157+
- Fetch all PENDING sections for `standard_id`
158+
- Call `model.encode(texts, batch_size=32)` for all sections in one call
159+
- For each section: pgvector similarity → top-20 candidate CRE nodes → cross-encoder re-rank → keep best result
160+
- Write `cre_db_id`, `confidence`, `status` back to `user_standard_sections`:
161+
- `AUTO_MAPPED` if confidence ≥ threshold
162+
- `NEEDS_REVIEW` if below threshold
163+
- `UNMAPPABLE` if no candidates returned
164+
165+
- Register `run_mapping` as an rq job (reuses existing rq infrastructure)
166+
167+
- Re-indexing: when the CRE graph is re-indexed, re-run mapping for all sections where `status IN (PENDING, AUTO_MAPPED, NEEDS_REVIEW)`. Never auto-remap `USER_CONFIRMED` — user decisions are ground truth.
168+
169+
**Tests:**
170+
- Upload ASVS fixture → deterministic count of AUTO_MAPPED vs NEEDS_REVIEW vs UNMAPPABLE
171+
- Batch encoding 200 sections completes in under 1 second on CI runner (CPU)
172+
- Re-running mapping on already-mapped sections does not change `USER_CONFIRMED` rows
173+
- rq job is idempotent: killing and restarting worker re-processes only PENDING sections
174+
175+
**Checkpoint — pass criteria:**
176+
- **Mapping accuracy:** Ground truth is the set of existing human-verified CRE links already in the graph. The metric is exact-match accuracy of the section→CRE mapping — an exact match of the predicted `cre_db_id` against the human-verified link, not precision/recall/F1 — evaluated only against those existing human mappings. Target: ≥ 98% exact-match accuracy on at least one fixture upload (ASVS, WSTG). Treat 98% as a target to validate in an initial spike rather than a hard gate; the deterministic re-run count below is the second pass criterion.
177+
- Deterministic mapped/unmapped counts on re-run of the same fixture
178+
179+
**Failure / rollback:**
180+
- Model fails to load → job fails immediately, sections stay PENDING, error logged
181+
- OOM during cross-encoder → reduce candidate pool from 20 to 10, retry once
182+
- Worker crash mid-job → job returns to queue; idempotent re-run handles recovery
183+
184+
---
185+
186+
## TODO 4 — Upload endpoint wiring
187+
188+
**What to build:**
189+
`POST /rest/v2/myopencre/upload` — requires login, creates user standard + pending rows, enqueues mapping job.
190+
191+
**Code changes:**
192+
- Add to `application/web/web_main.py`:
193+
194+
```
195+
POST /rest/v2/myopencre/upload
196+
requires: login_required
197+
accepts: multipart/form-data with cre_csv file + standard_name field
198+
returns: 202 Accepted + {"standard_id": "...", "job_id": "..."}
199+
```
200+
201+
Route logic:
202+
1. Validate file is CSV and non-empty; validate `standard_name` present
203+
2. Check for duplicate `standard_name` for this user → 409 if exists and no `overwrite=true`
204+
3. Call `create_user_standard(user_id, standard_name)`
205+
4. Parse CSV using existing `parse_export_format()` — reuse existing parser
206+
5. Call `upsert_user_standard_sections(standard_id, sections)`
207+
6. Enqueue `run_mapping(standard_id)` via rq
208+
7. Return 202
209+
210+
- Add `GET /rest/v2/myopencre/standards/<standard_id>/status`:
211+
```json
212+
{"total": 200, "pending": 145, "mapped": 55, "status": "processing"}
213+
```
214+
215+
**Tests:**
216+
- Anonymous POST → 401
217+
- Authenticated POST with valid CSV → 202; DB rows match uploaded section count
218+
- Duplicate `standard_name` without `overwrite=true` → 409
219+
- Duplicate with `overwrite=true` → prior sections deleted, new sections created
220+
- Status endpoint returns correct counts during and after processing
221+
- Status endpoint accessed by a different authenticated user → 403
222+
223+
**Checkpoint — pass criteria:**
224+
- Upload returns `standard_id`; DB rows exactly match uploaded section count
225+
- Status endpoint returns correct in-progress counts while rq job runs
226+
227+
**Failure / rollback:**
228+
- Empty or malformed CSV → 400 with descriptive message; no DB rows created
229+
- DB failure after standard row created but before sections written → rollback entire transaction
230+
231+
---
232+
233+
## TODO 5 — User result retrieval
234+
235+
**What to build:**
236+
Endpoints for the owner to read mapping results and confirm or reject suggestions.
237+
238+
**Code changes:**
239+
- Add to `application/web/web_main.py`:
240+
241+
```
242+
GET /rest/v2/myopencre/standards
243+
requires: login_required
244+
returns: list of user's standards with counts per status
245+
246+
GET /rest/v2/myopencre/standards/<standard_id>/sections
247+
requires: login_required + ownership check
248+
returns: list of sections with cre_db_id, confidence, status
249+
250+
POST /rest/v2/myopencre/standards/<standard_id>/sections/<section_id>/confirm
251+
requires: login_required + ownership check
252+
body: {"cre_db_id": "..."} ← optional override
253+
effect: status → USER_CONFIRMED
254+
255+
POST /rest/v2/myopencre/standards/<standard_id>/sections/<section_id>/reject
256+
requires: login_required + ownership check
257+
effect: status → USER_REJECTED, cre_db_id → null
258+
```
259+
260+
- Centralise ownership check in `require_owns_standard(standard_id, user_id)` helper — call at the top of every route that touches a standard.
261+
262+
**Tests:**
263+
- Owner reads their sections → 200 with correct rows
264+
- Another authenticated user reads the same `standard_id` → 403
265+
- Confirm sets `USER_CONFIRMED` + `cre_db_id`
266+
- Reject sets `USER_REJECTED` + clears `cre_db_id`
267+
- Confirm on an already `USER_CONFIRMED` row → idempotent (no error, no change)
268+
269+
**Checkpoint — pass criteria:**
270+
- Owner can read their results
271+
- Any other user gets 403 on the same endpoint
272+
- Safety and regression tests pass
273+
274+
**Failure / rollback:**
275+
- Confirm/reject on non-existent section → 404
276+
- DB failure during confirm → 503, status unchanged
277+
278+
---
279+
280+
## Future work
281+
282+
Gap analysis on user standards (`GET /rest/v2/gap_analysis?user_standard=<id>&compare=ASVS`) is a natural extension once TODO 5 is merged — user section rows already point to shared CRE nodes, so GA requires only a set difference query against the existing GA infrastructure.
283+
284+
---
285+
286+
## Key constraints summary
287+
288+
| Constraint | Decision |
289+
|---|---|
290+
| No per-user embeddings | Encoding is ephemeral; storing vectors wastes space and couples user tables to the shared index |
291+
| `google_sub` as identity anchor | `sub` is immutable; email changes would silently break user identity |
292+
| Soft reference for `cre_db_id` | Hard FK couples user tables to the graph layer; application validates at write time |
293+
| `/rest/v2/` for all new endpoints | Avoids breaking the current API; isolates new auth-required surface |
294+
| `user_id` mandatory on all DB methods | Prevents accidental cross-user reads; a call without `user_id` is a type error |
295+
| `USER_CONFIRMED` never auto-remapped | User decisions are ground truth; graph re-indexing must not silently overwrite them |
296+
| Login required for all uploads | Automated mapping has significant resource cost; no anonymous persisted flow |
297+
298+
---
299+
300+
## Privacy note
301+
302+
This RFC introduces the first persistent storage of user email and display name. Nothing is currently persisted to the database (per issue #375) — session-only identifying data (`name`, `google_id`) already exists in the Flask session, but this RFC is the first to store user email and display name in the database. Privacy policy must be updated before production rollout on opencre.org — tracked separately with maintainers, non-blocking for this coding increment.

0 commit comments

Comments
 (0)