Feature/pages - #66
Closed
Ericthilen wants to merge 32 commits into
Closed
Conversation
Ericthilen
requested review from
alicewersen-rgb,
codebyNorthsteep and
gitnes94
April 15, 2026 11:41
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverted many Lombok-generated models/DTOs to explicit getters/setters, added multi-assignee tickets with ticketCode and DB migrations, updated ticket service/repository/controller and frontend (JS/CSS/pages), added activity-log assignment support, and adjusted security, JWT key handling, and global exception logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Frontend as Frontend (app.js)
participant Backend as TicketController
participant Service as TicketService
participant Repo as TicketRepository
participant DB as Database
Browser->>Frontend: submit create ticket (form)
Frontend->>Backend: POST /api/tickets {dto, token}
Backend->>Service: createTicket(dto, creatorEmail)
Service->>Repo: find creator by email
Repo->>DB: SELECT staff WHERE email=...
DB-->>Repo: staff row
Service->>Repo: findAllById(assignedStaffIds)
Repo->>DB: SELECT staff WHERE id IN (...)
DB-->>Repo: staff list
Service->>Repo: save(ticket) -- obtain id
Repo->>DB: INSERT ticket -> id
Service->>Service: generate ticketCode
Service->>Repo: save(ticket) -- update ticket_code
Repo->>DB: UPDATE ticket_code
Service-->>Backend: TicketResponseDTO
Backend-->>Frontend: 201 Created with ticket DTO
Frontend-->>Browser: redirect to ticket detail
sequenceDiagram
participant AdminBrowser
participant Frontend as Frontend (app.js)
participant Backend as TicketController
participant Service as TicketService
participant Repo as TicketRepository
participant Activity as ActivityLogService
participant DB as Database
AdminBrowser->>Frontend: select multiple assignees + click assign
Frontend->>Backend: PUT /api/tickets/{id}/assign {staffIds}
Backend->>Service: assignTicket(ticketId, staffIds, assignedById)
Service->>Repo: findById(ticketId)
Repo->>DB: SELECT ticket JOIN assignedStaff...
DB-->>Repo: ticket with relations
Service->>Repo: findAllById(staffIds)
Repo->>DB: SELECT staff WHERE id IN (...)
DB-->>Repo: staff list
Service->>Repo: save(ticket with assignedStaff)
Repo->>DB: INSERT/UPDATE ticket_assignments
Service->>Activity: logAssignmentChange(ticket, performedBy, staffList)
Activity->>DB: INSERT activity_log
Service-->>Backend: updated TicketResponseDTO
Backend-->>Frontend: 200 OK
Frontend-->>AdminBrowser: refresh ticket view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/model/EmploymentForm.java (1)
18-19:⚠️ Potential issue | 🔴 CriticalDuplicate accessor methods will cause compilation errors.
The class retains
@Getterand@Setterannotations (lines 18-19) while also defining explicit getter/setter methods (lines 78-103). Lombok will generate methods with the same signatures, causing duplicate method compilation errors.Either remove the Lombok annotations or remove the explicit methods.
🐛 Proposed fix: Remove Lombok annotations
package org.example.cyberwatch.features.form.model; import jakarta.persistence.*; import jakarta.validation.constraints.*; -import lombok.Getter; -import lombok.Setter; import org.example.cyberwatch.features.staff.model.Staff; import org.example.cyberwatch.shared.model.enums.ApprovalStatus; import org.example.cyberwatch.shared.model.enums.Department; @@ -15,8 +13,6 @@ import java.time.LocalDateTime; //Could it be an idea that the EmployeeForm needs to be approved by an Manager, like a signature on a paper form? // Then we could have a status field in the EmployeeForm with the following states: //DRAFT -> SUBMITTED -> APPROVED -> COMPLETED -> REJECTED -@Getter -@Setter `@Entity` public class EmploymentForm {Also applies to: 78-103
🤖 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/model/EmploymentForm.java` around lines 18 - 19, The EmploymentForm class currently has Lombok `@Getter/`@Setter and also defines explicit accessor methods (e.g., getEmployer, setEmployer, getPosition, setPosition, etc.), which leads to duplicate method compilation errors; fix this by removing the Lombok annotations from the EmploymentForm class (or alternatively delete the explicit accessor methods) so only one source generates the getters/setters—preferably remove `@Getter/`@Setter at the top of EmploymentForm to keep the explicit methods intact (or vice versa) and ensure no duplicate accessor signatures remain.src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java (1)
89-95:⚠️ Potential issue | 🟠 Major
assignedByIdis still forgeable.This endpoint still trusts the caller to say who performed the assignment. Now that authentication exists, clients can impersonate another staff member in the audit trail by choosing any
assignedById.Please resolve the actor server-side from the authenticated user here as well, and remove
assignedByIdfrom the public request contract.Based on learnings, these caller-controlled actor IDs were already identified as a security problem and were meant to be removed once authentication was in place.
🤖 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/ticket/controller/TicketController.java` around lines 89 - 95, The assignTicket endpoint is trusting a caller-supplied assignedById (forgeable); remove the assignedById request parameter from TicketController.assignTicket and resolve the actor server-side from the authenticated principal (e.g., via `@AuthenticationPrincipal` or SecurityContextHolder) and pass that resolved staffId into ticketService.assignTicket(ticketId, dto.getStaffIds(), resolvedStaffId); also update the public contract (remove assignedById from AssignTicketDTO/endpoint) and adjust any downstream callers/tests to use the authenticated actor instead.
🟡 Minor comments (3)
src/main/resources/static/auth/callback.html-1-27 (1)
1-27:⚠️ Potential issue | 🟡 MinorThis callback page is currently unreachable.
src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.java:32-62redirects to/pages/auth-callback.html, not/auth/callback.html, so this file will never receive the OAuth2 redirect in its current state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/auth/callback.html` around lines 1 - 27, The callback HTML is unreachable because OAuth2SuccessHandler (class OAuth2SuccessHandler.java) redirects to /pages/auth-callback.html while the file is located at /auth/callback.html; update one side to match the other: either rename/move src/main/resources/static/auth/callback.html to src/main/resources/static/pages/auth-callback.html, or change the redirect path in OAuth2SuccessHandler.redirect (the method performing the redirect) from "/pages/auth-callback.html" to "/auth/callback.html" so the OAuth2 redirect reaches the callback page.src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorRemove the unused Lombok import.
This class has an explicit constructor and does not use the
@RequiredArgsConstructorannotation, making the import unnecessary.Suggested fix
-import lombok.RequiredArgsConstructor;🤖 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/controller/StaffRestController.java` at line 3, The file imports lombok.RequiredArgsConstructor but the StaffRestController class defines an explicit constructor and does not use the `@RequiredArgsConstructor` annotation; remove the unused import line for RequiredArgsConstructor to clean up imports and avoid unnecessary dependency usage—look for the import statement "import lombok.RequiredArgsConstructor;" at the top of StaffRestController and delete it, leaving the explicit constructor and other imports untouched.src/main/resources/static/css/styles.css-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorFix the Stylelint failure on
font-family.
Intershould not be quoted here, otherwise this keeps the stylesheet failing lint.Suggested fix
-body { font-family: 'Inter', sans-serif; background: var(--bg); color: var(--text); } +body { font-family: Inter, sans-serif; background: var(--bg); color: var(--text); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/styles.css` at line 10, The Stylelint error is caused by quoting the generic font name in the body rule's font-family; update the body selector's font-family declaration used in styles.css (the body { font-family: ... } rule) to use Inter without quotes (and keep the fallback generic sans-serif) so the value is valid for Stylelint.
🧹 Nitpick comments (16)
src/main/java/org/example/cyberwatch/features/ticket/model/AssignTicketDTO.java (1)
6-11: Add DTO validation forstaffIdsto fail earlier with consistent 400s.Right now validation is deferred to service logic. Adding bean validation here gives clearer API errors and avoids generic runtime exceptions.
♻️ Proposed refactor
+import jakarta.validation.constraints.NotEmpty; +import jakarta.validation.constraints.NotNull; import java.util.List; ... - private List<Long> staffIds; + `@NotEmpty`(message = "At least one staff member must be selected") + private List<@NotNull(message = "staffIds cannot contain null values") Long> staffIds;🤖 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/ticket/model/AssignTicketDTO.java` around lines 6 - 11, AssignTicketDTO currently defers validation of the staffIds list to service logic; annotate the staffIds field in AssignTicketDTO with appropriate bean validation annotations (e.g., `@NotNull` and `@Size`(min=1) or `@NotEmpty`) so requests with missing/empty staffIds produce a 400; ensure imports for javax.validation.constraints are added and that callers (controller methods that accept AssignTicketDTO) use `@Valid` on the request body so the validation is triggered; keep existing getStaffIds and setStaffIds unchanged.src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
4-4: Remove unused Lombok import.The
@RequiredArgsConstructorimport is no longer needed since the annotation was replaced with an explicit constructor.🧹 Proposed cleanup
package org.example.cyberwatch.features.form.service; import jakarta.persistence.EntityNotFoundException; -import lombok.RequiredArgsConstructor; import org.apache.commons.lang3.RandomStringUtils;🤖 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 4, Remove the unused Lombok import by deleting the line that imports lombok.RequiredArgsConstructor; the EmploymentFormService class no longer uses the `@RequiredArgsConstructor` annotation because an explicit constructor was added, so simply remove the import statement to clean up unused imports.src/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.java (2)
12-55: Consider moving field declarations before methods.The implementation is functionally correct. However, placing field declarations (lines 41-55) after constructors and methods is unconventional in Java. Most style guides recommend: fields → constructors → methods.
🤖 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/StaffDTO.java` around lines 12 - 55, The class StaffDTO currently declares its private fields (id, socialSecurityNumber, firstName, lastName, email, phoneNumber, role, department) after its constructors and accessor methods; move those field declarations to the top of the class, directly after the class declaration and before the no-arg and parameterized constructors, so the order becomes: fields → constructors → getters/setters (affecting StaffDTO).
3-6: Remove unused Lombok imports.These imports are no longer needed since the class no longer uses Lombok annotations.
🧹 Proposed cleanup
package org.example.cyberwatch.features.staff.model; -import lombok.AllArgsConstructor; -import lombok.Getter; -import lombok.NoArgsConstructor; -import lombok.Setter; import org.example.cyberwatch.shared.model.enums.Department; import org.example.cyberwatch.shared.model.enums.Role;🤖 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/StaffDTO.java` around lines 3 - 6, The StaffDTO class no longer uses Lombok annotations, so remove the unused imports to clean up the file: delete the import lines for lombok.AllArgsConstructor, lombok.Getter, lombok.NoArgsConstructor, and lombok.Setter from the top of StaffDTO.java so only required imports remain and no unused Lombok imports are present.src/main/java/org/example/cyberwatch/features/form/model/UpdateEmploymentDTO.java (1)
4-7: Remove unused Lombok imports.These imports are no longer needed since the class no longer uses Lombok annotations.
🧹 Proposed cleanup
package org.example.cyberwatch.features.form.model; import jakarta.validation.constraints.*; -import lombok.AllArgsConstructor; -import lombok.Getter; -import lombok.NoArgsConstructor; -import lombok.Setter; import org.example.cyberwatch.shared.model.enums.Department; import org.example.cyberwatch.shared.model.enums.Role;🤖 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/model/UpdateEmploymentDTO.java` around lines 4 - 7, The UpdateEmploymentDTO class still imports lombok.AllArgsConstructor, lombok.Getter, lombok.NoArgsConstructor, and lombok.Setter but no Lombok annotations are used; remove those four unused imports from the top of UpdateEmploymentDTO and keep only required imports so the class compiles without unused-import warnings.src/main/java/org/example/cyberwatch/features/staff/model/Staff.java (1)
3-14: Remove unused imports.The following imports are no longer used after removing Lombok annotations and the JPA relationship fields:
- Lines 5-6:
lombok.Getter,lombok.Setter(annotations removed)- Lines 7-9:
EmploymentForm,ReportForm,Ticket(relationship fields removed)- Lines 13-14:
HashSet,Set(no longer needed)🧹 Proposed cleanup
package org.example.cyberwatch.features.staff.model; import jakarta.persistence.*; import jakarta.validation.constraints.*; -import lombok.Getter; -import lombok.Setter; -import org.example.cyberwatch.features.form.model.EmploymentForm; -import org.example.cyberwatch.features.form.model.ReportForm; -import org.example.cyberwatch.features.ticket.model.Ticket; import org.example.cyberwatch.shared.model.enums.Department; import org.example.cyberwatch.shared.model.enums.Role; - -import java.util.HashSet; -import java.util.Set;🤖 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 3 - 14, The Staff class has leftover unused imports—remove lombok.Getter and lombok.Setter, the relationship model imports EmploymentForm, ReportForm, Ticket, and the collection imports HashSet and Set; update the import list to only include the actually used symbols (e.g., jakarta.persistence.*, jakarta.validation.constraints.*, org.example.cyberwatch.shared.model.enums.Department, org.example.cyberwatch.shared.model.enums.Role), then run your IDE's "optimize/organize imports" or rebuild to ensure no unused imports remain and the file compiles.src/main/resources/static/index.html (1)
1-11: Landing page redirect is correctly configured.The meta refresh properly redirects to
/pages/login.html, which is permitted publicly perSecurityConfig(lines 53-62). The Swedish language attribute matches the content.Consider adding a fallback link for accessibility in edge cases where meta refresh is disabled:
♿ Optional: Add fallback link
<body> <p>Skickar vidare till login...</p> +<p><a href="/pages/login.html">Klicka här om du inte omdirigeras automatiskt</a></p> </body>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/index.html` around lines 1 - 11, Add an accessible fallback link in index.html so users with meta-refresh disabled can still navigate: update the body (near the existing <p>Skickar vidare till login...</p>) to include a visible anchor pointing to /pages/login.html (e.g., "Click here to continue to login") and ensure the link text is in Swedish to match lang="sv"; keep the existing meta refresh and message.src/main/java/org/example/cyberwatch/features/ticket/model/TicketDTO.java (2)
5-5: Remove unused import.The
@Positiveannotation is imported but not used anywhere in this class.-import jakarta.validation.constraints.Positive;🤖 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/ticket/model/TicketDTO.java` at line 5, The import jakarta.validation.constraints.Positive is unused in the TicketDTO class; remove that import statement from TicketDTO.java to clean up unused imports and avoid IDE/compiler warnings (no other code changes required in the TicketDTO class).
37-40: Consider adding validation forassignedStaffIds.The service layer validates that
assignedStaffIdsis non-empty (seeTicketService.createTicket), but adding@NotEmptyhere would provide early validation with a clearer error message at the controller level.♻️ Suggested enhancement
+import jakarta.validation.constraints.NotEmpty; + - private List<Long> assignedStaffIds; + `@NotEmpty`(message = "Du måste välja minst en person att tilldela ärendet till") + private List<Long> assignedStaffIds;🤖 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/ticket/model/TicketDTO.java` around lines 37 - 40, Add bean validation to the TicketDTO.assignedStaffIds field by annotating the field (the private List<Long> assignedStaffIds) with `@NotEmpty` (and optionally `@NotNull` if you want to forbid null vs empty) so the controller-level validation fails fast with a clear message; import the correct annotation (javax.validation.constraints.NotEmpty) and ensure the DTO is validated in controller endpoints (e.g., `@Valid` on the request body) so TicketService.createTicket's non-empty requirement is enforced earlier.src/main/resources/static/pages/create-ticket.html (1)
6-6: Minor: Title language inconsistency.The
<title>is in English ("Create Ticket") while the page content is in Swedish ("Skapa ny ticket"). Consider aligning the title with the page language for consistency.- <title>Create Ticket</title> + <title>Skapa ny ticket</title>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/pages/create-ticket.html` at line 6, The page title string in the <title> element is English ("Create Ticket") while the page content is Swedish; update the <title> element to match the page language (e.g., change the <title> content to "Skapa ny ticket" or the preferred Swedish phrasing) so the page header and body are consistent.src/main/java/org/example/cyberwatch/features/ticket/model/Ticket.java (1)
4-5: Remove unused Lombok imports.The
@Getterand@Setterannotations are imported but no longer used since explicit accessor methods were added (lines 70-95).♻️ Suggested fix
-import lombok.Getter; -import lombok.Setter;🤖 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/ticket/model/Ticket.java` around lines 4 - 5, The Ticket class still imports lombok.Getter and lombok.Setter even though explicit getX/setX methods exist; remove the unused imports (the two import lines referencing lombok.Getter and lombok.Setter) from the top of the Ticket class to eliminate dead imports and any unused-warning complaints, leaving the class-level accessors as-is.src/main/java/org/example/cyberwatch/features/ticket/model/TicketResponseDTO.java (1)
3-4: Remove unused Lombok imports.The
@Getterand@Setterannotations are imported but no longer used since explicit accessor methods were added.-import lombok.Getter; -import lombok.Setter;🤖 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/ticket/model/TicketResponseDTO.java` around lines 3 - 4, Remove the now-unused Lombok imports for `@Getter` and `@Setter` from the TicketResponseDTO class: delete the import lines "import lombok.Getter;" and "import lombok.Setter;" since explicit accessor methods exist and the annotations are not used; ensure no other Lombok symbols are referenced in TicketResponseDTO after removal.src/main/resources/static/pages/dashboard.html (2)
62-65: Dead code:changeevent on button element.Line 63 adds a
changeevent listener tofilterBtn, but button elements don't firechangeevents. This listener will never execute. Line 65 correctly usesonclickfor the button.♻️ Suggested fix: Remove filterBtn from the change listener loop
- ['filterBtn', 'statusFilter', 'priorityFilter', 'staffFilter'].forEach(id => { + ['statusFilter', 'priorityFilter', 'staffFilter'].forEach(id => { document.getElementById(id).addEventListener('change', loadDashboardTickets); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/pages/dashboard.html` around lines 62 - 65, The loop is attaching a 'change' listener to 'filterBtn' which never fires for buttons; remove 'filterBtn' from the array so only inputs/selects get change events (keep 'statusFilter', 'priorityFilter', 'staffFilter') and keep the separate document.getElementById('filterBtn').onclick = loadDashboardTickets; to wire the button click; update the array where it's defined and ensure loadDashboardTickets remains the handler referenced.
71-71: Consider debouncing search input.
loadDashboardTickets()is called on every keystroke (oninput), which triggers an API request for each character typed. This could cause performance issues and excessive server load.♻️ Suggested fix: Add debounce
+ let searchTimeout; - document.getElementById('searchInput').oninput = loadDashboardTickets; + document.getElementById('searchInput').oninput = () => { + clearTimeout(searchTimeout); + searchTimeout = setTimeout(loadDashboardTickets, 300); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/pages/dashboard.html` at line 71, The oninput handler on the element with id 'searchInput' calls loadDashboardTickets on every keystroke; replace that direct binding with a debounced wrapper to avoid firing an API call for each character. Implement a simple debounce helper (or reuse an existing one) and bind document.getElementById('searchInput').oninput to the debounced version of loadDashboardTickets (e.g., debounce(loadDashboardTickets, 300)), ensuring the debounce helper uses clearTimeout/setTimeout so repeated keystrokes reset the timer and only calls loadDashboardTickets after the quiet period.src/main/resources/static/pages/ticket-detail.html (1)
6-6: Minor: Title language inconsistency.Same as
create-ticket.html, the<title>is in English while page content is in Swedish.- <title>Ticket Detail</title> + <title>Ticketdetaljer</title>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/pages/ticket-detail.html` at line 6, The page title in ticket-detail.html is in English while the page body is Swedish; update the <title> element in ticket-detail.html to match the site's Swedish language (as done in create-ticket.html) by replacing "Ticket Detail" with the appropriate Swedish text (e.g., "Biljettdetaljer" or the exact phrase used in create-ticket.html) so the title language is consistent with the page content and other pages.src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (1)
3-3: Remove unused import.
@RequiredArgsConstructoris imported but not used since an explicit constructor is provided on lines 18-20.-import lombok.RequiredArgsConstructor;🤖 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/service/StaffService.java` at line 3, Remove the unused Lombok import by deleting the import statement for RequiredArgsConstructor since StaffService declares an explicit constructor; locate the import "import lombok.RequiredArgsConstructor;" at the top of the file and remove it so the class no longer has an unused import while keeping the existing explicit StaffService(...) constructor and other members unchanged.
🤖 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/security/JwtService.java`:
- Around line 34-43: The current getKey() silently pads/truncates the secret and
breaks existing tokens; change getKey() to either (A) enforce a minimum raw
secret length by throwing an IllegalArgumentException when
secret.getBytes().length < 32 so short secrets are rejected, or (B) perform a
deterministic, explicit key derivation (e.g., PBKDF2WithHmacSHA256/HKDF) only
when a new configurable flag (e.g., enableKeyDerivation) is true so behavior is
opt-in and existing tokens remain valid; ensure generateToken() and
isTokenValid() continue to call getKey() so they stay consistent, and
document/configure the migration flag so operators can opt into derivation
without silent token breakage.
In
`@src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.java`:
- Line 32: FRONTEND_URL is hardcoded to localhost which breaks deployed
redirects; replace it by building the redirect from the incoming
HttpServletRequest or external configuration. Remove the private static final
FRONTEND_URL constant in OAuth2SuccessHandler and either inject a configurable
property (e.g. `@Value`("${app.frontend.base-url}") String frontendBase) or, if
you want request-derived URLs, compute the base using request.getScheme(),
request.getServerName(), request.getServerPort() and request.getContextPath(),
then append "/pages/auth-callback.html"; update the onAuthenticationSuccess
method (or wherever redirect is generated) to use the injected/config-built URL
for the redirect. Ensure tests and configuration keys are added/updated
accordingly.
In `@src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java`:
- Around line 72-75: GlobalExceptionHandler currently prints stack traces to
System.err and returns e.getMessage() in the 500 response which can leak
internals; replace System.err.println and e.printStackTrace() with a proper
logger call (e.g., logger.error("Unexpected error in GlobalExceptionHandler",
e)) and change the response payload built in the method that creates the error
Map (the Map named "error" or the method handling generic Exception) to use a
generic message like "An unexpected error occurred" (do not include
e.getMessage()); keep the full exception on server logs only by passing the
exception object to the logger.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 22-26: Controller calls several service methods that do not exist;
restore the missing service API by adding implementations for getStaffById(Long
id), updateStaff(Long id, StaffRequestDTO request), deleteStaff(Long id), and
getStaffByRoleOrDepartment(String role, String department) in StaffService so
the controller compiles. Each method should use staffRepository (e.g., findById,
save, deleteById, and a custom query or repository method for role/department),
map entities to/from StaffResponseDTO/StaffRequestDTO (use
StaffResponseDTO.from(...) for responses), handle not-found cases (throw/return
as your project expects), and keep signatures matching what StaffController
invokes. Ensure to import/annotate any exceptions used and preserve existing
getAllStaff() behavior.
In
`@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java`:
- Around line 58-61: The createTicket method currently prints the creatorEmail
to stdout using System.out.println (PII) — remove that raw debug print and any
other direct stdout logging of creatorEmail; if you need diagnostics use the
class logger (e.g., logger.debug) and avoid including the full email (use
non-sensitive identifiers or masked value) when logging around
TicketService.createTicket, Staff creator, and staffRepository lookups. Ensure
the System.out.println line is deleted and replace with a privacy-safe logger
call if necessary.
- Around line 76-80: The code currently uses
staffRepository.findAllById(dto.getAssignedStaffIds()) and saves whatever was
found, allowing partial matches; change this to validate that the returned
List<Staff> size equals dto.getAssignedStaffIds().size() and throw a clear
exception if they differ (e.g., "One or more assigned staff IDs not found")
instead of proceeding to ticket.setAssignedStaff(...); apply the same exact
validation logic in the assignTicket(...) code path so both creation/update and
assignTicket reject requests with any missing staff IDs.
In `@src/main/resources/db/migration/V6__add_test_user.sql`:
- Around line 1-3: The migration V6__add_test_user.sql inserts real personal
data into the staff table (social_security_number, first_name, last_name, email,
phone_number) which must be replaced with synthetic placeholders; edit the
INSERT for table "staff" so the social_security_number is a clearly fake value
(not real SSN format), first_name/last_name and email are non-identifying dummy
values (e.g., Test User, test.user@example.com), phone_number is a fake number,
and the password can remain a valid hashed placeholder or be replaced with a
clearly marked dummy hash; keep the ON CONFLICT clause intact and ensure no real
personal identifiers remain in the VALUES list.
In `@src/main/resources/db/migration/V7__add_ticket_assignments_many_to_many.sql`:
- Around line 10-11: Before dropping FK_TICKETS_ON_ASSIGNED_TO and
assigned_to_id, copy existing assignments into the new ticket_assignments table
to avoid data loss: run an INSERT INTO ticket_assignments (ticket_id, user_id)
SELECT id, assigned_to_id FROM tickets WHERE assigned_to_id IS NOT NULL (or use
INSERT ... SELECT ... WHERE NOT EXISTS to avoid duplicates), then drop the FK
and column; reference the existing FK_TICKETS_ON_ASSIGNED_TO, column
assigned_to_id on table tickets and the ticket_assignments table in your
migration and perform the backfill prior to executing ALTER TABLE tickets DROP
CONSTRAINT ... and ALTER TABLE tickets DROP COLUMN ....
In `@src/main/resources/static/auth/callback.html`:
- Around line 8-25: The current callback script reads a JWT from the URL and
stores it via localStorage.setItem('jwt_token', token), which must be removed;
instead implement a one-time code exchange or have the server set a Secure,
HttpOnly, SameSite cookie on redirect so the frontend never receives the raw
JWT. Replace the redirect-with-token flow: stop reading/persisting token in
callback.html (remove URLSearchParams(token) usage and localStorage.setItem),
and either POST the one-time code to your backend token endpoint to obtain the
token server-side or ensure the authentication redirect returns a Set-Cookie for
the JWT; keep only client-side handling for error routing and redirection
(window.location.href) without storing the token in script-accessible storage.
In `@src/main/resources/static/js/app.js`:
- Around line 70-74: The code is inserting untrusted API data via string
interpolation into innerHTML which enables stored XSS (seen in staffOptions and
other mappings using template literals); fix by constructing DOM nodes instead
of HTML strings: replace usages like the staff.map/template that builds an
option string with creating an Option or document.createElement('option'), set
its value via setAttribute or .value, set classes via .classList or
.setAttribute, and assign the visible text via .textContent (or option.text) so
names, emails, titles, descriptions, and comments are never injected as HTML;
apply the same pattern to the other affected blocks (lines referenced 126-147,
205-222, 241-245) where template literals are used to populate innerHTML.
- Around line 356-362: The bug is that you build a full update payload (body
using editTitle, editDescription, editPriority, editStatus) but then call the
status-only endpoint (`/tickets/${id}/status`) so only status persists; update
the call to use the full-ticket update endpoint instead of the status-only route
(or change the endpoint to accept a JSON body), e.g. call apiFetch for the
ticket update (e.g. `/tickets/${id}`) with method "PATCH", include headers for
JSON and pass JSON.stringify(body) as the body; remove the `/status?status=...`
query usage so title/description/priority are included.
- Around line 132-160: The dashboard single-select handler currently overwrites
all assignees by always sending body { staffIds: [staffId] } to
apiFetch(`/tickets/${ticketId}/assign?...`); update the change listener for
'.dashboard-assignment-select' to preserve existing assignees: read the current
assigned IDs for the ticket (use the same source used when rendering, e.g., the
assignedIds array or a data attribute on the item), compute a new array that
merges existing assignee IDs with the newly selected staffId (or removes/updates
as appropriate for your UX), then call apiFetch with body JSON.stringify({
staffIds: mergedIds }) so the request sends the full list instead of replacing
it with a single ID.
- Line 1: The API_BASE constant currently hardcodes "http://localhost:8080",
which breaks cross-origin and HTTPS deployments; update the declaration of
API_BASE to use a same-origin value (e.g., a relative base like "/api" or derive
it from window.location.origin + "/api") so all requests use the current host
and protocol; modify the const API_BASE in app.js accordingly and ensure any
code that imports/uses API_BASE still works with the new relative/derived value.
- Around line 158-160: Requests in app.js are sending actor IDs from the browser
(e.g. query params assignedById/performedById and DOM-read uploadedById) which
makes audits forgeable; remove those client-supplied actor parameters from calls
that use apiFetch (e.g. the ticket assign call to `/tickets/${ticketId}/assign`
and any calls constructing `performedById`/`uploadedById`) and stop reading
actor IDs from the DOM (e.g. where `uploadedById` is read); instead send only
the necessary action payload (e.g. `staffIds` or file data) and rely on the
server to resolve the authenticated principal for audit fields, and update every
occurrence referenced in the comment (the assign endpoint, the performedById
usages and the uploadedById usages) to omit actor params.
In `@src/main/resources/static/pages/404.html`:
- Around line 1-18: The 404 HTML currently under pages won't be picked up by
Spring Boot's error handling; move the 404.html file into the application's
static error directory (the static "error" folder used by Spring Boot) so it
becomes the default 404 error page, and update the internal link if needed (the
anchor to dashboard.html); alternatively implement a custom ErrorController (a
class implementing Spring's ErrorController and mapping 404 responses to this
404.html) if you prefer programmatic handling.
In `@src/main/resources/static/pages/500.html`:
- Around line 1-18: The custom 500.html is placed under the app's static/pages
location so Spring Boot won't auto-discover it for HTTP 500 responses; move the
500.html file into the framework's error discovery directory (static/error) or
into templates/error if using Thymeleaf, keep the existing HTML and UTF-8 meta
tag, remove the old pages copy, and verify a 500 response now returns the moved
500.html via the default Spring Boot error handling.
In `@src/main/resources/static/pages/auth-callback.html`:
- Around line 16-24: The redirect branches in this file currently use
window.location.href which leaves the JWT-bearing callback URL in browser
history; replace all uses of window.location.href in this block with
location.replace(...) so the callback URL is not kept in the back stack (update
the branch that handles token success where localStorage.setItem("jwt_token",
token) / localStorage.setItem("token", token) occurs, the else-if error branch
that shows alert("Inloggningen misslyckades: " + error), and the final else
fallback) to call location.replace with the same target paths instead of
assigning window.location.href.
In `@src/main/resources/static/pages/edit-ticket.html`:
- Around line 21-59: The form currently only persists status — update the
behavior in setupEditTicketForm (JS) so the submit handler reads editTitle,
editDescription, editPriority and editStatus and sends a full ticket update
(e.g., PUT /tickets/{id} with title, description, priority, status) instead of
calling /tickets/{id}/status; ensure the payload keys match the server DTO and
handle response/error to update editMessage. Alternatively, if the API doesn’t
support full updates, reduce the form to a status-only editor by removing
title/description/priority inputs and changing the button text and labels to
reflect “Update status only” so the UI matches actual behavior.
In `@src/main/resources/static/pages/login.html`:
- Around line 25-28: The click handler for the element with id "googleLoginBtn"
currently redirects to a hard-coded origin; update the event listener (the
function attached via
document.getElementById("googleLoginBtn").addEventListener) to navigate to a
relative path like "/oauth2/authorization/google" (or construct the URL using
location.origin if you need explicit origin) so the OAuth entrypoint is
environment-agnostic and works behind proxies/HTTPS terminators.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java`:
- Around line 18-19: The EmploymentForm class currently has Lombok
`@Getter/`@Setter and also defines explicit accessor methods (e.g., getEmployer,
setEmployer, getPosition, setPosition, etc.), which leads to duplicate method
compilation errors; fix this by removing the Lombok annotations from the
EmploymentForm class (or alternatively delete the explicit accessor methods) so
only one source generates the getters/setters—preferably remove `@Getter/`@Setter
at the top of EmploymentForm to keep the explicit methods intact (or vice versa)
and ensure no duplicate accessor signatures remain.
In
`@src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java`:
- Around line 89-95: The assignTicket endpoint is trusting a caller-supplied
assignedById (forgeable); remove the assignedById request parameter from
TicketController.assignTicket and resolve the actor server-side from the
authenticated principal (e.g., via `@AuthenticationPrincipal` or
SecurityContextHolder) and pass that resolved staffId into
ticketService.assignTicket(ticketId, dto.getStaffIds(), resolvedStaffId); also
update the public contract (remove assignedById from AssignTicketDTO/endpoint)
and adjust any downstream callers/tests to use the authenticated actor instead.
---
Minor comments:
In
`@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Line 3: The file imports lombok.RequiredArgsConstructor but the
StaffRestController class defines an explicit constructor and does not use the
`@RequiredArgsConstructor` annotation; remove the unused import line for
RequiredArgsConstructor to clean up imports and avoid unnecessary dependency
usage—look for the import statement "import lombok.RequiredArgsConstructor;" at
the top of StaffRestController and delete it, leaving the explicit constructor
and other imports untouched.
In `@src/main/resources/static/auth/callback.html`:
- Around line 1-27: The callback HTML is unreachable because
OAuth2SuccessHandler (class OAuth2SuccessHandler.java) redirects to
/pages/auth-callback.html while the file is located at /auth/callback.html;
update one side to match the other: either rename/move
src/main/resources/static/auth/callback.html to
src/main/resources/static/pages/auth-callback.html, or change the redirect path
in OAuth2SuccessHandler.redirect (the method performing the redirect) from
"/pages/auth-callback.html" to "/auth/callback.html" so the OAuth2 redirect
reaches the callback page.
In `@src/main/resources/static/css/styles.css`:
- Line 10: The Stylelint error is caused by quoting the generic font name in the
body rule's font-family; update the body selector's font-family declaration used
in styles.css (the body { font-family: ... } rule) to use Inter without quotes
(and keep the fallback generic sans-serif) so the value is valid for Stylelint.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/model/UpdateEmploymentDTO.java`:
- Around line 4-7: The UpdateEmploymentDTO class still imports
lombok.AllArgsConstructor, lombok.Getter, lombok.NoArgsConstructor, and
lombok.Setter but no Lombok annotations are used; remove those four unused
imports from the top of UpdateEmploymentDTO and keep only required imports so
the class compiles without unused-import warnings.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Line 4: Remove the unused Lombok import by deleting the line that imports
lombok.RequiredArgsConstructor; the EmploymentFormService class no longer uses
the `@RequiredArgsConstructor` annotation because an explicit constructor was
added, so simply remove the import statement to clean up unused imports.
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 3-14: The Staff class has leftover unused imports—remove
lombok.Getter and lombok.Setter, the relationship model imports EmploymentForm,
ReportForm, Ticket, and the collection imports HashSet and Set; update the
import list to only include the actually used symbols (e.g.,
jakarta.persistence.*, jakarta.validation.constraints.*,
org.example.cyberwatch.shared.model.enums.Department,
org.example.cyberwatch.shared.model.enums.Role), then run your IDE's
"optimize/organize imports" or rebuild to ensure no unused imports remain and
the file compiles.
In `@src/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.java`:
- Around line 12-55: The class StaffDTO currently declares its private fields
(id, socialSecurityNumber, firstName, lastName, email, phoneNumber, role,
department) after its constructors and accessor methods; move those field
declarations to the top of the class, directly after the class declaration and
before the no-arg and parameterized constructors, so the order becomes: fields →
constructors → getters/setters (affecting StaffDTO).
- Around line 3-6: The StaffDTO class no longer uses Lombok annotations, so
remove the unused imports to clean up the file: delete the import lines for
lombok.AllArgsConstructor, lombok.Getter, lombok.NoArgsConstructor, and
lombok.Setter from the top of StaffDTO.java so only required imports remain and
no unused Lombok imports are present.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Line 3: Remove the unused Lombok import by deleting the import statement for
RequiredArgsConstructor since StaffService declares an explicit constructor;
locate the import "import lombok.RequiredArgsConstructor;" at the top of the
file and remove it so the class no longer has an unused import while keeping the
existing explicit StaffService(...) constructor and other members unchanged.
In
`@src/main/java/org/example/cyberwatch/features/ticket/model/AssignTicketDTO.java`:
- Around line 6-11: AssignTicketDTO currently defers validation of the staffIds
list to service logic; annotate the staffIds field in AssignTicketDTO with
appropriate bean validation annotations (e.g., `@NotNull` and `@Size`(min=1) or
`@NotEmpty`) so requests with missing/empty staffIds produce a 400; ensure imports
for javax.validation.constraints are added and that callers (controller methods
that accept AssignTicketDTO) use `@Valid` on the request body so the validation is
triggered; keep existing getStaffIds and setStaffIds unchanged.
In `@src/main/java/org/example/cyberwatch/features/ticket/model/Ticket.java`:
- Around line 4-5: The Ticket class still imports lombok.Getter and
lombok.Setter even though explicit getX/setX methods exist; remove the unused
imports (the two import lines referencing lombok.Getter and lombok.Setter) from
the top of the Ticket class to eliminate dead imports and any unused-warning
complaints, leaving the class-level accessors as-is.
In `@src/main/java/org/example/cyberwatch/features/ticket/model/TicketDTO.java`:
- Line 5: The import jakarta.validation.constraints.Positive is unused in the
TicketDTO class; remove that import statement from TicketDTO.java to clean up
unused imports and avoid IDE/compiler warnings (no other code changes required
in the TicketDTO class).
- Around line 37-40: Add bean validation to the TicketDTO.assignedStaffIds field
by annotating the field (the private List<Long> assignedStaffIds) with `@NotEmpty`
(and optionally `@NotNull` if you want to forbid null vs empty) so the
controller-level validation fails fast with a clear message; import the correct
annotation (javax.validation.constraints.NotEmpty) and ensure the DTO is
validated in controller endpoints (e.g., `@Valid` on the request body) so
TicketService.createTicket's non-empty requirement is enforced earlier.
In
`@src/main/java/org/example/cyberwatch/features/ticket/model/TicketResponseDTO.java`:
- Around line 3-4: Remove the now-unused Lombok imports for `@Getter` and `@Setter`
from the TicketResponseDTO class: delete the import lines "import
lombok.Getter;" and "import lombok.Setter;" since explicit accessor methods
exist and the annotations are not used; ensure no other Lombok symbols are
referenced in TicketResponseDTO after removal.
In `@src/main/resources/static/index.html`:
- Around line 1-11: Add an accessible fallback link in index.html so users with
meta-refresh disabled can still navigate: update the body (near the existing
<p>Skickar vidare till login...</p>) to include a visible anchor pointing to
/pages/login.html (e.g., "Click here to continue to login") and ensure the link
text is in Swedish to match lang="sv"; keep the existing meta refresh and
message.
In `@src/main/resources/static/pages/create-ticket.html`:
- Line 6: The page title string in the <title> element is English ("Create
Ticket") while the page content is Swedish; update the <title> element to match
the page language (e.g., change the <title> content to "Skapa ny ticket" or the
preferred Swedish phrasing) so the page header and body are consistent.
In `@src/main/resources/static/pages/dashboard.html`:
- Around line 62-65: The loop is attaching a 'change' listener to 'filterBtn'
which never fires for buttons; remove 'filterBtn' from the array so only
inputs/selects get change events (keep 'statusFilter', 'priorityFilter',
'staffFilter') and keep the separate
document.getElementById('filterBtn').onclick = loadDashboardTickets; to wire the
button click; update the array where it's defined and ensure
loadDashboardTickets remains the handler referenced.
- Line 71: The oninput handler on the element with id 'searchInput' calls
loadDashboardTickets on every keystroke; replace that direct binding with a
debounced wrapper to avoid firing an API call for each character. Implement a
simple debounce helper (or reuse an existing one) and bind
document.getElementById('searchInput').oninput to the debounced version of
loadDashboardTickets (e.g., debounce(loadDashboardTickets, 300)), ensuring the
debounce helper uses clearTimeout/setTimeout so repeated keystrokes reset the
timer and only calls loadDashboardTickets after the quiet period.
In `@src/main/resources/static/pages/ticket-detail.html`:
- Line 6: The page title in ticket-detail.html is in English while the page body
is Swedish; update the <title> element in ticket-detail.html to match the site's
Swedish language (as done in create-ticket.html) by replacing "Ticket Detail"
with the appropriate Swedish text (e.g., "Biljettdetaljer" or the exact phrase
used in create-ticket.html) so the title language is consistent with the page
content and other pages.
🪄 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: e3e12213-07c3-4872-9198-498b3a4f7116
📒 Files selected for processing (49)
mvnwsrc/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/config/security/JwtService.javasrc/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.javasrc/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/main/java/org/example/cyberwatch/features/activitylog/model/ActivityLog.javasrc/main/java/org/example/cyberwatch/features/activitylog/model/ActivityType.javasrc/main/java/org/example/cyberwatch/features/activitylog/service/ActivityLogService.javasrc/main/java/org/example/cyberwatch/features/comment/model/Comment.javasrc/main/java/org/example/cyberwatch/features/comment/model/CommentDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentFormDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/UpdateEmploymentDTO.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/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffResponseDTO.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.javasrc/main/java/org/example/cyberwatch/features/ticket/model/AssignTicketDTO.javasrc/main/java/org/example/cyberwatch/features/ticket/model/Ticket.javasrc/main/java/org/example/cyberwatch/features/ticket/model/TicketAttachment.javasrc/main/java/org/example/cyberwatch/features/ticket/model/TicketDTO.javasrc/main/java/org/example/cyberwatch/features/ticket/model/TicketFilterParams.javasrc/main/java/org/example/cyberwatch/features/ticket/model/TicketResponseDTO.javasrc/main/java/org/example/cyberwatch/features/ticket/repository/TicketAttachmentRepository.javasrc/main/java/org/example/cyberwatch/features/ticket/repository/TicketRepository.javasrc/main/java/org/example/cyberwatch/features/ticket/service/TicketService.javasrc/main/resources/db/migration/V3__add_ticket_code_to_tickets.sqlsrc/main/resources/db/migration/V4__update_employment_form_for_s3.sqlsrc/main/resources/db/migration/V5__update_staff_with_s3_and_password.sqlsrc/main/resources/db/migration/V6__add_test_user.sqlsrc/main/resources/db/migration/V7__add_ticket_assignments_many_to_many.sqlsrc/main/resources/static/auth/callback.htmlsrc/main/resources/static/css/style.csssrc/main/resources/static/css/styles.csssrc/main/resources/static/index.htmlsrc/main/resources/static/js/app.jssrc/main/resources/static/pages/404.htmlsrc/main/resources/static/pages/500.htmlsrc/main/resources/static/pages/auth-callback.htmlsrc/main/resources/static/pages/create-ticket.htmlsrc/main/resources/static/pages/dashboard.htmlsrc/main/resources/static/pages/edit-ticket.htmlsrc/main/resources/static/pages/login.htmlsrc/main/resources/static/pages/ticket-detail.htmlsrc/main/resources/templates/html/index.html
💤 Files with no reviewable changes (1)
- src/main/resources/templates/html/index.html
Comment on lines
+34
to
+43
| // Vi använder den råa hemligheten direkt som bytes. | ||
| // För HS256 bör hemligheten vara minst 32 bytes (256 bitar). | ||
| byte[] keyBytes = secret.getBytes(); | ||
|
|
||
| // Om den är för kort, fyller vi ut den (eller så kan man kasta ett tydligare fel) | ||
| // Men för att appen ska starta och fungera för användaren justerar vi den till 32 bytes | ||
| byte[] paddedKey = new byte[32]; | ||
| System.arraycopy(keyBytes, 0, paddedKey, 0, Math.min(keyBytes.length, 32)); | ||
|
|
||
| return new SecretKeySpec(paddedKey, "HmacSHA256"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'getKey\(|paddedKey|System\.arraycopy|getBytes\(|MACSigner|MACVerifier' src/main/java/org/example/cyberwatch/config/security/JwtService.java
rg -n -C3 'isTokenValid\(' src/main/java/org/example/cyberwatch/config/security/JwtAuthFilter.java
rg -n -C3 'generateToken\(' src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 2321
Key derivation change breaks existing tokens without migration path.
Lines 34-43 silently normalize key material by truncating/padding to 32 bytes. Both generateToken() (line 60) and isTokenValid() (line 86) use the same getKey() method, so they are consistent—but any tokens signed before this change will fail validation. Additionally, weak/short secrets are silently accepted instead of rejected.
🔧 Safer direction
+import java.nio.charset.StandardCharsets;
...
private SecretKey getKey() {
- byte[] keyBytes = secret.getBytes();
- byte[] paddedKey = new byte[32];
- System.arraycopy(keyBytes, 0, paddedKey, 0, Math.min(keyBytes.length, 32));
- return new SecretKeySpec(paddedKey, "HmacSHA256");
+ byte[] keyBytes = secret.getBytes(StandardCharsets.UTF_8);
+ if (keyBytes.length < 32) {
+ throw new IllegalStateException("app.jwt.secret must be at least 32 bytes for HS256");
+ }
+ return new SecretKeySpec(keyBytes, "HmacSHA256");
}🤖 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/JwtService.java` around
lines 34 - 43, The current getKey() silently pads/truncates the secret and
breaks existing tokens; change getKey() to either (A) enforce a minimum raw
secret length by throwing an IllegalArgumentException when
secret.getBytes().length < 32 so short secrets are rejected, or (B) perform a
deterministic, explicit key derivation (e.g., PBKDF2WithHmacSHA256/HKDF) only
when a new configurable flag (e.g., enableKeyDerivation) is true so behavior is
opt-in and existing tokens remain valid; ensure generateToken() and
isTokenValid() continue to call getKey() so they stay consistent, and
document/configure the migration flag so operators can opt into derivation
without silent token breakage.
Comment on lines
+1
to
+18
| <!DOCTYPE html> | ||
| <html lang="sv"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>404 - Sidan hittades inte</title> | ||
| <link rel="stylesheet" href="styles.css" /> | ||
| </head> | ||
| <body> | ||
| <main class="error-page"> | ||
| <div class="error-card"> | ||
| <h1>404</h1> | ||
| <p>Sidan du försöker nå finns inte.</p> | ||
| <a href="dashboard.html" class="btn btn-primary">Till dashboard</a> | ||
| </div> | ||
| </main> | ||
| </body> | ||
| </html> No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the app has custom error-page wiring or only plain static pages.
fd -a '404\.html$' src/main/resources
rg -n 'ErrorController|ErrorPageRegistrar|server\.error|/error' src/main/java src/main/resources || trueRepository: ithsjava25/project-backend-super-cool-team
Length of output: 145
Move this file to the /error/ directory for Spring Boot to serve it as a 404 page.
This file at src/main/resources/static/pages/404.html will not be automatically served by Spring Boot for 404 errors. Static error pages must be placed under src/main/resources/static/error/404.html to work as intended. Alternatively, implement a custom ErrorController to handle 404 responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/pages/404.html` around lines 1 - 18, The 404 HTML
currently under pages won't be picked up by Spring Boot's error handling; move
the 404.html file into the application's static error directory (the static
"error" folder used by Spring Boot) so it becomes the default 404 error page,
and update the internal link if needed (the anchor to dashboard.html);
alternatively implement a custom ErrorController (a class implementing Spring's
ErrorController and mapping 404 responses to this 404.html) if you prefer
programmatic handling.
Comment on lines
+1
to
+18
| <!DOCTYPE html> | ||
| <html lang="sv"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>500 - Serverfel</title> | ||
| <link rel="stylesheet" href="styles.css" /> | ||
| </head> | ||
| <body> | ||
| <main class="error-page"> | ||
| <div class="error-card"> | ||
| <h1>500</h1> | ||
| <p>Något gick fel på servern. Försök igen senare.</p> | ||
| <a href="dashboard.html" class="btn btn-primary">Till dashboard</a> | ||
| </div> | ||
| </main> | ||
| </body> | ||
| </html> No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the app has custom error-page wiring or only plain static pages.
fd -a '500\.html$' src/main/resources
rg -n 'ErrorController|ErrorPageRegistrar|server\.error|/error' src/main/java src/main/resources || trueRepository: ithsjava25/project-backend-super-cool-team
Length of output: 145
Move 500.html to the correct location for Spring Boot error-page discovery.
src/main/resources/static/pages/500.html won't be served automatically for HTTP 500 responses. Spring Boot expects error pages at src/main/resources/static/error/500.html (or src/main/resources/templates/error/500.html for Thymeleaf). No custom error handling is present in the codebase to work around this placement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/pages/500.html` around lines 1 - 18, The custom
500.html is placed under the app's static/pages location so Spring Boot won't
auto-discover it for HTTP 500 responses; move the 500.html file into the
framework's error discovery directory (static/error) or into templates/error if
using Thymeleaf, keep the existing HTML and UTF-8 meta tag, remove the old pages
copy, and verify a 500 response now returns the moved 500.html via the default
Spring Boot error handling.
Comment on lines
+21
to
+59
| <form id="editTicketForm"> | ||
| <div class="form-group"> | ||
| <label for="editTitle">Titel</label> | ||
| <input type="text" id="editTitle" required /> | ||
| </div> | ||
|
|
||
| <div class="form-group"> | ||
| <label for="editDescription">Fullständig beskrivning</label> | ||
| <textarea id="editDescription" rows="8" required></textarea> | ||
| </div> | ||
|
|
||
| <div style="display: grid; grid-template-columns: 1fr 1fr; gap: 1.5rem;"> | ||
| <div class="form-group"> | ||
| <label for="editPriority">Prioritet</label> | ||
| <select id="editPriority" required> | ||
| <option value="LOW">Low</option> | ||
| <option value="MEDIUM">Medium</option> | ||
| <option value="HIGH">High</option> | ||
| <option value="CRITICAL">Critical</option> | ||
| </select> | ||
| </div> | ||
|
|
||
| <div class="form-group"> | ||
| <label for="editStatus">Status</label> | ||
| <select id="editStatus" required> | ||
| <option value="SUBMITTED">Submitted</option> | ||
| <option value="IN_PROGRESS">In Progress</option> | ||
| <option value="RESOLVED">Resolved</option> | ||
| <option value="CLOSED">Closed</option> | ||
| </select> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div style="margin-top: 1.5rem; display: flex; gap: 1rem;"> | ||
| <button type="submit" class="btn btn-primary">Spara ändringar</button> | ||
| <a href="/pages/dashboard.html" class="btn btn-secondary">Avbryt</a> | ||
| </div> | ||
| </form> | ||
| <p id="editMessage" class="message"></p> |
There was a problem hiding this comment.
This “edit ticket” page currently saves status only.
setupEditTicketForm() in src/main/resources/static/js/app.js:348-370 calls /tickets/{id}/status and ignores the edited title, description, and priority. The UI says “Spara ändringar”, but those fields are silently discarded.
Either wire this form to a real full-ticket update endpoint, or reduce the page to a status-only editor so the UI matches the behavior.
Also applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/pages/edit-ticket.html` around lines 21 - 59, The
form currently only persists status — update the behavior in setupEditTicketForm
(JS) so the submit handler reads editTitle, editDescription, editPriority and
editStatus and sends a full ticket update (e.g., PUT /tickets/{id} with title,
description, priority, status) instead of calling /tickets/{id}/status; ensure
the payload keys match the server DTO and handle response/error to update
editMessage. Alternatively, if the API doesn’t support full updates, reduce the
form to a status-only editor by removing title/description/priority inputs and
changing the button text and labels to reflect “Update status only” so the UI
matches actual behavior.
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/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 35-38: Replace the generic RuntimeException throws in StaffService
with the domain-specific StaffNotFoundException: in getStaffById (and the other
methods currently using .orElseThrow(() -> new RuntimeException(...))) change
the supplier to throw new StaffNotFoundException with the missing staff id (or
an appropriate message/identifier) so not-found cases map to the proper 404
semantics; update import/usages for StaffNotFoundException and ensure the
constructors/messages align with how other parts of the app expect the
exception.
- Around line 58-68: getStaffByRoleOrDepartment currently returns full StaffDTO
(which contains PII like socialSecurityNumber and phoneNumber) causing
/api/staff/filter to leak sensitive data; change the method to return a PII-safe
representation by using a new mapper method or DTO: add a StaffPublicDTO (or a
mapper method such as staffMapper.toPublicDTOList) that omits or redacts
socialSecurityNumber and phoneNumber, update getStaffByRoleOrDepartment to call
staffMapper.toPublicDTOList(staffList) (or map to StaffPublicDTO) and update
callers (e.g., StaffRestController) to expect the public DTO so PII is no longer
exposed.
- Around line 51-55: The deleteStaff method currently uses
staffRepository.existsById(id) followed by staffRepository.deleteById(id), which
can create a race; instead load the entity once and delete it: in
StaffService.deleteStaff use staffRepository.findById(id) (or
findById(...).orElseThrow(...)) to fetch the Staff entity, throw the same
RuntimeException if absent, then call staffRepository.delete(foundStaff) to
remove it so only a single read occurs and the check-delete race is avoided.
🪄 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: 8855b3b9-c1ac-44f9-85c4-474253c83536
📒 Files selected for processing (6)
src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/db/migration/V6__add_test_user.sqlsrc/main/resources/static/pages/create-ticket.htmlsrc/main/resources/static/pages/edit-ticket.html
✅ Files skipped from review due to trivial changes (3)
- src/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.java
- src/main/resources/db/migration/V6__add_test_user.sql
- src/main/resources/static/pages/edit-ticket.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/static/pages/create-ticket.html
Comment on lines
58
to
68
| public List<StaffDTO> getStaffByRoleOrDepartment(Role role, Department department) { | ||
| List<Staff> staffList; | ||
| if (role != null) { | ||
| return staffRepository.findByRole(role) | ||
| .stream().map(staffMapper::toDto).toList(); | ||
| staffList = staffRepository.findByRole(role); | ||
| } else if (department != null) { | ||
| return staffRepository.findByDepartment(department) | ||
| .stream().map(staffMapper::toDto).toList(); | ||
| staffList = staffRepository.findByDepartment(department); | ||
| } else { | ||
| staffList = staffRepository.findAll(); | ||
| } | ||
| return getAllStaff(); | ||
| return staffMapper.toDTOList(staffList); | ||
| } |
There was a problem hiding this comment.
Filtered staff listing still returns PII-heavy DTOs.
This method returns StaffDTO, which includes sensitive fields (socialSecurityNumber, phoneNumber). Because StaffRestController Line 47-50 directly exposes this output, /api/staff/filter can leak unnecessary PII in bulk responses.
Proposed fix
- public List<StaffDTO> getStaffByRoleOrDepartment(Role role, Department department) {
+ public List<StaffResponseDTO> getStaffByRoleOrDepartment(Role role, Department department) {
List<Staff> staffList;
if (role != null) {
staffList = staffRepository.findByRole(role);
} else if (department != null) {
staffList = staffRepository.findByDepartment(department);
} else {
staffList = staffRepository.findAll();
}
- return staffMapper.toDTOList(staffList);
+ return staffList.stream()
+ .map(StaffResponseDTO::from)
+ .toList();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public List<StaffDTO> getStaffByRoleOrDepartment(Role role, Department department) { | |
| List<Staff> staffList; | |
| if (role != null) { | |
| return staffRepository.findByRole(role) | |
| .stream().map(staffMapper::toDto).toList(); | |
| staffList = staffRepository.findByRole(role); | |
| } else if (department != null) { | |
| return staffRepository.findByDepartment(department) | |
| .stream().map(staffMapper::toDto).toList(); | |
| staffList = staffRepository.findByDepartment(department); | |
| } else { | |
| staffList = staffRepository.findAll(); | |
| } | |
| return getAllStaff(); | |
| return staffMapper.toDTOList(staffList); | |
| } | |
| public List<StaffResponseDTO> getStaffByRoleOrDepartment(Role role, Department department) { | |
| List<Staff> staffList; | |
| if (role != null) { | |
| staffList = staffRepository.findByRole(role); | |
| } else if (department != null) { | |
| staffList = staffRepository.findByDepartment(department); | |
| } else { | |
| staffList = staffRepository.findAll(); | |
| } | |
| return staffList.stream() | |
| .map(StaffResponseDTO::from) | |
| .toList(); | |
| } |
🤖 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/service/StaffService.java`
around lines 58 - 68, getStaffByRoleOrDepartment currently returns full StaffDTO
(which contains PII like socialSecurityNumber and phoneNumber) causing
/api/staff/filter to leak sensitive data; change the method to return a PII-safe
representation by using a new mapper method or DTO: add a StaffPublicDTO (or a
mapper method such as staffMapper.toPublicDTOList) that omits or redacts
socialSecurityNumber and phoneNumber, update getStaffByRoleOrDepartment to call
staffMapper.toPublicDTOList(staffList) (or map to StaffPublicDTO) and update
callers (e.g., StaffRestController) to expect the public DTO so PII is no longer
exposed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java (1)
183-186:⚠️ Potential issue | 🟠 MajorReject partial assignee lookups here too.
findAllById(staffIds)can return only the rows that exist, and this branch still accepts that as long as the list is non-empty. A request with one bad ID will therefore overwrite the ticket with a truncated assignee list. Please validate that every requested ID resolved before saving.🤖 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/ticket/service/TicketService.java` around lines 183 - 186, In TicketService, when loading assignees via staffRepository.findAllById(staffIds) you must reject partial resolutions: after obtaining staffList, verify that staffList.size() == staffIds.size() (or equivalently that every id in staffIds is present in staffList) and if not throw a RuntimeException (or a more specific checked/ custom exception) instead of proceeding; this ensures that a request containing any invalid staff ID is rejected rather than saving a truncated assignee list.src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (1)
61-70:⚠️ Potential issue | 🟠 MajorKeep the filtered staff endpoint on the PII-safe DTO.
getAllStaff()now returnsStaffResponseDTO, but the filtered path still maps toStaffDTO. That means/api/staff/filtercan still expose fields the new public listing intentionally stopped returning. Please switch this method to the same response shape asgetAllStaff().🤖 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/service/StaffService.java` around lines 61 - 70, The filtered endpoint getStaffByRoleOrDepartment currently returns internal StaffDTO via staffMapper.toDTOList(staffList) which can leak PII; change it to return the public response shape used by getAllStaff() by mapping to StaffResponseDTO instead (e.g., use the same mapper method that produces StaffResponseDTO list or add one such as staffMapper.toResponseDTOList or staffMapper.toStaffResponseList) and update the method signature to List<StaffResponseDTO> getStaffByRoleOrDepartment(Role role, Department department) so the filter and full listing use the same PII-safe DTO.src/main/resources/static/js/app.js (4)
356-362:⚠️ Potential issue | 🟠 MajorThe edit form still only persists status.
You collect
title,description, andpriorityintobody, but the submit path still calls the status-only endpoint and never sends that payload. Users can edit those fields in the UI and lose every change except status.🤖 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 356 - 362, You build a full update payload (body using editTitle/editDescription/editPriority/editStatus) but call the status-only endpoint; change the apiFetch call to call the ticket update endpoint (e.g., `/tickets/${id}`) and send the payload as JSON in the request options: set method: "PATCH", headers: { "Content-Type": "application/json" }, and body: JSON.stringify(body) instead of using the `/status?status=...` query; update the apiFetch invocation (the line referencing apiFetch(`/tickets/${id}/status?status=${body.status}&performedById=1`, ...)) so it sends the full body.
70-76:⚠️ Potential issue | 🔴 CriticalStop injecting ticket, staff, and comment data via
innerHTML.These blocks interpolate user-controlled fields like names, emails, titles, descriptions, and comment text directly into HTML. That leaves the new pages open to stored XSS. Please build these nodes with DOM APIs and assign user content through
textContent/.valueinstead.Also applies to: 126-147, 205-245
🤖 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 70 - 76, The code is injecting user-controlled data via template strings into innerHTML (see staffOptions and select.innerHTML), which enables stored XSS; replace this by constructing DOM nodes with document.createElement for each <option> (or ticket/comment element), set attributes via el.value/setAttribute and class via el.classList, and assign displayed text via el.textContent (or .value for inputs) before appending to the select (use appendChild or append). Apply the same pattern to the other innerHTML blocks referenced (lines ~126-147 and ~205-245) where tickets, staff, and comments are built: create elements programmatically, set user content through textContent/value, and never interpolate raw user strings into innerHTML.
151-160:⚠️ Potential issue | 🟠 MajorDashboard reassignment still drops the other assignees.
This handler always sends
staffIds: [staffId]. On tickets that already have multiple assignees, changing the select will overwrite the full assignment with just the newly picked person.🤖 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 151 - 160, The change handler for '.dashboard-assignment-select' overwrites all assignees because it always sends body staffIds: [staffId]; fix it by reading the ticket's current assignees, merging the new staffId into that list (deduplicated) and sending the merged array to apiFetch; you can obtain current assignees either from a data attribute on the select (e.g., e.target.dataset.currentAssignees) or by fetching `/tickets/${ticketId}` before the PUT, then build JSON.stringify({ staffIds: mergedStaffIds }) and call apiFetch(`/tickets/${ticketId}/assign?assignedById=1`, { method: "PUT", body: ... }) so other assignees are preserved.
158-160:⚠️ Potential issue | 🟠 MajorDon't keep sending actor IDs from the browser.
These requests still hardcode or read
assignedById,performedById, anduploadedByIdclient-side, so the audit trail remains forgeable and environment-dependent. The frontend should stop sending actor identifiers and let the backend resolve the authenticated user instead.Based on learnings: In
src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java, theperformedByIdanduploadedByIdparameters in endpoints are currently accepted as caller-controlled values, making the audit trail forgeable; the plan is to resolve the actor server-side viaAuthenticationPrincipal StaffUserDetails principaland remove these public parameters.Also applies to: 177-179, 299-303, 331-335, 362-362
🤖 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 158 - 160, The frontend is still sending actor IDs (assignedById / performedById / uploadedById) in API calls which makes the audit trail forgeable; remove all usages in app.js where apiFetch is invoked with query params or body fields for these actor IDs (e.g., the apiFetch call that includes `/tickets/${ticketId}/assign?assignedById=1` and the other calls at the locations you noted), stop adding those fields to JSON bodies, and let the backend (TicketController endpoints that will use AuthenticationPrincipal StaffUserDetails principal) resolve the authenticated user server-side; ensure the client only sends domain data (e.g., staffIds for assignment) and update any UI code that reads/stores actor IDs so it no longer supplies them.
🤖 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/security/OAuth2SuccessHandler.java`:
- Line 63: The redirect currently exposes the JWT via query string in
OAuth2SuccessHandler (see response.sendRedirect(... "?token=" + token));
instead, set the token in a secure cookie and remove it from the URL: create a
cookie (or Spring ResponseCookie) named e.g. "AUTH_TOKEN" with the JWT, set
Secure=true, HttpOnly=true, SameSite=None (or Lax if appropriate for your
frontend), set a short Max-Age, add it to the HttpServletResponse, then call
response.sendRedirect(request.getContextPath() + FRONTEND_PATH) without the
token param; alternatively implement a one-time code flow (generate short-lived
code, store server-side keyed to the JWT, redirect with that code, and have the
frontend exchange it server-side) if cookies are not acceptable.
In `@src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java`:
- Around line 73-79: The generic fallback handler handleOtherErrors in
GlobalExceptionHandler is converting expected EmploymentFormNotFound and
IllegalArgumentException cases into 500s; restore specific exception handlers by
adding dedicated `@ExceptionHandler` methods for EmploymentFormNotFound (returning
404 Not Found, log at info/warn and not expose stack trace) and for
IllegalArgumentException (returning 400 Bad Request, log the input-related
message), and keep handleOtherErrors as the final catch-all that logs the full
stack and returns 500; ensure the new handlers return
ResponseEntity<Map<String,String>> with a clear "message" entry and reference
the exception class names EmploymentFormNotFound and IllegalArgumentException
and the existing handleOtherErrors method when placing them in
GlobalExceptionHandler.
In
`@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java`:
- Around line 99-103: Replace the generic RuntimeException in
TicketService.getTicketByCode with a domain-specific not-found exception (e.g.,
TicketNotFoundException or ResourceNotFoundException) so the
controller/exception handler can return a 404; locate the lookup using
ticketRepository.findByTicketCode(...) and change the orElseThrow(...) to throw
the new exception (including the ticketCode in the message), and leave the
return TicketResponseDTO.from(ticket) as-is.
- Line 267: The early return in TicketService (the line "if (current == next)
return;") allows callers to proceed saving/logging a no-op transition; change
this to explicitly reject no-op transitions by throwing an
IllegalArgumentException (or a domain-specific exception) from setTicketStatus
so callers cannot save/log identical old/new statuses, and update any callers of
setTicketStatus to handle that exception; reference the setTicketStatus method
and the activity logging/save path so you can locate and update the related
save/logging behavior accordingly.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 61-70: The filtered endpoint getStaffByRoleOrDepartment currently
returns internal StaffDTO via staffMapper.toDTOList(staffList) which can leak
PII; change it to return the public response shape used by getAllStaff() by
mapping to StaffResponseDTO instead (e.g., use the same mapper method that
produces StaffResponseDTO list or add one such as staffMapper.toResponseDTOList
or staffMapper.toStaffResponseList) and update the method signature to
List<StaffResponseDTO> getStaffByRoleOrDepartment(Role role, Department
department) so the filter and full listing use the same PII-safe DTO.
In
`@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java`:
- Around line 183-186: In TicketService, when loading assignees via
staffRepository.findAllById(staffIds) you must reject partial resolutions: after
obtaining staffList, verify that staffList.size() == staffIds.size() (or
equivalently that every id in staffIds is present in staffList) and if not throw
a RuntimeException (or a more specific checked/ custom exception) instead of
proceeding; this ensures that a request containing any invalid staff ID is
rejected rather than saving a truncated assignee list.
In `@src/main/resources/static/js/app.js`:
- Around line 356-362: You build a full update payload (body using
editTitle/editDescription/editPriority/editStatus) but call the status-only
endpoint; change the apiFetch call to call the ticket update endpoint (e.g.,
`/tickets/${id}`) and send the payload as JSON in the request options: set
method: "PATCH", headers: { "Content-Type": "application/json" }, and body:
JSON.stringify(body) instead of using the `/status?status=...` query; update the
apiFetch invocation (the line referencing
apiFetch(`/tickets/${id}/status?status=${body.status}&performedById=1`, ...)) so
it sends the full body.
- Around line 70-76: The code is injecting user-controlled data via template
strings into innerHTML (see staffOptions and select.innerHTML), which enables
stored XSS; replace this by constructing DOM nodes with document.createElement
for each <option> (or ticket/comment element), set attributes via
el.value/setAttribute and class via el.classList, and assign displayed text via
el.textContent (or .value for inputs) before appending to the select (use
appendChild or append). Apply the same pattern to the other innerHTML blocks
referenced (lines ~126-147 and ~205-245) where tickets, staff, and comments are
built: create elements programmatically, set user content through
textContent/value, and never interpolate raw user strings into innerHTML.
- Around line 151-160: The change handler for '.dashboard-assignment-select'
overwrites all assignees because it always sends body staffIds: [staffId]; fix
it by reading the ticket's current assignees, merging the new staffId into that
list (deduplicated) and sending the merged array to apiFetch; you can obtain
current assignees either from a data attribute on the select (e.g.,
e.target.dataset.currentAssignees) or by fetching `/tickets/${ticketId}` before
the PUT, then build JSON.stringify({ staffIds: mergedStaffIds }) and call
apiFetch(`/tickets/${ticketId}/assign?assignedById=1`, { method: "PUT", body:
... }) so other assignees are preserved.
- Around line 158-160: The frontend is still sending actor IDs (assignedById /
performedById / uploadedById) in API calls which makes the audit trail
forgeable; remove all usages in app.js where apiFetch is invoked with query
params or body fields for these actor IDs (e.g., the apiFetch call that includes
`/tickets/${ticketId}/assign?assignedById=1` and the other calls at the
locations you noted), stop adding those fields to JSON bodies, and let the
backend (TicketController endpoints that will use AuthenticationPrincipal
StaffUserDetails principal) resolve the authenticated user server-side; ensure
the client only sends domain data (e.g., staffIds for assignment) and update any
UI code that reads/stores actor IDs so it no longer supplies them.
🪄 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: d3c42ba9-ae67-436d-b226-2dc0fc01bb39
📒 Files selected for processing (10)
src/main/java/org/example/cyberwatch/config/security/OAuth2SuccessHandler.javasrc/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/java/org/example/cyberwatch/features/ticket/service/TicketService.javasrc/main/resources/db/migration/v8__Fix_test_usersrc/main/resources/static/js/app.jssrc/main/resources/static/pages/404.htmlsrc/main/resources/static/pages/500.htmlsrc/main/resources/static/pages/auth-callback.htmlsrc/main/resources/static/pages/login.html
✅ Files skipped from review due to trivial changes (5)
- src/main/resources/db/migration/v8__Fix_test_user
- src/main/resources/static/pages/login.html
- src/main/resources/static/pages/500.html
- src/main/resources/static/pages/auth-callback.html
- src/main/resources/static/pages/404.html
| // Skicka token till frontend via query-parameter i redirect-URL:en | ||
| // Frontend sparar token och skickar den som "Authorization: Bearer <token>" på varje request | ||
| response.sendRedirect(FRONTEND_URL + "?token=" + token); | ||
| response.sendRedirect(request.getContextPath() + FRONTEND_PATH + "?token=" + token); |
There was a problem hiding this comment.
Stop putting the JWT in the redirect URL.
?token=... still exposes the auth token to browser history, intermediary logs, and Referer headers. Please switch this handoff to a Secure; HttpOnly; SameSite cookie or a short-lived one-time code that the frontend exchanges server-side.
Based on learnings: Avoid sending JWTs (or other sensitive auth tokens) to the frontend via redirect/query parameters (e.g., ?token=<jwt>). Query params can leak through browser history, proxy/access logs, and Referer headers.
🤖 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`
at line 63, The redirect currently exposes the JWT via query string in
OAuth2SuccessHandler (see response.sendRedirect(... "?token=" + token));
instead, set the token in a secure cookie and remove it from the URL: create a
cookie (or Spring ResponseCookie) named e.g. "AUTH_TOKEN" with the JWT, set
Secure=true, HttpOnly=true, SameSite=None (or Lax if appropriate for your
frontend), set a short Max-Age, add it to the HttpServletResponse, then call
response.sendRedirect(request.getContextPath() + FRONTEND_PATH) without the
token param; alternatively implement a one-time code flow (generate short-lived
code, store server-side keyed to the JWT, redirect with that code, and have the
frontend exchange it server-side) if cookies are not acceptable.
Comment on lines
73
to
79
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<Map<String, String>> handleOtherErrors(Exception e) { | ||
| // Logga internt med full stack trace — exponera INTE detaljer till klienten | ||
| log.error("Unhandled exception", e); | ||
| Map<String, String> error = new HashMap<>(); | ||
| error.put("message", "An unexpected error occurred"); | ||
| return new ResponseEntity<>(error, HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
The generic fallback now turns expected 400/404 cases into 500s.
EmploymentFormService still throws EmploymentFormNotFound and IllegalArgumentException, but those dedicated handlers are gone, so they now fall through here and return a generic 500. Please restore specific mappings for those expected exceptions before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java`
around lines 73 - 79, The generic fallback handler handleOtherErrors in
GlobalExceptionHandler is converting expected EmploymentFormNotFound and
IllegalArgumentException cases into 500s; restore specific exception handlers by
adding dedicated `@ExceptionHandler` methods for EmploymentFormNotFound (returning
404 Not Found, log at info/warn and not expose stack trace) and for
IllegalArgumentException (returning 400 Bad Request, log the input-related
message), and keep handleOtherErrors as the final catch-all that logs the full
stack and returns 500; ensure the new handlers return
ResponseEntity<Map<String,String>> with a clear "message" entry and reference
the exception class names EmploymentFormNotFound and IllegalArgumentException
and the existing handleOtherErrors method when placing them in
GlobalExceptionHandler.
Comment on lines
+99
to
+103
| public TicketResponseDTO getTicketByCode(String ticketCode) { | ||
| Ticket ticket = ticketRepository.findByTicketCode(ticketCode) | ||
| .orElseThrow(() -> new RuntimeException("Ticket not found: " + ticketCode)); | ||
|
|
||
| return TicketResponseDTO.from(ticket); |
There was a problem hiding this comment.
Return a 404 for unknown ticket codes.
This new lookup throws a generic RuntimeException, so /api/tickets/code/{ticketCode} will currently fall into the generic exception handler and come back as a 500. Please throw a domain-specific not-found exception here instead.
🤖 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/ticket/service/TicketService.java`
around lines 99 - 103, Replace the generic RuntimeException in
TicketService.getTicketByCode with a domain-specific not-found exception (e.g.,
TicketNotFoundException or ResourceNotFoundException) so the
controller/exception handler can return a 404; locate the lookup using
ticketRepository.findByTicketCode(...) and change the orElseThrow(...) to throw
the new exception (including the ticketCode in the message), and leave the
return TicketResponseDTO.from(ticket) as-is.
| } | ||
|
|
||
| private void validateStatusTransition(Status current, Status next) { | ||
| if (current == next) return; |
There was a problem hiding this comment.
Avoid treating same-status requests as a real transition.
Returning early here makes setTicketStatus(...) accept a no-op update, but that caller still saves the entity and writes a status-change activity entry with identical old/new values. Either reject no-op transitions or have the caller skip the save/log path when current == next.
🤖 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/ticket/service/TicketService.java`
at line 267, The early return in TicketService (the line "if (current == next)
return;") allows callers to proceed saving/logging a no-op transition; change
this to explicitly reject no-op transitions by throwing an
IllegalArgumentException (or a domain-specific exception) from setTicketStatus
so callers cannot save/log identical old/new statuses, and update any callers of
setTicketStatus to handle that exception; reference the setTicketStatus method
and the activity logging/save path so you can locate and update the related
save/logging behavior accordingly.
…ttachment and TicketFilterParams
…tors in DTOs; remove @requiredargsconstructor in EmploymentFormService and add explicit constructor
…, and dropdown UI
…uctors across DTOs and services; enhance dashboard UI with status updates, ticket stats, and filters; update status transition rules in TicketService
… add error alert on failure
…StaffService for CRUD operations and filtering
…on script" This reverts commit 95cdf2f.
Ericthilen
force-pushed
the
feature/pages
branch
from
April 16, 2026 08:44
e2d14f8 to
1ce00c5
Compare
…eption, and AccessDeniedException; simplify static file permissions in SecurityConfig; remove unused getAllStaff endpoint
…solve any issues with overlap
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.
This PR introduces new frontend pages and extends core ticket functionality.
Frontend
Ticket features
Issues: #45 #44 #43 #42 #41 #40 #39 #38 #37 #36 #22
Summary by CodeRabbit
New Features
Bug Fixes
Chores