fix(auth): admins can access any task; expose super_admin_mode; introduce authorize() PDP - #589
Conversation
… admin UI Admins see every user's task in the admin jobs list, but the task status/error/ logs/cancel endpoints (all via require_task_owner) were owner-only, so clicking another user's job in the admin UI was a guaranteed 403 dead-end. Introduce a single authorization decision point — AuthService.authorize(user, action, resource), deny-by-default — and route require_task_owner through it instead of inlining the check. Today it handles task access (owner OR admin); it is the seam other authz checks migrate onto, with a TODO(org) for org-scoped admins. Expose super_admin_mode via the admin-only /config so the admin UI's permission layer mirrors the backend rule instead of assuming every admin has partition access. Design: docs/refactoring/AUTHORIZATION_ARCHITECTURE.md (Phase 0).
|
Warning Review limit reached
Next review available in: 16 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.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesAuthorization centralization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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: 1
🤖 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 `@openrag/services/orchestrators/auth_service.py`:
- Around line 547-554: The `AuthService.authorize` handling for `task:access`
can incorrectly allow access when both the user identity and task owner are
missing, because the equality check in `resource.get("user_id") ==
cls._uget(user, "id", None)` treats two absent values as a match. Update the
`authorize` branch to explicitly require both `user_id` and the user `id` to be
present before comparing, while preserving the admin shortcut, and add a
regression test covering `user=None` and a task with missing or null `user_id`
to ensure deny-by-default 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: e266baaa-cbc7-4aba-898c-e49dbba7318c
📒 Files selected for processing (5)
openrag/api/dependencies/auth.pyopenrag/api/main.pyopenrag/services/orchestrators/auth_service.pytests/unit/api/dependencies/test_auth.pytests/unit/services/orchestrators/test_auth_service.py
Satisfy the layer-import guard (api must not import services directly) by injecting auth_service through DI in require_task_owner instead of importing AuthService, matching how the partition dependencies call check_partition_access. Also apply ruff format.
…default) Comparing resource.user_id to user.id returned True when both were None, so a missing/partially-hydrated principal or a malformed task (user_id=None) could be authorized. Require both ids present before comparing. Adds a regression test.
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
Approved!
I tested the task access flow locally: owner access works, admin access to another user’s task works, and another non-admin user is still denied. /config also exposes super_admin_mode as expected.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
I've tested it and it works
3 participants
First slice of the admin-UI permission work (review item #1), done through a small, extensible authorization seam rather than an inline bypass.
Problem
Admins see every user's task in the admin jobs list, but the task status/error/logs/cancel endpoints (all guarded by
require_task_owner) were owner-only — no admin path. So an admin clicking another user's job in the admin UI got a guaranteed 403 dead-end.Change
AuthService.authorize(user, action, resource)— deny-by-default, policy in one place instead of inlined at call sites (the fragmentation that produced these bugs). Today it handlestask:access(owner or admin); unknown actions raise rather than silently allow. It's the seam other checks migrate onto, with aTODO(org)for org-scoped admins.require_task_ownernow delegates to it (covers all four task endpoints).super_admin_modevia the admin-only/configso the admin UI permission layer can mirror the backend rule (admins only bypass partition checks whenSUPER_ADMIN_MODEis on) instead of assuming every admin has partition access.Scope / non-goals
docs/refactoring/AUTHORIZATION_ARCHITECTURE.md, Phase 0).Verification
authorize()tests +require_task_owneradmin-allow / non-owner-deny)./configreturnssuper_admin_modewithrdbintact.Summary by CodeRabbit
New Features
/configresponse to return settings in a client-friendly serialized format and added asuper_admin_modefield.Bug Fixes
Tests