mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-02-20 17:34:10 +01:00
🧬 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.
This commit is contained in:
Danny Avila
GitHub
parent
42718faad2
commit
50a48efa43
4 changed files with 370 additions and 19 deletions
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, boolean | undefined>];
|
||||
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<string, boolean | undefined> = {};
|
||||
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);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue