Conversation
WalkthroughThe pull request modifies CI/CD workflow trigger configuration and expands authorization controls across time-logging service endpoints. It adds SUPER_ADMIN role access to multiple endpoints, implements header-based role extraction for role-aware request filtering, and introduces validation for missing authentication headers with exception handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant AuthCheck
Client->>Controller: DELETE /timeLog/:id<br/>with X-User-Subject & X-User-Roles headers
Controller->>AuthCheck: `@PreAuthorize` check<br/>(EMPLOYEE | ADMIN | SUPER_ADMIN)
rect rgb(220, 240, 255)
Note over AuthCheck: Authorization granted
AuthCheck-->>Controller: Authorized
end
Controller->>Controller: Extract employeeId<br/>from X-User-Subject header
alt employeeId present
Controller->>Controller: Proceed with deletion
Controller-->>Client: 200 OK / Deletion successful
else employeeId missing
rect rgb(255, 220, 220)
Note over Controller: Guard clause triggered
Controller->>Controller: Throw UnauthorizedAccessException
end
Controller-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 adds SUPER_ADMIN role support to the time logging service and refines the GitHub Actions workflow. The changes expand authorization rules to include the new SUPER_ADMIN role alongside existing EMPLOYEE and ADMIN roles, while also modifying the CI/CD pipeline to trigger only on pull requests to specific branches.
- Added SUPER_ADMIN role to authorization rules across all endpoints
- Updated delete endpoint to accept optional user role parameter
- Modified GitHub Actions workflow to run only on pull requests to main, dev, and devOps branches
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| TimeLogController.java | Added SUPER_ADMIN to PreAuthorize annotations, made delete endpoint parameters optional, and added validation for missing user identification |
| buildtest.yaml | Changed workflow triggers from all pushes/PRs to only PRs targeting main, dev, and devOps branches |
Comments suppressed due to low confidence (1)
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java:189
- The description states 'Employees can only delete their own logs' but the PreAuthorize annotation now includes 'ADMIN' and 'SUPER_ADMIN' roles. The documentation should be updated to reflect that admins and super admins can also access this endpoint, or the authorization logic should be implemented to allow them to delete any log.
description = "Delete a time log entry. Employees can only delete their own logs."
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new UnauthorizedAccessException("User identification missing. Please ensure you are logged in."); | ||
| } | ||
|
|
||
| timeLogService.deleteTimeLogWithAuthorization(logId, employeeId); |
There was a problem hiding this comment.
The userRoles parameter is added but never used in the method. The comment on line 207 suggests checking admin role, but the service method deleteTimeLogWithAuthorization only checks employee ownership and doesn't support admin bypass logic like getTimeLogByIdWithAuthorization does. Either remove the unused parameter or implement role-based authorization to allow admins/super admins to delete any log without ownership checks.
| timeLogService.deleteTimeLogWithAuthorization(logId, employeeId); | |
| boolean isAdmin = false; | |
| if (userRoles != null && !userRoles.isEmpty()) { | |
| String[] roles = userRoles.split(","); | |
| for (String role : roles) { | |
| String trimmedRole = role.trim().toUpperCase(); | |
| if (trimmedRole.equals("ADMIN") || trimmedRole.equals("SUPER_ADMIN")) { | |
| isAdmin = true; | |
| break; | |
| } | |
| } | |
| } | |
| if (isAdmin) { | |
| // Admins can delete any log | |
| timeLogService.deleteTimeLogAsAdmin(logId); | |
| } else { | |
| // Employees can only delete their own logs | |
| timeLogService.deleteTimeLogWithAuthorization(logId, employeeId); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java (2)
93-100: Header name inconsistency: X-User-Roles vs X-User-Role.Line 96 uses
X-User-Roles(plural), line 149 usesX-User-Role(singular), and line 205 usesX-User-Roles(plural). This inconsistency will cause the header to be missing in getTimeLogById, potentially breaking authorization logic. Standardize on either the singular or plural form across all endpoints.Apply this diff to standardize on
X-User-Roles(plural):@GetMapping("/{logId}") @PreAuthorize("hasAnyRole('EMPLOYEE', 'ADMIN', 'SUPER_ADMIN')") public ResponseEntity<TimeLogResponse> getTimeLogById( @Parameter(description = "Time log ID", required = true) @PathVariable String logId, @Parameter(description = "User ID from authentication token") @RequestHeader(value = "X-User-Subject", required = false) String userId, @Parameter(description = "User role from authentication token") - @RequestHeader(value = "X-User-Role", required = false) String userRole) { + @RequestHeader(value = "X-User-Roles", required = false) String userRole) {
104-121: Use proper role parsing instead of string contains.Line 105 uses
roles.contains("ADMIN")which performs substring matching. This is vulnerable to false positives if role names contain these substrings (e.g., "NOT_ADMIN" would match). Consider parsing the roles string into a proper collection or using exact role matching with delimiters.Example fix:
- if (roles.contains("ADMIN") || roles.contains("SUPER_ADMIN")) { + List<String> roleList = Arrays.asList(roles.split(",")); + if (roleList.contains("ADMIN") || roleList.contains("SUPER_ADMIN")) {Note: This assumes comma-separated roles. Verify the actual format from your authentication system.
🧹 Nitpick comments (1)
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java (1)
106-110: Optimize date filtering by delegating to service layer.Lines 108-110 fetch all time logs and filter in-memory, which is inefficient for large datasets. Consider adding a service method like
getAllTimeLogsByDateRange(from, to)to perform filtering at the database level.In the service layer, add:
public List<TimeLogResponse> getAllTimeLogsByDateRange(LocalDate from, LocalDate to) { // Implement database-level filtering }Then update the controller:
- responses = timeLogService.getAllTimeLogs().stream() - .filter(log -> !log.getDate().isBefore(from) && !log.getDate().isAfter(to)) - .collect(java.util.stream.Collectors.toList()); + responses = timeLogService.getAllTimeLogsByDateRange(from, to);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/buildtest.yaml(1 hunks)time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java (1)
time-logging-service/src/main/java/com/techtorque/time_logging_service/exception/UnauthorizedAccessException.java (1)
UnauthorizedAccessException(9-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Push Docker Image
- GitHub Check: CodeQL analysis (java)
🔇 Additional comments (5)
.github/workflows/buildtest.yaml (1)
4-8: LGTM: Workflow trigger configuration is appropriate.The pull request trigger configuration for main, dev, and devOps branches aligns with a standard multi-branch development workflow.
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java (4)
7-7: LGTM: Import added for new exception handling.The UnauthorizedAccessException import is correctly added to support the enhanced authorization checks in deleteTimeLog.
170-181: LGTM: Update endpoint properly restricts access.The updateTimeLog endpoint correctly requires employee identification and delegates authorization to the service layer. The SUPER_ADMIN addition is consistent with other endpoints.
256-268: Verify intended behavior for SUPER_ADMIN in summary endpoint.SUPER_ADMIN is granted access but can only retrieve their own summary (using their employeeId from the auth header). If SUPER_ADMIN should be able to retrieve summaries for any employee, consider adding an optional query parameter for target employeeId and implementing role-based logic similar to getMyTimeLogs.
298-306: Same limitation applies to stats endpoint.Similar to getSummary, SUPER_ADMIN can only retrieve their own statistics. Verify if this is the intended behavior or if admins should have broader access.
| }) | ||
| @PostMapping | ||
| @PreAuthorize("hasRole('EMPLOYEE')") | ||
| @PreAuthorize("hasAnyRole('EMPLOYEE', 'ADMIN', 'SUPER_ADMIN')") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify SUPER_ADMIN role inclusion across all endpoints.
SUPER_ADMIN has been added consistently to most endpoints, which aligns with granting elevated privileges. However, lines 231 and 281 (getTimeLogsForService and getTimeLogsForProject) still use @PreAuthorize("hasAnyRole('CUSTOMER', 'EMPLOYEE', 'ADMIN')") without SUPER_ADMIN. Confirm whether this exclusion is intentional or an oversight.
Run the following script to identify all @PreAuthorize annotations and verify consistency:
Also applies to: 92-92, 142-142, 171-171, 198-198, 257-257, 299-299
🏁 Script executed:
#!/bin/bash
# Description: Find all @PreAuthorize annotations to check for SUPER_ADMIN consistency
# Search for all @PreAuthorize annotations in the controller
rg -n '@PreAuthorize' time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java -A1 -B2Length of output: 1720
Add SUPER_ADMIN to service/project-level time log queries for consistency.
The SUPER_ADMIN role has been correctly added to employee-focused endpoints (createTimeLog, getMyTimeLogs, getTimeLogById, updateTimeLog, deleteTimeLog, getSummary, getEmployeeStats). However, lines 231 and 281 still lack SUPER_ADMIN:
- Line 231:
getTimeLogsForService-@PreAuthorize("hasAnyRole('CUSTOMER', 'EMPLOYEE', 'ADMIN')") - Line 281:
getTimeLogsForProject-@PreAuthorize("hasAnyRole('CUSTOMER', 'EMPLOYEE', 'ADMIN')")
SUPER_ADMIN should have access to all time log data, including service and project-level queries. Add 'SUPER_ADMIN' to both annotations.
🤖 Prompt for AI Agents
In
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java
around lines 231 and 281, the @PreAuthorize annotations for
getTimeLogsForService and getTimeLogsForProject currently allow CUSTOMER,
EMPLOYEE, and ADMIN but omit SUPER_ADMIN; update both annotations to include
'SUPER_ADMIN' so they read
hasAnyRole('CUSTOMER','EMPLOYEE','ADMIN','SUPER_ADMIN') to grant super admin
access to service- and project-level time log queries.
| @Parameter(description = "Employee ID from authentication token", required = false) | ||
| @RequestHeader(value = "X-User-Subject", required = false) String employeeId, | ||
| @Parameter(description = "User role from authentication token", required = false) | ||
| @RequestHeader(value = "X-User-Roles", required = false) String userRoles) { | ||
|
|
||
| // If header is missing, check if user is admin (can delete any log) | ||
| if (employeeId == null || employeeId.isEmpty()) { | ||
| throw new UnauthorizedAccessException("User identification missing. Please ensure you are logged in."); | ||
| } |
There was a problem hiding this comment.
Incomplete authorization logic and unused parameter.
Several issues with this implementation:
- Misleading comment: Line 207 says "check if user is admin" but no admin check is performed.
- Unused parameter:
userRolesis accepted but never used, making the header acceptance pointless. - Logic contradiction: If employeeId is always required (lines 208-210), why make the header optional?
- Missing admin privilege: If SUPER_ADMIN/ADMIN should be able to delete any log without ownership checks, this logic is missing.
Clarify the intended behavior. If admins should be able to delete any log:
@Parameter(description = "Employee ID from authentication token", required = false)
@RequestHeader(value = "X-User-Subject", required = false) String employeeId,
@Parameter(description = "User role from authentication token", required = false)
@RequestHeader(value = "X-User-Roles", required = false) String userRoles) {
- // If header is missing, check if user is admin (can delete any log)
- if (employeeId == null || employeeId.isEmpty()) {
- throw new UnauthorizedAccessException("User identification missing. Please ensure you are logged in.");
- }
+ // Parse roles for admin check
+ boolean isAdmin = userRoles != null &&
+ (Arrays.asList(userRoles.split(",")).contains("ADMIN") ||
+ Arrays.asList(userRoles.split(",")).contains("SUPER_ADMIN"));
+
+ // If not admin, employeeId is required
+ if (!isAdmin && (employeeId == null || employeeId.isEmpty())) {
+ throw new UnauthorizedAccessException("User identification missing. Please ensure you are logged in.");
+ }
- timeLogService.deleteTimeLogWithAuthorization(logId, employeeId);
+ // Pass role info to service layer for proper authorization
+ timeLogService.deleteTimeLogWithAuthorization(logId, employeeId, isAdmin);Note: You'll need to update the service method signature accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
time-logging-service/src/main/java/com/techtorque/time_logging_service/controller/TimeLogController.java
around lines 202 to 210, the authorization logic is incomplete: the comment says
to check admin but no check is implemented, userRoles parameter is unused,
employeeId is treated as required despite the header being marked optional, and
admins are not allowed to bypass ownership checks. Fix by treating userRoles
header as the source of roles (parse comma-separated roles, trim and compare
case-insensitively to "ADMIN" or "SUPER_ADMIN"); change the null/empty
employeeId check to only throw UnauthorizedAccessException when employeeId is
missing AND the parsed roles do NOT include ADMIN or SUPER_ADMIN; if the caller
is an admin allow the operation without an employeeId; and update any downstream
service method signature or call to accept the resolved actingUserId/role
context (or perform the admin-vs-owner decision in the controller before calling
the service). Ensure header annotations reflect whether employeeId can be
optional and document that admin role allows global deletes.
Summary by CodeRabbit
Chores
New Features
Improvements