-
Notifications
You must be signed in to change notification settings - Fork 3
fix: improve onboarding and error handling #167
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.
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
Changes from all commits
6442ea6
273e0c4
624c3df
a7a2af3
2f95ea8
8590be6
e5367d2
40029ac
bd6499d
28380ad
3d5db73
f394c6f
2b60ca1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,11 @@ import { EUserRole } from '@/common/constants/user.constant'; | |
| import { SkipCSRF } from '@/common/decorators/skip-csrf.decorator'; | ||
| import { AuthService } from '@/modules/auth/auth.service'; | ||
| import { LoginDto } from '@/modules/auth/dto/login.dto'; | ||
| import { ResetPasswordDto } from '@/modules/auth/dto/reset-password.dto'; | ||
| import { CreateUserDto } from '@/modules/auth/dto/signup.dto'; | ||
| import { UserResponseDto } from '@/modules/auth/dto/user-response.dto'; | ||
| import { ResetPasswordDto } from '@/modules/auth/dto/reset-password.dto'; | ||
| import { UserStatus } from '@/modules/user/enum/userStatus.enum'; | ||
| import { getAppUrl } from '@/utils/app-config'; | ||
| import { generateCSRFToken } from '@/utils/csrf.util'; | ||
|
|
||
| @ApiTags('auth') | ||
|
|
@@ -173,20 +174,34 @@ export class AuthController { | |
| @Get('google/callback') | ||
| @UseGuards(AuthGuard('google')) | ||
| googleAuthRedirect(@Req() req: Request, @Res() res: Response): void { | ||
| // Check if user is authenticated | ||
| if (!req.user) { | ||
| const frontendUrl = getAppUrl(); | ||
| res.redirect(`${frontendUrl}/login?error=oauth_failed`); | ||
| return; | ||
| } | ||
|
|
||
| const { user, token, csrfToken } = req.user as { | ||
| user: Record<string, unknown>; | ||
| token: string; | ||
| csrfToken: string; | ||
| }; | ||
|
|
||
| // Validate required fields | ||
| if (!token || !csrfToken) { | ||
| const frontendUrl = getAppUrl(); | ||
| res.redirect(`${frontendUrl}/login?error=oauth_incomplete`); | ||
| return; | ||
| } | ||
|
|
||
| // Manually construct safe user object to preserve ObjectId | ||
| const safeUser = { | ||
| _id: user._id?.toString() ?? user._id, | ||
| email: user.email ?? '', | ||
| firstName: user.firstName, | ||
| lastName: user.lastName, | ||
| role: user.role as EUserRole, | ||
| status: user.status as UserStatus, | ||
| _id: user._id?.toString() ?? (user._id as string | undefined) ?? '', | ||
| email: (user.email as string | undefined) ?? '', | ||
| firstName: (user.firstName as string | undefined) ?? '', | ||
| lastName: (user.lastName as string | undefined) ?? '', | ||
| role: (user.role as EUserRole | undefined) ?? EUserRole.user, | ||
| status: (user.status as UserStatus | undefined) ?? UserStatus.active, | ||
| }; | ||
|
|
||
| // Set JWT token as httpOnly cookie | ||
|
|
@@ -208,22 +223,29 @@ export class AuthController { | |
| }); | ||
|
|
||
| // Redirect to frontend with user data (CSRF token is in regular cookie) | ||
| const frontendUrl = process.env.APP_URL ?? 'http://localhost:3000'; | ||
| const frontendUrl = getAppUrl(); | ||
| res.redirect( | ||
| `${frontendUrl}/auth/callback?user=${encodeURIComponent(JSON.stringify(safeUser))}&csrfToken=${encodeURIComponent(csrfToken)}`, | ||
| ); | ||
|
Comment on lines
227
to
229
|
||
| } | ||
|
|
||
| @ApiOperation({ | ||
| summary: 'Forgot Password', | ||
| description: 'Send a password reset link to the user\'s email', | ||
| description: "Send a password reset link to the user's email", | ||
| }) | ||
| @ApiResponse({ | ||
| status: 200, | ||
| description: 'If that email is registered, a reset link has been sent.', | ||
| }) | ||
| @ApiResponse({ status: 200, description: 'If that email is registered, a reset link has been sent.' }) | ||
| @Post('forgot-password') | ||
| @SkipCSRF() | ||
| async forgotPassword(@Body('email') email: string): Promise<{ message: string }> { | ||
| async forgotPassword( | ||
| @Body('email') email: string, | ||
| ): Promise<{ message: string }> { | ||
| await this.authService.forgotPassword(email); | ||
| return { message: 'If that email is registered, a reset link has been sent.' }; | ||
| return { | ||
| message: 'If that email is registered, a reset link has been sent.', | ||
| }; | ||
| } | ||
|
|
||
| @ApiOperation({ | ||
|
|
@@ -334,7 +356,9 @@ export class AuthController { | |
| @ApiResponse({ status: 400, description: 'Invalid token or password' }) | ||
| @Post('reset-password') | ||
| @SkipCSRF() | ||
| async resetPassword(@Body() dto: ResetPasswordDto): Promise<{ message: string }> { | ||
| async resetPassword( | ||
| @Body() dto: ResetPasswordDto, | ||
| ): Promise<{ message: string }> { | ||
| await this.authService.resetPassword(dto); | ||
| return { message: 'Password reset successful' }; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,4 +15,4 @@ export class ResetPasswordDto { | |
| @IsString() | ||
| @MinLength(6) | ||
| confirmPassword!: string; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,10 +21,30 @@ export class GoogleStrategy extends PassportStrategy(Strategy, 'google') { | |
| private readonly jwtService: JwtService, | ||
| @InjectModel(User.name) private readonly userModel: Model<UserDocument>, | ||
| ) { | ||
| let clientID = configService.get<string>('GOOGLE_CLIENT_ID') ?? ''; | ||
| let clientSecret = configService.get<string>('GOOGLE_CLIENT_SECRET') ?? ''; | ||
| let callbackURL = configService.get<string>('GOOGLE_CALLBACK_URL') ?? ''; | ||
|
|
||
| // In development, allow service to start without Google OAuth credentials | ||
| if ( | ||
| (clientID === '' || clientSecret === '' || callbackURL === '') && | ||
| (process.env.NODE_ENV === 'development' || | ||
| process.env.NODE_ENV === undefined) | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| '⚠️ Google OAuth credentials not found. Google OAuth features will be disabled.', | ||
| ); | ||
| // Provide dummy values for development | ||
| clientID = 'dummy_client_id'; | ||
| clientSecret = 'dummy_client_secret'; | ||
| callbackURL = 'http://localhost:4000/api/auth/google/callback'; | ||
| } | ||
|
|
||
| super({ | ||
| clientID: configService.get<string>('GOOGLE_CLIENT_ID') ?? '', | ||
| clientSecret: configService.get<string>('GOOGLE_CLIENT_SECRET') ?? '', | ||
| callbackURL: configService.get<string>('GOOGLE_CALLBACK_URL') ?? '', | ||
| clientID, | ||
| clientSecret, | ||
| callbackURL, | ||
| scope: ['email', 'profile'], | ||
| } as StrategyOptions); | ||
| } | ||
|
|
@@ -34,19 +54,26 @@ export class GoogleStrategy extends PassportStrategy(Strategy, 'google') { | |
| refreshToken: string, | ||
| profile: { | ||
| id: string; | ||
| name: { givenName: string; familyName: string }; | ||
| emails: { value: string }[]; | ||
| photos: { value: string }[]; | ||
| name?: { givenName?: string; familyName?: string }; | ||
| emails?: { value: string }[]; | ||
| photos?: { value: string }[]; | ||
| }, | ||
| done: VerifyCallback, | ||
| ): Promise<void> { | ||
| try { | ||
| // Validate profile data - profile.emails and profile.name are optional | ||
| if (!profile.emails || profile.emails.length === 0 || !profile.name) { | ||
| done(new UnauthorizedException('Invalid Google profile data'), false); | ||
| return; | ||
| } | ||
|
Comment on lines
+64
to
+68
|
||
|
|
||
| // At this point, we know profile.name and profile.emails exist and have items | ||
| const { name, emails } = profile; | ||
|
|
||
| const googleUser = { | ||
| email: emails[0].value, | ||
| firstName: name.givenName, | ||
| lastName: name.familyName, | ||
| firstName: name.givenName ?? '', | ||
| lastName: name.familyName ?? '', | ||
| }; | ||
|
|
||
| let user = await this.userModel.findOne({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ export class Company { | |
| user!: User; | ||
| @Prop() | ||
| calendar_access_token?: string; | ||
|
|
||
| _id!: Types.ObjectId; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock Twilio client provides incomplete type coverage. The Twilio.Twilio type includes many more properties and methods beyond just calls and messages (e.g., accounts, addresses, applications, etc.). Code that attempts to use other Twilio features will fail at runtime even though TypeScript won't catch it due to the type assertion. Consider either implementing a more complete mock object or using a proper mocking library, or at minimum document this limitation clearly.