feat(db): persist users + resource selection — Part of #586#973
Closed
skypank-coder wants to merge 6 commits into
Closed
feat(db): persist users + resource selection — Part of #586#973skypank-coder wants to merge 6 commits into
skypank-coder wants to merge 6 commits into
Conversation
…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)
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds SQLAlchemy models and persistence methods for OIDC users and per-user resource selections. Integrates user upserts into the OAuth callback, adds Alembic migrations including a head merge, and expands database and callback tests. 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)
✨ 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}
Contributor
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
approved these changes
Jul 9, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
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.
…int (#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.
skypank-coder
requested review from
Pa04rth,
paoga87 and
robvanderveer
as code owners
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/tests/web_main_test.py (1)
1193-1218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the missing-
subcallback path.Add a flag-on test with no
subclaim that asserts noUserrow is created andsession["user_id"]remains absent. This protects the new safe-skip behavior.🤖 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/tests/web_main_test.py` around lines 1193 - 1218, Add a companion callback test alongside test_callback_does_not_persist_when_login_flag_off with CRE_ENABLE_LOGIN enabled and an OAuth token payload missing "sub"; assert the request completes without creating a db.User row and that session["user_id"] remains absent, preserving the safe-skip behavior.
🤖 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/web/web_main.py`:
- Around line 1174-1182: Update the exception handling around
Node_collection().upsert_user in the login flow to avoid logging the raw
exception or its details. Preserve the best-effort redirect behavior, but
replace the interpolated exception text with a fixed message containing only a
safe, non-sensitive error classification.
---
Outside diff comments:
In `@application/tests/web_main_test.py`:
- Around line 1193-1218: Add a companion callback test alongside
test_callback_does_not_persist_when_login_flag_off with CRE_ENABLE_LOGIN enabled
and an OAuth token payload missing "sub"; assert the request completes without
creating a db.User row and that session["user_id"] remains absent, preserving
the safe-skip behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 387ed746-d906-41b1-b39a-ed097abdd3b0
📒 Files selected for processing (3)
application/database/db.pyapplication/tests/web_main_test.pyapplication/web/web_main.py
…#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.