feat(auth-profiles): admin-pinned default app profile per connector#764
Merged
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a connector-scoped default flag for oauth_app auth profiles with DB migration and partial-unique index; surfaces the flag in queries/serialization; adds transactional helpers to set/clear defaults and revoke apps with connection-status cascades; introduces a tool action to set/clear defaults; tightens role checks and connection-creation/reauth validation for non-admins. ChangesDefault OAuth App Profile Pinning
Sequence Diagram(s)sequenceDiagram
participant Caller
participant manageAuthProfiles
participant handleSetDefaultAuthProfile
participant setDefaultAuthProfileForConnector
participant Database
Caller->>manageAuthProfiles: set_default_auth_profile(connector_key, auth_profile_slug|null)
manageAuthProfiles->>handleSetDefaultAuthProfile: dispatch
handleSetDefaultAuthProfile->>Database: validate profile exists, is oauth_app, matches connector, active if pinning
handleSetDefaultAuthProfile->>setDefaultAuthProfileForConnector: transaction to clear siblings and optionally pin slug
setDefaultAuthProfileForConnector->>Database: UPDATE auth_profiles (clear and set is_default_for_connector)
setDefaultAuthProfileForConnector-->>handleSetDefaultAuthProfile: flagged row or null
handleSetDefaultAuthProfile-->>manageAuthProfiles: serialized result
manageAuthProfiles-->>Caller: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fcbe0a8aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+791
to
+792
| } else if (authProfile.status === 'active') { | ||
| await syncConnectionsForOAuthAppProfile(ctx.organizationId, authProfile.id, true); |
There was a problem hiding this comment.
When an oauth_app profile is already active, any update to it (renaming it, changing credentials, etc.) reaches this branch and syncConnectionsForOAuthAppProfile(..., true) sets every non-deleted connection using that app profile to active. That overwrites connection statuses that are intentionally not active — for example connections paused via manage_connections.update(status: 'paused'), revoked/error connections, or OAuth-account connections that are pending_auth because the user account is missing scopes (see syncOAuthConnectionsForAuthProfile). Only a transition from revoked/error back to active should consider repairing connections, and even then it should not blindly override unrelated connection-level status.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/tools/admin/manage_auth_profiles.ts (1)
505-535:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEnforce oauth_account ownership before member reuse/update.
Line 505+ and Line 668+ allow member-tier operations on
oauth_accountprofiles without verifying profile ownership. A member can target another member’s slug and mutate/reconnect that profile.Proposed fix
+async function canManageOAuthAccountProfile( + profile: { profile_kind: string; created_by: string | null }, + ctx: ToolContext +): Promise<boolean> { + if (profile.profile_kind !== 'oauth_account') return false; + const role = ctx.userId ? await getWorkspaceRole(getDb(), ctx.organizationId, ctx.userId) : null; + if (role === 'admin' || role === 'owner') return true; + return !!ctx.userId && profile.created_by === ctx.userId; +}Apply this check before:
- reusing an existing oauth_account in
handleCreateAuthProfile- permitting oauth_account edits in
handleUpdateAuthProfileAlso applies to: 668-683
🤖 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 `@packages/server/src/tools/admin/manage_auth_profiles.ts` around lines 505 - 535, The code reuses or reconnects an existing oauth_account without checking ownership; before allowing reuse in the create flow (the block that inspects existing from getAuthProfileBySlug/normalizeAuthProfileSlug and the later update flow in handleUpdateAuthProfile), validate that the profile is owned by the current user by comparing ctx.userId to the profile owner field (e.g., existing.created_by or existing.owner_id) and return an error if they differ; apply the same ownership check before permitting oauth_account edits in handleUpdateAuthProfile so members cannot target or mutate other members' oauth_account profiles.
🧹 Nitpick comments (1)
packages/server/src/utils/auth-profiles.ts (1)
539-547: ⚡ Quick winHarden connector scoping inside
setDefaultAuthProfileForConnector.Line 543–545 pins by
slug+profile_kindonly. This helper should enforceconnector_key = params.connectorKeyitself, or a future caller can accidentally flag a profile from a different connector.Proposed fix
const rows = await tx` UPDATE auth_profiles SET is_default_for_connector = true, updated_at = NOW() WHERE organization_id = ${params.organizationId} AND slug = ${params.slug} AND profile_kind = 'oauth_app' + AND connector_key = ${params.connectorKey} + AND status = 'active' RETURNING ${tx.unsafe(AUTH_PROFILE_COLUMNS)} `;🤖 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 `@packages/server/src/utils/auth-profiles.ts` around lines 539 - 547, The UPDATE in setDefaultAuthProfileForConnector currently restricts by organization_id, slug and profile_kind only; modify the WHERE clause to also require connector_key = ${params.connectorKey} so the query updates only profiles for the intended connector (i.e., add AND connector_key = ${params.connectorKey} to the tx`...` that updates auth_profiles and returns AUTH_PROFILE_COLUMNS). Ensure the same params.connectorKey symbol is used and adjust any tests if they relied on the broader behavior.
🤖 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 `@packages/server/src/tools/admin/manage_auth_profiles.ts`:
- Around line 410-417: The UPDATE currently flips every dependent connection to
active; restrict it so only connections that actually need reactivation are
changed (e.g., those with statuses like 'reauthorize_required' or 'pending'
instead of all rows). Modify the SQL in manage_auth_profiles.ts (the UPDATE
block that sets status = ${nextConnectionStatus}) to include an additional WHERE
clause such as AND status IN (...list of reactivation-only statuses...), and
update the caller syncConnectionsForOAuthAppProfile(...) usage (the site passing
true on oauth_app updates) to compute and pass the reactivation flag only when
the oauth_app change actually requires reactivating connections (e.g.,
credential rotation or reenable), not for any active update.
---
Outside diff comments:
In `@packages/server/src/tools/admin/manage_auth_profiles.ts`:
- Around line 505-535: The code reuses or reconnects an existing oauth_account
without checking ownership; before allowing reuse in the create flow (the block
that inspects existing from getAuthProfileBySlug/normalizeAuthProfileSlug and
the later update flow in handleUpdateAuthProfile), validate that the profile is
owned by the current user by comparing ctx.userId to the profile owner field
(e.g., existing.created_by or existing.owner_id) and return an error if they
differ; apply the same ownership check before permitting oauth_account edits in
handleUpdateAuthProfile so members cannot target or mutate other members'
oauth_account profiles.
---
Nitpick comments:
In `@packages/server/src/utils/auth-profiles.ts`:
- Around line 539-547: The UPDATE in setDefaultAuthProfileForConnector currently
restricts by organization_id, slug and profile_kind only; modify the WHERE
clause to also require connector_key = ${params.connectorKey} so the query
updates only profiles for the intended connector (i.e., add AND connector_key =
${params.connectorKey} to the tx`...` that updates auth_profiles and returns
AUTH_PROFILE_COLUMNS). Ensure the same params.connectorKey symbol is used and
adjust any tests if they relied on the broader behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78a919ca-ed1c-4223-b330-317eaf73c9b5
📒 Files selected for processing (9)
db/migrations/20260515170000_auth_profiles_default_for_connector.sqldb/schema.sqlpackages/server/src/auth/__tests__/tool-access.test.tspackages/server/src/auth/tool-access.tspackages/server/src/tools/admin/helpers/connection-helpers.tspackages/server/src/tools/admin/manage_auth_profiles.tspackages/server/src/tools/admin/manage_connections.tspackages/server/src/utils/auth-profiles.tspackages/web
Member
Author
pi review passReviewed with Fixed
Deferred (PR #2) |
Adds is_default_for_connector to auth_profiles with a partial unique index keeping at most one default per (org, connector_key) for oauth_app rows. getPrimaryAuthProfileForKind prefers the flagged profile so member-installed connections fall through to the admin's choice instead of "most recent". - New `manage_auth_profiles.set_default_auth_profile` action (admin-only via the existing OWNER_ADMIN_ACTIONS policy table). - Revoke / force-delete on oauth_app now flips dependent connections to pending_auth so admin app rotation doesn't silently break running connections. - Revoking the currently-pinned default also clears the flag. - Bumps packages/web pointer to the new admin settings page + install sheet UX (owletto-web#134).
Opens manage_connections.create + manage_connections.reauthenticate and
manage_auth_profiles.{create,update}_auth_profile to member-write. Handler
gates enforce intent so members can't bring their own org-shared
credentials:
- create_auth_profile / update_auth_profile reject non-oauth_account kinds
for non-admins (env / oauth_app / browser_session remain admin-only).
- manage_connections.create rejects an explicit app_auth_profile_slug from
a non-admin unless the profile is is_default_for_connector for that
connector. Omit the slug and the server auto-resolves via
getPrimaryAuthProfileForKind, which already prefers the admin-pinned
default after the previous commit.
- 'No app credentials' error rewords for non-admins to 'ask an admin to
set up the <provider> app'.
Updates the tool-access policy tests to match the new boundaries.
Bumps packages/web pointer for the OAuth-app indicator on connection
settings (admin picker, member-readable).
- handleCreate (manage_connections): non-admins fall through to getPrimaryAuthProfileForKind, which can return a recency-picked provider-wide row when no default is pinned. Now require the resolved profile to be is_default_for_connector === true for the exact connector_key when caller is non-admin; otherwise reject with the 'ask an admin' message. - handleUpdateAuthProfile: oauth_account ownership check — non-admins can only update profiles they created (created_by === userId), so the slug alone doesn't grant access to another member's tokens. - handleUpdateAuthProfile (revoke path): wrap status flip + cascade + default-clear in a single transaction via revokeOAuthAppProfileAtomic. Removes the window where connections still pointed at a revoked profile. - Bumps packages/web pointer for the non-admin read-only picker.
buremba
force-pushed
the
feat/oauth-app-profiles-admin
branch
from
49cf999 to
9a43247
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/utils/auth-profiles.ts (1)
580-588: 💤 Low valueConsider adding
connector_keyto the UPDATE for defense-in-depth.The handler validates that the profile's
connector_keymatches before calling this function. However, adding the constraint directly in the UPDATE would guard against future callers that might bypass the handler validation.♻️ Proposed change
const rows = await tx` UPDATE auth_profiles SET is_default_for_connector = true, updated_at = NOW() WHERE organization_id = ${params.organizationId} AND slug = ${params.slug} + AND connector_key = ${params.connectorKey} AND profile_kind = 'oauth_app' RETURNING ${tx.unsafe(AUTH_PROFILE_COLUMNS)} `;🤖 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 `@packages/server/src/utils/auth-profiles.ts` around lines 580 - 588, The UPDATE to auth_profiles (in the tx`...` block that currently sets is_default_for_connector for organization_id = ${params.organizationId}, slug = ${params.slug}, and profile_kind = 'oauth_app') should also include the connector_key check for defense-in-depth; add AND connector_key = ${params.connectorKey} to the WHERE clause (and ensure the caller provides params.connectorKey) so the UPDATE only affects the intended oauth_app with the matching connector_key.
🤖 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.
Nitpick comments:
In `@packages/server/src/utils/auth-profiles.ts`:
- Around line 580-588: The UPDATE to auth_profiles (in the tx`...` block that
currently sets is_default_for_connector for organization_id =
${params.organizationId}, slug = ${params.slug}, and profile_kind = 'oauth_app')
should also include the connector_key check for defense-in-depth; add AND
connector_key = ${params.connectorKey} to the WHERE clause (and ensure the
caller provides params.connectorKey) so the UPDATE only affects the intended
oauth_app with the matching connector_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dedb3fe7-07de-403f-8d57-8a44ae7f956a
📒 Files selected for processing (9)
db/migrations/20260515170000_auth_profiles_default_for_connector.sqldb/schema.sqlpackages/server/src/auth/__tests__/tool-access.test.tspackages/server/src/auth/tool-access.tspackages/server/src/tools/admin/helpers/connection-helpers.tspackages/server/src/tools/admin/manage_auth_profiles.tspackages/server/src/tools/admin/manage_connections.tspackages/server/src/utils/auth-profiles.tspackages/web
✅ Files skipped from review due to trivial changes (2)
- packages/web
- packages/server/src/tools/admin/helpers/connection-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- db/migrations/20260515170000_auth_profiles_default_for_connector.sql
- packages/server/src/auth/tool-access.ts
- db/schema.sql
- packages/server/src/auth/tests/tool-access.test.ts
- manage_connections.create: non-admins cannot bind to env profiles or oauth_account/browser_session profiles owned by other users - manage_connections.create: reject entity_link_overrides from non-admins (it mutates connector_definitions org-wide) - create_auth_profile slug-reuse: non-admins must own the existing slug before reconnecting; closes hijack-by-known-slug vector - reauthenticate: require connection-owner or admin; blocks members from disrupting another member's interactive auth - delete_auth_profile: lock profile row + sync + delete in one tx so a concurrent create can't leave an active connection with NULL app_auth_profile_id after FK ON DELETE SET NULL fires
… delete pi second pass flagged two follow-ups on the tx-wrapped delete: - browser_session delete must still pause dependent feeds + clear next_run_at (the previous syncConnectionsForBrowserAuthProfile did this and the tx version had dropped it) - the composite FK is (organization_id, *_auth_profile_id) ON DELETE SET NULL; without an explicit column list Postgres would attempt to null organization_id too (NOT NULL → constraint violation on force delete with active connections). Null the FK column explicitly inside the tx so the DELETE itself never depends on FK SET NULL.
Member
Author
pi review — final summaryThree full pi passes after rebasing onto main. All issues closed. Pass 1 (before rebase): 5 findings, 4 fixed in Pass 2 (after rebase): 5 new findings, all fixed in
Pass 3 (after follow-up fixes in
Pass 4: All closed, no new issues. CI: integration suite is failing on main HEAD too ( |
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
2 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.
Summary
Closes the gap surfaced when an org has multiple OAuth app profiles for a connector: today the resolver picks the most-recently-updated one, so members installing a connection get whichever profile an admin edited last. This PR adds an explicit per-connector default flag plus the UX to manage it.
auth_profiles.is_default_for_connector(NOT NULL, DEFAULT false) + partial unique index keeping at most one default per(organization_id, connector_key)foroauth_approws.getPrimaryAuthProfileForKindprefers the flagged profile before falling back to connector_key-match and recency.manage_auth_profiles.set_default_auth_profile(admin-only via the existingOWNER_ADMIN_ACTIONSpolicy). Passauth_profile_slug: nullto clear the default.oauth_appprofile torevoked/error, dependent connections move topending_authso the breakage surfaces in the UI. Force-deleting a profile does the same. Revoking the pinned default also clears the flag./{org}/oauth-appsadmin settings page, "Org default" pill + auto-pick in the install sheet, "Create new app profile" hidden from non-admins, "ask an admin" hint when no profile exists.Gaps explicitly not addressed here
manage_connections.createis admin-only via the policy table). This PR is forward-compatible with opening that up later.Test plan
bunx dbmate -d db/migrations upagainst a local Postgres; verify the column + index materialise.app_auth_profile_idresolves to the default.pending_authandis_default_for_connectoris cleared./{org}/oauth-apps→ "Admins only" empty state. Open an install sheet with no profiles → "ask an admin" hint, no Create button./{org}/oauth-apps→ list with badges, set/clear default, revoke, delete all work via inline two-step confirms.Summary by CodeRabbit
New Features
Improvements
Tests