feat(db): persist users + resource selection — Part of #586 #980
feat(db): persist users + resource selection — Part of #586 #980skypank-coder wants to merge 9 commits into
Conversation
…WASP#586) Persist accounts on OIDC login and store a per-user resource (standard) selection. Additive to the existing session auth: the callback upserts the User row and sets session['user_id'] only when CRE_ENABLE_LOGIN is on; the existing session keys and 401 behaviour are unchanged. - User(google_sub UNIQUE, email, display_name, created_at, last_seen_at) - UserResourceSelection(user_id FK CASCADE, standard_name, UNIQUE(user_id, name)) - Node_collection: upsert_user / get_user_by_sub / get_user_resource_selection / set_user_resource_selection - Alembic: dedicated merge of the two open heads + create tables (up/down verified on Postgres; guardrail green)
…int (OWASP#586) Address PR review: - upsert_user now mirrors add_node: catch IntegrityError, rollback, re-query by google_sub, update profile — safe under concurrent logins. - Drop column-level unique=True on User.google_sub; keep only the named constraint so create_all matches the Alembic migration (no schema drift). - Login callback skips persistence when the OIDC 'sub' claim is missing. - Callback test asserts session['user_id'] equals the persisted User.id.
…OWASP#586) Address PR review: - Log only the exception class on login-persistence failure; the raw message can carry SQL parameters (email, OIDC sub). - Add callback test: login enabled but token missing 'sub' creates no user row and leaves session['user_id'] unset.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds SQLAlchemy models, migration support, persistence APIs, and tests for OIDC users and per-user resource selections. Login callback and development login flows optionally persist users and store their IDs in sessions. ChangesUser persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
application/database/db.py (2)
1038-1089: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the repeated
datetime/timezoneimport to module scope.
from datetime import datetime, timezoneis imported locally in bothupsert_user(Line 1046) andset_user_resource_selection(Line 1105). Move it to the top-level imports once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/database/db.py` around lines 1038 - 1089, Move the repeated “from datetime import datetime, timezone” import from the methods upsert_user and set_user_resource_selection to the module-level imports, leaving both methods to use the shared top-level import without local import statements.
1101-1125: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
set_user_resource_selectionhas no guard against concurrent duplicate inserts.Unlike
upsert_userabove (which recovers fromIntegrityErrorongoogle_subraces), this delete-then-insert sequence has no equivalent safeguard. Two concurrent calls for the sameuser_id(e.g. a double-submitted "save selection" request) could both pass the delete step and then collide onuq_user_resource_selectionfor a sharedstandard_name, raising an unhandledIntegrityError. No endpoint calls this yet, but it's worth hardening before wiring it up.♻️ Suggested guard mirroring upsert_user's recovery pattern
self.session.query(UserResourceSelection).filter( UserResourceSelection.user_id == user_id ).delete() for name in deduped: self.session.add( UserResourceSelection( id=generate_uuid(), user_id=user_id, standard_name=name, created_at=now, ) ) - self.session.commit() + try: + self.session.commit() + except IntegrityError: + # A concurrent call raced on the same (user_id, standard_name) pair; + # the other writer's result wins, just return the current state. + self.session.rollback() return self.get_user_resource_selection(user_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/database/db.py` around lines 1101 - 1125, Harden set_user_resource_selection against concurrent IntegrityError races during its delete-and-insert sequence. Mirror the existing recovery pattern used by upsert_user: catch the uniqueness violation, roll back the failed transaction, and return the persisted selection for user_id without leaving the exception unhandled; preserve the current deduplication and replacement behavior for non-conflicting calls.application/web/web_main.py (1)
1164-1185: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBroad exception catch is intentional but worth narrowing eventually.
Ruff flags the blind
except Exceptionhere (BLE001). The rationale in the comment (avoid leaking SQL parameters) is sound forupsert_userfailures specifically, but the broad catch also silently swallows unrelated bugs (e.g., a malformedid_infodict) that would otherwise be visible. Consider catchingSQLAlchemyErrorspecifically if you want DB failures to degrade gracefully while other exceptions still surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/web/web_main.py` around lines 1164 - 1185, In the user persistence block around upsert_user, replace the broad Exception handler with SQLAlchemyError and import that exception from the project’s SQLAlchemy dependency. Preserve the existing sanitized class-name logging and graceful redirect for database failures, while allowing unrelated errors such as malformed id_info data to propagate.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/database/db.py`:
- Around line 1038-1089: Move the repeated “from datetime import datetime,
timezone” import from the methods upsert_user and set_user_resource_selection to
the module-level imports, leaving both methods to use the shared top-level
import without local import statements.
- Around line 1101-1125: Harden set_user_resource_selection against concurrent
IntegrityError races during its delete-and-insert sequence. Mirror the existing
recovery pattern used by upsert_user: catch the uniqueness violation, roll back
the failed transaction, and return the persisted selection for user_id without
leaving the exception unhandled; preserve the current deduplication and
replacement behavior for non-conflicting calls.
In `@application/web/web_main.py`:
- Around line 1164-1185: In the user persistence block around upsert_user,
replace the broad Exception handler with SQLAlchemyError and import that
exception from the project’s SQLAlchemy dependency. Preserve the existing
sanitized class-name logging and graceful redirect for database failures, while
allowing unrelated errors such as malformed id_info data to propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4fb83f8c-bd64-4f81-84cb-24a695e2c0d6
📒 Files selected for processing (6)
application/database/db.pyapplication/tests/user_model_test.pyapplication/tests/web_main_test.pyapplication/web/web_main.pymigrations/versions/a1b2c3d4e5f6_add_users_and_resource_selection.pymigrations/versions/f0e1d2c3b4a5_merge_heads_before_users.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/database/db.py (1)
1111-1114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify list deduplication.
You can simplify the deduplication logic and avoid an O(N^2) loop by using
dict.fromkeys(), which natively preserves insertion order and operates in O(N) time.♻️ Proposed refactor
- deduped: List[str] = [] - for name in standard_names: - if name not in deduped: - deduped.append(name) + deduped = list(dict.fromkeys(standard_names))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/database/db.py` around lines 1111 - 1114, Replace the manual deduplication loop over standard_names with dict.fromkeys() to preserve insertion order while reducing the operation to linear time, and keep the resulting value as a List[str] assigned to deduped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/database/db.py`:
- Around line 1111-1114: Replace the manual deduplication loop over
standard_names with dict.fromkeys() to preserve insertion order while reducing
the operation to linear time, and keep the resulting value as a List[str]
assigned to deduped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e65bb199-a9a4-482c-a895-0baf19a0a577
📒 Files selected for processing (3)
application/database/db.pyapplication/tests/web_main_test.pyapplication/web/web_main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- application/web/web_main.py
- application/tests/web_main_test.py
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Thanks — this is a clean foundation for #586: idempotent upsert with race recovery, cascade + uniqueness covered, callback gated so persistence can't break login, and solid Postgres-minded tests.
Blocking: migration lineage conflicts with current main (see inline). After that fix I'm happy to approve.
Also (not in the diff, so calling out here): NO_LOGIN=1 in /rest/v1/login still only sets google_id / name, not session["user_id"], and never upserts a User. Local/dev will look logged in while PR2's /rest/v1/user/resources has nothing to hang selection on. Please either fix here or explicitly handle in #981.
Once this lands, please rebase #981 onto updated main (or stack it on this branch) so we don't re-review the DB layer.
…NO_LOGIN (OWASP#586) Address review on OWASP#980: - Drop obsolete merge revision f0e1d2c3b4a5; main already merges the two embedding heads via c7d8e9f0a1b2 (OWASP#979). Second merge left two Alembic heads. - Point users migration at c7d8e9f0a1b2 and update the Revises docstring. Verified: flask db heads reports a single head; upgrade/downgrade/upgrade clean on Postgres. - NO_LOGIN dev login now upserts the User and sets session['user_id'] when CRE_ENABLE_LOGIN is on, so local dev sessions have a row for user-scoped endpoints. Covered by tests for flag-on and flag-off. - Narrow login persistence except from Exception to SQLAlchemyError.
…NO_LOGIN (OWASP#586) Address review on OWASP#980: - Drop obsolete merge revision f0e1d2c3b4a5; main already merges the two embedding heads via c7d8e9f0a1b2 (OWASP#979). Second merge left two Alembic heads. - Point users migration at c7d8e9f0a1b2 and update the Revises docstring. Verified: flask db heads reports a single head; upgrade/downgrade/upgrade clean on Postgres. - NO_LOGIN dev login now upserts the User and sets session['user_id'] when CRE_ENABLE_LOGIN is on, so local dev sessions have a row for user-scoped endpoints. Covered by tests for flag-on and flag-off. - Narrow login persistence except from Exception to SQLAlchemyError.
38b49a7 to
fd5bd59
Compare
|
@northdpole Thanks for the thorough review — all three points addressed in fd5bd59. Migration lineage (blocking). You were right, and I reproduced it before fixing: flask db heads reported both a1b2c3d4e5f6 and c7d8e9f0a1b2. Since c7d8e9f0a1b2 already carries down_revision = ("9b1c2d3e4f50", "ab12cd34ef56"), my revision was a duplicate merge of the same two parents. Deleted f0e1d2c3b4a5_merge_heads_before_users.py entirely NO_LOGIN dev path. Good catch — fixed here rather than deferring to #981, since it belongs with the persistence work. /rest/v1/login now upserts the dev User and sets session["user_id"] when CRE_ENABLE_LOGIN is on, so local/dev sessions have a real row for user-scoped endpoints. Two tests cover it: persists with the flag on, no-op with it off. Nit — bare except. Adopted. Narrowed to SQLAlchemyError in both the callback and the new dev path, so DB failures stay soft while unexpected non-DB bugs surface. On #981. It's stacked directly on this branch (verified this branch's tip is an ancestor of it). Once this merges I'll rebase #981 onto updated main so the DB layer drops out of its diff and you only review the resource-selection endpoint. Ready for another look. |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-reviewed after fd5bd59 — all three prior points addressed:
- obsolete merge revision dropped; users migration chains off
c7d8e9f0a1b2(single head) NO_LOGINpath upserts User + setssession["user_id"]when login is enabled (with flag-off coverage)- persistence failures narrowed to
SQLAlchemyError
CI green. LGTM. Please merge/rebase main (just #837) before merge if strict up-to-date checks require it, then rebase #981 onto updated main.
|
@northdpole Branch is updated with #837 and CI's green — ready to merge whenever you are. I'll rebase #981 onto main right after it lands. |
2 participants
What & why
First increment of #586 ("Add support for users"): login-first user
persistence. OIDC login existed but nothing was persisted — no User table,
nowhere to hang per-user data. This adds that foundation so per-user resource
filtering (PR2–PR4) can be built on it. Aligns with auth RFC #876 (TODO 1/2).
Changes
userstable anchored on the immutable OIDCsub(google_sub); emailand display_name refreshed on each login.
user_resource_selectiontable: one row per selected standard,UNIQUE(user_id, standard_name),FK → users.id ON DELETE CASCADE.session['user_id']— gated onCRE_ENABLE_LOGIN, wrapped so it never blocks login. Existing session keysand the anonymous 401 are unchanged (intentionally narrower than RFC RFC: User Authentication and Online MyOpenCRE Mapping #876's
login_requiredrewrite).Node_collectionmethods:upsert_user,get_user_by_sub,get_user_resource_selection,set_user_resource_selection. These back the/rest/v1/userresource endpoints coming in PR2.open heads (
ab12cd34ef56,9b1c2d3e4f50), then a revision creates the twotables — keeping the head single-parent so
flask db downgradeisunambiguous.
Testing
dedup / per-user isolation, unique constraint, cascade delete, callback upsert,
and the flag-off no-op path.
ON DELETE CASCADEenforced at the DB level (rows 1 → 0), both uniqueconstraints created cleanly, migration
upgrade → downgrade → upgradedeterministic, alembic revision guardrail green. 12/12 pass on Postgres.
blackclean; no new mypy errors vs. baseline.Scope boundaries
No new endpoints, no server-side filtering, no frontend, no
login_requiredrewrite — those are the follow-up PRs in #586.
Unrelated pre-existing issue (out of scope, flagging for visibility)
flask db upgradefrom a blank DB fails on Postgres before reaching myrevisions: migrations
3c65127871a6/7bf4eac76958reuse the constraint nameuq_pair(authored via SQLitebatch_alter_table), which collides on afrom-base replay. Postgres rolls the whole thing back cleanly (transactional
DDL). This predates this branch and doesn't affect the prod path (apply-onto-
existing-schema), so it's out of scope here — but worth a separate fix, since
anyone bootstrapping a fresh Postgres DB will hit it.