Skip to content

Commit 789ffb7

Browse files
committed
fix(access-requests): scope previews and audit events correctly
1 parent 83c165a commit 789ffb7

10 files changed

Lines changed: 281 additions & 20 deletions

File tree

‎apps/sim/lib/core/application/authorized-workspace-use-case.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ vi.mock('@sim/platform-authz/workspace', () => ({
4040

4141
import { AuditAction, AuditResourceType } from '@sim/audit'
4242
import { defineAuthorizedWorkspaceUseCase, defineWorkspaceOperation } from '@/lib/core/application'
43+
import { recordProjectedUseCaseAuditEntries } from '@/lib/core/application/authorized-workspace-use-case'
4344
import { resolveCurrentOutboundRoute } from '@/lib/core/network/context.server'
4445
import type { OrchestrationError } from '@/lib/core/orchestration/types'
4546
import { CREDENTIAL_GROUP_CREDENTIAL_USE_ACTION } from '@/lib/resource-policies/registry'
@@ -494,3 +495,34 @@ describe('defineAuthorizedWorkspaceUseCase', () => {
494495
)
495496
})
496497
})
498+
499+
describe('projected audit workspace attribution', () => {
500+
beforeEach(() => vi.clearAllMocks())
501+
502+
it.each([
503+
{ override: undefined, expected: 'workspace-1' },
504+
{ override: 'workspace-2', expected: 'workspace-2' },
505+
{ override: null, expected: null },
506+
])('records the canonical workspace override $override', ({ override, expected }) => {
507+
recordProjectedUseCaseAuditEntries(
508+
operation,
509+
'workspace-1',
510+
sessionPrincipal,
511+
undefined,
512+
[
513+
{
514+
action: AuditAction.FILE_UPDATED,
515+
resourceType: AuditResourceType.FILE,
516+
workspaceId: override,
517+
},
518+
],
519+
'organization-1'
520+
)
521+
expect(mocks.recordAudit).toHaveBeenCalledExactlyOnceWith(
522+
expect.objectContaining({
523+
workspaceId: expected,
524+
metadata: expect.objectContaining({ organizationId: 'organization-1' }),
525+
})
526+
)
527+
})
528+
})

‎apps/sim/lib/core/application/authorized-workspace-use-case.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ import type { OrchestrationRequestContext } from '@/lib/core/orchestration/types
1717
import type { ResourcePolicyBinding } from '@/lib/resource-policies/registry'
1818

1919
export interface WorkspaceUseCaseAuditEntry {
20-
/** Canonical workspace affected by a cross-workspace mutation, when different from its authorization scope. */
21-
workspaceId?: string
20+
/** Canonical affected workspace; null keeps an organization event outside the authorization workspace. */
21+
workspaceId?: string | null
2222
action: AuditActionType
2323
resourceType: AuditResourceTypeValue
2424
resourceId?: string
@@ -106,7 +106,7 @@ export function recordProjectedUseCaseAuditEntries(
106106
const attribution: PrincipalAuditAttribution = resolvePrincipalAuditAttribution(principal)
107107
for (const entry of entries) {
108108
recordAudit({
109-
workspaceId: entry.workspaceId ?? workspaceId,
109+
workspaceId: entry.workspaceId === undefined ? workspaceId : entry.workspaceId,
110110
actorId: attribution.actorId,
111111
actorName: attribution.actorName,
112112
action: entry.action,

‎apps/sim/lib/permission-access-requests/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ Members request access from locked features, the block picker, or **My access re
1515
- Fulfillment updates the current governing group. The preview lists every required change, including parent restrictions, and a conservative upper bound of affected people/workspaces. It does not create individual grants or move members between groups.
1616
- Approval rechecks administrator authority, requester membership identity, workspace ownership, entitlement at admission, organization preference, group resolution, and the preview fingerprint. Membership, policy, or scope changes require a fresh review or request.
1717
- The group/credit-limit update, final request record, and notification enqueue share one database transaction. The final decision stores its original change and impact for history; later policy edits do not rewrite it.
18-
- One pending request per requester/scope/target is enforced by a database index and organization serialization. Member cap requests share an organization-wide key. Submission is bounded to 25 requests per rolling day and 100 pending requests per requester/organization, in addition to HTTP rate admission.
18+
- One pending request per requester/scope/target is enforced by a database index and organization serialization. Member cap requests share an organization-wide key. Submission is bounded to 100 requests per rolling 24 hours and 100 pending requests per requester/organization, in addition to HTTP rate admission.
1919
- A usage request increases the existing member credit cap. It does not change the pooled organization budget, buy credits, or alter temporary request-rate limits.
2020
- Notifications recheck current membership and reviewer authority. Outbox fan-out is bounded and replay-safe; delivery to an email provider remains at-least-once across a crash after send.
2121

‎apps/sim/lib/permission-access-requests/application/requests.test.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ describe('create access requests', () => {
285285

286286
it.each([
287287
{ pending: 100, daily: 0, message: '100 pending requests' },
288-
{ pending: 0, daily: 25, message: 'maximum of 25 requests today' },
288+
{ pending: 0, daily: 100, message: '100 requests in the last 24 hours' },
289289
])('enforces bounded admissions ($message)', async ({ pending, daily, message }) => {
290290
queueTableRows(permissionAccessRequest, [])
291291
queueTableRows(permissionAccessRequest, [{ total: pending }])
@@ -297,6 +297,22 @@ describe('create access requests', () => {
297297
expect(mocks.outbox).not.toHaveBeenCalled()
298298
})
299299

300+
it.each([25, 99])(
301+
'allows a new request after %s submissions in the rolling window',
302+
async (daily) => {
303+
queueTableRows(permissionAccessRequest, [])
304+
queueTableRows(permissionAccessRequest, [{ total: 0 }])
305+
queueTableRows(permissionAccessRequest, [{ total: daily }])
306+
dbChainMockFns.returning.mockResolvedValueOnce([stored()])
307+
const result = await createAccessRequest.execute({ principal, input: { scope, target } })
308+
expect(result.changed).toBe(true)
309+
expect(dbChainMockFns.insert).toHaveBeenCalledWith(permissionAccessRequest)
310+
expect(mocks.audit.mock.calls[0][4]).toEqual([
311+
expect.objectContaining({ workspaceId: 'workspace', resourceId: 'request' }),
312+
])
313+
}
314+
)
315+
300316
it('normalizes member cap requests to one organization-wide request across workspaces', async () => {
301317
const cap = stored({
302318
target: { kind: 'usage_limit', id: 'member' },
@@ -315,6 +331,9 @@ describe('create access requests', () => {
315331
input: { scope, target: { kind: 'usage_limit', id: 'member' } },
316332
})
317333
expect(first.changed).toBe(true)
334+
expect(mocks.audit.mock.calls[0][4]).toEqual([
335+
expect.objectContaining({ workspaceId: null, resourceId: cap.id }),
336+
])
318337
expect(dbChainMockFns.values).toHaveBeenCalledWith(
319338
expect.objectContaining({
320339
workspaceId: null,
@@ -489,6 +508,27 @@ describe('discovery and request history', () => {
489508
})
490509

491510
describe('cancel access requests', () => {
511+
it.each(['workspace', null])(
512+
'keeps cancellation in the stored request scope %s',
513+
async (workspaceId) => {
514+
const row = stored({
515+
workspaceId,
516+
...(workspaceId === null
517+
? {
518+
scopeKey: 'organization:organization:member-limit',
519+
target: { kind: 'usage_limit', id: 'member' },
520+
}
521+
: {}),
522+
})
523+
mocks.stored.mockResolvedValue(row)
524+
dbChainMockFns.returning.mockResolvedValueOnce([{ ...row, status: 'cancelled' }])
525+
await cancelAccessRequest.execute({ principal, input: { scope, requestId: row.id } })
526+
expect(mocks.audit.mock.calls[0][4]).toEqual([
527+
expect.objectContaining({ workspaceId, resourceId: row.id }),
528+
])
529+
}
530+
)
531+
492532
it('allows cancellation while disabled and does not resend a decision for terminal requests', async () => {
493533
mocks.enabled.mockResolvedValue(false)
494534
dbChainMockFns.returning.mockResolvedValueOnce([stored({ status: 'cancelled' })])

‎apps/sim/lib/permission-access-requests/application/requests.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ import {
1818
listAccessRequestTargets,
1919
loadAccessRequestCatalog,
2020
} from '@/lib/permission-access-requests/catalog'
21+
import {
22+
ACCESS_REQUEST_MAX_DAILY_SUBMISSIONS,
23+
ACCESS_REQUEST_MAX_PENDING,
24+
ACCESS_REQUEST_SUBMISSION_WINDOW_MS,
25+
} from '@/lib/permission-access-requests/constants'
2126
import {
2227
PERMISSION_ACCESS_REQUEST_CREATED_EVENT,
2328
PERMISSION_ACCESS_REQUEST_DECIDED_EVENT,
@@ -151,7 +156,7 @@ export const discoverAccessRequests = defineAuthorizedAccessRequestUseCase({
151156
eq(permissionAccessRequest.status, 'pending')
152157
)
153158
)
154-
.limit(100)
159+
.limit(ACCESS_REQUEST_MAX_PENDING)
155160
const pendingMemberLimit = pending.find((row) => row.targetKey === 'usage_limit:member')
156161
const memberLimitMembershipMatches = pendingMemberLimit
157162
? await hasCurrentMemberLimitMembership(
@@ -351,10 +356,10 @@ export const createAccessRequest = defineAuthorizedAccessRequestUseCase({
351356
eq(permissionAccessRequest.status, 'pending')
352357
)
353358
)
354-
if ((outstanding?.total ?? 0) >= 100)
359+
if ((outstanding?.total ?? 0) >= ACCESS_REQUEST_MAX_PENDING)
355360
throw new OrchestrationError(
356361
'conflict',
357-
'You have 100 pending requests. Cancel an existing request before sending another.'
362+
`You have ${ACCESS_REQUEST_MAX_PENDING} pending requests. Cancel an existing request before sending another.`
358363
)
359364
const [daily] = await executor
360365
.select({ total: count() })
@@ -363,13 +368,16 @@ export const createAccessRequest = defineAuthorizedAccessRequestUseCase({
363368
and(
364369
eq(permissionAccessRequest.organizationId, organizationId),
365370
eq(permissionAccessRequest.requesterId, principal.userId),
366-
gte(permissionAccessRequest.createdAt, new Date(Date.now() - 86_400_000))
371+
gte(
372+
permissionAccessRequest.createdAt,
373+
new Date(Date.now() - ACCESS_REQUEST_SUBMISSION_WINDOW_MS)
374+
)
367375
)
368376
)
369-
if ((daily?.total ?? 0) >= 25)
377+
if ((daily?.total ?? 0) >= ACCESS_REQUEST_MAX_DAILY_SUBMISSIONS)
370378
throw new OrchestrationError(
371379
'conflict',
372-
'You have sent the maximum of 25 requests today. Try again tomorrow.'
380+
`You have sent ${ACCESS_REQUEST_MAX_DAILY_SUBMISSIONS} requests in the last 24 hours. Try again later.`
373381
)
374382
const [row] = await executor
375383
.insert(permissionAccessRequest)
@@ -402,6 +410,7 @@ export const createAccessRequest = defineAuthorizedAccessRequestUseCase({
402410
action: AuditAction.PERMISSION_ACCESS_REQUEST_CLOSED,
403411
resourceType: AuditResourceType.PERMISSION_ACCESS_REQUEST,
404412
resourceId: result.closedRequest.id,
413+
workspaceId: result.closedRequest.workspaceId,
405414
metadata: { target: result.closedRequest.target },
406415
},
407416
]
@@ -413,6 +422,7 @@ export const createAccessRequest = defineAuthorizedAccessRequestUseCase({
413422
: AuditAction.PERMISSION_ACCESS_REQUEST_CREATED,
414423
resourceType: AuditResourceType.PERMISSION_ACCESS_REQUEST,
415424
resourceId: result.request.id,
425+
workspaceId: result.request.workspaceId,
416426
metadata: { target: result.request.target },
417427
},
418428
]
@@ -492,6 +502,7 @@ export const cancelAccessRequest = defineAuthorizedAccessRequestUseCase({
492502
action: AuditAction.PERMISSION_ACCESS_REQUEST_CANCELLED,
493503
resourceType: AuditResourceType.PERMISSION_ACCESS_REQUEST,
494504
resourceId: result.request.id,
505+
workspaceId: result.request.workspaceId,
495506
},
496507
]
497508
: [],

‎apps/sim/lib/permission-access-requests/application/review.test.ts‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { AuditAction } from '@sim/audit'
45
import { db } from '@sim/db'
56
import {
67
organizationMemberUsageLimit,
@@ -407,6 +408,44 @@ describe('permission request review', () => {
407408
expect(mocks.outbox).toHaveBeenCalledOnce()
408409
})
409410

411+
it.each(['workspace', null])(
412+
'attributes a declined request to its stored scope %s',
413+
async (workspaceId) => {
414+
mocks.stored.mockResolvedValue(stored({ workspaceId }))
415+
dbChainMockFns.returning.mockResolvedValueOnce([stored({ workspaceId, status: 'declined' })])
416+
await resolveAccessRequest.execute({
417+
principal,
418+
input: { ...input, decision: { action: 'decline', reason: 'Use the existing provider.' } },
419+
})
420+
expect(mocks.audit.mock.calls[0][4]).toEqual([
421+
expect.objectContaining({
422+
action: AuditAction.PERMISSION_ACCESS_REQUEST_DECLINED,
423+
workspaceId,
424+
}),
425+
])
426+
}
427+
)
428+
429+
it('keeps request fulfillment workspace-scoped and the group change organization-scoped', async () => {
430+
const before = await preview()
431+
queueWorkspace()
432+
dbChainMockFns.returning.mockResolvedValueOnce([stored({ status: 'fulfilled' })])
433+
await resolveAccessRequest.execute({
434+
principal,
435+
input: { ...input, decision: { action: 'apply', expectedFingerprint: before.fingerprint } },
436+
})
437+
const [, defaultWorkspaceId, , , entries] = mocks.audit.mock.calls[0]
438+
expect(defaultWorkspaceId).toBeNull()
439+
expect(entries).toEqual([
440+
expect.objectContaining({
441+
action: AuditAction.PERMISSION_ACCESS_REQUEST_FULFILLED,
442+
workspaceId: 'workspace',
443+
}),
444+
expect.objectContaining({ action: AuditAction.PERMISSION_GROUP_UPDATED }),
445+
])
446+
expect(entries[1]).not.toHaveProperty('workspaceId')
447+
})
448+
410449
it('does not apply or notify a second time after resolution', async () => {
411450
mocks.stored.mockResolvedValue(stored({ status: 'fulfilled' }))
412451
const result = await resolveAccessRequest.execute({
@@ -417,6 +456,7 @@ describe('permission request review', () => {
417456
expect(dbChainMockFns.update).not.toHaveBeenCalled()
418457
expect(mocks.outbox).not.toHaveBeenCalled()
419458
expect(mocks.catalog).not.toHaveBeenCalled()
459+
expect(mocks.audit.mock.calls[0][4]).toEqual([])
420460
})
421461

422462
it('reads fulfilled history from its stored decision without rebuilding the catalog', async () => {
@@ -483,7 +523,11 @@ describe('member limit review', () => {
483523
})
484524
queueLimit(2000)
485525
dbChainMockFns.returning.mockResolvedValueOnce([
486-
stored({ status: 'fulfilled', target: { kind: 'usage_limit', id: 'member' } }),
526+
stored({
527+
workspaceId: null,
528+
status: 'fulfilled',
529+
target: { kind: 'usage_limit', id: 'member' },
530+
}),
487531
])
488532
await resolveAccessRequest.execute({
489533
principal,
@@ -498,6 +542,16 @@ describe('member limit review', () => {
498542
})
499543
expect(mocks.setLimit).toHaveBeenCalledWith('organization', 'requester', 15, 'admin', db)
500544
expect(dbChainMockFns.update).not.toHaveBeenCalledWith(permissionGroup)
545+
const [, defaultWorkspaceId, , , entries] = mocks.audit.mock.calls[0]
546+
expect(defaultWorkspaceId).toBeNull()
547+
expect(entries).toEqual([
548+
expect.objectContaining({
549+
action: AuditAction.PERMISSION_ACCESS_REQUEST_FULFILLED,
550+
workspaceId: null,
551+
}),
552+
expect.objectContaining({ action: AuditAction.ORG_MEMBER_USAGE_LIMIT_CHANGED }),
553+
])
554+
expect(entries[1]).not.toHaveProperty('workspaceId')
501555
expect(mocks.outbox).toHaveBeenCalledWith(db, PERMISSION_ACCESS_REQUEST_DECIDED_EVENT, {
502556
requestId: 'request',
503557
})

‎apps/sim/lib/permission-access-requests/application/review.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,7 @@ export const resolveAccessRequest = defineAuthorizedAccessRequestUseCase({
363363
: AuditAction.PERMISSION_ACCESS_REQUEST_DECLINED,
364364
resourceType: AuditResourceType.PERMISSION_ACCESS_REQUEST,
365365
resourceId: result.request.id,
366+
workspaceId: result.request.workspaceId,
366367
metadata: {
367368
target: result.request.target,
368369
requesterId: result.request.requester.id,

‎apps/sim/lib/permission-access-requests/constants.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,6 @@ export const ACCESS_REQUEST_LIST_PAGE_SIZE = 25
22
export const ACCESS_REQUEST_MAX_OFFSET = 1_000_000
33
export const ACCESS_REQUEST_MAX_SEARCH_LENGTH = 200
44
export const ACCESS_REQUEST_MAX_ID_LENGTH = 128
5+
export const ACCESS_REQUEST_MAX_DAILY_SUBMISSIONS = 100
6+
export const ACCESS_REQUEST_MAX_PENDING = 100
7+
export const ACCESS_REQUEST_SUBMISSION_WINDOW_MS = 24 * 60 * 60 * 1000

0 commit comments

Comments
 (0)