From 433971a72dad11b0fb7001a1f4de3bebca3c108f Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Fri, 15 Nov 2024 23:25:32 +0800 Subject: [PATCH] misc: addressed comments 1 --- backend/src/server/routes/index.ts | 3 +- backend/src/server/routes/v1/user-router.ts | 16 ++- backend/src/server/routes/v2/mfa-router.ts | 4 +- .../src/services/auth/auth-login-service.ts | 6 +- .../services/auth/auth-password-service.ts | 11 +- backend/src/services/totp/totp-service.ts | 81 ++++++++++++-- backend/src/services/totp/totp-types.ts | 4 + .../src/components/mfa/TotpRegistration.tsx | 28 +++-- frontend/src/hooks/api/users/mutation.tsx | 14 +++ frontend/src/hooks/api/users/queries.tsx | 2 +- frontend/src/views/Login/Mfa.tsx | 104 ++++++++++-------- .../OrgAuthTab/OrgGenericAuthSection.tsx | 4 +- .../SecuritySection/MFASection.tsx | 62 ++++++++--- 13 files changed, 249 insertions(+), 90 deletions(-) diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index f3d3dd7d5c..b9f46627bb 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -525,7 +525,8 @@ export const registerRoutes = async ( tokenService, smtpService, authDAL, - userDAL + userDAL, + totpConfigDAL }); const projectBotService = projectBotServiceFactory({ permissionService, projectBotDAL, projectDAL }); diff --git a/backend/src/server/routes/v1/user-router.ts b/backend/src/server/routes/v1/user-router.ts index 706bb9a8b5..a97f11be49 100644 --- a/backend/src/server/routes/v1/user-router.ts +++ b/backend/src/server/routes/v1/user-router.ts @@ -196,7 +196,7 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { method: "DELETE", url: "/me/totp", config: { - rateLimit: readLimit + rateLimit: writeLimit }, onRequest: verifyAuth([AuthMode.JWT]), handler: async (req) => { @@ -254,4 +254,18 @@ export const registerUserRouter = async (server: FastifyZodProvider) => { }); } }); + + server.route({ + method: "POST", + url: "/me/totp/recovery-codes", + config: { + rateLimit: writeLimit + }, + onRequest: verifyAuth([AuthMode.JWT]), + handler: async (req) => { + return server.services.totp.createUserTotpRecoveryCodes({ + userId: req.permission.id + }); + } + }); }; diff --git a/backend/src/server/routes/v2/mfa-router.ts b/backend/src/server/routes/v2/mfa-router.ts index 738e27824d..6f28ec34c2 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 { NotFoundError } from "@app/lib/errors"; +import { BadRequestError, NotFoundError } from "@app/lib/errors"; import { mfaRateLimit } from "@app/server/config/rateLimiter"; import { AuthModeMfaJwtTokenPayload, AuthTokenType, MfaMethod } from "@app/services/auth/auth-type"; @@ -73,7 +73,7 @@ export const registerMfaRouter = async (server: FastifyZodProvider) => { isVerified: Boolean(totpConfig) }; } catch (error) { - if (error instanceof NotFoundError) { + if (error instanceof NotFoundError || error instanceof BadRequestError) { return { isVerified: false }; } diff --git a/backend/src/services/auth/auth-login-service.ts b/backend/src/services/auth/auth-login-service.ts index a580af2f5c..dea41e60b1 100644 --- a/backend/src/services/auth/auth-login-service.ts +++ b/backend/src/services/auth/auth-login-service.ts @@ -361,9 +361,9 @@ export const authLoginServiceFactory = ({ } const shouldCheckMfa = selectedOrg.enforceMfa || user.isMfaEnabled; - const orgMfaMethod = selectedOrg.enforceMfa ? selectedOrg.selectedMfaMethod : undefined; - const userMfaMethod = user.isMfaEnabled ? user.selectedMfaMethod : undefined; - const mfaMethod = orgMfaMethod ?? userMfaMethod ?? MfaMethod.EMAIL; + const orgMfaMethod = selectedOrg.enforceMfa ? selectedOrg.selectedMfaMethod ?? MfaMethod.EMAIL : undefined; + const userMfaMethod = user.isMfaEnabled ? user.selectedMfaMethod ?? MfaMethod.EMAIL : undefined; + const mfaMethod = orgMfaMethod ?? userMfaMethod; if (shouldCheckMfa && (!decodedToken.isMfaVerified || decodedToken.mfaMethod !== mfaMethod)) { enforceUserLockStatus(Boolean(user.isLocked), user.temporaryLockDateEnd); diff --git a/backend/src/services/auth/auth-password-service.ts b/backend/src/services/auth/auth-password-service.ts index 0e65589664..9ed9951fe1 100644 --- a/backend/src/services/auth/auth-password-service.ts +++ b/backend/src/services/auth/auth-password-service.ts @@ -8,6 +8,7 @@ import { generateSrpServerKey, srpCheckClientProof } from "@app/lib/crypto"; import { TAuthTokenServiceFactory } from "../auth-token/auth-token-service"; import { TokenType } from "../auth-token/auth-token-types"; import { SmtpTemplates, TSmtpService } from "../smtp/smtp-service"; +import { TTotpConfigDALFactory } from "../totp/totp-config-dal"; import { TUserDALFactory } from "../user/user-dal"; import { TAuthDALFactory } from "./auth-dal"; import { TChangePasswordDTO, TCreateBackupPrivateKeyDTO, TResetPasswordViaBackupKeyDTO } from "./auth-password-type"; @@ -18,6 +19,7 @@ type TAuthPasswordServiceFactoryDep = { userDAL: TUserDALFactory; tokenService: TAuthTokenServiceFactory; smtpService: TSmtpService; + totpConfigDAL: Pick; }; export type TAuthPasswordFactory = ReturnType; @@ -25,7 +27,8 @@ export const authPaswordServiceFactory = ({ authDAL, userDAL, tokenService, - smtpService + smtpService, + totpConfigDAL }: TAuthPasswordServiceFactoryDep) => { /* * Pre setup for pass change with srp protocol @@ -185,6 +188,12 @@ export const authPaswordServiceFactory = ({ temporaryLockDateEnd: null, consecutiveFailedMfaAttempts: 0 }); + + /* we reset the mobile authenticator configs of the user + because we want this to be one of the recovery modes from account lockout */ + await totpConfigDAL.delete({ + userId + }); }; /* diff --git a/backend/src/services/totp/totp-service.ts b/backend/src/services/totp/totp-service.ts index 11d3ac7a2c..f304f01b8a 100644 --- a/backend/src/services/totp/totp-service.ts +++ b/backend/src/services/totp/totp-service.ts @@ -8,6 +8,7 @@ import { TKmsServiceFactory } from "../kms/kms-service"; import { TUserDALFactory } from "../user/user-dal"; import { TTotpConfigDALFactory } from "./totp-config-dal"; import { + TCreateUserTotpRecoveryCodesDTO, TDeleteUserTotpConfigDTO, TGetUserTotpConfigDTO, TRegisterUserTotpDTO, @@ -27,8 +28,7 @@ export type TTotpServiceFactory = ReturnType; export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotpServiceFactoryDep) => { const getUserTotpConfig = async ({ userId }: TGetUserTotpConfigDTO) => { const totpConfig = await totpConfigDAL.findOne({ - userId, - isVerified: true + userId }); if (!totpConfig) { @@ -37,6 +37,12 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp }); } + if (!totpConfig.isVerified) { + throw new BadRequestError({ + message: "TOTP configuration has not been verified" + }); + } + const decryptWithRoot = kmsService.decryptWithRootKey(); const recoveryCodes = decryptWithRoot(totpConfig.encryptedRecoveryCodes).toString().split(","); @@ -102,8 +108,7 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp const verifyUserTotpConfig = async ({ userId, totp }: TVerifyUserTotpConfigDTO) => { const totpConfig = await totpConfigDAL.findOne({ - userId, - isVerified: false + userId }); if (!totpConfig) { @@ -112,6 +117,12 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp }); } + if (totpConfig.isVerified) { + throw new BadRequestError({ + message: "TOTP configuration has already been verified" + }); + } + const decryptWithRoot = kmsService.decryptWithRootKey(); const secret = decryptWithRoot(totpConfig.encryptedSecret).toString(); const isValid = authenticator.verify({ @@ -132,8 +143,7 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp const verifyUserTotp = async ({ userId, totp }: TVerifyUserTotpDTO) => { const totpConfig = await totpConfigDAL.findOne({ - userId, - isVerified: true + userId }); if (!totpConfig) { @@ -142,6 +152,12 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp }); } + if (!totpConfig.isVerified) { + throw new BadRequestError({ + message: "TOTP configuration has not been verified" + }); + } + const decryptWithRoot = kmsService.decryptWithRootKey(); const secret = decryptWithRoot(totpConfig.encryptedSecret).toString(); const isValid = authenticator.verify({ @@ -158,8 +174,7 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp const verifyWithUserRecoveryCode = async ({ userId, recoveryCode }: TVerifyWithUserRecoveryCodeDTO) => { const totpConfig = await totpConfigDAL.findOne({ - userId, - isVerified: true + userId }); if (!totpConfig) { @@ -168,6 +183,12 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp }); } + if (!totpConfig.isVerified) { + throw new BadRequestError({ + message: "TOTP configuration has not been verified" + }); + } + const decryptWithRoot = kmsService.decryptWithRootKey(); const encryptWithRoot = kmsService.encryptWithRootKey(); @@ -188,8 +209,7 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp const deleteUserTotpConfig = async ({ userId }: TDeleteUserTotpConfigDTO) => { const totpConfig = await totpConfigDAL.findOne({ - userId, - isVerified: true + userId }); if (!totpConfig) { @@ -201,12 +221,51 @@ export const totpServiceFactory = ({ totpConfigDAL, kmsService, userDAL }: TTotp await totpConfigDAL.deleteById(totpConfig.id); }; + const createUserTotpRecoveryCodes = async ({ userId }: TCreateUserTotpRecoveryCodesDTO) => { + const decryptWithRoot = kmsService.decryptWithRootKey(); + const encryptWithRoot = kmsService.encryptWithRootKey(); + + return totpConfigDAL.transaction(async (tx) => { + const totpConfig = await totpConfigDAL.findOne( + { + userId, + isVerified: true + }, + tx + ); + + if (!totpConfig) { + throw new NotFoundError({ + message: "Valid TOTP configuration not found" + }); + } + + const recoveryCodes = decryptWithRoot(totpConfig.encryptedRecoveryCodes).toString().split(","); + if (recoveryCodes.length >= 10) { + throw new BadRequestError({ + message: "Cannot have more than 10 recovery codes at a time" + }); + } + + const toGenerateCount = 10 - recoveryCodes.length; + const newRecoveryCodes = Array.from({ length: toGenerateCount }).map(() => + String(crypto.randomInt(10 ** 7, 10 ** 8 - 1)) + ); + const encryptedRecoveryCodes = encryptWithRoot(Buffer.from([...recoveryCodes, ...newRecoveryCodes].join(","))); + + await totpConfigDAL.updateById(totpConfig.id, { + encryptedRecoveryCodes + }); + }); + }; + return { registerUserTotp, verifyUserTotpConfig, getUserTotpConfig, verifyUserTotp, verifyWithUserRecoveryCode, - deleteUserTotpConfig + deleteUserTotpConfig, + createUserTotpRecoveryCodes }; }; diff --git a/backend/src/services/totp/totp-types.ts b/backend/src/services/totp/totp-types.ts index 8564b062cf..15c0156197 100644 --- a/backend/src/services/totp/totp-types.ts +++ b/backend/src/services/totp/totp-types.ts @@ -24,3 +24,7 @@ export type TVerifyWithUserRecoveryCodeDTO = { export type TDeleteUserTotpConfigDTO = { userId: string; }; + +export type TCreateUserTotpRecoveryCodesDTO = { + userId: string; +}; diff --git a/frontend/src/components/mfa/TotpRegistration.tsx b/frontend/src/components/mfa/TotpRegistration.tsx index 37e28069be..59d4914f5a 100644 --- a/frontend/src/components/mfa/TotpRegistration.tsx +++ b/frontend/src/components/mfa/TotpRegistration.tsx @@ -13,11 +13,13 @@ type Props = { const TotpRegistration = ({ onComplete }: Props) => { const { data: registration, isLoading } = useGetUserTotpRegistration(); - const { mutateAsync: verifyUserTotp } = useVerifyUserTotpRegistration(); + const { mutateAsync: verifyUserTotp, isLoading: isVerifyLoading } = + useVerifyUserTotpRegistration(); const [qrCodeUrl, setQrCodeUrl] = useState(""); const [totp, setTotp] = useState(""); - const handleTotpVerify = async () => { + const handleTotpVerify = async (event: React.FormEvent) => { + event.preventDefault(); await verifyUserTotp({ totp }); @@ -54,15 +56,19 @@ const TotpRegistration = ({ onComplete }: Props) => {
registration-qr
-
Enter the resulting verification code
-
- setTotp(e.target.value)} - value={totp} - placeholder="Verification code" - /> - -
+
+
Enter the resulting verification code
+
+ setTotp(e.target.value)} + value={totp} + placeholder="Verification code" + /> + +
+
); }; diff --git a/frontend/src/hooks/api/users/mutation.tsx b/frontend/src/hooks/api/users/mutation.tsx index fe48b1f3b5..e8cf41acbd 100644 --- a/frontend/src/hooks/api/users/mutation.tsx +++ b/frontend/src/hooks/api/users/mutation.tsx @@ -140,3 +140,17 @@ export const useDeleteUserTotpConfiguration = () => { } }); }; + +export const useCreateNewTotpRecoveryCodes = () => { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: async () => { + await apiRequest.post("/api/v1/user/me/totp/recovery-codes"); + + return {}; + }, + onSuccess: () => { + queryClient.invalidateQueries(userKeys.totpConfiguration); + } + }); +}; diff --git a/frontend/src/hooks/api/users/queries.tsx b/frontend/src/hooks/api/users/queries.tsx index 262068bc7f..98d8e4b565 100644 --- a/frontend/src/hooks/api/users/queries.tsx +++ b/frontend/src/hooks/api/users/queries.tsx @@ -480,7 +480,7 @@ export const useGetUserTotpConfiguration = () => { return data; } catch (error) { - if (error instanceof AxiosError && error.response?.data?.statusCode === 404) { + if (error instanceof AxiosError && [404, 400].includes(error.response?.data?.statusCode)) { return { isVerified: false, recoveryCodes: [] diff --git a/frontend/src/views/Login/Mfa.tsx b/frontend/src/views/Login/Mfa.tsx index c9d196b35e..6132d05bfe 100644 --- a/frontend/src/views/Login/Mfa.tsx +++ b/frontend/src/views/Login/Mfa.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import React, { useEffect, useState } from "react"; import ReactCodeInput from "react-code-input"; import Image from "next/image"; import Link from "next/link"; @@ -62,7 +62,9 @@ export const Mfa = ({ successCallback, closeMfa, hideLogo, email, method }: Prop } }, []); - const verifyMfa = async () => { + const verifyMfa = async (event: React.FormEvent) => { + event.preventDefault(); + setIsLoading(true); try { const { token } = await verifyMfaToken({ @@ -110,14 +112,19 @@ export const Mfa = ({ successCallback, closeMfa, hideLogo, email, method }: Prop if (shouldShowTotpRegistration) { return ( -
- { - setShouldShowTotpRegistration(false); - await successCallback(); - }} - /> -
+ <> +
+ Your organization requires mobile authenticator to be configured. +
+
+ { + setShouldShowTotpRegistration(false); + await successCallback(); + }} + /> +
+ ); } @@ -147,42 +154,53 @@ export const Mfa = ({ successCallback, closeMfa, hideLogo, email, method }: Prop

)} -
- {method === MfaMethod.EMAIL && ( - +
+
+ {method === MfaMethod.EMAIL && ( + + )} + {method === MfaMethod.TOTP && ( +
+ setMfaCode(e.target.value)} /> +
+ )} +
+ {typeof triesLeft === "number" && ( + )} - {method === MfaMethod.TOTP && ( -
- setMfaCode(e.target.value)} /> +
+
+
- )} -
- {typeof triesLeft === "number" && ( - - )} -
-
-
-
+ + {method === MfaMethod.TOTP && ( +
+ + + No access to both codes? Reset your account + + +
+ )} {method === MfaMethod.EMAIL && (
diff --git a/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/OrgGenericAuthSection.tsx b/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/OrgGenericAuthSection.tsx index 6d1485ea5f..9b9aa65532 100644 --- a/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/OrgGenericAuthSection.tsx +++ b/frontend/src/views/Settings/OrgSettingsPage/components/OrgAuthTab/OrgGenericAuthSection.tsx @@ -58,7 +58,7 @@ export const OrgGenericAuthSection = () => { }); createNotification({ - text: "Successfully updated preferred MFA method", + text: "Successfully updated selected MFA method", type: "success" }); } catch (err) { @@ -90,7 +90,7 @@ export const OrgGenericAuthSection = () => { Enforce members to authenticate with MFA in order to access the organization

{currentOrg?.enforceMfa && ( - + { )} -
Mobile Authenticator
+
Mobile Authenticator
{isTotpConfigurationLoading ? ( ) : ( @@ -158,6 +182,9 @@ export const MFASection = () => { + @@ -171,13 +198,19 @@ export const MFASection = () => { )}
) : ( -
- { - await queryClient.invalidateQueries(userKeys.totpConfiguration); - }} - /> -
+ <> +
+ For added security, you can configure a mobile authenticator and set it as your + selected 2FA method. +
+
+ { + await queryClient.invalidateQueries(userKeys.totpConfiguration); + }} + /> +
+ )}
)} @@ -188,7 +221,8 @@ export const MFASection = () => { /> handlePopUpToggle("deleteTotpConfig", isOpen)} deleteKey="confirm" onDeleteApproved={handleTotpDeletion}