Skip to content

Commit 50a48ef

Browse files
danny-aviladanny-avila
authored
🧬 fix: Backfill Missing SHARE Permissions and Migrate Legacy SHARED_GLOBAL Fields (#11854)
* chore: Migrate legacy SHARED_GLOBAL permissions to SHARE and clean up orphaned fields - Implemented migration logic to convert legacy SHARED_GLOBAL permissions to SHARE for PROMPTS and AGENTS, preserving user intent. - Added cleanup process to remove orphaned SHARED_GLOBAL fields from the database after the schema change. - Enhanced unit tests to verify migration and cleanup functionality, ensuring correct behavior for existing roles and permissions. * fix: Enhance migration of SHARED_GLOBAL to SHARE permissions - Updated the `updateAccessPermissions` function to ensure that SHARED_GLOBAL values are inherited into SHARE when SHARE is absent from both the database and the update payload. - Implemented logic to prevent overwriting explicit SHARE values provided in the update, preserving user intent. - Enhanced unit tests to cover various scenarios, including migration from SHARED_GLOBAL to SHARE and ensuring orphaned SHARED_GLOBAL fields are cleaned up appropriately.
1 parent 42718fa commit 50a48ef

File tree

4 files changed

+370
-19
lines changed

4 files changed

+370
-19
lines changed

‎api/models/Role.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,28 @@ async function updateAccessPermissions(roleName, permissionsUpdate, roleData) {
114114
}
115115
}
116116

117+
// Migrate legacy SHARED_GLOBAL → SHARE for PROMPTS and AGENTS.
118+
// SHARED_GLOBAL was removed in favour of SHARE in PR #11283. If the DB still has
119+
// SHARED_GLOBAL but not SHARE, inherit the value so sharing intent is preserved.
120+
const legacySharedGlobalTypes = ['PROMPTS', 'AGENTS'];
121+
for (const legacyPermType of legacySharedGlobalTypes) {
122+
const existingTypePerms = currentPermissions[legacyPermType];
123+
if (
124+
existingTypePerms &&
125+
'SHARED_GLOBAL' in existingTypePerms &&
126+
!('SHARE' in existingTypePerms) &&
127+
updates[legacyPermType] &&
128+
// Don't override an explicit SHARE value the caller already provided
129+
!('SHARE' in updates[legacyPermType])
130+
) {
131+
const inheritedValue = existingTypePerms['SHARED_GLOBAL'];
132+
updates[legacyPermType]['SHARE'] = inheritedValue;
133+
logger.info(
134+
`Migrating '${roleName}' role ${legacyPermType}.SHARED_GLOBAL=${inheritedValue} → SHARE`,
135+
);
136+
}
137+
}
138+
117139
for (const [permissionType, permissions] of Object.entries(updates)) {
118140
const currentTypePermissions = currentPermissions[permissionType] || {};
119141
updatedPermissions[permissionType] = { ...currentTypePermissions };
@@ -129,6 +151,32 @@ async function updateAccessPermissions(roleName, permissionsUpdate, roleData) {
129151
}
130152
}
131153

154+
// Clean up orphaned SHARED_GLOBAL fields left in DB after the schema rename.
155+
// Since we $set the full permissions object, deleting from updatedPermissions
156+
// is sufficient to remove the field from MongoDB.
157+
for (const legacyPermType of legacySharedGlobalTypes) {
158+
const existingTypePerms = currentPermissions[legacyPermType];
159+
if (existingTypePerms && 'SHARED_GLOBAL' in existingTypePerms) {
160+
if (!updates[legacyPermType]) {
161+
// permType wasn't in the update payload so the migration block above didn't run.
162+
// Create a writable copy and handle the SHARED_GLOBAL → SHARE inheritance here
163+
// to avoid removing SHARED_GLOBAL without writing SHARE (data loss).
164+
updatedPermissions[legacyPermType] = { ...existingTypePerms };
165+
if (!('SHARE' in existingTypePerms)) {
166+
updatedPermissions[legacyPermType]['SHARE'] = existingTypePerms['SHARED_GLOBAL'];
167+
logger.info(
168+
`Migrating '${roleName}' role ${legacyPermType}.SHARED_GLOBAL=${existingTypePerms['SHARED_GLOBAL']} → SHARE`,
169+
);
170+
}
171+
}
172+
delete updatedPermissions[legacyPermType]['SHARED_GLOBAL'];
173+
hasChanges = true;
174+
logger.info(
175+
`Removed legacy SHARED_GLOBAL field from '${roleName}' role ${legacyPermType} permissions`,
176+
);
177+
}
178+
}
179+
132180
if (hasChanges) {
133181
const updateObj = { permissions: updatedPermissions };
134182

‎api/models/Role.spec.js‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,112 @@ describe('updateAccessPermissions', () => {
233233
expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO]).toEqual({ USE: true });
234234
});
235235

236+
it('should inherit SHARED_GLOBAL value into SHARE when SHARE is absent from both DB and update', async () => {
237+
// Simulates the startup backfill path: caller sends SHARE_PUBLIC but not SHARE;
238+
// migration should inherit SHARED_GLOBAL to preserve the deployment's sharing intent.
239+
await Role.collection.insertOne({
240+
name: SystemRoles.USER,
241+
permissions: {
242+
[PermissionTypes.PROMPTS]: { USE: true, CREATE: true, SHARED_GLOBAL: true },
243+
[PermissionTypes.AGENTS]: { USE: true, CREATE: true, SHARED_GLOBAL: false },
244+
},
245+
});
246+
247+
await updateAccessPermissions(SystemRoles.USER, {
248+
// No explicit SHARE — migration should inherit from SHARED_GLOBAL
249+
[PermissionTypes.PROMPTS]: { SHARE_PUBLIC: false },
250+
[PermissionTypes.AGENTS]: { SHARE_PUBLIC: false },
251+
});
252+
253+
const updatedRole = await getRoleByName(SystemRoles.USER);
254+
255+
// SHARED_GLOBAL=true → SHARE=true (inherited)
256+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true);
257+
// SHARED_GLOBAL=false → SHARE=false (inherited)
258+
expect(updatedRole.permissions[PermissionTypes.AGENTS].SHARE).toBe(false);
259+
// SHARED_GLOBAL cleaned up
260+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined();
261+
expect(updatedRole.permissions[PermissionTypes.AGENTS].SHARED_GLOBAL).toBeUndefined();
262+
});
263+
264+
it('should respect explicit SHARE in update payload and not override it with SHARED_GLOBAL', async () => {
265+
// Caller explicitly passes SHARE: false even though SHARED_GLOBAL=true in DB.
266+
// The explicit intent must win; migration must not silently overwrite it.
267+
await Role.collection.insertOne({
268+
name: SystemRoles.USER,
269+
permissions: {
270+
[PermissionTypes.PROMPTS]: { USE: true, SHARED_GLOBAL: true },
271+
},
272+
});
273+
274+
await updateAccessPermissions(SystemRoles.USER, {
275+
[PermissionTypes.PROMPTS]: { SHARE: false }, // explicit false — should be preserved
276+
});
277+
278+
const updatedRole = await getRoleByName(SystemRoles.USER);
279+
280+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(false);
281+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined();
282+
});
283+
284+
it('should migrate SHARED_GLOBAL to SHARE even when the permType is not in the update payload', async () => {
285+
// Bug #2 regression: cleanup block removes SHARED_GLOBAL but migration block only
286+
// runs when the permType is in the update payload. Without the fix, SHARE would be
287+
// lost when any other permType (e.g. MULTI_CONVO) is the only thing being updated.
288+
await Role.collection.insertOne({
289+
name: SystemRoles.USER,
290+
permissions: {
291+
[PermissionTypes.PROMPTS]: {
292+
USE: true,
293+
SHARED_GLOBAL: true, // legacy — NO SHARE present
294+
},
295+
[PermissionTypes.MULTI_CONVO]: { USE: false },
296+
},
297+
});
298+
299+
// Only update MULTI_CONVO — PROMPTS is intentionally absent from the payload
300+
await updateAccessPermissions(SystemRoles.USER, {
301+
[PermissionTypes.MULTI_CONVO]: { USE: true },
302+
});
303+
304+
const updatedRole = await getRoleByName(SystemRoles.USER);
305+
306+
// SHARE should have been inherited from SHARED_GLOBAL, not silently dropped
307+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true);
308+
// SHARED_GLOBAL should be removed
309+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined();
310+
// Original USE should be untouched
311+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].USE).toBe(true);
312+
// The actual update should have applied
313+
expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO].USE).toBe(true);
314+
});
315+
316+
it('should remove orphaned SHARED_GLOBAL when SHARE already exists and permType is not in update', async () => {
317+
// Safe cleanup case: SHARE already set, SHARED_GLOBAL is just orphaned noise.
318+
// SHARE must not be changed; SHARED_GLOBAL must be removed.
319+
await Role.collection.insertOne({
320+
name: SystemRoles.USER,
321+
permissions: {
322+
[PermissionTypes.PROMPTS]: {
323+
USE: true,
324+
SHARE: true, // already migrated
325+
SHARED_GLOBAL: true, // orphaned
326+
},
327+
[PermissionTypes.MULTI_CONVO]: { USE: false },
328+
},
329+
});
330+
331+
await updateAccessPermissions(SystemRoles.USER, {
332+
[PermissionTypes.MULTI_CONVO]: { USE: true },
333+
});
334+
335+
const updatedRole = await getRoleByName(SystemRoles.USER);
336+
337+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined();
338+
expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true);
339+
expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO].USE).toBe(true);
340+
});
341+
236342
it('should not update MULTI_CONVO permissions when no changes are needed', async () => {
237343
await new Role({
238344
name: SystemRoles.USER,

‎packages/api/src/app/permissions.spec.ts‎

Lines changed: 114 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -776,11 +776,19 @@ describe('updateInterfacePermissions - permissions', () => {
776776
});
777777

778778
it('should only update permissions that do not exist when no config provided', async () => {
779-
// Mock that some permissions already exist
779+
// Mock that some permissions already exist (with SHARE/SHARE_PUBLIC as they would be post-#11283)
780780
mockGetRoleByName.mockResolvedValue({
781781
permissions: {
782-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
783-
[PermissionTypes.AGENTS]: { [Permissions.USE]: true },
782+
[PermissionTypes.PROMPTS]: {
783+
[Permissions.USE]: false,
784+
[Permissions.SHARE]: false,
785+
[Permissions.SHARE_PUBLIC]: false,
786+
},
787+
[PermissionTypes.AGENTS]: {
788+
[Permissions.USE]: true,
789+
[Permissions.SHARE]: false,
790+
[Permissions.SHARE_PUBLIC]: false,
791+
},
784792
},
785793
});
786794

@@ -892,30 +900,38 @@ describe('updateInterfacePermissions - permissions', () => {
892900
SystemRoles.USER,
893901
expectedPermissionsForUser,
894902
expect.objectContaining({
895-
permissions: {
896-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
897-
[PermissionTypes.AGENTS]: { [Permissions.USE]: true },
898-
},
903+
permissions: expect.objectContaining({
904+
[PermissionTypes.PROMPTS]: expect.objectContaining({ [Permissions.USE]: false }),
905+
[PermissionTypes.AGENTS]: expect.objectContaining({ [Permissions.USE]: true }),
906+
}),
899907
}),
900908
);
901909
expect(mockUpdateAccessPermissions).toHaveBeenCalledWith(
902910
SystemRoles.ADMIN,
903911
expectedPermissionsForAdmin,
904912
expect.objectContaining({
905-
permissions: {
906-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
907-
[PermissionTypes.AGENTS]: { [Permissions.USE]: true },
908-
},
913+
permissions: expect.objectContaining({
914+
[PermissionTypes.PROMPTS]: expect.objectContaining({ [Permissions.USE]: false }),
915+
[PermissionTypes.AGENTS]: expect.objectContaining({ [Permissions.USE]: true }),
916+
}),
909917
}),
910918
);
911919
});
912920

913921
it('should override existing permissions when explicitly configured', async () => {
914-
// Mock that some permissions already exist
922+
// Mock that some permissions already exist (with SHARE/SHARE_PUBLIC as they would be post-#11283)
915923
mockGetRoleByName.mockResolvedValue({
916924
permissions: {
917-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
918-
[PermissionTypes.AGENTS]: { [Permissions.USE]: false },
925+
[PermissionTypes.PROMPTS]: {
926+
[Permissions.USE]: false,
927+
[Permissions.SHARE]: false,
928+
[Permissions.SHARE_PUBLIC]: false,
929+
},
930+
[PermissionTypes.AGENTS]: {
931+
[Permissions.USE]: false,
932+
[Permissions.SHARE]: false,
933+
[Permissions.SHARE_PUBLIC]: false,
934+
},
919935
[PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false },
920936
},
921937
});
@@ -1340,8 +1356,13 @@ describe('updateInterfacePermissions - permissions', () => {
13401356

13411357
it('should leave all existing permissions unchanged when nothing is configured', async () => {
13421358
// Mock existing permissions with values that differ from defaults
1359+
// SHARE/SHARE_PUBLIC included as they would be in a post-#11283 DB document
13431360
const existingUserPermissions = {
1344-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
1361+
[PermissionTypes.PROMPTS]: {
1362+
[Permissions.USE]: false,
1363+
[Permissions.SHARE]: false,
1364+
[Permissions.SHARE_PUBLIC]: false,
1365+
},
13451366
[PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false },
13461367
[PermissionTypes.MEMORIES]: { [Permissions.USE]: false },
13471368
[PermissionTypes.PEOPLE_PICKER]: {
@@ -1400,10 +1421,14 @@ describe('updateInterfacePermissions - permissions', () => {
14001421
});
14011422

14021423
it('should only update explicitly configured permissions and leave others unchanged', async () => {
1403-
// Mock existing permissions
1424+
// Mock existing permissions (with SHARE/SHARE_PUBLIC as they would be post-#11283)
14041425
mockGetRoleByName.mockResolvedValue({
14051426
permissions: {
1406-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: false },
1427+
[PermissionTypes.PROMPTS]: {
1428+
[Permissions.USE]: false,
1429+
[Permissions.SHARE]: false,
1430+
[Permissions.SHARE_PUBLIC]: false,
1431+
},
14071432
[PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false },
14081433
[PermissionTypes.MEMORIES]: { [Permissions.USE]: false },
14091434
[PermissionTypes.PEOPLE_PICKER]: {
@@ -1729,8 +1754,12 @@ describe('updateInterfacePermissions - permissions', () => {
17291754
[Permissions.UPDATE]: true,
17301755
[Permissions.OPT_OUT]: true,
17311756
},
1732-
// Other existing permissions
1733-
[PermissionTypes.PROMPTS]: { [Permissions.USE]: true },
1757+
// Other existing permissions (with SHARE/SHARE_PUBLIC as they would be post-#11283)
1758+
[PermissionTypes.PROMPTS]: {
1759+
[Permissions.USE]: true,
1760+
[Permissions.SHARE]: false,
1761+
[Permissions.SHARE_PUBLIC]: false,
1762+
},
17341763
[PermissionTypes.BOOKMARKS]: { [Permissions.USE]: true },
17351764
},
17361765
});
@@ -2005,4 +2034,70 @@ describe('updateInterfacePermissions - permissions', () => {
20052034
expect(userCall[1][PermissionTypes.PROMPTS]).not.toHaveProperty(Permissions.SHARE);
20062035
expect(userCall[1][PermissionTypes.PROMPTS]).not.toHaveProperty(Permissions.SHARE_PUBLIC);
20072036
});
2037+
2038+
it('should backfill SHARE/SHARE_PUBLIC when missing from an existing permission type (PR #11283 migration)', async () => {
2039+
// Simulates an existing deployment that has PROMPTS/AGENTS permissions from before PR #11283
2040+
// introduced SHARE/SHARE_PUBLIC. SHARED_GLOBAL existed previously; after the schema change
2041+
// the DB document has neither SHARE nor SHARE_PUBLIC set.
2042+
mockGetRoleByName.mockResolvedValue({
2043+
permissions: {
2044+
[PermissionTypes.AGENTS]: {
2045+
[Permissions.USE]: true,
2046+
[Permissions.CREATE]: true,
2047+
// SHARE and SHARE_PUBLIC intentionally absent — legacy pre-#11283 document
2048+
},
2049+
[PermissionTypes.PROMPTS]: {
2050+
[Permissions.USE]: true,
2051+
[Permissions.CREATE]: true,
2052+
// SHARE and SHARE_PUBLIC intentionally absent — legacy pre-#11283 document
2053+
},
2054+
[PermissionTypes.MCP_SERVERS]: {
2055+
[Permissions.USE]: true,
2056+
[Permissions.CREATE]: true,
2057+
[Permissions.SHARE]: false,
2058+
// SHARE_PUBLIC intentionally absent — added later
2059+
},
2060+
},
2061+
});
2062+
2063+
// Boolean configs — these should NOT override the backfilled values
2064+
const config = {
2065+
interface: {
2066+
agents: true,
2067+
prompts: true,
2068+
},
2069+
};
2070+
const configDefaults = {
2071+
interface: { agents: true, prompts: true },
2072+
} as TConfigDefaults;
2073+
const interfaceConfig = await loadDefaultInterface({ config, configDefaults });
2074+
const appConfig = { config, interfaceConfig } as unknown as AppConfig;
2075+
2076+
await updateInterfacePermissions({
2077+
appConfig,
2078+
getRoleByName: mockGetRoleByName,
2079+
updateAccessPermissions: mockUpdateAccessPermissions,
2080+
});
2081+
2082+
const userCall = mockUpdateAccessPermissions.mock.calls.find(
2083+
(call) => call[0] === SystemRoles.USER,
2084+
);
2085+
2086+
// AGENTS: SHARE and SHARE_PUBLIC should be backfilled with USER role defaults
2087+
expect(userCall[1][PermissionTypes.AGENTS]).toHaveProperty(Permissions.SHARE);
2088+
expect(userCall[1][PermissionTypes.AGENTS]).toHaveProperty(Permissions.SHARE_PUBLIC);
2089+
// USER role default for SHARE is false
2090+
expect(userCall[1][PermissionTypes.AGENTS][Permissions.SHARE]).toBe(false);
2091+
expect(userCall[1][PermissionTypes.AGENTS][Permissions.SHARE_PUBLIC]).toBe(false);
2092+
2093+
// PROMPTS: same backfill behaviour
2094+
expect(userCall[1][PermissionTypes.PROMPTS]).toHaveProperty(Permissions.SHARE);
2095+
expect(userCall[1][PermissionTypes.PROMPTS]).toHaveProperty(Permissions.SHARE_PUBLIC);
2096+
expect(userCall[1][PermissionTypes.PROMPTS][Permissions.SHARE]).toBe(false);
2097+
expect(userCall[1][PermissionTypes.PROMPTS][Permissions.SHARE_PUBLIC]).toBe(false);
2098+
2099+
// MCP_SERVERS: SHARE already exists, only SHARE_PUBLIC should be backfilled
2100+
expect(userCall[1][PermissionTypes.MCP_SERVERS]).toHaveProperty(Permissions.SHARE_PUBLIC);
2101+
expect(userCall[1][PermissionTypes.MCP_SERVERS]).not.toHaveProperty(Permissions.SHARE);
2102+
});
20082103
});

0 commit comments

Comments
 (0)