Conversation
…nd preferences management
…aders and update registration response status
…e user deletion process
…l fields and error handling
…ge of auth-service
- Use select(.kind == "Deployment") to ensure yq only modifies the Deployment - Prevents accidental modification of Service resource in multi-document YAML - Add debugging step to display file contents before apply - Explicitly checkout main branch from k8s-config repo - Matches the fix applied to API Gateway workflow
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 22222403 | Triggered | Username Password | f1d30fe | test-auth-complete.sh | View secret |
| 22222402 | Triggered | Generic High Entropy Secret | 775f9db | auth-service/src/test/resources/application-test.properties | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Warning Rate limit exceeded@RandithaK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (62)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive authentication and user management system for TechTorque 2025, upgrading from 58% completion (14.5/25 endpoints) to 100% completion (25/25 endpoints). The implementation adds critical features including email verification, JWT refresh tokens, password reset, profile management, and user preferences.
Key Changes:
- Email verification system with token-based verification
- JWT refresh token mechanism with 7-day expiry
- Password reset flow with email notifications
- User profile and preferences management
- Enhanced security features including login attempt tracking
Reviewed Changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
auth-service/src/main/java/com/techtorque/auth_service/service/AuthService.java |
Added email verification, refresh token, and password reset functionality |
auth-service/src/main/java/com/techtorque/auth_service/service/TokenService.java |
New service for managing verification and refresh tokens |
auth-service/src/main/java/com/techtorque/auth_service/service/EmailService.java |
New service for sending verification, reset, and welcome emails |
auth-service/src/main/java/com/techtorque/auth_service/service/ProfilePhotoService.java |
New service for managing profile photos with BLOB storage and caching |
auth-service/src/main/java/com/techtorque/auth_service/service/PreferencesService.java |
New service for managing user notification preferences |
auth-service/src/main/java/com/techtorque/auth_service/controller/AuthController.java |
Added 7 new authentication endpoints |
auth-service/src/main/java/com/techtorque/auth_service/controller/UserController.java |
Added 4 new profile/preferences endpoints |
auth-service/src/main/java/com/techtorque/auth_service/controller/ProfilePhotoController.java |
New controller for profile photo management |
auth-service/src/main/java/com/techtorque/auth_service/entity/* |
New entities: RefreshToken, VerificationToken, UserPreferences; updated User entity |
auth-service/src/main/java/com/techtorque/auth_service/config/* |
Added EmailConfig, CacheConfig, GatewayHeaderFilter, CorsFilter |
auth-service/src/main/resources/application.properties |
Added email, token, and CORS configuration |
auth-service/pom.xml |
Added dependencies for email, Guava cache, and dotenv |
test-auth-complete.sh |
Comprehensive test script for all endpoints |
| Documentation | Added IMPLEMENTATION_SUMMARY.md and COMPLETE_IMPLEMENTATION_REPORT.md |
Files not reviewed (1)
- .idea/workspace.xml: Language not supported
Comments suppressed due to low confidence (2)
auth-service/src/main/resources/application.properties:1
- The comment says '1 week deadline' but the code adds 7 days, which is correct. However, this deadline field is set during registration but never enforced - there's no code that checks if this deadline has passed and disables the account. Either implement the enforcement logic or remove this unused field to avoid confusion.
auth-service/src/main/java/com/techtorque/auth_service/entity/User.java:91 - getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
getRoles exposes the internal representation stored in field roles. The value may be modified after this call to getRoles.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String registerUser(RegisterRequest registerRequest) { | ||
| if (userRepository.existsByUsername(registerRequest.getUsername())) { | ||
| throw new RuntimeException("Error: Username is already taken!"); | ||
| } | ||
|
|
||
| if (userRepository.existsByEmail(registerRequest.getEmail())) { | ||
| throw new RuntimeException("Error: Email is already in use!"); | ||
| } | ||
|
|
||
| User user = User.builder() | ||
| .username(registerRequest.getUsername()) | ||
| .username(registerRequest.getEmail()) // Use email as username for simplicity |
There was a problem hiding this comment.
Using email as username removes the username field from RegisterRequest but the validation checks and error messages may be confusing. Consider either: (1) accepting both username and email with a clear API contract, or (2) updating the RegisterRequest DTO to remove the username field entirely if it's not needed. The current approach where username is derived from email but not explicitly communicated in the API design could confuse API consumers.
| .enabled(true) // Allow login without email verification | ||
| .emailVerified(false) // Track verification status separately |
There was a problem hiding this comment.
The comment 'Allow login without email verification' is misleading. Looking at the login logic, the code blocks unverified users from logging in (lines 58-66 check emailVerified and throw exceptions). This comment should be updated to clarify that the user account is created as enabled but requires email verification before login.
| VerificationToken verificationToken = verificationTokenRepository.findByToken(token) | ||
| .orElseThrow(() -> new RuntimeException("Invalid token")); | ||
|
|
||
| if (verificationToken.getTokenType() != tokenType) { | ||
| throw new RuntimeException("Invalid token type"); | ||
| } | ||
|
|
||
| if (verificationToken.isUsed()) { | ||
| throw new RuntimeException("Token has already been used"); | ||
| } | ||
|
|
||
| if (verificationToken.isExpired()) { | ||
| throw new RuntimeException("Token has expired"); | ||
| } |
There was a problem hiding this comment.
Generic RuntimeException messages like 'Invalid token' provide insufficient context for debugging and client error handling. Consider creating custom exceptions (e.g., TokenExpiredException, TokenAlreadyUsedException, InvalidTokenTypeException) or at minimum provide more descriptive error messages that distinguish between different failure scenarios.
| if (!base64Image.matches("^[A-Za-z0-9+/]*={0,2}$")) { | ||
| throw new IllegalArgumentException("Base64 string contains invalid characters"); | ||
| } |
There was a problem hiding this comment.
The regex validation on line 94 will be extremely slow for large base64 strings (up to 5MB of base64 data is allowed, which could be ~7 million characters). This regex has catastrophic backtracking potential. Since Base64.getDecoder().decode() already validates the base64 format and throws IllegalArgumentException for invalid input, this redundant validation should be removed.
| refreshTokenRepository.deleteByUser(user); | ||
|
|
||
| // Delete related verification tokens | ||
| verificationTokenRepository.deleteByUser(user); | ||
|
|
||
| // Delete related login locks | ||
| loginLockRepository.deleteByUsername(username); | ||
|
|
||
| // Delete related login logs | ||
| loginLogRepository.deleteByUsername(username); |
There was a problem hiding this comment.
The deleteUser method manually cascades deletes across multiple tables. This is error-prone and could lead to orphaned records if new related entities are added later. Consider using JPA cascade delete annotations on the User entity relationships (e.g., @onetomany(cascade = CascadeType.ALL, orphanRemoval = true)) to handle this automatically and ensure consistency.
|
|
||
| public static void main(String[] args) { | ||
| // Load environment variables from .env file | ||
| Dotenv dotenv = Dotenv.load(); |
There was a problem hiding this comment.
The Dotenv.load() call will throw an exception if the .env file doesn't exist, causing the application to fail to start. This is problematic for production deployments where environment variables are typically provided by the container/orchestration platform (not via .env file). Wrap this in try-catch or use Dotenv.configure().ignoreIfMissing().load() to gracefully handle missing .env files.
| Dotenv dotenv = Dotenv.load(); | |
| Dotenv dotenv = Dotenv.configure().ignoreIfMissing().load(); |
| userService.changeUserPassword(username, changeRequest.getCurrentPassword(), changeRequest.getNewPassword()); | ||
| return ResponseEntity.ok(ApiSuccess.of("Password changed successfully")); | ||
| } catch (RuntimeException e) { | ||
| return ResponseEntity.badRequest().body(ApiSuccess.of("Error: " + e.getMessage())); |
There was a problem hiding this comment.
Returning an ApiSuccess object for an error response is semantically incorrect. The response should use ApiError instead. This inconsistency will confuse API consumers who expect error responses to follow a different structure than success responses.
| class PhotoMetadata { | ||
| public Long size; | ||
| public String mimeType; | ||
| public Long lastUpdated; | ||
|
|
||
| public PhotoMetadata(Long size, String mimeType, Long lastUpdated) { | ||
| this.size = size; | ||
| this.mimeType = mimeType; | ||
| this.lastUpdated = lastUpdated; | ||
| } | ||
| } |
There was a problem hiding this comment.
The PhotoMetadata class has public fields without encapsulation. This is poor practice in Java. Use Lombok's @DaTa annotation or create proper getters/setters. Alternatively, use the existing ProfilePhotoCacheEntry DTO which already has this same structure with proper encapsulation.
| public static final long MIN_IMAGE_SIZE = 1024; // 1KB minimum | ||
| public static final long MAX_IMAGE_SIZE = 5_242_880; // 5MB maximum |
There was a problem hiding this comment.
[nitpick] Magic number constants for size limits are defined in the service class but should be externalized to application.properties for easier configuration across environments (dev, staging, prod). Consider using @value annotations to inject configurable limits like 'app.profile-photo.min-size' and 'app.profile-photo.max-size'.
| echo "Look for: 'Verification token for $USERNAME: YOUR_TOKEN'" | ||
| echo "Since email is disabled by default, token is logged to console" | ||
| echo "" | ||
| read -p "Enter verification token from logs (or press Enter to skip): " VERIFY_TOKEN |
There was a problem hiding this comment.
[nitpick] The test script requires manual intervention to copy tokens from logs, which breaks automation. Consider either: (1) parsing the token directly from service logs using grep/sed, (2) providing a test mode that returns tokens in API responses, or (3) documenting this as a limitation and providing an automated alternative script for CI/CD pipelines.
No description provided.