Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,11 @@ USE_ENTRA_ID_FOR_PEOPLE_SEARCH=false
# When enabled, entra id groups owners will be considered as members of the group
ENTRA_ID_INCLUDE_OWNERS_AS_MEMBERS=false

# When enabled, login sync only updates membership of Entra ID groups already in the database
# (e.g. created when a resource is shared with them) instead of creating every group the user is in
# permissions.syncOnlyExistingEntraGroups in librechat.yaml takes precedence
ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS=false
Comment thread
jtodaroii marked this conversation as resolved.

# Microsoft Graph API scopes needed for people/group search
# Default scopes provide access to user profiles and group memberships
OPENID_GRAPH_SCOPES=User.Read,People.Read,GroupMember.Read.All
Expand Down
10 changes: 9 additions & 1 deletion api/server/controllers/auth/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ const {
getAdminPanelUrl,
isAdminPanelRedirect,
generateAdminExchangeCode,
getAppConfigOptionsFromUser,
} = require('@librechat/api');
const { syncUserEntraGroupMemberships } = require('~/server/services/PermissionService');
const { setAuthTokens } = require('~/server/services/AuthService');
const { getAppConfig } = require('~/server/services/Config');
const { sendOpenIDAuthResponse } = require('~/server/services/OpenIDRefreshRecovery');
const getLogStores = require('~/cache/getLogStores');
const { checkBan } = require('~/server/middleware');
Expand Down Expand Up @@ -76,7 +78,13 @@ function createOAuthHandler(redirectUri = domains.client) {
req.user.provider == 'openid' &&
isEnabled(process.env.OPENID_REUSE_TOKENS) === true
) {
await syncUserEntraGroupMemberships(req.user, req.user.tokenset.access_token);
const appConfig = await getAppConfig(getAppConfigOptionsFromUser(req.user));
await syncUserEntraGroupMemberships(
req.user,
req.user.tokenset.access_token,
null,
appConfig,
);
await sendOpenIDAuthResponse({
tokenset: req.user.tokenset,
user: req.user,
Expand Down
30 changes: 23 additions & 7 deletions api/server/services/PermissionService.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
const mongoose = require('mongoose');
const { AccessControlService, isEnabled, ensureDirectoryPrincipalUser } = require('@librechat/api');
const {
isEnabled,
AccessControlService,
ensureDirectoryPrincipalUser,
syncsOnlyExistingEntraGroups,
} = require('@librechat/api');
const {
tenantStorage,
getTenantId,
Expand Down Expand Up @@ -483,7 +488,8 @@ const ensureGroupPrincipalExists = async function (principal, authContext = null

/**
* Sync user's Entra ID group memberships with auto-creation of missing groups
* Optimized approach:
* Optimized approach (steps 4-6 are skipped when `permissions.syncOnlyExistingEntraGroups`
* or `ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS` is enabled, so only existing groups are synced):
* 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)
Expand All @@ -498,24 +504,25 @@ const ensureGroupPrincipalExists = async function (principal, authContext = null
* @param {string} user.provider - Authentication provider ('openid')
* @param {string} accessToken - Access token for Graph API calls
* @param {mongoose.ClientSession} [session] - Optional MongoDB session for transactions
* @param {AppConfig} [appConfig] - Resolved app config for the user
* @returns {Promise<void>}
*/
const syncUserEntraGroupMemberships = async (user, accessToken, session = null) => {
const syncUserEntraGroupMemberships = async (user, accessToken, session = null, appConfig) => {
const tenantId = user?.tenantId ? String(user.tenantId) : undefined;
if (!tenantId || getTenantId() != null) {
return performEntraGroupMembershipSync(user, accessToken, session);
return performEntraGroupMembershipSync(user, accessToken, session, appConfig);
}
/**
* The OAuth callback runs before `tenantContextMiddleware`, so establish the
* user's tenant context here: group queries, created groups, and principal
* cache invalidation are then scoped exactly like authenticated reads.
*/
return tenantStorage.run({ tenantId, userId: user._id?.toString() }, async () =>
performEntraGroupMembershipSync(user, accessToken, session),
performEntraGroupMembershipSync(user, accessToken, session, appConfig),
);
};

const performEntraGroupMembershipSync = async (user, accessToken, session = null) => {
const performEntraGroupMembershipSync = async (user, accessToken, session = null, appConfig) => {
try {
if (!entraIdPrincipalFeatureEnabled(user) || !accessToken || !user.idOnTheSource) {
return;
Expand Down Expand Up @@ -570,7 +577,16 @@ const performEntraGroupMembershipSync = async (user, accessToken, session = null

const missingGroupIds = allGroupIds.filter((id) => !existingGroupIds.has(id));

if (missingGroupIds.length > 0) {
const syncOnlyExisting = syncsOnlyExistingEntraGroups(
appConfig?.config?.permissions,
process.env.ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS,
);

if (missingGroupIds.length > 0 && syncOnlyExisting) {
logger.debug(
`[PermissionService.syncUserEntraGroupMemberships] Skipping ${missingGroupIds.length} groups not in database`,
);
} else if (missingGroupIds.length > 0) {
logger.info(
`[PermissionService.syncUserEntraGroupMemberships] Found ${missingGroupIds.length} groups that don't exist, fetching details...`,
);
Expand Down
72 changes: 72 additions & 0 deletions api/server/services/PermissionService.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2536,4 +2536,76 @@ describe('syncUserEntraGroupMemberships - $pullAll on Group.memberIds', () => {
// Reset mock
getEntraGroupDetailsBatch.mockResolvedValue([]);
});

describe('with ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS enabled', () => {
const { getEntraGroupDetailsBatch } = require('~/server/services/GraphApiService');

beforeEach(() => {
process.env.ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS = 'true';
getEntraGroupDetailsBatch.mockClear();
});

afterEach(() => {
delete process.env.ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS;
getEntraGroupDetailsBatch.mockResolvedValue([]);
});

it('does not create groups missing from the database', async () => {
await Group.create({
name: 'Shared',
source: 'entra',
idOnTheSource: 'entra-shared',
memberIds: [],
});
getUserEntraGroups.mockResolvedValue(['entra-shared', 'entra-unused']);
getEntraGroupDetailsBatch.mockResolvedValue([{ id: 'entra-unused', name: 'Unused' }]);

await syncUserEntraGroupMemberships(user, 'fake-token');

expect(getEntraGroupDetailsBatch).not.toHaveBeenCalled();
expect(await Group.countDocuments({ idOnTheSource: 'entra-unused' })).toBe(0);
const shared = await Group.findOne({ idOnTheSource: 'entra-shared' }).lean();
expect(shared.memberIds).toContain(userEntraId);
});

it('still removes the user from existing groups they left', async () => {
await Group.create({
name: 'Left',
source: 'entra',
idOnTheSource: 'entra-left',
memberIds: [userEntraId],
});
getUserEntraGroups.mockResolvedValue(['entra-unused']);
getEntraGroupDetailsBatch.mockResolvedValue([{ id: 'entra-unused', name: 'Unused' }]);

await syncUserEntraGroupMemberships(user, 'fake-token');

const left = await Group.findOne({ idOnTheSource: 'entra-left' }).lean();
expect(left.memberIds).not.toContain(userEntraId);
expect(await Group.countDocuments({ idOnTheSource: 'entra-unused' })).toBe(0);
});

it('creates missing groups when librechat.yaml turns the setting off', async () => {
getUserEntraGroups.mockResolvedValue(['entra-unused']);
getEntraGroupDetailsBatch.mockResolvedValue([{ id: 'entra-unused', name: 'Unused' }]);
const appConfig = { config: { permissions: { syncOnlyExistingEntraGroups: false } } };

await syncUserEntraGroupMemberships(user, 'fake-token', null, appConfig);

const created = await Group.findOne({ idOnTheSource: 'entra-unused' }).lean();
expect(created.memberIds).toContain(userEntraId);
});
});

it('skips missing groups when only librechat.yaml enables the setting', async () => {
const { getEntraGroupDetailsBatch } = require('~/server/services/GraphApiService');
getUserEntraGroups.mockResolvedValue(['entra-unused']);
getEntraGroupDetailsBatch.mockResolvedValue([{ id: 'entra-unused', name: 'Unused' }]);
const appConfig = { config: { permissions: { syncOnlyExistingEntraGroups: true } } };

await syncUserEntraGroupMemberships(user, 'fake-token', null, appConfig);

expect(await Group.countDocuments({ idOnTheSource: 'entra-unused' })).toBe(0);
getEntraGroupDetailsBatch.mockResolvedValue([]);
});
});
8 changes: 4 additions & 4 deletions e2e/specs/mock/usage.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,8 @@ test.describe('context usage gauge', () => {
await page.setViewportSize({ width: viewport.width, height: viewport.height });
await expect
.poll(() =>
details.evaluate((element) =>
getComputedStyle(element).gridTemplateColumns.split(' ').length,
details.evaluate(
(element) => getComputedStyle(element).gridTemplateColumns.split(' ').length,
),
)
.toBe(viewport.columns);
Expand Down Expand Up @@ -326,8 +326,8 @@ test.describe('context usage gauge', () => {
await page.setViewportSize({ width: 375, height: 812 });
await expect
.poll(() =>
details.evaluate((element) =>
getComputedStyle(element).gridTemplateColumns.split(' ').length,
details.evaluate(
(element) => getComputedStyle(element).gridTemplateColumns.split(' ').length,
),
)
.toBe(1);
Expand Down
3 changes: 3 additions & 0 deletions librechat.example.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1799,3 +1799,6 @@ endpoints:
# maxWriteAttempts: 3 # Integer 1-100, including the first attempt.
# leaseTtlMs: 30000 # Integer 1000-600000. Sharing updates on deployments without transactions hold a per-resource lease this long between renewals.
# leaseWaitMs: 3000 # Integer 0-60000. How long an update waits for a contended lease before answering 409.
# # Login sync updates only Entra ID groups already in the database instead of creating
# # every group the user is in. Unset defers to ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS.
# syncOnlyExistingEntraGroups: false
19 changes: 18 additions & 1 deletion packages/api/src/acl/principals.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ensureDirectoryPrincipalUser } from './principals';
import { ensureDirectoryPrincipalUser, syncsOnlyExistingEntraGroups } from './principals';

const createMethods = () => ({
findUserBySourceId: jest.fn().mockResolvedValue(null),
Expand Down Expand Up @@ -52,3 +52,20 @@ describe('ensureDirectoryPrincipalUser', () => {
expect(methods.findUserBySourceId).not.toHaveBeenCalled();
});
});

describe('syncsOnlyExistingEntraGroups', () => {
it('defaults to false when neither setting is present', () => {
expect(syncsOnlyExistingEntraGroups(undefined, undefined)).toBe(false);
});

it('falls back to the env flag when the yaml setting is unset', () => {
expect(syncsOnlyExistingEntraGroups({ maxWriteAttempts: 3 }, 'true')).toBe(true);
});

it('lets the yaml setting override the env flag', () => {
expect(syncsOnlyExistingEntraGroups({ syncOnlyExistingEntraGroups: false }, 'true')).toBe(
false,
);
expect(syncsOnlyExistingEntraGroups({ syncOnlyExistingEntraGroups: true }, 'false')).toBe(true);
});
});
9 changes: 8 additions & 1 deletion packages/api/src/acl/principals.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { TPrincipal } from 'librechat-data-provider';
import type { TCustomConfig, TPrincipal } from 'librechat-data-provider';
import { isEnabled } from '~/utils';

export interface DirectoryPrincipalUser {
id: string;
Expand Down Expand Up @@ -46,3 +47,9 @@ export const ensureDirectoryPrincipalUser = async (
idOnTheSource: principal.idOnTheSource,
});
};

/** `permissions.syncOnlyExistingEntraGroups` in `librechat.yaml` takes precedence over the env flag. */
export const syncsOnlyExistingEntraGroups = (
permissions: TCustomConfig['permissions'],
envValue: string | undefined,
): boolean => permissions?.syncOnlyExistingEntraGroups ?? isEnabled(envValue);
3 changes: 3 additions & 0 deletions packages/data-provider/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4126,6 +4126,9 @@ export const configSchema = z.object({
maxWriteAttempts: permissionWriteAttemptsSchema,
leaseTtlMs: permissionLeaseTtlSchema,
leaseWaitMs: permissionLeaseWaitSchema,
/** Login sync updates only Entra ID groups already in the database instead of creating
* every group the user is in. Unset defers to `ENTRA_ID_SYNC_ONLY_EXISTING_GROUPS`. */
syncOnlyExistingEntraGroups: z.boolean().optional(),
})
.optional(),
cache: z.boolean().default(true),
Expand Down