From 277fdd2b43197f00ff44c8b63f60e8577e01a008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Airam=20Hern=C3=A1ndez=20Hern=C3=A1ndez?= <100208966+Airamhh@users.noreply.github.com> Date: Mon, 13 Apr 2026 13:50:52 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20feat:=20Optimized=20Entra=20ID?= =?UTF-8?q?=20Group=20Sync=20with=20Auto-Creation=20(#12606)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: implement optimized Entra group sync with auto-creation ## Changes ### MUST FIX (Critical Issues) - RESOLVED 1. **BUG FIX: Prevent unintended user removal from existing groups** - ISSUE: db.syncUserEntraGroups() was called with only missing groups, causing removal from all existing Entra groups (full bidirectional sync behavior) - SOLUTION: Replaced with db.upsertGroupByExternalId() for each missing group followed by single bulkUpdateGroups() to add memberships (race-safe, idempotent) - BENEFIT: User memberships correctly maintained for mix of existing + new groups 2. **JSDoc @throws contradiction** - ISSUE: JSDoc declared function throws, but implementation catches all errors - SOLUTION: Removed @throws from JSDoc - function is best-effort - BENEFIT: Prevents unnecessary try/catch in caller code 3. **Missing test for group creation flow** - ISSUE: Auto-creating missing Entra groups had no test coverage - SOLUTION: Added regression test for mix of existing + new groups scenario - BENEFIT: Prevents future regressions on critical path ### SHOULD FIX (Important Improvements) - RESOLVED 4. **E11000 race condition handling** - SOLUTION: Upserts are idempotent and race-safe by design - BENEFIT: Concurrent logins no longer race each other 5. **Direct Mongoose access instead of db layer** - SOLUTION: Added findGroupsByExternalIds() helper to userGroup.ts - BENEFIT: Centralized data access, easier to add tenant scoping 6. **Serial DB round-trips on login path** - ISSUE: 40+ queries for user with 20 new groups - SOLUTION: Promise.all() for parallel upserts + single bulkUpdate - BENEFIT: ~10x performance improvement 7. **Graph API 429/503 throttling unhandled** - SOLUTION: Retry logic with exponential backoff (1s, 2s delays) - BENEFIT: Temporary API issues no longer cause permanent membership loss 8. **Sequential batch requests slow** - ISSUE: 200 groups = 10 batches × 200ms = ~2s sequential - SOLUTION: Promise.all() with concurrency limit (5 parallel batches) - BENEFIT: ~400ms total time ## Minor Fixes - Removed dead code check - PII removal: user._id instead of user.email in logs - ES6 shorthand fixes - Style consistency (blank lines) - Projection optimization ## Verification ✅ npm run build - success ✅ npm run test:api - 61/61 passing (+ new regression test) ✅ npm run lint - no errors ✅ All feedback from danny-avila resolved * docs: better JSDoc for the syncUserEntraGroupMemberships method --------- Co-authored-by: Airam Hernández Hernández --- api/server/services/GraphApiService.js | 110 +++++++++++++++++ api/server/services/PermissionService.js | 112 ++++++++++++++++-- api/server/services/PermissionService.spec.js | 43 +++++++ .../data-schemas/src/methods/userGroup.ts | 24 ++++ 4 files changed, 282 insertions(+), 7 deletions(-) diff --git a/api/server/services/GraphApiService.js b/api/server/services/GraphApiService.js index 08ca253964..aca7e4a168 100644 --- a/api/server/services/GraphApiService.js +++ b/api/server/services/GraphApiService.js @@ -279,6 +279,115 @@ const getGroupOwners = async (accessToken, sub, groupId) => { return []; } }; + +/** + * Get detailed information for specific Entra ID groups using batch requests + * Efficiently fetches group details for multiple groups in batches of 20 (Microsoft Graph limit) + * @param {string} accessToken - OpenID Connect access token + * @param {string} sub - Subject identifier + * @param {string[]} groupIds - Array of group IDs (GUIDs) to fetch details for + * @returns {Promise>} Array of group details + */ +const getEntraGroupDetailsBatch = async (accessToken, sub, groupIds) => { + try { + if (!groupIds || groupIds.length === 0) { + return []; + } + + // Safety check: warn if user has an unusually large number of groups + const MAX_REASONABLE_GROUPS = 200; + if (groupIds.length > MAX_REASONABLE_GROUPS) { + logger.warn( + `[getEntraGroupDetailsBatch] User has ${groupIds.length} groups (>${MAX_REASONABLE_GROUPS}). This may impact performance.`, + ); + } + + const graphClient = await createGraphClient(accessToken, sub); + const batchSize = 20; // Microsoft Graph batch API limit + const maxConcurrent = 5; // Limit concurrent batch requests + + // Helper function to process a single batch with retry logic for throttling + const processBatch = async (batchIds, retryCount = 0) => { + const batchRequest = { + requests: batchIds.map((id) => ({ + id, + method: 'GET', + url: `/groups/${id}?$select=id,displayName,mail,description`, + })), + }; + + try { + const batchResponse = await graphClient.api('/$batch').post(batchRequest); + const results = []; + const throttledIds = []; + + // Process batch responses + if (batchResponse.responses) { + for (const response of batchResponse.responses) { + if (response.status === 200 && response.body) { + results.push({ + id: response.body.id, + name: response.body.displayName, + email: + typeof response.body.mail === 'string' + ? response.body.mail.toLowerCase() + : response.body.mail, + description: response.body.description, + }); + } else if (response.status === 429 || response.status === 503) { + // Track throttled/unavailable groups for retry + throttledIds.push(response.id); + logger.warn( + `[getEntraGroupDetailsBatch] Group ${response.id} throttled/unavailable (${response.status}), will retry`, + ); + } else { + logger.warn( + `[getEntraGroupDetailsBatch] Failed to fetch group ${response.id}. Status: ${response.status}`, + ); + } + } + } + + // Retry throttled requests after a brief delay (max 2 retries) + if (throttledIds.length > 0 && retryCount < 2) { + const retryAfter = 1000 * (retryCount + 1); // Exponential backoff: 1s, 2s + logger.info( + `[getEntraGroupDetailsBatch] Retrying ${throttledIds.length} throttled groups after ${retryAfter}ms`, + ); + await new Promise((resolve) => setTimeout(resolve, retryAfter)); + const retryResults = await processBatch(throttledIds, retryCount + 1); + results.push(...retryResults); + } + + return results; + } catch (batchError) { + logger.error(`[getEntraGroupDetailsBatch] Error processing batch:`, batchError); + return []; + } + }; + + // Split groups into batches and process with limited concurrency + const batches = []; + for (let i = 0; i < groupIds.length; i += batchSize) { + batches.push(groupIds.slice(i, i + batchSize)); + } + + // Process batches with controlled concurrency + const allGroupDetails = []; + for (let i = 0; i < batches.length; i += maxConcurrent) { + const batchSlice = batches.slice(i, i + maxConcurrent); + const batchPromises = batchSlice.map((batch) => processBatch(batch)); + const batchResults = await Promise.all(batchPromises); + allGroupDetails.push(...batchResults.flat()); + } + + return allGroupDetails; + } catch (error) { + logger.error('[getEntraGroupDetailsBatch] Error fetching group details:', error); + return []; + } +}; + /** * Search for contacts (users only) using Microsoft Graph /me/people endpoint * Returns mapped TPrincipalSearchResult objects for users only @@ -535,6 +644,7 @@ module.exports = { createGraphClient, getUserEntraGroups, getUserOwnedEntraGroups, + getEntraGroupDetailsBatch, testGraphApiAccess, searchEntraIdPrincipals, exchangeTokenForGraphAccess, diff --git a/api/server/services/PermissionService.js b/api/server/services/PermissionService.js index fc67b0bc49..9d40564127 100644 --- a/api/server/services/PermissionService.js +++ b/api/server/services/PermissionService.js @@ -6,6 +6,7 @@ const { entraIdPrincipalFeatureEnabled, getUserOwnedEntraGroups, getUserEntraGroups, + getEntraGroupDetailsBatch, getGroupMembers, getGroupOwners, } = require('~/server/services/GraphApiService'); @@ -461,9 +462,16 @@ const ensureGroupPrincipalExists = async function (principal, authContext = null }; /** - * Synchronize user's Entra ID group memberships on sign-in - * Gets user's group IDs from GraphAPI and updates memberships only for existing groups in database - * Optionally includes groups the user owns if ENTRA_ID_INCLUDE_OWNERS_AS_MEMBERS is enabled + * Sync user's Entra ID group memberships with auto-creation of missing groups + * Optimized approach: + * 1. Get all group IDs user should be member of from Entra + * 2. Try to add user to existing groups (fast, no Graph API calls) + * 3. Query DB to identify which groups don't exist (indexed query, fast) + * 4. For missing groups only, fetch details from Graph API in batches + * 5. Upsert missing groups using upsertGroupByExternalId (race-safe) + * 6. Add user to newly created/upserted groups via bulkUpdate + * 7. Remove user from groups they're no longer member of + * * @param {Object} user - User object with authentication context * @param {string} user.openidId - User's OpenID subject identifier * @param {string} user.idOnTheSource - User's Entra ID (oid from token claims) @@ -478,6 +486,7 @@ const syncUserEntraGroupMemberships = async (user, accessToken, session = null) return; } + // Step 1: Get all group IDs user should be member of const memberGroupIds = await getUserEntraGroups(accessToken, user.openidId); let allGroupIds = [...(memberGroupIds || [])]; @@ -491,13 +500,22 @@ const syncUserEntraGroupMemberships = async (user, accessToken, session = null) } } - if (!allGroupIds || allGroupIds.length === 0) { + const sessionOptions = session ? { session } : {}; + + // Early return if no groups found (protects against temporary API failures) + if (allGroupIds.length === 0) { + logger.debug( + `[PermissionService.syncUserEntraGroupMemberships] No groups found for user ${user._id}`, + ); return; } - const sessionOptions = session ? { session } : {}; + logger.info( + `[PermissionService.syncUserEntraGroupMemberships] Syncing ${allGroupIds.length} groups for user ${user._id}`, + ); - await db.bulkUpdateGroups( + // Step 2: Try to add user to existing groups (fast operation) + const addResult = await db.bulkUpdateGroups( { idOnTheSource: { $in: allGroupIds }, source: 'entra', @@ -507,7 +525,77 @@ const syncUserEntraGroupMemberships = async (user, accessToken, session = null) sessionOptions, ); - await db.bulkUpdateGroups( + logger.debug( + `[PermissionService.syncUserEntraGroupMemberships] Added user to ${addResult.modifiedCount || 0} existing groups`, + ); + + // Step 3: Find which groups don't exist in DB using db layer + const existingGroups = await db.findGroupsByExternalIds(allGroupIds, 'entra', session); + const existingGroupIds = new Set(existingGroups.map((g) => g.idOnTheSource)); + + const missingGroupIds = allGroupIds.filter((id) => !existingGroupIds.has(id)); + + if (missingGroupIds.length > 0) { + logger.info( + `[PermissionService.syncUserEntraGroupMemberships] Found ${missingGroupIds.length} groups that don't exist, fetching details...`, + ); + + // Step 4: Fetch details only for missing groups (optimized batch request) + const groupDetails = await getEntraGroupDetailsBatch( + accessToken, + user.openidId, + missingGroupIds, + ); + + if (groupDetails.length > 0) { + logger.info( + `[PermissionService.syncUserEntraGroupMemberships] Creating ${groupDetails.length} new groups`, + ); + + // Step 5: Upsert missing groups (race-safe by design) + // Use upsertGroupByExternalId for each group to handle concurrent creates gracefully + const upsertPromises = groupDetails.map((group) => + db.upsertGroupByExternalId( + group.id, + 'entra', + { + name: group.name, + email: group.email, + description: group.description, + }, + session, + ), + ); + + await Promise.all(upsertPromises); + + // Step 6: Add user to all newly created/upserted groups + await db.bulkUpdateGroups( + { + idOnTheSource: { $in: missingGroupIds }, + source: 'entra', + memberIds: { $ne: user.idOnTheSource }, + }, + { $addToSet: { memberIds: user.idOnTheSource } }, + sessionOptions, + ); + + logger.info( + `[PermissionService.syncUserEntraGroupMemberships] Successfully created/updated ${groupDetails.length} groups`, + ); + } else { + logger.warn( + `[PermissionService.syncUserEntraGroupMemberships] Could not fetch details for ${missingGroupIds.length} missing groups`, + ); + } + } else { + logger.debug( + `[PermissionService.syncUserEntraGroupMemberships] All ${allGroupIds.length} groups already exist in database`, + ); + } + + // Step 7: Remove user from Entra groups they're no longer member of + const removeResult = await db.bulkUpdateGroups( { source: 'entra', memberIds: user.idOnTheSource, @@ -516,7 +604,17 @@ const syncUserEntraGroupMemberships = async (user, accessToken, session = null) { $pullAll: { memberIds: [user.idOnTheSource] } }, sessionOptions, ); + + logger.debug( + `[PermissionService.syncUserEntraGroupMemberships] Removed user from ${removeResult.modifiedCount || 0} groups`, + ); + + logger.info( + `[PermissionService.syncUserEntraGroupMemberships] Successfully synced groups for user ${user._id}`, + ); } catch (error) { + // Log error but don't re-throw: group sync is best-effort operation + // and should not block authentication even if temporary API/DB issues occur logger.error(`[PermissionService.syncUserEntraGroupMemberships] Error syncing groups:`, error); } }; diff --git a/api/server/services/PermissionService.spec.js b/api/server/services/PermissionService.spec.js index 477b0702b9..3fcda2a840 100644 --- a/api/server/services/PermissionService.spec.js +++ b/api/server/services/PermissionService.spec.js @@ -30,6 +30,7 @@ jest.mock('~/server/services/GraphApiService', () => ({ entraIdPrincipalFeatureEnabled: jest.fn().mockReturnValue(false), getUserOwnedEntraGroups: jest.fn().mockResolvedValue([]), getUserEntraGroups: jest.fn().mockResolvedValue([]), + getEntraGroupDetailsBatch: jest.fn().mockResolvedValue([]), getGroupMembers: jest.fn().mockResolvedValue([]), getGroupOwners: jest.fn().mockResolvedValue([]), })); @@ -2068,4 +2069,46 @@ describe('syncUserEntraGroupMemberships - $pullAll on Group.memberIds', () => { const group = await Group.findOne({ idOnTheSource: 'entra-safe' }).lean(); expect(group.memberIds).toContain(userEntraId); }); + + it('should handle mix of existing and missing groups correctly (regression test)', async () => { + // This is the critical bug fix: previously syncUserEntraGroups would remove user from existing groups + // when called with only missing groups. Now using upserts + bulkUpdate to avoid this issue. + + const { getEntraGroupDetailsBatch } = require('~/server/services/GraphApiService'); + + // Create existing groups A and B + await Group.create([ + { name: 'Group A', source: 'entra', idOnTheSource: 'entra-group-a', memberIds: [] }, + { name: 'Group B', source: 'entra', idOnTheSource: 'entra-group-b', memberIds: [] }, + ]); + + // User is member of A, B, and C (but C doesn't exist yet) + getUserEntraGroups.mockResolvedValue(['entra-group-a', 'entra-group-b', 'entra-group-c']); + + // Mock batch fetch to return details for missing group C + getEntraGroupDetailsBatch.mockResolvedValue([ + { + id: 'entra-group-c', + name: 'Group C', + email: 'groupc@example.com', + description: 'Group C Description', + }, + ]); + + await syncUserEntraGroupMemberships(user, 'fake-token'); + + // Verify ALL three groups now have the user as member + const groupA = await Group.findOne({ idOnTheSource: 'entra-group-a' }).lean(); + const groupB = await Group.findOne({ idOnTheSource: 'entra-group-b' }).lean(); + const groupC = await Group.findOne({ idOnTheSource: 'entra-group-c' }).lean(); + + expect(groupA.memberIds).toContain(userEntraId); + expect(groupB.memberIds).toContain(userEntraId); + expect(groupC).toBeTruthy(); + expect(groupC.memberIds).toContain(userEntraId); + expect(groupC.name).toBe('Group C'); + + // Reset mock + getEntraGroupDetailsBatch.mockResolvedValue([]); + }); }); diff --git a/packages/data-schemas/src/methods/userGroup.ts b/packages/data-schemas/src/methods/userGroup.ts index 0e6b57adb2..b337fb667b 100644 --- a/packages/data-schemas/src/methods/userGroup.ts +++ b/packages/data-schemas/src/methods/userGroup.ts @@ -48,6 +48,29 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { return await query.lean(); } + /** + * Find multiple groups by their external IDs (e.g., Entra IDs) in a single query + * @param idsOnTheSource - Array of external IDs + * @param source - The source ('entra' or 'local') + * @param session - Optional MongoDB session for transactions + * @returns Array of group documents + */ + async function findGroupsByExternalIds( + idsOnTheSource: string[], + source: 'entra' | 'local' = 'entra', + session?: ClientSession, + ): Promise { + const Group = mongoose.models.Group as Model; + const query = Group.find( + { idOnTheSource: { $in: idsOnTheSource }, source }, + { idOnTheSource: 1, _id: 0 }, + ); + if (session) { + query.session(session); + } + return await query.lean(); + } + /** * Find groups by name pattern (case-insensitive partial match) * @param namePattern - The name pattern to search for @@ -754,6 +777,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { return { findGroupById, findGroupByExternalId, + findGroupsByExternalIds, findGroupsByNamePattern, findGroupsByMemberId, createGroup,