Feature force 2fa (#28) #29
Conversation
* feat: Implement first login password change and 2FA setup flow * feat: Enhance first login flow with token return and 2FA setup option * feat: Implement secure password generation and reset confirmation dialog in Users component
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a comprehensive first login flow with password change requirements and 2FA setup functionality, enhancing the application's security posture. The implementation adds forced password changes on first login with strong password requirements and optional 2FA setup for new users.
Key changes:
- Added forced password change flow for first-time users with comprehensive validation
- Implemented 2FA setup flow with QR codes and backup codes
- Enhanced password reset functionality with secure password generation and improved UI
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/services/auth.service.ts | Added interfaces and service methods for first login password change flow |
| apps/web/src/components/pages/Users.tsx | Enhanced password reset with secure generation and confirmation dialog UI |
| apps/web/src/components/pages/Login.tsx | Added multi-step login flow supporting password change and 2FA setup |
| apps/web/src/components/pages/ForcePasswordChange.tsx | New component for mandatory password change with strength validation |
| apps/web/src/components/pages/Force2FASetup.tsx | New component for 2FA setup with QR codes and backup codes |
| apps/web/src/components/pages/Account.tsx | Enhanced password validation with stronger requirements |
| apps/web/src/auth.tsx | Updated auth interfaces to support new login flow parameters |
| apps/api/src/utils/password.ts | Added secure password generation utility with required character types |
| apps/api/src/utils/jwt.ts | Added temporary token generation and verification for first login flow |
| apps/api/src/domains/users/users.service.ts | Updated to use secure password generation for reset functionality |
| apps/api/src/domains/auth/* | Comprehensive auth system updates supporting first login and 2FA flows |
| apps/api/prisma/* | Database schema updates to track first login status |
Comments suppressed due to low confidence (1)
apps/web/src/components/pages/Users.tsx:1
- The import for
useTranslationhas been removed but the corresponding removal of its usage was not shown in the diff. Ensure this removal is intentional and doesn't break functionality elsewhere in the component.
import { useState } from "react";
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"; | ||
| import { Switch } from "@/components/ui/switch"; | ||
| import { UserPlus, Mail, Key, Trash2, Edit, Shield, Loader2, Users as UsersIcon } from "lucide-react"; | ||
| import { UserPlus, Key, Trash2, Edit, Shield, Loader2, Users as UsersIcon, Copy, CheckCircle2, AlertTriangle } from "lucide-react"; |
There was a problem hiding this comment.
The Mail import has been removed from lucide-react imports. Verify that this icon is not used elsewhere in the component to avoid runtime errors.
| // Force re-render: close and reopen dialog with password | ||
| const currentUserId = resetPasswordDialog.userId; | ||
| const currentUsername = resetPasswordDialog.username; | ||
|
|
||
| // Close dialog first | ||
| setResetPasswordDialog({ isOpen: false, userId: '', username: '' }); | ||
|
|
||
| // Then reopen with password after a tiny delay | ||
| setTimeout(() => { | ||
| setResetPasswordDialog({ | ||
| isOpen: true, | ||
| userId: currentUserId, | ||
| username: currentUsername, | ||
| newPassword: newPassword | ||
| }); | ||
| setPasswordCopied(false); | ||
| }, 100); | ||
|
|
There was a problem hiding this comment.
Using setTimeout with arbitrary delays for state management is fragile and creates timing dependencies. Consider redesigning the dialog state to handle password display without requiring a close/reopen cycle, or use proper React patterns like useEffect with dependencies.
| // Force re-render: close and reopen dialog with password | |
| const currentUserId = resetPasswordDialog.userId; | |
| const currentUsername = resetPasswordDialog.username; | |
| // Close dialog first | |
| setResetPasswordDialog({ isOpen: false, userId: '', username: '' }); | |
| // Then reopen with password after a tiny delay | |
| setTimeout(() => { | |
| setResetPasswordDialog({ | |
| isOpen: true, | |
| userId: currentUserId, | |
| username: currentUsername, | |
| newPassword: newPassword | |
| }); | |
| setPasswordCopied(false); | |
| }, 100); | |
| // Update dialog state to show new password, keep dialog open | |
| setResetPasswordDialog({ | |
| isOpen: true, | |
| userId: resetPasswordDialog.userId, | |
| username: resetPasswordDialog.username, | |
| newPassword: newPassword | |
| }); | |
| setPasswordCopied(false); | |
| const router = useRouter(); | ||
| const isLoading = useRouterState({ select: (s) => s.isLoading }); | ||
| const { login, loginWith2FA, isAuthenticated, isLoading: authLoading } = useAuth(); | ||
| const { login, loginWith2FA, isLoading: authLoading } = useAuth(); |
There was a problem hiding this comment.
The isAuthenticated variable was removed from the destructuring but may still be needed elsewhere in the component. Verify this removal doesn't break authentication state checks.
| AlertDialogTitle, | ||
| } from '@/components/ui/alert-dialog'; | ||
| import { toast } from 'sonner'; | ||
| import { accountService } from '@/services/auth.service'; |
There was a problem hiding this comment.
The import references accountService from auth.service, but based on the auth service changes shown in other files, this should likely be importing from a dedicated account service file or the correct service method name.
| import { accountService } from '@/services/auth.service'; | |
| import { accountService } from '@/services/account.service'; |
| return jwt.sign({ userId, type: 'temp' }, config.jwt.accessSecret, { | ||
| expiresIn: '15m', // Temporary token valid for 15 minutes | ||
| } as any); |
There was a problem hiding this comment.
Using as any bypasses TypeScript type checking. Define proper types for the JWT sign options or use the correct SignOptions type from the jsonwebtoken library.
| // Return successful login response (LoginResult type) | ||
| if ('accessToken' in result && 'refreshToken' in result) { |
There was a problem hiding this comment.
The type checking logic relies on duck typing which can be error-prone. Consider using proper type guards or discriminated unions with explicit type fields to make the code more maintainable and type-safe.
feat: Implement first login password change and 2FA setup flow
feat: Enhance first login flow with token return and 2FA setup option
feat: Implement secure password generation and reset confirmation dialog in Users component