🔗 fix: Prevent duplicate share rows from clobbering pending role edits (dedupe adds by stable id) (#14655)

Follow-up to #14316 / #14317. The share dialog deduped picker adds by raw
idOnTheSource, but loaded ACL rows carry the external oid for OpenID/Entra
users while search results carry the local _id (searchPrincipals dropped the
selected idOnTheSource before transforming). Re-adding an already-listed user
therefore appended a duplicate row, and the stable-id de-dup in
computeShareChanges let the appended default-role row silently shadow a
pending role edit (no-op PUT with a success toast).

- Dedupe adds via a new dedupeNewShares helper keyed by principalKey, and
  skip marking the dialog dirty when nothing was added
- Propagate idOnTheSource through the user-search transform so picker
  results match ACL rows (also fixes excludeIds filtering)
- Add regression tests

Closes #14654

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Danny Avila <danny@librechat.ai>

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,

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>): 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]);
+  });
+});

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.

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<IUser[]>().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 {