Skip to content

Commit f9ad878

Browse files
authored
fix(access-requests): align permission checks and pending request flows (#7880)
* fix(access-requests): align permission checks and pending request flows * fix(access-requests): align settings and state with shared patterns * fix(access-requests): show readable integration policy names * improvement(access-requests): trim redundant interface copy * fix(access-requests): clear recovered limit dialogs and align review spacing
1 parent 713f247 commit f9ad878

31 files changed

Lines changed: 1425 additions & 166 deletions
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
/** @vitest-environment node */
2+
import { authMockFns } from '@sim/testing'
3+
import { beforeEach, describe, expect, it, vi } from 'vitest'
4+
5+
const { redirect } = vi.hoisted(() => ({ redirect: vi.fn() }))
6+
vi.mock('next/navigation', () => ({ redirect }))
7+
vi.mock('@/components/access-requests/my-access-requests', () => ({ MyAccessRequests: () => null }))
8+
vi.mock('@/components/access-requests/organization-access-requests', () => ({
9+
OrganizationAccessRequests: () => null,
10+
}))
11+
12+
import AccessRequestsPage from '@/app/access-requests/page'
13+
14+
describe('access request sign-in redirect', () => {
15+
beforeEach(() => {
16+
vi.clearAllMocks()
17+
authMockFns.mockGetSession.mockResolvedValue(null)
18+
redirect.mockImplementation(() => {
19+
throw new Error('Redirect')
20+
})
21+
})
22+
23+
it.each([
24+
{
25+
organizationId: 'organization',
26+
view: 'catalog',
27+
requestId: 'request',
28+
search: 'Slack & Notion',
29+
page: '3',
30+
},
31+
{
32+
organizationId: 'organization',
33+
view: 'admin',
34+
requestId: 'request',
35+
'request-status': 'declined',
36+
'request-page': '2',
37+
},
38+
])('preserves the supported $view state through sign-in', async (params) => {
39+
await expect(AccessRequestsPage({ searchParams: Promise.resolve(params) })).rejects.toThrow(
40+
'Redirect'
41+
)
42+
const loginUrl = new URL(redirect.mock.calls[0][0], 'https://example.com')
43+
expect(loginUrl.pathname).toBe('/login')
44+
const callback = new URL(loginUrl.searchParams.get('callbackUrl')!, loginUrl.origin)
45+
expect(callback.pathname).toBe('/access-requests')
46+
expect(Object.fromEntries(callback.searchParams)).toEqual(params)
47+
})
48+
49+
it('drops invalid and unsupported state instead of forwarding raw query parameters', async () => {
50+
await expect(
51+
AccessRequestsPage({
52+
searchParams: Promise.resolve({
53+
organizationId: 'organization',
54+
view: 'invalid',
55+
page: '40001',
56+
search: 'x'.repeat(201),
57+
requestId: 'x'.repeat(129),
58+
'request-page': '-1',
59+
'request-status': 'invalid',
60+
callbackUrl: 'https://example.com/untrusted',
61+
}),
62+
})
63+
).rejects.toThrow('Redirect')
64+
expect(redirect).toHaveBeenCalledWith(
65+
`/login?callbackUrl=${encodeURIComponent('/access-requests?organizationId=organization')}`
66+
)
67+
})
68+
})

‎apps/sim/app/access-requests/page.tsx‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { Suspense } from 'react'
22
import { ChipLink } from '@sim/emcn'
33
import type { Metadata } from 'next'
44
import { redirect } from 'next/navigation'
5-
import { createSearchParamsCache } from 'nuqs/server'
5+
import { createSearchParamsCache, createSerializer } from 'nuqs/server'
66
import { AccessRequestsLoading } from '@/components/access-requests/access-requests-loading'
77
import { MyAccessRequests } from '@/components/access-requests/my-access-requests'
88
import { OrganizationAccessRequests } from '@/components/access-requests/organization-access-requests'
@@ -22,19 +22,16 @@ interface AccessRequestsPageProps {
2222
}
2323

2424
const entrySearchParams = createSearchParamsCache(accessRequestEntrySearchParams)
25+
const serializeEntrySearchParams = createSerializer(accessRequestEntrySearchParams)
2526

2627
/** Session-only entry so access requests remain reachable outside the organization Search rollout. */
2728
export default async function AccessRequestsPage({ searchParams }: AccessRequestsPageProps) {
2829
const [rawParams, session] = await Promise.all([searchParams, getSession()])
2930
const params = entrySearchParams.parse(rawParams)
30-
const query = new URLSearchParams()
31-
if (params.organizationId) query.set('organizationId', params.organizationId)
32-
if (params.view !== 'requests') query.set('view', params.view)
33-
if (params.requestId) query.set('requestId', params.requestId)
3431
if (!session?.user) {
3532
redirect(
3633
buildAuthCrossLink('/login', {
37-
callbackUrl: `/access-requests?${query}`,
34+
callbackUrl: serializeEntrySearchParams('/access-requests', params),
3835
isInviteFlow: false,
3936
})
4037
)

‎apps/sim/app/api/permission-groups/user/route.test.ts‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/** @vitest-environment node */
2-
import { createMockRequest } from '@sim/testing'
3-
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
import { createMockRequest, resetEnvFlagsMock, setEnvFlags } from '@sim/testing'
3+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
44

55
const mocks = vi.hoisted(() => ({
66
session: vi.fn(),
@@ -22,7 +22,10 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({ isOrganizationAdminOrOwne
2222
vi.mock('@/lib/billing/core/subscription', () => ({
2323
isOrganizationOnEnterprisePlan: mocks.enterprise,
2424
}))
25-
vi.mock('@/lib/permission-groups/resolve.server', () => ({ resolveWorkspaceGroup: mocks.group }))
25+
vi.mock('@/lib/permission-groups/resolve.server', async (importOriginal) => ({
26+
...(await importOriginal<typeof import('@/lib/permission-groups/resolve.server')>()),
27+
resolveWorkspaceGroup: mocks.group,
28+
}))
2629

2730
import { userPermissionConfigSchema } from '@/lib/api/contracts/permission-groups'
2831
import { OrchestrationError } from '@/lib/core/orchestration/types'
@@ -53,6 +56,7 @@ function get(query = '?workspaceId=workspace') {
5356

5457
beforeEach(() => {
5558
vi.clearAllMocks()
59+
setEnvFlags({ isHosted: true, isAccessControlEnabled: true })
5660
mocks.session.mockResolvedValue({
5761
user: { id: 'viewer' },
5862
session: { id: 'session', activeOrganizationId: 'unrelated-org' },
@@ -64,7 +68,40 @@ beforeEach(() => {
6468
mocks.group.mockResolvedValue(null)
6569
})
6670

71+
afterEach(resetEnvFlagsMock)
72+
6773
describe('user permission policy shared read', () => {
74+
it.each([
75+
{ hosted: false, accessControl: false, entitled: false },
76+
{ hosted: false, accessControl: true, entitled: true },
77+
{ hosted: true, accessControl: false, entitled: true },
78+
])(
79+
'matches the active permission regime ($hosted, $accessControl)',
80+
async ({ hosted, accessControl, entitled }) => {
81+
setEnvFlags({
82+
isHosted: hosted,
83+
isAccessControlEnabled: accessControl,
84+
isBillingEnabled: false,
85+
})
86+
mocks.admin.mockResolvedValue(true)
87+
const group = {
88+
permissionGroupId: 'group',
89+
groupName: 'Restricted',
90+
config: { ...DEFAULT_PERMISSION_GROUP_CONFIG, hideCopilot: true },
91+
}
92+
mocks.group.mockResolvedValue(group)
93+
const expected = { ...unrestricted, ...(entitled ? group : {}), entitled, isOrgAdmin: true }
94+
expect(await (await get()).json()).toEqual(expected)
95+
expect(
96+
await readUserPermissionConfig.execute({ principal, input: { workspaceId: 'workspace' } })
97+
).toEqual(expected)
98+
if (!entitled) {
99+
expect(mocks.group).not.toHaveBeenCalled()
100+
expect(mocks.enterprise).not.toHaveBeenCalled()
101+
}
102+
}
103+
)
104+
68105
it('authenticates before parsing or protected lookups', async () => {
69106
mocks.session.mockResolvedValue(null)
70107
expect((await get('')).status).toBe(401)

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/toolbar/toolbar.test.tsx‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,4 +204,21 @@ describe('toolbar access requests', () => {
204204
expect(container.textContent).not.toContain('Access required')
205205
expect(container.textContent).not.toContain('Locked')
206206
})
207+
208+
it('does not reopen a request after requests are disabled and re-enabled', () => {
209+
act(() => root.render(<Toolbar />))
210+
act(() =>
211+
container
212+
.querySelector<HTMLButtonElement>('[aria-label="Request access to Locked tool"]')
213+
?.click()
214+
)
215+
expect(document.querySelector('[role="dialog"]')).not.toBeNull()
216+
discovery.mockReturnValue({ data: { enabled: false } })
217+
act(() => root.render(<Toolbar isActive={false} />))
218+
expect(document.querySelector('[role="dialog"]')).toBeNull()
219+
discovery.mockReturnValue({ data: { enabled: true } })
220+
act(() => root.render(<Toolbar isActive />))
221+
expect(document.querySelector('[role="dialog"]')).toBeNull()
222+
expect(container.querySelector('[aria-label="Request access to Locked tool"]')).not.toBeNull()
223+
})
207224
})

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/toolbar/toolbar.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,13 @@ export const Toolbar = memo(
500500
allTools.find((item) => item.type === requestedBlockType))
501501
: undefined
502502

503+
if (
504+
requestedBlockType !== null &&
505+
(!requestedBlock || !workspaceId || !accessRequestsEnabled)
506+
) {
507+
setRequestedBlockType(null)
508+
}
509+
503510
// Published custom blocks are their own section. Exclude disabled blocks (still
504511
// resolvable so placed instances survive, but not offered for new placement) and
505512
// the block bound to the CURRENT workflow — adding a workflow's own block recurses.

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,16 +217,21 @@ export const Panel = memo(function Panel() {
217217
scope: usageLimitScope,
218218
isLoading: isUsageGateLoading,
219219
} = useUsageLimits({ workspaceId })
220+
const isMemberLimitExceeded = usageExceeded && usageLimitScope === 'member'
220221
const memberLimitRequest = useDiscoverAccessRequests(
221222
{ kind: 'workspace', workspaceId, targetKind: 'usage_limit', limit: 1, offset: 0 },
222-
usageExceeded && usageLimitScope === 'member'
223+
isMemberLimitExceeded
223224
)
224225
const [showLimitRequest, setShowLimitRequest] = useState(false)
225226
const memberLimitTarget =
226-
memberLimitRequest.isSuccess && memberLimitRequest.data.enabled
227+
isMemberLimitExceeded && memberLimitRequest.isSuccess && memberLimitRequest.data.enabled
227228
? memberLimitRequest.data.entries.find((entry) => entry.state === 'requestable')
228229
: undefined
229230

231+
if (showLimitRequest && !memberLimitTarget) {
232+
setShowLimitRequest(false)
233+
}
234+
230235
// Workflow execution hook
231236
const { handleRunWorkflow, handleCancelExecution, isExecuting } = useWorkflowExecution()
232237

‎apps/sim/components/access-requests/access-request-review.tsx‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,6 @@ export function AccessRequestReview({
118118
<p className='break-words text-[var(--text-body)] text-sm'>
119119
{data.group?.name ?? 'No longer available'}
120120
</p>
121-
<p className='text-[var(--text-muted)] text-sm'>
122-
May affect up to {data.impact.memberCount}{' '}
123-
{data.impact.memberCount === 1 ? 'person' : 'people'} across{' '}
124-
{data.impact.workspaceCount}{' '}
125-
{data.impact.workspaceCount === 1 ? 'workspace' : 'workspaces'}.
126-
</p>
127121
</ChipModalField>
128122
)}
129123
{pending && alreadyAvailable && (
@@ -171,7 +165,7 @@ export function AccessRequestReview({
171165
value={newLimit}
172166
onChange={setNewLimit}
173167
required
174-
hint='Applies to this member. Enter a whole number above their current limit.'
168+
hint='Enter a whole number above the current limit.'
175169
/>
176170
)}
177171
</>

‎apps/sim/components/access-requests/my-access-requests.test.tsx‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,28 @@ describe('compact requester history', () => {
101101
expect(mocks.mine).toHaveBeenCalledWith(scope, 50, undefined, false)
102102
expect(mocks.mine).toHaveBeenCalledWith(scope, 0, 'request')
103103
expect(mocks.discovery).toHaveBeenCalledWith(
104-
expect.objectContaining({ search: 'slack', offset: 50 }),
104+
expect.objectContaining({ search: 'slack', offset: 50, state: 'requestable' }),
105105
true
106106
)
107107
})
108+
109+
it('exposes the selected view and resets pagination when switching views through nuqs', async () => {
110+
render('?view=catalog&search=slack&page=2')
111+
const views = container.querySelector('[role="radiogroup"][aria-label="Access request views"]')
112+
expect(views?.querySelector('[role="radio"][aria-checked="true"]')?.textContent).toBe(
113+
'Browse access'
114+
)
115+
const history = views?.querySelector<HTMLButtonElement>('[role="radio"][value="requests"]')
116+
expect(history).not.toBeNull()
117+
await act(async () => history?.click())
118+
expect(views?.querySelector('[role="radio"][aria-checked="true"]')?.textContent).toBe(
119+
'My requests'
120+
)
121+
await vi.waitFor(() =>
122+
expect(mocks.url).toHaveBeenLastCalledWith(
123+
expect.objectContaining({ queryString: '?search=slack' })
124+
)
125+
)
126+
expect(mocks.mine).toHaveBeenLastCalledWith(scope, 0, undefined, true)
127+
})
108128
})

‎apps/sim/components/access-requests/my-access-requests.tsx‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client'
22

3-
import { Chip, ChipInput, ChipLink, ChipTag } from '@sim/emcn'
3+
import { Chip, ChipInput, ChipLink, ChipSwitch, ChipTag } from '@sim/emcn'
44
import { Lock, Search } from '@sim/emcn/icons'
55
import { useQueryStates } from 'nuqs'
66
import { MyAccessRequestDetails } from '@/components/access-requests/my-access-request-details'
@@ -66,20 +66,15 @@ export function MyAccessRequests({ scope }: MyAccessRequestsProps) {
6666
<ChipLink href={WORKSPACES_PATH}>Your workspaces</ChipLink>
6767
)}
6868
</div>
69-
<div className='flex flex-wrap items-center gap-2' aria-label='Access request views'>
70-
<Chip
71-
active={view === 'requests'}
72-
onClick={() => void setParams({ view: 'requests', page: 0, requestId: null })}
73-
>
74-
My requests
75-
</Chip>
76-
<Chip
77-
active={view === 'catalog'}
78-
onClick={() => void setParams({ view: 'catalog', page: 0, requestId: null })}
79-
>
80-
Browse access
81-
</Chip>
82-
</div>
69+
<ChipSwitch
70+
aria-label='Access request views'
71+
options={[
72+
{ value: 'requests', label: 'My requests' },
73+
{ value: 'catalog', label: 'Browse access' },
74+
]}
75+
value={view}
76+
onChange={(value) => void setParams({ view: value, page: 0, requestId: null })}
77+
/>
8378
{view === 'catalog' && (
8479
<ChipInput
8580
icon={Search}
@@ -132,7 +127,7 @@ export function MyAccessRequests({ scope }: MyAccessRequestsProps) {
132127
) : !catalog.data?.enabled ? (
133128
<EmptyState
134129
title='Access requests are unavailable'
135-
description='Your organization is not accepting new access requests. Existing requests remain in My requests.'
130+
description='Your organization is not accepting new requests.'
136131
/>
137132
) : (
138133
<div className={RESOURCE_LIST_STACK}>
@@ -148,9 +143,6 @@ export function MyAccessRequests({ scope }: MyAccessRequestsProps) {
148143
icon={entry.state === 'allowed' ? undefined : <Lock />}
149144
iconVariant='plain'
150145
title={entry.label}
151-
description={
152-
entry.reason ?? (entry.state === 'allowed' ? 'Available to you' : undefined)
153-
}
154146
badge={
155147
entry.state === 'allowed' ? (
156148
<ChipTag variant='gray'>Available</ChipTag>

0 commit comments

Comments
 (0)