From 50a48efa43c3178a283b34848394254bac9ddf9a Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 18 Feb 2026 12:48:33 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AC=20fix:=20Backfill=20Missing=20SHAR?= =?UTF-8?q?E=20Permissions=20and=20Migrate=20Legacy=20SHARED=5FGLOBAL=20Fi?= =?UTF-8?q?elds=20(#11854)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- api/models/Role.js | 48 ++++++++ api/models/Role.spec.js | 106 ++++++++++++++++++ packages/api/src/app/permissions.spec.ts | 133 +++++++++++++++++++---- packages/api/src/app/permissions.ts | 102 +++++++++++++++++ 4 files changed, 370 insertions(+), 19 deletions(-) diff --git a/api/models/Role.js b/api/models/Role.js index 1766dc9b08..b7f806f3b6 100644 --- a/api/models/Role.js +++ b/api/models/Role.js @@ -114,6 +114,28 @@ async function updateAccessPermissions(roleName, permissionsUpdate, roleData) { } } + // Migrate legacy SHARED_GLOBAL → SHARE for PROMPTS and AGENTS. + // SHARED_GLOBAL was removed in favour of SHARE in PR #11283. If the DB still has + // SHARED_GLOBAL but not SHARE, inherit the value so sharing intent is preserved. + const legacySharedGlobalTypes = ['PROMPTS', 'AGENTS']; + for (const legacyPermType of legacySharedGlobalTypes) { + const existingTypePerms = currentPermissions[legacyPermType]; + if ( + existingTypePerms && + 'SHARED_GLOBAL' in existingTypePerms && + !('SHARE' in existingTypePerms) && + updates[legacyPermType] && + // Don't override an explicit SHARE value the caller already provided + !('SHARE' in updates[legacyPermType]) + ) { + const inheritedValue = existingTypePerms['SHARED_GLOBAL']; + updates[legacyPermType]['SHARE'] = inheritedValue; + logger.info( + `Migrating '${roleName}' role ${legacyPermType}.SHARED_GLOBAL=${inheritedValue} → SHARE`, + ); + } + } + for (const [permissionType, permissions] of Object.entries(updates)) { const currentTypePermissions = currentPermissions[permissionType] || {}; updatedPermissions[permissionType] = { ...currentTypePermissions }; @@ -129,6 +151,32 @@ async function updateAccessPermissions(roleName, permissionsUpdate, roleData) { } } + // Clean up orphaned SHARED_GLOBAL fields left in DB after the schema rename. + // Since we $set the full permissions object, deleting from updatedPermissions + // is sufficient to remove the field from MongoDB. + for (const legacyPermType of legacySharedGlobalTypes) { + const existingTypePerms = currentPermissions[legacyPermType]; + if (existingTypePerms && 'SHARED_GLOBAL' in existingTypePerms) { + if (!updates[legacyPermType]) { + // permType wasn't in the update payload so the migration block above didn't run. + // Create a writable copy and handle the SHARED_GLOBAL → SHARE inheritance here + // to avoid removing SHARED_GLOBAL without writing SHARE (data loss). + updatedPermissions[legacyPermType] = { ...existingTypePerms }; + if (!('SHARE' in existingTypePerms)) { + updatedPermissions[legacyPermType]['SHARE'] = existingTypePerms['SHARED_GLOBAL']; + logger.info( + `Migrating '${roleName}' role ${legacyPermType}.SHARED_GLOBAL=${existingTypePerms['SHARED_GLOBAL']} → SHARE`, + ); + } + } + delete updatedPermissions[legacyPermType]['SHARED_GLOBAL']; + hasChanges = true; + logger.info( + `Removed legacy SHARED_GLOBAL field from '${roleName}' role ${legacyPermType} permissions`, + ); + } + } + if (hasChanges) { const updateObj = { permissions: updatedPermissions }; diff --git a/api/models/Role.spec.js b/api/models/Role.spec.js index deac4e5c35..0ec2f831e2 100644 --- a/api/models/Role.spec.js +++ b/api/models/Role.spec.js @@ -233,6 +233,112 @@ describe('updateAccessPermissions', () => { expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO]).toEqual({ USE: true }); }); + it('should inherit SHARED_GLOBAL value into SHARE when SHARE is absent from both DB and update', async () => { + // Simulates the startup backfill path: caller sends SHARE_PUBLIC but not SHARE; + // migration should inherit SHARED_GLOBAL to preserve the deployment's sharing intent. + await Role.collection.insertOne({ + name: SystemRoles.USER, + permissions: { + [PermissionTypes.PROMPTS]: { USE: true, CREATE: true, SHARED_GLOBAL: true }, + [PermissionTypes.AGENTS]: { USE: true, CREATE: true, SHARED_GLOBAL: false }, + }, + }); + + await updateAccessPermissions(SystemRoles.USER, { + // No explicit SHARE — migration should inherit from SHARED_GLOBAL + [PermissionTypes.PROMPTS]: { SHARE_PUBLIC: false }, + [PermissionTypes.AGENTS]: { SHARE_PUBLIC: false }, + }); + + const updatedRole = await getRoleByName(SystemRoles.USER); + + // SHARED_GLOBAL=true → SHARE=true (inherited) + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true); + // SHARED_GLOBAL=false → SHARE=false (inherited) + expect(updatedRole.permissions[PermissionTypes.AGENTS].SHARE).toBe(false); + // SHARED_GLOBAL cleaned up + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined(); + expect(updatedRole.permissions[PermissionTypes.AGENTS].SHARED_GLOBAL).toBeUndefined(); + }); + + it('should respect explicit SHARE in update payload and not override it with SHARED_GLOBAL', async () => { + // Caller explicitly passes SHARE: false even though SHARED_GLOBAL=true in DB. + // The explicit intent must win; migration must not silently overwrite it. + await Role.collection.insertOne({ + name: SystemRoles.USER, + permissions: { + [PermissionTypes.PROMPTS]: { USE: true, SHARED_GLOBAL: true }, + }, + }); + + await updateAccessPermissions(SystemRoles.USER, { + [PermissionTypes.PROMPTS]: { SHARE: false }, // explicit false — should be preserved + }); + + const updatedRole = await getRoleByName(SystemRoles.USER); + + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(false); + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined(); + }); + + it('should migrate SHARED_GLOBAL to SHARE even when the permType is not in the update payload', async () => { + // Bug #2 regression: cleanup block removes SHARED_GLOBAL but migration block only + // runs when the permType is in the update payload. Without the fix, SHARE would be + // lost when any other permType (e.g. MULTI_CONVO) is the only thing being updated. + await Role.collection.insertOne({ + name: SystemRoles.USER, + permissions: { + [PermissionTypes.PROMPTS]: { + USE: true, + SHARED_GLOBAL: true, // legacy — NO SHARE present + }, + [PermissionTypes.MULTI_CONVO]: { USE: false }, + }, + }); + + // Only update MULTI_CONVO — PROMPTS is intentionally absent from the payload + await updateAccessPermissions(SystemRoles.USER, { + [PermissionTypes.MULTI_CONVO]: { USE: true }, + }); + + const updatedRole = await getRoleByName(SystemRoles.USER); + + // SHARE should have been inherited from SHARED_GLOBAL, not silently dropped + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true); + // SHARED_GLOBAL should be removed + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined(); + // Original USE should be untouched + expect(updatedRole.permissions[PermissionTypes.PROMPTS].USE).toBe(true); + // The actual update should have applied + expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO].USE).toBe(true); + }); + + it('should remove orphaned SHARED_GLOBAL when SHARE already exists and permType is not in update', async () => { + // Safe cleanup case: SHARE already set, SHARED_GLOBAL is just orphaned noise. + // SHARE must not be changed; SHARED_GLOBAL must be removed. + await Role.collection.insertOne({ + name: SystemRoles.USER, + permissions: { + [PermissionTypes.PROMPTS]: { + USE: true, + SHARE: true, // already migrated + SHARED_GLOBAL: true, // orphaned + }, + [PermissionTypes.MULTI_CONVO]: { USE: false }, + }, + }); + + await updateAccessPermissions(SystemRoles.USER, { + [PermissionTypes.MULTI_CONVO]: { USE: true }, + }); + + const updatedRole = await getRoleByName(SystemRoles.USER); + + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARED_GLOBAL).toBeUndefined(); + expect(updatedRole.permissions[PermissionTypes.PROMPTS].SHARE).toBe(true); + expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO].USE).toBe(true); + }); + it('should not update MULTI_CONVO permissions when no changes are needed', async () => { await new Role({ name: SystemRoles.USER, diff --git a/packages/api/src/app/permissions.spec.ts b/packages/api/src/app/permissions.spec.ts index 86d0c83095..41014dc200 100644 --- a/packages/api/src/app/permissions.spec.ts +++ b/packages/api/src/app/permissions.spec.ts @@ -776,11 +776,19 @@ describe('updateInterfacePermissions - permissions', () => { }); it('should only update permissions that do not exist when no config provided', async () => { - // Mock that some permissions already exist + // Mock that some permissions already exist (with SHARE/SHARE_PUBLIC as they would be post-#11283) mockGetRoleByName.mockResolvedValue({ permissions: { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, - [PermissionTypes.AGENTS]: { [Permissions.USE]: true }, + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: false, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, + [PermissionTypes.AGENTS]: { + [Permissions.USE]: true, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, }, }); @@ -892,30 +900,38 @@ describe('updateInterfacePermissions - permissions', () => { SystemRoles.USER, expectedPermissionsForUser, expect.objectContaining({ - permissions: { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, - [PermissionTypes.AGENTS]: { [Permissions.USE]: true }, - }, + permissions: expect.objectContaining({ + [PermissionTypes.PROMPTS]: expect.objectContaining({ [Permissions.USE]: false }), + [PermissionTypes.AGENTS]: expect.objectContaining({ [Permissions.USE]: true }), + }), }), ); expect(mockUpdateAccessPermissions).toHaveBeenCalledWith( SystemRoles.ADMIN, expectedPermissionsForAdmin, expect.objectContaining({ - permissions: { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, - [PermissionTypes.AGENTS]: { [Permissions.USE]: true }, - }, + permissions: expect.objectContaining({ + [PermissionTypes.PROMPTS]: expect.objectContaining({ [Permissions.USE]: false }), + [PermissionTypes.AGENTS]: expect.objectContaining({ [Permissions.USE]: true }), + }), }), ); }); it('should override existing permissions when explicitly configured', async () => { - // Mock that some permissions already exist + // Mock that some permissions already exist (with SHARE/SHARE_PUBLIC as they would be post-#11283) mockGetRoleByName.mockResolvedValue({ permissions: { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, - [PermissionTypes.AGENTS]: { [Permissions.USE]: false }, + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: false, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, + [PermissionTypes.AGENTS]: { + [Permissions.USE]: false, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, [PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false }, }, }); @@ -1340,8 +1356,13 @@ describe('updateInterfacePermissions - permissions', () => { it('should leave all existing permissions unchanged when nothing is configured', async () => { // Mock existing permissions with values that differ from defaults + // SHARE/SHARE_PUBLIC included as they would be in a post-#11283 DB document const existingUserPermissions = { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: false, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, [PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false }, [PermissionTypes.MEMORIES]: { [Permissions.USE]: false }, [PermissionTypes.PEOPLE_PICKER]: { @@ -1400,10 +1421,14 @@ describe('updateInterfacePermissions - permissions', () => { }); it('should only update explicitly configured permissions and leave others unchanged', async () => { - // Mock existing permissions + // Mock existing permissions (with SHARE/SHARE_PUBLIC as they would be post-#11283) mockGetRoleByName.mockResolvedValue({ permissions: { - [PermissionTypes.PROMPTS]: { [Permissions.USE]: false }, + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: false, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, [PermissionTypes.BOOKMARKS]: { [Permissions.USE]: false }, [PermissionTypes.MEMORIES]: { [Permissions.USE]: false }, [PermissionTypes.PEOPLE_PICKER]: { @@ -1729,8 +1754,12 @@ describe('updateInterfacePermissions - permissions', () => { [Permissions.UPDATE]: true, [Permissions.OPT_OUT]: true, }, - // Other existing permissions - [PermissionTypes.PROMPTS]: { [Permissions.USE]: true }, + // Other existing permissions (with SHARE/SHARE_PUBLIC as they would be post-#11283) + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: true, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, [PermissionTypes.BOOKMARKS]: { [Permissions.USE]: true }, }, }); @@ -2005,4 +2034,70 @@ describe('updateInterfacePermissions - permissions', () => { expect(userCall[1][PermissionTypes.PROMPTS]).not.toHaveProperty(Permissions.SHARE); expect(userCall[1][PermissionTypes.PROMPTS]).not.toHaveProperty(Permissions.SHARE_PUBLIC); }); + + it('should backfill SHARE/SHARE_PUBLIC when missing from an existing permission type (PR #11283 migration)', async () => { + // Simulates an existing deployment that has PROMPTS/AGENTS permissions from before PR #11283 + // introduced SHARE/SHARE_PUBLIC. SHARED_GLOBAL existed previously; after the schema change + // the DB document has neither SHARE nor SHARE_PUBLIC set. + mockGetRoleByName.mockResolvedValue({ + permissions: { + [PermissionTypes.AGENTS]: { + [Permissions.USE]: true, + [Permissions.CREATE]: true, + // SHARE and SHARE_PUBLIC intentionally absent — legacy pre-#11283 document + }, + [PermissionTypes.PROMPTS]: { + [Permissions.USE]: true, + [Permissions.CREATE]: true, + // SHARE and SHARE_PUBLIC intentionally absent — legacy pre-#11283 document + }, + [PermissionTypes.MCP_SERVERS]: { + [Permissions.USE]: true, + [Permissions.CREATE]: true, + [Permissions.SHARE]: false, + // SHARE_PUBLIC intentionally absent — added later + }, + }, + }); + + // Boolean configs — these should NOT override the backfilled values + const config = { + interface: { + agents: true, + prompts: true, + }, + }; + const configDefaults = { + interface: { agents: true, prompts: true }, + } as TConfigDefaults; + const interfaceConfig = await loadDefaultInterface({ config, configDefaults }); + const appConfig = { config, interfaceConfig } as unknown as AppConfig; + + await updateInterfacePermissions({ + appConfig, + getRoleByName: mockGetRoleByName, + updateAccessPermissions: mockUpdateAccessPermissions, + }); + + const userCall = mockUpdateAccessPermissions.mock.calls.find( + (call) => call[0] === SystemRoles.USER, + ); + + // AGENTS: SHARE and SHARE_PUBLIC should be backfilled with USER role defaults + expect(userCall[1][PermissionTypes.AGENTS]).toHaveProperty(Permissions.SHARE); + expect(userCall[1][PermissionTypes.AGENTS]).toHaveProperty(Permissions.SHARE_PUBLIC); + // USER role default for SHARE is false + expect(userCall[1][PermissionTypes.AGENTS][Permissions.SHARE]).toBe(false); + expect(userCall[1][PermissionTypes.AGENTS][Permissions.SHARE_PUBLIC]).toBe(false); + + // PROMPTS: same backfill behaviour + expect(userCall[1][PermissionTypes.PROMPTS]).toHaveProperty(Permissions.SHARE); + expect(userCall[1][PermissionTypes.PROMPTS]).toHaveProperty(Permissions.SHARE_PUBLIC); + expect(userCall[1][PermissionTypes.PROMPTS][Permissions.SHARE]).toBe(false); + expect(userCall[1][PermissionTypes.PROMPTS][Permissions.SHARE_PUBLIC]).toBe(false); + + // MCP_SERVERS: SHARE already exists, only SHARE_PUBLIC should be backfilled + expect(userCall[1][PermissionTypes.MCP_SERVERS]).toHaveProperty(Permissions.SHARE_PUBLIC); + expect(userCall[1][PermissionTypes.MCP_SERVERS]).not.toHaveProperty(Permissions.SHARE); + }); }); diff --git a/packages/api/src/app/permissions.ts b/packages/api/src/app/permissions.ts index 28d475e14e..3638bdc0bb 100644 --- a/packages/api/src/app/permissions.ts +++ b/packages/api/src/app/permissions.ts @@ -409,6 +409,108 @@ export async function updateInterfacePermissions({ addPermissionIfNeeded(permType as PermissionTypes, permissions); } + /** + * Backfill SHARE / SHARE_PUBLIC for permission types that already exist in the DB but are + * missing these fields — caused by the PR #11283 schema change that added SHARE/SHARE_PUBLIC + * to PROMPTS and AGENTS (replacing the removed SHARED_GLOBAL field) without a DB migration. + * + * This is intentionally kept separate from `addPermissionIfNeeded` to avoid overwriting + * user-customised share settings when the config uses a boolean (e.g. `agents: true`). + * Only fields that are literally absent from the existing DB document are backfilled here; + * any field that is already set keeps its current value. + */ + type ShareBackfillEntry = [PermissionTypes, Record]; + const shareBackfill: ShareBackfillEntry[] = [ + [ + PermissionTypes.PROMPTS, + { + [Permissions.SHARE]: getPermissionValue( + getConfigShare(loadedInterface.prompts), + defaultPerms[PermissionTypes.PROMPTS]?.[Permissions.SHARE], + promptsDefaultShare, + ), + [Permissions.SHARE_PUBLIC]: getPermissionValue( + getConfigPublic(loadedInterface.prompts), + defaultPerms[PermissionTypes.PROMPTS]?.[Permissions.SHARE_PUBLIC], + promptsDefaultPublic, + ), + }, + ], + [ + PermissionTypes.AGENTS, + { + [Permissions.SHARE]: getPermissionValue( + getConfigShare(loadedInterface.agents), + defaultPerms[PermissionTypes.AGENTS]?.[Permissions.SHARE], + agentsDefaultShare, + ), + [Permissions.SHARE_PUBLIC]: getPermissionValue( + getConfigPublic(loadedInterface.agents), + defaultPerms[PermissionTypes.AGENTS]?.[Permissions.SHARE_PUBLIC], + agentsDefaultPublic, + ), + }, + ], + [ + PermissionTypes.MCP_SERVERS, + { + [Permissions.SHARE]: getPermissionValue( + loadedInterface.mcpServers?.share, + defaultPerms[PermissionTypes.MCP_SERVERS]?.[Permissions.SHARE], + defaults.mcpServers?.share, + ), + [Permissions.SHARE_PUBLIC]: getPermissionValue( + loadedInterface.mcpServers?.public, + defaultPerms[PermissionTypes.MCP_SERVERS]?.[Permissions.SHARE_PUBLIC], + defaults.mcpServers?.public, + ), + }, + ], + [ + PermissionTypes.REMOTE_AGENTS, + { + [Permissions.SHARE]: getPermissionValue( + loadedInterface.remoteAgents?.share, + defaultPerms[PermissionTypes.REMOTE_AGENTS]?.[Permissions.SHARE], + defaults.remoteAgents?.share, + ), + [Permissions.SHARE_PUBLIC]: getPermissionValue( + loadedInterface.remoteAgents?.public, + defaultPerms[PermissionTypes.REMOTE_AGENTS]?.[Permissions.SHARE_PUBLIC], + defaults.remoteAgents?.public, + ), + }, + ], + ]; + + for (const [permType, shareDefaults] of shareBackfill) { + const existingPerms = existingPermissions?.[permType]; + // Skip permission types that don't exist yet — addPermissionIfNeeded already handles those + if (!existingPerms) { + continue; + } + + const missingFields: Record = {}; + for (const [field, value] of Object.entries(shareDefaults)) { + if ( + value !== undefined && + existingPerms[field] === undefined && + // Don't clobber a value already queued by addPermissionIfNeeded (e.g. explicit config) + permissionsToUpdate[permType as PermissionTypes]?.[field] === undefined + ) { + missingFields[field] = value; + } + } + + if (Object.keys(missingFields).length > 0) { + logger.debug( + `Role '${roleName}': Backfilling missing share fields for '${permType}': ${Object.keys(missingFields).join(', ')}`, + ); + // Merge into any update already queued by addPermissionIfNeeded, or create a new entry + permissionsToUpdate[permType] = { ...permissionsToUpdate[permType], ...missingFields }; + } + } + // Update permissions if any need updating if (Object.keys(permissionsToUpdate).length > 0) { await updateAccessPermissions(roleName, permissionsToUpdate, existingRole);