Skip to content

Commit 91c9da5

Browse files
committed
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.
1 parent c269e88 commit 91c9da5

7 files changed

Lines changed: 153 additions & 23 deletions

File tree

apps/sim/app/workspace/[workspaceId]/integrations/connected/[credentialId]/connected-credential-detail.tsx

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ import {
4646
} from '@/hooks/queries/credentials'
4747
import {
4848
useConnectOAuthService,
49-
useDisconnectOAuthService,
5049
useOAuthConnections,
5150
} from '@/hooks/queries/oauth/oauth-connections'
5251
import { useOAuthReturnRouter } from '@/hooks/use-oauth-return'
@@ -74,7 +73,6 @@ export function ConnectedCredentialDetail({
7473

7574
const { data: oauthConnections = [] } = useOAuthConnections()
7675
const connectOAuthService = useConnectOAuthService()
77-
const disconnectOAuthService = useDisconnectOAuthService()
7876
const createDraft = useCreateCredentialDraft()
7977
const deleteCredential = useDeleteWorkspaceCredential()
8078

@@ -146,24 +144,18 @@ export function ConnectedCredentialDetail({
146144
}
147145
}
148146

147+
/**
148+
* Every credential type disconnects through the workspace-scoped credential
149+
* delete, which authorizes against credential admin — explicit members and
150+
* derived workspace admins alike. The personal OAuth disconnect is scoped to
151+
* the acting user's own `account` rows, so routing an admin through it
152+
* silently matched nothing when the connection belonged to a teammate.
153+
*/
149154
const handleConfirmDelete = async () => {
150155
if (!credential) return
151156
try {
152-
if (credential.type === 'service_account') {
153-
await deleteCredential.mutateAsync(credential.id)
154-
} else {
155-
if (!credential.accountId || !credential.providerId) {
156-
toast.error("Can't disconnect", {
157-
description: 'Missing account information. Try reconnecting this credential first.',
158-
})
159-
return
160-
}
161-
await disconnectOAuthService.mutateAsync({
162-
provider: credential.providerId.split('-')[0] || credential.providerId,
163-
providerId: credential.providerId,
164-
serviceId: credential.providerId,
165-
accountId: credential.accountId,
166-
})
157+
await deleteCredential.mutateAsync(credential.id)
158+
if (credential.type === 'oauth' && credential.providerId) {
167159
window.dispatchEvent(
168160
new CustomEvent('oauth-credentials-updated', {
169161
detail: { providerId: credential.providerId, workspaceId },
@@ -207,7 +199,7 @@ export function ConnectedCredentialDetail({
207199
</Chip>
208200
<Chip
209201
onClick={() => setShowDeleteConfirmDialog(true)}
210-
disabled={disconnectOAuthService.isPending || deleteCredential.isPending}
202+
disabled={deleteCredential.isPending}
211203
>
212204
Disconnect
213205
</Chip>
@@ -303,7 +295,7 @@ export function ConnectedCredentialDetail({
303295
confirm={{
304296
label: 'Disconnect',
305297
onClick: handleConfirmDelete,
306-
pending: disconnectOAuthService.isPending || deleteCredential.isPending,
298+
pending: deleteCredential.isPending,
307299
pendingLabel: 'Disconnecting...',
308300
}}
309301
/>

apps/sim/components/permissions/role-lock.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,20 @@ interface RoleLockTooltipProps {
5454
/**
5555
* Wraps a disabled role control in a tooltip explaining why the role is fixed.
5656
* Renders children unchanged when there is no lock reason.
57+
*
58+
* The trigger is a `grid` so the wrapper stays layout-transparent: an unwrapped
59+
* control is a direct grid item of the member row and stretches to that row's
60+
* fixed role track, and a lone grid child stretches identically. An
61+
* `inline-flex` wrapper instead let the control shrink to its label, so locked
62+
* and editable rows of the same list rendered at two different widths.
5763
*/
5864
export function RoleLockTooltip({ reason, children }: RoleLockTooltipProps) {
5965
if (!reason) return <>{children}</>
6066

6167
return (
6268
<Tooltip.Root>
6369
<Tooltip.Trigger asChild>
64-
<div className='inline-flex'>{children}</div>
70+
<div className='grid'>{children}</div>
6571
</Tooltip.Trigger>
6672
<Tooltip.Content>{reason}</Tooltip.Content>
6773
</Tooltip.Root>

apps/sim/lib/credentials/application/service-account.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,34 @@ describe('credential service-account application operations', () => {
270270
})
271271
})
272272

273+
it('disconnects an oauth credential through the record manager so its grant is revoked', async () => {
274+
const oauthCredential = {
275+
...credential,
276+
type: 'oauth',
277+
providerId: 'google-email',
278+
accountId: 'acct-1',
279+
}
280+
mocks.getCredential.mockResolvedValue(oauthCredential)
281+
mocks.getActor.mockResolvedValue({
282+
credential: oauthCredential,
283+
member: { role: 'admin' },
284+
hasWorkspaceAccess: true,
285+
isAdmin: true,
286+
})
287+
288+
const result = await deleteCredentialUseCase.execute({
289+
principal,
290+
input: { workspaceId: WORKSPACE_ID, credentialId: oauthCredential.id },
291+
})
292+
293+
expect(result).toEqual({ credential: oauthCredential, deleted: true })
294+
expect(mocks.deleteRecord).toHaveBeenCalledWith({
295+
credential: oauthCredential,
296+
reason: 'user_delete',
297+
})
298+
expect(mocks.delete).not.toHaveBeenCalled()
299+
})
300+
273301
it('treats a concurrent disconnect as an idempotent success', async () => {
274302
mocks.delete.mockResolvedValue(false)
275303

apps/sim/lib/credentials/application/service-account.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,12 @@ export const deleteCredentialUseCase = defineAuthorizedCredentialUseCase({
159159
)
160160
}
161161
const reason = principal.kind === 'delegated' ? 'copilot_delete' : 'user_delete'
162+
// Every type but `service_account` goes through the record manager so it also
163+
// tears down the credential's secret source — for `oauth` that is the backing
164+
// `account` row, which an admin deleting a teammate's connection must revoke
165+
// too, not just the credential that pointed at it.
162166
const deleted =
163-
context.credential.type === 'oauth' || context.credential.type === 'service_account'
167+
context.credential.type === 'service_account'
164168
? await deleteConnectionCredential({
165169
credentialId: context.credential.id,
166170
workspaceId: context.workspaceId,

apps/sim/lib/credentials/deletion.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,35 @@ export async function deleteConnectionCredential(
101101
return deleted.length === 1
102102
}
103103

104+
/**
105+
* Deletes the OAuth grant backing a credential once no credential references it
106+
* any more, and reports whether it went. The `account` row is an OAuth
107+
* credential's secret source, so leaving it behind keeps the provider grant
108+
* alive after the credential it backed is gone.
109+
*
110+
* Scoped by `accountId` rather than by owner: the caller has already been
111+
* authorized against the credential, and the grant may belong to a different
112+
* user than the one deleting it (a workspace admin removing a teammate's
113+
* connection). Returns false when another credential still uses the grant.
114+
*/
115+
export async function deleteOrphanedOAuthAccount(accountId: string): Promise<boolean> {
116+
const [stillReferenced] = await db
117+
.select({ id: schema.credential.id })
118+
.from(schema.credential)
119+
.where(eq(schema.credential.accountId, accountId))
120+
.limit(1)
121+
if (stillReferenced) return false
122+
123+
const deleted = await db
124+
.delete(schema.account)
125+
.where(eq(schema.account.id, accountId))
126+
.returning({ id: schema.account.id })
127+
if (deleted.length > 0) {
128+
logger.info('Deleted orphaned OAuth account', { accountId })
129+
}
130+
return deleted.length > 0
131+
}
132+
104133
/**
105134
* Clears stored references to a credential across mutable workspace state
106135
* (editor blocks, copilot checkpoints, knowledge connectors) and frozen

apps/sim/lib/credentials/orchestration/index.test.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ const {
1818
mockIsClientCredentialAccountProviderId,
1919
mockGetClientCredentialAccountDescriptor,
2020
mockDeleteConnectionCredential,
21+
mockDeleteOrphanedOAuthAccount,
2122
} = vi.hoisted(() => ({
2223
mockRecordAudit: vi.fn(),
2324
mockGetCredentialActorContext: vi.fn(),
@@ -28,6 +29,7 @@ const {
2829
// providers must not trigger the stored-blob read for authMethod/username.
2930
mockGetClientCredentialAccountDescriptor: vi.fn(() => undefined),
3031
mockDeleteConnectionCredential: vi.fn(),
32+
mockDeleteOrphanedOAuthAccount: vi.fn(),
3133
}))
3234

3335
vi.mock('@sim/audit', () => ({
@@ -51,6 +53,7 @@ vi.mock('@/lib/credentials/client-credential-accounts/descriptors', () => ({
5153
}))
5254
vi.mock('@/lib/credentials/deletion', () => ({
5355
deleteConnectionCredential: mockDeleteConnectionCredential,
56+
deleteOrphanedOAuthAccount: mockDeleteOrphanedOAuthAccount,
5457
}))
5558
vi.mock('@/lib/credentials/environment', () => ({
5659
deleteWorkspaceEnvCredentials: vi.fn(),
@@ -550,4 +553,57 @@ describe('deleteCredentialRecord', () => {
550553
})
551554
expect(mockDeleteConnectionCredential).not.toHaveBeenCalled()
552555
})
556+
557+
it('revokes the backing OAuth grant of a deleted oauth credential', async () => {
558+
mockDeleteConnectionCredential.mockResolvedValueOnce(true)
559+
560+
const deleted = await deleteCredentialRecord({
561+
credential: {
562+
id: 'cred-1',
563+
workspaceId: 'ws-1',
564+
type: 'oauth',
565+
providerId: 'google-email',
566+
accountId: 'acct-1',
567+
} as never,
568+
reason: 'user_delete',
569+
})
570+
571+
expect(deleted).toBe(true)
572+
expect(mockDeleteOrphanedOAuthAccount).toHaveBeenCalledWith('acct-1')
573+
})
574+
575+
it('leaves the OAuth grant alone when the credential row was already gone', async () => {
576+
mockDeleteConnectionCredential.mockResolvedValueOnce(false)
577+
578+
const deleted = await deleteCredentialRecord({
579+
credential: {
580+
id: 'cred-1',
581+
workspaceId: 'ws-1',
582+
type: 'oauth',
583+
providerId: 'google-email',
584+
accountId: 'acct-1',
585+
} as never,
586+
reason: 'user_delete',
587+
})
588+
589+
expect(deleted).toBe(false)
590+
expect(mockDeleteOrphanedOAuthAccount).not.toHaveBeenCalled()
591+
})
592+
593+
it('does not touch OAuth grants for a service-account credential', async () => {
594+
mockDeleteConnectionCredential.mockResolvedValueOnce(true)
595+
596+
await deleteCredentialRecord({
597+
credential: {
598+
id: 'cred-1',
599+
workspaceId: 'ws-1',
600+
type: 'service_account',
601+
providerId: 'google-service-account',
602+
accountId: null,
603+
} as never,
604+
reason: 'user_delete',
605+
})
606+
607+
expect(mockDeleteOrphanedOAuthAccount).not.toHaveBeenCalled()
608+
})
553609
})

apps/sim/lib/credentials/orchestration/index.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@ import {
2424
getClientCredentialAccountDescriptor,
2525
isClientCredentialAccountProviderId,
2626
} from '@/lib/credentials/client-credential-accounts/descriptors'
27-
import { type CredentialDeleteReason, deleteConnectionCredential } from '@/lib/credentials/deletion'
27+
import {
28+
type CredentialDeleteReason,
29+
deleteConnectionCredential,
30+
deleteOrphanedOAuthAccount,
31+
} from '@/lib/credentials/deletion'
2832
import { slackCustomBotDisplayName } from '@/lib/credentials/display-name'
2933
import {
3034
deleteWorkspaceEnvCredentials,
@@ -554,11 +558,22 @@ export async function deleteCredentialRecord(
554558
return true
555559
}
556560

557-
return deleteConnectionCredential({
561+
const deleted = await deleteConnectionCredential({
558562
credentialId: credentialRow.id,
559563
workspaceId: credentialRow.workspaceId,
560564
reason: params.reason,
561565
})
566+
567+
// An OAuth credential's secret source is its `account` row, so deleting the
568+
// credential alone would leave the provider grant behind. Revoke it here
569+
// rather than only on the personal disconnect path, so an admin removing a
570+
// teammate's connection tears down the same state the owner's own disconnect
571+
// would. Idempotent for the disconnect path, which sweeps accounts after.
572+
if (deleted && credentialRow.type === 'oauth' && credentialRow.accountId) {
573+
await deleteOrphanedOAuthAccount(credentialRow.accountId)
574+
}
575+
576+
return deleted
562577
}
563578

564579
/** Preserves the legacy callers while application adapters migrate to the manager above. */

0 commit comments

Comments
 (0)