diff --git a/.env-template b/.env-template index eb9e5828..afff3835 100644 --- a/.env-template +++ b/.env-template @@ -54,9 +54,17 @@ MICROSOFT_AUTHORITY=https://{tenantId}.ciamlogin.com/{tenantId} # OIDC_PROVIDER_NAME= # OIDC_ALLOWED_GROUPS= # OIDC_GROUPS_CLAIM=groups +# OIDC_ADMIN_GROUPS= # Add offline_access to OIDC_SCOPES for silent session renewal on IdPs that # require it for refresh tokens (Authentik does; Keycloak does not). +# RBAC (admin/user roles). Persisted admin grants live in the user_roles table +# and apply only under AUTH_TYPE=oidc — manage them with scripts/grant_admin.py +# (bootstrap the first admin) or OIDC_ADMIN_GROUPS above. LOCAL_MODE_ADMIN is the +# ONLY non-DB admin path; it applies solely to AUTH_TYPE=None (no-auth self-host) +# and MUST stay false in any networked deployment. +# LOCAL_MODE_ADMIN=false + # SCIM 2.0 provisioning (IdP-driven user create/deactivate at /scim/v2; # pair with OIDC_USER_ID_CLAIM=email so SCIM userName matches the OIDC user id) # SCIM_ENABLED=false diff --git a/application/alembic/versions/0020_user_roles.py b/application/alembic/versions/0020_user_roles.py new file mode 100644 index 00000000..428291fe --- /dev/null +++ b/application/alembic/versions/0020_user_roles.py @@ -0,0 +1,50 @@ +"""0020 user roles — admin/user RBAC grant table. + +``user_roles`` records elevated role grants only; the ``user`` role is implicit +(every authenticated principal has it without a row). A grant is keyed by +``(user_id, role, source)`` so a manual grant and an OIDC-group-derived grant +for the same user coexist and revoke independently. ``user_id`` is the auth +``sub`` (the TEXT business key used everywhere else, not ``users.id``); no FK or +trigger, mirroring ``auth_events`` — grants may legitimately precede user +provisioning, and we don't want ``ON DELETE RESTRICT`` blocking user deletion. + +The ``CHECK (role IN ('admin'))`` is the role catalog — there is no separate +``roles`` table. Widen the check when new roles are introduced. The PK indexes +``user_id`` as its leading column, so ``WHERE user_id = :sub`` lookups are a +single index probe with no extra index. + +Revision ID: 0020_user_roles +Revises: 0019_agent_slug +""" + +from typing import Sequence, Union + +from alembic import op + + +revision: str = "0020_user_roles" +down_revision: Union[str, None] = "0019_agent_slug" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # IF NOT EXISTS keeps the migration idempotent across partial/out-of-band + # schema states, matching the convention used by the surrounding migrations. + op.execute( + """ + CREATE TABLE IF NOT EXISTS user_roles ( + user_id TEXT NOT NULL, + role TEXT NOT NULL CHECK (role IN ('admin')), + source TEXT NOT NULL DEFAULT 'manual' + CHECK (source IN ('manual', 'oidc_group')), + granted_by TEXT, + granted_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (user_id, role, source) + ); + """ + ) + + +def downgrade() -> None: + op.execute("DROP TABLE IF EXISTS user_roles;") diff --git a/application/api/admin/__init__.py b/application/api/admin/__init__.py new file mode 100644 index 00000000..8fa8b444 --- /dev/null +++ b/application/api/admin/__init__.py @@ -0,0 +1,3 @@ +from .routes import admin_ns + +__all__ = ["admin_ns"] diff --git a/application/api/admin/routes.py b/application/api/admin/routes.py new file mode 100644 index 00000000..dca86fa6 --- /dev/null +++ b/application/api/admin/routes.py @@ -0,0 +1,65 @@ +"""Admin-gated management endpoints (RBAC ``admin`` role required). + +Every resource here is behind ``@admin_required``. The frontend route guard is +cosmetic — this server-side decorator is the actual security boundary, so any +new endpoint added to this namespace MUST carry it. +""" + +from __future__ import annotations + +import logging + +from flask import jsonify, make_response, request +from flask_restx import Namespace, Resource + +from application.api.user.authz import admin_required +from application.storage.db.repositories.users import UsersRepository +from application.storage.db.session import db_readonly + +logger = logging.getLogger(__name__) + +admin_ns = Namespace("admin", description="Admin-only management endpoints", path="/api") + +_DEFAULT_PAGE_SIZE = 25 +_MAX_PAGE_SIZE = 100 + + +def _int_arg(name: str, default: int) -> int: + try: + return int(request.args.get(name, default)) + except (TypeError, ValueError): + return default + + +@admin_ns.route("/admin/users") +class AdminUsersResource(Resource): + @admin_required + def get(self): + """List users, paginated. Optional exact ``user_id`` filter.""" + page = max(1, _int_arg("page", 1)) + page_size = max(1, min(_MAX_PAGE_SIZE, _int_arg("page_size", _DEFAULT_PAGE_SIZE))) + offset = (page - 1) * page_size + user_id_filter = request.args.get("user_id") or None + with db_readonly() as conn: + total, rows = UsersRepository(conn).list_paginated(user_id_filter, offset, page_size) + users = [ + { + "user_id": row.get("user_id"), + "active": bool(row.get("active", True)), + "created_at": row.get("created_at"), + } + for row in rows + ] + return make_response( + jsonify( + { + "success": True, + "users": users, + "page": page, + "page_size": page_size, + "total": total, + "has_more": offset + len(rows) < total, + } + ), + 200, + ) diff --git a/application/api/oidc/routes.py b/application/api/oidc/routes.py index 5bf8e4fe..dede3801 100644 --- a/application/api/oidc/routes.py +++ b/application/api/oidc/routes.py @@ -27,6 +27,7 @@ from application.auth import handle_auth from application.cache import get_redis_instance from application.core.settings import settings from application.storage.db.repositories.auth_events import AuthEventsRepository +from application.storage.db.repositories.user_roles import UserRolesRepository from application.storage.db.repositories.users import UsersRepository from application.storage.db.session import db_readonly, db_session @@ -95,6 +96,44 @@ def _allowed_groups() -> list[str]: return [group.strip() for group in raw.split(",") if group.strip()] +def _admin_groups() -> list[str]: + """Parse the comma-separated admin-group list; empty/unset disables OIDC admin mapping.""" + raw = settings.OIDC_ADMIN_GROUPS or "" + return [group.strip() for group in raw.split(",") if group.strip()] + + +def _reconcile_oidc_admin(user_id: str, groups: list[str] | None) -> None: + """Sync the OIDC-group-derived admin grant for ``user_id``; best-effort, never raises. + + Empty/unset ``OIDC_ADMIN_GROUPS`` disables the mapping entirely (no-op), so a + blank env var never mass-revokes. ``groups`` is ``None`` when the IdP asserted + no groups claim at all (absent from both id_token and userinfo); admin-ness is + then unknowable, so we skip rather than revoke — otherwise a silent refresh + whose token omits groups would demote a still-eligible admin. Only the + ``oidc_group`` grant source is touched — manual grants are left intact. A + change is audited. + """ + admin_groups = _admin_groups() + if not admin_groups: + return + if groups is None: + return + is_admin = bool(set(groups) & set(admin_groups)) + try: + with db_session() as conn: + change = UserRolesRepository(conn).reconcile_oidc_admin(user_id, is_admin) + if change: + AuthEventsRepository(conn).insert( + user_id, + "role_granted" if change == "granted" else "role_revoked", + ip=request.remote_addr, + user_agent=request.headers.get("User-Agent"), + metadata={"role": "admin", "source": "oidc_group"}, + ) + except Exception: + logger.error("OIDC admin reconcile failed for %s", user_id, exc_info=True) + + def _claim_groups(claims: dict) -> list[str]: """Read the groups claim as a list of strings; missing means no groups.""" value = claims.get(settings.OIDC_GROUPS_CLAIM) @@ -111,7 +150,12 @@ def _effective_claims(tokens: dict, claims: dict) -> dict: """Merge userinfo into the id_token claims when required claims are missing.""" effective = dict(claims) need_user_id = not effective.get(settings.OIDC_USER_ID_CLAIM) - need_groups = bool(_allowed_groups()) and settings.OIDC_GROUPS_CLAIM not in effective + # Fetch userinfo to backfill groups when EITHER a login allowlist or an admin + # mapping is configured — many IdPs (Okta/Auth0/Azure AD) only expose groups + # via userinfo, and without this the admin reconcile would see no groups. + need_groups = ( + bool(_allowed_groups()) or bool(_admin_groups()) + ) and settings.OIDC_GROUPS_CLAIM not in effective if not (need_user_id or need_groups) or not tokens.get("access_token"): return effective try: @@ -300,6 +344,12 @@ def oidc_callback(): if not _gate_and_audit_login(user_id, effective, groups): return _frontend_redirect("oidc_error=account_disabled") + # Reconcile OIDC-group-derived admin against the fresh group claims so + # adds/removals take effect at login (manual grants are untouched). Pass + # None when the claim is absent entirely so a missing claim never revokes. + groups_present = settings.OIDC_GROUPS_CLAIM in effective + _reconcile_oidc_admin(user_id, groups if groups_present else None) + # No need to lift prior revocations here: the denylist keys on a revocation # timestamp and the session minted below carries a newer ``iat``, so it is # allowed automatically while still-live sessions revoked on other devices @@ -449,6 +499,11 @@ def oidc_refresh(): identity["oidc_sub"] = effective.get("sub") or identity["oidc_sub"] if effective.get("sid"): identity["oidc_sid"] = effective["sid"] + # Re-reconcile admin on every renewal carrying fresh group claims, so an + # OIDC group add/remove takes effect without waiting for a full re-login. + # None when the claim is absent so a missing claim never revokes admin. + groups_present = settings.OIDC_GROUPS_CLAIM in effective + _reconcile_oidc_admin(identity["sub"], groups if groups_present else None) # Re-check revocation right before minting, against the (possibly remapped) # identity but with the ORIGINAL session's iat: a back-channel logout or diff --git a/application/api/user/authz.py b/application/api/user/authz.py new file mode 100644 index 00000000..e0404c2d --- /dev/null +++ b/application/api/user/authz.py @@ -0,0 +1,97 @@ +"""Role resolution and authorization helpers (admin/user RBAC). + +Roles are resolved per-request from the database plus computed overlays and are +NEVER read from the inbound JWT: ``simple_jwt``/``session_jwt`` tokens are +self-mintable, so trusting a ``roles`` claim would be a trivial privilege +escalation. ``resolve_roles`` always rebuilds the set from scratch. + +Policy (see ``rbac-spec.md``): +- Persisted RBAC (``user_roles``) applies only under ``AUTH_TYPE=oidc``. +- ``AUTH_TYPE=None`` (no-auth self-host) grants admin only when + ``LOCAL_MODE_ADMIN`` is enabled (default off). +- ``simple_jwt`` / ``session_jwt`` can never be admin (shared/throwaway ``sub``). +- ``/v1`` agent keys, device tokens, and pre-auth requests are role-less. +""" + +from __future__ import annotations + +import logging +from functools import wraps + +from flask import jsonify, make_response, request + +from application.core.settings import settings +from application.storage.db.repositories.user_roles import UserRolesRepository +from application.storage.db.session import db_readonly + +logger = logging.getLogger(__name__) + +ROLE_USER = "user" +ROLE_ADMIN = "admin" + + +def resolve_roles(token: dict | None) -> list[str]: + """Compute the sorted role set for a request principal. + + Fails open to less privilege: a DB error while reading grants demotes + DB-backed admins to ``user`` rather than raising (which would turn a roles + outage into a total-auth outage on the universal before-request path). + """ + if not token: + return [ROLE_USER] + roles = {ROLE_USER} + sub = token.get("sub") + if settings.AUTH_TYPE == "oidc": + if sub: + try: + with db_readonly() as conn: + roles.update(UserRolesRepository(conn).role_names_for(sub)) + except Exception: + logger.error( + "resolve_roles: user_roles read failed for sub=%s", sub, exc_info=True + ) + elif settings.AUTH_TYPE is None and settings.LOCAL_MODE_ADMIN: + roles.add(ROLE_ADMIN) + # simple_jwt / session_jwt: never admin. + return sorted(roles) + + +def has_role(token: dict | None, name: str) -> bool: + """True if the principal holds ``name``. + + ``user`` is implicit and always true. Tolerates a ``None`` token or a token + that never went through ``resolve_roles`` (e.g. async/SSE or /v1 paths) — + those are treated as role-less and only satisfy the implicit ``user`` role. + """ + if name == ROLE_USER: + return True + roles = (token or {}).get("roles") or [] + return name in roles + + +def require_role(name: str): + """Decorator factory: 401 when unauthenticated, 403 when lacking ``name``. + + Fails closed — a missing/None token or absent ``roles`` key never raises and + never passes through. The frontend route guard is cosmetic; this is the + security boundary. + """ + + def decorator(func): + @wraps(func) + def wrapper(*args, **kwargs): + token = getattr(request, "decoded_token", None) + if not token: + return make_response( + jsonify({"success": False, "message": "Authentication required"}), 401 + ) + if not has_role(token, name): + return make_response(jsonify({"success": False, "message": "Forbidden"}), 403) + return func(*args, **kwargs) + + return wrapper + + return decorator + + +admin_required = require_role(ROLE_ADMIN) diff --git a/application/api/user/me/__init__.py b/application/api/user/me/__init__.py new file mode 100644 index 00000000..5d2cf791 --- /dev/null +++ b/application/api/user/me/__init__.py @@ -0,0 +1,3 @@ +from .routes import me_ns + +__all__ = ["me_ns"] diff --git a/application/api/user/me/routes.py b/application/api/user/me/routes.py new file mode 100644 index 00000000..34e108ad --- /dev/null +++ b/application/api/user/me/routes.py @@ -0,0 +1,34 @@ +"""Current-principal endpoint. + +``GET /api/user/me`` returns the caller's user id and resolved roles, sourced +only from ``request.decoded_token`` (already populated and role-resolved by the +auth chokepoint in ``app.py``). Auth-mode-agnostic. ``email``/``name``/ +``picture`` are OIDC-only and optional — they are echoed from the token and are +never present for ``simple_jwt``/``session_jwt``/no-auth modes. +""" + +from __future__ import annotations + +from flask import jsonify, make_response, request +from flask_restx import Namespace, Resource + +me_ns = Namespace("me", description="Current user identity and roles", path="/api") + + +@me_ns.route("/user/me") +class MeResource(Resource): + def get(self): + """Return ``{user_id, roles, email?, name?, picture?}`` for the caller.""" + decoded_token = getattr(request, "decoded_token", None) + if not decoded_token: + return make_response(jsonify({"success": False}), 401) + body = { + "success": True, + "user_id": decoded_token.get("sub"), + "roles": decoded_token.get("roles") or ["user"], + } + for field in ("email", "name", "picture"): + value = decoded_token.get(field) + if value: + body[field] = value + return make_response(jsonify(body), 200) diff --git a/application/api/user/routes.py b/application/api/user/routes.py index 53f0daad..aa792c93 100644 --- a/application/api/user/routes.py +++ b/application/api/user/routes.py @@ -15,6 +15,7 @@ from .agents import ( from .analytics import analytics_ns from .attachments import attachments_ns from .conversations import conversations_ns +from .me import me_ns from .models import models_ns from .prompts import prompts_ns from .schedules import schedules_ns @@ -35,6 +36,9 @@ api.add_namespace(attachments_ns) # Conversations api.add_namespace(conversations_ns) +# Current user (identity + roles) +api.add_namespace(me_ns) + # Models api.add_namespace(models_ns) diff --git a/application/app.py b/application/app.py index feec15a3..616b12c3 100644 --- a/application/app.py +++ b/application/app.py @@ -15,6 +15,7 @@ from application.core.logging_config import setup_logging setup_logging() from application.api import api # noqa: E402 +from application.api.admin import admin_ns # noqa: E402 from application.api.answer import answer # noqa: E402 from application.api.devices import devices_bp # noqa: E402 from application.api.events.routes import events # noqa: E402 @@ -22,6 +23,7 @@ from application.api.internal.routes import internal # noqa: E402 from application.api.oidc import oidc_bp # noqa: E402 from application.api.oidc.denylist import is_denied as oidc_session_denied # noqa: E402 from application.api.scim import scim_bp # noqa: E402 +from application.api.user.authz import resolve_roles # noqa: E402 from application.api.user.routes import user # noqa: E402 from application.api.connector.routes import connector # noqa: E402 from application.api.v1 import v1_bp # noqa: E402 @@ -67,6 +69,7 @@ app.register_blueprint(devices_bp) app.register_blueprint(oidc_bp) app.register_blueprint(scim_bp) app.register_blueprint(v1_bp) +api.add_namespace(admin_ns) app.config.update( UPLOAD_FOLDER="inputs", CELERY_BROKER_URL=settings.CELERY_BROKER_URL, @@ -242,6 +245,11 @@ def authenticate_request(): 401, ) else: + # Resolve roles once here, the single authenticated chokepoint. Roles + # are computed (never read from the JWT) and overwrite any inbound + # 'roles' claim. /v1, device, oidc, and scim paths set decoded_token + # above and never reach here, so they stay role-less by design. + decoded_token["roles"] = resolve_roles(decoded_token) request.decoded_token = decoded_token diff --git a/application/core/settings.py b/application/core/settings.py index 2afa1615..a4cf30c6 100644 --- a/application/core/settings.py +++ b/application/core/settings.py @@ -31,6 +31,13 @@ class Settings(BaseSettings): OIDC_PROVIDER_NAME: Optional[str] = None # sign-in button label, e.g. "Acme SSO" OIDC_ALLOWED_GROUPS: Optional[str] = None # comma-separated allowlist; unset = any authenticated user OIDC_GROUPS_CLAIM: str = "groups" # ID-token/userinfo claim carrying group membership + OIDC_ADMIN_GROUPS: Optional[str] = None # comma-separated groups granted admin; unset = no OIDC admin mapping + + # RBAC (admin/user roles). Persisted admin grants live in the user_roles + # table and apply only under AUTH_TYPE=oidc. LOCAL_MODE_ADMIN is the only + # non-DB admin path and applies only to AUTH_TYPE=None (no-auth self-host). + # It MUST stay False in any networked deployment. + LOCAL_MODE_ADMIN: bool = False # SCIM 2.0 provisioning (IdP-driven user create/deactivate at /scim/v2) SCIM_ENABLED: bool = False diff --git a/application/storage/db/models.py b/application/storage/db/models.py index 5061c2d8..83da164d 100644 --- a/application/storage/db/models.py +++ b/application/storage/db/models.py @@ -19,6 +19,7 @@ from sqlalchemy import ( BigInteger, Boolean, CHAR, + CheckConstraint, Column, DateTime, ForeignKey, @@ -26,6 +27,7 @@ from sqlalchemy import ( Index, Integer, MetaData, + PrimaryKeyConstraint, UniqueConstraint, Table, Text, @@ -67,6 +69,23 @@ auth_events_table = Table( Column("created_at", DateTime(timezone=True), nullable=False, server_default=func.now()), ) +# Elevated RBAC role grants. The ``user`` role is implicit (no row); only +# admin grants are stored. ``(user_id, role, source)`` is the key so manual and +# oidc_group grants coexist and revoke independently. ``user_id`` is the auth +# ``sub`` (no FK/trigger, mirroring ``auth_events``). See migration 0020. +user_roles_table = Table( + "user_roles", + metadata, + Column("user_id", Text, nullable=False), + Column("role", Text, nullable=False), + Column("source", Text, nullable=False, server_default="manual"), + Column("granted_by", Text), + Column("granted_at", DateTime(timezone=True), nullable=False, server_default=func.now()), + PrimaryKeyConstraint("user_id", "role", "source"), + CheckConstraint("role IN ('admin')", name="user_roles_role_check"), + CheckConstraint("source IN ('manual', 'oidc_group')", name="user_roles_source_check"), +) + prompts_table = Table( "prompts", metadata, diff --git a/application/storage/db/repositories/user_roles.py b/application/storage/db/repositories/user_roles.py new file mode 100644 index 00000000..a61b4eb3 --- /dev/null +++ b/application/storage/db/repositories/user_roles.py @@ -0,0 +1,115 @@ +"""Repository for the ``user_roles`` RBAC grant table. + +The table stores only elevated grants; the ``user`` role is implicit. Grants +are keyed by ``(user_id, role, source)`` so a manual grant and an +OIDC-group-derived grant for the same user coexist and revoke independently. +All methods take a ``Connection`` and do not manage their own transactions. +""" + +from __future__ import annotations + +from typing import Optional + +from sqlalchemy import Connection, text + +from application.storage.db.base_repository import row_to_dict + +VALID_SOURCES = ("manual", "oidc_group") + + +class UserRolesRepository: + """Persisted role grants keyed by the auth ``sub`` (``user_id``).""" + + def __init__(self, conn: Connection) -> None: + self._conn = conn + + # ------------------------------------------------------------------ + # Reads + # ------------------------------------------------------------------ + def role_names_for(self, user_id: str) -> list[str]: + """Return the distinct elevated roles granted to ``user_id`` (any source).""" + if not user_id: + return [] + result = self._conn.execute( + text("SELECT DISTINCT role FROM user_roles WHERE user_id = :user_id"), + {"user_id": user_id}, + ) + return [row[0] for row in result.fetchall()] + + def list_admins(self) -> list[dict]: + """Return one row per user holding ``admin``, with earliest grant + sources.""" + result = self._conn.execute( + text( + """ + SELECT user_id, + min(granted_at) AS granted_at, + array_agg(DISTINCT source ORDER BY source) AS sources + FROM user_roles + WHERE role = 'admin' + GROUP BY user_id + ORDER BY min(granted_at) + """ + ) + ) + return [ + {"user_id": row[0], "granted_at": row[1], "sources": list(row[2])} + for row in result.fetchall() + ] + + def list_for(self, user_id: str) -> list[dict]: + """Return all grant rows for ``user_id`` (for audit/inspection).""" + result = self._conn.execute( + text("SELECT * FROM user_roles WHERE user_id = :user_id ORDER BY granted_at"), + {"user_id": user_id}, + ) + return [row_to_dict(row) for row in result.fetchall()] + + # ------------------------------------------------------------------ + # Writes + # ------------------------------------------------------------------ + def grant( + self, + user_id: str, + role: str = "admin", + source: str = "manual", + granted_by: Optional[str] = None, + ) -> bool: + """Idempotently grant ``role`` to ``user_id`` from ``source``. + + Returns True when a new row was inserted, False when it already existed. + """ + result = self._conn.execute( + text( + """ + INSERT INTO user_roles (user_id, role, source, granted_by) + VALUES (:user_id, :role, :source, :granted_by) + ON CONFLICT (user_id, role, source) DO NOTHING + """ + ), + {"user_id": user_id, "role": role, "source": source, "granted_by": granted_by}, + ) + return bool(result.rowcount) + + def revoke(self, user_id: str, role: str = "admin", source: str = "manual") -> bool: + """Revoke ``role`` from ``user_id`` for ``source``. Returns True if removed.""" + result = self._conn.execute( + text( + """ + DELETE FROM user_roles + WHERE user_id = :user_id AND role = :role AND source = :source + """ + ), + {"user_id": user_id, "role": role, "source": source}, + ) + return bool(result.rowcount) + + def reconcile_oidc_admin(self, user_id: str, is_admin: bool) -> Optional[str]: + """Sync the ``oidc_group`` admin grant for ``user_id``; never touches manual grants. + + Returns ``"granted"`` / ``"revoked"`` when a change occurred, else ``None``. + """ + if is_admin: + granted = self.grant(user_id, "admin", source="oidc_group", granted_by="oidc") + return "granted" if granted else None + revoked = self.revoke(user_id, "admin", source="oidc_group") + return "revoked" if revoked else None diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index c4028d9c..e3ee6542 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -4,10 +4,12 @@ import { useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Outlet, Route, Routes, useLocation } from 'react-router-dom'; +import Admin from './admin'; import Agents from './agents'; import SharedAgentGate from './agents/SharedAgentGate'; import DocsGPT3 from './assets/cute_docsgpt3.svg'; import ActionButtons from './components/ActionButtons'; +import AdminRoute from './components/AdminRoute'; import Spinner from './components/Spinner'; import { Button } from './components/ui/button'; import UploadToast from './components/UploadToast'; @@ -140,6 +142,14 @@ export default function App() { /> } /> } /> + + + + } + /> } /> } /> diff --git a/frontend/src/Navigation.tsx b/frontend/src/Navigation.tsx index a31f7090..ae8a0a10 100644 --- a/frontend/src/Navigation.tsx +++ b/frontend/src/Navigation.tsx @@ -10,6 +10,7 @@ import { Plus, Search as SearchIcon, Settings as SettingsIcon, + ShieldCheck as ShieldCheckIcon, } from 'lucide-react'; import { Agent } from './agents/types'; @@ -44,6 +45,7 @@ import { selectAgents, selectConversationId, selectConversations, + selectIsAdmin, selectModalStateDeleteConv, selectSelectedAgent, selectSharedAgents, @@ -72,6 +74,7 @@ export default function Navigation({ navOpen, setNavOpen }: NavigationProps) { const queries = useSelector(selectQueries); const conversations = useSelector(selectConversations); const conversationId = useSelector(selectConversationId); + const isAdmin = useSelector(selectIsAdmin); const modalStateDeleteConv = useSelector(selectModalStateDeleteConv); const agents = useSelector(selectAgents); const sharedAgents = useSelector(selectSharedAgents); @@ -624,6 +627,31 @@ export default function Navigation({ navOpen, setNavOpen }: NavigationProps) {
+ {isAdmin && ( + { + if (isMobile || isTablet) { + setNavOpen(false); + } + resetConversation(); + }} + to="/admin" + className={({ isActive }) => + `hover:bg-sidebar-accent mx-4 my-auto flex h-9 cursor-pointer items-center gap-2.5 rounded-3xl pl-3 ${ + isActive ? 'bg-sidebar-accent' : '' + }` + } + > + +

+ {t('admin.label', 'Admin')} +

+ + )} { if (isMobile || isTablet) { diff --git a/frontend/src/admin/index.tsx b/frontend/src/admin/index.tsx new file mode 100644 index 00000000..dcbe889a --- /dev/null +++ b/frontend/src/admin/index.tsx @@ -0,0 +1,17 @@ +/** + * Placeholder admin dashboard. Reached only via (cosmetic guard); + * the real dashboard is built out here in a later PR. Server-side + * @admin_required on /api/admin/* is the actual access boundary. + */ +export default function Admin() { + return ( +
+

+ Admin +

+

+ Admin dashboard coming soon. +

+
+ ); +} diff --git a/frontend/src/api/endpoints.ts b/frontend/src/api/endpoints.ts index ff694485..ca6b3b30 100644 --- a/frontend/src/api/endpoints.ts +++ b/frontend/src/api/endpoints.ts @@ -1,6 +1,7 @@ const endpoints = { USER: { CONFIG: '/api/config', + ME: '/api/user/me', NEW_TOKEN: '/api/generate_token', OIDC_LOGIN: '/api/auth/oidc/login', OIDC_TOKEN: '/api/auth/oidc/token', diff --git a/frontend/src/api/services/userService.ts b/frontend/src/api/services/userService.ts index 0887833d..7c58107c 100644 --- a/frontend/src/api/services/userService.ts +++ b/frontend/src/api/services/userService.ts @@ -5,6 +5,8 @@ import endpoints from '../endpoints'; const userService = { getConfig: (): Promise => throttledApiClient.get(endpoints.USER.CONFIG, null), + getMe: (token: string | null): Promise => + apiClient.get(endpoints.USER.ME, token), getNewToken: (): Promise => throttledApiClient.get(endpoints.USER.NEW_TOKEN, null), // Token deliberately null: a stale Authorization header must not be able diff --git a/frontend/src/components/AdminRoute.test.tsx b/frontend/src/components/AdminRoute.test.tsx new file mode 100644 index 00000000..d3650ab8 --- /dev/null +++ b/frontend/src/components/AdminRoute.test.tsx @@ -0,0 +1,36 @@ +import { configureStore } from '@reduxjs/toolkit'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { Provider } from 'react-redux'; +import { MemoryRouter } from 'react-router-dom'; +import { describe, expect, it } from 'vitest'; + +import preferenceReducer, { setRoles } from '../preferences/preferenceSlice'; +import AdminRoute from './AdminRoute'; + +function renderWith(roles: string[] | null) { + const store = configureStore({ reducer: { preference: preferenceReducer } }); + if (roles !== null) store.dispatch(setRoles(roles)); + return renderToStaticMarkup( + + + +
admin-secret
+ + + , + ); +} + +describe('AdminRoute', () => { + it('hides content until roles are resolved', () => { + expect(renderWith(null)).not.toContain('admin-secret'); + }); + + it('redirects (renders nothing) for non-admins', () => { + expect(renderWith(['user'])).not.toContain('admin-secret'); + }); + + it('renders children for admins', () => { + expect(renderWith(['admin', 'user'])).toContain('admin-secret'); + }); +}); diff --git a/frontend/src/components/AdminRoute.tsx b/frontend/src/components/AdminRoute.tsx new file mode 100644 index 00000000..90e9a5cc --- /dev/null +++ b/frontend/src/components/AdminRoute.tsx @@ -0,0 +1,35 @@ +import { useSelector } from 'react-redux'; +import { Navigate } from 'react-router-dom'; + +import { + selectIsAdmin, + selectRolesResolved, +} from '../preferences/preferenceSlice'; +import Spinner from './Spinner'; + +/** + * Cosmetic route guard — NOT a security boundary. The server enforces admin + * access via @admin_required on every /api/admin/* endpoint. This only avoids + * rendering admin chrome to non-admins. Renders a spinner until roles are + * resolved so a real admin is never bounced on first paint. + */ +export default function AdminRoute({ + children, +}: { + children: React.ReactNode; +}) { + const isAdmin = useSelector(selectIsAdmin); + const rolesResolved = useSelector(selectRolesResolved); + + if (!rolesResolved) { + return ( +
+ +
+ ); + } + if (!isAdmin) { + return ; + } + return <>{children}; +} diff --git a/frontend/src/conversation/conversationListener.test.ts b/frontend/src/conversation/conversationListener.test.ts index 202192a6..e8e5c30e 100644 --- a/frontend/src/conversation/conversationListener.test.ts +++ b/frontend/src/conversation/conversationListener.test.ts @@ -79,6 +79,8 @@ const makeStore = ( availableModels: [], modelsLoading: false, agentFolders: null, + roles: [], + rolesResolved: false, }; const conversation: ConversationState = { queries: [], diff --git a/frontend/src/hooks/fetchMeRoles.test.ts b/frontend/src/hooks/fetchMeRoles.test.ts new file mode 100644 index 00000000..01385ad6 --- /dev/null +++ b/frontend/src/hooks/fetchMeRoles.test.ts @@ -0,0 +1,55 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import userService from '../api/services/userService'; +import { fetchMeRoles } from './useTokenAuth'; + +afterEach(() => vi.restoreAllMocks()); + +const okResponse = (roles: unknown) => + ({ ok: true, json: async () => ({ roles }) }) as unknown as Response; + +describe('fetchMeRoles', () => { + // Each test uses a distinct token so the module-level cache doesn't collide. + + it('dedupes concurrent calls for the same token', async () => { + const spy = vi + .spyOn(userService, 'getMe') + .mockResolvedValue(okResponse(['user', 'admin'])); + const [a, b] = await Promise.all([ + fetchMeRoles('tok-dedupe'), + fetchMeRoles('tok-dedupe'), + ]); + expect(spy).toHaveBeenCalledTimes(1); + expect(a).toEqual(['user', 'admin']); + expect(b).toEqual(['user', 'admin']); + }); + + it('caches a successful result for the same token', async () => { + const spy = vi + .spyOn(userService, 'getMe') + .mockResolvedValue(okResponse(['user'])); + expect(await fetchMeRoles('tok-cache')).toEqual(['user']); + expect(await fetchMeRoles('tok-cache')).toEqual(['user']); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('returns null on failure and retries on the next call', async () => { + const spy = vi + .spyOn(userService, 'getMe') + .mockResolvedValueOnce({ ok: false } as unknown as Response) + .mockResolvedValueOnce(okResponse(['user'])); + expect(await fetchMeRoles('tok-retry')).toBeNull(); + expect(await fetchMeRoles('tok-retry')).toEqual(['user']); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it('returns null when getMe throws', async () => { + vi.spyOn(userService, 'getMe').mockRejectedValue(new Error('network')); + expect(await fetchMeRoles('tok-throw')).toBeNull(); + }); + + it('defaults to [] when a successful response omits roles', async () => { + vi.spyOn(userService, 'getMe').mockResolvedValue(okResponse(undefined)); + expect(await fetchMeRoles('tok-noroles')).toEqual([]); + }); +}); diff --git a/frontend/src/hooks/useTokenAuth.ts b/frontend/src/hooks/useTokenAuth.ts index d78d9edd..b592e71f 100644 --- a/frontend/src/hooks/useTokenAuth.ts +++ b/frontend/src/hooks/useTokenAuth.ts @@ -4,7 +4,12 @@ import { useDispatch, useSelector } from 'react-redux'; import { baseURL } from '../api/client'; import endpoints from '../api/endpoints'; import userService from '../api/services/userService'; -import { selectToken, setToken } from '../preferences/preferenceSlice'; +import { + clearRoles, + selectToken, + setRoles, + setToken, +} from '../preferences/preferenceSlice'; import { decodeJwtPayload, getJwtRemainingMs, @@ -40,6 +45,37 @@ let oidcRenewalPromise: Promise | null = null; // which re-authenticates silently while the IdP session is still alive. let oidcRenewalUnavailable = false; +// Dedupe the /api/user/me fetch across the two hook instances (AuthWrapper + +// Navigation) and StrictMode's double-invoked effects, keyed by the active +// token. A failed fetch clears the cache so a later run can retry; a successful +// one stays cached for that token. +let meFetchKey: string | null = null; +let meFetchPromise: Promise | null = null; + +export function fetchMeRoles(token: string | null): Promise { + const key = token ?? '__anon__'; + if (meFetchKey === key && meFetchPromise) return meFetchPromise; + meFetchKey = key; + const promise = (async () => { + try { + const response = await userService.getMe(token); + if (!response.ok) return null; + const data = await response.json(); + return Array.isArray(data.roles) ? data.roles : []; + } catch { + return null; + } + })(); + meFetchPromise = promise; + promise.then((roles) => { + if (roles === null && meFetchPromise === promise) { + meFetchKey = null; + meFetchPromise = null; + } + }); + return promise; +} + const claimString = (value: unknown): string | undefined => typeof value === 'string' && value !== '' ? value : undefined; @@ -289,6 +325,30 @@ export default function useAuth() { }; }, [authType, token, dispatch]); + useEffect(() => { + // Resolve RBAC roles from the server once auth has settled. Roles are + // DB-authoritative and mode-agnostic — even no-auth ('local') can be admin + // via LOCAL_MODE_ADMIN — so /me is fetched whenever we're authenticated (or + // running token-less in no-auth mode), and cleared while a login is pending. + if (isAuthLoading) return; // auth still resolving — leave roles unresolved + if (oidcFailed || showTokenModal) { + dispatch(clearRoles()); + return; + } + // Token-required modes that don't have a token yet: stay unresolved. + if (authType && authType !== 'none' && !token) { + dispatch(clearRoles()); + return; + } + let cancelled = false; + fetchMeRoles(token).then((roles) => { + if (!cancelled) dispatch(setRoles(roles ?? [])); + }); + return () => { + cancelled = true; + }; + }, [authType, token, isAuthLoading, oidcFailed, showTokenModal, dispatch]); + const handleTokenSubmit = (enteredToken: string) => { localStorage.setItem('authToken', enteredToken); dispatch(setToken(enteredToken)); diff --git a/frontend/src/preferences/preferenceSlice.test.ts b/frontend/src/preferences/preferenceSlice.test.ts new file mode 100644 index 00000000..cf9480e5 --- /dev/null +++ b/frontend/src/preferences/preferenceSlice.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; + +import type { RootState } from '../store'; +import reducer, { + clearRoles, + selectIsAdmin, + selectRoles, + selectRolesResolved, + setRoles, +} from './preferenceSlice'; + +const baseState = () => reducer(undefined, { type: '@@INIT' }); + +describe('preference roles reducer', () => { + it('starts unresolved with no roles', () => { + const state = baseState(); + expect(state.roles).toEqual([]); + expect(state.rolesResolved).toBe(false); + }); + + it('setRoles stores roles and marks resolved', () => { + const state = reducer(baseState(), setRoles(['user', 'admin'])); + expect(state.roles).toEqual(['user', 'admin']); + expect(state.rolesResolved).toBe(true); + }); + + it('clearRoles resets roles and resolution', () => { + const granted = reducer(baseState(), setRoles(['admin'])); + const state = reducer(granted, clearRoles()); + expect(state.roles).toEqual([]); + expect(state.rolesResolved).toBe(false); + }); +}); + +const stateWith = (roles: string[], rolesResolved = true) => + ({ preference: { roles, rolesResolved } }) as unknown as RootState; + +describe('roles selectors', () => { + it('selectIsAdmin is true only when admin is present', () => { + expect(selectIsAdmin(stateWith(['admin', 'user']))).toBe(true); + expect(selectIsAdmin(stateWith(['user']))).toBe(false); + expect(selectIsAdmin(stateWith([]))).toBe(false); + }); + + it('selectRoles and selectRolesResolved read state', () => { + expect(selectRoles(stateWith(['user']))).toEqual(['user']); + expect(selectRolesResolved(stateWith(['user'], false))).toBe(false); + }); +}); diff --git a/frontend/src/preferences/preferenceSlice.ts b/frontend/src/preferences/preferenceSlice.ts index d674c910..37cb7d4b 100644 --- a/frontend/src/preferences/preferenceSlice.ts +++ b/frontend/src/preferences/preferenceSlice.ts @@ -38,6 +38,12 @@ export interface Preference { availableModels: Model[]; modelsLoading: boolean; agentFolders: AgentFolder[] | null; + // RBAC roles for the current principal, fetched fresh from /api/user/me each + // session (DB-authoritative — deliberately NOT persisted to localStorage, so + // a demotion can't survive a reload). ``rolesResolved`` gates UI that must + // not act before the first /me response (avoids a first-paint admin bounce). + roles: string[]; + rolesResolved: boolean; } const initialState: Preference = { @@ -75,6 +81,8 @@ const initialState: Preference = { availableModels: [], modelsLoading: false, agentFolders: null, + roles: [], + rolesResolved: false, }; export const prefSlice = createSlice({ @@ -135,6 +143,14 @@ export const prefSlice = createSlice({ setAgentFolders: (state, action: PayloadAction) => { state.agentFolders = action.payload; }, + setRoles: (state, action: PayloadAction) => { + state.roles = action.payload; + state.rolesResolved = true; + }, + clearRoles: (state) => { + state.roles = []; + state.rolesResolved = false; + }, }, }); @@ -157,6 +173,8 @@ export const { setAvailableModels, setModelsLoading, setAgentFolders, + setRoles, + clearRoles, } = prefSlice.actions; export default prefSlice.reducer; @@ -302,3 +320,8 @@ export const selectModelsLoading = (state: RootState) => state.preference.modelsLoading; export const selectAgentFolders = (state: RootState) => state.preference.agentFolders; +export const selectRoles = (state: RootState) => state.preference.roles; +export const selectRolesResolved = (state: RootState) => + state.preference.rolesResolved; +export const selectIsAdmin = (state: RootState) => + state.preference.roles.includes('admin'); diff --git a/frontend/src/store.ts b/frontend/src/store.ts index 7125c3d9..f9edd034 100644 --- a/frontend/src/store.ts +++ b/frontend/src/store.ts @@ -61,6 +61,8 @@ const preloadedState: { preference: Preference } = { availableModels: [], modelsLoading: false, agentFolders: null, + roles: [], + rolesResolved: false, }, }; const store = configureStore({ diff --git a/scripts/grant_admin.py b/scripts/grant_admin.py new file mode 100644 index 00000000..ec401b0d --- /dev/null +++ b/scripts/grant_admin.py @@ -0,0 +1,130 @@ +"""Grant, revoke, or list the ``admin`` role for DocsGPT users. + +Manual admin grants are the bootstrap mechanism for RBAC: the first admin is +created here (there is no UI to grant admin until you already are one), and that +admin can then manage others. Grants are written to ``user_roles`` with +``source='manual'`` and take effect on the user's next request (persisted RBAC +applies under ``AUTH_TYPE=oidc``; ``user_id`` is the OIDC ``sub``). + +Usage:: + + python scripts/grant_admin.py # grant admin + python scripts/grant_admin.py --revoke # revoke the manual admin grant + python scripts/grant_admin.py --list # list current admins + python scripts/grant_admin.py --force # grant even if no users row exists + +Exit codes: + 0 — success + 1 — bad usage / user not found (without --force) + 2 — database error +""" + +from __future__ import annotations + +import argparse +import logging +import sys +from pathlib import Path + +# Make the project root importable regardless of cwd. +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +import dotenv # noqa: E402 + +dotenv.load_dotenv() + +from application.storage.db.repositories.auth_events import AuthEventsRepository # noqa: E402 +from application.storage.db.repositories.user_roles import UserRolesRepository # noqa: E402 +from application.storage.db.repositories.users import UsersRepository # noqa: E402 +from application.storage.db.session import db_readonly, db_session # noqa: E402 + +logging.basicConfig(level=logging.WARNING, format="%(levelname)s %(message)s") +logger = logging.getLogger("grant_admin") + +ACTOR = "cli" + + +def _list_admins() -> int: + with db_readonly() as conn: + admins = UserRolesRepository(conn).list_admins() + if not admins: + print("No admins found.") + return 0 + print(f"{'user_id':40} {'sources':20} granted_at") + for row in admins: + sources = ",".join(row.get("sources") or []) + print(f"{row['user_id']:40} {sources:20} {row.get('granted_at')}") + return 0 + + +def _grant(user_id: str, force: bool) -> int: + with db_session() as conn: + if not force: + if UsersRepository(conn).get(user_id) is None: + print( + f"No users row for {user_id!r}. The user must have signed in at least " + f"once, or pass --force to grant anyway (creates a dangling grant).", + file=sys.stderr, + ) + return 1 + inserted = UserRolesRepository(conn).grant( + user_id, "admin", source="manual", granted_by=ACTOR + ) + if inserted: + AuthEventsRepository(conn).insert( + user_id, + "role_granted", + metadata={"role": "admin", "source": "manual", "granted_by": ACTOR}, + ) + print(f"Granted admin to {user_id!r}.") + else: + print(f"{user_id!r} already has a manual admin grant; nothing to do.") + return 0 + + +def _revoke(user_id: str) -> int: + with db_session() as conn: + removed = UserRolesRepository(conn).revoke(user_id, "admin", source="manual") + if removed: + AuthEventsRepository(conn).insert( + user_id, + "role_revoked", + metadata={"role": "admin", "source": "manual", "revoked_by": ACTOR}, + ) + print(f"Revoked the manual admin grant from {user_id!r}.") + else: + print( + f"{user_id!r} has no manual admin grant. " + f"(OIDC-group grants are managed by group membership, not this script.)" + ) + return 0 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Manage the admin role for DocsGPT users.") + parser.add_argument("user_id", nargs="?", help="The user's auth sub (OIDC subject id).") + parser.add_argument("--revoke", action="store_true", help="Revoke the manual admin grant.") + parser.add_argument("--list", action="store_true", help="List current admins and exit.") + parser.add_argument( + "--force", action="store_true", help="Grant even if no users row exists yet." + ) + args = parser.parse_args(argv) + + if args.list: + action = _list_admins + elif not args.user_id: + parser.error("user_id is required unless --list is given.") + elif args.revoke: + action = lambda: _revoke(args.user_id) # noqa: E731 + else: + action = lambda: _grant(args.user_id, args.force) # noqa: E731 + + try: + return action() + except Exception: + logger.error("Database operation failed", exc_info=True) + return 2 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/api/test_rbac_endpoints.py b/tests/api/test_rbac_endpoints.py new file mode 100644 index 00000000..887c8239 --- /dev/null +++ b/tests/api/test_rbac_endpoints.py @@ -0,0 +1,204 @@ +"""Endpoint tests for GET /api/user/me and GET /api/admin/users.""" + +from __future__ import annotations + +import json +from contextlib import contextmanager +from unittest.mock import Mock, patch + +import pytest + + +@pytest.fixture +def client(): + from application.app import app as flask_app + + flask_app.config["TESTING"] = True + return flask_app.test_client() + + +@contextmanager +def _fake_readonly(): + yield Mock() + + +@pytest.mark.unit +class TestMeEndpoint: + def test_returns_user_id_and_roles(self, client): + with patch("application.app.handle_auth", return_value={"sub": "u1"}), patch( + "application.app.resolve_roles", return_value=["user"] + ): + resp = client.get("/api/user/me") + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["user_id"] == "u1" + assert data["roles"] == ["user"] + + def test_echoes_admin_role(self, client): + with patch("application.app.handle_auth", return_value={"sub": "a1"}), patch( + "application.app.resolve_roles", return_value=["admin", "user"] + ): + resp = client.get("/api/user/me") + data = json.loads(resp.data) + assert data["roles"] == ["admin", "user"] + + def test_unauthenticated_returns_401(self, client): + with patch("application.app.handle_auth", return_value=None): + resp = client.get("/api/user/me") + assert resp.status_code == 401 + + +@pytest.mark.unit +class TestAdminUsersEndpoint: + def test_non_admin_forbidden(self, client): + with patch("application.app.handle_auth", return_value={"sub": "u1"}), patch( + "application.app.resolve_roles", return_value=["user"] + ): + resp = client.get("/api/admin/users") + assert resp.status_code == 403 + + def test_unauthenticated_401(self, client): + with patch("application.app.handle_auth", return_value=None): + resp = client.get("/api/admin/users") + assert resp.status_code == 401 + + @staticmethod + def _admin_get(client, repo, query=""): + with patch("application.app.handle_auth", return_value={"sub": "a1"}), patch( + "application.app.resolve_roles", return_value=["admin", "user"] + ), patch("application.api.admin.routes.db_readonly", _fake_readonly), patch( + "application.api.admin.routes.UsersRepository", return_value=repo + ): + return client.get(f"/api/admin/users{query}") + + @staticmethod + def _rows(n): + return [ + {"user_id": f"u{i}", "active": True, "created_at": "2026-01-01"} + for i in range(n) + ] + + def test_admin_gets_user_list(self, client): + repo = Mock() + repo.list_paginated.return_value = ( + 1, + [{"user_id": "u1", "active": True, "created_at": "2026-01-01"}], + ) + resp = self._admin_get(client, repo) + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["success"] is True + assert data["total"] == 1 + assert data["users"][0] == { + "user_id": "u1", + "active": True, + "created_at": "2026-01-01", + } + repo.list_paginated.assert_called_once_with(None, 0, 25) + + def test_page_size_clamped_and_offset_computed(self, client): + repo = Mock() + repo.list_paginated.return_value = (0, []) + self._admin_get(client, repo, "?page=2&page_size=999") + # page_size clamps to _MAX_PAGE_SIZE=100, offset=(2-1)*100 + repo.list_paginated.assert_called_once_with(None, 100, 100) + + def test_non_int_params_fall_back_to_defaults(self, client): + repo = Mock() + repo.list_paginated.return_value = (0, []) + self._admin_get(client, repo, "?page=abc&page_size=xyz") + repo.list_paginated.assert_called_once_with(None, 0, 25) + + def test_user_id_filter_passed_first_positional(self, client): + repo = Mock() + repo.list_paginated.return_value = (0, []) + self._admin_get(client, repo, "?user_id=foo") + repo.list_paginated.assert_called_once_with("foo", 0, 25) + + def test_has_more_true_when_more_pages_remain(self, client): + repo = Mock() + repo.list_paginated.return_value = (5, self._rows(3)) + resp = self._admin_get(client, repo, "?page=1&page_size=3") + assert json.loads(resp.data)["has_more"] is True + + def test_has_more_false_at_last_page(self, client): + repo = Mock() + repo.list_paginated.return_value = (3, self._rows(3)) + resp = self._admin_get(client, repo, "?page=1&page_size=3") + assert json.loads(resp.data)["has_more"] is False + + +@pytest.mark.unit +class TestChokepointOverwritesForgedRoles: + """The real resolver must overwrite any inbound 'roles' claim end-to-end. + + These drive a forged admin token through the actual app.py chokepoint (only + handle_auth is patched; resolve_roles runs for real), so a regression that + trusted the JWT's roles claim would be caught. + """ + + def test_forged_admin_ignored_in_session_jwt_mode(self, client): + from application.api.user import authz + + forged = lambda *a, **k: {"sub": "attacker", "roles": ["admin"]} # noqa: E731 + with patch("application.app.handle_auth", side_effect=forged), patch.object( + authz.settings, "AUTH_TYPE", "session_jwt" + ): + admin_resp = client.get("/api/admin/users") + me_resp = client.get("/api/user/me") + assert admin_resp.status_code == 403 + assert json.loads(me_resp.data)["roles"] == ["user"] + + def test_forged_admin_ignored_in_no_auth_with_local_admin_off(self, client): + from application.api.user import authz + + forged = lambda *a, **k: {"sub": "local", "roles": ["admin"]} # noqa: E731 + with patch("application.app.handle_auth", side_effect=forged), patch.object( + authz.settings, "AUTH_TYPE", None + ), patch.object(authz.settings, "LOCAL_MODE_ADMIN", False): + admin_resp = client.get("/api/admin/users") + me_resp = client.get("/api/user/me") + assert admin_resp.status_code == 403 + assert json.loads(me_resp.data)["roles"] == ["user"] + + def test_forged_admin_ignored_in_oidc_mode_without_grant(self, client): + # OIDC is the only privilege-bearing mode: drive a forged admin claim + # through the real chokepoint with NO DB grant — must resolve to user. + from application.api.user import authz + + repo = Mock() + repo.role_names_for.return_value = [] # no persisted grant + forged = lambda *a, **k: {"sub": "attacker", "roles": ["admin"]} # noqa: E731 + with patch("application.app.handle_auth", side_effect=forged), patch( + "application.app.oidc_session_denied", return_value=False + ), patch.object(authz.settings, "AUTH_TYPE", "oidc"), patch.object( + authz, "db_readonly", _fake_readonly + ), patch.object(authz, "UserRolesRepository", return_value=repo): + admin_resp = client.get("/api/admin/users") + me_resp = client.get("/api/user/me") + assert admin_resp.status_code == 403 + assert json.loads(me_resp.data)["roles"] == ["user"] + # Proves the chokepoint fired and resolved from the DB, not the claim. + repo.role_names_for.assert_called_with("attacker") + + def test_oidc_db_grant_yields_admin_end_to_end(self, client): + # The DB grant (not any claim) is the source of admin, end-to-end. + from application.api.user import authz + + authz_repo = Mock() + authz_repo.role_names_for.return_value = ["admin"] + admin_repo = Mock() + admin_repo.list_paginated.return_value = (0, []) + with patch( + "application.app.handle_auth", return_value={"sub": "alice"} + ), patch("application.app.oidc_session_denied", return_value=False), patch.object( + authz.settings, "AUTH_TYPE", "oidc" + ), patch.object(authz, "db_readonly", _fake_readonly), patch.object( + authz, "UserRolesRepository", return_value=authz_repo + ), patch("application.api.admin.routes.db_readonly", _fake_readonly), patch( + "application.api.admin.routes.UsersRepository", return_value=admin_repo + ): + admin_resp = client.get("/api/admin/users") + me_resp = client.get("/api/user/me") + assert admin_resp.status_code == 200 + assert json.loads(me_resp.data)["roles"] == ["admin", "user"] diff --git a/tests/scripts/__init__.py b/tests/scripts/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/scripts/test_grant_admin.py b/tests/scripts/test_grant_admin.py new file mode 100644 index 00000000..4eca826a --- /dev/null +++ b/tests/scripts/test_grant_admin.py @@ -0,0 +1,100 @@ +"""Tests for scripts/grant_admin.py orchestration (grant/revoke/list/exit codes). + +Drives ``grant_admin.main(argv)`` against the ephemeral ``pg_conn`` by +redirecting the script's ``db_session`` / ``db_readonly`` to yield that +transactional connection — so every write rolls back at teardown. The +underlying repository SQL is covered by +``tests/storage/db/repositories/test_user_roles.py``; these tests pin the +script's own decision logic (audit-gating, manual-only revoke, exit codes). +""" + +from __future__ import annotations + +import sys +from contextlib import contextmanager +from pathlib import Path + +import pytest + +# Project root on sys.path so ``scripts`` is importable. +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from scripts import grant_admin # noqa: E402 +from application.storage.db.repositories.auth_events import ( # noqa: E402 + AuthEventsRepository, +) +from application.storage.db.repositories.user_roles import ( # noqa: E402 + UserRolesRepository, +) +from application.storage.db.repositories.users import UsersRepository # noqa: E402 + + +@pytest.fixture +def patched_db(pg_conn, monkeypatch): + @contextmanager + def _use_pg_conn(): + yield pg_conn + + monkeypatch.setattr(grant_admin, "db_session", _use_pg_conn) + monkeypatch.setattr(grant_admin, "db_readonly", _use_pg_conn) + return pg_conn + + +def _audit_count(conn, user_id: str, event: str) -> int: + rows = AuthEventsRepository(conn).list_recent(user_id, limit=100) + return sum(1 for r in rows if r["event"] == event) + + +class TestGrant: + def test_missing_user_without_force_returns_1_and_writes_nothing(self, patched_db): + assert grant_admin.main(["ghost"]) == 1 + assert UserRolesRepository(patched_db).role_names_for("ghost") == [] + assert _audit_count(patched_db, "ghost", "role_granted") == 0 + + def test_force_inserts_grant_and_exactly_one_audit(self, patched_db): + assert grant_admin.main(["alice", "--force"]) == 0 + assert UserRolesRepository(patched_db).role_names_for("alice") == ["admin"] + assert _audit_count(patched_db, "alice", "role_granted") == 1 + + def test_existing_user_grants_without_force(self, patched_db): + UsersRepository(patched_db).upsert("bob") + assert grant_admin.main(["bob"]) == 0 + assert UserRolesRepository(patched_db).role_names_for("bob") == ["admin"] + + def test_idempotent_grant_does_not_double_audit(self, patched_db): + assert grant_admin.main(["alice", "--force"]) == 0 + assert grant_admin.main(["alice", "--force"]) == 0 + assert _audit_count(patched_db, "alice", "role_granted") == 1 + + +class TestRevoke: + def test_revoke_removes_manual_only_and_audits(self, patched_db): + repo = UserRolesRepository(patched_db) + repo.grant("alice", source="manual") + repo.grant("alice", source="oidc_group") + assert grant_admin.main(["alice", "--revoke"]) == 0 + assert {r["source"] for r in repo.list_for("alice")} == {"oidc_group"} + assert _audit_count(patched_db, "alice", "role_revoked") == 1 + + def test_revoke_without_grant_returns_0_and_no_audit(self, patched_db): + assert grant_admin.main(["nobody", "--revoke"]) == 0 + assert _audit_count(patched_db, "nobody", "role_revoked") == 0 + + +class TestList: + def test_list_prints_admins(self, patched_db, capsys): + UserRolesRepository(patched_db).grant("alice", source="manual") + assert grant_admin.main(["--list"]) == 0 + assert "alice" in capsys.readouterr().out + + +class TestErrors: + def test_db_error_returns_2(self, monkeypatch): + @contextmanager + def _boom(): + raise RuntimeError("db down") + yield # pragma: no cover + + monkeypatch.setattr(grant_admin, "db_session", _boom) + monkeypatch.setattr(grant_admin, "db_readonly", _boom) + assert grant_admin.main(["alice", "--force"]) == 2 diff --git a/tests/storage/db/repositories/test_user_roles.py b/tests/storage/db/repositories/test_user_roles.py new file mode 100644 index 00000000..6d186f97 --- /dev/null +++ b/tests/storage/db/repositories/test_user_roles.py @@ -0,0 +1,104 @@ +"""Tests for UserRolesRepository against a real Postgres instance.""" + +from __future__ import annotations + +from application.storage.db.repositories.user_roles import UserRolesRepository + + +def _repo(conn) -> UserRolesRepository: + return UserRolesRepository(conn) + + +class TestGrantRevoke: + def test_grant_then_role_names(self, pg_conn): + repo = _repo(pg_conn) + assert repo.grant("alice") is True + assert repo.role_names_for("alice") == ["admin"] + + def test_grant_is_idempotent(self, pg_conn): + repo = _repo(pg_conn) + assert repo.grant("alice") is True + assert repo.grant("alice") is False + assert repo.role_names_for("alice") == ["admin"] + + def test_revoke_removes_grant(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice") + assert repo.revoke("alice") is True + assert repo.role_names_for("alice") == [] + + def test_revoke_missing_is_false(self, pg_conn): + assert _repo(pg_conn).revoke("nobody") is False + + def test_role_names_unknown_user(self, pg_conn): + assert _repo(pg_conn).role_names_for("nobody") == [] + + def test_role_names_empty_sub_short_circuits(self, pg_conn): + assert _repo(pg_conn).role_names_for("") == [] + + def test_granted_by_recorded(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice", granted_by="cli") + rows = repo.list_for("alice") + assert rows[0]["granted_by"] == "cli" + + +class TestSourceCoexistence: + def test_manual_and_oidc_grants_coexist(self, pg_conn): + repo = _repo(pg_conn) + assert repo.grant("alice", source="manual") is True + assert repo.grant("alice", source="oidc_group") is True + # role_names_for dedups to a single role across sources + assert repo.role_names_for("alice") == ["admin"] + assert {r["source"] for r in repo.list_for("alice")} == {"manual", "oidc_group"} + + def test_revoking_one_source_keeps_other(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice", source="manual") + repo.grant("alice", source="oidc_group") + repo.revoke("alice", source="oidc_group") + assert repo.role_names_for("alice") == ["admin"] + assert {r["source"] for r in repo.list_for("alice")} == {"manual"} + + +class TestReconcileOidcAdmin: + def test_reconcile_grants_then_no_change(self, pg_conn): + repo = _repo(pg_conn) + assert repo.reconcile_oidc_admin("alice", True) == "granted" + assert repo.reconcile_oidc_admin("alice", True) is None + assert repo.role_names_for("alice") == ["admin"] + + def test_reconcile_revokes_then_no_change(self, pg_conn): + repo = _repo(pg_conn) + repo.reconcile_oidc_admin("alice", True) + assert repo.reconcile_oidc_admin("alice", False) == "revoked" + assert repo.reconcile_oidc_admin("alice", False) is None + assert repo.role_names_for("alice") == [] + + def test_reconcile_revoke_preserves_manual_grant(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice", source="manual") + repo.reconcile_oidc_admin("alice", True) + assert repo.reconcile_oidc_admin("alice", False) == "revoked" + # manual grant survives the oidc-group revoke + assert repo.role_names_for("alice") == ["admin"] + assert {r["source"] for r in repo.list_for("alice")} == {"manual"} + + +class TestListAdmins: + def test_empty(self, pg_conn): + assert _repo(pg_conn).list_admins() == [] + + def test_lists_distinct_users(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice", source="manual") + repo.grant("bob", source="oidc_group") + assert {a["user_id"] for a in repo.list_admins()} == {"alice", "bob"} + + def test_dedups_user_with_multiple_sources(self, pg_conn): + repo = _repo(pg_conn) + repo.grant("alice", source="manual") + repo.grant("alice", source="oidc_group") + admins = repo.list_admins() + assert len(admins) == 1 + assert set(admins[0]["sources"]) == {"manual", "oidc_group"} diff --git a/tests/storage/db/repositories/test_users.py b/tests/storage/db/repositories/test_users.py index d4fd401d..767e7c94 100644 --- a/tests/storage/db/repositories/test_users.py +++ b/tests/storage/db/repositories/test_users.py @@ -356,3 +356,40 @@ class TestDefaultToolPreferences: doc = repo.get("u-tp-mix") assert doc["agent_preferences"]["pinned"] == ["agent-99"] assert doc["tool_preferences"]["disabled_default_tools"] == ["memory"] + + +class TestListPaginated: + """Validates the (filter, offset, limit) contract + the row keys the admin + /users endpoint reads, against real SQL output — a mock can't catch a column + rename or a swapped positional arg.""" + + def test_returns_rows_with_admin_endpoint_keys(self, pg_conn): + repo = _repo(pg_conn) + repo.upsert("page-a") + repo.upsert("page-b") + total, rows = repo.list_paginated(None, 0, 10) + assert total >= 2 + assert {"page-a", "page-b"} <= {r["user_id"] for r in rows} + sample = next(r for r in rows if r["user_id"] == "page-a") + # the keys application/api/admin/routes.py projects must be real columns + assert "active" in sample and "created_at" in sample + + def test_first_positional_arg_is_the_filter(self, pg_conn): + repo = _repo(pg_conn) + repo.upsert("page-a") + repo.upsert("page-b") + total, rows = repo.list_paginated("page-a", 0, 10) + assert total == 1 + assert [r["user_id"] for r in rows] == ["page-a"] + + def test_offset_and_limit_paginate_without_overlap(self, pg_conn): + repo = _repo(pg_conn) + for i in range(3): + repo.upsert(f"pg-{i}") + total, first = repo.list_paginated(None, 0, 2) + assert total >= 3 + assert len(first) == 2 + _, second = repo.list_paginated(None, 2, 2) + assert {r["user_id"] for r in first}.isdisjoint( + {r["user_id"] for r in second} + ) diff --git a/tests/storage/db/test_migration_user_roles.py b/tests/storage/db/test_migration_user_roles.py new file mode 100644 index 00000000..88895e8a --- /dev/null +++ b/tests/storage/db/test_migration_user_roles.py @@ -0,0 +1,61 @@ +"""Guards for the 0020_user_roles migration. + +The single-head assertion catches a branched Alembic history (e.g. a new +migration numbered off the wrong parent), which would wedge ``upgrade head``. +""" + +from __future__ import annotations + +import pytest +from alembic.config import Config +from alembic.script import ScriptDirectory + + +@pytest.mark.unit +def test_single_migration_head(_alembic_ini_path): + script = ScriptDirectory.from_config(Config(str(_alembic_ini_path))) + heads = script.get_heads() + assert len(heads) == 1, f"expected one head, found {heads}" + + +@pytest.mark.unit +def test_user_roles_revision_chains_off_0019(_alembic_ini_path): + script = ScriptDirectory.from_config(Config(str(_alembic_ini_path))) + rev = script.get_revision("0020_user_roles") + assert rev is not None + assert rev.down_revision == "0019_agent_slug" + + +class TestUserRolesSchema: + def test_table_exists_after_migration(self, pg_conn): + from sqlalchemy import text + + cols = pg_conn.execute( + text( + "SELECT column_name FROM information_schema.columns " + "WHERE table_name = 'user_roles'" + ) + ).fetchall() + names = {c[0] for c in cols} + assert names == {"user_id", "role", "source", "granted_by", "granted_at"} + + def test_role_check_rejects_unknown_role(self, pg_conn): + from sqlalchemy import text + from sqlalchemy.exc import IntegrityError + + with pytest.raises(IntegrityError): + pg_conn.execute( + text("INSERT INTO user_roles (user_id, role) VALUES ('x', 'superuser')") + ) + + def test_source_check_rejects_unknown_source(self, pg_conn): + from sqlalchemy import text + from sqlalchemy.exc import IntegrityError + + with pytest.raises(IntegrityError): + pg_conn.execute( + text( + "INSERT INTO user_roles (user_id, role, source) " + "VALUES ('x', 'admin', 'sso')" + ) + ) diff --git a/tests/test_authz.py b/tests/test_authz.py new file mode 100644 index 00000000..c992807c --- /dev/null +++ b/tests/test_authz.py @@ -0,0 +1,128 @@ +"""Unit tests for role resolution and the authorization decorator.""" + +from __future__ import annotations + +from contextlib import contextmanager +from unittest.mock import Mock, patch + +import pytest +from flask import Flask, request + +from application.api.user import authz + + +@contextmanager +def _fake_conn(): + yield Mock() + + +def _patch_oidc(role_names=None, raise_db=False): + """Patch authz for OIDC mode with a stubbed UserRolesRepository.""" + repo = Mock() + repo.role_names_for.return_value = role_names or [] + settings_patch = patch.object(authz, "settings") + db_patch = patch.object(authz, "db_readonly", _fake_conn) + if raise_db: + repo_patch = patch.object(authz, "UserRolesRepository", side_effect=RuntimeError("db down")) + else: + repo_patch = patch.object(authz, "UserRolesRepository", return_value=repo) + return settings_patch, db_patch, repo_patch, repo + + +@pytest.mark.unit +class TestResolveRoles: + def test_no_token_is_user_only(self): + assert authz.resolve_roles(None) == ["user"] + + def test_simple_jwt_never_admin(self): + with patch.object(authz, "settings") as s: + s.AUTH_TYPE = "simple_jwt" + assert authz.resolve_roles({"sub": "local"}) == ["user"] + + def test_session_jwt_never_admin(self): + with patch.object(authz, "settings") as s: + s.AUTH_TYPE = "session_jwt" + assert authz.resolve_roles({"sub": "abc-uuid"}) == ["user"] + + def test_none_mode_local_admin_disabled(self): + with patch.object(authz, "settings") as s: + s.AUTH_TYPE = None + s.LOCAL_MODE_ADMIN = False + assert authz.resolve_roles({"sub": "local"}) == ["user"] + + def test_none_mode_local_admin_enabled(self): + with patch.object(authz, "settings") as s: + s.AUTH_TYPE = None + s.LOCAL_MODE_ADMIN = True + assert authz.resolve_roles({"sub": "local"}) == ["admin", "user"] + + def test_oidc_db_grant_makes_admin(self): + sp, dp, rp, repo = _patch_oidc(role_names=["admin"]) + with sp as s, dp, rp: + s.AUTH_TYPE = "oidc" + assert authz.resolve_roles({"sub": "alice"}) == ["admin", "user"] + repo.role_names_for.assert_called_once_with("alice") + + def test_oidc_no_grant_is_user(self): + sp, dp, rp, _ = _patch_oidc(role_names=[]) + with sp as s, dp, rp: + s.AUTH_TYPE = "oidc" + assert authz.resolve_roles({"sub": "bob"}) == ["user"] + + def test_oidc_ignores_forged_roles_claim(self): + sp, dp, rp, _ = _patch_oidc(role_names=[]) + with sp as s, dp, rp: + s.AUTH_TYPE = "oidc" + # A self-minted token asserting admin must NOT be trusted. + assert authz.resolve_roles({"sub": "bob", "roles": ["admin"]}) == ["user"] + + def test_oidc_db_error_fails_open_to_user(self): + sp, dp, rp, _ = _patch_oidc(raise_db=True) + with sp as s, dp, rp: + s.AUTH_TYPE = "oidc" + assert authz.resolve_roles({"sub": "bob"}) == ["user"] + + +@pytest.mark.unit +class TestHasRole: + def test_user_role_always_true(self): + assert authz.has_role(None, "user") + assert authz.has_role({}, "user") + assert authz.has_role({"roles": []}, "user") + + def test_admin_requires_grant(self): + assert authz.has_role({"roles": ["admin"]}, "admin") + assert not authz.has_role({"roles": ["user"]}, "admin") + + def test_missing_roles_key_is_not_admin(self): + assert not authz.has_role({"sub": "x"}, "admin") + assert not authz.has_role(None, "admin") + + +@pytest.mark.unit +class TestRequireRole: + def _invoke(self, token): + app = Flask(__name__) + + @authz.require_role("admin") + def view(): + return "ok" + + with app.test_request_context("/"): + request.decoded_token = token + return view() + + def test_no_token_returns_401(self): + resp = self._invoke(None) + assert resp.status_code == 401 + + def test_non_admin_returns_403(self): + resp = self._invoke({"sub": "x", "roles": ["user"]}) + assert resp.status_code == 403 + + def test_missing_roles_key_returns_403_without_raising(self): + resp = self._invoke({"sub": "x"}) + assert resp.status_code == 403 + + def test_admin_passes_through(self): + assert self._invoke({"sub": "x", "roles": ["admin"]}) == "ok" diff --git a/tests/test_oidc.py b/tests/test_oidc.py index b60e3a53..10b7d895 100644 --- a/tests/test_oidc.py +++ b/tests/test_oidc.py @@ -988,6 +988,105 @@ class TestCallbackGroups: assert decoded["sub"] == "alice" +@pytest.mark.unit +class TestCallbackAdminReconcile: + """OIDC group -> admin reconcile fires on the login callback.""" + + @pytest.fixture(autouse=True) + def _db(self, db_mocks): + self.db = db_mocks + + @pytest.fixture + def roles_repo(self): + repo = Mock() + repo.reconcile_oidc_admin.return_value = None + with patch("application.api.oidc.routes.UserRolesRepository", return_value=repo): + yield repo + + def _callback(self, client, fake_redis, claims, userinfo=None): + state, nonce = _seed_state(client, fake_redis) + claims = {**claims, "nonce": nonce} + with patch("application.api.oidc.provider.requests") as mock_requests: + mock_requests.get.side_effect = make_fake_get([PUBLIC_JWK], userinfo=userinfo) + mock_requests.post.return_value = _signed_token_response(claims) + return client.get(f"/api/auth/oidc/callback?code=abc&state={state}") + + def _role_events(self, name): + return [ + c + for c in self.db.events.insert.call_args_list + if len(c.args) >= 2 and c.args[1] == name + ] + + def test_no_admin_groups_is_a_noop(self, client, fake_redis, roles_repo): + # OIDC_ADMIN_GROUPS unset (default) — reconcile must never run. + response = self._callback(client, fake_redis, id_token_claims(groups=["devs"])) + assert "#oidc_code=" in response.headers["Location"] + roles_repo.reconcile_oidc_admin.assert_not_called() + + def test_matching_group_grants_admin_and_audits( + self, client, fake_redis, roles_repo, monkeypatch + ): + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + roles_repo.reconcile_oidc_admin.return_value = "granted" + response = self._callback(client, fake_redis, id_token_claims(groups=["admins", "devs"])) + assert "#oidc_code=" in response.headers["Location"] + roles_repo.reconcile_oidc_admin.assert_called_once_with("oidc-user-1", True) + granted = self._role_events("role_granted") + assert len(granted) == 1 + assert granted[0].kwargs["metadata"] == {"role": "admin", "source": "oidc_group"} + + def test_non_matching_group_revokes_and_audits( + self, client, fake_redis, roles_repo, monkeypatch + ): + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + roles_repo.reconcile_oidc_admin.return_value = "revoked" + response = self._callback(client, fake_redis, id_token_claims(groups=["devs"])) + assert "#oidc_code=" in response.headers["Location"] + roles_repo.reconcile_oidc_admin.assert_called_once_with("oidc-user-1", False) + assert len(self._role_events("role_revoked")) == 1 + + def test_absent_groups_claim_does_not_revoke( + self, client, fake_redis, roles_repo, monkeypatch + ): + # Admin mapping configured, but the IdP asserts NO groups claim (absent + # from both id_token and userinfo) — must skip, not revoke. + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + response = self._callback( + client, + fake_redis, + id_token_claims(), + userinfo={"sub": "oidc-user-1"}, + ) + assert "#oidc_code=" in response.headers["Location"] + roles_repo.reconcile_oidc_admin.assert_not_called() + + def test_reconcile_failure_does_not_block_login( + self, client, fake_redis, roles_repo, monkeypatch + ): + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + roles_repo.reconcile_oidc_admin.side_effect = RuntimeError("db down") + response = self._callback(client, fake_redis, id_token_claims(groups=["admins"])) + assert "#oidc_code=" in response.headers["Location"] + + def test_admin_group_backfilled_from_userinfo_grants( + self, client, fake_redis, roles_repo, monkeypatch + ): + # OIDC_ALLOWED_GROUPS unset but OIDC_ADMIN_GROUPS set, and the id_token + # omits groups: userinfo backfill (driven by the admin-groups clause in + # _effective_claims) must still surface the group and grant admin. + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + roles_repo.reconcile_oidc_admin.return_value = "granted" + response = self._callback( + client, + fake_redis, + id_token_claims(), # no groups claim in the id_token + userinfo={"sub": "oidc-user-1", "groups": ["admins"]}, + ) + assert "#oidc_code=" in response.headers["Location"] + roles_repo.reconcile_oidc_admin.assert_called_once_with("oidc-user-1", True) + + @pytest.mark.unit class TestCallbackUserGate: @@ -1318,6 +1417,57 @@ class TestRefreshRoute: # IdP kept the old refresh token, so it is re-stored under the new jti. assert fake_redis.store[f"oidc:refresh:{decoded['jti']}"] == b"rt-old" + def test_refresh_reconciles_oidc_admin_with_fresh_claims( + self, client, fake_redis, monkeypatch + ): + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + token = make_session_token() + fake_redis.store["oidc:refresh:jti-1"] = b"rt-old" + id_claims = id_token_claims(groups=["admins"]) + del id_claims["nonce"] + roles_repo = Mock() + roles_repo.reconcile_oidc_admin.return_value = "granted" + with patch( + "application.api.oidc.routes.UserRolesRepository", return_value=roles_repo + ): + response, _ = self._refresh( + client, + token, + idp_response={ + "access_token": "at-2", + "refresh_token": "rt-new", + "id_token": sign_id_token(id_claims), + }, + ) + assert response.status_code == 200 + roles_repo.reconcile_oidc_admin.assert_called_once_with("oidc-user-1", True) + + def test_refresh_absent_groups_claim_does_not_revoke( + self, client, fake_redis, monkeypatch + ): + # Same absent-claim guard as the login path: a refresh id_token carrying + # no groups claim (and no userinfo backfill) must NOT revoke admin. + monkeypatch.setattr(settings, "OIDC_ADMIN_GROUPS", "admins") + token = make_session_token() + fake_redis.store["oidc:refresh:jti-1"] = b"rt-old" + id_claims = id_token_claims() # no groups claim + del id_claims["nonce"] + roles_repo = Mock() + with patch( + "application.api.oidc.routes.UserRolesRepository", return_value=roles_repo + ): + response, _ = self._refresh( + client, + token, + idp_response={ + "access_token": "at-2", + "refresh_token": "rt-new", + "id_token": sign_id_token(id_claims), + }, + ) + assert response.status_code == 200 + roles_repo.reconcile_oidc_admin.assert_not_called() + def test_refresh_denied_when_groups_no_longer_allowed( self, client, fake_redis, monkeypatch ):