🔗 fix: Preserve resource owner access when sharing (key share diff by stable id) (#14317)

* 🔗 fix: Preserve resource owner access when sharing (key share diff by stable id)

The share dialog diff (GenericGrantAccessDialog.handleSave) keyed added/removed
principals by `idOnTheSource`, which is inconsistent for the same user across
sources: getResourcePermissions returns `userInfo.idOnTheSource || _id` (the
external oid for OpenID/Entra users) while the people-picker returns the local
`_id`. The resource owner then appears in both `updated` and `removed`, and — since
updateResourcePermissions applies grants (upsert) before revocations (delete) — the
owner's own ACL entry is deleted when they add anyone to the share list. They then
get 403 on GET/edit/re-sharing their own resource.

Extract the diff into a pure computeShareChanges() helper keyed by
`id ?? idOnTheSource` (stable local id when present, external oid fallback for
principals not yet synced locally, e.g. unsynced Entra groups/users). Add unit tests.

Not reproducible with local-only users, where idOnTheSource falls back to _id and
both sources agree.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* 🔧 fix: address review — dedupe share diff by principalKey; drop test assertion

- computeShareChanges now diffs over the de-duplicated map values, so a principal
  that appears more than once in the input (possible while the add/dedupe path still
  keys on idOnTheSource) is never emitted multiple times in updated/removed.
- Drop the `as TPrincipal` assertion in the test helper — the literal is structurally
  compatible with TPrincipal, so TypeScript validates the shape directly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

client/src/components/Sharing/GenericGrantAccessDialog.tsx

@@ -26,6 +26,7 @@ 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({
@@ -162,26 +163,10 @@ export default function GenericGrantAccessDialog({
     }
 
     try {
-      // Calculate changes for unified list
-      const originalSharesMap = new Map(
-        currentShares.map((share) => [`${share.type}-${share.idOnTheSource}`, share]),
-      );
-      const allSharesMap = new Map(
-        allShares.map((share) => [`${share.type}-${share.idOnTheSource}`, share]),
-      );
-
-      // Find newly added and updated shares
-      const updated = allShares.filter((share) => {
-        const key = `${share.type}-${share.idOnTheSource}`;
-        const original = originalSharesMap.get(key);
-        return !original || original.accessRoleId !== share.accessRoleId;
-      });
-
-      // Find removed shares
-      const removed = currentShares.filter((share) => {
-        const key = `${share.type}-${share.idOnTheSource}`;
-        return !allSharesMap.has(key);
-      });
+      // Diff persisted shares against the working list. Keyed by stable `id`
+      // (falling back to idOnTheSource) so the same principal is never simultaneously
+      // granted and revoked — see computeShareChanges.
+      const { updated, removed } = computeShareChanges(currentShares, allShares);
 
       const publicChanged = isPublic !== currentIsPublic;
       const publicRoleChanged = isPublic && publicRole !== currentPublicRole;

client/src/components/Sharing/__tests__/shareChanges.spec.ts

@@ -0,0 +1,67 @@
+import { AccessRoleIds, PrincipalType } from 'librechat-data-provider';
+import type { TPrincipal } from 'librechat-data-provider';
+import { computeShareChanges, principalKey } from '../shareChanges';
+
+const principal = (over: Partial<TPrincipal>): TPrincipal => ({
+  type: PrincipalType.USER,
+  id: 'id',
+  name: 'name',
+  accessRoleId: AccessRoleIds.AGENT_VIEWER,
+  ...over,
+});
+
+describe('computeShareChanges', () => {
+  it('does not revoke a principal when the same id appears with a different idOnTheSource', () => {
+    // Loaded ACL returns the owner keyed by the external oid...
+    const currentShares = [
+      principal({
+        id: 'owner',
+        accessRoleId: AccessRoleIds.AGENT_OWNER,
+        idOnTheSource: 'entra-oid-abc',
+      }),
+    ];
+    // ...while the working list (people-picker) carries the local `_id`.
+    const allShares = [
+      principal({ id: 'owner', accessRoleId: AccessRoleIds.AGENT_OWNER, idOnTheSource: 'owner' }),
+      principal({
+        id: 'viewer',
+        accessRoleId: AccessRoleIds.AGENT_VIEWER,
+        idOnTheSource: 'viewer',
+      }),
+    ];
+
+    const { updated, removed } = computeShareChanges(currentShares, allShares);
+
+    expect(removed.some((p) => p.id === 'owner')).toBe(false);
+    expect(updated.some((p) => p.id === 'viewer')).toBe(true);
+  });
+
+  it('still revokes a principal that is genuinely gone from the working list', () => {
+    const currentShares = [
+      principal({ id: 'owner', accessRoleId: AccessRoleIds.AGENT_OWNER, idOnTheSource: 'owner' }),
+      principal({ id: 'gone', accessRoleId: AccessRoleIds.AGENT_VIEWER, idOnTheSource: 'gone' }),
+    ];
+    const allShares = [
+      principal({ id: 'owner', accessRoleId: AccessRoleIds.AGENT_OWNER, idOnTheSource: 'owner' }),
+    ];
+
+    const { removed } = computeShareChanges(currentShares, allShares);
+    expect(removed.map((p) => p.id)).toEqual(['gone']);
+  });
+
+  it('falls back to idOnTheSource for principals without a local id (unsynced Entra)', () => {
+    const currentShares = [
+      principal({
+        type: PrincipalType.GROUP,
+        id: undefined,
+        idOnTheSource: 'group-oid',
+        accessRoleId: AccessRoleIds.AGENT_VIEWER,
+      }),
+    ];
+    const allShares: TPrincipal[] = [];
+
+    const { removed } = computeShareChanges(currentShares, allShares);
+    expect(removed.some((p) => p.idOnTheSource === 'group-oid')).toBe(true);
+    expect(principalKey(currentShares[0])).toBe(`${PrincipalType.GROUP}-group-oid`);
+  });
+});

client/src/components/Sharing/shareChanges.ts

@@ -0,0 +1,42 @@
+import type { TPrincipal } from 'librechat-data-provider';
+
+/**
+ * Key a principal by its stable local `id` when present, falling back to
+ * `idOnTheSource` for principals not yet synced locally (e.g. Entra groups/users
+ * that only carry an external oid).
+ *
+ * Keying by `idOnTheSource` alone is unsafe: the same user can be represented with a
+ * different `idOnTheSource` across sources. `getResourcePermissions` returns
+ * `userInfo.idOnTheSource || _id` (the external oid for OpenID/Entra users), while the
+ * people-picker returns the local `_id`. That mismatch places the same principal in
+ * both the `updated` and `removed` sets — and because the server applies grants before
+ * revocations, the resource owner's own grant is silently revoked when they share.
+ */
+export const principalKey = (share: TPrincipal): string =>
+  `${share.type}-${share.id ?? share.idOnTheSource}`;
+
+/**
+ * Diff the currently-persisted shares (`currentShares`) against the working list
+ * (`allShares`) to derive which principals to grant/update and which to revoke.
+ */
+export function computeShareChanges(
+  currentShares: TPrincipal[],
+  allShares: TPrincipal[],
+): { updated: TPrincipal[]; removed: TPrincipal[] } {
+  const originalSharesMap = new Map(currentShares.map((share) => [principalKey(share), share]));
+  const allSharesMap = new Map(allShares.map((share) => [principalKey(share), share]));
+
+  // Diff over the de-duplicated map values so a principal that appears more than once
+  // in the input (possible while the add/dedupe path still keys on idOnTheSource) is
+  // never emitted multiple times.
+  const updated = [...allSharesMap.values()].filter((share) => {
+    const original = originalSharesMap.get(principalKey(share));
+    return !original || original.accessRoleId !== share.accessRoleId;
+  });
+
+  const removed = [...originalSharesMap.values()].filter(
+    (share) => !allSharesMap.has(principalKey(share)),
+  );
+
+  return { updated, removed };
+}