Enhancement/refactor security in employment - #65
Merged
codebyNorthsteep merged 10 commits intoApr 15, 2026
Conversation
…ole-based access from `EmploymentFormController` to service layer.
…olidate approval status filtering logic into a single endpoint, enhance security with `@PreAuthorize` in service layer.
…nnotations for role-based access control in service layer. Clean up redundant controller-level security annotations.
…loymentFormNotFound` and `StaffNotFoundException`, update exception types, and enhance `GlobalExceptionHandler` with custom responses for new exceptions.
…lerTests` with `SecurityIntegrationTests`, update `EmploymentFormService` to use `StaffNotFoundException` instead of `EntityNotFoundException`, and adjust related service tests for role-specific logic.
📝 WalkthroughWalkthroughRemoved controller-level role checks and consolidated list endpoints; added domain exception Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as EmploymentFormController
participant Service as EmploymentFormService
participant Security as MethodSecurityInterceptor
participant Repo as Repository/DB
Client->>Controller: HTTP request (e.g., GET /api/forms?status=...)
Controller->>Service: getFormsByFilterApproval(status)
Service->>Security: method security check (`@PreAuthorize`)
Security-->>Service: allow / deny
alt allowed
Service->>Repo: findByStatus or findAll
Repo-->>Service: entities
Service-->>Controller: mapped DTOs
Controller-->>Client: HTTP 200 with body
else denied
Security-->>Controller: AccessDeniedException
Controller->>Client: HTTP 403 {"error":"Access denied"}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
91-109:⚠️ Potential issue | 🟠 MajorAlign the in-method authorization with the new
ADMINallowance.
ADMINnow passes the Spring Security gate on Lines 91 and 156, but the business checks still only allow the creator or CEO/CTO. A non-creator admin will hitIllegalStateExceptioninstead of the behavior implied by the new annotations.Also applies to: 156-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 91 - 109, The creator-only business check in updateFormBeforeApproval (and the similar post-approval update method around lines 156-170) still throws IllegalStateException for non-creator ADMIN users even though `@PreAuthorize` allows ADMIN; change the authorization logic to allow the update when the logged-in user is either the form creator (existingForm.getCreatedBy().getEmail().equals(loggedInHrEmail)) OR has the ADMIN role. Implement this by checking the current Authentication authorities (SecurityContextHolder.getContext().getAuthentication().getAuthorities()) for "ROLE_ADMIN" (or use an isAdmin() helper) and include that in the if-condition that currently blocks non-creators; apply the same change to the other method that enforces the creator check so ADMINs can proceed without hitting IllegalStateException.
🧹 Nitpick comments (1)
src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java (1)
34-68: Add tests that prove the new service-layer role split.These cases only validate the coarse URL rules. They do not exercise the new
@PreAuthorizedifferences insrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java, such as HR being denied on approve/reject and CTO/CEO being denied on create/update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java` around lines 34 - 68, Current tests only verify URL-level access; add integration tests that exercise the service-layer role split implemented in EmploymentFormService by invoking the controller endpoints that map to the service methods for create/update and approve/reject (the endpoints that delegate to EmploymentFormService.create/update and EmploymentFormService.approve/reject). Specifically add tests using `@WithMockUser`(role = "HR") asserting HR gets 403 when calling the approve/reject endpoints, and tests using `@WithMockUser`(role = "CTO") and `@WithMockUser`(role = "CEO") asserting they get 403 when calling the create/update endpoints; use mockMvc to perform the appropriate POST/PUT requests and assert status().isForbidden() (or isOk() for the allowed roles) so the `@PreAuthorize` rules in EmploymentFormService are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 106-109: The test is incorrectly stubbing
staffRepository.findByEmail(anyString()) which returns the CTO for every email
and masks verification of the authenticated approver; change the stubs to match
the exact emails used in the test calls (e.g.,
when(staffRepository.findByEmail(eq("cto@cyberwatch.local"))).thenReturn(Optional.of(cto))
and
when(staffRepository.findByEmail(eq("manager@cyberwatch.local"))).thenReturn(Optional.of(manager)))
so service.rejectForm(id, "cto@cyberwatch.local") and the other cases exercise
lookup of the actual approver identity via staffRepository.findByEmail rather
than a wildcard; update all occurrences noted (lines around the existing
when(...) stubs and the other test blocks) to use eq(...) or specific string
arguments instead of anyString().
In
`@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java`:
- Around line 52-57: The test method consultantCannotAccessForms targets a
removed route; update it to call the new EmploymentFormController endpoint by
changing the request from GET "/api/forms/pending" to GET "/api/forms" and
supply the status query parameter (e.g., status=pending) so the call exercises
the new handler mapping (use mockMvc.perform(get("/api/forms").param("status",
"pending"))), keeping the test expectation that a CONSULTANT receives
status().isForbidden().
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 91-109: The creator-only business check in
updateFormBeforeApproval (and the similar post-approval update method around
lines 156-170) still throws IllegalStateException for non-creator ADMIN users
even though `@PreAuthorize` allows ADMIN; change the authorization logic to allow
the update when the logged-in user is either the form creator
(existingForm.getCreatedBy().getEmail().equals(loggedInHrEmail)) OR has the
ADMIN role. Implement this by checking the current Authentication authorities
(SecurityContextHolder.getContext().getAuthentication().getAuthorities()) for
"ROLE_ADMIN" (or use an isAdmin() helper) and include that in the if-condition
that currently blocks non-creators; apply the same change to the other method
that enforces the creator check so ADMINs can proceed without hitting
IllegalStateException.
---
Nitpick comments:
In
`@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java`:
- Around line 34-68: Current tests only verify URL-level access; add integration
tests that exercise the service-layer role split implemented in
EmploymentFormService by invoking the controller endpoints that map to the
service methods for create/update and approve/reject (the endpoints that
delegate to EmploymentFormService.create/update and
EmploymentFormService.approve/reject). Specifically add tests using
`@WithMockUser`(role = "HR") asserting HR gets 403 when calling the approve/reject
endpoints, and tests using `@WithMockUser`(role = "CTO") and `@WithMockUser`(role =
"CEO") asserting they get 403 when calling the create/update endpoints; use
mockMvc to perform the appropriate POST/PUT requests and assert
status().isForbidden() (or isOk() for the allowed roles) so the `@PreAuthorize`
rules in EmploymentFormService are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8015cff4-89ee-4943-806f-5e28558f0665
📒 Files selected for processing (7)
src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/exception/EmploymentFormNotFound.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/test/java/org/example/cyberwatch/features/form/EmploymentFormControllerTests.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.javasrc/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java
💤 Files with no reviewable changes (1)
- src/test/java/org/example/cyberwatch/features/form/EmploymentFormControllerTests.java
…-based access. Enhance `GlobalExceptionHandler` to handle `AccessDeniedException`. Adjust `EmploymentFormServiceTest` to verify email usage logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java`:
- Line 67: Update the DisplayName annotation in the SecurityIntegrationTests
class to fix the typo: change the string in `@DisplayName`("HR should access form
endpoints and gett 400 for bad request in body") to use "get 400" instead of
"gett 400" so it reads `@DisplayName`("HR should access form endpoints and get 400
for bad request in body"); locate this annotation in
SecurityIntegrationTests.java to make the edit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 751a8ff8-bc31-488c-851e-19d78a79149e
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.javasrc/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java (1)
65-65:⚠️ Potential issue | 🟡 MinorFix typo in display name text.
Line 65 has
"gett 400"; this should be"get 400".Suggested fix
- `@DisplayName`("HR should access form endpoints and gett 400 for bad request in body") + `@DisplayName`("HR should access form endpoints and get 400 for bad request in body")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java` at line 65, Fix the typo in the DisplayName annotation string for the test in SecurityIntegrationTests: change the text in the `@DisplayName` on the test method annotated in class SecurityIntegrationTests from "HR should access form endpoints and gett 400 for bad request in body" to "HR should access form endpoints and get 400 for bad request in body" so the word "gett" becomes "get".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.java`:
- Line 65: Fix the typo in the DisplayName annotation string for the test in
SecurityIntegrationTests: change the text in the `@DisplayName` on the test method
annotated in class SecurityIntegrationTests from "HR should access form
endpoints and gett 400 for bad request in body" to "HR should access form
endpoints and get 400 for bad request in body" so the word "gett" becomes "get".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bbc5afc-863e-421c-9417-61d9d77e1f6b
📒 Files selected for processing (3)
src/test/java/org/example/cyberwatch/CyberWatchApplicationTests.javasrc/test/java/org/example/cyberwatch/features/security/SecurityIntegrationTests.javasrc/test/resources/application-test.properties
✅ Files skipped from review due to trivial changes (2)
- src/test/resources/application-test.properties
- src/test/java/org/example/cyberwatch/CyberWatchApplicationTests.java
…update or delete forms, refactor permission logic, and update related tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the employment form feature to improve exception handling, centralize and clarify authorization logic, and simplify the controller endpoints. It introduces a custom exception for missing employment forms, consolidates filtering logic for forms, and moves security annotations from the controller to the service layer for better separation of concerns. Additionally, it removes outdated or redundant controller endpoints and updates test coverage accordingly.
Exception handling improvements:
EmploymentFormNotFoundexception and updated the service to throw it when a form is not found, replacing the genericEntityNotFoundException. The global exception handler now returns a 404 response for this case. [1] [2] [3] [4]StaffNotFoundExceptionfor missing staff, improving error clarity. [1] [2] [3] [4]Authorization and security changes:
@PreAuthorizeannotations from the controller to the service layer, enforcing role-based access control closer to the business logic. [1] [2] [3] [4] [5] [6] [7] [8] [9]Controller and API simplification:
getFormsendpoint that supports filtering by approval status. [1] [2]EmploymentFormController. [1] [2] [3] [4]Testing updates:
EmploymentFormControllerTests.java, likely due to changes in endpoint structure or security configuration.These changes collectively make the employment form feature more robust, maintainable, and secure.
Summary by CodeRabbit
Bug Fixes
New Features
Changes
Tests