mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 01:53:33 +00:00
📮 fix: Consume an Invite Only Once the Account Exists (#15611)
* 📮 fix: Consume an Invite Only Once the Account Exists `checkInviteUser` deleted the invite token and then called `next()`, but everything that can still reject a registration runs after it: the schema, the allowed-domain check, and the email-already-in-use check. A mistyped password confirmation therefore destroyed the invite — the invitee corrected it, resubmitted, and got "Invalid invite token" with no way back short of an admin re-inviting them. The deletion moves to `registrationController`, after `registerUser` reports success. That report needed a new signal. `registerUser` returns the same 200 and the same generic message whether it created an account or found the email already in use — deliberately, so the response cannot be used to enumerate accounts — so the status alone cannot say whether an account exists. It now also returns `userCreated` on the creation path, which the controller reads and never forwards; the response body is unchanged. A failed deletion is logged rather than surfaced. By that point the account exists, and leaving a usable invite behind is recoverable in a way that telling the user their registration failed is not. Fixes #15541 * ♻️ test: Fold the Invite Tests Into the Existing AuthController Spec From Copilot's review. The new spec reached for `jest.requireActual` on `@librechat/data-schemas`, `~/server/services/AuthService` and `~/models`, which pulls real implementations into a unit test — `~/models` builds the data-schemas methods against mongoose — and diverges from `AuthController.spec.js`, which stubs each module outright. Rather than restate that harness with narrower stubs, the tests move into the spec that already has it. `deleteTokens` joins its `~/models` mock, which also keeps that shared mock in step with the controller's imports: a stale mock there hands the controller an undefined function, and the omission only surfaces when some later test happens to exercise the path.
This commit is contained in:
parent
fe30e3db32
commit
5e3cdd0b2c
5 changed files with 181 additions and 6 deletions
|
|
@ -19,7 +19,13 @@ const {
|
|||
setAuthTokens,
|
||||
registerUser,
|
||||
} = require('~/server/services/AuthService');
|
||||
const { deleteAllUserSessions, getUserById, findSession, updateUser } = require('~/models');
|
||||
const {
|
||||
deleteAllUserSessions,
|
||||
getUserById,
|
||||
findSession,
|
||||
updateUser,
|
||||
deleteTokens,
|
||||
} = require('~/models');
|
||||
const { getGraphApiToken } = require('~/server/services/GraphTokenService');
|
||||
const { getRefreshTokenBridge } = require('~/server/services/RefreshTokenBridge');
|
||||
const {
|
||||
|
|
@ -52,6 +58,18 @@ const registrationController = async (req, res) => {
|
|||
try {
|
||||
const response = await registerUser(req.body);
|
||||
const { status, message } = response;
|
||||
/** Consume the invite only once the account exists. `registerUser` returns the same
|
||||
* 200 whether it created a user or found the email already in use, so the decision
|
||||
* rests on `userCreated` rather than the status. A failure to delete leaves a
|
||||
* usable invite, which is recoverable; failing the response here would tell a user
|
||||
* whose account was just created that registration failed, which is not. */
|
||||
if (response.userCreated === true && req.invite?.token != null) {
|
||||
try {
|
||||
await deleteTokens({ token: req.invite.token });
|
||||
} catch (error) {
|
||||
logger.error('[registrationController] Failed to consume invite after registration', error);
|
||||
}
|
||||
}
|
||||
res.status(status).send({ message });
|
||||
} catch (err) {
|
||||
logger.error('[registrationController]', err);
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ jest.mock('~/models', () => ({
|
|||
findSession: jest.fn(),
|
||||
updateUser: jest.fn(),
|
||||
findUser: jest.fn(),
|
||||
deleteTokens: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/server/services/RefreshTokenBridge', () => ({
|
||||
OPENID_REFRESH_BRIDGE_GRACE_MS: 60 * 1000,
|
||||
|
|
@ -119,7 +120,11 @@ const openIdClient = require('openid-client');
|
|||
const jwt = require('jsonwebtoken');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { isEnabled, findOpenIDUser, buildOpenIDRefreshParams } = require('@librechat/api');
|
||||
const { graphTokenController, refreshController } = require('./AuthController');
|
||||
const {
|
||||
graphTokenController,
|
||||
refreshController,
|
||||
registrationController,
|
||||
} = require('./AuthController');
|
||||
const { getGraphApiToken } = require('~/server/services/GraphTokenService');
|
||||
const {
|
||||
clearOpenIDAuthTokens,
|
||||
|
|
@ -128,9 +133,10 @@ const {
|
|||
storeOpenIDSession,
|
||||
setCloudFrontAuthCookies,
|
||||
setAuthTokens,
|
||||
registerUser,
|
||||
} = require('~/server/services/AuthService');
|
||||
const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies');
|
||||
const { deleteSession, getUserById, findSession, updateUser } = require('~/models');
|
||||
const { deleteSession, getUserById, findSession, updateUser, deleteTokens } = require('~/models');
|
||||
const {
|
||||
createRefreshTokenBridgeFlightKey,
|
||||
deleteRefreshTokenBridges,
|
||||
|
|
@ -2361,3 +2367,77 @@ describe('refreshController – LibreChat path', () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('registrationController - invite consumption', () => {
|
||||
const invite = { token: 'hashed-invite', email: 'invitee@example.com' };
|
||||
|
||||
const buildRes = () => {
|
||||
const res = {};
|
||||
res.status = jest.fn(() => res);
|
||||
res.send = jest.fn(() => res);
|
||||
res.json = jest.fn(() => res);
|
||||
return res;
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
it('consumes the invite once the account exists', async () => {
|
||||
registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true });
|
||||
|
||||
await registrationController({ body: {}, invite }, buildRes());
|
||||
|
||||
expect(deleteTokens).toHaveBeenCalledWith({ token: 'hashed-invite' });
|
||||
});
|
||||
|
||||
it('leaves the invite when registration is rejected', () => {
|
||||
/** A mistyped password confirmation is the common case; it has to stay retryable. */
|
||||
registerUser.mockResolvedValue({ status: 404, message: 'The passwords did not match' });
|
||||
|
||||
return registrationController({ body: {}, invite }, buildRes()).then(() => {
|
||||
expect(deleteTokens).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
it('leaves the invite when the email is already in use, despite the 200', async () => {
|
||||
/** `registerUser` returns the same status and message whether it created an account
|
||||
* or found the email taken, so the status alone cannot drive this decision. */
|
||||
registerUser.mockResolvedValue({ status: 200, message: 'ok' });
|
||||
|
||||
await registrationController({ body: {}, invite }, buildRes());
|
||||
|
||||
expect(deleteTokens).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('does not attempt a deletion for an uninvited registration', async () => {
|
||||
registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true });
|
||||
|
||||
await registrationController({ body: {} }, buildRes());
|
||||
|
||||
expect(deleteTokens).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('still reports success when consuming the invite fails', async () => {
|
||||
/** The account exists by this point; reporting failure would be worse than
|
||||
* leaving a usable invite behind. */
|
||||
registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true });
|
||||
deleteTokens.mockRejectedValue(new Error('mongo unavailable'));
|
||||
const res = buildRes();
|
||||
|
||||
await registrationController({ body: {}, invite }, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
expect(res.send).toHaveBeenCalledWith({ message: 'ok' });
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('never forwards the creation signal to the client', async () => {
|
||||
registerUser.mockResolvedValue({ status: 200, message: 'ok', userCreated: true });
|
||||
const res = buildRes();
|
||||
|
||||
await registrationController({ body: {}, invite }, res);
|
||||
|
||||
expect(res.send).toHaveBeenCalledWith({ message: 'ok' });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
const { getInvite: getInviteFn } = require('@librechat/api');
|
||||
const { createToken, findToken, deleteTokens } = require('~/models');
|
||||
const { createToken, findToken } = require('~/models');
|
||||
|
||||
const getInvite = (encodedToken, email) =>
|
||||
getInviteFn(encodedToken, email, { createToken, findToken });
|
||||
|
|
@ -19,7 +19,11 @@ async function checkInviteUser(req, res, next) {
|
|||
return res.status(400).json({ message: 'Invalid invite token' });
|
||||
}
|
||||
|
||||
await deleteTokens({ token: invite.token });
|
||||
/** The invite is deliberately left in place here. Everything that can still
|
||||
* reject this registration — the schema, the allowed-domain check, an email
|
||||
* already in use — runs after this middleware, and consuming the invite first
|
||||
* meant a mistyped password confirmation destroyed it. `registrationController`
|
||||
* deletes it once an account actually exists. */
|
||||
req.invite = invite;
|
||||
next();
|
||||
} catch (error) {
|
||||
|
|
|
|||
70
api/server/middleware/checkInviteUser.spec.js
Normal file
70
api/server/middleware/checkInviteUser.spec.js
Normal file
|
|
@ -0,0 +1,70 @@
|
|||
const mockGetInvite = jest.fn();
|
||||
const mockDeleteTokens = jest.fn();
|
||||
|
||||
jest.mock('@librechat/api', () => ({
|
||||
getInvite: (...args) => mockGetInvite(...args),
|
||||
}));
|
||||
|
||||
jest.mock('~/models', () => ({
|
||||
createToken: jest.fn(),
|
||||
findToken: jest.fn(),
|
||||
deleteTokens: (...args) => mockDeleteTokens(...args),
|
||||
}));
|
||||
|
||||
const checkInviteUser = require('./checkInviteUser');
|
||||
|
||||
const buildRes = () => {
|
||||
const res = {};
|
||||
res.status = jest.fn(() => res);
|
||||
res.json = jest.fn(() => res);
|
||||
return res;
|
||||
};
|
||||
|
||||
describe('checkInviteUser', () => {
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
it('does not consume the invite, so a later failure cannot destroy it', async () => {
|
||||
/** The reported bug: the invite was deleted here, before the schema, the
|
||||
* allowed-domain check or the email-in-use check had run. A mistyped password
|
||||
* confirmation left the invitee with no invite and no account. */
|
||||
mockGetInvite.mockResolvedValue({ token: 'hashed-invite', email: 'invitee@example.com' });
|
||||
const req = { body: { token: 'raw-token', email: 'invitee@example.com' } };
|
||||
const next = jest.fn();
|
||||
|
||||
await checkInviteUser(req, buildRes(), next);
|
||||
|
||||
expect(mockDeleteTokens).not.toHaveBeenCalled();
|
||||
expect(next).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('hands the validated invite to the rest of the chain', async () => {
|
||||
const invite = { token: 'hashed-invite', email: 'invitee@example.com' };
|
||||
mockGetInvite.mockResolvedValue(invite);
|
||||
const req = { body: { token: 'raw-token', email: 'invitee@example.com' } };
|
||||
|
||||
await checkInviteUser(req, buildRes(), jest.fn());
|
||||
|
||||
expect(req.invite).toBe(invite);
|
||||
});
|
||||
|
||||
it('rejects an invalid invite without touching the token store', async () => {
|
||||
mockGetInvite.mockResolvedValue(null);
|
||||
const res = buildRes();
|
||||
|
||||
await checkInviteUser({ body: { token: 'raw-token', email: 'a@b.c' } }, res, jest.fn());
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(400);
|
||||
expect(mockDeleteTokens).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('passes straight through when no invite token is supplied', async () => {
|
||||
const next = jest.fn();
|
||||
|
||||
await checkInviteUser({ body: {} }, buildRes(), next);
|
||||
|
||||
expect(next).toHaveBeenCalledTimes(1);
|
||||
expect(mockGetInvite).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -443,7 +443,10 @@ const registerUser = async (user, additionalData = {}) => {
|
|||
await updateUser(newUserId, { emailVerified: true });
|
||||
}
|
||||
|
||||
return { status: 200, message: genericVerificationMessage };
|
||||
/** `userCreated` separates this from the identical 200 returned when the email is
|
||||
* already in use, so a caller can act on an account having actually been created
|
||||
* without the response body revealing which of the two happened. */
|
||||
return { status: 200, message: genericVerificationMessage, userCreated: true };
|
||||
} catch (err) {
|
||||
logger.error('[registerUser] Error in registering user:', err);
|
||||
if (newUserId) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue