From 41d72d5dc6936962b7575f251f49575cd12406a0 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Wed, 22 May 2024 21:55:56 +0800 Subject: [PATCH 1/9] feat: added user-locking on mfa failure --- ...0522072006_add-user-account-mfa-locking.ts | 43 +++++++++++++++ backend/src/db/schemas/users.ts | 10 +++- backend/src/server/routes/v1/user-router.ts | 30 ++++++++++- .../services/auth-token/auth-token-service.ts | 8 ++- .../services/auth-token/auth-token-types.ts | 3 +- backend/src/services/auth/auth-fns.ts | 21 ++++++++ .../src/services/auth/auth-login-service.ts | 51 +++++++++++++++--- backend/src/services/smtp/smtp-service.ts | 1 + .../smtp/templates/unlockAccount.handlebars | 16 ++++++ backend/src/services/user/user-fns.ts | 53 +++++++++++++++++++ backend/src/services/user/user-service.ts | 16 +++++- 11 files changed, 240 insertions(+), 12 deletions(-) create mode 100644 backend/src/db/migrations/20240522072006_add-user-account-mfa-locking.ts create mode 100644 backend/src/services/smtp/templates/unlockAccount.handlebars diff --git a/backend/src/db/migrations/20240522072006_add-user-account-mfa-locking.ts b/backend/src/db/migrations/20240522072006_add-user-account-mfa-locking.ts new file mode 100644 index 000000000..2b2ecd783 --- /dev/null +++ b/backend/src/db/migrations/20240522072006_add-user-account-mfa-locking.ts @@ -0,0 +1,43 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + const hasConsecutiveFailedMfaAttempts = await knex.schema.hasColumn(TableName.Users, "consecutiveFailedMfaAttempts"); + const hasIsLocked = await knex.schema.hasColumn(TableName.Users, "isLocked"); + const hasTemporaryLockDateEnd = await knex.schema.hasColumn(TableName.Users, "temporaryLockDateEnd"); + + await knex.schema.alterTable(TableName.Users, (t) => { + if (!hasConsecutiveFailedMfaAttempts) { + t.integer("consecutiveFailedMfaAttempts").defaultTo(0); + } + + if (!hasIsLocked) { + t.boolean("isLocked").defaultTo(false); + } + + if (!hasTemporaryLockDateEnd) { + t.dateTime("temporaryLockDateEnd").nullable(); + } + }); +} + +export async function down(knex: Knex): Promise { + const hasConsecutiveFailedMfaAttempts = await knex.schema.hasColumn(TableName.Users, "consecutiveFailedMfaAttempts"); + const hasIsLocked = await knex.schema.hasColumn(TableName.Users, "isLocked"); + const hasTemporaryLockDateEnd = await knex.schema.hasColumn(TableName.Users, "temporaryLockDateEnd"); + + await knex.schema.alterTable(TableName.Users, (t) => { + if (hasConsecutiveFailedMfaAttempts) { + t.dropColumn("consecutiveFailedMfaAttempts"); + } + + if (hasIsLocked) { + t.dropColumn("isLocked"); + } + + if (hasTemporaryLockDateEnd) { + t.dropColumn("temporaryLockDateEnd"); + } + }); +} diff --git a/backend/src/db/schemas/users.ts b/backend/src/db/schemas/users.ts index d5a4d5b49..935351d33 100644 --- a/backend/src/db/schemas/users.ts +++ b/backend/src/db/schemas/users.ts @@ -22,9 +22,15 @@ export const UsersSchema = z.object({ updatedAt: z.date(), isGhost: z.boolean().default(false), username: z.string(), - isEmailVerified: z.boolean().default(false).nullable().optional() + isEmailVerified: z.boolean().default(false).nullable().optional(), + consecutiveFailedMfaAttempts: z.number(), + isLocked: z.boolean(), + temporaryLockDateEnd: z.date().nullable().optional() }); export type TUsers = z.infer; -export type TUsersInsert = Omit, TImmutableDBKeys>; +export type TUsersInsert = Omit< + z.input, + TImmutableDBKeys | "isLocked" | "consecutiveFailedMfaAttempts" +>; export type TUsersUpdate = Partial, TImmutableDBKeys>>; diff --git a/backend/src/server/routes/v1/user-router.ts b/backend/src/server/routes/v1/user-router.ts index bdede8a3a..aa3962c5a 100644 --- a/backend/src/server/routes/v1/user-router.ts +++ b/backend/src/server/routes/v1/user-router.ts @@ -1,11 +1,15 @@ import { z } from "zod"; import { UserEncryptionKeysSchema, UsersSchema } from "@app/db/schemas"; -import { readLimit } from "@app/server/config/rateLimiter"; +import { getConfig } from "@app/lib/config/env"; +import { logger } from "@app/lib/logger"; +import { authRateLimit, readLimit } from "@app/server/config/rateLimiter"; import { verifyAuth } from "@app/server/plugins/auth/verify-auth"; import { AuthMode } from "@app/services/auth/auth-type"; export const registerUserRouter = async (server: FastifyZodProvider) => { + const appCfg = getConfig(); + server.route({ method: "GET", url: "/", @@ -25,4 +29,28 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { return { user }; } }); + + server.route({ + method: "GET", + url: "/:userId/unlock-verify", + config: { + rateLimit: authRateLimit + }, + schema: { + querystring: z.object({ + token: z.string().trim() + }), + params: z.object({ + userId: z.string() + }) + }, + handler: async (req, res) => { + try { + await server.services.user.unlockUser(req.params.userId, req.query.token); + } catch (err) { + logger.error(`User unlock failed for ${req.params.userId}`); + } + return res.redirect(`${appCfg.SITE_URL}/login`); + } + }); }; diff --git a/backend/src/services/auth-token/auth-token-service.ts b/backend/src/services/auth-token/auth-token-service.ts index 5d68a4e94..b1f8aa2f6 100644 --- a/backend/src/services/auth-token/auth-token-service.ts +++ b/backend/src/services/auth-token/auth-token-service.ts @@ -13,8 +13,9 @@ import { TCreateTokenForUserDTO, TIssueAuthTokenDTO, TokenType, TValidateTokenFo type TAuthTokenServiceFactoryDep = { tokenDAL: TTokenDALFactory; - userDAL: Pick; + userDAL: Pick; }; + export type TAuthTokenServiceFactory = ReturnType; export const getTokenConfig = (tokenType: TokenType) => { @@ -53,6 +54,11 @@ export const getTokenConfig = (tokenType: TokenType) => { const expiresAt = new Date(new Date().getTime() + 86400000); return { token, expiresAt }; } + case TokenType.TOKEN_USER_UNLOCK: { + const token = crypto.randomBytes(16).toString("hex"); + const expiresAt = new Date(new Date().getTime() + 259200000); + return { token, expiresAt }; + } default: { const token = crypto.randomBytes(16).toString("hex"); const expiresAt = new Date(); diff --git a/backend/src/services/auth-token/auth-token-types.ts b/backend/src/services/auth-token/auth-token-types.ts index 630e36310..8917bd672 100644 --- a/backend/src/services/auth-token/auth-token-types.ts +++ b/backend/src/services/auth-token/auth-token-types.ts @@ -3,7 +3,8 @@ export enum TokenType { TOKEN_EMAIL_VERIFICATION = "emailVerification", // unverified -> verified TOKEN_EMAIL_MFA = "emailMfa", TOKEN_EMAIL_ORG_INVITATION = "organizationInvitation", - TOKEN_EMAIL_PASSWORD_RESET = "passwordReset" + TOKEN_EMAIL_PASSWORD_RESET = "passwordReset", + TOKEN_USER_UNLOCK = "userUnlock" } export type TCreateTokenForUserDTO = { diff --git a/backend/src/services/auth/auth-fns.ts b/backend/src/services/auth/auth-fns.ts index 80fb0b325..4911cc258 100644 --- a/backend/src/services/auth/auth-fns.ts +++ b/backend/src/services/auth/auth-fns.ts @@ -44,3 +44,24 @@ export const validateSignUpAuthorization = (token: string, userId: string, valid if (decodedToken.authTokenType !== AuthTokenType.SIGNUP_TOKEN) throw new UnauthorizedError(); if (decodedToken.userId !== userId) throw new UnauthorizedError(); }; + +export const enforceUserLockStatus = (isLocked: boolean, temporaryLockDateEnd?: Date | null) => { + if (isLocked) { + throw new UnauthorizedError({ + name: "User Locked", + message: + "User is locked due to multiple failed login attempts. An email has been sent to you in order to unlock your account." + }); + } + + if (temporaryLockDateEnd) { + const timeDiff = new Date().getTime() - temporaryLockDateEnd.getTime(); + if (timeDiff < 0) + throw new UnauthorizedError({ + name: "User Locked", + message: `User is locked due to multiple failed login attempts. Try logging in again after ${Math.round( + (-1 * timeDiff) / 1000 + )} seconds.` + }); + } +}; diff --git a/backend/src/services/auth/auth-login-service.ts b/backend/src/services/auth/auth-login-service.ts index 4d2a302c6..96c32c0df 100644 --- a/backend/src/services/auth/auth-login-service.ts +++ b/backend/src/services/auth/auth-login-service.ts @@ -13,7 +13,8 @@ import { TokenType } from "../auth-token/auth-token-types"; import { TOrgDALFactory } from "../org/org-dal"; import { SmtpTemplates, TSmtpService } from "../smtp/smtp-service"; import { TUserDALFactory } from "../user/user-dal"; -import { validateProviderAuthToken } from "./auth-fns"; +import { processFailedMfaAttempt } from "../user/user-fns"; +import { enforceUserLockStatus, validateProviderAuthToken } from "./auth-fns"; import { TLoginClientProofDTO, TLoginGenServerPublicKeyDTO, @@ -212,6 +213,9 @@ export const authLoginServiceFactory = ({ }); // send multi factor auth token if they it enabled if (userEnc.isMfaEnabled && userEnc.email) { + const user = await userDAL.findById(userEnc.userId); + enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); + const mfaToken = jwt.sign( { authMethod, @@ -300,6 +304,7 @@ export const authLoginServiceFactory = ({ const resendMfaToken = async (userId: string) => { const user = await userDAL.findById(userId); if (!user || !user.email) return; + enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); await sendUserMfaCode({ userId: user.id, email: user.email @@ -311,17 +316,51 @@ export const authLoginServiceFactory = ({ * Third step of login in which user completes with mfa * */ const verifyMfaToken = async ({ userId, mfaToken, mfaJwtToken, ip, userAgent, orgId }: TVerifyMfaTokenDTO) => { - await tokenService.validateTokenForUser({ - type: TokenType.TOKEN_EMAIL_MFA, - userId, - code: mfaToken - }); + const appCfg = getConfig(); + const user = await userDAL.findById(userId); + enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); + + try { + await tokenService.validateTokenForUser({ + type: TokenType.TOKEN_EMAIL_MFA, + userId, + code: mfaToken + }); + } catch (err) { + const updatedUser = await processFailedMfaAttempt(userId, userDAL); + if (updatedUser.isLocked) { + if (updatedUser.email) { + const unlockToken = await tokenService.createTokenForUser({ + type: TokenType.TOKEN_USER_UNLOCK, + userId: updatedUser.id + }); + + await smtpService.sendMail({ + template: SmtpTemplates.UnlockAccount, + subjectLine: "Unlock your Infisical account", + recipients: [updatedUser.email], + substitutions: { + token: unlockToken, + callback_url: `${appCfg.SITE_URL}/api/v1/user/${updatedUser.id}/unlock-verify` + } + }); + } + } + + throw err; + } const decodedToken = jwt.verify(mfaJwtToken, getConfig().AUTH_SECRET) as AuthModeMfaJwtTokenPayload; const userEnc = await userDAL.findUserEncKeyByUserId(userId); if (!userEnc) throw new Error("Failed to authenticate user"); + // reset lock states + await userDAL.updateById(userId, { + consecutiveFailedMfaAttempts: 0, + temporaryLockDateEnd: null + }); + const token = await generateUserTokens({ user: { ...userEnc, diff --git a/backend/src/services/smtp/smtp-service.ts b/backend/src/services/smtp/smtp-service.ts index 81680537d..7d6b98b31 100644 --- a/backend/src/services/smtp/smtp-service.ts +++ b/backend/src/services/smtp/smtp-service.ts @@ -21,6 +21,7 @@ export enum SmtpTemplates { EmailVerification = "emailVerification.handlebars", SecretReminder = "secretReminder.handlebars", EmailMfa = "emailMfa.handlebars", + UnlockAccount = "unlockAccount.handlebars", AccessApprovalRequest = "accessApprovalRequest.handlebars", HistoricalSecretList = "historicalSecretLeakIncident.handlebars", NewDeviceJoin = "newDevice.handlebars", diff --git a/backend/src/services/smtp/templates/unlockAccount.handlebars b/backend/src/services/smtp/templates/unlockAccount.handlebars new file mode 100644 index 000000000..cb1859e51 --- /dev/null +++ b/backend/src/services/smtp/templates/unlockAccount.handlebars @@ -0,0 +1,16 @@ + + + + + + Your Infisical account has been locked + + + +

Unlock your Infisical account

+

Your account has been temporarily locked due to multiple failed login attempts. + Unlock your account now +

If these attempts were not made by you, reset your password immediately.

+ + + \ No newline at end of file diff --git a/backend/src/services/user/user-fns.ts b/backend/src/services/user/user-fns.ts index 639320e24..2750bb87e 100644 --- a/backend/src/services/user/user-fns.ts +++ b/backend/src/services/user/user-fns.ts @@ -1,5 +1,7 @@ import slugify from "@sindresorhus/slugify"; +import { TableName } from "@app/db/schemas"; +import { DatabaseError } from "@app/lib/errors"; import { alphaNumericNanoId } from "@app/lib/nanoid"; import { TUserDALFactory } from "@app/services/user/user-dal"; @@ -19,3 +21,54 @@ export const normalizeUsername = async (username: string, userDAL: Pick) => { + try { + const updatedUser = await userDAL.transaction(async (tx) => { + const PROGRESSIVE_DELAY_INTERVAL = 3; + const [user] = await tx(TableName.Users) + .where("id", userId) + .increment("consecutiveFailedMfaAttempts", 1) + .returning("*"); + + if (!user) { + throw new Error("User not found"); + } + + const progressiveDelaysInMins = [5, 30, 60]; + + // lock user when failed attempt exceeds threshold + if (user.consecutiveFailedMfaAttempts > PROGRESSIVE_DELAY_INTERVAL * progressiveDelaysInMins.length) { + return ( + await tx(TableName.Users) + .where("id", userId) + .update({ + isLocked: true, + temporaryLockDateEnd: null + }) + .returning("*") + )[0]; + } + + // delay user only when failed MFA attempts is a multiple of configured delay interval + if (user.consecutiveFailedMfaAttempts % PROGRESSIVE_DELAY_INTERVAL === 0) { + const delayIndex = user.consecutiveFailedMfaAttempts / PROGRESSIVE_DELAY_INTERVAL - 1; + + return ( + await tx(TableName.Users) + .where("id", userId) + .update({ + temporaryLockDateEnd: new Date(new Date().getTime() + progressiveDelaysInMins[delayIndex] * 60 * 1000) + }) + .returning("*") + )[0]; + } + + return user; + }); + + return updatedUser; + } catch (error) { + throw new DatabaseError({ error, name: "Process failed MFA Attempt" }); + } +}; diff --git a/backend/src/services/user/user-service.ts b/backend/src/services/user/user-service.ts index 089f3b8c6..a82259db6 100644 --- a/backend/src/services/user/user-service.ts +++ b/backend/src/services/user/user-service.ts @@ -207,6 +207,19 @@ export const userServiceFactory = ({ return userAction; }; + const unlockUser = async (userId: string, token: string) => { + await tokenService.validateTokenForUser({ + userId, + code: token, + type: TokenType.TOKEN_USER_UNLOCK + }); + + await userDAL.update( + { id: userId }, + { consecutiveFailedMfaAttempts: 0, isLocked: false, temporaryLockDateEnd: null } + ); + }; + return { sendEmailVerificationCode, verifyEmailVerificationCode, @@ -216,6 +229,7 @@ export const userServiceFactory = ({ deleteMe, getMe, createUserAction, - getUserAction + getUserAction, + unlockUser }; }; From e3dae9d498218f11a3afb05b0324344fe3050aab Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Wed, 22 May 2024 22:51:20 +0800 Subject: [PATCH 2/9] feat: integration user lock flow to frontend --- backend/src/server/routes/v1/user-router.ts | 1 + backend/src/services/auth/auth-fns.ts | 2 +- backend/src/services/user/user-fns.ts | 2 +- .../components/InitialStep/InitialStep.tsx | 12 +++++- .../Login/components/MFAStep/MFAStep.tsx | 42 +++++++++---------- 5 files changed, 34 insertions(+), 25 deletions(-) diff --git a/backend/src/server/routes/v1/user-router.ts b/backend/src/server/routes/v1/user-router.ts index aa3962c5a..0253ac86b 100644 --- a/backend/src/server/routes/v1/user-router.ts +++ b/backend/src/server/routes/v1/user-router.ts @@ -49,6 +49,7 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { await server.services.user.unlockUser(req.params.userId, req.query.token); } catch (err) { logger.error(`User unlock failed for ${req.params.userId}`); + logger.error(err); } return res.redirect(`${appCfg.SITE_URL}/login`); } diff --git a/backend/src/services/auth/auth-fns.ts b/backend/src/services/auth/auth-fns.ts index 4911cc258..036d9068e 100644 --- a/backend/src/services/auth/auth-fns.ts +++ b/backend/src/services/auth/auth-fns.ts @@ -59,7 +59,7 @@ export const enforceUserLockStatus = (isLocked: boolean, temporaryLockDateEnd?: if (timeDiff < 0) throw new UnauthorizedError({ name: "User Locked", - message: `User is locked due to multiple failed login attempts. Try logging in again after ${Math.round( + message: `User is locked due to multiple failed login attempts. Try again after ${Math.round( (-1 * timeDiff) / 1000 )} seconds.` }); diff --git a/backend/src/services/user/user-fns.ts b/backend/src/services/user/user-fns.ts index 2750bb87e..e8a0905f8 100644 --- a/backend/src/services/user/user-fns.ts +++ b/backend/src/services/user/user-fns.ts @@ -38,7 +38,7 @@ export const processFailedMfaAttempt = async (userId: string, userDAL: Pick PROGRESSIVE_DELAY_INTERVAL * progressiveDelaysInMins.length) { + if (user.consecutiveFailedMfaAttempts >= PROGRESSIVE_DELAY_INTERVAL * (progressiveDelaysInMins.length + 1)) { return ( await tx(TableName.Users) .where("id", userId) diff --git a/frontend/src/views/Login/components/InitialStep/InitialStep.tsx b/frontend/src/views/Login/components/InitialStep/InitialStep.tsx index f3b40300e..a4e5e89f5 100644 --- a/frontend/src/views/Login/components/InitialStep/InitialStep.tsx +++ b/frontend/src/views/Login/components/InitialStep/InitialStep.tsx @@ -105,8 +105,18 @@ export const InitialStep = ({ setStep, email, setEmail, password, setPassword }: }); } } - } catch (err) { + } catch (err: any) { console.error(err); + if (err.response.data.error === "User Locked") { + createNotification({ + title: err.response.data.error, + text: err.response.data.message, + type: "error" + }); + setIsLoading(false); + return; + } + setLoginError(true); createNotification({ text: "Login unsuccessful. Double-check your credentials and try again.", diff --git a/frontend/src/views/Login/components/MFAStep/MFAStep.tsx b/frontend/src/views/Login/components/MFAStep/MFAStep.tsx index eaaac88a9..249c043b6 100644 --- a/frontend/src/views/Login/components/MFAStep/MFAStep.tsx +++ b/frontend/src/views/Login/components/MFAStep/MFAStep.tsx @@ -46,20 +46,7 @@ type Props = { callbackPort?: string | null; }; -interface VerifyMfaTokenError { - response: { - data: { - context: { - code: string; - triesLeft: number; - }; - }; - status: number; - }; -} - export const MFAStep = ({ email, password, providerAuthToken }: Props) => { - const router = useRouter(); const [isLoading, setIsLoading] = useState(false); const [isLoadingResend, setIsLoadingResend] = useState(false); @@ -178,20 +165,31 @@ export const MFAStep = ({ email, password, providerAuthToken }: Props) => { }); } } - } catch (err) { - const error = err as VerifyMfaTokenError; + } catch (err: any) { + if (err.response.data.error === "User Locked") { + createNotification({ + title: err.response.data.error, + text: err.response.data.message, + type: "error" + }); + setIsLoading(false); + return; + } + createNotification({ text: "Failed to log in", type: "error" }); - if (error?.response?.status === 500) { - window.location.reload(); - } else if (error?.response?.data?.context?.triesLeft) { - setTriesLeft(error?.response?.data?.context?.triesLeft); - if (error.response.data.context.triesLeft === 0) { - window.location.reload(); - } + if (triesLeft) { + setTriesLeft((left) => { + if (triesLeft === 1) { + window.location.reload(); + } + return (left as number) - 1; + }); + } else { + setTriesLeft(2); } setIsLoading(false); From 8ec8b1ce2f496c45fb74c77a04089a3e8781f245 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Wed, 22 May 2024 23:51:58 +0800 Subject: [PATCH 3/9] feat: add custom rate limiting for mfa --- backend/src/server/config/rateLimiter.ts | 9 +++++++++ backend/src/server/routes/v2/mfa-router.ts | 6 +++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/backend/src/server/config/rateLimiter.ts b/backend/src/server/config/rateLimiter.ts index 6c92de62c..0f6903773 100644 --- a/backend/src/server/config/rateLimiter.ts +++ b/backend/src/server/config/rateLimiter.ts @@ -52,6 +52,15 @@ export const inviteUserRateLimit: RateLimitOptions = { keyGenerator: (req) => req.realIp }; +export const mfaRateLimit: RateLimitOptions = { + timeWindow: 60 * 1000, + max: 20, + keyGenerator: (req) => { + // fallback to IP to avoid global rate limiting when authorization is set to empty + return req.headers.authorization?.split(" ")[1] || req.realIp; + } +}; + export const creationLimit: RateLimitOptions = { // identity, project, org timeWindow: 60 * 1000, diff --git a/backend/src/server/routes/v2/mfa-router.ts b/backend/src/server/routes/v2/mfa-router.ts index 973804c7c..1c685866d 100644 --- a/backend/src/server/routes/v2/mfa-router.ts +++ b/backend/src/server/routes/v2/mfa-router.ts @@ -2,7 +2,7 @@ import jwt from "jsonwebtoken"; import { z } from "zod"; import { getConfig } from "@app/lib/config/env"; -import { writeLimit } from "@app/server/config/rateLimiter"; +import { mfaRateLimit } from "@app/server/config/rateLimiter"; import { AuthModeMfaJwtTokenPayload, AuthTokenType } from "@app/services/auth/auth-type"; export const registerMfaRouter = async (server: FastifyZodProvider) => { @@ -34,7 +34,7 @@ export const registerMfaRouter = async (server: FastifyZodProvider) => { method: "POST", url: "/mfa/send", config: { - rateLimit: writeLimit + rateLimit: mfaRateLimit }, schema: { response: { @@ -53,7 +53,7 @@ export const registerMfaRouter = async (server: FastifyZodProvider) => { url: "/mfa/verify", method: "POST", config: { - rateLimit: writeLimit + rateLimit: mfaRateLimit }, schema: { body: z.object({ From a0d9331e67df9e1cb64076327a16087f2ea0e672 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Thu, 23 May 2024 00:21:19 +0800 Subject: [PATCH 4/9] misc: removed comment --- backend/src/server/config/rateLimiter.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/src/server/config/rateLimiter.ts b/backend/src/server/config/rateLimiter.ts index 0f6903773..dfecfb495 100644 --- a/backend/src/server/config/rateLimiter.ts +++ b/backend/src/server/config/rateLimiter.ts @@ -56,7 +56,6 @@ export const mfaRateLimit: RateLimitOptions = { timeWindow: 60 * 1000, max: 20, keyGenerator: (req) => { - // fallback to IP to avoid global rate limiting when authorization is set to empty return req.headers.authorization?.split(" ")[1] || req.realIp; } }; From c0daa11aeb310acf33093d03817af83ea49d05b2 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Fri, 24 May 2024 23:45:16 +0800 Subject: [PATCH 5/9] misc: addressed PR comments --- backend/src/db/schemas/users.ts | 9 +-- backend/src/server/routes/v1/user-router.ts | 2 +- backend/src/services/auth/auth-fns.ts | 2 +- .../src/services/auth/auth-login-service.ts | 61 ++++++++++++++++--- .../services/auth/auth-password-service.ts | 5 ++ backend/src/services/user/user-dal.ts | 16 ++++- backend/src/services/user/user-fns.ts | 53 ---------------- 7 files changed, 79 insertions(+), 69 deletions(-) diff --git a/backend/src/db/schemas/users.ts b/backend/src/db/schemas/users.ts index 935351d33..c10af4ba4 100644 --- a/backend/src/db/schemas/users.ts +++ b/backend/src/db/schemas/users.ts @@ -23,14 +23,11 @@ export const UsersSchema = z.object({ isGhost: z.boolean().default(false), username: z.string(), isEmailVerified: z.boolean().default(false).nullable().optional(), - consecutiveFailedMfaAttempts: z.number(), - isLocked: z.boolean(), + consecutiveFailedMfaAttempts: z.number().optional(), + isLocked: z.boolean().optional(), temporaryLockDateEnd: z.date().nullable().optional() }); export type TUsers = z.infer; -export type TUsersInsert = Omit< - z.input, - TImmutableDBKeys | "isLocked" | "consecutiveFailedMfaAttempts" ->; +export type TUsersInsert = Omit, TImmutableDBKeys>; export type TUsersUpdate = Partial, TImmutableDBKeys>>; diff --git a/backend/src/server/routes/v1/user-router.ts b/backend/src/server/routes/v1/user-router.ts index 0253ac86b..3d9f531b9 100644 --- a/backend/src/server/routes/v1/user-router.ts +++ b/backend/src/server/routes/v1/user-router.ts @@ -32,7 +32,7 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { server.route({ method: "GET", - url: "/:userId/unlock-verify", + url: "/:userId/unlock", config: { rateLimit: authRateLimit }, diff --git a/backend/src/services/auth/auth-fns.ts b/backend/src/services/auth/auth-fns.ts index 036d9068e..30297e605 100644 --- a/backend/src/services/auth/auth-fns.ts +++ b/backend/src/services/auth/auth-fns.ts @@ -50,7 +50,7 @@ export const enforceUserLockStatus = (isLocked: boolean, temporaryLockDateEnd?: throw new UnauthorizedError({ name: "User Locked", message: - "User is locked due to multiple failed login attempts. An email has been sent to you in order to unlock your account." + "User is locked due to multiple failed login attempts. An email has been sent to you in order to unlock your account. You can also reset your password to unlock." }); } diff --git a/backend/src/services/auth/auth-login-service.ts b/backend/src/services/auth/auth-login-service.ts index 96c32c0df..a97e94145 100644 --- a/backend/src/services/auth/auth-login-service.ts +++ b/backend/src/services/auth/auth-login-service.ts @@ -4,7 +4,7 @@ import { TUsers, UserDeviceSchema } from "@app/db/schemas"; import { isAuthMethodSaml } from "@app/ee/services/permission/permission-fns"; import { getConfig } from "@app/lib/config/env"; import { generateSrpServerKey, srpCheckClientProof } from "@app/lib/crypto"; -import { BadRequestError, UnauthorizedError } from "@app/lib/errors"; +import { BadRequestError, DatabaseError, UnauthorizedError } from "@app/lib/errors"; import { getServerCfg } from "@app/services/super-admin/super-admin-service"; import { TTokenDALFactory } from "../auth-token/auth-token-dal"; @@ -13,7 +13,6 @@ import { TokenType } from "../auth-token/auth-token-types"; import { TOrgDALFactory } from "../org/org-dal"; import { SmtpTemplates, TSmtpService } from "../smtp/smtp-service"; import { TUserDALFactory } from "../user/user-dal"; -import { processFailedMfaAttempt } from "../user/user-fns"; import { enforceUserLockStatus, validateProviderAuthToken } from "./auth-fns"; import { TLoginClientProofDTO, @@ -214,7 +213,7 @@ export const authLoginServiceFactory = ({ // send multi factor auth token if they it enabled if (userEnc.isMfaEnabled && userEnc.email) { const user = await userDAL.findById(userEnc.userId); - enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); + enforceUserLockStatus(Boolean(user.isLocked), user.temporaryLockDateEnd); const mfaToken = jwt.sign( { @@ -304,13 +303,61 @@ export const authLoginServiceFactory = ({ const resendMfaToken = async (userId: string) => { const user = await userDAL.findById(userId); if (!user || !user.email) return; - enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); + enforceUserLockStatus(Boolean(user.isLocked), user.temporaryLockDateEnd); await sendUserMfaCode({ userId: user.id, email: user.email }); }; + const processFailedMfaAttempt = async (userId: string) => { + try { + const updatedUser = await userDAL.transaction(async (tx) => { + const PROGRESSIVE_DELAY_INTERVAL = 3; + const user = await userDAL.incrementFailedMfaAttempt(userId, tx); + + if (!user) { + throw new Error("User not found"); + } + + const progressiveDelaysInMins = [5, 30, 60]; + + // lock user when failed attempt exceeds threshold + if ( + user.consecutiveFailedMfaAttempts && + user.consecutiveFailedMfaAttempts >= PROGRESSIVE_DELAY_INTERVAL * (progressiveDelaysInMins.length + 1) + ) { + return userDAL.updateById( + userId, + { + isLocked: true, + temporaryLockDateEnd: null + }, + tx + ); + } + + // delay user only when failed MFA attempts is a multiple of configured delay interval + if (user.consecutiveFailedMfaAttempts && user.consecutiveFailedMfaAttempts % PROGRESSIVE_DELAY_INTERVAL === 0) { + const delayIndex = user.consecutiveFailedMfaAttempts / PROGRESSIVE_DELAY_INTERVAL - 1; + return userDAL.updateById( + userId, + { + temporaryLockDateEnd: new Date(new Date().getTime() + progressiveDelaysInMins[delayIndex] * 60 * 1000) + }, + tx + ); + } + + return user; + }); + + return updatedUser; + } catch (error) { + throw new DatabaseError({ error, name: "Process failed MFA Attempt" }); + } + }; + /* * Multi factor authentication verification of code * Third step of login in which user completes with mfa @@ -318,7 +365,7 @@ export const authLoginServiceFactory = ({ const verifyMfaToken = async ({ userId, mfaToken, mfaJwtToken, ip, userAgent, orgId }: TVerifyMfaTokenDTO) => { const appCfg = getConfig(); const user = await userDAL.findById(userId); - enforceUserLockStatus(user.isLocked, user.temporaryLockDateEnd); + enforceUserLockStatus(Boolean(user.isLocked), user.temporaryLockDateEnd); try { await tokenService.validateTokenForUser({ @@ -327,7 +374,7 @@ export const authLoginServiceFactory = ({ code: mfaToken }); } catch (err) { - const updatedUser = await processFailedMfaAttempt(userId, userDAL); + const updatedUser = await processFailedMfaAttempt(userId); if (updatedUser.isLocked) { if (updatedUser.email) { const unlockToken = await tokenService.createTokenForUser({ @@ -341,7 +388,7 @@ export const authLoginServiceFactory = ({ recipients: [updatedUser.email], substitutions: { token: unlockToken, - callback_url: `${appCfg.SITE_URL}/api/v1/user/${updatedUser.id}/unlock-verify` + callback_url: `${appCfg.SITE_URL}/api/v1/user/${updatedUser.id}/unlock` } }); } diff --git a/backend/src/services/auth/auth-password-service.ts b/backend/src/services/auth/auth-password-service.ts index 4025e4903..8f4e8a6d2 100644 --- a/backend/src/services/auth/auth-password-service.ts +++ b/backend/src/services/auth/auth-password-service.ts @@ -138,6 +138,11 @@ export const authPaswordServiceFactory = ({ code }); + await userDAL.updateById(user.id, { + isLocked: false, + temporaryLockDateEnd: null + }); + const token = jwt.sign( { authTokenType: AuthTokenType.SIGNUP_TOKEN, diff --git a/backend/src/services/user/user-dal.ts b/backend/src/services/user/user-dal.ts index f2da0df0e..7c73fc4f4 100644 --- a/backend/src/services/user/user-dal.ts +++ b/backend/src/services/user/user-dal.ts @@ -143,6 +143,19 @@ export const userDALFactory = (db: TDbClient) => { } }; + const incrementFailedMfaAttempt = async (userId: string, tx?: Knex) => { + try { + const [user] = await (tx || db)(TableName.Users) + .where("id", userId) + .increment("consecutiveFailedMfaAttempts", 1) + .returning("*"); + + return user; + } catch (error) { + throw new DatabaseError({ error, name: "Increment Failed MFA Attempt" }); + } + }; + return { ...userOrm, findUserByUsername, @@ -155,6 +168,7 @@ export const userDALFactory = (db: TDbClient) => { upsertUserEncryptionKey, createUserEncryption, findOneUserAction, - createUserAction + createUserAction, + incrementFailedMfaAttempt }; }; diff --git a/backend/src/services/user/user-fns.ts b/backend/src/services/user/user-fns.ts index e8a0905f8..639320e24 100644 --- a/backend/src/services/user/user-fns.ts +++ b/backend/src/services/user/user-fns.ts @@ -1,7 +1,5 @@ import slugify from "@sindresorhus/slugify"; -import { TableName } from "@app/db/schemas"; -import { DatabaseError } from "@app/lib/errors"; import { alphaNumericNanoId } from "@app/lib/nanoid"; import { TUserDALFactory } from "@app/services/user/user-dal"; @@ -21,54 +19,3 @@ export const normalizeUsername = async (username: string, userDAL: Pick) => { - try { - const updatedUser = await userDAL.transaction(async (tx) => { - const PROGRESSIVE_DELAY_INTERVAL = 3; - const [user] = await tx(TableName.Users) - .where("id", userId) - .increment("consecutiveFailedMfaAttempts", 1) - .returning("*"); - - if (!user) { - throw new Error("User not found"); - } - - const progressiveDelaysInMins = [5, 30, 60]; - - // lock user when failed attempt exceeds threshold - if (user.consecutiveFailedMfaAttempts >= PROGRESSIVE_DELAY_INTERVAL * (progressiveDelaysInMins.length + 1)) { - return ( - await tx(TableName.Users) - .where("id", userId) - .update({ - isLocked: true, - temporaryLockDateEnd: null - }) - .returning("*") - )[0]; - } - - // delay user only when failed MFA attempts is a multiple of configured delay interval - if (user.consecutiveFailedMfaAttempts % PROGRESSIVE_DELAY_INTERVAL === 0) { - const delayIndex = user.consecutiveFailedMfaAttempts / PROGRESSIVE_DELAY_INTERVAL - 1; - - return ( - await tx(TableName.Users) - .where("id", userId) - .update({ - temporaryLockDateEnd: new Date(new Date().getTime() + progressiveDelaysInMins[delayIndex] * 60 * 1000) - }) - .returning("*") - )[0]; - } - - return user; - }); - - return updatedUser; - } catch (error) { - throw new DatabaseError({ error, name: "Process failed MFA Attempt" }); - } -}; From b9a6f94eea18c10fd43f027aa648ebe1f5802e60 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Fri, 24 May 2024 23:56:24 +0800 Subject: [PATCH 6/9] misc: moved user lock reset after backup success --- backend/src/services/auth/auth-password-service.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/backend/src/services/auth/auth-password-service.ts b/backend/src/services/auth/auth-password-service.ts index 8f4e8a6d2..a400c297b 100644 --- a/backend/src/services/auth/auth-password-service.ts +++ b/backend/src/services/auth/auth-password-service.ts @@ -138,11 +138,6 @@ export const authPaswordServiceFactory = ({ code }); - await userDAL.updateById(user.id, { - isLocked: false, - temporaryLockDateEnd: null - }); - const token = jwt.sign( { authTokenType: AuthTokenType.SIGNUP_TOKEN, @@ -179,6 +174,12 @@ export const authPaswordServiceFactory = ({ salt, verifier }); + + await userDAL.updateById(userId, { + isLocked: false, + temporaryLockDateEnd: null, + consecutiveFailedMfaAttempts: 0 + }); }; /* From 3639a7fc18c5ad4e59bcc5a07c599e58433b697a Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Mon, 27 May 2024 11:01:22 +0800 Subject: [PATCH 7/9] misc: migrated to native DAL method --- backend/src/services/auth/auth-login-service.ts | 2 +- backend/src/services/user/user-dal.ts | 16 +--------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/backend/src/services/auth/auth-login-service.ts b/backend/src/services/auth/auth-login-service.ts index a97e94145..cbf43b245 100644 --- a/backend/src/services/auth/auth-login-service.ts +++ b/backend/src/services/auth/auth-login-service.ts @@ -314,7 +314,7 @@ export const authLoginServiceFactory = ({ try { const updatedUser = await userDAL.transaction(async (tx) => { const PROGRESSIVE_DELAY_INTERVAL = 3; - const user = await userDAL.incrementFailedMfaAttempt(userId, tx); + const user = await userDAL.updateById(userId, { $incr: { consecutiveFailedMfaAttempts: 1 } }, tx); if (!user) { throw new Error("User not found"); diff --git a/backend/src/services/user/user-dal.ts b/backend/src/services/user/user-dal.ts index 7c73fc4f4..f2da0df0e 100644 --- a/backend/src/services/user/user-dal.ts +++ b/backend/src/services/user/user-dal.ts @@ -143,19 +143,6 @@ export const userDALFactory = (db: TDbClient) => { } }; - const incrementFailedMfaAttempt = async (userId: string, tx?: Knex) => { - try { - const [user] = await (tx || db)(TableName.Users) - .where("id", userId) - .increment("consecutiveFailedMfaAttempts", 1) - .returning("*"); - - return user; - } catch (error) { - throw new DatabaseError({ error, name: "Increment Failed MFA Attempt" }); - } - }; - return { ...userOrm, findUserByUsername, @@ -168,7 +155,6 @@ export const userDALFactory = (db: TDbClient) => { upsertUserEncryptionKey, createUserEncryption, findOneUserAction, - createUserAction, - incrementFailedMfaAttempt + createUserAction }; }; From a0f678a295fa810d081f7efc157cbc6cfcf15c34 Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Mon, 27 May 2024 22:59:24 +0800 Subject: [PATCH 8/9] misc: moved to using router push instead of reload --- frontend/src/views/Login/components/MFAStep/MFAStep.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/views/Login/components/MFAStep/MFAStep.tsx b/frontend/src/views/Login/components/MFAStep/MFAStep.tsx index 249c043b6..bf90b9cab 100644 --- a/frontend/src/views/Login/components/MFAStep/MFAStep.tsx +++ b/frontend/src/views/Login/components/MFAStep/MFAStep.tsx @@ -184,7 +184,7 @@ export const MFAStep = ({ email, password, providerAuthToken }: Props) => { if (triesLeft) { setTriesLeft((left) => { if (triesLeft === 1) { - window.location.reload(); + router.push("/"); } return (left as number) - 1; }); From fe4cc950d31d0eb5fab825b2cd6c3132772ccfae Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Tue, 28 May 2024 01:41:16 +0800 Subject: [PATCH 9/9] misc: updated temporary lock error message --- backend/src/services/auth/auth-fns.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/backend/src/services/auth/auth-fns.ts b/backend/src/services/auth/auth-fns.ts index 30297e605..2a322fdea 100644 --- a/backend/src/services/auth/auth-fns.ts +++ b/backend/src/services/auth/auth-fns.ts @@ -56,12 +56,15 @@ export const enforceUserLockStatus = (isLocked: boolean, temporaryLockDateEnd?: if (temporaryLockDateEnd) { const timeDiff = new Date().getTime() - temporaryLockDateEnd.getTime(); - if (timeDiff < 0) + if (timeDiff < 0) { + const secondsDiff = (-1 * timeDiff) / 1000; + const timeDisplay = + secondsDiff > 60 ? `${Math.ceil(secondsDiff / 60)} minutes` : `${Math.ceil(secondsDiff)} seconds`; + throw new UnauthorizedError({ name: "User Locked", - message: `User is locked due to multiple failed login attempts. Try again after ${Math.round( - (-1 * timeDiff) / 1000 - )} seconds.` + message: `User is temporary locked due to multiple failed login attempts. Try again after ${timeDisplay}. You can also reset your password now to proceed.` }); + } } };