Feature/employment - #81
Merged
Merged
Conversation
… UI for navbar and async rendering; update API endpoints with Staff status management
…staff management; implement real-time status updates, ticket CRUD functionality, and employment handling
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 24 seconds. ⌛ 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 (3)
📝 WalkthroughWalkthroughAdds profilePictureUrl and status to Staff (DB, entity, DTO, mapper, migration); OAuth2 success handler persists picture and ensures default "ONLINE"; exposes current-user endpoints and status update; replaces monolithic frontend bundle with shared common.js + page scripts; changes employment controller/service to accept Staff principal and updates tests and routing. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant OAuth as OAuth2 Provider
participant App as Server (App)
participant DB as Database (staff table)
Browser->>OAuth: authenticate (OAuth2)
OAuth->>App: callback with OAuth2User info
App->>App: OAuth2SuccessHandler extracts picture & email
App->>DB: find Staff by email
DB-->>App: Staff entity (or null)
alt Staff exists
App->>App: if picture present -> set profilePictureUrl
App->>App: if status blank -> set status = "ONLINE"
App->>DB: save Staff
end
App-->>Browser: establish session / replace principal
Browser->>App: GET /api/staff/me
App->>DB: load Staff by id (from principal)
DB-->>App: Staff data (incl. profilePictureUrl,status)
App-->>Browser: 200 OK with StaffDTO
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
Actionable comments posted: 14
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/app.js (1)
218-245:⚠️ Potential issue | 🟠 MajorAvoid dropping existing assignees from multi-assignee tickets.
The UI marks assigned staff with
selected, but this is a single-select and the update sends only[staffId]. Changing one dashboard assignment will replace all existing assignees with one person, even though the backend DTO accepts aList<Long>insrc/main/java/org/example/cyberwatch/features/ticket/model/AssignTicketDTO.java:1-12.🐛 Proposed fix using a multi-select
- <select class="dashboard-assignment-select" data-id="${t.id}" style="max-width: 150px;"> - <option value="">Tilldela...</option> + <select class="dashboard-assignment-select" data-id="${t.id}" multiple style="max-width: 150px;"> + <option value="" disabled>Tilldela...</option> ${allStaff.map(staff => { const isAssigned = assignedIds.includes(staff.id); return `<option value="${staff.id}" ${isAssigned ? 'selected' : ''}> ${isAssigned ? '✓ ' : ''}${escapeHtml(staff.fullName)} </option>`; }).join('')} </select>- const staffId = parseInt(e.target.value); - if (!staffId) return; + const staffIds = Array.from(e.target.selectedOptions) + .map(o => parseInt(o.value, 10)) + .filter(Number.isFinite); + if (staffIds.length === 0) return; const res = await apiFetch(`/tickets/${ticketId}/assign`, { method: "PUT", - body: JSON.stringify({staffIds: [staffId]}) + body: JSON.stringify({staffIds}) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/app.js` around lines 218 - 245, The assignment UI and handler drop existing assignees because .dashboard-assignment-select is single-select and the change handler sends only [staffId] to /tickets/${ticketId}/assign (which expects a List<Long> via AssignTicketDTO); update the select element to allow multiple selection (make it a multi-select) and modify the change handler to collect all selected option values (parse to integers, filter out invalid) and send that array as staffIds in the PUT body to preserve all assignees; keep existing option rendering (including selected markers) but ensure options use value=staff.id and adjust any UI text if needed.
🧹 Nitpick comments (2)
src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.java (1)
65-73: Avoid unconditional save on every login.
staffRepository.save(staff)runs on every successful OAuth2 login even when neitherprofilePictureUrlnorstatuschanged, producing a needless DB write per login. Save only when a field was actually mutated.♻️ Proposed change
- if (picture != null && !picture.isBlank()) { - staff.setProfilePictureUrl(picture); - } - - if (staff.getStatus() == null || staff.getStatus().isBlank()) { - staff.setStatus("ONLINE"); - } - - staffRepository.save(staff); + boolean dirty = false; + if (picture != null && !picture.isBlank() + && !picture.equals(staff.getProfilePictureUrl())) { + staff.setProfilePictureUrl(picture); + dirty = true; + } + if (staff.getStatus() == null || staff.getStatus().isBlank()) { + staff.setStatus("ONLINE"); + dirty = true; + } + if (dirty) { + staffRepository.save(staff); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.java` around lines 65 - 73, The current OAuth2SuccessHandler unconditionally calls staffRepository.save(staff) on every login; change this to only persist when a field actually changes by tracking mutations: read existing values, compare incoming picture to staff.getProfilePictureUrl() and incoming status logic to staff.getStatus(), set via staff.setProfilePictureUrl(...) and staff.setStatus(...) only if different, maintain a boolean dirty flag (e.g., mutated) and call staffRepository.save(staff) only when mutated is true; update references to staffRepository.save, staff.setProfilePictureUrl, and staff.setStatus in OAuth2SuccessHandler accordingly.src/main/java/org/example/cyberwatch/features/staff/model/Staff.java (1)
64-65: Consider modellingstatusas an enum.
roleanddepartmentare typed as enums with@Enumerated(EnumType.STRING), butstatusis a free-formString. This diverges from the existing convention and lets any value be persisted (see related comment inStaffService.updateStatus). AStaffStatusenum (ONLINE,OFFLINE,AWAY,BUSY) would provide compile-time safety and DB-level consistency.🤖 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/staff/model/Staff.java` around lines 64 - 65, Change the free-form String status in the Staff model into a typed enum: create a StaffStatus enum with values (ONLINE, OFFLINE, AWAY, BUSY), update the Staff class field signature from String status to StaffStatus status and annotate it with `@Enumerated`(EnumType.STRING) (same pattern as role and department), update the Staff#getStatus/setStatus signatures and any usage in StaffService.updateStatus to accept or convert to StaffStatus (validate inputs or map incoming strings to enum values), and ensure persistence/migration will store the enum as string for DB compatibility.
🤖 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/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Around line 34-40: The updateMyStatus controller accepts an unchecked raw
status string; validate it against the allowed values (ONLINE, BUSY, AWAY,
OFFLINE) before calling staffService.updateStatus to prevent persisting
unsupported values. In updateMyStatus(`@AuthenticationPrincipal` Staff staff,
`@RequestParam` String status) check the incoming status against the allowed set
(or a StaffStatus enum if one exists), and return
ResponseEntity.status(400).body(...) or ResponseEntity.badRequest().build() when
invalid; only call staffService.updateStatus(staff.getId(), status) when the
value is valid. Ensure the error response is clear and consistent with other
controller validations.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 91-96: The updateStatus method currently accepts any String; add
validation and an enum to constrain allowed values: create a StaffStatus enum
(ONLINE, OFFLINE, AWAY, BUSY), change the Staff.status field to use
`@Enumerated`(EnumType.STRING), and update updateStatus(Long staffId, String
status) to null-check staffId (consistent with other methods), reject null/blank
status, convert the incoming String to StaffStatus (throw
IllegalArgumentException for unknown values), set staff.setStatus(statusEnum)
and then save via staffRepository.save(staff) as before; ensure error messages
clearly identify invalid input.
In `@src/main/resources/static/js/app.js`:
- Around line 66-89: The profilePictureUrl is injected directly into
navbar.innerHTML via the profileImage variable, creating an XSS risk; add a
helper function (e.g., safeImageUrl) that validates and normalizes the value
(returning a safe fallback when null, non-HTTP(S), or parsing fails) and use
that helper to compute profileImage before injecting into the src attribute, and
ensure you use the normalized parsed href (not the raw string) so quotes/event
handlers cannot break the attribute.
- Around line 205-216: Replace the clickable <div> used for navigation inside
item.innerHTML with a real anchor element: remove the inline onclick handler and
render an <a> element (preserving the "ticket-main" class and inner structure)
with its href set to '/pages/ticket-detail.html?id=${t.id}' so keyboard and
assistive tech get native link semantics; ensure existing content (badge, title,
meta) and injected values (escapeHtml(t.title), priorityIcon,
assignedStaffNames) remain unchanged and the link styling/behavior is preserved
via CSS for the "ticket-main" class.
In `@src/main/resources/static/js/common.js`:
- Around line 56-79: Profile image URLs from currentUser.profilePictureUrl are
interpolated directly into navbar.innerHTML via profileImage and can contain
unsafe or non-HTTP(S) schemes; add a sanitizer function (e.g., safeImageUrl)
next to escapeHtml that validates and normalizes the URL using the URL
constructor and only allows http: and https: (falling back to
"https://via.placeholder.com/40" on invalid input), then use
safeImageUrl(currentUser?.profilePictureUrl) to set profileImage before
inserting into navbar.innerHTML so only safe image URLs are rendered.
In `@src/main/resources/static/js/dashboard.js`:
- Around line 55-66: The ticket card uses a div with an onclick handler (inside
item.innerHTML and class "ticket-main") which loses native link semantics and
keyboard accessibility; replace the clickable <div class="ticket-main"
onclick="..."> with a semantic anchor (<a
href="/pages/ticket-detail.html?id=${t.id}" class="ticket-main">) so the href
carries navigation, remove the inline onclick, preserve the inner structure
(ticket-header, ticket-meta, escaped title via escapeHtml(t.title), badges, and
assignedStaffNames) and ensure any styling that targeted .ticket-main still
applies; optionally add an aria-label to the anchor for clearer screen-reader
text.
- Line 39: The query string is built by string interpolation which allows
user-controlled searchInput (q) to inject characters like & = #; replace the
inline template with a URLSearchParams-based builder so values are properly
encoded: create a URLSearchParams with keys status, priority, search,
assignedStaffId (using the existing variables s, p, q, staffId) and then call
apiFetch(`/tickets?${params.toString()}`) instead of the current template;
update the code around the apiFetch call to use params.toString() so searchInput
is safely encoded.
- Around line 68-95: The assignment select currently renders multiple assigned
users as selected but the change handler in dashboard.js (the listener attached
to '.dashboard-assignment-select') reads only e.target.value and sends a single
staffId to the /tickets/{ticketId}/assign endpoint, which overwrites existing
assignees; update the UI and handler to submit the full list of selected
assignee IDs matching the backend AssignTicketDTO.staffIds: either convert the
<select> to a multi-select (allow multiple) and collect all selected option
values, or (if keeping single select UI) merge the newly chosen id with the
existing assignedIds for that ticket before sending; ensure the change handler
builds an array of ints and sends JSON.stringify({staffIds: [...]}) to the same
PUT endpoint so AssignTicketDTO receives the complete list.
In `@src/main/resources/static/js/edit-ticket.js`:
- Around line 27-33: The code builds a full edit payload in the body variable
(title, description, priority, status) but then calls apiFetch only to PATCH the
status via `/tickets/${id}/status?status=...`, so edited fields are dropped;
either send the full body to the ticket edit endpoint or restrict the form to
status-only. Fix by changing the apiFetch call to PATCH the full ticket (e.g.
apiFetch(`/tickets/${id}`, { method: "PATCH", body: JSON.stringify(body),
headers: { "Content-Type": "application/json" } })) so
title/description/priority are persisted, or remove creation of
title/description/priority and update the UI and form to only allow status edits
and keep the existing `/tickets/${id}/status` call.
In `@src/main/resources/static/js/employment.js`:
- Around line 226-230: The approval handler currently shows server response text
(variable text from the fetch response) in a browser alert which may contain a
generated password; change the UX to stop displaying credentials directly:
replace the alert("Godkänd!\n" + text) use with a generic success notification
(e.g. "Godkänd!" only) and implement a secure delivery mechanism for credentials
(e.g. server-sent email or a one-time link) or, if you must surface
non-credential info, parse the response from the fetch to strip any password
before displaying; update the same handler that calls
loadPendingEmploymentForms() so it no longer exposes text in the client alert.
- Around line 122-128: The staff list currently renders sensitive PII via
staff.socialSecurityNumber inside the employment-card-body; remove that
unconditional exposure by either removing the `<p><strong>Personnummer:</strong>
${escapeHtml(staff.socialSecurityNumber || "-")}</p>` line entirely or wrapping
it in an explicit role check (e.g., only render when current user has HR/Admin
privileges using your app's auth helper such as isUserInRole/currentUser.roles)
and otherwise render a masked value like "-" or "•••". Ensure you reference and
use the existing escapeHtml helper when conditionally rendering to avoid
introducing XSS.
In `@src/main/resources/static/js/ticket-detail.js`:
- Around line 53-78: The loadComments function can throw unhandled rejections on
network failures; wrap the fetch/JSON parsing logic inside loadComments in a
try/catch (around the calls to apiFetch and res.json()) and on error set the
comments container (list.innerHTML) to the error state (e.g. "<p>Kunde inte
hämta kommentarer.</p>") and optionally console.error the exception; also ensure
the caller (where loadComments(id) is invoked) either awaits the promise or
appends .catch(...) so failures don't become unhandled rejections — target the
loadComments function, the apiFetch call, the res.json() call, and the code that
calls loadComments(id).
In `@src/main/resources/static/pages/create-ticket.html`:
- Around line 92-100: The code uses badge.innerHTML with dynamic staffName which
enables XSS; change this to create text and control nodes instead: set
badge.className as before, append a text node or assign badge.textContent =
staffName, create a separate closeSpan via document.createElement('span'), set
closeSpan.style and closeSpan.textContent = '×', attach the click handler to
closeSpan that deletes selectedStaffIds, removes the badge, and calls
updateRequiredStatus(), then append closeSpan to badge and badge to
badgeContainer (replace the existing badge.innerHTML usage and current
badge.onclick logic).
In `@src/main/resources/static/pages/ticket-detail.html`:
- Around line 70-80: The badge creation in window.addStaffBadge currently uses
badge.innerHTML with untrusted name which can run markup; replace that by
creating child text/span nodes and a separate remove-button element instead of
injecting HTML: use a nameNode (set textContent) for the staff name, create a
removeIcon element (e.g., a span with textContent '×' and cursor styling),
attach the click handler to the removeIcon (or use addEventListener) to delete
the id from window.currentAssignedIds and remove the badge, and append nameNode
and removeIcon to badgeContainer; update references to badge.onclick and
badge.innerHTML accordingly in window.addStaffBadge.
---
Outside diff comments:
In `@src/main/resources/static/js/app.js`:
- Around line 218-245: The assignment UI and handler drop existing assignees
because .dashboard-assignment-select is single-select and the change handler
sends only [staffId] to /tickets/${ticketId}/assign (which expects a List<Long>
via AssignTicketDTO); update the select element to allow multiple selection
(make it a multi-select) and modify the change handler to collect all selected
option values (parse to integers, filter out invalid) and send that array as
staffIds in the PUT body to preserve all assignees; keep existing option
rendering (including selected markers) but ensure options use value=staff.id and
adjust any UI text if needed.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.java`:
- Around line 65-73: The current OAuth2SuccessHandler unconditionally calls
staffRepository.save(staff) on every login; change this to only persist when a
field actually changes by tracking mutations: read existing values, compare
incoming picture to staff.getProfilePictureUrl() and incoming status logic to
staff.getStatus(), set via staff.setProfilePictureUrl(...) and
staff.setStatus(...) only if different, maintain a boolean dirty flag (e.g.,
mutated) and call staffRepository.save(staff) only when mutated is true; update
references to staffRepository.save, staff.setProfilePictureUrl, and
staff.setStatus in OAuth2SuccessHandler accordingly.
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 64-65: Change the free-form String status in the Staff model into
a typed enum: create a StaffStatus enum with values (ONLINE, OFFLINE, AWAY,
BUSY), update the Staff class field signature from String status to StaffStatus
status and annotate it with `@Enumerated`(EnumType.STRING) (same pattern as role
and department), update the Staff#getStatus/setStatus signatures and any usage
in StaffService.updateStatus to accept or convert to StaffStatus (validate
inputs or map incoming strings to enum values), and ensure persistence/migration
will store the enum as string for DB compatibility.
🪄 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: 57e29567-c92b-4dec-a6ed-c502736ca6a4
📒 Files selected for processing (20)
src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/static/css/styles.csssrc/main/resources/static/js/app.jssrc/main/resources/static/js/common.jssrc/main/resources/static/js/create-ticket.jssrc/main/resources/static/js/dashboard.jssrc/main/resources/static/js/edit-ticket.jssrc/main/resources/static/js/employment.jssrc/main/resources/static/js/ticket-detail.jssrc/main/resources/static/pages/create-ticket.htmlsrc/main/resources/static/pages/dashboard.htmlsrc/main/resources/static/pages/edit-ticket.htmlsrc/main/resources/static/pages/employment.htmlsrc/main/resources/static/pages/ticket-detail.htmlsrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
…validations in `StaffService` and `StaffRestController`
…iew controllers in WebConfig; update SecurityConfig with new endpoints and logout handling
…ove and view pending employment forms; update logging to use Staff object for better clarity.
…aff object for authentication; improve logging and authorization checks for form approval, rejection, and deletion.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
189-205:⚠️ Potential issue | 🟠 MajorAvoid writing to S3 before the database transaction is durable.
archiveToS3(form)runs before the staff/form saves. If either DB save fails or the transaction later rolls back, S3 can contain an archive for an approval that never committed. Prefer persisting the approval state/key first and moving the S3 upload to an after-commit hook or outbox/retry flow.🤖 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 189 - 205, The code calls archiveToS3(form) before the DB changes are durable; move S3 writes to after the transaction commits to avoid orphaned S3 objects. Persist the updated form and newStaff first (use employmentFormRepository.save(form) and staffRepository.save(newStaff) after setting form.setApprovedBy(...) and form.setStatus(...)), then trigger archiveToS3(form) from an after-commit hook/event or an outbox/retry mechanism that runs only when the transaction succeeds; ensure newStaff.setEmployedS3Key(...) is set from the result of the after-commit upload or updated in a follow-up transactional step so the DB always reflects the canonical state.
207-209:⚠️ Potential issue | 🟠 MajorDon’t return the generated password in the API response.
This exposes a live credential to the browser and any intermediaries/client logs. Return a generic success response and deliver onboarding through a reset-token/email flow.
Proposed immediate containment
- //No need to worry, this will be replaced with an email service - return "Employment has been approved, generated password for new employee: " + rawPassword; + // TODO: Send onboarding/reset instructions through the email service. + return "Employment has been approved.";🤖 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 207 - 209, The method in EmploymentFormService that logs and returns the generated rawPassword (the return line returning "Employment has been approved, generated password for new employee: " + rawPassword) must be changed to avoid exposing credentials: stop including rawPassword in the API response and in logs, return a generic success message such as "Employment approved; onboarding email sent" from the approving method (e.g., approveEmploymentForm or whatever method contains logger.info("Form {} approved by {}", formId, loggedInManagement)); instead generate a one-time reset token, persist only hashed credentials/password, and invoke the onboarding/email workflow (e.g., EmailService.sendOnboardingEmail(formId, resetToken) or OnboardingService.sendResetToken) to deliver the initial login link; ensure any logging (logger.info/logger.debug) never prints rawPassword and only logs non-sensitive identifiers.src/main/java/org/example/cyberwatch/config/SecurityConfig.java (1)
60-65:⚠️ Potential issue | 🟠 MajorAllow self-status updates before the broad staff write rule.
PATCH /api/staff/me/statusonly updates the caller’s own status, but it currently falls through to.requestMatchers("/api/staff/**").hasAnyRole("HR", "CEO", "CTO", "ADMIN"), so non-HR/ADMIN/CEO/CTO users cannot update their own presence.🔐 Proposed matcher ordering fix
).permitAll() + // Alla inloggade får uppdatera sin egen status + .requestMatchers(HttpMethod.PATCH, "/api/staff/me/status").authenticated() // Alla inloggade får läsa staff (behövs för ticket-dropdowns) .requestMatchers(HttpMethod.GET, "/api/staff/**").authenticated() // Endast HR, CEO, CTO & ADMIN får skriva/ändra staff🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/SecurityConfig.java` around lines 60 - 65, The PATCH endpoint for users to update their own status is being blocked by the broader staff write rule in SecurityConfig; add a specific matcher for HttpMethod.PATCH on "/api/staff/me/status" that allows authenticated users (e.g., .requestMatchers(HttpMethod.PATCH, "/api/staff/me/status").authenticated()) and place it before the existing .requestMatchers("/api/staff/**").hasAnyRole("HR","CEO","CTO","ADMIN") rule so the specific self-update rule takes precedence over the broad staff write rule.
🤖 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/java/org/example/cyberwatch/config/SecurityConfig.java`:
- Around line 90-93: Frontend triggers logout with GET (common.js uses
window.location.href = "/logout") but SecurityConfig.java currently relies on
Spring Security's default POST logout (CSRF enabled via CsrfConfigurer::spa) and
only sets logoutSuccessUrl(); fix by either making the frontend issue a POST
with the CSRF token/header or changing the backend logout matcher to accept
GETs: update the code in SecurityConfig.java where .logout(...) is configured
(the logout() block and logoutSuccessUrl(...) usage) to call
logoutRequestMatcher(...) or permit GET (e.g., configure an
AntPathRequestMatcher for "/logout" with HttpMethod.GET) if GET logout is
intentional, or change the frontend common.js to send a POST to "/logout" with
the XSRF token/header so CSRF protection is satisfied.
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Line 48: The log message in EmploymentFormController at the approval endpoint
is misleading — it currently logs "Creating employment form for HR staff" via
logger.info; update that log to reflect the approval action (e.g., "Approved
employment form" or "Approving employment form") and include identifying details
such as the form id and approver email (use the same local variables used in the
method, e.g., form.getId() and staff.getEmail()) so audit/debug traces correctly
show the approval event.
- Around line 31-33: Replace the inline principal checks in
EmploymentFormController (currently using "Object principal =
authentication.getPrincipal()" and throwing IllegalArgumentException) with a
centralized server-side staff extraction that throws AccessDeniedException on
failure, modeled after TicketController.getAuthenticatedStaff(); add a private
helper in EmploymentFormController (e.g., getAuthenticatedStaff(Authentication))
or call the existing TicketController.getAuthenticatedStaff(...) to obtain
org.example.cyberwatch.features.staff.model.Staff for all five endpoints
(create, approve, update, reject, delete), remove any actor resolution from
caller-controlled parameters and use the resolved Staff for actor fields, and
ensure every location that formerly threw IllegalArgumentException now throws
org.springframework.security.access.AccessDeniedException so the endpoints
return 403.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 128-139: The runtime role checks in
EmploymentFormService.rejectForm (and the similar method around lines 177-183)
are inconsistent with the `@PreAuthorize` that permits ADMIN; update the checks to
allow Role.ADMIN as well (e.g., include loggedInRejecter.getRole() != Role.ADMIN
in the allowed roles) and adjust the IllegalStateException message to reflect
the correct allowed roles (CEO, CTO, ADMIN) so both the annotation and service
pre-checks match; apply the same change to the counterpart approval method
referenced in the comment.
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 251-263: The test is not exercising the non-PENDING branch because
approver.getRole() is null; set a valid management role on the Staff approver
before invoking service.approveAndFinalizeEmployment so the service checks the
form status (which is APPROVED) rather than failing on role null. Update the
test's approver setup (the Staff approver instance in EmploymentFormServiceTest)
to call approver.setRole(...) with a valid management role (for example
ManagementRole.MANAGER or the appropriate enum/constant used in your codebase)
and then keep the rest of the test (form with ApprovalStatus.APPROVED,
when(formRepository.findById(formId))...) and the assertThrows unchanged.
---
Outside diff comments:
In `@src/main/java/org/example/cyberwatch/config/SecurityConfig.java`:
- Around line 60-65: The PATCH endpoint for users to update their own status is
being blocked by the broader staff write rule in SecurityConfig; add a specific
matcher for HttpMethod.PATCH on "/api/staff/me/status" that allows authenticated
users (e.g., .requestMatchers(HttpMethod.PATCH,
"/api/staff/me/status").authenticated()) and place it before the existing
.requestMatchers("/api/staff/**").hasAnyRole("HR","CEO","CTO","ADMIN") rule so
the specific self-update rule takes precedence over the broad staff write rule.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 189-205: The code calls archiveToS3(form) before the DB changes
are durable; move S3 writes to after the transaction commits to avoid orphaned
S3 objects. Persist the updated form and newStaff first (use
employmentFormRepository.save(form) and staffRepository.save(newStaff) after
setting form.setApprovedBy(...) and form.setStatus(...)), then trigger
archiveToS3(form) from an after-commit hook/event or an outbox/retry mechanism
that runs only when the transaction succeeds; ensure
newStaff.setEmployedS3Key(...) is set from the result of the after-commit upload
or updated in a follow-up transactional step so the DB always reflects the
canonical state.
- Around line 207-209: The method in EmploymentFormService that logs and returns
the generated rawPassword (the return line returning "Employment has been
approved, generated password for new employee: " + rawPassword) must be changed
to avoid exposing credentials: stop including rawPassword in the API response
and in logs, return a generic success message such as "Employment approved;
onboarding email sent" from the approving method (e.g., approveEmploymentForm or
whatever method contains logger.info("Form {} approved by {}", formId,
loggedInManagement)); instead generate a one-time reset token, persist only
hashed credentials/password, and invoke the onboarding/email workflow (e.g.,
EmailService.sendOnboardingEmail(formId, resetToken) or
OnboardingService.sendResetToken) to deliver the initial login link; ensure any
logging (logger.info/logger.debug) never prints rawPassword and only logs
non-sensitive identifiers.
🪄 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: 27adf71d-55e9-41c0-bafd-b89900ee195b
📒 Files selected for processing (10)
src/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/config/WebConfig.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/db/migration/V10__add_profile_picture_and_status_to_staff.sqlsrc/main/resources/static/js/app.jssrc/main/resources/static/js/employment.jssrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/resources/static/js/app.js
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V10__add_profile_picture_and_status_to_staff.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java
- src/main/resources/static/js/employment.js
…non-Staff principals; update logging messages for form creation and approval
…nd deletions; handle exceptions for non-existent staff
…t fetching with URLSearchParams for better query handling
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/common.js`:
- Around line 109-124: The change listener on statusSelect sets the UI before
confirming the server PATCH succeeded, so on failure the select still shows the
new value; capture the previous value (e.g., const prev = statusSelect.value or
e.target.value before awaiting apiFetch) and if the response is not ok or the
request throws (res.ok === false or in the catch), reset the select back to prev
(statusSelect.value = prev) and show the existing error alerts; update the
listener attached to statusSelect and references to apiFetch to perform this
rollback on both non-OK responses and exceptions.
- Around line 139-142: Normalize selectedIds to a consistent type (e.g., map to
strings or build a Set of String values) and then mark options as selected when
their IDs match; update the staffOptions generation (the staffOptions mapping,
the isAssigned check against s.id, and the className usage) to compare
String(s.id) to the normalized selectedIds and include a selected attribute
(e.g., ' selected') in the returned <option> when isAssigned, while still adding
the badge-assigned class when appropriate.
- Around line 44-45: The logout() function currently does a GET by setting
window.location.href; change it to send a POST to "/logout" so Spring Security
will invalidate the session — implement logout() to issue a POST (e.g., using
fetch or XMLHttpRequest) to "/logout" with credentials: "same-origin" and
include the CSRF token header (read the token from your page’s CSRF meta tags or
cookie, e.g., the _csrf token/meta or the header name like X-CSRF-TOKEN) so the
request is accepted by .csrf(CsrfConfigurer::spa) and triggers session
invalidation.
🪄 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: 28f404e3-dbb3-41bf-91dd-30ab0a6f12be
📒 Files selected for processing (2)
src/main/resources/static/js/common.jssrc/main/resources/static/js/dashboard.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/static/js/dashboard.js
…led when no staff is selected; allow multiple staff assignments for tickets.
…improve user feedback on failure
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
210-211:⚠️ Potential issue | 🟠 MajorDo not return the generated password in the API response.
This exposes a reusable credential through HTTP clients, browser/devtool history, logs, and monitoring. Return a generic success message and deliver onboarding credentials via a reset-link/email flow instead.
🔐 Minimal response hardening
- //No need to worry, this will be replaced with an email service - return "Employment has been approved, generated password for new employee: " + rawPassword; + // TODO: Send onboarding/reset instructions via the email service. + return "Employment has been approved";🤖 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 210 - 211, The API currently returns the generated rawPassword in the response string inside EmploymentFormService (the approval flow method), which leaks credentials; remove rawPassword from the returned message and replace it with a generic success message (e.g., "Employment approved; onboarding email sent"), and instead invoke the onboarding/email/reset-link flow from the same method (call your EmailService.sendOnboardingLink or PasswordResetService.createAndSendResetLink using the new user's id/email). Also ensure no rawPassword is logged or included in any return type from methods like approveEmployment / createNewEmployee and that any audit logs store only non-sensitive identifiers.
143-151:⚠️ Potential issue | 🟠 MajorMove S3 archiving out of the rollback-sensitive path.
Both reject and approve upload the archived form to S3 before the final database writes complete. If
save(...)fails afterward, the transaction rolls back but the S3 object remains, potentially archiving PII for a state change that never committed. Prefer an after-commit/outbox flow, or add compensating cleanup around failed post-upload persistence.Also applies to: 197-207
🤖 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 143 - 151, The S3 upload (archiveToS3) is currently executed before the DB commit in EmploymentFormService (e.g., in the reject/approve flow surrounding employmentFormRepository.save), which can leave S3 objects when the transaction later rolls back; move the archiveToS3 call out of the rollback-sensitive path by invoking it after the transaction commits (use a transaction synchronization/afterCommit callback or an outbox event dispatched from EmploymentFormService) or, if you must upload synchronously, perform the upload only after employmentFormRepository.save succeeds and add compensating cleanup (delete from S3) if a subsequent persistence step fails; update both places referenced (the block around form.setStatus(...)/archiveToS3 and the similar block at lines 197-207) accordingly.
🧹 Nitpick comments (3)
src/main/resources/static/js/common.js (1)
34-42:apiFetchfires-and-forgetslogout()on 401.
logout()isasyncand issues a POST before redirecting, but here it's invoked withoutawait. The function that received the 401 keeps executing against a stale session, callers get back the 401responseand mayalert(...)/res.json()on it, and in some flowsloadDashboardTickets/loadTicketDetailrender "Kunde inte hämta..." error states just before the redirect. Considerawait logout(); return response;(or throw a sentinel) so the redirect wins the race and callers don't render transient error UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/common.js` around lines 34 - 42, apiFetch currently calls the async logout() without awaiting, causing callers to continue working with a stale 401 response; update apiFetch to await logout() when response.status === 401 (e.g., await logout(); return response;) or alternatively throw a clear sentinel error after awaiting so callers don't continue rendering transient error UI. Locate the apiFetch function and modify the 401 branch to await the async logout() (or await then throw) to ensure the redirect completes before callers proceed.src/main/resources/static/js/ticket-detail.js (1)
104-131:setupUploadFormbypassesapiFetchand loses 401 handling.Using the raw
fetchhere is necessary to avoid the JSONContent-TypefromgetHeaders(), but it means a session that expires during upload won't trigger the sharedlogout()redirect, and any future cross-cutting concern added toapiFetchwill silently skip this call. Consider extendingapiFetchto accept an option like{ multipart: true }(or askipJsonContentTypeflag) so all API calls go through one wrapper and keep the 401 contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/ticket-detail.js` around lines 104 - 131, Modify the upload to go through the shared wrapper: extend apiFetch to accept an option like { multipart: true } (or skipJsonContentType) so it will omit setting "Content-Type" and will still include credentials, CSRF headers and the existing 401/logout handling; then update setupUploadForm to call apiFetch(`/api/tickets/${id}/upload`, { method: "POST", body: formData, multipart: true }) instead of raw fetch and remove the manual getCsrfToken/fetch header logic, ensuring form.reset() and loadTicketDetail() remain on success and the existing apiFetch error handling runs on failure.src/main/resources/static/js/dashboard.js (1)
21-45: Redundant "stats" fetch of the full ticket list.
loadDashboardTicketsfires two back-to-backGET /ticketsrequests on every keystroke of#searchInput(plus another to/staff): one unfiltered fetch purely to compute four counters, and one filtered fetch for the list. On a large ticket table this doubles load and amplifies any search-as-you-type latency.Consider either (a) computing counters from the already-fetched filtered list when filters are empty and caching the "all tickets" response, or (b) adding a dedicated lightweight
/tickets/statsendpoint. Debouncing theinputlistener would also help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/dashboard.js` around lines 21 - 45, The code is doing two back-to-back GET /tickets calls (the "stats" fetch using apiFetch and the filtered fetch using apiFetch(`/tickets?${params}`)), which duplicates work and doubles latency; remove the redundant statsRes fetch and instead compute the four counters (totalCount, openCount, inProgressCount, closedCount) from the already-fetched ticket list returned by the main fetch (res.json()), and/or cache a single "allTickets" result in a module-level variable so subsequent calls can reuse it when filters are empty; update the DOM using the same element IDs ("totalTickets", "openTickets", "inProgressTickets", "closedTickets") after parsing the main response, and add debouncing to the search input listener to reduce request frequency.
🤖 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/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 107-109: The ownership check in EmploymentFormService currently
calls existingForm.getCreatedBy().getId().equals(loggedInHr.getId()), which can
NPE if createdBy or its id is null; change the isCreator logic to be null-safe
by first checking existingForm.getCreatedBy() != null and then using a null-safe
equality (e.g., Objects.equals(existingForm.getCreatedBy().getId(),
loggedInHr.getId())) to compare IDs; apply the same null-safe pattern to the
equivalent ownership/delete checks referenced around lines 167-168 so all
authorization checks avoid dereferencing potentially-null Staff.id.
In `@src/main/resources/static/js/common.js`:
- Around line 8-13: The escapeHtml function only encodes & < > via the
textContent -> innerHTML trick and therefore leaves " and ' unescaped, making
attribute contexts vulnerable; update escapeHtml to also replace double-quote
(") with " and single-quote (') with ' (in addition to existing &, <, >
handling) so any use in attributes (e.g., the ticket card aria-label in
dashboard.js and attachment href in ticket-detail.js) is safe; keep the function
name escapeHtml and ensure callers continue to receive a string (no DOM writes)
so existing usages need no other changes.
In `@src/main/resources/static/js/dashboard.js`:
- Around line 62-78: The aria-label and inline handlers in the ticket card HTML
(the template building code that sets item.innerHTML and uses onclick/onkeydown
on the .ticket-main element) are vulnerable because escapeHtml does not escape
quotes; fix this by either (A) hardening escapeHtml (in common.js) to also
replace double-quote (") with " and single-quote (') with ' so
interpolation into attributes is safe, or (B — preferred) stop using innerHTML
and inline onclick/onkeydown: build the card using createElement, setAttribute
(for aria-label), textContent for titles and IDs, addEventListener for
click/keydown handlers, and use element.classList to set badges so no user data
is ever injected into an HTML string; update references to escapeHtml,
item.innerHTML, onclick, onkeydown, and .ticket-main accordingly.
In `@src/main/resources/static/js/ticket-detail.js`:
- Around line 19-32: The attachment links currently only use escapeHtml and can
still render unsafe schemes; add a URL-sanitizer helper in common.js (mirroring
safeImageUrl) that parses the URL and returns it only if parsed.protocol is
"http:" or "https:" (otherwise return null/empty), then update the template that
builds attachmentsHtml to call that helper for a.downloadUrl and, on
invalid/empty result, render a non-clickable filename or use "#" as the href and
avoid target/rel attributes; reference the attachmentsHtml code and escapeHtml
usage so you replace escapeHtml(a.downloadUrl) with the new safe URL helper
result and keep escapeHtml(a.fileName) for the label.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 210-211: The API currently returns the generated rawPassword in
the response string inside EmploymentFormService (the approval flow method),
which leaks credentials; remove rawPassword from the returned message and
replace it with a generic success message (e.g., "Employment approved;
onboarding email sent"), and instead invoke the onboarding/email/reset-link flow
from the same method (call your EmailService.sendOnboardingLink or
PasswordResetService.createAndSendResetLink using the new user's id/email). Also
ensure no rawPassword is logged or included in any return type from methods like
approveEmployment / createNewEmployee and that any audit logs store only
non-sensitive identifiers.
- Around line 143-151: The S3 upload (archiveToS3) is currently executed before
the DB commit in EmploymentFormService (e.g., in the reject/approve flow
surrounding employmentFormRepository.save), which can leave S3 objects when the
transaction later rolls back; move the archiveToS3 call out of the
rollback-sensitive path by invoking it after the transaction commits (use a
transaction synchronization/afterCommit callback or an outbox event dispatched
from EmploymentFormService) or, if you must upload synchronously, perform the
upload only after employmentFormRepository.save succeeds and add compensating
cleanup (delete from S3) if a subsequent persistence step fails; update both
places referenced (the block around form.setStatus(...)/archiveToS3 and the
similar block at lines 197-207) accordingly.
---
Nitpick comments:
In `@src/main/resources/static/js/common.js`:
- Around line 34-42: apiFetch currently calls the async logout() without
awaiting, causing callers to continue working with a stale 401 response; update
apiFetch to await logout() when response.status === 401 (e.g., await logout();
return response;) or alternatively throw a clear sentinel error after awaiting
so callers don't continue rendering transient error UI. Locate the apiFetch
function and modify the 401 branch to await the async logout() (or await then
throw) to ensure the redirect completes before callers proceed.
In `@src/main/resources/static/js/dashboard.js`:
- Around line 21-45: The code is doing two back-to-back GET /tickets calls (the
"stats" fetch using apiFetch and the filtered fetch using
apiFetch(`/tickets?${params}`)), which duplicates work and doubles latency;
remove the redundant statsRes fetch and instead compute the four counters
(totalCount, openCount, inProgressCount, closedCount) from the already-fetched
ticket list returned by the main fetch (res.json()), and/or cache a single
"allTickets" result in a module-level variable so subsequent calls can reuse it
when filters are empty; update the DOM using the same element IDs
("totalTickets", "openTickets", "inProgressTickets", "closedTickets") after
parsing the main response, and add debouncing to the search input listener to
reduce request frequency.
In `@src/main/resources/static/js/ticket-detail.js`:
- Around line 104-131: Modify the upload to go through the shared wrapper:
extend apiFetch to accept an option like { multipart: true } (or
skipJsonContentType) so it will omit setting "Content-Type" and will still
include credentials, CSRF headers and the existing 401/logout handling; then
update setupUploadForm to call apiFetch(`/api/tickets/${id}/upload`, { method:
"POST", body: formData, multipart: true }) instead of raw fetch and remove the
manual getCsrfToken/fetch header logic, ensuring form.reset() and
loadTicketDetail() remain on success and the existing apiFetch error handling
runs on failure.
🪄 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: d62b355c-53dd-4bfe-b89e-cd2987767ed6
⛔ Files ignored due to path filters (1)
src/main/resources/static/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (7)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/resources/static/js/common.jssrc/main/resources/static/js/dashboard.jssrc/main/resources/static/js/ticket-detail.jssrc/main/resources/static/pages/create-ticket.htmlsrc/main/resources/static/pages/ticket-detail.htmlsrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
Comment on lines
+19
to
+32
| const attachmentsHtml = t.attachments && t.attachments.length > 0 | ||
| ? `<div style="margin-top: 1rem;"> | ||
| <strong>Bilagor:</strong> | ||
| <ul style="margin-top: 0.5rem; padding-left: 1.2rem;"> | ||
| ${t.attachments.map(a => ` | ||
| <li style="margin-bottom: 0.4rem;"> | ||
| <a href="${escapeHtml(a.downloadUrl)}" target="_blank" rel="noopener noreferrer"> | ||
| ${escapeHtml(a.fileName)} | ||
| </a> | ||
| </li> | ||
| `).join('')} | ||
| </ul> | ||
| </div>` | ||
| : `<p class="muted" style="margin-top: 1rem;">Inga bilagor uppladdade.</p>`; |
Contributor
There was a problem hiding this comment.
this is implemented
…nction for better security and use safeHttpUrl for download links in ticket details
Comment on lines
+19
to
+32
| const attachmentsHtml = t.attachments && t.attachments.length > 0 | ||
| ? `<div style="margin-top: 1rem;"> | ||
| <strong>Bilagor:</strong> | ||
| <ul style="margin-top: 0.5rem; padding-left: 1.2rem;"> | ||
| ${t.attachments.map(a => ` | ||
| <li style="margin-bottom: 0.4rem;"> | ||
| <a href="${escapeHtml(a.downloadUrl)}" target="_blank" rel="noopener noreferrer"> | ||
| ${escapeHtml(a.fileName)} | ||
| </a> | ||
| </li> | ||
| `).join('')} | ||
| </ul> | ||
| </div>` | ||
| : `<p class="muted" style="margin-top: 1rem;">Inga bilagor uppladdade.</p>`; |
Contributor
There was a problem hiding this comment.
this is implemented
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (3)
144-152:⚠️ Potential issue | 🟠 MajorAvoid S3 writes before the database transaction commits.
Both reject and approve upload the archive inside the transaction before the final DB write/commit. If the later save or commit fails, S3 can contain an archive for a state that rolled back. Prefer a transactional outbox or an
AFTER_COMMITevent with retry/compensation.Also applies to: 201-209
🤖 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 144 - 152, The S3 upload (archiveToS3) is happening inside the DB transaction and must be deferred until after the transaction commits to avoid orphaned S3 objects; refactor EmploymentFormService to remove direct calls to archiveToS3 from the reject/approve flow (the blocks that call form.setStatus(...), form.setApprovedBy(...), employmentFormRepository.save(form)) and instead publish a post-commit action—either register a TransactionSynchronization via TransactionSynchronizationManager.registerSynchronization or emit an event handled by a method annotated with `@TransactionalEventListener`(phase = AFTER_COMMIT) that calls archiveToS3(formId) and handles retries/errors; apply the same change to the approve path (lines referenced 201-209) so both flows archive only after successful commit.
213-213:⚠️ Potential issue | 🔴 CriticalDo not return the generated password in the API response.
rawPasswordis a credential; returning it exposes it to browser history, proxies, logs, and any caller with approval access. Return a generic approval result and deliver account setup through a reset/invite flow.🛡️ Proposed fix
- return "Employment has been approved, generated password for new employee: " + rawPassword; + return "Employment has been approved";🤖 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` at line 213, The method in EmploymentFormService currently returns the generated credential by concatenating rawPassword into the response; remove any exposure of rawPassword from the API return value and instead return a generic success message (e.g., "Employment approved; account setup initiated") from the method that contains the return statement referencing rawPassword, and trigger a secure invite/reset flow (for example by calling the existing invite/email or createPasswordResetToken method) to deliver credentials out-of-band; ensure rawPassword is only used internally for account creation and not included in logs, responses, or exceptions.
193-198:⚠️ Potential issue | 🟠 MajorAdd SSN re-validation in
approveAndFinalizeEmployment()before creating the Staff record.Between form creation and approval, another employee with the same SSN could be added through a different flow. The approval method bypasses the existing
validateSsnNotExists()check, creating a race condition. CallvalidateSsnNotExists(form.getSocialSecurityNumber())before line 195 to validate against both the forms and staff tables, preventing a database-level exception.Proposed fix
if (form.getStatus() != ApprovalStatus.PENDING) { throw new IllegalStateException("Only PENDING forms can be approved. Current status: " + form.getStatus()); } + validateSsnNotExists(form.getSocialSecurityNumber()); + // Create and save new staff first (all DB operations before S3 write) Staff newStaff = employmentMapper.formToStaff(form);🤖 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 193 - 198, In approveAndFinalizeEmployment(), re-run the SSN uniqueness check before creating the Staff record: call validateSsnNotExists(form.getSocialSecurityNumber()) immediately before constructing the Staff (before employmentMapper.formToStaff(form) / newStaff creation) so the method verifies the SSN against current forms and staff records and aborts with the same validation error if a duplicate was added in the meantime.
♻️ Duplicate comments (3)
src/main/resources/static/js/ticket-detail.js (1)
53-63:⚠️ Potential issue | 🟡 Minor
loadComments(id)is still fire-and-forget; the outercatchwon't see its failures.
loadCommentsis now properly wrapped intry/catchinternally, but the call on Line 53 is not awaited, so any error thrown before it sets up its own try/catch (e.g. the element lookup, or a thrown error from an earlier microtask) will surface as an unhandled rejection rather than falling intoloadTicketDetail's catch on Line 61. Also, the outer catch swallows the error silently without logging — making production debugging hard.🛠️ Small fix
- loadComments(id); + await loadComments(id); @@ - } catch (e) { - container.innerHTML = "<p>Något gick fel.</p>"; - } + } catch (e) { + console.error("Error loading ticket detail", e); + container.innerHTML = "<p>Något gick fel.</p>"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/ticket-detail.js` around lines 53 - 63, The call to loadComments(id) is not awaited so its early failures can escape loadTicketDetail's catch and become unhandled rejections; change the call to await loadComments(id) in the same async function and update the outer catch to log the caught error (e.g., console.error or your logger) before setting container.innerHTML so failures are visible; locate the call to loadComments and the surrounding try/catch (references: loadComments(id), t.assignedStaff, container.innerHTML) and make these two changes.src/main/resources/static/js/dashboard.js (2)
80-115:⚠️ Potential issue | 🟠 MajorAssignment dropdown is still single-select — previous assignees get overwritten.
The
<select class="dashboard-assignment-select">does not have amultipleattribute, so it is a single-select regardless of how manyselectedoptions the template emits (browsers keep only the lastselectedvisible). When the user picks anyone,Array.from(e.target.selectedOptions)yields exactly one id, and thePUT /tickets/{id}/assignbody becomes{staffIds: [chosenId]}— silently wiping out all other assignees on that ticket. This is the same regression that was previously flagged; the handler was updated to readselectedOptions, but themultipleattribute never made it into the rendered markup.🐛 Proposed fix
- <select class="dashboard-assignment-select" data-id="${t.id}" style="max-width: 150px;"> - <option value="" disabled>Tilldela...</option> + <select class="dashboard-assignment-select" data-id="${t.id}" multiple style="max-width: 150px;"> + <option value="" disabled>Tilldela...</option>If a multi-select control doesn't fit the dashboard UX, at minimum merge the chosen id with
assignedIdsbefore PUT-ing so existing assignees are preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/dashboard.js` around lines 80 - 115, The assignment select is rendered as a single-select so choosing a staff overwrites other assignees; update the template that builds the <select class="dashboard-assignment-select" data-id="${t.id}" ...> (the code that maps allStaff and uses assignedIds) to include the multiple attribute so it becomes a true multi-select, keeping the existing selected options, and leave the existing change handler (which reads e.target.selectedOptions, maps to staffIds, calls apiFetch(`/tickets/${ticketId}/assign`, ...) and then loadDashboardTickets()) unchanged; alternatively, if you prefer single-select UX, modify the change handler to merge the newly chosen id with the ticket's existing assignedIds before sending the PUT instead of replacing them.
62-78:⚠️ Potential issue | 🟡 MinorInline
onclick/onkeydowninterpolate rawt.idand block a strict CSP.Two issues in this template:
t.idis embedded directly inside a JS string literal inside an HTML attribute (onclick="window.location.href='/pages/ticket-detail.html?id=${t.id}'").escapeHtmlis not applied here, and even if it were,'wouldn't be interpreted as an apostrophe-closing character in JS. Ift.idis ever not a plain number (UUID string, slug, etc.) this becomes injection-prone. Same for${t.status}inbadge-${t.status}anddata-id="${t.id}"on the selects below.- The inline handlers make it impossible to adopt a strict
script-srcCSP for the app.Prefer building the card with DOM APIs (or at minimum, delegating clicks via a single listener that reads
data-ticket-id+encodeURIComponent):🛠️ Sketch
- item.innerHTML = ` - <div class="ticket-main" - role="link" - tabindex="0" - aria-label="Ticket ${t.id}: ${escapeHtml(t.title)}" - onclick="window.location.href='/pages/ticket-detail.html?id=${t.id}'" - onkeydown="if(event.key==='Enter'||event.key===' ')window.location.href='/pages/ticket-detail.html?id=${t.id}'"> + const href = `/pages/ticket-detail.html?id=${encodeURIComponent(t.id)}`; + item.innerHTML = ` + <a class="ticket-main" href="${escapeHtml(href)}" + aria-label="Ticket ${escapeHtml(String(t.id))}: ${escapeHtml(t.title)}">…and drop the
onclick/onkeydownentirely (an<a>is keyboard-accessible by default).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/dashboard.js` around lines 62 - 78, The template currently injects raw values via item.innerHTML (notably t.id in onclick/onkeydown and badge-${t.status}/badge-${t.priority}), which risks injection and blocks strict CSP; replace the HTML string construction by creating DOM nodes (or at minimum set data-ticket-id on the root element and remove the inline onclick/onkeydown), assign textContent for titles and use classList.add with a whitelist/sanitizer for status/priority values (referencing ticket-main, ticket-header, badge-${t.status}, badge-${t.priority}, escapeHtml, and t.id), and wire navigation via an external event listener or an <a> element that builds the URL using encodeURIComponent(data-ticket-id) to avoid inline JS and ensure safe encoding of t.id.
🤖 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/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Line 62: Replace any logging that prints Staff objects or PII (e.g., hrStaff,
staff.getEmail(), or direct Staff instances) with the staff identifier only;
locate logger calls in EmploymentFormService that currently use
savedForm.getId() with hrStaff.getEmail() and other logger.* calls that pass
Staff objects, and change them to use staff.getId() or a staffId variable (e.g.,
logger.info("... by staffId: {}", staff.getId())). Ensure no logger statements
pass the Staff instance or email/password/SSN fields so Lombok's toString()
cannot leak credentials.
In `@src/main/resources/static/js/common.js`:
- Around line 37-45: In apiFetch, when response.status === 401 you should
short-circuit after calling logout() so callers cannot continue using the
response; modify the response.status === 401 branch in apiFetch to call logout()
and then immediately throw a rejected error (e.g., throw new
Error('Unauthorized') or return Promise.reject(...)) instead of returning the
response, ensuring callers will not attempt to read the body after navigation.
Use the existing apiFetch and logout symbol names so the change is easy to find.
- Around line 18-28: The fallback URL in safeImageUrl should be replaced with a
local asset or inline SVG data URL to avoid third-party failure; update the
fallback constant inside the safeImageUrl function (currently set to
"https://via.placeholder.com/40") to point to your local default avatar path
(e.g., "/images/default-avatar.svg") or an inline data:image/svg+xml;base64,...
string, keep the existing URL parsing/validation logic (const parsed = new
URL(...), protocol check for "http:"/"https:"), and ensure the catch branch also
returns the same new fallback so all failure paths use the local/data fallback.
In `@src/main/resources/static/js/dashboard.js`:
- Around line 6-45: loadDashboardTickets currently causes duplicate ticket
fetches on each keystroke and refetches staff every render; debounce the search
input by wiring `#searchInput` to a 250–300ms debounced call to
loadDashboardTickets (create a debounce helper and use it when adding the
"input" listener), move any /staff fetch out of loadDashboardTickets into a
cached module-level allStaff variable populated once on init (e.g.,
populateAllStaff() called on page load and have loadDashboardTickets read from
allStaff), stop fetching the entire ticket list for client-side counts by
calling a dedicated stats endpoint (replace the
apiFetch(`/tickets?status=&priority=&search=&assignedStaffId=`) call inside
loadDashboardTickets with
apiFetch(`/tickets/stats?status=${s}&priority=${p}&search=${q}&assignedStaffId=${staffId}`)
or a single /tickets/stats call), and add simple request sequencing/cancellation
(use an AbortController or incremental requestId in loadDashboardTickets to
ignore out-of-order responses from apiFetch) so slow responses cannot overwrite
newer UI state.
In `@src/main/resources/static/js/ticket-detail.js`:
- Line 17: The assignment to document.getElementById("editTicketLink").href can
throw if the element is missing; change the code to first retrieve the element
into a variable (e.g., const editLink =
document.getElementById("editTicketLink")), check for null (if (editLink) { ...
}) or use optional chaining before setting href, and ensure you also guard t.id
exists before interpolating; alternatively move this logic to run after
DOMContentLoaded so the element is present.
- Around line 19-32: The attachments rendering calls an undefined safeHttpUrl,
causing a runtime ReferenceError; either replace safeHttpUrl(a.downloadUrl) with
the existing safeImageUrl(a.downloadUrl) call or add a generic safeHttpUrl
helper to common.js and use it here (ensure it mirrors safeImageUrl behavior for
http/https and returns a safe fallback like "#"); update the template in the
attachmentsHtml construction (and keep escapeHtml around the filename) so the
anchor href uses the defined safe helper.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 144-152: The S3 upload (archiveToS3) is happening inside the DB
transaction and must be deferred until after the transaction commits to avoid
orphaned S3 objects; refactor EmploymentFormService to remove direct calls to
archiveToS3 from the reject/approve flow (the blocks that call
form.setStatus(...), form.setApprovedBy(...),
employmentFormRepository.save(form)) and instead publish a post-commit
action—either register a TransactionSynchronization via
TransactionSynchronizationManager.registerSynchronization or emit an event
handled by a method annotated with `@TransactionalEventListener`(phase =
AFTER_COMMIT) that calls archiveToS3(formId) and handles retries/errors; apply
the same change to the approve path (lines referenced 201-209) so both flows
archive only after successful commit.
- Line 213: The method in EmploymentFormService currently returns the generated
credential by concatenating rawPassword into the response; remove any exposure
of rawPassword from the API return value and instead return a generic success
message (e.g., "Employment approved; account setup initiated") from the method
that contains the return statement referencing rawPassword, and trigger a secure
invite/reset flow (for example by calling the existing invite/email or
createPasswordResetToken method) to deliver credentials out-of-band; ensure
rawPassword is only used internally for account creation and not included in
logs, responses, or exceptions.
- Around line 193-198: In approveAndFinalizeEmployment(), re-run the SSN
uniqueness check before creating the Staff record: call
validateSsnNotExists(form.getSocialSecurityNumber()) immediately before
constructing the Staff (before employmentMapper.formToStaff(form) / newStaff
creation) so the method verifies the SSN against current forms and staff records
and aborts with the same validation error if a duplicate was added in the
meantime.
---
Duplicate comments:
In `@src/main/resources/static/js/dashboard.js`:
- Around line 80-115: The assignment select is rendered as a single-select so
choosing a staff overwrites other assignees; update the template that builds the
<select class="dashboard-assignment-select" data-id="${t.id}" ...> (the code
that maps allStaff and uses assignedIds) to include the multiple attribute so it
becomes a true multi-select, keeping the existing selected options, and leave
the existing change handler (which reads e.target.selectedOptions, maps to
staffIds, calls apiFetch(`/tickets/${ticketId}/assign`, ...) and then
loadDashboardTickets()) unchanged; alternatively, if you prefer single-select
UX, modify the change handler to merge the newly chosen id with the ticket's
existing assignedIds before sending the PUT instead of replacing them.
- Around line 62-78: The template currently injects raw values via
item.innerHTML (notably t.id in onclick/onkeydown and
badge-${t.status}/badge-${t.priority}), which risks injection and blocks strict
CSP; replace the HTML string construction by creating DOM nodes (or at minimum
set data-ticket-id on the root element and remove the inline onclick/onkeydown),
assign textContent for titles and use classList.add with a whitelist/sanitizer
for status/priority values (referencing ticket-main, ticket-header,
badge-${t.status}, badge-${t.priority}, escapeHtml, and t.id), and wire
navigation via an external event listener or an <a> element that builds the URL
using encodeURIComponent(data-ticket-id) to avoid inline JS and ensure safe
encoding of t.id.
In `@src/main/resources/static/js/ticket-detail.js`:
- Around line 53-63: The call to loadComments(id) is not awaited so its early
failures can escape loadTicketDetail's catch and become unhandled rejections;
change the call to await loadComments(id) in the same async function and update
the outer catch to log the caught error (e.g., console.error or your logger)
before setting container.innerHTML so failures are visible; locate the call to
loadComments and the surrounding try/catch (references: loadComments(id),
t.assignedStaff, container.innerHTML) and make these two changes.
🪄 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: 9bdd3e36-ec70-4aff-a47b-03946f28fbf5
⛔ Files ignored due to path filters (1)
src/main/resources/static/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (7)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/resources/static/js/common.jssrc/main/resources/static/js/dashboard.jssrc/main/resources/static/js/ticket-detail.jssrc/main/resources/static/pages/create-ticket.htmlsrc/main/resources/static/pages/ticket-detail.htmlsrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/static/pages/create-ticket.html
- src/main/resources/static/pages/ticket-detail.html
- src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
138-142: Consider dropping the redundant runtime role checks.
@PreAuthorize("hasAnyRole('CEO', 'CTO', 'ADMIN')")onrejectForm(L129) andapproveAndFinalizeEmployment(L181) already enforces exactly the same role set that's re-validated at L138–141 and L185–186. Unless you want explicit defense-in-depth against@PreAuthorizebeing bypassed (e.g., direct method invocation from another service), these blocks are dead branches and duplicate the allow-list in two places, which makes future role changes error-prone (the past ADMIN-inconsistency bug is a good example).If kept intentionally as defense-in-depth, consider a short comment to that effect; otherwise they can be removed. Applies equally to L185–186.
Also applies to: 185-187
🤖 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 138 - 142, The runtime role-check block that inspects loggedInRejecter.getRole() inside rejectForm (and the analogous check inside approveAndFinalizeEmployment) duplicates the `@PreAuthorize`("hasAnyRole('CEO','CTO','ADMIN')") annotation and should be removed to avoid duplicate allow-lists; locate the conditional that throws IllegalStateException in EmploymentFormService (the loggedInRejecter.getRole() comparisons) and delete that entire if/throw branch, or if you want defense-in-depth keep the branch but replace it with a one-line comment stating it exists intentionally for defense-in-depth to avoid silent duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 138-142: The runtime role-check block that inspects
loggedInRejecter.getRole() inside rejectForm (and the analogous check inside
approveAndFinalizeEmployment) duplicates the
`@PreAuthorize`("hasAnyRole('CEO','CTO','ADMIN')") annotation and should be
removed to avoid duplicate allow-lists; locate the conditional that throws
IllegalStateException in EmploymentFormService (the loggedInRejecter.getRole()
comparisons) and delete that entire if/throw branch, or if you want
defense-in-depth keep the branch but replace it with a one-line comment stating
it exists intentionally for defense-in-depth to avoid silent duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a75a7c39-7ccb-4040-a083-5530e8b9058f
📒 Files selected for processing (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
Merged
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
3 participants
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.
🚀 Employment Management + Role Update
🧩 Vad som har gjorts
Denna PR introducerar en ny Employment-sektion i dashboarden samt uppdaterar roller/struktur för användare.
👥 Employment (ny funktion)
Ny sida:
/pages/employment.htmlFunktionalitet:
Visa alla anställda
Sök + filtrera på roll och department
Skapa ny staff (endast HR & ADMIN)
Approval-flöde:
CTO & CEO kan approve/reject
Vid approve skapas en riktig user i systemet
🔐 Rollbaserad access
Funktion | Roller -- | -- Skapa staff | HR, ADMIN Approve staff | CEO, CTO Se anställda | Alla🧱 Strukturförbättring (frontend)
Separat sida för employment
Tydligare uppdelning av funktioner i JS
Återanvänder befintliga API-endpoints (
/forms,/staff)🧪 Hur man testar
1. HR / ADMIN
Gå till Employment
Klicka “Lägg till staff”
Skapa ny person
Kontrollera att den hamnar i Pending
2. CEO / CTO
Gå till Employment
Se Pending approvals
Klicka Approve
Verifiera att personen dyker upp i “Alla anställda”
📦 Påverkan
Ingen backend-ändring
Endast frontend + databasupdate (role/department)
✅ Resultat
Fullt fungerande employment flow
Tydlig rollhantering
Bättre struktur i dashboarden
Summary by CodeRabbit
New Features
Improvements
Other