feat: add connectors permissions for Saas#1782
Merged
Merged
Conversation
Introduce admin Connectors Permission settings (cloud-only) backed by workspace_config, enforce availability on connector APIs and OAuth init, and sync the new connectors:manage:access permission at startup without an Alembic migration. OSS/dev OSS brand bypasses policy; add dev role toggle and brand-aware proxy headers for local RBAC testing.
github-actions
Bot
added
frontend
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR integrates workspace connector-access policy enforcement and dev-role switching across backend and frontend. It adds environment toggles and cloud-context determination, enforces workspace connector availability via database policy checks in API handlers, provides admin UI to manage connector access per workspace, and enables developers to switch roles locally for testing. Frontend contexts and permission evaluation were refactored to support SaaS policy gating. ChangesConnector policy, settings access, and dev-role controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Wallgau
changed the title
github-actions
Bot
added
enhancement
Wallgau
changed the title
github-actions
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/auth.py (1)
1-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix import ordering and unused import to unblock Ruff/mypy CI.
Current import layout triggers E402, and
typing.Optionalis unused (F401). This is currently failing pipeline checks.Suggested fix
-from typing import Optional - -from fastapi import Depends, HTTPException, Request -from fastapi.responses import JSONResponse - -from utils.logging_config import get_logger -from utils.telemetry import Category, MessageId, TelemetryClient -from utils.version_utils import OPENRAG_VERSION - -logger = get_logger(__name__) - -from pydantic import BaseModel -from sqlalchemy.ext.asyncio import AsyncSession - -from dependencies import ( +from fastapi import Depends, HTTPException, Request +from fastapi.responses import JSONResponse +from pydantic import BaseModel +from sqlalchemy.ext.asyncio import AsyncSession + +from dependencies import ( get_auth_service, get_current_user, get_db_session, get_optional_user, ) from services.connector_access_service import ( CONNECTOR_TYPES, is_connector_access_policy_enforced, is_connector_allowed_for_request, ) from session_manager import User +from utils.logging_config import get_logger +from utils.telemetry import Category, MessageId, TelemetryClient +from utils.version_utils import OPENRAG_VERSION + +logger = get_logger(__name__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/auth.py` around lines 1 - 26, Reorder and clean up imports in auth module to satisfy linters: remove the unused typing.Optional import and reorder imports so standard library imports come first, then third-party (fastapi, pydantic, sqlalchemy), then local project imports; ensure symbols referenced like get_logger, OPENRAG_VERSION, BaseModel, AsyncSession, get_auth_service, get_current_user, get_db_session, get_optional_user, CONNECTOR_TYPES, is_connector_access_policy_enforced, is_connector_allowed_for_request, and User remain correctly imported after the reorder to avoid E402/F401 errors.
🧹 Nitpick comments (3)
frontend/components/dev-role-toggle.tsx (1)
63-68: 💤 Low valueConsider parallelizing independent invalidations.
The
invalidateQueriescalls (lines 64 and 66) are independent and could run concurrently. While the current sequential approach is safe, wrapping them inPromise.allwould improve perceived responsiveness.⚡ Proposed refactor
onSuccess: async (data) => { await refreshPermissions(); - await queryClient.invalidateQueries({ queryKey: ["connectors"] }); - await queryClient.invalidateQueries({ - queryKey: ["connector-user-access"], - }); + await Promise.all([ + queryClient.invalidateQueries({ queryKey: ["connectors"] }), + queryClient.invalidateQueries({ queryKey: ["connector-user-access"] }), + ]); router.refresh();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/dev-role-toggle.tsx` around lines 63 - 68, The two independent calls to queryClient.invalidateQueries for queryKey ["connectors"] and ["connector-user-access"] should run in parallel to reduce latency: replace the sequential awaits with a single await Promise.all([queryClient.invalidateQueries({ queryKey: ["connectors"] }), queryClient.invalidateQueries({ queryKey: ["connector-user-access"] })]) while keeping refreshPermissions awaited before and router.refresh() after, referencing refreshPermissions, queryClient.invalidateQueries and router.refresh to locate the changes.src/api/users.py (1)
129-167: ⚡ Quick winAdd an explicit response model for
POST /users/me/dev-role.This endpoint returns structured JSON but doesn’t declare
response_model, so schema drift won’t be caught and docs are less reliable.Suggested refactor
+class DevRoleResponse(BaseModel): + roles: list[str] + role: str + permissions: list[str] + -@router.post("/me/dev-role") +@router.post("/me/dev-role", response_model=DevRoleResponse) async def set_my_dev_role( @@ - return JSONResponse( - { - "roles": roles, - "role": body.role, - "permissions": sorted(perms), - } - ) + return DevRoleResponse( + roles=roles, + role=body.role, + permissions=sorted(perms), + )As per coding guidelines,
src/api/**/*.pyroutes should verify response model typing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/users.py` around lines 129 - 167, The endpoint set_my_dev_role should declare an explicit Pydantic response model to keep OpenAPI/docs and schema validation correct: create a response model (e.g., DevRoleResponse) with fields roles: List[str], role: str, permissions: List[str] and add response_model=DevRoleResponse to the `@router.post` decorator on set_my_dev_role; ensure the handler returns a dict or model instance that matches that schema (adjust the JSONResponse usage or return the model directly) and add any necessary imports for the new DevRoleResponse type.tests/unit/services/test_dev_role_toggle.py (1)
55-56: ⚡ Quick winInclude the
"user"switch case in the parameterized test.The feature supports switching to any built-in role; adding
"user"closes the remaining branch in the happy-path matrix.Suggested refactor
-@pytest.mark.parametrize("target", ["admin", "developer", "viewer"]) +@pytest.mark.parametrize("target", ["admin", "developer", "user", "viewer"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/test_dev_role_toggle.py` around lines 55 - 56, Update the parameterized test decorator for test_switch_to_any_builtin_role to include the "user" role so the test covers all built-in roles; modify the `@pytest.mark.parametrize`("target", ["admin", "developer", "viewer"]) declaration in test_switch_to_any_builtin_role to also contain "user" to exercise that branch of the happy-path matrix.
🤖 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 `@frontend/lib/fetch-server.ts`:
- Around line 15-23: The fetch currently always sends "X-OpenRAG-Brand" using
resolveBrand, causing a default header even when no brand cookie exists; change
this so you read the raw brand cookie via cookieStore.get(BRAND_COOKIE) and only
add the "X-OpenRAG-Brand" header when that cookie value is present (do not call
resolveBrand to supply a fallback), updating the headers building in the fetch
call to conditionally include the header based on the cookie existence.
In `@src/api/connectors.py`:
- Around line 33-49: The connector access policy is only enforced in some
handlers and can be bypassed via other connector routes; ensure all
connector-related endpoints call the central check _connector_access_denied (or
use a middleware/decorator that invokes it) before performing any connector
action. Update the token, browse, disconnect, preview and any other connector
route handlers to call await _connector_access_denied(request, session, user,
connector_type) and return its JSONResponse if non-None, or implement a
request-level middleware that uses is_connector_access_policy_enforced and
is_connector_allowed_for_request to reject disallowed connector_type values with
a 403; keep the existing error message format and status code when rejecting.
In `@src/services/connector_access_service.py`:
- Around line 51-59: Currently src/services/connector_access_service.py reads
IBM_AUTH_ENABLED directly via os.getenv and assigns it to ibm_auth; move this
env read into config/settings.py as a typed setting (e.g., IBM_AUTH_ENABLED:
bool or is_connector_access_policy_enforced: bool) and then replace the
os.getenv usage by importing and using that setting (referencing the ibm_auth
variable and the config setting) so connector_access_service consumes the
centralized, typed config instead of reading os.environ itself.
In `@src/services/dev_role_toggle.py`:
- Around line 49-50: rbac.invalidate is being called before the caller's write
transaction commits, allowing a race that can repopulate stale permissions; move
the invalidation to run only after the DB commit completes. Either register an
on_commit/post_commit callback from within role_repo.assign_role (or return a
token and have the caller call rbac.invalidate after commit) or use your DB
session's after_commit hook to call rbac.invalidate(db_user_id) (target.id can
remain for logging) so invalidation happens post-commit and not inside the
pre-commit window.
---
Outside diff comments:
In `@src/api/auth.py`:
- Around line 1-26: Reorder and clean up imports in auth module to satisfy
linters: remove the unused typing.Optional import and reorder imports so
standard library imports come first, then third-party (fastapi, pydantic,
sqlalchemy), then local project imports; ensure symbols referenced like
get_logger, OPENRAG_VERSION, BaseModel, AsyncSession, get_auth_service,
get_current_user, get_db_session, get_optional_user, CONNECTOR_TYPES,
is_connector_access_policy_enforced, is_connector_allowed_for_request, and User
remain correctly imported after the reorder to avoid E402/F401 errors.
---
Nitpick comments:
In `@frontend/components/dev-role-toggle.tsx`:
- Around line 63-68: The two independent calls to queryClient.invalidateQueries
for queryKey ["connectors"] and ["connector-user-access"] should run in parallel
to reduce latency: replace the sequential awaits with a single await
Promise.all([queryClient.invalidateQueries({ queryKey: ["connectors"] }),
queryClient.invalidateQueries({ queryKey: ["connector-user-access"] })]) while
keeping refreshPermissions awaited before and router.refresh() after,
referencing refreshPermissions, queryClient.invalidateQueries and router.refresh
to locate the changes.
In `@src/api/users.py`:
- Around line 129-167: The endpoint set_my_dev_role should declare an explicit
Pydantic response model to keep OpenAPI/docs and schema validation correct:
create a response model (e.g., DevRoleResponse) with fields roles: List[str],
role: str, permissions: List[str] and add response_model=DevRoleResponse to the
`@router.post` decorator on set_my_dev_role; ensure the handler returns a dict or
model instance that matches that schema (adjust the JSONResponse usage or return
the model directly) and add any necessary imports for the new DevRoleResponse
type.
In `@tests/unit/services/test_dev_role_toggle.py`:
- Around line 55-56: Update the parameterized test decorator for
test_switch_to_any_builtin_role to include the "user" role so the test covers
all built-in roles; modify the `@pytest.mark.parametrize`("target", ["admin",
"developer", "viewer"]) declaration in test_switch_to_any_builtin_role to also
contain "user" to exercise that branch of the happy-path matrix.
🪄 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: ea8e8b32-ff61-4d87-ac27-f4da2bc9bec1
📒 Files selected for processing (30)
.env.examplefrontend/app/api/[...path]/route.tsfrontend/app/api/mutations/useUpdateConnectorAccessMutation.tsfrontend/app/api/queries/useGetConnectorAccessQuery.tsfrontend/app/api/queries/useGetConnectorsQuery.tsfrontend/app/settings/[tab]/page.tsxfrontend/app/settings/_components/connector-access-section.tsxfrontend/app/settings/_components/settings-nav.tsxfrontend/components/dev-role-toggle.tsxfrontend/components/header.tsxfrontend/contexts/auth-context.tsxfrontend/contexts/brand-context.tsxfrontend/lib/brand.tsfrontend/lib/fetch-server.tsfrontend/lib/settings-tab-access.tssrc/api/auth.pysrc/api/connectors.pysrc/api/users.pysrc/app/routes/internal.pysrc/config/settings.pysrc/db/migrations_runtime.pysrc/db/seed.pysrc/dependencies.pysrc/services/connector_access_service.pysrc/services/dev_role_toggle.pytests/unit/api/test_auth_init_connector_access.pytests/unit/db/migrations/test_rbac_catalog_sync.pytests/unit/services/test_connector_access_service.pytests/unit/services/test_dev_role_toggle.pytests/unit/services/test_rbac_service.py
github-actions
Bot
added
enhancement
mfortman11
requested changes
Jun 8, 2026
Comment on lines
+103
to
+107
| const brand = request.cookies.get(BRAND_COOKIE)?.value; | ||
| if (brand === "oss" || brand === "ibm") { | ||
| headers.set("X-OpenRAG-Brand", brand); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
why are we setting a new cookie for this?
| # Connector type not found or validation not needed | ||
| pass | ||
|
|
||
| if denied := await _connector_access_denied(request, session, connector_type): |
Contributor
There was a problem hiding this comment.
This check is happening after the handle_webhook_validation it should be before
github-actions
Bot
added
enhancement
Add admin Connectors Permission (workspace_config-backed) with RBAC connectors:manage:access, enforce policy on connector APIs and OAuth init, and keep the permission list independent of the live connectors tab. Consolidate frontend connector-access hooks into useGetConnectorsQuery and settings tab helpers into brand.ts. Fix Connectors Permission tab redirects (RSC dev-brand default, wait for permissionsResolved). Let explicitly enabled connectors override deployment visibility filters; add OPENRAG_DEV_CONNECTOR_POLICY for local OSS dev backend enforcement Snapshot and restore all cached connector query keys in connect/disconnect mutations so optimistic updates stay correct when policy context changes mid-mutation (brand toggle, permissions resolving).
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/settings/_components/connector-cards.tsx (1)
33-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't collapse fetch failures into the SaaS "not authorized" empty state.
queryConnectorsdefaults to[], so whenuseGetConnectorsQueryerrors this branch still renders “No connectors available or authorized…”. A transient/api/connectorsfailure will look like a policy denial instead of a load error.💡 Minimal fix
- const { data: queryConnectors = [], isLoading: connectorsLoading } = + const { + data: queryConnectors = [], + isLoading: connectorsLoading, + isError: connectorsError, + } = useGetConnectorsQuery({ enabled: isAuthenticated || isNoAuthMode, }); @@ - if (!connectorsLoading && connectors.length === 0) { + if (!connectorsLoading && !connectorsError && connectors.length === 0) { if (isSaasPolicy) { return ( <p className="text-sm text-muted-foreground"> No connectors available or authorized for your organization. </p> ); }Also applies to: 82-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/settings/_components/connector-cards.tsx` around lines 33 - 36, The component collapses fetch failures into the "not authorized" empty state because data is defaulted to []; remove the default assignment from the useGetConnectorsQuery destructure (do not set data: queryConnectors = []) and instead use the hook's isError/error flags to distinguish failure from an empty list: when useGetConnectorsQuery reports isError (or error) show a load-error UI/message, when !connectorsLoading and data is an empty array show the "no connectors or authorized" message; apply the same change to the other occurrence that uses useGetConnectorsQuery (the block around where queryConnectors is read again).
🤖 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 `@frontend/app/settings/_components/settings-nav.tsx`:
- Around line 57-68: The redirect effect (useEffect) currently only reacts to
tabIsVisible and fallbackTab so navigating between two hidden tabs can leave the
effect stale; include the routed-tab identifier in the dependency array (the
variable that represents the current route tab — e.g., the routed tab value or
route.query.tab / currentTab) so the effect reruns when the user navigates to a
different routed tab; update the dependency list that includes isLoading,
permissionsResolved, tabIsVisible, fallbackTab, visibleTabKey, router to also
include the routed-tab variable (or replace visibleTabKey with the actual
routed-tab variable if visibleTabKey isn't the routed value) so
router.replace(`/settings/${fallbackTab}`) fires correctly.
In `@tests/unit/api/test_connectors_connector_access.py`:
- Around line 45-54: The test fixtures _FakeRequest and _FakeWebhookRequest
currently define mutable dicts as class attributes (headers, query_params) which
can be shared across tests; change them to per-instance attributes by adding
__init__ constructors that assign fresh dicts to self.headers and
self.query_params (and keep method as an instance attribute for
_FakeWebhookRequest if needed) so each test gets its own independent dicts and
mutations cannot leak between tests.
---
Outside diff comments:
In `@frontend/app/settings/_components/connector-cards.tsx`:
- Around line 33-36: The component collapses fetch failures into the "not
authorized" empty state because data is defaulted to []; remove the default
assignment from the useGetConnectorsQuery destructure (do not set data:
queryConnectors = []) and instead use the hook's isError/error flags to
distinguish failure from an empty list: when useGetConnectorsQuery reports
isError (or error) show a load-error UI/message, when !connectorsLoading and
data is an empty array show the "no connectors or authorized" message; apply the
same change to the other occurrence that uses useGetConnectorsQuery (the block
around where queryConnectors is read again).
🪄 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: 54675c31-3697-4a56-b4b7-50e405c5a6e1
📒 Files selected for processing (26)
.env.examplefrontend/app/api/mutations/useConnectConnectorMutation.tsfrontend/app/api/mutations/useDisconnectConnectorMutation.tsfrontend/app/api/queries/useGetConnectorsQuery.tsfrontend/app/settings/[tab]/page.tsxfrontend/app/settings/_components/connector-access-section.tsxfrontend/app/settings/_components/connector-card.tsxfrontend/app/settings/_components/connector-cards.tsxfrontend/app/settings/_components/settings-nav.tsxfrontend/components/dev-role-toggle.tsxfrontend/contexts/auth-context.tsxfrontend/contexts/brand-context.tsxfrontend/hooks/use-permissions.tsfrontend/lib/brand.tssrc/api/auth.pysrc/api/connectors.pysrc/api/users.pysrc/app/routes/internal.pysrc/config/settings.pysrc/dependencies.pysrc/services/connector_access_service.pysrc/services/dev_role_toggle.pytests/unit/api/test_auth_init_connector_access.pytests/unit/api/test_connectors_connector_access.pytests/unit/services/test_connector_access_service.pytests/unit/services/test_dev_role_toggle.py
💤 Files with no reviewable changes (3)
- src/dependencies.py
- src/services/dev_role_toggle.py
- tests/unit/services/test_dev_role_toggle.py
✅ Files skipped from review due to trivial changes (1)
- frontend/app/settings/_components/connector-card.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/api/test_auth_init_connector_access.py
- src/app/routes/internal.py
- src/api/auth.py
- src/api/users.py
- frontend/components/dev-role-toggle.tsx
- src/api/connectors.py
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
mfortman11
approved these changes
Jun 9, 2026
Comment on lines
+365
to
+373
| # Local/dev only: enables POST /users/me/dev-role and the header role toggle | ||
| # (pair with NEXT_PUBLIC_IBM_THEME_DEV=true). Never enable in production. | ||
| # OPENRAG_DEV_ROLE_TOGGLE=true | ||
|
|
||
| # Local OSS dev: enforce workspace connector policy on the backend (pair with | ||
| # NEXT_PUBLIC_IBM_THEME_DEV=true). Without this, the UI filters connectors but | ||
| # connect/OAuth routes ignore Connectors Permission until run mode is saas. | ||
| # OPENRAG_DEV_CONNECTOR_POLICY=true | ||
|
|
Contributor
There was a problem hiding this comment.
Let's avoid putting mentions of dev only envs in the env.example especially for anything IBM specific
github-actions
Bot
added
enhancement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce admin Connectors Permission settings (cloud-only) backed by workspace_config, enforce availability on connector APIs and OAuth init, and sync the new connectors:manage:access permission at startup without an Alembic migration. OSS/dev OSS brand bypasses policy; add dev role toggle and brand-aware proxy headers for local RBAC testing.
Screen.Recording.2026-06-05.at.5.13.10.PM.mov
Summary by CodeRabbit
New Features
Documentation