Conversation
📝 WalkthroughWalkthroughReplaces single-string note authors with structured author fields (displayName, githubUsername, role); expands Actor to include displayName/githubUsername; security adapter resolves/enriches Actor from authentication and EmployeeRepository; employee/githubUsername added across DTO/entity/mappers/services; UI and templates updated to manage and display mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UiController
participant Sec as SecurityActorAdapter
participant Repo as EmployeeRepository
participant Service as CaseService
participant NoteRepo as CaseNoteRepository/Mapper
User->>UI: POST /cases/{id}/notes (content)
UI->>Sec: currentUser()
Sec->>Repo: findById(deterministicUserIdFromName)
alt employee found
Repo-->>Sec: EmployeeEntity (id, displayName, githubUsername, role)
Sec-->>UI: Actor(userId, role, displayName, githubUsername)
else not found
Sec-->>UI: Actor(userId, mappedRole, null, resolvedName)
end
UI->>Service: addNote(caseId, content, Actor)
Service->>NoteRepo: map and persist note (set authorDisplayName/githubUsername/role)
NoteRepo-->>Service: persisted CaseNote
Service-->>UI: redirect to case detail
UI-->>User: HTTP redirect
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java (1)
30-39:⚠️ Potential issue | 🟠 MajorAddress the upsert behavior caused by deterministic UUID generation.
The mapper generates a deterministic UUID from
githubUsername(viaUUID.nameUUIDFromBytes), which means callingcreateEmployeetwice with the samegithubUsernamewill produce identical entity IDs. The conditional check for null ID won't prevent this, andsave()will update the existing record rather than creating a duplicate.Either document and rename this method to
createOrUpdateEmployeeif the upsert behavior is intentional, or add an existence check usingfindByGithubUsernameon the repository to prevent accidental overwrites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java` around lines 30 - 39, The createEmployee method in EmployeeService currently upserts because the mapper produces a deterministic UUID from githubUsername; to fix, either (A) rename and document the method as createOrUpdateEmployee to reflect intentional upsert behavior and keep current flow, or (B) enforce create-only semantics by checking employeeRepository.findByGithubUsername(dto.getGithubUsername()) before saving and fail/throw an appropriate exception (or return an error) if an existing record is found; update references to the method accordingly and keep employeeMapper.toEntity, employeeRepository.save, and requireCanManageEmployees usages intact.
🧹 Nitpick comments (4)
src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java (1)
53-65: Consider logging when falling back to authority-based role resolution.When no employee mapping exists, the code falls back to Spring Security authorities. This is correct for handling unmapped users, but adding debug/info logging here would help troubleshoot why specific users aren't getting their expected roles from the employee mapping.
♻️ Optional: Add logging for fallback scenario
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; ... + private static final Logger log = LoggerFactory.getLogger(SecurityActorAdapter.class); ... // 2. Fallback to existing logic (checking Spring authorities) + log.debug("No employee mapping found for user '{}', falling back to authority-based role resolution", name); Role role = Role.PATIENT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java` around lines 53 - 65, The fallback branch in SecurityActorAdapter that assigns Role (Role.PATIENT default and checks authentication.getAuthorities()) should emit a debug/info log when used so we can trace why employee mapping was absent; update the method that builds the Actor (the code block using authentication and Role enum) to log a clear message including the userId and the authorities list before returning new Actor(userId, role, null, name), using the class logger (e.g., logger.debug/info) to indicate “falling back to authority-based role resolution” and the resolved role.src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java (1)
22-23: Consider adding a unique constraint ongithubUsername.Since
githubUsernameis used to derive a deterministic UUID for employee identification (viaUUID.nameUUIDFromBytes), it represents a logical unique identifier. Adding a database constraint ensures data integrity and prevents edge cases where duplicate usernames could be inserted through alternative code paths.♻️ Proposed fix
+ `@Column`(unique = true) private String githubUsername;Or at the table level:
-@Table(name = "employees") +@Table(name = "employees", uniqueConstraints = `@UniqueConstraint`(columnNames = "githubUsername"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java` around lines 22 - 23, Add a database-level uniqueness constraint for the githubUsername field in the EmployeeEntity class: update the EmployeeEntity JPA mapping to declare githubUsername as unique (e.g., `@Column`(unique = true) on the githubUsername field or add a `@Table`(uniqueConstraints = `@UniqueConstraint`(columnNames = "githubUsername")) at the class level) and create a corresponding migration to add the unique index/constraint in the database so duplicates cannot be inserted through other code paths.src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java (1)
37-37: Avoid exposing all enum roles to the UI by default.At Line 37,
Role.values()will auto-expose any newly added enum constant. Prefer an explicit allow-list for assignable roles.Suggested fix
+import java.util.List; @@ public class EmployeeUiController { + private static final List<Role> ASSIGNABLE_ROLES = List.of( + Role.ADMIN, + Role.HANDLER, + Role.CASE_OWNER + ); @@ - model.addAttribute("roles", Role.values()); + model.addAttribute("roles", ASSIGNABLE_ROLES); @@ - model.addAttribute("roles", Role.values()); + model.addAttribute("roles", ASSIGNABLE_ROLES);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java` at line 37, The code currently exposes Role.values() in EmployeeUiController (model.addAttribute("roles", Role.values())), which will automatically include any future enum constants; replace this with an explicit allow-list of assignable roles (e.g., define a static collection like ALLOW_ASSIGNABLE_ROLES or a Role.getAssignableRoles() method that returns only the intended enums) and use that collection when calling model.addAttribute("roles", ...); update EmployeeUiController to reference that allow-list instead of Role.values() so new enum constants are not auto-exposed.src/main/resources/templates/employees/new.html (1)
28-29: Make the placeholder option non-selectable after selection.At Line 28, the placeholder is currently a valid option. Mark it
disabled/selectedto reduce accidental invalid submissions.Suggested fix
- <option value="">-- Select Role --</option> + <option value="" disabled selected>-- Select Role --</option>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/employees/new.html` around lines 28 - 29, The placeholder option "-- Select Role --" is currently selectable; update the first <option> (the placeholder) so it cannot be chosen by adding the disabled and selected attributes (and optionally hidden) to that element, leaving the th:each loop that renders roles (th:each="r : ${roles}", th:value="${r}", th:text="${r}") unchanged so valid roles remain selectable; ensure the placeholder keeps value="" so any client-side validation will catch an unselected role.
🤖 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/projektarendehantering/application/service/CaseService.java`:
- Around line 48-59: addNote currently creates and saves a CaseNoteEntity
without verifying the actor's permission; update addNote in CaseService to check
authorization on the loaded CaseEntity before creating/saving the note by
calling your authorization component (e.g., AuthorizationService or
PermissionService) with the actor and CaseEntity, and if the check fails throw a
ResponseStatusException(HttpStatus.FORBIDDEN, "Not authorized to add note");
retain the existing actor null handling but ensure null actors are denied unless
your auth service explicitly permits system/anonymous writes.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Around line 34-39: Add method-level authorization so only managers can open
the new employee form: enable method security by annotating SecurityConfig with
`@EnableMethodSecurity`, then protect the controller method
EmployeeUiController.newEmployee with `@PreAuthorize` (e.g.
`@PreAuthorize`("hasRole('MANAGER')")) so non-managers cannot access the GET
/ui/employees/new page; ensure the newEmployee method imports the PreAuthorize
annotation (or, if you prefer not to enable method security, perform an explicit
role check inside EmployeeUiController.newEmployee using SecurityContextHolder
and redirect/deny if the caller lacks the MANAGER role).
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 17-23: The currentActor method in GlobalControllerAdvice is
catching all Exceptions which masks real errors; change the catch to only handle
authentication-related failures (e.g., catch
org.springframework.security.core.AuthenticationException or your project's
specific SecurityException) around the call to
securityActorAdapter.currentUser(), return null for those specific exceptions,
and let other exceptions propagate (or rethrow/log and rethrow) so real runtime
errors are not silently swallowed; update the catch clause on currentActor() to
reference the specific exception type(s) instead of Exception and adjust
behavior accordingly.
In `@src/main/resources/templates/employees/new.html`:
- Line 11: Replace the hard-coded href on the "Back to list" anchor with
Thymeleaf URL resolution: change the anchor element that currently uses
href="/ui/employees" to use th:href with the Thymeleaf URL expression (e.g.,
th:href="@{/ui/employees}") while keeping the existing class and text; update
the anchor in the employees/new.html template so the link becomes context-path
aware.
In `@src/main/resources/templates/fragments/header.html`:
- Line 14: The nav visibility check calls currentActor.role().name() without
guarding role(), which can NPE when role is null; update the th:if to explicitly
check the role is non-null (e.g., include a condition like currentActor.role()
!= null or currentActor.role != null) before comparing the name so the anchor's
visibility only evaluates .name() when role exists (refer to the existing th:if
expression that uses currentActor and the role()/name() calls to locate the
check).
- Around line 19-20: The auth label assumes currentActor is non-null and can
throw during template evaluation; update the span.auth-label th:text expression
to use Thymeleaf's null-safe navigation for currentActor (e.g.,
currentActor?.displayName/currentActor?.githubUsername/currentActor?.role) and
provide a final literal fallback like "User" so the whole expression never
dereferences a null currentActor; modify the th:text attribute (on the
span.auth-label) to chain null-safe accesses and a final ?: fallback.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 30-39: The createEmployee method in EmployeeService currently
upserts because the mapper produces a deterministic UUID from githubUsername; to
fix, either (A) rename and document the method as createOrUpdateEmployee to
reflect intentional upsert behavior and keep current flow, or (B) enforce
create-only semantics by checking
employeeRepository.findByGithubUsername(dto.getGithubUsername()) before saving
and fail/throw an appropriate exception (or return an error) if an existing
record is found; update references to the method accordingly and keep
employeeMapper.toEntity, employeeRepository.save, and requireCanManageEmployees
usages intact.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java`:
- Around line 22-23: Add a database-level uniqueness constraint for the
githubUsername field in the EmployeeEntity class: update the EmployeeEntity JPA
mapping to declare githubUsername as unique (e.g., `@Column`(unique = true) on the
githubUsername field or add a `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = "githubUsername")) at the class level) and
create a corresponding migration to add the unique index/constraint in the
database so duplicates cannot be inserted through other code paths.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.java`:
- Around line 53-65: The fallback branch in SecurityActorAdapter that assigns
Role (Role.PATIENT default and checks authentication.getAuthorities()) should
emit a debug/info log when used so we can trace why employee mapping was absent;
update the method that builds the Actor (the code block using authentication and
Role enum) to log a clear message including the userId and the authorities list
before returning new Actor(userId, role, null, name), using the class logger
(e.g., logger.debug/info) to indicate “falling back to authority-based role
resolution” and the resolved role.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`:
- Line 37: The code currently exposes Role.values() in EmployeeUiController
(model.addAttribute("roles", Role.values())), which will automatically include
any future enum constants; replace this with an explicit allow-list of
assignable roles (e.g., define a static collection like ALLOW_ASSIGNABLE_ROLES
or a Role.getAssignableRoles() method that returns only the intended enums) and
use that collection when calling model.addAttribute("roles", ...); update
EmployeeUiController to reference that allow-list instead of Role.values() so
new enum constants are not auto-exposed.
In `@src/main/resources/templates/employees/new.html`:
- Around line 28-29: The placeholder option "-- Select Role --" is currently
selectable; update the first <option> (the placeholder) so it cannot be chosen
by adding the disabled and selected attributes (and optionally hidden) to that
element, leaving the th:each loop that renders roles (th:each="r : ${roles}",
th:value="${r}", th:text="${r}") unchanged so valid roles remain selectable;
ensure the placeholder keeps value="" so any client-side validation will catch
an unselected role.
🪄 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: 9435cf40-0b03-41ef-baeb-d6779294dd4c
📒 Files selected for processing (18)
src/main/java/org/example/projektarendehantering/application/service/CaseNoteMapper.javasrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/common/Actor.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseNoteEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseNoteDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/EmployeeCreateDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/EmployeeDTO.javasrc/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/java/org/example/projektarendehantering/presentation/web/UiController.javasrc/main/resources/templates/cases/detail.htmlsrc/main/resources/templates/employees/list.htmlsrc/main/resources/templates/employees/new.htmlsrc/main/resources/templates/fragments/header.html
Comment on lines
+48
to
+59
| public void addNote(UUID caseId, String content, Actor actor) { | ||
| CaseEntity caseEntity = caseRepository.findById(caseId) | ||
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found")); | ||
|
|
||
| CaseNoteEntity note = new CaseNoteEntity(); | ||
| note.setCaseEntity(caseEntity); | ||
| note.setContent(content); | ||
| note.setAuthor(author); | ||
| if (actor != null) { | ||
| note.setAuthorDisplayName(actor.displayName()); | ||
| note.setAuthorGithubUsername(actor.githubUsername()); | ||
| note.setAuthorRole(actor.role() != null ? actor.role().name() : null); | ||
| } |
There was a problem hiding this comment.
Missing authorization check in addNote allows unauthorized note writes.
addNote loads the case and saves a note without validating that the actor may access/modify that case.
Proposed fix
`@Transactional`
public void addNote(UUID caseId, String content, Actor actor) {
+ if (actor == null) {
+ throw new NotAuthorizedException("Not allowed to add note");
+ }
CaseEntity caseEntity = caseRepository.findById(caseId)
.orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found"));
+ requireCanRead(actor, caseEntity);
CaseNoteEntity note = new CaseNoteEntity();
note.setCaseEntity(caseEntity);
note.setContent(content);
- if (actor != null) {
- note.setAuthorDisplayName(actor.displayName());
- note.setAuthorGithubUsername(actor.githubUsername());
- note.setAuthorRole(actor.role() != null ? actor.role().name() : null);
- }
+ note.setAuthorDisplayName(actor.displayName());
+ note.setAuthorGithubUsername(actor.githubUsername());
+ note.setAuthorRole(actor.role() != null ? actor.role().name() : null);
note.setCreatedAt(Instant.now());
caseNoteRepository.save(note);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`
around lines 48 - 59, addNote currently creates and saves a CaseNoteEntity
without verifying the actor's permission; update addNote in CaseService to check
authorization on the loaded CaseEntity before creating/saving the note by
calling your authorization component (e.g., AuthorizationService or
PermissionService) with the actor and CaseEntity, and if the check fails throw a
ResponseStatusException(HttpStatus.FORBIDDEN, "Not authorized to add note");
retain the existing actor null handling but ensure null actors are denied unless
your auth service explicitly permits system/anonymous writes.
Comment on lines
+34
to
+39
| @GetMapping("/ui/employees/new") | ||
| public String newEmployee(Model model) { | ||
| model.addAttribute("employeeCreateDTO", new EmployeeCreateDTO()); | ||
| model.addAttribute("roles", Role.values()); | ||
| return "employees/new"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether endpoint-level restrictions already exist for /ui/employees/new
rg -nP --type=java '(/ui/employees/new|/ui/employees)' -C3
rg -nP --type=java 'requestMatchers\s*\(|antMatchers\s*\(|mvcMatchers\s*\(' -C3
rg -nP --type=java '@EnableMethodSecurity|@EnableGlobalMethodSecurity|@PreAuthorize' -C3Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 4676
🏁 Script executed:
# Check if method-level security is enabled anywhere
rg -nP --type=java '@EnableMethodSecurity|@EnableGlobalMethodSecurity' -A5 -B2
# Get complete SecurityConfig
fd --type=f -name 'SecurityConfig.java' --exec cat -n {}
# Get complete EmployeeUiController to verify all endpoints
fd --type=f -name 'EmployeeUiController.java' --exec cat -n {}
# Check what imports are already in EmployeeUiController
rg -nP --type=java '^import' src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 1305
🏁 Script executed:
# Check if method-level security is enabled
rg '@EnableMethodSecurity|@EnableGlobalMethodSecurity' --type java -A5 -B2
# Get complete SecurityConfig
cat src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java
# Get complete EmployeeUiController
cat src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 3980
🏁 Script executed:
# Find and read EmployeeService
find . -type f -name 'EmployeeService.java' | head -1 | xargs cat
# Also check for any method-level security annotations in the application main class
find . -type f -name '*Application.java' -o -name '*SpringBootApplication.java' | xargs grep -l '@EnableMethodSecurity\|@EnableGlobalMethodSecurity\|@SpringBootApplication'Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 2603
🏁 Script executed:
# Search for any `@PreAuthorize` usage in the codebase
rg '@PreAuthorize' --type java
# Check the main application class
cat ./src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 457
Add authorization check to GET /ui/employees/new endpoint.
The form page is served to any authenticated user without a role check. While unauthenticated access is prevented by servlet-level security, non-managers can still view the form even though POST operations reject them via employeeService.createEmployee(actor, dto). Add endpoint-level protection to prevent unauthorized users from accessing the form.
To implement the suggested fix with @PreAuthorize, you must first enable method-level security in SecurityConfig:
Required changes
- Add
@EnableMethodSecuritytoSecurityConfig:
`@Configuration`
`@EnableWebSecurity`
+@EnableMethodSecurity
public class SecurityConfig {- Then apply
@PreAuthorizeto the GET endpoint:
+import org.springframework.security.access.prepost.PreAuthorize;
- `@GetMapping`("/ui/employees/new")
+ `@PreAuthorize`("hasRole('MANAGER')")
+ `@GetMapping`("/ui/employees/new")
public String newEmployee(Model model) {
model.addAttribute("employeeCreateDTO", new EmployeeCreateDTO());
model.addAttribute("roles", Role.values());
return "employees/new";
}Alternatively, add an authorization check directly in the controller method (without requiring @EnableMethodSecurity).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java`
around lines 34 - 39, Add method-level authorization so only managers can open
the new employee form: enable method security by annotating SecurityConfig with
`@EnableMethodSecurity`, then protect the controller method
EmployeeUiController.newEmployee with `@PreAuthorize` (e.g.
`@PreAuthorize`("hasRole('MANAGER')")) so non-managers cannot access the GET
/ui/employees/new page; ensure the newEmployee method imports the PreAuthorize
annotation (or, if you prefer not to enable method security, perform an explicit
role check inside EmployeeUiController.newEmployee using SecurityContextHolder
and redirect/deny if the caller lacks the MANAGER role).
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
Show resolved
Hide resolved
Comment on lines
+19
to
+20
| <span class="auth-label" | ||
| th:text="'Logged in as: ' + (${currentActor.displayName} ?: (${currentActor.githubUsername} ?: ${currentActor.role}))">User</span> |
There was a problem hiding this comment.
Add a null-safe fallback for currentActor in auth label rendering.
Line 20 assumes currentActor is always present for authenticated users, but currentActor can be null (advice fallback), which risks template evaluation errors.
Proposed fix
-<span class="auth-label"
- th:text="'Logged in as: ' + (${currentActor.displayName} ?: (${currentActor.githubUsername} ?: ${currentActor.role}))">User</span>
+<span class="auth-label"
+ th:text="${currentActor != null
+ ? 'Logged in as: ' + (currentActor.displayName ?: (currentActor.githubUsername ?: currentActor.role))
+ : 'Logged in as: ' + `#authentication.name`}">User</span>📝 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
| <span class="auth-label" | |
| th:text="'Logged in as: ' + (${currentActor.displayName} ?: (${currentActor.githubUsername} ?: ${currentActor.role}))">User</span> | |
| <span class="auth-label" | |
| th:text="${currentActor != null | |
| ? 'Logged in as: ' + (currentActor.displayName ?: (currentActor.githubUsername ?: currentActor.role)) | |
| : 'Logged in as: ' + `#authentication.name`}">User</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/fragments/header.html` around lines 19 - 20, The
auth label assumes currentActor is non-null and can throw during template
evaluation; update the span.auth-label th:text expression to use Thymeleaf's
null-safe navigation for currentActor (e.g.,
currentActor?.displayName/currentActor?.githubUsername/currentActor?.role) and
provide a final literal fallback like "User" so the whole expression never
dereferences a null currentActor; modify the th:text attribute (on the
span.auth-label) to chain null-safe accesses and a final ?: fallback.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java`:
- Around line 70-87: The tests (e.g.,
currentUser_whenEmployeeFoundInRepository_shouldReturnActorFromEmployee)
generate UUIDs via username.getBytes() which uses the platform default charset;
update UUID creation to use username.getBytes(StandardCharsets.UTF_8) to match
production code and avoid platform-dependent failures, and add the import for
java.nio.charset.StandardCharsets; apply this change for all UUID
initializations in SecurityActorAdapterTest (the occurrences where
UUID.nameUUIDFromBytes(...) is called).
- Around line 30-161: Add a test exercising the OAuth2AuthenticationToken branch
in SecurityActorAdapter.currentUser: mock an OAuth2AuthenticationToken (or mock
Authentication and set instanceof behavior) and an OAuth2User that returns a
"login" attribute (e.g., "oauth-user") for getAttribute("login"), set
SecurityContext.getAuthentication() to that token, stub
employeeRepository.findById(UUID.nameUUIDFromBytes(login.getBytes())) to return
Optional.empty() or an EmployeeEntity as needed, and stub
authentication.getAuthorities() for role fallback checks; then call
securityActorAdapter.currentUser() and assert the Actor's githubUsername, userId
(derived from the login), role, and displayName as in the other tests.
🪄 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: 4876c4e1-e2a6-48ac-a994-5e4c54d9f46d
📒 Files selected for processing (1)
src/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
...ava/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
Show resolved
Hide resolved
...ava/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java (1)
16-28:⚠️ Potential issue | 🟠 MajorOAuth2 role mapping is still missing from the login flow.
The
oauth2Login()block (lines 26–28) does not configure aGrantedAuthoritiesMapperviauserInfoEndpoint(). GitHub OAuth2 users will not be assigned the required ADMIN/CASE_OWNER roles from issue#21. The in-memoryuserDetailsService()only handles form login and cannot apply GitHub role mapping.🔧 Suggested patch
+import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.authority.mapping.GrantedAuthoritiesMapper; +import org.springframework.security.oauth2.client.userinfo.OAuth2UserAuthority; + +import java.util.Arrays; +import java.util.Set; +import java.util.stream.Collectors; @@ - public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + public SecurityFilterChain securityFilterChain(HttpSecurity http, + GrantedAuthoritiesMapper githubAuthoritiesMapper) throws Exception { http .oauth2Login(oauth2 -> oauth2 + .userInfoEndpoint(userInfo -> userInfo.userAuthoritiesMapper(githubAuthoritiesMapper)) .defaultSuccessUrl("/", true) ) @@ } + + `@Bean` + public GrantedAuthoritiesMapper githubAuthoritiesMapper() { + Set<String> admins = Arrays.stream( + System.getenv().getOrDefault("GITHUB_ADMIN_USERNAMES", "").split(",")) + .map(String::trim) + .filter(s -> !s.isBlank()) + .collect(Collectors.toSet()); + + return authorities -> authorities.stream() + .filter(OAuth2UserAuthority.class::isInstance) + .map(OAuth2UserAuthority.class::cast) + .findFirst() + .map(oauthAuthority -> { + String username = String.valueOf(oauthAuthority.getAttributes().get("login")); + if (admins.contains(username)) { + return Set.of(new SimpleGrantedAuthority("ROLE_ADMIN")); + } + return Set.of(new SimpleGrantedAuthority("ROLE_CASE_OWNER")); + }) + .orElse(authorities); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java` around lines 16 - 28, The oauth2Login() block in SecurityConfig.securityFilterChain(HttpSecurity) lacks mapping of OAuth2 user authorities to application roles, so GitHub logins won't receive ADMIN/CASE_OWNER roles; add a GrantedAuthoritiesMapper and wire it into the OAuth2 flow via oauth2Login().userInfoEndpoint().userAuthoritiesMapper(...). Implement a mapper method (e.g., grantedAuthoritiesMapper()) that examines OAuth2UserAuthority or user attributes (org membership, team, or a configured claim) and returns a Set of SimpleGrantedAuthority with "ROLE_ADMIN" / "ROLE_CASE_OWNER" as needed, expose it as a bean or private method, and reference that mapper from securityFilterChain to ensure OAuth2 users get the required roles (note: userDetailsService() only covers form login).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 16-28: The oauth2Login() block in
SecurityConfig.securityFilterChain(HttpSecurity) lacks mapping of OAuth2 user
authorities to application roles, so GitHub logins won't receive
ADMIN/CASE_OWNER roles; add a GrantedAuthoritiesMapper and wire it into the
OAuth2 flow via oauth2Login().userInfoEndpoint().userAuthoritiesMapper(...).
Implement a mapper method (e.g., grantedAuthoritiesMapper()) that examines
OAuth2UserAuthority or user attributes (org membership, team, or a configured
claim) and returns a Set of SimpleGrantedAuthority with "ROLE_ADMIN" /
"ROLE_CASE_OWNER" as needed, expose it as a bean or private method, and
reference that mapper from securityFilterChain to ensure OAuth2 users get the
required roles (note: userDetailsService() only covers form login).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caf7cdf6-637c-4672-af5b-2429d7c6b8c8
📒 Files selected for processing (9)
src/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.javasrc/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/resources/templates/employees/new.htmlsrc/main/resources/templates/fragments/header.htmlsrc/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.java
- src/main/resources/templates/employees/new.html
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.java
- src/main/java/org/example/projektarendehantering/presentation/web/EmployeeUiController.java
- src/main/resources/templates/fragments/header.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #21
Summary by CodeRabbit
New Features
Bug Fixes
Tests