feat: Workspace OAuth connector overrides + UI#2040
feat: Workspace OAuth connector overrides + UI#2040edwinjosechittilappilly wants to merge 3 commits into
Conversation
Introduce workspace-level OAuth client credential overrides and admin UI. Adds a new service (src/services/connector_oauth_config_service.py) that stores/decrypts overrides, maintains an in-process cache warmed at startup, and exposes helper functions for BaseConnector resolution. Registers new internal API routes (/connectors/oauth-config GET/PUT/DELETE) and updates startup to warm the cache. BaseConnector gains OAUTH_CREDENTIAL_KEY and resolution logic for client_id/secret (override -> workspace override -> env). OneDrive/SharePoint share a microsoft_graph key; GoogleDrive/connector now resolves creds via the centralized lookup. AuthService now supports a "test" purpose and a _handle_test_auth flow so admins can run Test Connection without persisting connections. Frontend: add query/mutation hooks for connector OAuth config, update Connector Settings UI to manage credential groups, save/clear overrides, and run Test Connection; update auth callback and connect mutation to handle the test purpose and redirect back with UX to surface results. Also extend StatusBadge with new statuses for credential visibility.
github-actions
Bot
added
frontend
|
React Doctor found 5 new issues in 2 files · 5 warnings · score 83 / 100 (Needs work) · 1 fixed · vs 5 warnings
Reviewed by React Doctor for commit |
github-actions
Bot
added
enhancement
WalkthroughAdds workspace-level OAuth client credential overrides for OAuth-kind connectors, including a backend cache/service, CRUD API endpoints, connector credential resolution changes, a new "test" OAuth purpose/flow, and frontend UI (accordion, status badges, query/mutation hooks) for configuring and testing overrides. ChangesOAuth credential override feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectorAccessSection
participant AuthCallbackPage
participant AuthService
participant ConnectorOAuthConfigService
User->>ConnectorAccessSection: Click "Test Connection"
ConnectorAccessSection->>AuthService: init_oauth(purpose="test")
AuthService->>ConnectorOAuthConfigService: get_cached_client_id/secret
AuthService-->>ConnectorAccessSection: OAuth redirect URL
User->>AuthCallbackPage: OAuth provider redirects back
AuthCallbackPage->>AuthService: handle_oauth_callback(purpose="test")
AuthService->>AuthService: _handle_test_auth (delete connection)
AuthService-->>AuthCallbackPage: test_success
AuthCallbackPage-->>ConnectorAccessSection: redirect with oauth_test=success
ConnectorAccessSection-->>User: show success toast
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!cancelled) { | ||
| redirectTimeoutId = setTimeout(() => { | ||
| if (!cancelled) | ||
| router.push( |
There was a problem hiding this comment.
React Doctor · react-doctor/nextjs-no-client-side-redirect (warning)
router.push() in useEffect flashes the wrong page before redirecting.
Fix → Avoid redirects inside useEffect. Use an event handler, middleware, or server-side redirect (App Router: redirect() from next/navigation; Pages Router: getServerSideProps redirect)
| return "not-configured"; | ||
| } | ||
|
|
||
| export function ConnectorAccessSection() { |
There was a problem hiding this comment.
React Doctor · react-doctor/no-giant-component (warning)
Component "ConnectorAccessSection" is 385 lines long, which is hard to read & change. Split it into a few smaller components.
Fix → Pull each section into its own component so the parent is easier to read, test, and change.
| } else { | ||
| toast.error("Test connection failed"); | ||
| } | ||
| router.replace("/settings/connector-access"); |
There was a problem hiding this comment.
React Doctor · react-doctor/nextjs-no-client-side-redirect (warning)
router.replace() in useEffect flashes the wrong page before redirecting.
Fix → Avoid redirects inside useEffect. Use an event handler, middleware, or server-side redirect (App Router: redirect() from next/navigation; Pages Router: getServerSideProps redirect)
| router.replace("/settings/connector-access"); | ||
| // Only re-run when the query params themselves change. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [searchParams]); |
There was a problem hiding this comment.
React Doctor · react-doctor/exhaustive-deps (warning)
useEffect can run with a stale router.replace & show your users old data.
Fix → Don't blindly add missing dependencies. Read the hook callback first.
Bad:
useEffect(() => {
setCount(count + 1);
}, [count]);
Better:
useEffect(() => {
setCount((currentCount) => currentCount + 1);
}, []);
If the missing value is recreated every render, move it inside the hook or stabilize it before adding it to deps.
github-actions
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/connectors/base.py (1)
121-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffEnv-var access outside
config/settings.py.
get_client_id()/get_client_secret()read credentials viaos.getenv(self.CLIENT_ID_ENV_VAR)/os.getenv(self.CLIENT_SECRET_ENV_VAR)(lines 127, 155). Per repo convention,config/settings.pyis the only place that should reados.environ. Since this is a dynamic, per-connector env-var name, routing it through settings is non-trivial, so please confirm whether this should be centralized or is an accepted exception for connector credential resolution.As per path instructions: "Config values must come from config/settings.py (the only place os.environ is read); never access os.environ elsewhere in the codebase."
🤖 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/connectors/base.py` around lines 121 - 159, The OAuth credential lookup in get_client_id() and get_client_secret() is reading environment variables directly via os.getenv, which violates the repo rule that only config/settings.py may access os.environ. Update BaseConnector so these methods obtain the env-backed values through a settings/config helper instead of reading os.getenv themselves, and preserve the current override precedence used with get_cached_client_id() and get_cached_client_secret(). If dynamic env-var names make centralization impossible, refactor the resolution path so the env access still happens only in config/settings.py.Source: Path instructions
🤖 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/connector-access-section.tsx`:
- Around line 231-234: The client_id input in connector-access-section stays
bound to status?.client_id whenever draft.client_id is empty, so clearing the
field visually snaps back to the saved value. Update the value logic in the
connector access form so the input uses the draft state as the source of truth
in edit mode, and only falls back to status?.client_id when no draft exists yet;
use the credentialDrafts[group.credentialKey] draft object and the client_id
input handler to keep an explicit empty string.
In `@src/services/auth_service.py`:
- Around line 142-149: The oauth credential validation in auth_service’s
connector setup assigns client_secret but never uses it, triggering Ruff F841.
Keep the get_client_secret() call in the try block for validation, but stop
binding its return value in the oauth_config/connector_instance flow so only
client_id is stored, and leave the error handling in place for
connector_class_any and the credential fetches.
In `@src/services/connector_oauth_config_service.py`:
- Around line 122-124: The `connector_oauth_config_service` is reading
environment variables directly via `os.getenv` for `CLIENT_ID_ENV_VAR` and
`CLIENT_SECRET_ENV_VAR`, which violates the repo rule that all env access must
go through `config/settings.py`. Move these lookups into `config/settings.py`
(or a settings helper there) and update the service to consume the values from
that settings interface in the `cls`/`_representative_connector_class` flow
instead of calling `os.getenv` directly.
---
Nitpick comments:
In `@src/connectors/base.py`:
- Around line 121-159: The OAuth credential lookup in get_client_id() and
get_client_secret() is reading environment variables directly via os.getenv,
which violates the repo rule that only config/settings.py may access os.environ.
Update BaseConnector so these methods obtain the env-backed values through a
settings/config helper instead of reading os.getenv themselves, and preserve the
current override precedence used with get_cached_client_id() and
get_cached_client_secret(). If dynamic env-var names make centralization
impossible, refactor the resolution path so the env access still happens only in
config/settings.py.
🪄 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: 242ecf33-d183-42d1-b38d-596064c07e35
📒 Files selected for processing (16)
frontend/app/api/mutations/useConnectConnectorMutation.tsfrontend/app/api/mutations/useConnectorOAuthConfigMutation.tsfrontend/app/api/queries/useConnectorOAuthConfigQuery.tsfrontend/app/auth/callback/page.tsxfrontend/app/settings/_components/connector-access-section.tsxfrontend/app/settings/_components/settings-nav.tsxfrontend/components/ui/status-badge.tsxsrc/api/connectors.pysrc/app/routes/internal.pysrc/connectors/base.pysrc/connectors/google_drive/connector.pysrc/connectors/onedrive/connector.pysrc/connectors/sharepoint/connector.pysrc/services/auth_service.pysrc/services/connector_oauth_config_service.pysrc/services/startup_orchestrator.py
| const draft = credentialDrafts[group.credentialKey] ?? { | ||
| client_id: "", | ||
| client_secret: "", | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Client ID field cannot be visually cleared while editing.
draft.client_id defaults to "", and the input falls back to status?.client_id whenever draft.client_id is falsy (line 267). Once a user backspaces the field to empty, the displayed value snaps back to the previously-saved client ID instead of staying blank, making it look like the edit didn't register.
🐛 Proposed fix
const draft = credentialDrafts[group.credentialKey] ?? {
- client_id: "",
+ client_id: status?.client_id ?? "",
client_secret: "",
}; <Input
id={`${group.credentialKey}-client-id`}
- value={draft.client_id || status?.client_id || ""}
+ value={draft.client_id}
onChange={(e) =>
setDraft({ client_id: e.target.value })
}Also applies to: 265-274
🤖 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-access-section.tsx` around lines
231 - 234, The client_id input in connector-access-section stays bound to
status?.client_id whenever draft.client_id is empty, so clearing the field
visually snaps back to the saved value. Update the value logic in the connector
access form so the input uses the draft state as the source of truth in edit
mode, and only falls back to status?.client_id when no draft exists yet; use the
credentialDrafts[group.credentialKey] draft object and the client_id input
handler to keep an explicit empty string.
| try: | ||
| connector_instance = connector_class_any({}) | ||
| client_id = connector_instance.get_client_id() | ||
| client_secret = connector_instance.get_client_secret() | ||
| except (ValueError, NotImplementedError, RuntimeError) as e: | ||
| raise RuntimeError( | ||
| f"Missing OAuth env vars for {connector_class.__name__}. " | ||
| f"Set {client_key} and {secret_key} in the environment." | ||
| ) | ||
| f"Missing OAuth credentials for {connector_class.__name__}: {e}" | ||
| ) from e |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused client_secret fails CI (Ruff F841).
client_secret is assigned but never read (only client_id is returned in oauth_config). The get_client_secret() call is still useful as an existence check, so drop the binding to satisfy the linter while keeping validation.
🔧 Proposed fix
try:
connector_instance = connector_class_any({})
client_id = connector_instance.get_client_id()
- client_secret = connector_instance.get_client_secret()
+ # Resolve the secret to validate it is configured; it is not
+ # returned to the frontend (only client_id is).
+ connector_instance.get_client_secret()
except (ValueError, NotImplementedError, RuntimeError) as e:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| connector_instance = connector_class_any({}) | |
| client_id = connector_instance.get_client_id() | |
| client_secret = connector_instance.get_client_secret() | |
| except (ValueError, NotImplementedError, RuntimeError) as e: | |
| raise RuntimeError( | |
| f"Missing OAuth env vars for {connector_class.__name__}. " | |
| f"Set {client_key} and {secret_key} in the environment." | |
| ) | |
| f"Missing OAuth credentials for {connector_class.__name__}: {e}" | |
| ) from e | |
| try: | |
| connector_instance = connector_class_any({}) | |
| client_id = connector_instance.get_client_id() | |
| # Resolve the secret to validate it is configured; it is not | |
| # returned to the frontend (only client_id is). | |
| connector_instance.get_client_secret() | |
| except (ValueError, NotImplementedError, RuntimeError) as e: | |
| raise RuntimeError( | |
| f"Missing OAuth credentials for {connector_class.__name__}: {e}" | |
| ) from e |
🧰 Tools
🪛 GitHub Actions: autofix.ci / 0_ruff autofix.txt
[error] 145-145: ruff check failed (F841): Local variable client_secret is assigned to but never used. Remove assignment to unused variable client_secret.
🪛 GitHub Actions: autofix.ci / ruff autofix
[error] 145-145: ruff check failed: F841 Local variable client_secret is assigned to but never used. Help: Remove assignment to unused variable client_secret.
🪛 GitHub Actions: Lint Backend / 0_Ruff and mypy on changed files.txt
[error] 145-145: ruff check failed (F841): Local variable client_secret is assigned to but never used.
🪛 GitHub Actions: Lint Backend / Ruff and mypy on changed files
[error] 145-145: Ruff check failed (F841): Local variable client_secret is assigned to but never used.
[error] 145-145: Command failed: uv run ruff check --no-fix --output-format=github src/api/connectors.py src/app/routes/internal.py src/connectors/base.py src/connectors/google_drive/connector.py src/connectors/onedrive/connector.py src/connectors/sharepoint/connector.py src/services/auth_service.py src/services/connector_oauth_config_service.py src/services/startup_orchestrator.py
🪛 GitHub Check: Ruff and mypy on changed files
[failure] 145-145: ruff (F841)
src/services/auth_service.py:145:13: F841 Local variable client_secret is assigned to but never used
help: Remove assignment to unused variable client_secret
🤖 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/services/auth_service.py` around lines 142 - 149, The oauth credential
validation in auth_service’s connector setup assigns client_secret but never
uses it, triggering Ruff F841. Keep the get_client_secret() call in the try
block for validation, but stop binding its return value in the
oauth_config/connector_instance flow so only client_id is stored, and leave the
error handling in place for connector_class_any and the credential fetches.
Source: Linters/SAST tools
| cls = _representative_connector_class(key) | ||
| env_client_id_set = bool(cls and os.getenv(cls.CLIENT_ID_ENV_VAR)) | ||
| env_client_secret_set = bool(cls and os.getenv(cls.CLIENT_SECRET_ENV_VAR)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Env access must route through config/settings.py.
os.getenv(cls.CLIENT_ID_ENV_VAR) / os.getenv(cls.CLIENT_SECRET_ENV_VAR) read os.environ directly inside a service module. Per repo convention, config/settings.py is the only place os.environ is read; expose these values there (or via a settings helper) and import them here instead.
As per path instructions: "Config values must come from config/settings.py (the only place os.environ is read); never access os.environ elsewhere in the codebase."
🤖 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/services/connector_oauth_config_service.py` around lines 122 - 124, The
`connector_oauth_config_service` is reading environment variables directly via
`os.getenv` for `CLIENT_ID_ENV_VAR` and `CLIENT_SECRET_ENV_VAR`, which violates
the repo rule that all env access must go through `config/settings.py`. Move
these lookups into `config/settings.py` (or a settings helper there) and update
the service to consume the values from that settings interface in the
`cls`/`_representative_connector_class` flow instead of calling `os.getenv`
directly.
Source: Path instructions
1 participant
Screen.Recording.2026-07-07.at.1.41.33.PM.mov
Introduce workspace-level OAuth client credential overrides and admin UI. Adds a new service (src/services/connector_oauth_config_service.py) that stores/decrypts overrides, maintains an in-process cache warmed at startup, and exposes helper functions for BaseConnector resolution. Registers new internal API routes (/connectors/oauth-config GET/PUT/DELETE) and updates startup to warm the cache.
BaseConnector gains OAUTH_CREDENTIAL_KEY and resolution logic for client_id/secret (override -> workspace override -> env). OneDrive/SharePoint share a microsoft_graph key; GoogleDrive/connector now resolves creds via the centralized lookup. AuthService now supports a "test" purpose and a _handle_test_auth flow so admins can run Test Connection without persisting connections.
Frontend: add query/mutation hooks for connector OAuth config, update Connector Settings UI to manage credential groups, save/clear overrides, and run Test Connection; update auth callback and connect mutation to handle the test purpose and redirect back with UX to surface results. Also extend StatusBadge with new statuses for credential visibility.
Summary by CodeRabbit
New Features
Bug Fixes