🚪 fix: Keep Owners From Being Locked Out of Their Own Resource When Sharing (#14347)

* 🔒 fix: Skip revoke for principals also being granted (owner-lockout guard)

bulkUpdateResourcePermissions flushes grants (upserts) before revokes (deletes).
If a principal appears in both updatedPrincipals and revokedPrincipals, the ACL
entry is granted and then immediately deleted, stripping access the caller just
set. This can strip a resource owner's own grant when the share dialog places
the owner in both lists from a client id/idOnTheSource mismatch (OpenID/Entra).

Add a server-side guard: track principals granted in the same request and skip
any revoke for the same principal, so granting wins and owner lockout is
impossible regardless of how the client computes the share diff. Complements the
client-side keying fix in #14317.

Refs #14316

* 🔒 fix: Exclude PUBLIC from grant-wins guard so public-disable is honored

The grant-wins guard must not apply to PrincipalType.PUBLIC. An explicit
public: false disable adds the public principal to the revoke list; a
contradictory payload that also grants public (public in the updated list) would
otherwise skip the revoke and leave the resource public. Disabling public access
must always win. User/group owner-lockout protection is unchanged.

Addresses Codex P2 on #14347.

* 🔒 fix: Move revoke guard inside per-principal try (tolerate malformed entries)

The grant-wins guard read principal.type before the per-principal try/catch, so
a malformed revoke entry (e.g. removed: [null]) would throw out of
bulkUpdateResourcePermissions after grants were already flushed on
non-transactional MongoDB, leaving partial permission changes. Move the guard
inside the try so a malformed entry is recorded in results.errors and skipped,
matching prior behavior.

Addresses Codex P2 on #14347.

api/server/services/PermissionService.js

@@ -758,6 +758,20 @@ const bulkUpdateResourcePermissions = async ({
 
     const bulkWrites = [];
 
+    /**
+     * Tracks non-public principals granted in this same request so their revoke is skipped below.
+     * Grants are flushed before deletes, so a principal present in both `updatedPrincipals` and
+     * `revokedPrincipals` would be upserted and then deleted, stripping access the caller just set
+     * (e.g. a resource owner landing in both lists from a client `id`/`idOnTheSource` mismatch).
+     * Granting wins to make owner lockout impossible regardless of the client-side diff (#14316).
+     *
+     * PUBLIC is deliberately excluded: an explicit `public: false` disable adds the public principal
+     * to the revoke list, and disabling public access must always win over a stale/contradictory
+     * grant so a resource is never left public when the caller asked to make it private.
+     */
+    const grantedPrincipalKeys = new Set();
+    const principalKey = (principal) => `${principal.type}:${principal.id}`;
+
     for (const principal of updatedPrincipals) {
       try {
         if (!principal.accessRoleId) {
@@ -838,6 +852,9 @@ const bulkUpdateResourcePermissions = async ({
           memberCount: principal.memberCount,
           memberIds: principal.memberIds,
         });
+        if (principal.type !== PrincipalType.PUBLIC) {
+          grantedPrincipalKeys.add(principalKey(principal));
+        }
       } catch (error) {
         results.errors.push({
           principal,
@@ -853,6 +870,14 @@ const bulkUpdateResourcePermissions = async ({
     const deleteQueries = [];
     for (const principal of revokedPrincipals) {
       try {
+        // Inside the try so a malformed revoke entry (e.g. a nullish principal) is recorded in
+        // results.errors and skipped, rather than throwing out after grants were already flushed.
+        if (
+          principal.type !== PrincipalType.PUBLIC &&
+          grantedPrincipalKeys.has(principalKey(principal))
+        ) {
+          continue;
+        }
         const query = {
           principalType: principal.type,
           resourceType,

api/server/services/PermissionService.spec.js

@@ -894,6 +894,103 @@ describe('PermissionService', () => {
       expect(remainingEntries[0].principalId.toString()).toBe(userId.toString());
     });
 
+    test('grant wins over revoke when a principal is in both lists (prevents owner lockout, #14316)', async () => {
+      // Simulates the share dialog sending the owner in both updatedPrincipals (grant OWNER) and
+      // revokedPrincipals (e.g. from a client id/idOnTheSource mismatch). Grants flush before
+      // deletes, so without the guard the owner would be upserted and then deleted. The owner
+      // must keep access.
+      const results = await bulkUpdateResourcePermissions({
+        resourceType: ResourceType.AGENT,
+        resourceId,
+        updatedPrincipals: [
+          {
+            type: PrincipalType.USER,
+            id: userId,
+            accessRoleId: AccessRoleIds.AGENT_OWNER,
+          },
+        ],
+        revokedPrincipals: [
+          {
+            type: PrincipalType.USER,
+            id: userId,
+          },
+        ],
+        grantedBy: grantedById,
+      });
+
+      expect(results.granted).toHaveLength(1);
+      // The revoke for the same principal is skipped, not applied, so it is absent from results.
+      expect(results.revoked).toHaveLength(0);
+      expect(results.errors).toHaveLength(0);
+
+      const userEntry = await AclEntry.findOne({
+        principalType: PrincipalType.USER,
+        principalId: userId,
+        resourceType: ResourceType.AGENT,
+        resourceId,
+      }).populate('roleId', 'accessRoleId');
+      expect(userEntry).not.toBeNull();
+      expect(userEntry.roleId.accessRoleId).toBe(AccessRoleIds.AGENT_OWNER);
+    });
+
+    test('revoke wins for PUBLIC so an explicit public disable is honored (#14316)', async () => {
+      // A contradictory payload that both grants public and disables it puts the public principal
+      // in both lists. Unlike user/group principals, disabling public access must win so the
+      // resource is never left public when the caller asked to make it private.
+      const results = await bulkUpdateResourcePermissions({
+        resourceType: ResourceType.AGENT,
+        resourceId,
+        updatedPrincipals: [
+          {
+            type: PrincipalType.PUBLIC,
+            accessRoleId: AccessRoleIds.AGENT_VIEWER,
+          },
+        ],
+        revokedPrincipals: [
+          {
+            type: PrincipalType.PUBLIC,
+          },
+        ],
+        grantedBy: grantedById,
+      });
+
+      expect(results.revoked).toHaveLength(1);
+      expect(results.errors).toHaveLength(0);
+
+      const publicEntry = await AclEntry.findOne({
+        principalType: PrincipalType.PUBLIC,
+        resourceType: ResourceType.AGENT,
+        resourceId,
+      });
+      expect(publicEntry).toBeNull();
+    });
+
+    test('records a malformed revoke entry in errors instead of throwing after grants flush (#14316)', async () => {
+      // A nullish/malformed entry in revokedPrincipals must not throw out of the function after
+      // grants have already been flushed; it is captured in results.errors and processing continues.
+      const results = await bulkUpdateResourcePermissions({
+        resourceType: ResourceType.AGENT,
+        resourceId,
+        updatedPrincipals: [
+          { type: PrincipalType.USER, id: otherUserId, accessRoleId: AccessRoleIds.AGENT_VIEWER },
+        ],
+        revokedPrincipals: [null],
+        grantedBy: grantedById,
+      });
+
+      expect(results.errors).toHaveLength(1);
+      expect(results.granted).toHaveLength(1);
+
+      // The valid grant still landed despite the malformed revoke entry.
+      const grantedEntry = await AclEntry.findOne({
+        principalType: PrincipalType.USER,
+        principalId: otherUserId,
+        resourceType: ResourceType.AGENT,
+        resourceId,
+      });
+      expect(grantedEntry).not.toBeNull();
+    });
+
     test('should handle mixed operations (grant, update, revoke)', async () => {
       const updatedPrincipals = [
         {