From b30e9b0094de64b5a5e2ad2a6a1663c387e797ab Mon Sep 17 00:00:00 2001 From: andyne13 Date: Mon, 29 Jun 2026 10:01:06 +0200 Subject: [PATCH 1/3] fix(auth): admins can access any task; expose super_admin_mode to the admin UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- openrag/api/dependencies/auth.py | 7 ++++- openrag/api/main.py | 11 ++++++-- .../services/orchestrators/auth_service.py | 27 +++++++++++++++++++ tests/unit/api/dependencies/test_auth.py | 27 +++++++++++++++++++ .../orchestrators/test_auth_service.py | 23 ++++++++++++++++ 5 files changed, 92 insertions(+), 3 deletions(-) diff --git a/openrag/api/dependencies/auth.py b/openrag/api/dependencies/auth.py index fdfc4412f..946a1b743 100644 --- a/openrag/api/dependencies/auth.py +++ b/openrag/api/dependencies/auth.py @@ -5,6 +5,7 @@ from core.utils.logging import get_logger from di.providers import get_auth_service, get_config, get_job_service, get_partition_service from fastapi import Depends, HTTPException, Request, status +from services.orchestrators.auth_service import AuthService logger = get_logger() @@ -199,7 +200,11 @@ async def require_task_owner( status_code=status.HTTP_404_NOT_FOUND, detail=f"Task '{task_id}' not found", ) - if task_details.get("user_id") != user.get("id"): + # Delegate the decision to the central PDP (AuthService.authorize) rather than + # inlining policy here — admins may access any task (parity with the admin jobs + # list), owners their own. Without this, an admin opening another user's job in + # the admin UI hits a 403 dead-end. + if not AuthService.authorize(user=user, action="task:access", resource=task_details): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="You do not have permission to access this task", diff --git a/openrag/api/main.py b/openrag/api/main.py index b9a2e5de3..319af7bfd 100644 --- a/openrag/api/main.py +++ b/openrag/api/main.py @@ -319,8 +319,15 @@ def root_redirect(): @app.get("/config", summary="Get current configuration", tags=["Configuration"], dependencies=[Depends(require_admin)]) def get_config(): - """Return the loaded application settings for admins.""" - return settings + """Return the loaded application settings for admins, plus runtime auth flags + the admin UI needs. ``super_admin_mode`` governs whether an admin bypasses + partition-membership checks; the UI permission layer mirrors the backend rule + instead of assuming every admin has partition access. + """ + from api.dependencies.auth import SUPER_ADMIN_MODE + from fastapi.encoders import jsonable_encoder + + return {**jsonable_encoder(settings), "super_admin_mode": SUPER_ADMIN_MODE} # Router mounts. Phase 10F finished moving these into diff --git a/openrag/services/orchestrators/auth_service.py b/openrag/services/orchestrators/auth_service.py index 502549892..39cb0293e 100644 --- a/openrag/services/orchestrators/auth_service.py +++ b/openrag/services/orchestrators/auth_service.py @@ -528,6 +528,33 @@ def check_partition_access( ) return True + @classmethod + def authorize(cls, *, user: Any, action: str, resource: dict[str, Any] | None = None) -> bool: + """Single authorization decision point (PDP) — deny-by-default. + + The seam every capability check should route through so policy lives in one + place instead of being inlined at call sites (which is how the rules drift). + Each ``action`` owns its rule here; an unknown action is a programming error + and is denied loudly rather than silently allowed. + + Intentionally minimal today (the only migrated action is task access). Other + checks (partition roles via :meth:`check_partition_access`) will migrate here. + + TODO(org): add an org-admin tier — allow when ``user`` is an admin of the + organization that owns ``resource`` (``resource['org_id'] == user's org``). + Until orgs exist, ``is_admin`` is the system-wide admin signal. + """ + match action: + case "task:access": + # Admins may access any task — consistent with the admin jobs list, + # which lists every user's tasks. Otherwise only the task's owner. + if cls._uget(user, "is_admin", False): + return True + resource = resource or {} + return resource.get("user_id") == cls._uget(user, "id", None) + case _: + raise ValueError(f"Unknown authorization action: {action!r}") + @classmethod def validate_file_quota( cls, diff --git a/tests/unit/api/dependencies/test_auth.py b/tests/unit/api/dependencies/test_auth.py index f6800fb4e..236e44f6b 100644 --- a/tests/unit/api/dependencies/test_auth.py +++ b/tests/unit/api/dependencies/test_auth.py @@ -200,6 +200,33 @@ async def test_require_task_owner_reads_task_details_through_job_service(): assert job_service.detail_checks == ["task-1"] +@pytest.mark.asyncio +async def test_require_task_owner_allows_admin_for_another_users_task(): + # Admins may open any task (parity with the admin jobs list) — no 403 dead-end. + job_service = FakeJobService(details={"user_id": 2, "filename": "b.pdf"}) + + details = await require_task_owner( + task_id="task-2", + user={"id": 1, "is_admin": True}, + job_service=job_service, + ) + + assert details == {"user_id": 2, "filename": "b.pdf"} + + +@pytest.mark.asyncio +async def test_require_task_owner_rejects_non_owner_non_admin(): + job_service = FakeJobService(details={"user_id": 2, "filename": "b.pdf"}) + + with pytest.raises(HTTPException) as exc: + await require_task_owner( + task_id="task-2", + user={"id": 1, "is_admin": False}, + job_service=job_service, + ) + assert exc.value.status_code == 403 + + @pytest.mark.asyncio async def test_check_user_file_quota_reads_pending_count_through_job_service(): job_service = FakeJobService(pending_count=2) diff --git a/tests/unit/services/orchestrators/test_auth_service.py b/tests/unit/services/orchestrators/test_auth_service.py index 4b79a6ba4..27d4d0e85 100644 --- a/tests/unit/services/orchestrators/test_auth_service.py +++ b/tests/unit/services/orchestrators/test_auth_service.py @@ -585,6 +585,29 @@ def test_check_partition_access_super_admin_bypass(): ) +def test_authorize_task_access_owner_admin_and_deny(): + # Owner is allowed. + assert AuthService.authorize( + user={"id": 7, "is_admin": False}, action="task:access", resource={"user_id": 7} + ) + # Admin may access any task. + assert AuthService.authorize( + user={"id": 1, "is_admin": True}, action="task:access", resource={"user_id": 2} + ) + # Non-owner non-admin is denied (deny-by-default). + assert not AuthService.authorize( + user={"id": 1, "is_admin": False}, action="task:access", resource={"user_id": 2} + ) + # Missing resource denies rather than crashes. + assert not AuthService.authorize(user={"id": 1, "is_admin": False}, action="task:access") + + +def test_authorize_unknown_action_raises(): + # An unknown action is a programming error — denied loudly, never silently allowed. + with pytest.raises(ValueError, match="Unknown authorization action"): + AuthService.authorize(user={"id": 1, "is_admin": True}, action="bogus:action") + + def test_validate_file_quota(): # Admin bypass. AuthService.validate_file_quota({"is_admin": True}, pending_task_count=99, default_quota=1) From 1d01cec53e1287419bb7854d5e27d2a02dc614a7 Mon Sep 17 00:00:00 2001 From: andyne13 Date: Mon, 29 Jun 2026 10:38:15 +0200 Subject: [PATCH 2/3] fix(auth): reach AuthService.authorize via DI; ruff format 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. --- openrag/api/dependencies/auth.py | 13 +++++++------ tests/unit/api/dependencies/test_auth.py | 4 ++++ .../services/orchestrators/test_auth_service.py | 12 +++--------- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/openrag/api/dependencies/auth.py b/openrag/api/dependencies/auth.py index 946a1b743..a0ab1ca2d 100644 --- a/openrag/api/dependencies/auth.py +++ b/openrag/api/dependencies/auth.py @@ -5,7 +5,6 @@ from core.utils.logging import get_logger from di.providers import get_auth_service, get_config, get_job_service, get_partition_service from fastapi import Depends, HTTPException, Request, status -from services.orchestrators.auth_service import AuthService logger = get_logger() @@ -193,6 +192,7 @@ async def require_task_owner( task_id=Depends(request_task_id), user=Depends(current_user), job_service=Depends(get_job_service), + auth_service=Depends(get_auth_service), ): task_details = await job_service.get_task_details(task_id) if not task_details: @@ -200,11 +200,12 @@ async def require_task_owner( status_code=status.HTTP_404_NOT_FOUND, detail=f"Task '{task_id}' not found", ) - # Delegate the decision to the central PDP (AuthService.authorize) rather than - # inlining policy here — admins may access any task (parity with the admin jobs - # list), owners their own. Without this, an admin opening another user's job in - # the admin UI hits a 403 dead-end. - if not AuthService.authorize(user=user, action="task:access", resource=task_details): + # Delegate the decision to the central PDP (AuthService.authorize, reached via DI + # — the api layer must not import services directly) rather than inlining policy + # here — admins may access any task (parity with the admin jobs list), owners + # their own. Without this, an admin opening another user's job in the admin UI + # hits a 403 dead-end. + if not auth_service.authorize(user=user, action="task:access", resource=task_details): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="You do not have permission to access this task", diff --git a/tests/unit/api/dependencies/test_auth.py b/tests/unit/api/dependencies/test_auth.py index 236e44f6b..58fba6719 100644 --- a/tests/unit/api/dependencies/test_auth.py +++ b/tests/unit/api/dependencies/test_auth.py @@ -9,6 +9,7 @@ ) from core.utils.exceptions import AuthError from fastapi import HTTPException +from services.orchestrators.auth_service import AuthService class FakeAuthService: @@ -194,6 +195,7 @@ async def test_require_task_owner_reads_task_details_through_job_service(): task_id="task-1", user={"id": 7}, job_service=job_service, + auth_service=AuthService, ) assert details == {"user_id": 7, "filename": "a.pdf"} @@ -209,6 +211,7 @@ async def test_require_task_owner_allows_admin_for_another_users_task(): task_id="task-2", user={"id": 1, "is_admin": True}, job_service=job_service, + auth_service=AuthService, ) assert details == {"user_id": 2, "filename": "b.pdf"} @@ -223,6 +226,7 @@ async def test_require_task_owner_rejects_non_owner_non_admin(): task_id="task-2", user={"id": 1, "is_admin": False}, job_service=job_service, + auth_service=AuthService, ) assert exc.value.status_code == 403 diff --git a/tests/unit/services/orchestrators/test_auth_service.py b/tests/unit/services/orchestrators/test_auth_service.py index 27d4d0e85..0c5ca4a89 100644 --- a/tests/unit/services/orchestrators/test_auth_service.py +++ b/tests/unit/services/orchestrators/test_auth_service.py @@ -587,17 +587,11 @@ def test_check_partition_access_super_admin_bypass(): def test_authorize_task_access_owner_admin_and_deny(): # Owner is allowed. - assert AuthService.authorize( - user={"id": 7, "is_admin": False}, action="task:access", resource={"user_id": 7} - ) + assert AuthService.authorize(user={"id": 7, "is_admin": False}, action="task:access", resource={"user_id": 7}) # Admin may access any task. - assert AuthService.authorize( - user={"id": 1, "is_admin": True}, action="task:access", resource={"user_id": 2} - ) + assert AuthService.authorize(user={"id": 1, "is_admin": True}, action="task:access", resource={"user_id": 2}) # Non-owner non-admin is denied (deny-by-default). - assert not AuthService.authorize( - user={"id": 1, "is_admin": False}, action="task:access", resource={"user_id": 2} - ) + assert not AuthService.authorize(user={"id": 1, "is_admin": False}, action="task:access", resource={"user_id": 2}) # Missing resource denies rather than crashes. assert not AuthService.authorize(user={"id": 1, "is_admin": False}, action="task:access") From 234098d0664207bf7a620365e03a9f7588027259 Mon Sep 17 00:00:00 2001 From: andyne13 Date: Mon, 29 Jun 2026 10:47:01 +0200 Subject: [PATCH 3/3] fix(auth): require both identities in task:access authorize (deny-by-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. --- openrag/services/orchestrators/auth_service.py | 8 ++++++-- tests/unit/services/orchestrators/test_auth_service.py | 8 ++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/openrag/services/orchestrators/auth_service.py b/openrag/services/orchestrators/auth_service.py index 39cb0293e..6fca873d6 100644 --- a/openrag/services/orchestrators/auth_service.py +++ b/openrag/services/orchestrators/auth_service.py @@ -550,8 +550,12 @@ def authorize(cls, *, user: Any, action: str, resource: dict[str, Any] | None = # which lists every user's tasks. Otherwise only the task's owner. if cls._uget(user, "is_admin", False): return True - resource = resource or {} - return resource.get("user_id") == cls._uget(user, "id", None) + # Deny-by-default: both identities must be present before comparing, + # so a missing/partially-hydrated principal or a malformed task + # (user_id=None) can never match by two Nones. + owner_id = (resource or {}).get("user_id") + user_id = cls._uget(user, "id", None) + return owner_id is not None and user_id is not None and owner_id == user_id case _: raise ValueError(f"Unknown authorization action: {action!r}") diff --git a/tests/unit/services/orchestrators/test_auth_service.py b/tests/unit/services/orchestrators/test_auth_service.py index 0c5ca4a89..daac97bdb 100644 --- a/tests/unit/services/orchestrators/test_auth_service.py +++ b/tests/unit/services/orchestrators/test_auth_service.py @@ -596,6 +596,14 @@ def test_authorize_task_access_owner_admin_and_deny(): assert not AuthService.authorize(user={"id": 1, "is_admin": False}, action="task:access") +def test_authorize_task_access_denies_missing_identities(): + # Deny-by-default: two missing ids must NOT match (no None == None bypass). + assert not AuthService.authorize(user=None, action="task:access", resource={"user_id": None}) + assert not AuthService.authorize(user={"id": None}, action="task:access", resource={"user_id": None}) + # A real user vs a malformed task with no owner is denied. + assert not AuthService.authorize(user={"id": 5}, action="task:access", resource={"user_id": None}) + + def test_authorize_unknown_action_raises(): # An unknown action is a programming error — denied loudly, never silently allowed. with pytest.raises(ValueError, match="Unknown authorization action"):