feat(api): per-user resource selection endpoint — Part of #586#981
feat(api): per-user resource selection endpoint — Part of #586#981skypank-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.
skypank-coder
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
|
Warning Review limit reached
Next review available in: 15 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: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds persisted OIDC user identities and per-user resource selections, integrates persistence into the login callback, exposes feature-gated GET/PUT endpoints, documents them, and adds database, callback, and API tests. ChangesUser persistence and resource selection
Estimated code review effort: 4 (Complex) | ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
application/web/web_main.py (2)
1233-1246: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider disabling caching for per-user selection responses.
Neither
GETnorPUT /rest/v1/user/resourcessetsCache-Control, so a per-user list could be cached by an intermediary proxy or browser (e.g., shared-computer back/forward cache) and served to a different session.Also applies to: 1249-1269
🤖 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 1233 - 1246, Disable caching for both get_user_resources and the corresponding PUT user-resources handler by adding an explicit no-cache/no-store Cache-Control response header to their JSON responses. Ensure the header applies to authenticated per-user selections, including the feature-disabled fallback where applicable.
846-882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten type hints for mypy compliance.
feature_enabled_or_default(is_enabled: Any, default_factory: Any) -> Anyand_resolve_current_user(database):useAny/no annotations, unlike the fully-typedupsert_user/get_user_by_subindb.py. As per coding guidelines,**/*.pyshould "Runmake mypyfor Python type checking" — looseAnytyping here defeats that check for callable misuse (e.g. a non-callabledefault_factory).def feature_enabled_or_default( is_enabled: Callable[[], bool], default_factory: Callable[[], Any] ) -> Callable[[Callable], Callable]: ... def _resolve_current_user(database: "db.Node_collection") -> Optional["db.User"]: ...🤖 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 846 - 882, Tighten the annotations on feature_enabled_or_default by replacing Any with callable types for is_enabled and default_factory and an appropriately typed decorator return, importing the required typing symbols. Annotate _resolve_current_user’s database parameter with db.Node_collection and its return value with Optional[db.User], preserving the existing user lookup and creation behavior.Source: Coding guidelines
🤖 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 1101-1125: Update set_user_resource_selection to handle concurrent
writes using the same IntegrityError recovery pattern established by
upsert_user: catch the insert/commit failure, roll back the session, and retry
the replacement operation so concurrent PUTs do not surface an unhandled error.
Preserve deduplication and return the refreshed selection after a successful
commit.
In `@application/web/openapi_registry.py`:
- Around line 333-340: Update put_user_resources in
application/web/openapi_registry.py to configure PathSpec request-body
generation for a required JSON object containing a required selected array of
non-empty strings, then update docs/api/openapi.yaml at lines 516-533 with the
matching required application/json requestBody schema.
In `@application/web/web_main.py`:
- Around line 1256-1263: Normalize each string in selected by trimming leading
and trailing whitespace before passing it to set_user_resource_selection in
put_user_resources. Validate and persist the normalized values so equivalent
entries such as whitespace-padded and unpadded names deduplicate consistently.
---
Nitpick comments:
In `@application/web/web_main.py`:
- Around line 1233-1246: Disable caching for both get_user_resources and the
corresponding PUT user-resources handler by adding an explicit no-cache/no-store
Cache-Control response header to their JSON responses. Ensure the header applies
to authenticated per-user selections, including the feature-disabled fallback
where applicable.
- Around line 846-882: Tighten the annotations on feature_enabled_or_default by
replacing Any with callable types for is_enabled and default_factory and an
appropriately typed decorator return, importing the required typing symbols.
Annotate _resolve_current_user’s database parameter with db.Node_collection and
its return value with Optional[db.User], preserving the existing user lookup and
creation 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 688f202a-5958-4507-a9d8-43f0df37825c
📒 Files selected for processing (9)
application/database/db.pyapplication/tests/user_model_test.pyapplication/tests/user_resources_api_test.pyapplication/tests/web_main_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yamlmigrations/versions/a1b2c3d4e5f6_add_users_and_resource_selection.pymigrations/versions/f0e1d2c3b4a5_merge_heads_before_users.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
application/web/openapi_registry.py (1)
307-334: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the
401response for both resource endpoints.
web_main.pyreturns401for anonymous requests when both feature flags are enabled, but thesePathSpecs document only200and, forPUT,400. Add a401response to both entries and keepdocs/api/openapi.yamlsynchronized so clients can discover the authentication failure.Also applies to: 336-343
🤖 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/openapi_registry.py` around lines 307 - 334, Add a documented 401 response to both resource endpoint PathSpec entries, including the GET entry identified by get_user_resources and the corresponding PUT entry, while preserving their existing responses. Regenerate or update docs/api/openapi.yaml so the OpenAPI specification remains synchronized and exposes the authentication failure.docs/api/openapi.yaml (2)
510-515: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRequire
selectedin both response schemas.The documented response is
{"selected": [...]}, but omittingrequired: [selected]allows{}according to OpenAPI.Proposed schema fix
type: object + required: + - selected properties: selected:Apply this to both the GET and PUT response schemas.
Also applies to: 542-547
🤖 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 `@docs/api/openapi.yaml` around lines 510 - 515, Add `required: [selected]` to both GET and PUT response object schemas containing the `selected` property in the OpenAPI definition, ensuring each documented response requires that field while preserving its existing array-of-string type.
501-505: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the conditional 401 behavior.
The endpoint contract says anonymous users receive
401when both login and MyOpenCRE flags are enabled, but neither operation documents that response. Clarify the GET description and add a401response to both GET and PUT; when either flag is disabled, document the empty-selection fallback instead.Also applies to: 536-549
🤖 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 `@docs/api/openapi.yaml` around lines 501 - 505, Update the selected-standards GET and PUT OpenAPI definitions to document that authenticated access is required when both login and MyOpenCRE are enabled, including a 401 response for anonymous users. Clarify each GET/PUT description to state that the endpoint returns an empty selection when either feature flag is disabled, and add the corresponding 401 response entries to both operations.
🤖 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.
Outside diff comments:
In `@application/web/openapi_registry.py`:
- Around line 307-334: Add a documented 401 response to both resource endpoint
PathSpec entries, including the GET entry identified by get_user_resources and
the corresponding PUT entry, while preserving their existing responses.
Regenerate or update docs/api/openapi.yaml so the OpenAPI specification remains
synchronized and exposes the authentication failure.
In `@docs/api/openapi.yaml`:
- Around line 510-515: Add `required: [selected]` to both GET and PUT response
object schemas containing the `selected` property in the OpenAPI definition,
ensuring each documented response requires that field while preserving its
existing array-of-string type.
- Around line 501-505: Update the selected-standards GET and PUT OpenAPI
definitions to document that authenticated access is required when both login
and MyOpenCRE are enabled, including a 401 response for anonymous users. Clarify
each GET/PUT description to state that the endpoint returns an empty selection
when either feature flag is disabled, and add the corresponding 401 response
entries to both operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 55780973-4784-49cb-b6fd-8379a657de19
📒 Files selected for processing (5)
application/database/db.pyapplication/tests/user_resources_api_test.pyapplication/web/openapi_registry.pyapplication/web/web_main.pydocs/api/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- application/tests/user_resources_api_test.py
- application/web/web_main.py
- application/database/db.py
1 participant
What & why
Second increment of #586. Builds on PR1's user persistence to expose the
per-user resource selection over the API, so the frontend (PR4) and server-side
filtering (PR3) have a read/write surface. No new tables or migrations — pure
HTTP + validation over PR1's
Node_collectionmethods.Endpoints
/rest/v1/user/resources(canonical/rest/v1surface):GET→{"selected": [...]}— the current user's saved standard names, sorted.PUT{"selected": ["ASVS","CWE"]}→ dedup + replace, persists via PR1'sset_user_resource_selection, returns the stored list.Gating
Requires
login_required+is_login_enabled()+is_myopencre_enabled()(per the #966 decision):
{"selected": []}, 200, no auth, no writes.any DB read/write.
selected, not a list, empty/non-string entries) → 400.Implemented via a generalized
feature_enabled_or_defaulthelper composed overlogin_required(a generalization of PR1's flag-gating; PR1's behaviour isunchanged).
OpenAPI
Endpoint added to the spec properly — two
PathSpecentries +@openapi_documenteddecorators,docs/api/openapi.yamlregenerated (nothand-edited). The OpenAPI guardrail passes all four checks (documented views,
freshness, validity, route coverage).
Testing
12 new tests, verified on real Postgres (prod parity): flag-off default,
myopencre-off default + no-write, anonymous 401, dedup, replace against the live
UNIQUE(user_id, standard_name), and body-validation 400s. The myopencre-offtests are mutation-proven — removing the capability from the gate makes them fail
(GET leaks the saved selection, PUT persists), restoring it makes them pass — so
they genuinely detect a bypassed gate rather than passing incidentally.
blackclean; no new mypy errors vs. baseline.Scope boundaries
No server-side filtering (PR3), no frontend (PR4), no new DB surface. Reuses
PR1's methods and
login_required.Files (4)
application/web/web_main.py— routes + gate +feature_enabled_or_default.application/web/openapi_registry.py— twoPathSpecentries.docs/api/openapi.yaml— regenerated.application/tests/user_resources_api_test.py— new, 12 tests.