feat(login): add 2FA#4205
Conversation
|
@boutterudy is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds a comprehensive TOTP-based 2FA implementation including enrollment, login challenge, backup codes, admin enforcement at global/team/user levels, and rate limiting with replay prevention. Previous review rounds addressed the major security issues (transaction wrapping for confirm, race-condition-safe backup code consumption, serializable rate-limit transactions, and the setup/initiate bypass). Remaining findings are all P2 quality/maintainability items. Confidence Score: 5/5Safe to merge — all previously identified P0/P1 issues have been addressed; remaining findings are P2 style and maintenance concerns. All critical security and data-integrity bugs raised in prior review rounds have been fixed (transaction wrapping, backup code double-use, rate-limit atomicity, setup/initiate bypass). Only P2 suggestions remain: shared helper for the duplicated 2FA-requirement check, lazy-only OTP record cleanup, unhandled max-retry throw, and a React strict-mode development redirect loop. src/lib/two-factor/replay-prevention.ts (unbounded table growth), src/app/api/2fa/disable/route.ts (duplicated requirement check logic), src/lib/two-factor/rate-limit.ts (unhandled max-retry exception) Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant API as Next.js API
participant DB as Database
rect rgb(230,245,255)
Note over User,DB: Enrollment
User->>Browser: Click Enable 2FA
Browser->>API: POST /api/2fa/setup/initiate
API->>DB: findUnique twoFactorAuth (isEnabled guard)
API->>DB: upsert twoFactorAuth isEnabled=false encryptedSecret
API-->>Browser: qrCodeDataUrl manualKey
User->>Browser: Scan QR + enter OTP
Browser->>API: POST /api/2fa/setup/confirm token
API->>DB: checkRateLimit / isOtpReplayed / verifyTotp
API->>DB: transaction update isEnabled=true deleteMany+createMany backupCodes markOtpUsed
API-->>Browser: backupCodes[]
end
rect rgb(255,245,230)
Note over User,DB: Login with 2FA
User->>Browser: username + password
Browser->>API: POST /api/auth/login
API->>DB: findUnique twoFactorAuth
API-->>Browser: requiresTwoFactor=true partialToken 5m TTL
Browser->>Browser: sessionStorage.setItem partialToken
Browser->>Browser: redirect /login/two-factor
User->>Browser: Enter OTP or backup code
Browser->>API: POST /api/2fa/verify Bearer partialToken
API->>API: parseSecureToken userId type=partial-auth
API->>DB: checkRateLimit / verifyTotp or verifyBackupCode
API-->>Browser: token user
end
rect rgb(230,255,230)
Note over User,DB: Disable 2FA
User->>Browser: password + OTP
Browser->>API: POST /api/2fa/disable
API->>DB: check global/user/team requirement 4 queries
API->>DB: checkPassword / checkRateLimit / isOtpReplayed / verifyTotp
API->>DB: transaction markOtpUsed delete twoFactorAuth deleteMany backupCodes
API-->>Browser: ok true
end
Reviews (6): Last reviewed commit: "fix(2FA): mark OTP used in transaction" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai You've mentioned two issues, and I've fixed them:
|
1 participant
❓ Context
Two-factor authentication is widely anticipated and requested by the community, as evidenced by the discussions #1195, #4167 (comment), #929 and #3163 (comment) 👀
In light of this, I took the initiative to suggest various options during a conversation (#1195 (comment)), and after receiving some feedback, I set to work on implementing 2FA.
💡 Flow & Features
Enforce 2FA
Administrators are given the option to make 2FA required at several levels:
When 2FA is required for a user, the next time they perform an action (e.g. navigate on a page) or log in, they will be required to set up 2FA for their account.
When 2FA is required for a user, once it has been set up, they cannot disable it (unless the admin changes the 2FA settings).
Optional 2FA
When a user is not required to set up 2FA, they can still do so by accessing their security settings and enabling it.
When 2FA is not required for a user, they can disable it by entering their password and a code generated by their authentication app.
Backup codes
As a backup login method, backup codes are generated when 2FA is enabled and must be saved by the user. They can then use these codes to log in if necessary.
Login
When a user logs in, if 2FA is enabled for that user, they will need to enter a code generated by the authentication app they have set up in order to log in.
If 2FA is not enabled for a user, the login process remains the same as before.
🔐 Security
As the aim of 2FA is to enhance Umami’s security, I have tried to take every possible precaution to ensure the security of this implementation.
In a nutshell:
TWO_FACTOR_ENCRYPTION_KEYenv var must be a 64-char hex stringTWO_FACTOR_ENCRYPTION_KEYenv var), so a database leak does not expose itIf you’re interested in understanding the full implementation, take a look at the sequence diagram below
sequenceDiagram autonumber actor User participant Browser participant API as Next.js API participant DB as Database rect rgb(230, 245, 255) Note over User,DB: Enrollment User->>Browser: Opens 2FA settings Browser->>API: GET /api/2fa/status (full session) API->>DB: SELECT two_factor_auth WHERE userId DB-->>API: row (or null) API-->>Browser: { isEnabled: false, isRequired, requiredReason } User->>Browser: Clicks "Enable 2FA" Browser->>API: POST /api/2fa/setup/initiate (full session) API->>API: generateSecret() → plaintext secret API->>API: encryptSecret() → AES-256-GCM ciphertext API->>DB: UPSERT two_factor_auth (isEnabled=false) API-->>Browser: { qrCodeDataUrl, manualKey } User->>Browser: Scans QR code in authenticator app User->>Browser: Enters first 6-digit OTP Browser->>API: POST /api/2fa/setup/confirm { token } API->>DB: SELECT two_factor_auth (isEnabled=false) API->>DB: SELECT two_factor_rate_limit API->>DB: SELECT two_factor_otp_used (replay check) API->>API: decryptSecret() + verifyTotp() alt TOTP invalid API->>DB: INCREMENT rate limit attempts API-->>Browser: 400 { code: two-factor-error-invalid-code } Browser-->>User: Error message else TOTP valid API->>DB: UPDATE two_factor_auth SET isEnabled=true API->>DB: INSERT two_factor_otp_used (90s TTL) API->>DB: DELETE two_factor_rate_limit API->>API: generateBackupCodes() → 10 bcrypt-hashed codes API->>DB: REPLACE all two_factor_backup_code rows API-->>Browser: { backupCodes: string[] } Browser-->>User: Shows backup codes (one-time display) end end rect rgb(255, 245, 230) Note over User,DB: Login with 2FA User->>Browser: Submits username + password Browser->>API: POST /api/auth/login { username, password } API->>DB: SELECT user WHERE username API->>API: checkPassword() API->>DB: SELECT two_factor_auth WHERE userId alt 2FA not enabled API-->>Browser: { token, user } else 2FA enabled API->>API: createSecureToken({ userId, type:'partial-auth' }, 5m TTL) API-->>Browser: { requiresTwoFactor: true, partialToken } Browser->>Browser: sessionStorage.setItem('umami.partial-token', partialToken) Browser->>Browser: redirect → /login/two-factor end User->>Browser: Enters 6-digit OTP (or backup code) Browser->>Browser: partialToken = sessionStorage.removeItem(...) Browser->>API: POST /api/2fa/verify { token } Bearer: partialToken API->>API: parseSecureToken() → verify type='partial-auth', extract userId API->>DB: SELECT two_factor_auth WHERE userId (isEnabled=true) API->>DB: SELECT two_factor_rate_limit alt Rate limit exceeded API-->>Browser: 429 { code: two-factor-error-too-many-attempts, lockedUntil } Browser-->>User: Locked until HH:MM:SS else Using TOTP token API->>DB: SELECT two_factor_otp_used (replay check) API->>API: decryptSecret() + verifyTotp() alt TOTP invalid API->>DB: INCREMENT rate limit attempts API-->>Browser: 400 { code: two-factor-error-invalid-code } Browser-->>User: Error message else TOTP valid API->>DB: INSERT two_factor_otp_used (90s TTL) API->>DB: DELETE two_factor_rate_limit API->>API: createSecureToken / saveAuth → full session token API-->>Browser: { token, user } Browser->>Browser: setClientAuthToken(token), setUser(user) Browser->>Browser: router.push('/') end else Using backup code API->>DB: SELECT unused two_factor_backup_code WHERE userId API->>API: bcrypt.compare() against each hash alt No match API->>DB: INCREMENT rate limit attempts API-->>Browser: 400 { code: two-factor-error-invalid-backup-code } else Match found API->>DB: UPDATE backup_code SET used=true API->>DB: DELETE two_factor_rate_limit API->>API: createSecureToken / saveAuth → full session token API-->>Browser: { token, user } Browser->>Browser: setClientAuthToken(token), setUser(user) Browser->>Browser: router.push('/') end end end✨ Wonderful Results
🛡️ As admin
Security settings
As.admin.-.Security.settings.mov
Team settings
As.admin.-.Team.settings.mov
User security settings
2FA not required
2FA required but not configured
2FA required and active
👤 As user
2FA setup
As.user.-.2FA.setup.mov
2FA setup when required
Login with 2FA
As.user.-.Login.with.2FA.mov
Security settings
2FA not required and not configured
2FA required and active
2FA not required and active
Disable 2FA
As.user.-.Disable.2FA.mov
Disable 2FA, error when incorrect password
Disable 2FA, error when invalid code
Disable 2FA, error when already used code
Greptile AI review context
This project uses
relationMode = "prisma"inprisma/schema.prisma. Foreign key constraints are intentionally enforced at the Prisma ORM layer rather than the database level. Migration files will not containADD CONSTRAINT ... FOREIGN KEYorON DELETE CASCADEstatements and this is expected, not an oversight.