Skip to content

Revert "Add Redis-backed per-email login lockout after 5 failures" #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yash-pouranik merged 1 commit into from
May 9, 2026
Merged

Revert "Add Redis-backed per-email login lockout after 5 failures" #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 1 addition & 63 deletions apps/public-api/src/__tests__/userAuth.email.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ jest.mock('@urbackend/common', () => {
getRefreshSession: jest.fn(),
persistRefreshSession: jest.fn().mockResolvedValue(undefined),
revokeSessionChain: jest.fn().mockResolvedValue(undefined),
checkLockout: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0 }),
recordFailedAttempt: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0, attempts: 1 }),
clearLockout: jest.fn().mockResolvedValue(undefined),
};
});

Expand All @@ -102,7 +99,7 @@ jest.mock('../utils/refreshToken', () => ({

const bcrypt = require('bcryptjs');
const jwt = require('jsonwebtoken');
const { redis, authEmailQueue, __mockModel: mockModel, checkLockout, recordFailedAttempt, clearLockout } = require('@urbackend/common');
const { redis, authEmailQueue, __mockModel: mockModel } = require('@urbackend/common');
const { issueAuthTokens } = require('../utils/refreshToken');
const controller = require('../controllers/userAuth.controller');

Expand Down Expand Up @@ -241,8 +238,6 @@ describe('Email Authentication Flow', () => {
type: 'verification'
}));

expect(clearLockout).toHaveBeenCalledWith('project_1', 'new@user.com');

expect(issueAuthTokens).toHaveBeenCalled();
expect(res.status).toHaveBeenCalledWith(201);
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
Expand Down Expand Up @@ -338,9 +333,7 @@ describe('Email Authentication Flow', () => {

await controller.login(req, res);

expect(checkLockout).toHaveBeenCalledWith('project_1', 'test@user.com');
expect(bcrypt.compare).toHaveBeenCalledWith('password123', 'hashed_pw');
expect(clearLockout).toHaveBeenCalledWith('project_1', 'test@user.com');
expect(issueAuthTokens).toHaveBeenCalled();
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
token: 'signed_access_token'
Expand All @@ -363,62 +356,8 @@ describe('Email Authentication Flow', () => {

await controller.login(req, res);

expect(recordFailedAttempt).toHaveBeenCalledWith('project_1', 'test@user.com');
expect(res.status).toHaveBeenCalledWith(400);
});

test('returns 423 when account is already locked', async () => {
const req = makeReq({
body: { email: 'test@user.com', password: 'password123' }
});
const res = makeRes();

checkLockout.mockResolvedValueOnce({ locked: true, retryAfterSeconds: 900 });

await controller.login(req, res);

expect(bcrypt.compare).not.toHaveBeenCalled();
expect(recordFailedAttempt).not.toHaveBeenCalled();
expect(res.status).toHaveBeenCalledWith(423);
});

test('returns 423 when failures reach lockout threshold', async () => {
const req = makeReq({
body: { email: 'test@user.com', password: 'wrongpassword' }
});
const res = makeRes();

mockModel.findOne.mockReturnValueOnce({
select: jest.fn().mockResolvedValueOnce({
_id: 'user_123',
password: 'hashed_pw'
})
});
bcrypt.compare.mockResolvedValueOnce(false);
recordFailedAttempt.mockResolvedValueOnce({ locked: true, retryAfterSeconds: 900, attempts: 5 });

await controller.login(req, res);

expect(res.status).toHaveBeenCalledWith(423);
});

test('user-not-found branch records attempt and returns 423 when lockout is reached', async () => {
const req = makeReq({
body: { email: 'missing@user.com', password: 'wrongpassword' }
});
const res = makeRes();

checkLockout.mockResolvedValueOnce({ locked: false, retryAfterSeconds: 0 });
mockModel.findOne.mockReturnValueOnce({
select: jest.fn().mockResolvedValueOnce(null)
});
recordFailedAttempt.mockResolvedValueOnce({ locked: true, retryAfterSeconds: 900, attempts: 5 });

await controller.login(req, res);

expect(recordFailedAttempt).toHaveBeenCalledWith('project_1', 'missing@user.com');
expect(res.status).toHaveBeenCalledWith(423);
});
});

describe('requestPasswordReset', () => {
Expand Down Expand Up @@ -457,7 +396,6 @@ describe('Email Authentication Flow', () => {
{ email: 'reset@user.com' },
{ $set: { password: 'hashed_pw' } }
);
expect(clearLockout).toHaveBeenCalledWith('project_1', 'reset@user.com');
expect(redis.del).toHaveBeenCalled();
expect(res.json).toHaveBeenCalled();
});
Expand Down
3 changes: 0 additions & 3 deletions apps/public-api/src/__tests__/userAuth.refresh.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ jest.mock('@urbackend/common', () => {
getConnection: jest.fn().mockResolvedValue({}),
getCompiledModel: jest.fn(() => mockModel),
__mockModel: mockModel,
checkLockout: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0 }),
recordFailedAttempt: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0, attempts: 1 }),
clearLockout: jest.fn().mockResolvedValue(undefined),
// session manager exports
getRefreshSession: jest.fn(),
persistRefreshSession: jest.fn().mockResolvedValue(undefined),
Expand Down
3 changes: 0 additions & 3 deletions apps/public-api/src/__tests__/userAuth.social.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ jest.mock('@urbackend/common', () => {
sanitize: jest.fn((value) => value),
getConnection: jest.fn().mockResolvedValue({}),
getCompiledModel: jest.fn(() => mockUsersModel),
checkLockout: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0 }),
recordFailedAttempt: jest.fn().mockResolvedValue({ locked: false, retryAfterSeconds: 0, attempts: 1 }),
clearLockout: jest.fn().mockResolvedValue(undefined),
decrypt: jest.fn((encrypted) => {
if (!encrypted?.encrypted) return null;
if (encrypted.encrypted === 'github') return 'github_secret';
Expand Down
76 changes: 3 additions & 73 deletions apps/public-api/src/controllers/userAuth.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ const crypto = require('crypto');
const {redis} = require('@urbackend/common');
const {Project} = require('@urbackend/common');
const { authEmailQueue } = require('@urbackend/common');
const { checkLockout, recordFailedAttempt, clearLockout } = require('@urbackend/common');
const { AppError } = require('@urbackend/common');
const { getRefreshSession, persistRefreshSession, revokeSessionChain } = require('@urbackend/common');
const { loginSchema, userSignupSchema, resetPasswordSchema, onlyEmailSchema, verifyOtpSchema, changePasswordSchema, sanitize } = require('@urbackend/common');
const { getConnection } = require('@urbackend/common');
Expand Down Expand Up @@ -1006,13 +1004,6 @@ module.exports.signup = async (req, res) => {
// Model.create handles validation and default values
const result = await Model.create(newUserPayload);

try {
// Fail-open: if Redis is unavailable, do not block successful signup.
await clearLockout(String(project._id), normalizedEmail);
} catch (lockErr) {
console.error('[login-lockout] clearLockout failed after signup:', lockErr?.message || lockErr);
}

await redis.set(`project:${project._id}:otp:verification:${normalizedEmail}`, otp, 'EX', 300);
await setPublicOtpCooldown(project._id, normalizedEmail, 'verification');

Expand Down Expand Up @@ -1055,31 +1046,11 @@ module.exports.signup = async (req, res) => {
* Issues access and refresh tokens upon successful authentication.
* @route POST /api/userAuth/login
*/
module.exports.login = async (req, res, next) => {
const sendAuthError = (statusCode, message) => {
if (typeof next === 'function') {
return next(new AppError(statusCode, message));
}
return res.status(statusCode).json({ error: message });
};

module.exports.login = async (req, res) => {
try {
const project = req.project;
const { email, password } = loginSchema.parse(req.body);
const normalizedEmail = email.toLowerCase().trim();
const projectId = String(project._id);

let lockStatus = { locked: false, retryAfterSeconds: 0 };
try {
// Fail-open: if Redis is unavailable, do not block login attempts.
lockStatus = await checkLockout(projectId, normalizedEmail);
} catch (lockErr) {
console.error('[login-lockout] checkLockout failed:', lockErr?.message || lockErr);
}

if (lockStatus.locked) {
return sendAuthError(423, `Account temporarily locked. Try again in ${lockStatus.retryAfterSeconds} seconds.`);
}

const usersColConfig = project.collections.find(c => c.name === 'users');
if (!usersColConfig) return res.status(404).json({ error: "Auth collection not found" });
Expand All @@ -1089,43 +1060,10 @@ module.exports.login = async (req, res, next) => {

const user = await Model.findOne({ email: normalizedEmail }).select('+password');

if (!user) {
let failedStatus = { locked: false, retryAfterSeconds: 0, attempts: 0 };
try {
// Fail-open: if Redis is unavailable, do not block login on attempt tracking.
failedStatus = await recordFailedAttempt(projectId, normalizedEmail);
} catch (attemptErr) {
console.error('[login-lockout] recordFailedAttempt failed (user missing):', attemptErr?.message || attemptErr);
}

if (failedStatus.locked) {
return sendAuthError(423, `Account temporarily locked. Try again in ${failedStatus.retryAfterSeconds} seconds.`);
}
return sendAuthError(400, 'Invalid email or password');
}
if (!user) return res.status(400).json({ error: "Invalid email or password" });

const validPass = await bcrypt.compare(password, user.password);
if (!validPass) {
let failedStatus = { locked: false, retryAfterSeconds: 0, attempts: 0 };
try {
// Fail-open: if Redis is unavailable, do not block login on attempt tracking.
failedStatus = await recordFailedAttempt(projectId, normalizedEmail);
} catch (attemptErr) {
console.error('[login-lockout] recordFailedAttempt failed (invalid password):', attemptErr?.message || attemptErr);
}

if (failedStatus.locked) {
return sendAuthError(423, `Account temporarily locked. Try again in ${failedStatus.retryAfterSeconds} seconds.`);
}
return sendAuthError(400, 'Invalid email or password');
}

try {
// Fail-open: if Redis is unavailable, do not block successful login.
await clearLockout(projectId, normalizedEmail);
} catch (clearErr) {
console.error('[login-lockout] clearLockout failed:', clearErr?.message || clearErr);
}
if (!validPass) return res.status(400).json({ error: "Invalid email or password" });

Comment on lines 1049 to 1067
const issuedTokens = await issueAuthTokens({
project,
Expand Down Expand Up @@ -1420,7 +1358,6 @@ module.exports.resetPasswordUser = async (req, res) => {
try {
const project = req.project;
const { email, otp, newPassword } = resetPasswordSchema.parse(req.body);
const normalizedEmail = email.toLowerCase().trim();

const redisKey = `project:${project._id}:otp:reset:${email}`;
const storedOtp = await redis.get(redisKey);
Expand All @@ -1442,13 +1379,6 @@ module.exports.resetPasswordUser = async (req, res) => {

if (result.matchedCount === 0) return res.status(404).json({ error: "User not found" });

try {
// Fail-open: if Redis is unavailable, do not block password recovery success.
await clearLockout(String(project._id), normalizedEmail);
} catch (lockErr) {
console.error('[login-lockout] clearLockout failed after password reset:', lockErr?.message || lockErr);
}

await redis.del(redisKey);
res.json({ message: "Password updated successfully" });

Expand Down
4 changes: 0 additions & 4 deletions packages/common/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ const { validateData, validateUpdateData } = require("./utils/validateData");
const sessionManager = require("./utils/session.manager");
const planLimits = require("./utils/planLimits");
const AppError = require("./utils/AppError");
Comment on lines 94 to 96
const { checkLockout, recordFailedAttempt, clearLockout } = require("./utils/loginLockout");

module.exports = {
connectDB,
Expand Down Expand Up @@ -180,7 +179,4 @@ module.exports = {
getPresignedUploadUrl,
verifyUploadedFile,
ApiAnalytics,
checkLockout,
recordFailedAttempt,
clearLockout,
};
113 changes: 0 additions & 113 deletions packages/common/src/utils/loginLockout.js
View file
Open in desktop

This file was deleted.

Loading