feat(db): persist users + resource selection — Part of #586#973
feat(db): persist users + resource selection — Part of #586#973skypank-coder wants to merge 4 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)
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds SQLAlchemy ChangesUser persistence and resource selection feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@application/database/db.py`:
- Around line 264-277: The User model defines google_sub with both column-level
unique=True and a separate UniqueConstraint in __table_args__, which creates
duplicate unique constraints during schema creation. Update the User model in
db.py to keep only the single named UniqueConstraint on google_sub and remove
the column-level unique flag so the SQLAlchemy metadata matches the Alembic
migration and avoids schema drift.
- Around line 1038-1067: `upsert_user` has a check-then-act race that can cause
a duplicate insert and leave the session in a failed state. Update `upsert_user`
to follow the same concurrency-safe pattern used by `add_node`: attempt the
insert, catch `IntegrityError`, call `rollback()`, then re-query the existing
`User` by `google_sub` and apply the updates. Keep the existing refresh behavior
for `email`, `display_name`, and `last_seen_at`, and make sure the method always
returns a valid `User` after the retry path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7acbdb65-8751-46e1-89bc-1022ff1cc976
📒 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
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Alembic / deploy
Verified migration graph locally; prod alembic_version confirmed — both embedding heads (ab12cd34ef56, 9b1c2d3e4f50) present. This PR correctly merges them (no-op) then adds two new tables. Deploy path is additive only.
Prod impact
Low. Runtime change is flag-gated (CRE_ENABLE_LOGIN) and failure-safe on callback. No user-facing behavior in PR1. LGTM on scope for #586 / RFC #876 TODO 1.
Approved. A few non-blocking nits inline — address now or track for a follow-up.
|
|
||
|
|
||
| revision = "f0e1d2c3b4a5" | ||
| down_revision = ("ab12cd34ef56", "9b1c2d3e4f50") |
There was a problem hiding this comment.
Good call merging the two embedding heads before adding users — keeps downgrade unambiguous and matches prod's two-row alembic_version state. No concerns on the no-op merge pattern.
| sa.Column("user_id", sa.String(), nullable=False), | ||
| sa.Column("standard_name", sa.String(), nullable=False), | ||
| sa.Column("created_at", sa.DateTime(), nullable=False), | ||
| sa.ForeignKeyConstraint( |
There was a problem hiding this comment.
Clean additive migration. ON DELETE CASCADE on user_id matches the model and tests.
|
|
||
| __tablename__ = "users" | ||
| id = sqla.Column(sqla.String, primary_key=True, default=generate_uuid) | ||
| google_sub = sqla.Column(sqla.String, nullable=False, unique=True) |
There was a problem hiding this comment.
Nit: google_sub has both unique=True on the column and UniqueConstraint(..., name="uq_users_google_sub") in __table_args__. The Alembic migration only creates the named constraint, so sqla.create_all() in tests may diverge (double unique). Suggest dropping the column-level unique=True and keeping only the named constraint.
| from datetime import datetime, timezone | ||
|
|
||
| now = datetime.now(timezone.utc) | ||
| user = self.session.query(User).filter(User.google_sub == google_sub).first() |
There was a problem hiding this comment.
Minor robustness: check-then-insert can race on concurrent logins for the same google_sub. Second commit() would raise IntegrityError and leave the session dirty. Worth mirroring the add_node pattern: catch IntegrityError, rollback(), re-query by google_sub, update profile fields, return. Low urgency for PR1 but cheap to add now.
| # unchanged if this no-ops (flag off) or fails. | ||
| if is_login_enabled(): | ||
| try: | ||
| user = db.Node_collection().upsert_user( |
There was a problem hiding this comment.
Like the flag gate + broad try/except — login must not break if persistence fails. Two small notes:
id_info.get("sub")can beNone; consider bailing early (log + skip upsert) rather than hitting a NOT NULL violation.Node_collection()initializesNEO_DB.instance()unlessNO_LOAD_GRAPH_DB=1. Probably fine (singleton), but a lighter login path could avoid pulling Neo4j into the auth callback. Non-blocking for PR1.
|
|
||
| @patch("application.web.web_main.id_token") | ||
| @patch("application.web.web_main.CREFlow") | ||
| def test_callback_upserts_user_and_sets_session( |
There was a problem hiding this comment.
Good coverage for flag-on persist and flag-off no-op. Small addition: assert session['user_id'] equals the persisted User.id in the flag-on test — catches session/DB mismatch before PR2 wires /rest/v1/user resource endpoints.
| @@ -0,0 +1,164 @@ | |||
| """Tests for user persistence and per-user resource selection (issue #586, RFC #876 TODO 1/2).""" | |||
There was a problem hiding this comment.
Solid SQL-layer coverage: idempotent upsert, selection round-trip/replace/dedup/isolation, unique constraint, cascade delete. Postgres verification in the PR description is the right bar for prod parity.
…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.
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.