fix(credentials): let workspace admins disconnect a teammate's OAuth credential (#6737)

* fix(credentials): let workspace admins disconnect a teammate's OAuth credential

Disconnecting an OAuth credential routed through POST /api/auth/oauth/disconnect,
a credential *user* operation scoped to the acting user's own `account` rows. A
workspace or org admin acting on a teammate's connection matched no accounts, so
the call returned `{ credentials: [] }` and the route still answered 200 — the UI
navigated away and the credential was still there. Not a denial, a silent no-op.

Route every type through the workspace-scoped credential delete instead, which
authorizes against credential admin and already resolves workspace and org admins
as derived credential admins for shared credential types.

That path only deleted the `credential` row, so send `oauth` through
`deleteCredentialRecord` — the manager that also tears down a credential's secret
source — and teach it to revoke the backing `account` grant once no credential
references it. Scoped by account id, not owner: the caller is already authorized
against the credential, and the grant belongs to the teammate.

Also make RoleLockTooltip layout-transparent. It wrapped locked controls in an
`inline-flex` div, which let the chip shrink to its label while unwrapped
controls stretched to the member row's fixed role track — so a credential's own
members list rendered Admin at two different widths. A `grid` wrapper stretches
like the unwrapped control, aligning the credential, secrets, and skills member
lists that share the row.

* refactor(credentials): consolidate credential deletion onto one path

Follow-ups from review of the previous commit.

- Collapse the delete use case's remaining `service_account` carve-out. Both
  ternary arms reached the same `deleteConnectionCredential` tail, but the
  carve-out skipped `deleteCredentialRecord`'s Slack custom-bot guard — and
  custom bots are exactly the type it guards, so the single-delete surface
  could orphan a credential group that the batch path refuses to.
- Make `deleteOrphanedOAuthAccount` one conditional statement instead of a
  read then a write. `credential.accountId` is ON DELETE CASCADE, so a
  credential racing the gap would have been reaped by Postgres without
  `clearCredentialRefs` running, stranding its id in workflow state.
- Give oauth its own branch in `deleteCredentialRecord`, matching the shape
  the env types already use, rather than a conditional tail after the return.
- Point `handleReconnectCredential` at the shared helper; it carried its own
  copy of the same orphan-grant rule.
- Invalidate the OAuth connections query on credential delete. The removed
  disconnect hook owned that invalidation, and the detail page reads it. This
  also retires the hand-dispatched `oauth-credentials-updated` event from the
  delete path; the connect/reconnect path still dispatches it for its listener.
- Delete `useDisconnectOAuthService`, now callerless.
- Trim comments to the behavior rather than the bug that motivated it.
This commit is contained in:
Waleed
2026-08-15 14:09:01 -07:00
committed by GitHub
parent e3b428e502
commit 03ba95ad5d
10 changed files with 156 additions and 123 deletions
@@ -46,7 +46,6 @@ import {
} from '@/hooks/queries/credentials'
import {
useConnectOAuthService,
useDisconnectOAuthService,
useOAuthConnections,
} from '@/hooks/queries/oauth/oauth-connections'
import { useOAuthReturnRouter } from '@/hooks/use-oauth-return'
@@ -74,7 +73,6 @@ export function ConnectedCredentialDetail({
const { data: oauthConnections = [] } = useOAuthConnections()
const connectOAuthService = useConnectOAuthService()
const disconnectOAuthService = useDisconnectOAuthService()
const createDraft = useCreateCredentialDraft()
const deleteCredential = useDeleteWorkspaceCredential()
@@ -146,30 +144,15 @@ export function ConnectedCredentialDetail({
}
}
/**
* Every credential type disconnects through the workspace-scoped credential
* delete, which authorizes against credential admin — explicit members and
* derived workspace admins alike.
*/
const handleConfirmDelete = async () => {
if (!credential) return
try {
if (credential.type === 'service_account') {
await deleteCredential.mutateAsync(credential.id)
} else {
if (!credential.accountId || !credential.providerId) {
toast.error("Can't disconnect", {
description: 'Missing account information. Try reconnecting this credential first.',
})
return
}
await disconnectOAuthService.mutateAsync({
provider: credential.providerId.split('-')[0] || credential.providerId,
providerId: credential.providerId,
serviceId: credential.providerId,
accountId: credential.accountId,
})
window.dispatchEvent(
new CustomEvent('oauth-credentials-updated', {
detail: { providerId: credential.providerId, workspaceId },
})
)
}
await deleteCredential.mutateAsync(credential.id)
setShowDeleteConfirmDialog(false)
router.push(integrationsHref)
} catch (error) {
@@ -207,7 +190,7 @@ export function ConnectedCredentialDetail({
</Chip>
<Chip
onClick={() => setShowDeleteConfirmDialog(true)}
disabled={disconnectOAuthService.isPending || deleteCredential.isPending}
disabled={deleteCredential.isPending}
>
Disconnect
</Chip>
@@ -303,7 +286,7 @@ export function ConnectedCredentialDetail({
confirm={{
label: 'Disconnect',
onClick: handleConfirmDelete,
pending: disconnectOAuthService.isPending || deleteCredential.isPending,
pending: deleteCredential.isPending,
pendingLabel: 'Disconnecting...',
}}
/>
@@ -54,6 +54,9 @@ interface RoleLockTooltipProps {
/**
* Wraps a disabled role control in a tooltip explaining why the role is fixed.
* Renders children unchanged when there is no lock reason.
*
* The trigger is a `grid` so the wrapper stays layout-transparent; a
* shrink-to-content wrapper would size locked and unlocked rows differently.
*/
export function RoleLockTooltip({ reason, children }: RoleLockTooltipProps) {
if (!reason) return <>{children}</>
@@ -61,7 +64,7 @@ export function RoleLockTooltip({ reason, children }: RoleLockTooltipProps) {
return (
<Tooltip.Root>
<Tooltip.Trigger asChild>
<div className='inline-flex'>{children}</div>
<div className='grid'>{children}</div>
</Tooltip.Trigger>
<Tooltip.Content>{reason}</Tooltip.Content>
</Tooltip.Root>
+2
View File
@@ -20,6 +20,7 @@ import {
type WorkspaceCredentialType,
} from '@/lib/api/contracts'
import { environmentKeys } from '@/hooks/queries/environment'
import { oauthConnectionsKeys } from '@/hooks/queries/oauth/oauth-connections'
import { workspaceCredentialKeys } from '@/hooks/queries/utils/credential-keys'
import {
fetchWorkspaceCredentialList,
@@ -211,6 +212,7 @@ export function useDeleteWorkspaceCredential() {
queryClient.invalidateQueries({ queryKey: workspaceCredentialKeys.lists() })
queryClient.invalidateQueries({ queryKey: OAUTH_CREDENTIALS_KEY })
queryClient.invalidateQueries({ queryKey: environmentKeys.all })
queryClient.invalidateQueries({ queryKey: oauthConnectionsKeys.connections() })
},
})
}
@@ -3,7 +3,6 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
import { requestJson } from '@/lib/api/client/request'
import {
type ConnectedAccount,
disconnectOAuthContract,
listOAuthConnectionsContract,
type OAuthAccountSummary,
type OAuthConnection,
@@ -199,68 +198,5 @@ export function useConnectOAuthService() {
})
}
interface DisconnectServiceParams {
provider: string
providerId?: string
serviceId: string
accountId?: string
}
/**
* Disconnects an OAuth service account.
* Performs optimistic update and rolls back on failure.
*/
export function useDisconnectOAuthService() {
const queryClient = useQueryClient()
return useMutation({
mutationFn: async ({ provider, providerId, accountId }: DisconnectServiceParams) => {
return requestJson(disconnectOAuthContract, {
body: {
provider,
providerId,
accountId,
},
})
},
onMutate: async ({ serviceId, accountId }) => {
await queryClient.cancelQueries({ queryKey: oauthConnectionsKeys.connections() })
const previousServices = queryClient.getQueryData<ServiceInfo[]>(
oauthConnectionsKeys.connections()
)
if (previousServices) {
queryClient.setQueryData<ServiceInfo[]>(
oauthConnectionsKeys.connections(),
previousServices.map((svc) => {
if (svc.id === serviceId) {
const updatedAccounts =
accountId && svc.accounts ? svc.accounts.filter((acc) => acc.id !== accountId) : []
return {
...svc,
accounts: updatedAccounts,
isConnected: updatedAccounts.length > 0,
}
}
return svc
})
)
}
return { previousServices }
},
onError: (_err, _variables, context) => {
if (context?.previousServices) {
queryClient.setQueryData(oauthConnectionsKeys.connections(), context.previousServices)
}
logger.error('Failed to disconnect service')
},
onSettled: () => {
queryClient.invalidateQueries({ queryKey: oauthConnectionsKeys.connections() })
},
})
}
/** Connected OAuth account for a specific provider. */
export type { ConnectedAccount }
@@ -13,7 +13,6 @@ const mocks = vi.hoisted(() => ({
requireProvider: vi.fn(),
getCredential: vi.fn(),
getActor: vi.fn(),
delete: vi.fn(),
deleteRecord: vi.fn(),
capture: vi.fn(),
}))
@@ -29,7 +28,6 @@ vi.mock('@sim/platform-authz/workspace', () => ({
}))
vi.mock('@/lib/credentials/orchestration', () => ({
createServiceAccountCredential: mocks.create,
deleteConnectionCredential: mocks.delete,
deleteCredentialRecord: mocks.deleteRecord,
}))
vi.mock('@/lib/credentials/application/provider-catalog', () => ({
@@ -96,7 +94,6 @@ describe('credential service-account application operations', () => {
auditMetadata: { tenantId: 'tenant-1' },
})
mocks.listCatalog.mockResolvedValue([{ providerId: 'zoom-service-account' }])
mocks.delete.mockResolvedValue(true)
mocks.deleteRecord.mockResolvedValue(true)
mocks.requireProvider.mockReturnValue({
type: 'service_account',
@@ -196,7 +193,7 @@ describe('credential service-account application operations', () => {
code: 'forbidden',
detailCode: 'CREDENTIAL_ADMIN_ACCESS_REQUIRED',
})
expect(mocks.delete).not.toHaveBeenCalled()
expect(mocks.deleteRecord).not.toHaveBeenCalled()
})
it('rejects workspace keys before canonical loading on disconnect', async () => {
@@ -215,7 +212,7 @@ describe('credential service-account application operations', () => {
})
expect(mocks.loadWorkspace).not.toHaveBeenCalled()
expect(mocks.getActor).not.toHaveBeenCalled()
expect(mocks.delete).not.toHaveBeenCalled()
expect(mocks.deleteRecord).not.toHaveBeenCalled()
})
it('allows an explicit credential admin with workspace read access to disconnect', async () => {
@@ -227,7 +224,7 @@ describe('credential service-account application operations', () => {
input: { workspaceId: WORKSPACE_ID, credentialId: credential.id },
})
).resolves.toEqual({ credential, deleted: true })
expect(mocks.delete).toHaveBeenCalledOnce()
expect(mocks.deleteRecord).toHaveBeenCalledOnce()
})
it('applies credential admin policy during authorization-only checks', async () => {
@@ -237,7 +234,7 @@ describe('credential service-account application operations', () => {
})
expect(mocks.getActor).toHaveBeenCalledWith(credential.id, principal.userId)
expect(mocks.delete).not.toHaveBeenCalled()
expect(mocks.deleteRecord).not.toHaveBeenCalled()
})
it('enforces personal-key workspace policy before credential authorization', async () => {
@@ -253,7 +250,7 @@ describe('credential service-account application operations', () => {
detailCode: 'PERSONAL_API_KEYS_DISABLED',
})
expect(mocks.getActor).not.toHaveBeenCalled()
expect(mocks.delete).not.toHaveBeenCalled()
expect(mocks.deleteRecord).not.toHaveBeenCalled()
})
it('disconnects an administered credential', async () => {
@@ -263,15 +260,38 @@ describe('credential service-account application operations', () => {
})
expect(result).toEqual({ credential, deleted: true })
expect(mocks.delete).toHaveBeenCalledWith({
credentialId: credential.id,
workspaceId: WORKSPACE_ID,
expect(mocks.deleteRecord).toHaveBeenCalledWith({ credential, reason: 'user_delete' })
})
it('deletes every type through the record manager so secret sources are torn down', async () => {
const oauthCredential = {
...credential,
type: 'oauth',
providerId: 'google-email',
accountId: 'acct-1',
}
mocks.getCredential.mockResolvedValue(oauthCredential)
mocks.getActor.mockResolvedValue({
credential: oauthCredential,
member: { role: 'admin' },
hasWorkspaceAccess: true,
isAdmin: true,
})
const result = await deleteCredentialUseCase.execute({
principal,
input: { workspaceId: WORKSPACE_ID, credentialId: oauthCredential.id },
})
expect(result).toEqual({ credential: oauthCredential, deleted: true })
expect(mocks.deleteRecord).toHaveBeenCalledWith({
credential: oauthCredential,
reason: 'user_delete',
})
})
it('treats a concurrent disconnect as an idempotent success', async () => {
mocks.delete.mockResolvedValue(false)
mocks.deleteRecord.mockResolvedValue(false)
const result = await deleteCredentialUseCase.execute({
principal,
@@ -15,7 +15,6 @@ import {
import {
type CreateServiceAccountCredentialParams,
createServiceAccountCredential,
deleteConnectionCredential,
deleteCredentialRecord,
} from '@/lib/credentials/orchestration'
import type { CredentialRow } from '@/lib/credentials/queries'
@@ -159,14 +158,7 @@ export const deleteCredentialUseCase = defineAuthorizedCredentialUseCase({
)
}
const reason = principal.kind === 'delegated' ? 'copilot_delete' : 'user_delete'
const deleted =
context.credential.type === 'oauth' || context.credential.type === 'service_account'
? await deleteConnectionCredential({
credentialId: context.credential.id,
workspaceId: context.workspaceId,
reason,
})
: await deleteCredentialRecord({ credential: context.credential, reason })
const deleted = await deleteCredentialRecord({ credential: context.credential, reason })
return { credential: context.credential, deleted }
},
projectAudit: ({ principal, result }) => {
+34 -1
View File
@@ -2,7 +2,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
import { db } from '@sim/db'
import * as schema from '@sim/db/schema'
import { createLogger } from '@sim/logger'
import { and, eq, or, sql } from 'drizzle-orm'
import { and, eq, notExists, or, sql } from 'drizzle-orm'
import type { NextRequest } from 'next/server'
import { CREDENTIAL_SUBBLOCK_IDS } from '@/lib/workflows/persistence/utils'
@@ -101,6 +101,39 @@ export async function deleteConnectionCredential(
return deleted.length === 1
}
/**
* Deletes the stored OAuth grant behind a credential once nothing references
* it. The `account` row is an OAuth credential's secret source, so leaving it
* behind keeps a usable grant; nothing is revoked provider-side.
*
* Scoped by `accountId`, not by owner — the caller is already authorized
* against the credential, which may belong to another user.
*
* The reference check is a predicate on the delete: `credential.accountId` is
* `ON DELETE CASCADE`, so a credential racing a separate check would be reaped
* by Postgres without {@link clearCredentialRefs} ever running.
*/
export async function deleteOrphanedOAuthAccount(accountId: string): Promise<void> {
const deleted = await db
.delete(schema.account)
.where(
and(
eq(schema.account.id, accountId),
notExists(
db
.select({ referenced: sql`1` })
.from(schema.credential)
.where(eq(schema.credential.accountId, accountId))
)
)
)
.returning({ id: schema.account.id })
if (deleted.length > 0) {
logger.info('Deleted orphaned OAuth account', { accountId })
}
}
/**
* Clears stored references to a credential across mutable workspace state
* (editor blocks, copilot checkpoints, knowledge connectors) and frozen
+2 -10
View File
@@ -4,6 +4,7 @@ import * as schema from '@sim/db/schema'
import { createLogger } from '@sim/logger'
import { generateId } from '@sim/utils/id'
import { and, eq, sql } from 'drizzle-orm'
import { deleteOrphanedOAuthAccount } from '@/lib/credentials/deletion'
import { clearDeadFlag } from '@/lib/oauth/terminal-errors'
import { captureServerEvent } from '@/lib/posthog/server'
@@ -184,15 +185,6 @@ export async function handleReconnectCredential(params: {
})
if (oldAccountId) {
const [stillReferenced] = await db
.select({ id: schema.credential.id })
.from(schema.credential)
.where(eq(schema.credential.accountId, oldAccountId))
.limit(1)
if (!stillReferenced) {
await db.delete(schema.account).where(eq(schema.account.id, oldAccountId))
logger.info('Deleted orphaned account after reconnect', { accountId: oldAccountId })
}
await deleteOrphanedOAuthAccount(oldAccountId)
}
}
@@ -18,6 +18,7 @@ const {
mockIsClientCredentialAccountProviderId,
mockGetClientCredentialAccountDescriptor,
mockDeleteConnectionCredential,
mockDeleteOrphanedOAuthAccount,
} = vi.hoisted(() => ({
mockRecordAudit: vi.fn(),
mockGetCredentialActorContext: vi.fn(),
@@ -28,6 +29,7 @@ const {
// providers must not trigger the stored-blob read for authMethod/username.
mockGetClientCredentialAccountDescriptor: vi.fn(() => undefined),
mockDeleteConnectionCredential: vi.fn(),
mockDeleteOrphanedOAuthAccount: vi.fn(),
}))
vi.mock('@sim/audit', () => ({
@@ -51,6 +53,7 @@ vi.mock('@/lib/credentials/client-credential-accounts/descriptors', () => ({
}))
vi.mock('@/lib/credentials/deletion', () => ({
deleteConnectionCredential: mockDeleteConnectionCredential,
deleteOrphanedOAuthAccount: mockDeleteOrphanedOAuthAccount,
}))
vi.mock('@/lib/credentials/environment', () => ({
deleteWorkspaceEnvCredentials: vi.fn(),
@@ -550,4 +553,57 @@ describe('deleteCredentialRecord', () => {
})
expect(mockDeleteConnectionCredential).not.toHaveBeenCalled()
})
it('revokes the backing OAuth grant of a deleted oauth credential', async () => {
mockDeleteConnectionCredential.mockResolvedValueOnce(true)
const deleted = await deleteCredentialRecord({
credential: {
id: 'cred-1',
workspaceId: 'ws-1',
type: 'oauth',
providerId: 'google-email',
accountId: 'acct-1',
} as never,
reason: 'user_delete',
})
expect(deleted).toBe(true)
expect(mockDeleteOrphanedOAuthAccount).toHaveBeenCalledWith('acct-1')
})
it('leaves the OAuth grant alone when the credential row was already gone', async () => {
mockDeleteConnectionCredential.mockResolvedValueOnce(false)
const deleted = await deleteCredentialRecord({
credential: {
id: 'cred-1',
workspaceId: 'ws-1',
type: 'oauth',
providerId: 'google-email',
accountId: 'acct-1',
} as never,
reason: 'user_delete',
})
expect(deleted).toBe(false)
expect(mockDeleteOrphanedOAuthAccount).not.toHaveBeenCalled()
})
it('does not touch OAuth grants for a service-account credential', async () => {
mockDeleteConnectionCredential.mockResolvedValueOnce(true)
await deleteCredentialRecord({
credential: {
id: 'cred-1',
workspaceId: 'ws-1',
type: 'service_account',
providerId: 'google-service-account',
accountId: 'acct-1',
} as never,
reason: 'user_delete',
})
expect(mockDeleteOrphanedOAuthAccount).not.toHaveBeenCalled()
})
})
@@ -24,7 +24,11 @@ import {
getClientCredentialAccountDescriptor,
isClientCredentialAccountProviderId,
} from '@/lib/credentials/client-credential-accounts/descriptors'
import { type CredentialDeleteReason, deleteConnectionCredential } from '@/lib/credentials/deletion'
import {
type CredentialDeleteReason,
deleteConnectionCredential,
deleteOrphanedOAuthAccount,
} from '@/lib/credentials/deletion'
import { slackCustomBotDisplayName } from '@/lib/credentials/display-name'
import {
deleteWorkspaceEnvCredentials,
@@ -554,6 +558,18 @@ export async function deleteCredentialRecord(
return true
}
if (credentialRow.type === 'oauth') {
const deleted = await deleteConnectionCredential({
credentialId: credentialRow.id,
workspaceId: credentialRow.workspaceId,
reason: params.reason,
})
if (deleted && credentialRow.accountId) {
await deleteOrphanedOAuthAccount(credentialRow.accountId)
}
return deleted
}
return deleteConnectionCredential({
credentialId: credentialRow.id,
workspaceId: credentialRow.workspaceId,