Skip to content

Commit cf41d74

Browse files
waleedlatif1waleedlatif1
committed
fix(access-requests): show readable integration policy names
1 parent e2bf544 commit cf41d74

4 files changed

Lines changed: 546 additions & 35 deletions

File tree

‎apps/sim/components/access-requests/policy-changes.test.ts‎

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
* @vitest-environment node
33
*/
44
import { describe, expect, it } from 'vitest'
5-
import { describePolicyChange } from '@/components/access-requests/policy-changes'
5+
import {
6+
describePolicyChange,
7+
describePolicyValue,
8+
} from '@/components/access-requests/policy-changes'
69

710
const target = { kind: 'integration', id: 'slack_v2' } as const
811

@@ -68,7 +71,7 @@ describe('permission change summaries', () => {
6871
target,
6972
'Slack'
7073
)
71-
).toBe('Allow Slack; Remove github_v2')
74+
).toBe('Allow Slack; Remove GitHub')
7275
})
7376
it('distinguishes unrestricted and empty allowlists', () => {
7477
expect(
@@ -108,3 +111,93 @@ describe('permission change summaries', () => {
108111
).toBe('Restricted → Allowed')
109112
})
110113
})
114+
115+
describe('permission change details', () => {
116+
it('uses canonical names for every integration and preserves the original snapshot', () => {
117+
const values = ['github_v2', 'notion_v2', 'slack_v2', 'loop', 'parallel']
118+
const before = structuredClone(values)
119+
expect(describePolicyValue(values, 'allowedIntegrations', target, 'Slack')).toBe(
120+
'GitHub, Notion, Slack, Loop, Parallel'
121+
)
122+
expect(values).toEqual(before)
123+
})
124+
125+
it('collapses only equivalent integration aliases in both lists and change summaries', () => {
126+
expect(
127+
describePolicyValue(
128+
['GitHub', 'github_v2', 'notion', 'notion_v2'],
129+
'allowedIntegrations',
130+
target,
131+
'Slack'
132+
)
133+
).toBe('GitHub, Notion')
134+
expect(
135+
describePolicyChange(
136+
{
137+
configKey: 'allowedIntegrations',
138+
label: 'Integrations',
139+
before: ['github', 'notion'],
140+
after: ['github_v2', 'notion_v2', 'slack_v2'],
141+
},
142+
target,
143+
'Slack'
144+
)
145+
).toBe('Allow Slack')
146+
expect(
147+
describePolicyChange(
148+
{
149+
configKey: 'allowedIntegrations',
150+
label: 'Integrations',
151+
before: ['slack'],
152+
after: ['slack_v2'],
153+
},
154+
target,
155+
'Slack'
156+
)
157+
).toBe('No membership change')
158+
})
159+
160+
it('retains unknown IDs exactly and does not read inherited object properties', () => {
161+
expect(
162+
describePolicyValue(
163+
['Unknown_Integration_v2', 'constructor', 'toString', '__proto__'],
164+
'allowedIntegrations',
165+
target,
166+
'Slack'
167+
)
168+
).toBe('Unknown_Integration_v2, constructor, toString, __proto__')
169+
})
170+
171+
it('uses the request label for an integration outside the built-in registry', () => {
172+
expect(
173+
describePolicyValue(
174+
['Custom_Integration'],
175+
'allowedIntegrations',
176+
{ kind: 'integration', id: 'custom_integration' },
177+
'Custom integration'
178+
)
179+
).toBe('Custom integration')
180+
})
181+
182+
it('keeps meaningful model and tool versions distinct', () => {
183+
expect(describePolicyValue(['model_v1', 'model_v2'], 'deniedModels', target, 'Slack')).toBe(
184+
'model_v1, model_v2'
185+
)
186+
expect(describePolicyValue(['tool_v1', 'tool_v2'], 'deniedTools', target, 'Slack')).toBe(
187+
'tool_v1, tool_v2'
188+
)
189+
})
190+
191+
it('preserves the difference between unrestricted, empty, and boolean values', () => {
192+
expect(describePolicyValue(null, 'allowedIntegrations', target, 'Slack')).toBe('All allowed')
193+
expect(describePolicyValue([], 'allowedIntegrations', target, 'Slack')).toBe('None')
194+
expect(
195+
describePolicyValue(
196+
true,
197+
'hideTablesTab',
198+
{ kind: 'feature', configKey: 'hideTablesTab' },
199+
'Tables'
200+
)
201+
).toBe('Restricted')
202+
})
203+
})

‎apps/sim/components/access-requests/policy-changes.tsx‎

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import type {
77
AccessRequestPreviewResponse,
88
AccessRequestTarget,
99
} from '@/lib/api/contracts/access-requests'
10+
import { BLOCK_NAMES } from '@/lib/permission-groups/block-names.generated'
1011
import { PERMISSION_GROUP_FIELDS } from '@/lib/permission-groups/fields'
12+
import { resolveAccessControlBlockType } from '@/lib/permission-groups/integration-allowlist'
1113

1214
interface PolicyChangesProps {
1315
changes: AccessRequestPolicyChange[]
@@ -16,10 +18,44 @@ interface PolicyChangesProps {
1618
targetLabel: string
1719
}
1820

19-
function describePolicyValue(value: AccessRequestPolicyChange['before']): string {
21+
function describePolicyItems(
22+
values: string[],
23+
configKey: AccessRequestPolicyChange['configKey'],
24+
target: AccessRequestTarget,
25+
targetLabel: string
26+
): ReadonlyMap<string, string> {
27+
const items = new Map<string, string>()
28+
const integrationTarget =
29+
target.kind === 'integration'
30+
? resolveAccessControlBlockType(target.id.toLowerCase()).toLowerCase()
31+
: null
32+
for (const value of values) {
33+
if (configKey === 'allowedIntegrations') {
34+
const canonical = resolveAccessControlBlockType(value.toLowerCase()).toLowerCase()
35+
const label = Object.hasOwn(BLOCK_NAMES, canonical)
36+
? BLOCK_NAMES[canonical]!
37+
: canonical === integrationTarget
38+
? targetLabel
39+
: value
40+
items.set(canonical, label)
41+
} else {
42+
items.set(value, target.kind !== 'feature' && value === target.id ? targetLabel : value)
43+
}
44+
}
45+
return items
46+
}
47+
48+
export function describePolicyValue(
49+
value: AccessRequestPolicyChange['before'],
50+
configKey: AccessRequestPolicyChange['configKey'],
51+
target: AccessRequestTarget,
52+
targetLabel: string
53+
): string {
2054
if (value === null) return 'All allowed'
2155
if (typeof value === 'boolean') return value ? 'Restricted' : 'Allowed'
22-
return value.length ? value.join(', ') : 'None'
56+
return value.length
57+
? [...describePolicyItems(value, configKey, target, targetLabel).values()].join(', ')
58+
: 'None'
2359
}
2460

2561
export function describePolicyChange(
@@ -28,24 +64,22 @@ export function describePolicyChange(
2864
targetLabel: string
2965
): string {
3066
const { before, after } = change
31-
if (typeof after === 'boolean')
32-
return `${describePolicyValue(before)} → ${describePolicyValue(after)}`
67+
const describe = (value: AccessRequestPolicyChange['before']) =>
68+
describePolicyValue(value, change.configKey, target, targetLabel)
69+
if (typeof after === 'boolean') return `${describe(before)} → ${describe(after)}`
3370
if (after === null) return 'Allow all'
34-
const label = (value: string) =>
35-
target.kind !== 'feature' && value === target.id ? targetLabel : value
71+
const next = describePolicyItems(after, change.configKey, target, targetLabel)
3672
if (before === null)
37-
return after.length ? `Allow only ${after.map(label).join(', ')}` : 'Allow none'
38-
if (!Array.isArray(before))
39-
return `${describePolicyValue(before)} → ${describePolicyValue(after)}`
40-
const previous = new Set(before)
41-
const next = new Set(after)
42-
const added = after.filter((value) => !previous.has(value))
43-
const removed = before.filter((value) => !next.has(value))
73+
return next.size ? `Allow only ${[...next.values()].join(', ')}` : 'Allow none'
74+
if (!Array.isArray(before)) return `${describe(before)} → ${describe(after)}`
75+
const previous = describePolicyItems(before, change.configKey, target, targetLabel)
76+
const added = [...next].filter(([id]) => !previous.has(id)).map(([, label]) => label)
77+
const removed = [...previous].filter(([id]) => !next.has(id)).map(([, label]) => label)
4478
const denylist = PERMISSION_GROUP_FIELDS[change.configKey].kind === 'denylist'
4579
return (
4680
[
47-
added.length ? `${denylist ? 'Block' : 'Allow'} ${added.map(label).join(', ')}` : '',
48-
removed.length ? `${denylist ? 'Unblock' : 'Remove'} ${removed.map(label).join(', ')}` : '',
81+
added.length ? `${denylist ? 'Block' : 'Allow'} ${added.join(', ')}` : '',
82+
removed.length ? `${denylist ? 'Unblock' : 'Remove'} ${removed.join(', ')}` : '',
4983
]
5084
.filter(Boolean)
5185
.join('; ') || 'No membership change'
@@ -94,11 +128,11 @@ export function PolicyChanges({ changes, impact, target, targetLabel }: PolicyCh
94128
<dl className='grid grid-cols-[auto_minmax(0,1fr)] gap-x-3 gap-y-1 text-sm'>
95129
<dt className='text-[var(--text-muted)]'>Before</dt>
96130
<dd className='whitespace-pre-wrap break-words text-[var(--text-body)]'>
97-
{describePolicyValue(change.before)}
131+
{describePolicyValue(change.before, change.configKey, target, targetLabel)}
98132
</dd>
99133
<dt className='text-[var(--text-muted)]'>After</dt>
100134
<dd className='whitespace-pre-wrap break-words text-[var(--text-body)]'>
101-
{describePolicyValue(change.after)}
135+
{describePolicyValue(change.after, change.configKey, target, targetLabel)}
102136
</dd>
103137
</dl>
104138
</ChipModalField>

0 commit comments

Comments
 (0)