Skip to content

Commit d520146

Browse files
waleedlatif1waleedlatif1
committed
fix(invitations): revalidate policy and expiry under mutation locks
1 parent 22e5858 commit d520146

12 files changed

Lines changed: 463 additions & 114 deletions

File tree

‎apps/sim/app/api/invitations/[id]/resend/route.test.ts‎

Lines changed: 69 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { db } from '@sim/db'
45
import { member, user } from '@sim/db/schema'
56
import { authMockFns, createMockRequest, queueTableRows, resetDbChainMock } from '@sim/testing'
67
import { beforeEach, describe, expect, it, vi } from 'vitest'
@@ -52,6 +53,7 @@ vi.mock('@/ee/access-control/utils/permission-check', () => ({
5253
vi.mock('@/lib/invitations/core', () => ({
5354
getInvitationById: mockGetInvitationById,
5455
resolveInvitationAdmissionOrganizationId: mockResolveInvitationAdmissionOrganizationId,
56+
requireInvitationResendAuthority: vi.fn(),
5557
}))
5658
vi.mock('@/lib/invitations/send', () => ({
5759
sendInvitationEmail: mockSendInvitationEmail,
@@ -70,13 +72,15 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
7072
}))
7173
vi.mock('@/lib/workspaces/policy', () => ({
7274
getWorkspaceInvitePolicy: mockGetWorkspaceInvitePolicy,
75+
WORKSPACE_MODE: { ORGANIZATION: 'organization' },
7376
}))
7477

7578
vi.mock('@/lib/permission-groups/resolve.server', () => ({
7679
getUserPermissionConfigForOrganization: vi.fn().mockResolvedValue(null),
7780
}))
7881

7982
import { OrchestrationError } from '@/lib/core/orchestration/types'
83+
import { lockInvitationResendPolicy } from '@/lib/invitations/resend-policy'
8084
import type { PreparedInvitationResend } from '@/lib/invitations/send'
8185
import { POST } from '@/app/api/invitations/[id]/resend/route'
8286

@@ -141,10 +145,21 @@ describe('POST /api/invitations/[id]/resend', () => {
141145
mockGetWorkspaceWithOwner.mockResolvedValue({
142146
id: 'workspace-1',
143147
organizationId: 'organization-1',
148+
workspaceMode: 'organization',
149+
billedAccountUserId: 'owner',
150+
ownerId: 'owner',
144151
})
145152
mockGetWorkspaceInvitePolicy.mockResolvedValue({ allowed: true })
146153
mockValidateInvitationsAllowed.mockResolvedValue(undefined)
147-
mockPrepareInvitationResend.mockResolvedValue(preparedResend)
154+
mockPrepareInvitationResend.mockImplementation(async (params) => {
155+
await lockInvitationResendPolicy(
156+
db,
157+
await mockGetInvitationById(params.invitationId),
158+
params.actorUserId,
159+
params.expectedOrganizationId
160+
)
161+
return preparedResend
162+
})
148163
mockSendInvitationEmail.mockResolvedValue({ success: true })
149164
mockRevertInvitationResend.mockResolvedValue(true)
150165
})
@@ -153,9 +168,13 @@ describe('POST /api/invitations/[id]/resend', () => {
153168
const response = await callResend()
154169

155170
expect(response.status).toBe(200)
156-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
157-
workspaceId: 'workspace-1',
158-
})
171+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
172+
'user-1',
173+
{
174+
workspaceId: 'workspace-1',
175+
},
176+
db
177+
)
159178
expect(mockSendInvitationEmail).toHaveBeenCalled()
160179
expect(mockPrepareInvitationResend.mock.invocationCallOrder[0]).toBeLessThan(
161180
mockSendInvitationEmail.mock.invocationCallOrder[0]
@@ -210,12 +229,20 @@ describe('POST /api/invitations/[id]/resend', () => {
210229
const response = await callResend()
211230

212231
expect(response.status).toBe(200)
213-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
214-
organizationId: 'organization-1',
215-
})
216-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
217-
workspaceId: 'workspace-1',
218-
})
232+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
233+
'user-1',
234+
{
235+
organizationId: 'organization-1',
236+
},
237+
db
238+
)
239+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
240+
'user-1',
241+
{
242+
workspaceId: 'workspace-1',
243+
},
244+
db
245+
)
219246
})
220247

221248
it('refuses an organization invitation the organization default group withholds, even when its granted workspace allows', async () => {
@@ -245,13 +272,24 @@ describe('POST /api/invitations/[id]/resend', () => {
245272
const response = await callResend()
246273

247274
expect(response.status).toBe(200)
248-
expect(mockResolveInvitationAdmissionOrganizationId).toHaveBeenCalledWith(workspaceInvitation)
249-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
250-
organizationId: 'organization-1',
251-
})
252-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
253-
workspaceId: 'workspace-1',
254-
})
275+
expect(mockResolveInvitationAdmissionOrganizationId).toHaveBeenCalledWith(
276+
workspaceInvitation,
277+
db
278+
)
279+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
280+
'user-1',
281+
{
282+
organizationId: 'organization-1',
283+
},
284+
db
285+
)
286+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
287+
'user-1',
288+
{
289+
workspaceId: 'workspace-1',
290+
},
291+
db
292+
)
255293
})
256294

257295
it('refuses a workspace invitation whose admitting organization withholds invitations', async () => {
@@ -281,9 +319,13 @@ describe('POST /api/invitations/[id]/resend', () => {
281319

282320
expect(response.status).toBe(200)
283321
expect(mockValidateInvitationsAllowed).toHaveBeenCalledTimes(1)
284-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
285-
workspaceId: 'workspace-1',
286-
})
322+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
323+
'user-1',
324+
{
325+
workspaceId: 'workspace-1',
326+
},
327+
db
328+
)
287329
})
288330

289331
it('resolves the organization default group for an invitation with no grants', async () => {
@@ -298,9 +340,13 @@ describe('POST /api/invitations/[id]/resend', () => {
298340
const response = await callResend()
299341

300342
expect(response.status).toBe(200)
301-
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith('user-1', {
302-
organizationId: 'organization-1',
303-
})
343+
expect(mockValidateInvitationsAllowed).toHaveBeenCalledWith(
344+
'user-1',
345+
{
346+
organizationId: 'organization-1',
347+
},
348+
db
349+
)
304350
})
305351
it.each(['pending', 'expired'])(
306352
'rejects an expired %s invitation consistently',

‎apps/sim/ee/access-control/utils/permission-check.test.ts‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { db } from '@sim/db'
45
import { permissionGroup } from '@sim/db/schema'
56
import {
67
envFlagsMockFns,
@@ -49,6 +50,7 @@ import {
4950
CustomToolsNotAllowedError,
5051
getUserPermissionConfig,
5152
IntegrationNotAllowedError,
53+
InvitationsNotAllowedError,
5254
McpToolsNotAllowedError,
5355
ModelNotAllowedError,
5456
ProviderNotAllowedError,
@@ -57,9 +59,10 @@ import {
5759
ToolNotAllowedError,
5860
validateBlockType,
5961
validateChatDeployAuth,
62+
validateInvitationsAllowed,
6063
validateModelProvider,
6164
validatePublicFileSharing,
62-
} from './permission-check'
65+
} from '@/ee/access-control/utils/permission-check'
6366

6467
/** Default an org-backed, enterprise-entitled workspace so resolution reaches the group queries. */
6568
function setEnterpriseOrgWorkspace() {
@@ -902,3 +905,37 @@ describe('assertPermissionsAllowed', () => {
902905
})
903906
})
904907
})
908+
909+
describe('transactional invitation permission checks', () => {
910+
beforeEach(() => {
911+
vi.clearAllMocks()
912+
resetDbChainMock()
913+
setEnvFlags({ isAccessControlEnabled: true, isHosted: true, isInvitationsDisabled: false })
914+
mockGetAllowedIntegrationsFromEnv.mockReturnValue(null)
915+
setEnterpriseOrgWorkspace()
916+
})
917+
918+
it('bypasses a cached allow decision when the transaction sees a newly restricted workspace', async () => {
919+
await withPermissionGroupScope(async () => {
920+
queueGroupResolution([], [{ config: { disableInvitations: false } }])
921+
await validateInvitationsAllowed('actor', { workspaceId: 'workspace-1' })
922+
queueGroupResolution([], [{ config: { disableInvitations: true } }])
923+
await expect(
924+
validateInvitationsAllowed('actor', { workspaceId: 'workspace-1' }, db)
925+
).rejects.toBeInstanceOf(InvitationsNotAllowedError)
926+
})
927+
expect(mockGetWorkspaceWithOwner).toHaveBeenLastCalledWith('workspace-1', {
928+
includeArchived: true,
929+
executor: db,
930+
})
931+
expect(mockIsOrganizationOnEnterprisePlan).toHaveBeenLastCalledWith('org-1', db)
932+
})
933+
934+
it('resolves organization admission on the transaction executor', async () => {
935+
queueTableRows(permissionGroup, [{ config: { disableInvitations: true } }])
936+
await expect(
937+
validateInvitationsAllowed('actor', { organizationId: 'org-1' }, db)
938+
).rejects.toBeInstanceOf(InvitationsNotAllowedError)
939+
expect(mockIsOrganizationOnEnterprisePlan).toHaveBeenCalledWith('org-1', db)
940+
})
941+
})

‎apps/sim/ee/access-control/utils/permission-check.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
} from '@/lib/core/config/env-flags'
1111
import { findDatabaseQueryError } from '@/lib/core/errors/database-query-error'
1212
import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure'
13+
import type { DbOrTx } from '@/lib/db/types'
1314
import { isBlockTypeAccessControlExempt } from '@/lib/permission-groups/block-access'
1415
import {
1516
CAPABILITY_RULES,
@@ -427,7 +428,8 @@ const INVITATIONS_RULE = CAPABILITY_RULES['invitations.send']
427428
/** permission-group-enforced: invitations.send — organization-scoped, so it resolves the default group rather than a workspace one */
428429
export async function validateInvitationsAllowed(
429430
userId: string | undefined,
430-
scope: string | { workspaceId?: string; organizationId?: string } = {}
431+
scope: string | { workspaceId?: string; organizationId?: string } = {},
432+
executor?: DbOrTx
431433
): Promise<void> {
432434
if (isInvitationsDisabled) {
433435
logger.warn('Invitations blocked by feature flag')
@@ -442,7 +444,9 @@ export async function validateInvitationsAllowed(
442444
typeof scope === 'string' ? { workspaceId: scope, organizationId: undefined } : scope
443445

444446
if (workspaceId) {
445-
const config = await resolvePermissionGroupConfig(userId, workspaceId, undefined)
447+
const config = executor
448+
? await getUserPermissionConfig(userId, workspaceId, executor)
449+
: await resolvePermissionGroupConfig(userId, workspaceId, undefined)
446450
if (config && INVITATIONS_RULE.deniedBy(config)) {
447451
logger.warn('Invitations blocked by permission group', { userId, workspaceId })
448452
throw new InvitationsNotAllowedError()
@@ -451,7 +455,9 @@ export async function validateInvitationsAllowed(
451455
}
452456

453457
if (organizationId) {
454-
const config = await getUserPermissionConfigForOrganization(organizationId)
458+
const config = executor
459+
? await getUserPermissionConfigForOrganization(organizationId, executor)
460+
: await getUserPermissionConfigForOrganization(organizationId)
455461
if (config && INVITATIONS_RULE.deniedBy(config)) {
456462
logger.warn('Invitations blocked by permission group (organization-wide)', {
457463
userId,

‎apps/sim/lib/invitations/core.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1783,6 +1783,7 @@ export async function revokeInvitationAsAdmin(input: {
17831783
const revoked = await revokeInvitationWorkspaceGrantTx(tx, {
17841784
invitationId: input.invitationId,
17851785
workspaceId: input.workspaceId,
1786+
requireUnexpired: true,
17861787
})
17871788
if (!revoked.revoked) return { success: false, kind: 'not-cancellable' }
17881789
return {
@@ -1851,9 +1852,12 @@ export async function revokeInvitationWorkspaceGrantTx(
18511852
{
18521853
invitationId,
18531854
workspaceId,
1855+
requireUnexpired = false,
18541856
}: {
18551857
invitationId: string
18561858
workspaceId: string
1859+
/** User revocation checks expiry; direct-grant cleanup may remove stale pending grants. */
1860+
requireUnexpired?: boolean
18571861
}
18581862
): Promise<{ revoked: boolean; invitationCancelled: boolean }> {
18591863
const [pending] = await tx
@@ -1869,7 +1873,13 @@ export async function revokeInvitationWorkspaceGrantTx(
18691873
.where(
18701874
and(
18711875
eq(invitationWorkspaceGrant.invitationId, invitationId),
1872-
eq(invitationWorkspaceGrant.workspaceId, workspaceId)
1876+
eq(invitationWorkspaceGrant.workspaceId, workspaceId),
1877+
requireUnexpired
1878+
? sql`exists (select 1 from ${invitation}
1879+
where ${invitation.id} = ${invitationId}
1880+
and ${invitation.status} = 'pending'
1881+
and ${invitation.expiresAt} > clock_timestamp())`
1882+
: undefined
18731883
)
18741884
)
18751885
.returning({ id: invitationWorkspaceGrant.id })

‎apps/sim/lib/invitations/grant-revocation.test.ts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,13 @@
33
*/
44
import { db } from '@sim/db'
55
import { invitation, invitationWorkspaceGrant } from '@sim/db/schema'
6-
import { auditMock, dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
6+
import {
7+
auditMock,
8+
dbChainMockFns,
9+
hasMockCondition,
10+
queueTableRows,
11+
resetDbChainMock,
12+
} from '@sim/testing'
713
import { beforeEach, describe, expect, it, vi } from 'vitest'
814

915
vi.mock('@sim/audit', () => auditMock)
@@ -53,4 +59,24 @@ describe('revokeInvitationWorkspaceGrantTx', () => {
5359
expect.objectContaining({ status: 'cancelled' })
5460
)
5561
})
62+
63+
it('checks database expiry in the grant deletion itself and leaves expired invitations untouched', async () => {
64+
queueTableRows(invitation, [{ id: 'inv-1' }])
65+
dbChainMockFns.returning.mockResolvedValueOnce([])
66+
await expect(
67+
revokeInvitationWorkspaceGrantTx(db, {
68+
invitationId: 'inv-1',
69+
workspaceId: 'ws-1',
70+
requireUnexpired: true,
71+
})
72+
).resolves.toEqual({ revoked: false, invitationCancelled: false })
73+
const [predicate] = dbChainMockFns.where.mock.calls[1]
74+
expect(
75+
hasMockCondition(
76+
predicate,
77+
(node) => Array.isArray(node.strings) && node.strings.join('').includes('clock_timestamp()')
78+
)
79+
).toBe(true)
80+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
81+
})
5682
})

0 commit comments

Comments
 (0)