🏃 fix: Improve OpenID Lookup Planning (#13229)

* fix: improve OpenID lookup planning

* fix: add issuer-bound source id index

* fix: align OpenID source id index

* fix: preserve admin refresh recency
This commit is contained in:
Danny Avila 2026-05-21 14:17:55 -04:00 • committed by GitHub
parent c345fd6bdb
commit cbdfe4614b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 372 additions and 86 deletions

View file

@ -339,7 +339,7 @@ describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => {
role: SystemRoles.USER,
};
findUser.mockImplementation(async (query) => {
if (query.$or && query.$or.some((c) => c.openidId === payload.sub)) {
if (query.openidId === payload.sub && query.openidIssuer === 'https://issuer.example.com') {
return existingUser;
}
return null;
@ -348,13 +348,10 @@ describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => {
const req = { headers: { authorization: 'Bearer tok' }, session: {} };
await invokeVerify(req, payload);
expect(findUser).toHaveBeenCalledWith(
expect.objectContaining({
$or: expect.arrayContaining([
{ openidId: payload.sub, openidIssuer: 'https://issuer.example.com' },
]),
}),
);
expect(findUser).toHaveBeenCalledWith({
openidId: payload.sub,
openidIssuer: 'https://issuer.example.com',
});
});
it('should use OPENID_EMAIL_CLAIM when set for email lookup', async () => {
@ -365,12 +362,13 @@ describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => {
const { user } = await invokeVerify(req, payload);
expect(findUser).toHaveBeenCalledTimes(2);
expect(findUser.mock.calls[0][0]).toMatchObject({
$or: expect.arrayContaining([
{ openidId: payload.sub, openidIssuer: 'https://issuer.example.com' },
]),
expect(findUser.mock.calls[0][0]).toEqual({
openidId: payload.sub,
openidIssuer: 'https://issuer.example.com',
});
expect(findUser.mock.calls[1][0]).toEqual({
email: 'test@corp.example.com',
});
expect(findUser.mock.calls[1][0]).toEqual({ email: 'test@corp.example.com' });
expect(user).toBe(false);
});

View file

@ -1,7 +1,10 @@
import { Types } from 'mongoose';
import { logger } from '@librechat/data-schemas';
import mongoose, { Types } from 'mongoose';
import { MongoMemoryServer } from 'mongodb-memory-server';
import { logger, createMethods, createModels } from '@librechat/data-schemas';
import { ErrorTypes } from 'librechat-data-provider';
import type { IUser, UserMethods } from '@librechat/data-schemas';
import type { CommandStartedEvent } from 'mongodb';
import type { FilterQuery } from 'mongoose';
import { findOpenIDUser, getOpenIdEmail, getOpenIdIssuer, normalizeOpenIdIssuer } from './openid';
function newId() {
@ -100,7 +103,8 @@ describe('findOpenIDUser', () => {
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [{ openidId: 'openid_123', openidIssuer: issuer }],
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(result).toEqual({
user: mockUser,
@ -118,7 +122,7 @@ describe('findOpenIDUser', () => {
username: 'testuser',
} as IUser;
mockFindUser.mockResolvedValueOnce(mockUser);
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
const result = await findOpenIDUser({
openidId: 'openid_123',
@ -127,11 +131,13 @@ describe('findOpenIDUser', () => {
idOnTheSource: 'source_123',
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [
{ openidId: 'openid_123', openidIssuer: issuer },
{ idOnTheSource: 'source_123', openidIssuer: issuer },
],
expect(mockFindUser).toHaveBeenNthCalledWith(1, {
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(mockFindUser).toHaveBeenNthCalledWith(2, {
idOnTheSource: 'source_123',
openidIssuer: issuer,
});
expect(result).toEqual({
user: mockUser,
@ -161,11 +167,10 @@ describe('findOpenIDUser', () => {
email: 'user@example.com',
});
expect(mockFindUser).toHaveBeenCalledTimes(1);
expect(mockFindUser).toHaveBeenCalledWith({
$or: [
{ openidId: 'openid_123', openidIssuer: issuer },
{ idOnTheSource: 'source_123', openidIssuer: issuer },
],
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(result).toEqual({
user: mockUser,
@ -194,7 +199,8 @@ describe('findOpenIDUser', () => {
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [{ openidId: 'openid_123', openidIssuer: 'https://issuer.example.com' }],
openidId: 'openid_123',
openidIssuer: 'https://issuer.example.com',
});
expect(result).toEqual({
user: mockUser,
@ -213,7 +219,7 @@ describe('findOpenIDUser', () => {
username: 'testuser',
} as IUser;
mockFindUser.mockResolvedValueOnce(mockUser);
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
const result = await findOpenIDUser({
openidId: 'openid_123',
@ -221,18 +227,13 @@ describe('findOpenIDUser', () => {
findUser: mockFindUser,
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [
{ openidId: 'openid_123', openidIssuer: 'https://issuer.example.com' },
{
openidId: 'openid_123',
$or: [
{ openidIssuer: { $exists: false } },
{ openidIssuer: null },
{ openidIssuer: '' },
],
},
],
expect(mockFindUser).toHaveBeenNthCalledWith(1, {
openidId: 'openid_123',
openidIssuer: 'https://issuer.example.com',
});
expect(mockFindUser).toHaveBeenNthCalledWith(2, {
openidId: 'openid_123',
openidIssuer: { $exists: false },
});
expect(result).toEqual({
user: { ...mockUser, openidIssuer: 'https://issuer.example.com' },
@ -251,7 +252,7 @@ describe('findOpenIDUser', () => {
username: 'testuser',
} as IUser;
mockFindUser.mockResolvedValueOnce(mockUser);
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
const result = await findOpenIDUser({
openidId: 'openid_123',
@ -259,18 +260,13 @@ describe('findOpenIDUser', () => {
findUser: mockFindUser,
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [
{ openidId: 'openid_123', openidIssuer: 'https://issuer.example.com' },
{
openidId: 'openid_123',
$or: [
{ openidIssuer: { $exists: false } },
{ openidIssuer: null },
{ openidIssuer: '' },
],
},
],
expect(mockFindUser).toHaveBeenNthCalledWith(1, {
openidId: 'openid_123',
openidIssuer: 'https://issuer.example.com',
});
expect(mockFindUser).toHaveBeenNthCalledWith(2, {
openidId: 'openid_123',
openidIssuer: { $exists: false },
});
expect(result).toEqual({
user: { ...mockUser, openidIssuer: 'https://issuer.example.com' },
@ -320,9 +316,12 @@ describe('findOpenIDUser', () => {
});
expect(mockFindUser).toHaveBeenNthCalledWith(1, {
$or: [{ openidId: 'openid_123', openidIssuer: issuer }],
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(mockFindUser).toHaveBeenNthCalledWith(2, {
email: 'user@example.com',
});
expect(mockFindUser).toHaveBeenNthCalledWith(2, { email: 'user@example.com' });
expect(result).toEqual({
user: mockUser,
error: null,
@ -361,7 +360,8 @@ describe('findOpenIDUser', () => {
expect(mockFindUser).toHaveBeenCalledTimes(1);
expect(mockFindUser).toHaveBeenCalledWith({
$or: [{ openidId: 'openid_123', openidIssuer: issuer }],
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(result).toEqual({
user: null,
@ -674,7 +674,8 @@ describe('findOpenIDUser', () => {
});
expect(mockFindUser).toHaveBeenCalledWith({
$or: [{ openidId: 'openid_123', openidIssuer: issuer }],
openidId: 'openid_123',
openidIssuer: issuer,
});
expect(result).toEqual({
user: null,
@ -763,6 +764,153 @@ describe('findOpenIDUser', () => {
});
});
type CapturedFindCommand = {
find?: unknown;
filter?: unknown;
};
function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === 'object' && value !== null;
}
function planContainsStage(value: unknown, stage: string): boolean {
if (!isRecord(value)) return false;
if (value.stage === stage) return true;
return Object.values(value).some((entry) => {
if (Array.isArray(entry)) return entry.some((item) => planContainsStage(item, stage));
return planContainsStage(entry, stage);
});
}
function getTotalDocsExamined(explain: unknown): number | undefined {
if (!isRecord(explain)) return undefined;
const executionStats = explain.executionStats;
if (!isRecord(executionStats)) return undefined;
const totalDocsExamined = executionStats.totalDocsExamined;
return typeof totalDocsExamined === 'number' ? totalDocsExamined : undefined;
}
describe('findOpenIDUser Mongo compatibility', () => {
let mongoServer: MongoMemoryServer;
let User: mongoose.Model<IUser>;
let methods: ReturnType<typeof createMethods>;
const issuer = 'https://issuer.example.com';
const originalOpenIdIssuer = process.env.OPENID_ISSUER;
async function seedUsers(count: number) {
await User.insertMany(
Array.from({ length: count }, (_, index) => ({
email: `filler-${index}@example.com`,
provider: 'openid',
openidId: `filler-sub-${index}`,
openidIssuer: issuer,
idOnTheSource: `filler-oid-${index}`,
})),
);
}
async function captureFindFilters<T>(run: () => Promise<T>): Promise<{
result: T;
filters: unknown[];
}> {
const filters: unknown[] = [];
const client = mongoose.connection.getClient();
const listener = (event: CommandStartedEvent) => {
const command = event.command as CapturedFindCommand;
if (event.commandName === 'find' && command.find === User.collection.name) {
filters.push(command.filter);
}
};
client.on('commandStarted', listener);
try {
const result = await run();
return { result, filters };
} finally {
client.off('commandStarted', listener);
}
}
beforeAll(async () => {
mongoServer = await MongoMemoryServer.create();
await mongoose.connect(mongoServer.getUri(), { monitorCommands: true });
createModels(mongoose);
User = mongoose.models.User as mongoose.Model<IUser>;
methods = createMethods(mongoose);
});
afterAll(async () => {
if (originalOpenIdIssuer == null) {
delete process.env.OPENID_ISSUER;
} else {
process.env.OPENID_ISSUER = originalOpenIdIssuer;
}
await mongoose.disconnect();
await mongoServer.stop();
});
beforeEach(async () => {
process.env.OPENID_ISSUER = issuer;
await mongoose.connection.dropDatabase();
await User.syncIndexes();
});
it('keeps exact issuer lookup indexable on a seeded user collection', async () => {
await seedUsers(1500);
await User.create({
email: 'target@example.com',
provider: 'openid',
openidId: 'target-sub',
openidIssuer: issuer,
idOnTheSource: 'target-oid',
});
const { result, filters } = await captureFindFilters(() =>
findOpenIDUser({
openidId: 'target-sub',
idOnTheSource: 'target-oid',
openidIssuer: issuer,
findUser: methods.findUser,
}),
);
expect(result.user?.email).toBe('target@example.com');
expect(filters).toEqual([{ openidId: 'target-sub', openidIssuer: issuer }]);
const explain = await User.findOne(filters[0] as FilterQuery<IUser>).explain('executionStats');
expect(planContainsStage(explain, 'IXSCAN')).toBe(true);
expect(getTotalDocsExamined(explain)).toBeLessThanOrEqual(1);
});
it('resolves legacy issuer-less users without nested or disjunctive filters', async () => {
await User.create({
email: 'legacy@example.com',
provider: 'openid',
openidId: 'legacy-sub',
idOnTheSource: 'legacy-oid',
});
const { result, filters } = await captureFindFilters(() =>
findOpenIDUser({
openidId: 'legacy-sub',
idOnTheSource: 'legacy-oid',
openidIssuer: issuer,
findUser: methods.findUser,
}),
);
expect(result.user?.email).toBe('legacy@example.com');
expect(result.migration).toBe(true);
expect(filters).toEqual([
{ openidId: 'legacy-sub', openidIssuer: issuer },
{ idOnTheSource: 'legacy-oid', openidIssuer: issuer },
{ openidId: 'legacy-sub', openidIssuer: { $exists: false } },
]);
});
});
describe('getOpenIdEmail', () => {
const originalEmailClaim = process.env.OPENID_EMAIL_CLAIM;

View file

@ -20,6 +20,11 @@ type OpenIdLookupField = 'openidId' | 'idOnTheSource';
type OpenIdUserResolution = { user: IUser | null; error: string | null; migration: boolean };
const OPENID_DISCOVERY_PATH = '/.well-known/openid-configuration';
const LEGACY_ISSUER_FILTERS: Array<FilterQuery<IUser>['openidIssuer']> = [
{ $exists: false },
null,
'',
];
export function normalizeOpenIdIssuer(issuer: string | undefined): string | undefined {
const normalized = issuer?.trim().replace(/\/+$/, '');
@ -56,24 +61,68 @@ function isLegacyOpenIdIssuer(openidIssuer: string | undefined): boolean {
return openidIssuer != null && loginIssuer != null && openidIssuer === loginIssuer;
}
function hasOpenIdLookupValue(value: string | undefined): value is string {
return typeof value === 'string' && value.length > 0;
}
function getIssuerExactCondition(
field: OpenIdLookupField,
value: string | undefined,
openidIssuer: string | undefined,
): FilterQuery<IUser> | null {
if (!hasOpenIdLookupValue(value) || !openidIssuer) return null;
return { [field]: value, openidIssuer };
}
function getLegacyIssuerConditions(
field: OpenIdLookupField,
value: string | undefined,
openidIssuer: string | undefined,
): FilterQuery<IUser>[] {
if (!hasOpenIdLookupValue(value) || !isLegacyOpenIdIssuer(openidIssuer)) return [];
return LEGACY_ISSUER_FILTERS.map((issuerFilter) => ({
[field]: value,
openidIssuer: issuerFilter,
}));
}
export function getIssuerBoundConditions(
field: OpenIdLookupField,
value: string | undefined,
openidIssuer: string | undefined,
): FilterQuery<IUser>[] {
if (!value || typeof value !== 'string') return [];
if (!openidIssuer) return [];
const exactCondition = getIssuerExactCondition(field, value, openidIssuer);
if (!exactCondition) return [];
return [exactCondition, ...getLegacyIssuerConditions(field, value, openidIssuer)];
}
const conditions: FilterQuery<IUser>[] = [{ [field]: value, openidIssuer }];
function getPrimaryLookupConditions(
openidId: string | undefined,
idOnTheSource: string | undefined,
openidIssuer: string | undefined,
): FilterQuery<IUser>[] {
const exactConditions = [
getIssuerExactCondition('openidId', openidId, openidIssuer),
getIssuerExactCondition('idOnTheSource', idOnTheSource, openidIssuer),
].filter((condition): condition is FilterQuery<IUser> => condition != null);
if (isLegacyOpenIdIssuer(openidIssuer)) {
conditions.push({
[field]: value,
$or: [{ openidIssuer: { $exists: false } }, { openidIssuer: null }, { openidIssuer: '' }],
});
return [
...exactConditions,
...getLegacyIssuerConditions('openidId', openidId, openidIssuer),
...getLegacyIssuerConditions('idOnTheSource', idOnTheSource, openidIssuer),
];
}
async function findFirstOpenIdUser(
findUser: UserMethods['findUser'],
conditions: FilterQuery<IUser>[],
): Promise<IUser | null> {
for (const condition of conditions) {
const user = await findUser(condition);
if (user) return user;
}
return conditions;
return null;
}
export function isUserIssuerAllowed(user: IUser, openidIssuer: string | undefined): boolean {
@ -160,14 +209,11 @@ export async function findOpenIDUser({
strategyName?: string;
}): Promise<OpenIdUserResolution> {
const normalizedIssuer = normalizeOpenIdIssuer(openidIssuer);
const primaryConditions = [
...getIssuerBoundConditions('openidId', openidId, normalizedIssuer),
...getIssuerBoundConditions('idOnTheSource', idOnTheSource, normalizedIssuer),
];
const primaryConditions = getPrimaryLookupConditions(openidId, idOnTheSource, normalizedIssuer);
let user = null;
let user: IUser | null = null;
if (primaryConditions.length > 0) {
user = await findUser({ $or: primaryConditions });
user = await findFirstOpenIdUser(findUser, primaryConditions);
}
const primaryIssuerResolution = resolveIssuerBoundUser(

View file

@ -19,6 +19,7 @@ const SUB = 'idp-sub-12345';
const ORIGINAL_OPENID_SCOPE = process.env.OPENID_SCOPE;
const ORIGINAL_OPENID_REFRESH_AUDIENCE = process.env.OPENID_REFRESH_AUDIENCE;
const ORIGINAL_OPENID_ISSUER = process.env.OPENID_ISSUER;
function makeUser(overrides: Partial<IUser> = {}): IUser {
const _id = overrides._id ?? new Types.ObjectId();
@ -75,6 +76,12 @@ describe('buildOpenIDRefreshParams', () => {
} else {
process.env.OPENID_REFRESH_AUDIENCE = ORIGINAL_OPENID_REFRESH_AUDIENCE;
}
if (ORIGINAL_OPENID_ISSUER === undefined) {
delete process.env.OPENID_ISSUER;
} else {
process.env.OPENID_ISSUER = ORIGINAL_OPENID_ISSUER;
}
});
it('returns scope-only params when OPENID_SCOPE is set', () => {
@ -296,10 +303,9 @@ describe('applyAdminRefresh', () => {
});
const [filter] = (deps.findUsers as jest.Mock).mock.calls[0];
expect(filter).toMatchObject({
$or: expect.arrayContaining([
{ openidId: SUB, openidIssuer: 'https://issuer.example.com' },
]),
expect(filter).toEqual({
openidId: SUB,
openidIssuer: 'https://issuer.example.com',
});
});
@ -316,6 +322,54 @@ describe('applyAdminRefresh', () => {
);
});
it('selects the most recently updated user across legacy issuer fallback filters', async () => {
const previousOpenIDIssuer = process.env.OPENID_ISSUER;
process.env.OPENID_ISSUER = 'https://issuer.example.com';
try {
const olderMissingIssuer = makeUser({
email: 'older@example.com',
openidIssuer: undefined,
updatedAt: new Date('2026-01-01T00:00:00.000Z'),
});
const newerEmptyIssuer = makeUser({
email: 'newer@example.com',
openidIssuer: '',
updatedAt: new Date('2026-02-01T00:00:00.000Z'),
});
const findUsers = jest
.fn()
.mockImplementation(async (filter: { openidIssuer?: unknown }) => {
if (filter.openidIssuer === '') return [newerEmptyIssuer];
if (
typeof filter.openidIssuer === 'object' &&
filter.openidIssuer != null &&
'$exists' in filter.openidIssuer
) {
return [olderMissingIssuer];
}
return [];
});
const deps = makeDeps(undefined, { findUsers });
const tokenset = makeTokenset({
claims: () => ({ sub: SUB, iss: 'https://issuer.example.com' }),
});
const result = await applyAdminRefresh(tokenset, deps, {
expectedIssuer: 'https://issuer.example.com',
});
expect(result.user.email).toBe('newer@example.com');
expect(deps.mintToken).toHaveBeenCalledWith(newerEmptyIssuer, tokenset);
expect(findUsers).toHaveBeenCalledTimes(4);
} finally {
if (previousOpenIDIssuer === undefined) {
delete process.env.OPENID_ISSUER;
} else {
process.env.OPENID_ISSUER = previousOpenIDIssuer;
}
}
});
it('throws USER_ID_MISMATCH when direct user_id resolves but issuer differs', async () => {
const id = new Types.ObjectId();
const user = makeUser({ _id: id, openidIssuer: 'https://other-issuer.example.com' });

View file

@ -133,6 +133,38 @@ export function buildOpenIDRefreshParams(): OpenIDRefreshParams {
return params;
}
function applyTenantFilter(
filter: FilterQuery<IUser>,
tenantId: string | undefined,
): FilterQuery<IUser> {
return tenantId ? ({ ...filter, tenantId } as FilterQuery<IUser>) : filter;
}
async function findLatestAdminUser(
deps: AdminRefreshDeps,
filters: FilterQuery<IUser>[],
tenantId: string | undefined,
): Promise<IUser | undefined> {
let latestUser: IUser | undefined;
for (const baseFilter of filters) {
const [user] = await deps.findUsers(
applyTenantFilter(baseFilter, tenantId),
SAFE_USER_PROJECTION,
{
sort: { updatedAt: -1 },
limit: 1,
},
);
if (!user) continue;
if (!latestUser || (user.updatedAt?.getTime() ?? 0) > (latestUser.updatedAt?.getTime() ?? 0)) {
latestUser = user;
}
}
return latestUser;
}
async function resolveAdminUser(
deps: AdminRefreshDeps,
openidId: string,
@ -173,17 +205,10 @@ async function resolveAdminUser(
}
const issuerBound = getIssuerBoundConditions('openidId', openidId, normalizedIssuer);
const baseFilter: FilterQuery<IUser> =
issuerBound.length > 0 ? { $or: issuerBound } : ({ openidId } as FilterQuery<IUser>);
const filter: FilterQuery<IUser> = expectedTenantId
? ({ ...baseFilter, tenantId: expectedTenantId } as FilterQuery<IUser>)
: baseFilter;
const filters =
issuerBound.length > 0 ? issuerBound : ([{ openidId }] as Array<FilterQuery<IUser>>);
const [user] = await deps.findUsers(filter, SAFE_USER_PROJECTION, {
sort: { updatedAt: -1 },
limit: 1,
});
return user;
return findLatestAdminUser(deps, filters, expectedTenantId);
}
function readClaims(tokenset: RefreshTokenset): AdminRefreshClaims {

View file

@ -38,6 +38,20 @@ beforeEach(async () => {
});
describe('User schema indexes', () => {
test('should define an issuer-bound idOnTheSource lookup index', async () => {
await User.syncIndexes();
const indexes = await User.collection.indexes();
expect(indexes).toEqual(
expect.arrayContaining([
expect.objectContaining({
key: { idOnTheSource: 1, openidIssuer: 1, tenantId: 1 },
}),
]),
);
});
test('should allow the same OpenID subject from different issuers', async () => {
await User.syncIndexes();

View file

@ -168,6 +168,7 @@ const userSchema = new Schema<IUser>(
userSchema.index({ email: 1, tenantId: 1 }, { unique: true });
userSchema.index({ role: 1, tenantId: 1 });
userSchema.index({ idOnTheSource: 1, openidIssuer: 1, tenantId: 1 });
const oAuthIdFields = [
'googleId',