fix: preserve SavedRequest in auth success handler and fix deleteFile…#130
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracted role-based redirect logic from SecurityConfig into a new SavedRequest-aware Changes
Sequence DiagramsequenceDiagram
actor User
participant App as "Application / Login Controller"
participant Spring as "Spring Security"
participant Handler as "CustomAuthenticationSuccessHandler"
participant Session as "HttpSession (SavedRequest)"
participant Browser as "HttpServletResponse / Redirect"
User->>App: POST /login (credentials)
App->>Spring: Authenticate user
Spring->>Handler: onAuthenticationSuccess(authentication)
Handler->>Session: lookup SavedRequest
alt SavedRequest exists
Handler->>Spring: delegate to superclass (SavedRequest-aware)
Spring->>Browser: redirect to original destination
else No SavedRequest
Handler->>Handler: map authorities -> Role (ADMIN/HANDLER/USER)
Handler->>Handler: clear auth attributes
Handler->>Browser: redirect to role-based URL (/admin,/handler,/user)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/static/js/script.js (1)
66-71:⚠️ Potential issue | 🟡 MinorGuard against
nullresponse fromapiReq.
apiReqreturnsnullon 401/403 (line 86), sores.okon line 67 will throw aTypeErrorin that scenario. Other call sites (e.g.,fetchAFile,downloadFile,uploadNewFile) useif (!res) return;/continue;for this reason —deleteFileshould follow the same pattern.🛡️ Suggested guard
const res = await apiReq(`/upload/api/files/delete-url?fileName=${encodeURIComponent(fileName)}`, {method: 'DELETE'}); + if (!res) return; if(res.ok){ await fetchAFile(); } else { status.innerText= 'Error: ' + res.status; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/script.js` around lines 66 - 71, The deleteFile call assumes apiReq returned a Response but apiReq can return null on 401/403; update the deleteFile flow to guard for a falsy res (same pattern used in fetchAFile/downloadFile/uploadNewFile) and exit early (return) if res is null before accessing res.ok, then proceed to handle res.ok and error cases as before.
🧹 Nitpick comments (1)
src/main/java/org/example/untitled/security/CustomAuthenticationSuccessHandler.java (1)
28-38: Consider role precedence instead offindFirst()when picking the redirect target.Today
CustomUserDetailsServiceassigns exactly one authority per user, sofindFirst()is fine. If that ever changes (e.g. a user is granted bothROLE_USERandROLE_ADMIN, or role hierarchy expansion gets applied), the order returned byAuthentication#getAuthorities()is implementation‑defined and an ADMIN could end up redirected to/user. Selecting the highest‑privilege role explicitly makes the redirect deterministic and future‑proof.Also, the
default -> "/user"branch implicitly handlesRole.USER. ListingUSERexplicitly would make the switch exhaustive overRole, so adding a new enum constant later forces the developer to revisit this code instead of silently routing to/user.♻️ Proposed refactor
- Role role = authentication.getAuthorities().stream() - .map(a -> Role.fromAuthority(a.getAuthority())) - .flatMap(Optional::stream) - .findFirst() - .orElse(Role.USER); - - String targetUrl = switch (role) { - case ADMIN -> "/admin"; - case HANDLER, SUPERVISOR -> "/handler"; - default -> "/user"; - }; + Set<Role> roles = authentication.getAuthorities().stream() + .map(a -> Role.fromAuthority(a.getAuthority())) + .flatMap(Optional::stream) + .collect(Collectors.toUnmodifiableSet()); + + String targetUrl; + if (roles.contains(Role.ADMIN)) { + targetUrl = "/admin"; + } else if (roles.contains(Role.HANDLER) || roles.contains(Role.SUPERVISOR)) { + targetUrl = "/handler"; + } else { + targetUrl = "/user"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/security/CustomAuthenticationSuccessHandler.java` around lines 28 - 38, The redirect logic in CustomAuthenticationSuccessHandler uses findFirst() on authentication.getAuthorities(), which is order‑dependent; instead collect all Roles via Role.fromAuthority(...) into a Set and determine the highest‑privilege role explicitly (e.g., check for Role.ADMIN first, then Role.SUPERVISOR, Role.HANDLER, then Role.USER) to pick the targetUrl deterministically; also make the switch over Role exhaustive by listing USER explicitly (or replace the switch with the explicit if/else precedence checks) so adding new Role enum constants forces revisiting this code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/resources/static/js/script.js`:
- Around line 66-71: The deleteFile call assumes apiReq returned a Response but
apiReq can return null on 401/403; update the deleteFile flow to guard for a
falsy res (same pattern used in fetchAFile/downloadFile/uploadNewFile) and exit
early (return) if res is null before accessing res.ok, then proceed to handle
res.ok and error cases as before.
---
Nitpick comments:
In
`@src/main/java/org/example/untitled/security/CustomAuthenticationSuccessHandler.java`:
- Around line 28-38: The redirect logic in CustomAuthenticationSuccessHandler
uses findFirst() on authentication.getAuthorities(), which is order‑dependent;
instead collect all Roles via Role.fromAuthority(...) into a Set and determine
the highest‑privilege role explicitly (e.g., check for Role.ADMIN first, then
Role.SUPERVISOR, Role.HANDLER, then Role.USER) to pick the targetUrl
deterministically; also make the switch over Role exhaustive by listing USER
explicitly (or replace the switch with the explicit if/else precedence checks)
so adding new Role enum constants forces revisiting this code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f38c1c74-be72-4c26-8ca0-f5e7874793fc
📒 Files selected for processing (3)
src/main/java/org/example/untitled/config/SecurityConfig.javasrc/main/java/org/example/untitled/security/CustomAuthenticationSuccessHandler.javasrc/main/resources/static/js/script.js
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/main/resources/static/js/script.js`:
- Around line 58-63: The frontend is appending multiple hidden inputs named
"fileNames" which do not bind to the backend DTO field
CreateCaseRequest.fileName, causing uploaded files to be orphaned; either change
the UI to only add a single hidden input named "fileName" (update the code that
creates inputs in the hidden-file-inputs container to set input.name =
'fileName' and ensure only the last/selected uploadedFileName is appended), or
extend the backend by changing CreateCaseRequest.fileName to List<String>
fileNames and update CaseService.createTicket and S3Service to accept and
persist multiple file names accordingly so that hidden inputs named "fileNames"
are properly bound.
- Around line 84-91: In deleteFile, res from apiReq can be null (apiReq returns
null on 401/403) so accessing res.ok may throw; update deleteFile to check for a
falsy res immediately after the await (e.g., if (!res) return or set an
error/exit) before touching res.ok, then proceed to call fetchAFile() or set
status when res.ok is true/false; reference the apiReq call and the surrounding
deleteFile function and fetchAFile to locate where to add the early null check
and return.
- Line 19: Client API calls currently use the `/tickets/upload/api/files/...`
prefix but S3RestController exposes endpoints at `/upload/api/files/...`; update
the JS calls (the apiReq invocations in script.js that build URLs like
`/tickets/upload/api/files/download-url`, `/tickets/upload/api/files/...`) to
remove the `/tickets` prefix so they call `/upload/api/files/...`
(alternatively, if you prefer server-side change, add a class-level
`@RequestMapping`("/tickets") to S3RestController), ensuring the paths used by
apiReq match the controller's routes.
- Around line 30-33: Replace direct form.submit() calls with
form.requestSubmit() so HTML5 validation and submit event listeners are
respected (change document.querySelector('form').submit() to const form =
document.querySelector('form'); form.requestSubmit()). In the async file
upload/submit flow, ensure the submit button (e.g., the variable used to disable
the button, such as submitButton or submitBtn) is re-enabled in all error
handlers and in a finally block so the UI is not left disabled on failure;
update the error callbacks around the file upload logic to call
submitButton.disabled = false (or the equivalent) and show an appropriate
user-facing message. Also ensure you preserve existing event listener behavior
by using requestSubmit() everywhere you previously called form.submit().
🪄 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: 830355b8-cfce-434d-a81a-171b241a9e99
📒 Files selected for processing (1)
src/main/resources/static/js/script.js
2 participants
… condition
Summary by CodeRabbit
New Features
Bug Fixes
Refactor