mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-01 11:53:55 +00:00
🔁 fix: Tighten Google admin refresh and limit social-login changes
Brutal-review findings on top of the upstream feature work.
socialLogin.js: the migrate-or-reject pattern from the previous commit
applied to every provider's chat-side verify callback, not just the admin
flow. Gate both branches on `options.existingUsersOnly` so the chat-side
googleLogin / facebookLogin / etc. keep their pre-existing email-fallback
behavior unchanged. Tests follow: restore the original `should fallback to
finding user by email` chat-side case and re-add the migration and
mismatch-reject cases as admin-only by passing `{ existingUsersOnly: true }`
to socialLogin in those tests.
googleRefresh.ts: add a defense-in-depth `isEmailAllowed(user)` dep that
the helper invokes before `canAccessAdmin`. Mirrors the
`isEmailDomainAllowed` check the initial Google admin login already runs,
so a deployment that removes a domain from `registration.allowedDomains`
after issuance can no longer mint fresh JWTs for that admin via refresh.
The route handler wires it up with `resolveAppConfigForUser` +
`isEmailDomainAllowed`, falling back to `baseOnly` config for users
without a tenantId.
googleRefresh.ts: drop the unreachable `?? ''` defensive coalescing in
`fetchGoogleTokenset`. The `GOOGLE_NOT_CONFIGURED` guard upstream already
narrows `clientId`/`clientSecret` to non-empty strings; the function
takes a narrowed `GoogleAdminRefreshConfiguredOptions` shape and
`applyGoogleAdminRefresh` constructs that shape after the guard.
This commit is contained in:
parent
21922eea78
commit
0e55d8a1df
5 changed files with 112 additions and 16 deletions
|
|
@ -21,6 +21,8 @@ const {
|
|||
applyGoogleAdminRefresh,
|
||||
AdminRefreshError,
|
||||
buildOpenIDRefreshParams,
|
||||
isEmailDomainAllowed,
|
||||
resolveAppConfigForUser,
|
||||
} = require('@librechat/api');
|
||||
const { loginController } = require('~/server/controllers/auth/LoginController');
|
||||
const { hasCapability, requireCapability } = require('~/server/middleware/roles/capabilities');
|
||||
|
|
@ -58,6 +60,20 @@ function buildGoogleAdminRefreshDeps(sessionExpiry) {
|
|||
return false;
|
||||
}
|
||||
},
|
||||
isEmailAllowed: async (user) => {
|
||||
if (!user?.email) return false;
|
||||
try {
|
||||
const appConfig = user.tenantId
|
||||
? await resolveAppConfigForUser(getAppConfig, user)
|
||||
: await getAppConfig({ baseOnly: true });
|
||||
return isEmailDomainAllowed(user.email, appConfig?.registration?.allowedDomains);
|
||||
} catch (err) {
|
||||
logger.warn(
|
||||
`[admin/oauth/refresh] domain allowlist check failed, denying: ${err?.message}`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
mintToken: async (user) => ({
|
||||
token: await generateToken(user, sessionExpiry),
|
||||
expiresAt: Date.now() + sessionExpiry,
|
||||
|
|
|
|||
|
|
@ -59,15 +59,20 @@ const socialLogin =
|
|||
refreshToken ? cb(null, user, { refreshToken }) : cb(null, user);
|
||||
|
||||
if (existingUser?.provider === provider) {
|
||||
if (id && existingUser[providerKey] && existingUser[providerKey] !== id) {
|
||||
if (
|
||||
options.existingUsersOnly &&
|
||||
id &&
|
||||
existingUser[providerKey] &&
|
||||
existingUser[providerKey] !== id
|
||||
) {
|
||||
logger.warn(
|
||||
`[${provider}Login] Rejected email fallback for ${email}: stored ${providerKey} does not match`,
|
||||
`[${provider}Login] Rejected admin email fallback for ${email}: stored ${providerKey} does not match`,
|
||||
);
|
||||
const error = new Error(ErrorTypes.AUTH_FAILED);
|
||||
error.code = ErrorTypes.AUTH_FAILED;
|
||||
return cb(error);
|
||||
}
|
||||
if (id && !existingUser[providerKey]) {
|
||||
if (options.existingUsersOnly && id && !existingUser[providerKey]) {
|
||||
await updateUser(existingUser._id, { [providerKey]: id });
|
||||
existingUser[providerKey] = id;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -139,8 +139,7 @@ describe('socialLogin', () => {
|
|||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('migrates the missing provider id when finding by email fallback', async () => {
|
||||
const { updateUser } = require('~/models');
|
||||
it('should fallback to finding user by email if not found by provider ID', async () => {
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-789';
|
||||
const email = 'user@example.com';
|
||||
|
|
@ -149,6 +148,7 @@ describe('socialLogin', () => {
|
|||
_id: 'user789',
|
||||
email: email,
|
||||
provider: 'google',
|
||||
googleId: 'old-google-id',
|
||||
};
|
||||
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
|
|
@ -173,19 +173,49 @@ describe('socialLogin', () => {
|
|||
`[${provider}Login] User found by email: ${email} but not by ${provider}Id`,
|
||||
);
|
||||
|
||||
expect(updateUser).toHaveBeenCalledWith('user789', { googleId });
|
||||
expect(handleExistingUser).toHaveBeenCalled();
|
||||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('migrates the missing provider id when finding by email fallback (admin path)', async () => {
|
||||
const { updateUser } = require('~/models');
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-789';
|
||||
const email = 'admin@example.com';
|
||||
|
||||
const existingUser = {
|
||||
_id: 'admin789',
|
||||
email: email,
|
||||
provider: 'google',
|
||||
};
|
||||
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
|
||||
const mockProfile = {
|
||||
id: googleId,
|
||||
emails: [{ value: email, verified: true }],
|
||||
photos: [{ value: 'https://example.com/avatar.png' }],
|
||||
name: { givenName: 'Admin', familyName: 'User' },
|
||||
};
|
||||
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails, { existingUsersOnly: true });
|
||||
const callback = jest.fn();
|
||||
|
||||
await loginFn(null, null, null, mockProfile, callback);
|
||||
|
||||
expect(updateUser).toHaveBeenCalledWith('admin789', { googleId });
|
||||
expect(existingUser.googleId).toBe(googleId);
|
||||
expect(handleExistingUser).toHaveBeenCalled();
|
||||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('rejects the email fallback when the stored provider id differs from the current sub', async () => {
|
||||
it('rejects the admin email fallback when stored provider id differs from the current sub', async () => {
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-new';
|
||||
const email = 'user@example.com';
|
||||
const email = 'admin@example.com';
|
||||
|
||||
const existingUser = {
|
||||
_id: 'user789',
|
||||
_id: 'admin789',
|
||||
email: email,
|
||||
provider: 'google',
|
||||
googleId: 'google-user-old',
|
||||
|
|
@ -197,16 +227,16 @@ describe('socialLogin', () => {
|
|||
id: googleId,
|
||||
emails: [{ value: email, verified: true }],
|
||||
photos: [{ value: 'https://example.com/avatar.png' }],
|
||||
name: { givenName: 'Bob', familyName: 'Johnson' },
|
||||
name: { givenName: 'Admin', familyName: 'User' },
|
||||
};
|
||||
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails);
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails, { existingUsersOnly: true });
|
||||
const callback = jest.fn();
|
||||
|
||||
await loginFn(null, null, null, mockProfile, callback);
|
||||
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
`[${provider}Login] Rejected email fallback for ${email}: stored ${provider}Id does not match`,
|
||||
`[${provider}Login] Rejected admin email fallback for ${email}: stored ${provider}Id does not match`,
|
||||
);
|
||||
expect(handleExistingUser).not.toHaveBeenCalled();
|
||||
expect(callback).toHaveBeenCalledWith(
|
||||
|
|
|
|||
|
|
@ -69,6 +69,7 @@ describe('applyGoogleAdminRefresh', () => {
|
|||
findUsers: jest.fn(),
|
||||
getUserById: jest.fn(),
|
||||
canAccessAdmin: jest.fn(),
|
||||
isEmailAllowed: jest.fn().mockResolvedValue(true),
|
||||
mintToken: jest.fn(),
|
||||
};
|
||||
originalFetch = global.fetch;
|
||||
|
|
@ -247,6 +248,22 @@ describe('applyGoogleAdminRefresh', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('throws FORBIDDEN when isEmailAllowed rejects the refreshed identity', async () => {
|
||||
const user = makeUser();
|
||||
fetchMock.mockResolvedValueOnce(
|
||||
makeOkJson({ access_token: 'new-access', id_token: makeIdToken() }),
|
||||
);
|
||||
deps.findUsers.mockResolvedValue([user]);
|
||||
(deps.isEmailAllowed as jest.Mock).mockResolvedValue(false);
|
||||
|
||||
await expect(applyGoogleAdminRefresh(deps, baseOptions)).rejects.toMatchObject({
|
||||
code: 'FORBIDDEN',
|
||||
status: 403,
|
||||
message: expect.stringContaining('domain'),
|
||||
});
|
||||
expect(deps.canAccessAdmin).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns the rotated refresh_token when Google supplies one', async () => {
|
||||
const user = makeUser();
|
||||
fetchMock.mockResolvedValueOnce(
|
||||
|
|
|
|||
|
|
@ -35,6 +35,13 @@ export interface GoogleAdminRefreshDeps {
|
|||
) => Promise<IUser[]>;
|
||||
getUserById: (id: string, projection: string) => Promise<IUser | null>;
|
||||
canAccessAdmin: (user: IUser) => Promise<boolean>;
|
||||
/**
|
||||
* Re-runs the deployment's `registration.allowedDomains` check against the
|
||||
* resolved user's email. Returns true to allow refresh, false to reject.
|
||||
* Mirrors the `isEmailDomainAllowed` call the initial OAuth login enforces
|
||||
* so a domain removed from the allowlist after issuance can't refresh.
|
||||
*/
|
||||
isEmailAllowed?: (user: IUser) => Promise<boolean>;
|
||||
mintToken: (user: IUser) => Promise<MintedGoogleAdminToken>;
|
||||
}
|
||||
|
||||
|
|
@ -80,15 +87,22 @@ async function resolveSubFromUserinfo(accessToken: string): Promise<string | und
|
|||
}
|
||||
}
|
||||
|
||||
async function fetchGoogleTokenset(options: GoogleAdminRefreshOptions): Promise<GoogleTokenset> {
|
||||
interface GoogleAdminRefreshConfiguredOptions extends GoogleAdminRefreshOptions {
|
||||
clientId: string;
|
||||
clientSecret: string;
|
||||
}
|
||||
|
||||
async function fetchGoogleTokenset(
|
||||
options: GoogleAdminRefreshConfiguredOptions,
|
||||
): Promise<GoogleTokenset> {
|
||||
let response: Response;
|
||||
try {
|
||||
response = await fetch(GOOGLE_TOKEN_ENDPOINT, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
|
||||
body: new URLSearchParams({
|
||||
client_id: options.clientId ?? '',
|
||||
client_secret: options.clientSecret ?? '',
|
||||
client_id: options.clientId,
|
||||
client_secret: options.clientSecret,
|
||||
refresh_token: options.refreshToken,
|
||||
grant_type: 'refresh_token',
|
||||
}),
|
||||
|
|
@ -212,10 +226,24 @@ export async function applyGoogleAdminRefresh(
|
|||
);
|
||||
}
|
||||
|
||||
const tokenset = await fetchGoogleTokenset(options);
|
||||
const configured: GoogleAdminRefreshConfiguredOptions = {
|
||||
...options,
|
||||
clientId: options.clientId,
|
||||
clientSecret: options.clientSecret,
|
||||
};
|
||||
|
||||
const tokenset = await fetchGoogleTokenset(configured);
|
||||
const googleId = await resolveGoogleSub(tokenset);
|
||||
const user = await resolveAdminUser(googleId, deps, options);
|
||||
|
||||
if (deps.isEmailAllowed && !(await deps.isEmailAllowed(user))) {
|
||||
throw new AdminRefreshError(
|
||||
'FORBIDDEN',
|
||||
403,
|
||||
'User email domain is not on the deployment allowlist',
|
||||
);
|
||||
}
|
||||
|
||||
if (!(await deps.canAccessAdmin(user))) {
|
||||
throw new AdminRefreshError('FORBIDDEN', 403, 'User does not have admin access');
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue