diff --git a/client/src/components/Sharing/GenericGrantAccessDialog.tsx b/client/src/components/Sharing/GenericGrantAccessDialog.tsx index ecf246491c..81806ed6e9 100644 --- a/client/src/components/Sharing/GenericGrantAccessDialog.tsx +++ b/client/src/components/Sharing/GenericGrantAccessDialog.tsx @@ -22,11 +22,11 @@ import { useCanSharePublic, useLocalize, } from '~/hooks'; +import { computeShareChanges, dedupeNewShares } from './shareChanges'; import UnifiedPeopleSearch from './PeoplePicker/UnifiedPeopleSearch'; import PeoplePickerAdminSettings from './PeoplePickerAdminSettings'; import PublicSharingToggle from './PublicSharingToggle'; import { SelectedPrincipalsList } from './PeoplePicker'; -import { computeShareChanges } from './shareChanges'; import { cn } from '~/utils'; export default function GenericGrantAccessDialog({ @@ -110,10 +110,10 @@ export default function GenericGrantAccessDialog({ // Handler for adding users from search (immediate add to unified list) const handleAddFromSearch = (newShares: TPrincipal[]) => { - const sharesToAdd = newShares.filter( - (newShare) => - !allShares.some((existing) => existing.idOnTheSource === newShare.idOnTheSource), - ); + const sharesToAdd = dedupeNewShares(allShares, newShares); + if (!sharesToAdd.length) { + return; + } const sharesWithDefaults = sharesToAdd.map((share) => ({ ...share, diff --git a/client/src/components/Sharing/__tests__/shareChanges.spec.ts b/client/src/components/Sharing/__tests__/shareChanges.spec.ts index c287688d9e..ae0f8d1b4c 100644 --- a/client/src/components/Sharing/__tests__/shareChanges.spec.ts +++ b/client/src/components/Sharing/__tests__/shareChanges.spec.ts @@ -1,6 +1,6 @@ import { AccessRoleIds, PrincipalType } from 'librechat-data-provider'; import type { TPrincipal } from 'librechat-data-provider'; -import { computeShareChanges, principalKey } from '../shareChanges'; +import { computeShareChanges, dedupeNewShares, principalKey } from '../shareChanges'; const principal = (over: Partial): TPrincipal => ({ type: PrincipalType.USER, @@ -65,3 +65,50 @@ describe('computeShareChanges', () => { expect(principalKey(currentShares[0])).toBe(`${PrincipalType.GROUP}-group-oid`); }); }); + +describe('dedupeNewShares', () => { + it('filters out an already-listed principal even when idOnTheSource differs (oid vs local id)', () => { + // Loaded ACL row carries the external oid; the picker returns the local `_id`. + const existing = [principal({ id: 'u1', idOnTheSource: 'entra-oid-abc' })]; + const incoming = [principal({ id: 'u1', idOnTheSource: 'u1' })]; + + expect(dedupeNewShares(existing, incoming)).toEqual([]); + }); + + it('keeps genuinely new principals', () => { + const existing = [principal({ id: 'u1', idOnTheSource: 'entra-oid-abc' })]; + const incoming = [principal({ id: 'u2', idOnTheSource: 'u2' })]; + + expect(dedupeNewShares(existing, incoming)).toHaveLength(1); + expect(dedupeNewShares(existing, incoming)[0].id).toBe('u2'); + }); + + it('prevents a re-added picker row from clobbering a pending role edit', () => { + // Persisted: Entra-linked user shared as viewer, ACL row keyed by external oid. + const currentShares = [ + principal({ + id: 'u1', + idOnTheSource: 'entra-oid-abc', + accessRoleId: AccessRoleIds.AGENT_VIEWER, + }), + ]; + // Working list: the admin promoted the row to editor... + const workingRow = principal({ + id: 'u1', + idOnTheSource: 'entra-oid-abc', + accessRoleId: AccessRoleIds.AGENT_EDITOR, + }); + // ...then re-added the same user from the picker (local `_id`, default role). + const pickerResult = principal({ + id: 'u1', + idOnTheSource: 'u1', + accessRoleId: AccessRoleIds.AGENT_VIEWER, + }); + + const allShares = [workingRow, ...dedupeNewShares([workingRow], [pickerResult])]; + const { updated, removed } = computeShareChanges(currentShares, allShares); + + expect(removed).toHaveLength(0); + expect(updated.map((p) => p.accessRoleId)).toEqual([AccessRoleIds.AGENT_EDITOR]); + }); +}); diff --git a/client/src/components/Sharing/shareChanges.ts b/client/src/components/Sharing/shareChanges.ts index 3b29beb8fc..0c57dbc16a 100644 --- a/client/src/components/Sharing/shareChanges.ts +++ b/client/src/components/Sharing/shareChanges.ts @@ -15,6 +15,23 @@ import type { TPrincipal } from 'librechat-data-provider'; export const principalKey = (share: TPrincipal): string => `${share.type}-${share.id ?? share.idOnTheSource}`; +/** + * Filter people-picker results down to principals not already present in the working + * list, keyed by the same stable identity `computeShareChanges` diffs on. + * + * Comparing raw `idOnTheSource` here is not enough: loaded ACL rows carry the external + * oid for OpenID/Entra-linked users while search results carry the local `_id`, so the + * same principal would be appended twice — and the duplicate (collapsing to the same + * key in the diff) would silently shadow a pending role edit. + */ +export function dedupeNewShares( + existingShares: TPrincipal[], + newShares: TPrincipal[], +): TPrincipal[] { + const existingKeys = new Set(existingShares.map(principalKey)); + return newShares.filter((share) => !existingKeys.has(principalKey(share))); +} + /** * Diff the currently-persisted shares (`currentShares`) against the working list * (`allShares`) to derive which principals to grant/update and which to revoke. diff --git a/packages/data-schemas/src/methods/userGroup.ts b/packages/data-schemas/src/methods/userGroup.ts index cf31eb7298..11ab0cd0f9 100644 --- a/packages/data-schemas/src/methods/userGroup.ts +++ b/packages/data-schemas/src/methods/userGroup.ts @@ -1,7 +1,7 @@ import { Types } from 'mongoose'; import { AsyncLocalStorage } from 'async_hooks'; import { CacheKeys, PrincipalType } from 'librechat-data-provider'; -import type { TUser, TPrincipalSearchResult } from 'librechat-data-provider'; +import type { TPrincipalSearchResult } from 'librechat-data-provider'; import type { Model, ClientSession, FilterQuery } from 'mongoose'; import type { CacheStore, IGroup, IRole, IUser } from '~/types'; import { isValidObjectIdString } from '~/utils/objectId'; @@ -1013,7 +1013,12 @@ export function createUserGroupMethods( * @param user - User object from database * @returns Transformed user result */ - function transformUserToTPrincipalSearchResult(user: TUser): TPrincipalSearchResult { + function transformUserToTPrincipalSearchResult( + user: Pick< + IUser, + 'id' | 'name' | 'email' | 'username' | 'avatar' | 'provider' | 'idOnTheSource' + >, + ): TPrincipalSearchResult { return { id: user.id, type: PrincipalType.USER, @@ -1023,7 +1028,7 @@ export function createUserGroupMethods( avatar: user.avatar, provider: user.provider, source: 'local', - idOnTheSource: (user as TUser & { idOnTheSource?: string }).idOnTheSource || user.id, + idOnTheSource: user.idOnTheSource || user.id, }; } @@ -1087,17 +1092,17 @@ export function createUserGroupMethods( promises.push( userQuery.lean().then((users) => - users.map((user) => { - const userWithId = user as IUser & { idOnTheSource?: string }; - return transformUserToTPrincipalSearchResult({ - id: userWithId._id?.toString() || '', - name: userWithId.name, - email: userWithId.email, - username: userWithId.username, - avatar: userWithId.avatar, - provider: userWithId.provider, - } as TUser); - }), + users.map((user) => + transformUserToTPrincipalSearchResult({ + id: user._id?.toString() || '', + name: user.name, + email: user.email, + username: user.username, + avatar: user.avatar, + provider: user.provider, + idOnTheSource: user.idOnTheSource, + }), + ), ), ); } else {