Conversation
…update user request handling
…the admin service
…e; update appointment handling and UI for better clarity and user experience
Randitha superbranch
|
Warning Rate limit exceeded@RandithaK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 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 (11)
✨ 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 JWT-based authentication for the admin service, enabling direct service-to-service communication and public API access. The changes include JWT token validation, token propagation across microservices, and refactoring of user and service type management.
- Added JWT authentication filter and token propagation for WebClient calls
- Refactored user update logic to handle complex role management and status updates
- Changed service type deletion from soft to hard delete and updated response field names
- Created public endpoints for service types accessible by all authenticated users
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| admin-service/src/main/resources/application.properties | Added JWT secret configuration property |
| admin-service/src/main/java/com/techtorque/admin_service/service/impl/AdminUserServiceImpl.java | Refactored getUserById and updateUser methods with new role management logic |
| admin-service/src/main/java/com/techtorque/admin_service/service/impl/AdminServiceConfigServiceImpl.java | Changed service type deletion from soft delete to hard delete |
| admin-service/src/main/java/com/techtorque/admin_service/dto/response/ServiceTypeResponse.java | Renamed price and duration fields to match frontend expectations |
| admin-service/src/main/java/com/techtorque/admin_service/dto/request/UpdateUserRequest.java | Added roles list field and activation status helper method |
| admin-service/src/main/java/com/techtorque/admin_service/controller/PublicServiceTypeController.java | Created new public controller for unauthenticated service type access |
| admin-service/src/main/java/com/techtorque/admin_service/config/WebClientConfig.java | Added JWT token propagation filter to all WebClient beans |
| admin-service/src/main/java/com/techtorque/admin_service/config/SecurityConfig.java | Integrated JWT authentication filter and defined public endpoints |
| admin-service/src/main/java/com/techtorque/admin_service/config/JwtAuthenticationFilter.java | Implemented JWT token validation and authentication |
| admin-service/pom.xml | Added JWT and WebFlux dependencies |
| .github/workflows/buildtest.yaml | Updated workflow triggers to specific branches only |
Comments suppressed due to low confidence (1)
admin-service/src/main/java/com/techtorque/admin_service/service/impl/AdminUserServiceImpl.java:243
- The
getUserByIdandupdateUsermethods have significantly changed behavior but lack test coverage. Since the file has no corresponding test class, these critical changes (especially the performance-impacting getAllUsers call and complex role management logic) should have unit tests to verify correctness.
public UserResponse getUserById(String userId) {
log.info("Fetching user by ID: {} from auth service", userId);
try {
// Auth service endpoints use username, not userId
// We need to first get all users and find the one with matching ID
List<UserResponse> allUsers = getAllUsers(null, null, 0, 1000);
UserResponse user = allUsers.stream()
.filter(u -> {
String userIdStr = u.getUserId() != null ? u.getUserId() : String.valueOf(u.getId());
return userIdStr.equals(userId);
})
.findFirst()
.orElseThrow(() -> new RuntimeException("User not found with ID: " + userId));
log.info("Found user: {} with username: {}", userId, user.getUsername());
return user;
} catch (Exception e) {
log.error("Error fetching user: {}", userId, e);
throw new RuntimeException("User not found: " + userId);
}
}
@Override
public UserResponse createEmployee(CreateEmployeeRequest request) {
log.info("Creating employee: {} via auth service", request.getEmail());
try {
UserResponse response = authServiceWebClient.post()
.uri("/users/employee")
.bodyValue(request)
.retrieve()
.bodyToMono(UserResponse.class)
.block();
return response;
} catch (Exception e) {
log.error("Error creating employee", e);
throw new RuntimeException("Failed to create employee: " + e.getMessage());
}
}
@Override
public UserResponse createAdmin(CreateEmployeeRequest request) {
log.info("Creating admin: {} via auth service", request.getEmail());
try {
UserResponse response = authServiceWebClient.post()
.uri("/users/admin")
.bodyValue(request)
.retrieve()
.bodyToMono(UserResponse.class)
.block();
return response;
} catch (Exception e) {
log.error("Error creating admin", e);
throw new RuntimeException("Failed to create admin: " + e.getMessage());
}
}
@Override
public UserResponse updateUser(String userId, UpdateUserRequest request) {
log.info("Updating user: {} via auth service", userId);
try {
// First, get the current user details to obtain the username
UserResponse currentUser = getUserById(userId);
String username = currentUser.getUsername();
// Handle role updates separately via the roles endpoint
if (request.getRoles() != null || request.getRole() != null) {
// Get current roles
List<String> currentRoles = currentUser.getRoles() != null ? currentUser.getRoles() : new java.util.ArrayList<>();
List<String> newRoles = new java.util.ArrayList<>();
// Build the new role list - prioritize roles array over single role
if (request.getRoles() != null && !request.getRoles().isEmpty()) {
newRoles.addAll(request.getRoles());
} else if (request.getRole() != null) {
newRoles.add(request.getRole());
}
// Always preserve CUSTOMER role if it exists
if (currentRoles.contains("CUSTOMER")) {
if (!newRoles.contains("CUSTOMER")) {
newRoles.add("CUSTOMER");
}
}
// Always preserve SUPER_ADMIN role if it exists (cannot be removed via this endpoint)
if (currentRoles.contains("SUPER_ADMIN")) {
if (!newRoles.contains("SUPER_ADMIN")) {
newRoles.add("SUPER_ADMIN");
}
}
// Determine which roles to add and which to remove
List<String> rolesToAdd = new java.util.ArrayList<>();
List<String> rolesToRemove = new java.util.ArrayList<>();
// Find roles to add
for (String role : newRoles) {
if (!currentRoles.contains(role)) {
rolesToAdd.add(role);
}
}
// Find roles to remove (only remove EMPLOYEE and ADMIN, never CUSTOMER or SUPER_ADMIN)
for (String role : currentRoles) {
if (!newRoles.contains(role) && (role.equals("EMPLOYEE") || role.equals("ADMIN"))) {
rolesToRemove.add(role);
}
}
// Apply role changes
for (String roleToAdd : rolesToAdd) {
log.info("Assigning role {} to user {}", roleToAdd, username);
java.util.Map<String, Object> roleRequest = new java.util.HashMap<>();
roleRequest.put("roleName", roleToAdd);
roleRequest.put("action", "ASSIGN");
authServiceWebClient.post()
.uri("/users/" + username + "/roles")
.bodyValue(roleRequest)
.retrieve()
.bodyToMono(Void.class)
.block();
}
for (String roleToRemove : rolesToRemove) {
log.info("Revoking role {} from user {}", roleToRemove, username);
java.util.Map<String, Object> roleRequest = new java.util.HashMap<>();
roleRequest.put("roleName", roleToRemove);
roleRequest.put("action", "REVOKE");
authServiceWebClient.post()
.uri("/users/" + username + "/roles")
.bodyValue(roleRequest)
.retrieve()
.bodyToMono(Void.class)
.block();
}
}
// Handle other updates (active status, department, etc.)
Boolean activationStatus = request.getActivationStatus();
if (activationStatus != null || request.getDepartment() != null) {
java.util.Map<String, Object> updateRequest = new java.util.HashMap<>();
if (activationStatus != null) {
updateRequest.put("enabled", activationStatus);
}
if (updateRequest.size() > 0) {
authServiceWebClient.put()
.uri("/users/" + username)
.bodyValue(updateRequest)
.retrieve()
.bodyToMono(Void.class)
.block();
}
}
// Return the updated user
return getUserById(userId);
} catch (Exception e) {
log.error("Error updating user: {}", userId, e);
throw new RuntimeException("Failed to update user: " + e.getMessage());
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Component | ||
| public class JwtAuthenticationFilter extends OncePerRequestFilter { | ||
|
|
||
| @Value("${jwt.secret:mysecretkey}") |
There was a problem hiding this comment.
The hardcoded default JWT secret 'mysecretkey' in the filter differs from the default in application.properties. Both should use the same default value, or better yet, the filter should reference the same property key without a default.
| @Value("${jwt.secret:mysecretkey}") | |
| @Value("${jwt.secret}") |
| // Auth service endpoints use username, not userId | ||
| // We need to first get all users and find the one with matching ID |
There was a problem hiding this comment.
[nitpick] The comment states "Auth service endpoints use username, not userId" but this is misleading. The real issue is that this service's API uses userId while the auth service likely doesn't have a direct "get by ID" endpoint. The comment should be clearer about the actual limitation to avoid confusion.
| // Auth service endpoints use username, not userId | |
| // We need to first get all users and find the one with matching ID | |
| // The auth service does not provide a direct "get user by ID" endpoint; | |
| // therefore, we must fetch all users and filter by userId in this service. |
| private BigDecimal basePriceLKR; // Changed from 'price' to match frontend | ||
| private Integer estimatedDurationMinutes; // Changed from 'durationMinutes' to match frontend |
There was a problem hiding this comment.
The response DTO field names were changed to basePriceLKR and estimatedDurationMinutes, but the request DTOs (CreateServiceTypeRequest and UpdateServiceTypeRequest) still use the old field names price and durationMinutes. This creates an inconsistency in the API where request and response field names don't match, which can confuse API consumers. Consider updating the request DTOs to use the same field names or providing clear documentation about this discrepancy.
| # Development/Production Profile | ||
| spring.profiles.active=${SPRING_PROFILE:dev} | ||
|
|
||
| # JWT Configuration (must match Auth Service secret) |
There was a problem hiding this comment.
The default JWT secret 'YourSuperSecretKeyForJWTGoesHereAndItMustBeVeryLongForSecurityPurposes' is exposed in the properties file and should never be used in production. Consider adding a comment warning that this MUST be overridden in production environments and should not be committed to version control.
| # JWT Configuration (must match Auth Service secret) | |
| # JWT Configuration (must match Auth Service secret) | |
| # WARNING: The default JWT secret below is INSECURE and MUST be overridden in production! | |
| # Do NOT use this value in production environments. | |
| # Set the JWT_SECRET environment variable to a strong, unique value in production. | |
| # This file and its secrets should not be committed to version control. |
| } | ||
| // Auth service endpoints use username, not userId | ||
| // We need to first get all users and find the one with matching ID | ||
| List<UserResponse> allUsers = getAllUsers(null, null, 0, 1000); |
There was a problem hiding this comment.
Fetching all users with a hardcoded limit of 1000 to find one user by ID is inefficient and will not scale. This creates a performance bottleneck and will fail if there are more than 1000 users. Consider either: (1) adding a proper API endpoint in the auth service to fetch by ID, or (2) implementing pagination to handle all users if necessary.
| ServletRequestAttributes attributes = (ServletRequestAttributes) RequestContextHolder.getRequestAttributes(); | ||
| if (attributes != null) { | ||
| HttpServletRequest request = attributes.getRequest(); | ||
| String authHeader = request.getHeader(HttpHeaders.AUTHORIZATION); | ||
| if (authHeader != null && authHeader.startsWith("Bearer ")) { | ||
| return Mono.just( | ||
| org.springframework.web.reactive.function.client.ClientRequest | ||
| .from(clientRequest) | ||
| .header(HttpHeaders.AUTHORIZATION, authHeader) | ||
| .build() | ||
| ); | ||
| } | ||
| } | ||
| return Mono.just(clientRequest); |
There was a problem hiding this comment.
[nitpick] The JWT token propagation filter silently fails and returns the original request if RequestContextHolder.getRequestAttributes() returns null or if there's no Authorization header. This will occur during async operations or when called outside the HTTP request thread. Consider logging a debug message when the token cannot be propagated to help troubleshoot authentication issues in service-to-service calls.
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-oauth2-resource-server</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
[nitpick] The spring-boot-starter-oauth2-resource-server dependency is added but not used in the configuration. The JWT authentication is implemented manually via JwtAuthenticationFilter using the jjwt library. Consider either using the OAuth2 resource server configuration properly or removing this unused dependency to avoid confusion and reduce the application size.
| <groupId>org.springframework.boot</groupId> | |
| <artifactId>spring-boot-starter-oauth2-resource-server</artifactId> | |
| </dependency> |
| List<String> currentRoles = currentUser.getRoles() != null ? currentUser.getRoles() : new java.util.ArrayList<>(); | ||
| List<String> newRoles = new java.util.ArrayList<>(); | ||
|
|
||
| // Build the new role list - prioritize roles array over single role | ||
| if (request.getRoles() != null && !request.getRoles().isEmpty()) { | ||
| newRoles.addAll(request.getRoles()); | ||
| } else if (request.getRole() != null) { | ||
| newRoles.add(request.getRole()); | ||
| } | ||
|
|
||
| // Always preserve CUSTOMER role if it exists | ||
| if (currentRoles.contains("CUSTOMER")) { | ||
| if (!newRoles.contains("CUSTOMER")) { | ||
| newRoles.add("CUSTOMER"); | ||
| } | ||
| } | ||
|
|
||
| // Always preserve SUPER_ADMIN role if it exists (cannot be removed via this endpoint) | ||
| if (currentRoles.contains("SUPER_ADMIN")) { | ||
| if (!newRoles.contains("SUPER_ADMIN")) { | ||
| newRoles.add("SUPER_ADMIN"); | ||
| } | ||
| } | ||
|
|
||
| // Determine which roles to add and which to remove | ||
| List<String> rolesToAdd = new java.util.ArrayList<>(); | ||
| List<String> rolesToRemove = new java.util.ArrayList<>(); | ||
|
|
||
| // Find roles to add | ||
| for (String role : newRoles) { | ||
| if (!currentRoles.contains(role)) { | ||
| rolesToAdd.add(role); | ||
| } | ||
| } | ||
|
|
||
| // Find roles to remove (only remove EMPLOYEE and ADMIN, never CUSTOMER or SUPER_ADMIN) | ||
| for (String role : currentRoles) { | ||
| if (!newRoles.contains(role) && (role.equals("EMPLOYEE") || role.equals("ADMIN"))) { | ||
| rolesToRemove.add(role); | ||
| } | ||
| } | ||
|
|
||
| // Apply role changes | ||
| for (String roleToAdd : rolesToAdd) { | ||
| log.info("Assigning role {} to user {}", roleToAdd, username); | ||
| java.util.Map<String, Object> roleRequest = new java.util.HashMap<>(); | ||
| roleRequest.put("roleName", roleToAdd); | ||
| roleRequest.put("action", "ASSIGN"); | ||
|
|
||
| authServiceWebClient.post() | ||
| .uri("/users/" + username + "/roles") | ||
| .bodyValue(roleRequest) | ||
| .retrieve() | ||
| .bodyToMono(Void.class) | ||
| .block(); | ||
| } | ||
|
|
||
| for (String roleToRemove : rolesToRemove) { | ||
| log.info("Revoking role {} from user {}", roleToRemove, username); | ||
| java.util.Map<String, Object> roleRequest = new java.util.HashMap<>(); |
There was a problem hiding this comment.
Using fully qualified class names (java.util.ArrayList, java.util.HashMap) instead of proper imports makes the code harder to read and is not idiomatic Java. Add proper imports at the top of the file: import java.util.ArrayList; and import java.util.HashMap; and use the simple class names.
| @Pattern(regexp = "ADMIN|EMPLOYEE|CUSTOMER", message = "Role must be ADMIN, EMPLOYEE, or CUSTOMER") | ||
| private String role; | ||
|
|
||
| private List<String> roles; |
There was a problem hiding this comment.
The roles field (line 27) has no validation, while the singular role field has a pattern constraint. If roles can contain values other than "ADMIN", "EMPLOYEE", or "CUSTOMER", this could lead to invalid role assignments. Consider adding validation to ensure all items in the roles list match the allowed values.
| // Hard delete - actually remove from database | ||
| serviceTypeRepository.delete(serviceType); | ||
|
|
||
| log.info("Service type deleted successfully: {}", id); |
There was a problem hiding this comment.
Changing from soft delete to hard delete is a breaking change that could cause data integrity issues. If there are any appointments, projects, or other entities referencing this service type, the hard delete will fail with a foreign key constraint violation or leave orphaned references. Consider either: (1) keeping soft delete, (2) adding cascade delete logic, or (3) validating that no references exist before deletion.
| // Hard delete - actually remove from database | |
| serviceTypeRepository.delete(serviceType); | |
| log.info("Service type deleted successfully: {}", id); | |
| // Soft delete - mark as inactive instead of removing from database | |
| serviceType.setActive(false); | |
| serviceType.setUpdatedAt(java.time.LocalDateTime.now()); | |
| serviceTypeRepository.save(serviceType); | |
| log.info("Service type soft-deleted (marked inactive) successfully: {}", id); |
No description provided.