Conversation
Mrr integration
…te API base URL configuration
Randitha superbranch
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 22346740 | Triggered | Username Password | 97fca6b | COMPLETE_FIX.sh | 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 secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- 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 1 minutes and 56 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 (15)
✨ 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 migration from direct JavaMail-based email sending to a gRPC-based notification service architecture, enforces email verification before login, and addresses user role assignment issues. The changes centralize email handling, improve security by requiring verified accounts, and fix database seeding problems.
Key changes:
- Migration from JavaMail to gRPC notification service for all transactional emails
- Enforcement of email verification requirement before user login
- Database role assignment fixes via SQL scripts and improved cascade configuration
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| fix_user_roles.sql | Provides SQL script to diagnose and fix missing CUSTOMER role assignments |
| check_user_roles.sql | Query script to verify user role assignments in the database |
| application.properties | Adds gRPC client configuration for notification service; documents CORS handled by gateway |
| email.proto | Defines gRPC service contract for transactional email delivery |
| EmailService.java | Replaces JavaMail implementation with gRPC client calls to notification service |
| AuthService.java | Enforces email verification by checking enabled flag and providing clear error messages |
| User.java | Adds cascade settings to ensure role persistence during user operations |
| UserController.java | Adds role filtering capability and authorization to role management endpoint |
| GatewayHeaderFilter.java | Updates filter to prioritize JWT tokens over gateway headers for direct service calls |
| EmailConfig.java | Removes explicit bean definition as @service annotation handles registration |
| DataSeeder.java | Adds @transactional annotation and production-safe logic to prevent duplicate superadmin |
| CorsFilter.java | Explicitly disables and deprecates filter since CORS is handled by API Gateway |
| pom.xml | Adds gRPC dependencies and protobuf Maven plugin configuration |
| LOGIN_ISSUE_FIX.md | Comprehensive troubleshooting guide for empty roles issue |
| COMPLETE_FIX.sh | Automated script to fix role assignments and guide service restart |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .phone(registerRequest.getPhone()) | ||
| .address(registerRequest.getAddress()) | ||
| .enabled(true) // Allow login without email verification | ||
| .enabled(false) // Require email verification before login |
There was a problem hiding this comment.
Setting enabled(false) for new users will break login for all existing users in the database who were created with enabled(true). This is a breaking change that requires a database migration to set existing verified users to enabled(true). Consider adding a migration script or checking emailVerified status when setting the initial enabled value.
| -- Summary | ||
| SELECT 'SUMMARY:' as status; | ||
| SELECT u.username, | ||
| ARRAY_AGG(r.name ORDER BY r.name) as roles |
There was a problem hiding this comment.
The script uses PostgreSQL-specific syntax (ARRAY_AGG, line 89) but the codebase may support MySQL/MariaDB (referenced in comments at line 41-42). ARRAY_AGG doesn't exist in MySQL; use GROUP_CONCAT instead for MySQL compatibility, or detect the database type and use conditional SQL.
| private Environment env; | ||
|
|
||
| @Override | ||
| @Transactional // <-- FIX: Ensures all seeding runs in one transaction |
There was a problem hiding this comment.
Adding @Transactional to CommandLineRunner.run() can cause issues because it runs during application startup before transaction management is fully initialized. Consider using @Transactional on individual methods (seedRoles(), seedUsers()) instead, or use programmatic transaction management via TransactionTemplate.
| WHERE u.username = 'customer'; | ||
|
|
||
| -- Check all users and their roles | ||
| SELECT u.username, u.email, GROUP_CONCAT(r.name) as roles |
There was a problem hiding this comment.
Using GROUP_CONCAT which is MySQL-specific. This won't work with PostgreSQL (which uses STRING_AGG or ARRAY_AGG). Since COMPLETE_FIX.sh uses PostgreSQL syntax, ensure database consistency or provide both SQL variants.
| SELECT u.username, u.email, GROUP_CONCAT(r.name) as roles | |
| SELECT u.username, u.email, STRING_AGG(r.name, ',') as roles |
|
|
||
| // This is the other side of the relationship | ||
| @ManyToMany(fetch = FetchType.EAGER) | ||
| @ManyToMany(fetch = FetchType.EAGER, cascade = {CascadeType.PERSIST, CascadeType.MERGE}) |
There was a problem hiding this comment.
Adding CascadeType.MERGE to the roles relationship may cause unintended side effects. When a User is merged, attached Role entities will also be merged, potentially updating role data if they've been modified. Since roles are typically system-wide entities, consider removing CascadeType.MERGE and only keeping CascadeType.PERSIST if needed.
| @ManyToMany(fetch = FetchType.EAGER, cascade = {CascadeType.PERSIST, CascadeType.MERGE}) | |
| @ManyToMany(fetch = FetchType.EAGER, cascade = {CascadeType.PERSIST}) |
| List<UserDto> users = userService.findAllUsers().stream() | ||
| .map(this::convertToDto) | ||
| .filter(user -> role == null || user.getRoles().contains(role)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Filtering by role in-memory after fetching all users from the database is inefficient. Consider adding a database-level query method findByRole(String role) to the repository/service layer to filter at the SQL level, especially if the user count grows large.
| List<UserDto> users = userService.findAllUsers().stream() | |
| .map(this::convertToDto) | |
| .filter(user -> role == null || user.getRoles().contains(role)) | |
| .collect(Collectors.toList()); | |
| List<UserDto> users; | |
| if (role == null) { | |
| users = userService.findAllUsers().stream() | |
| .map(this::convertToDto) | |
| .collect(Collectors.toList()); | |
| } else { | |
| users = userService.findUsersByRole(role).stream() | |
| .map(this::convertToDto) | |
| .collect(Collectors.toList()); | |
| } |
| request.getRequestURI(), hasJwtToken, userId, rolesHeader); | ||
|
|
||
| if (userId != null && !userId.isEmpty()) { | ||
| // Only use gateway headers if there's no JWT token present |
There was a problem hiding this comment.
The logic change to prioritize JWT tokens over gateway headers is significant but lacks documentation about edge cases. What happens if both JWT and gateway headers are present but contain conflicting user information? Consider adding a comment explaining the precedence rule and potential security implications.
| // Only use gateway headers if there's no JWT token present | |
| // Precedence rule: If both a JWT Authorization header and gateway headers (X-User-Subject, X-User-Roles) are present, | |
| // the JWT token takes precedence and gateway headers are ignored. This prevents potential header spoofing attacks, | |
| // as only a validated JWT is trusted when available. Gateway headers are only used for authentication if no JWT is present. |
| * POST /api/v1/users/{username}/roles | ||
| */ | ||
| @PostMapping("/{username}/roles") | ||
| @PreAuthorize("hasRole('ADMIN') or hasRole('SUPER_ADMIN')") |
There was a problem hiding this comment.
[nitpick] The authorization expression is duplicated as a string literal. Consider extracting this to a constant (e.g., SecurityConstants.ADMIN_OR_SUPER_ADMIN) to ensure consistency across endpoints and make future updates easier.
No description provided.