Refactor/restructuring(DONT MERGE)#39
Closed
mattknatt wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughMajor modularization refactor reorganizing code into domain-specific modules (audit, cases, identity), renaming entities (EmployeeEntity→AccountEntity, PatientEntity→PatientProfileEntity), introducing profile tables, adding Spring Modulith support, and implementing OAuth2 user provisioning with account auto-creation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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: 11
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/projektarendehantering/identity/infrastructure/SecurityConfig.java (1)
47-55:⚠️ Potential issue | 🟠 MajorRemove hardcoded admin user or externalize to environment-specific configuration before production.
The in-memory
admin/passworduser with{noop}encoding (line 51) is present unconditionally in all environments—no production profile override exists. The comment marking it as "fine for dev" is not enforced programmatically. Since OAuth2 is the primary authentication method, also disableformLogin(line 38) in production or make it conditional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/identity/infrastructure/SecurityConfig.java` around lines 47 - 55, The code unconditionally registers a hardcoded in-memory admin user in userDetailsService (User.builder / InMemoryUserDetailsManager) with "{noop}password" and leaves formLogin enabled; remove or externalize this before production by either deleting the userDetailsService bean and relying on OAuth2, or making it conditional (use `@Profile`("dev") or a conditional bean) and reading username/password from configuration/env vars (avoid {noop} in prod); also wrap the formLogin() call behind the same dev-only condition or disable it for non-dev profiles so form-based auth is not enabled in production.src/main/java/org/example/projektarendehantering/audit/infrastructure/AuditInterceptor.java (1)
73-81:⚠️ Potential issue | 🟠 MajorDon't store non-case IDs in audit
caseIdfield.The fallback from
"caseId"to"id"captures employee IDs (from/api/employees/{id}) and patient IDs (from/api/patients/{id}) ascaseIdin the audit log, corrupting audit semantics and breaking any reporting that expectscaseIdto contain only actual case identifiers. Check the route pattern before applying the fallback—restrict the"id"fallback to case routes only.Proposed fix
private UUID extractCaseId(HttpServletRequest request) { + Object pattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); + String routePattern = pattern != null ? pattern.toString() : ""; + if (!routePattern.startsWith("/api/cases/") && !routePattern.startsWith("/ui/cases/")) { + return null; + } + Object attr = request.getAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE); if (!(attr instanceof Map<?, ?> vars)) return null; Object caseId = vars.get("caseId"); if (caseId == null) { caseId = vars.get("id"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/audit/infrastructure/AuditInterceptor.java` around lines 73 - 81, The current extractCaseId(HttpServletRequest) method falls back from "caseId" to "id" and thus can capture non-case resources; modify extractCaseId to only treat the "id" template variable as a caseId when the matched route/pattern indicates a case endpoint (use HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE or request.getRequestURI() to inspect the route), e.g., check that the pattern contains "case" or matches "/api/cases/{id}" before using vars.get("id"), otherwise return null; keep using HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE to read vars and only apply the "id" fallback when the route check passes.
🧹 Nitpick comments (8)
pom.xml (1)
55-59: Preferspring-modulith-starter-testover directspring-modulith-test.The Spring Modulith documentation recommends using
spring-modulith-starter-testfor testing setup. This starter bundlesspring-modulith-test,spring-modulith-docs,spring-boot-starter-test, andawaitility—the complete test toolkit—keeping your configuration aligned with Spring's idiomatic approach.♻️ Suggested dependency adjustment
<dependency> <groupId>org.springframework.modulith</groupId> - <artifactId>spring-modulith-test</artifactId> + <artifactId>spring-modulith-starter-test</artifactId> <scope>test</scope> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 55 - 59, Replace the direct modulith test dependency with the recommended starter: remove the dependency whose artifactId is "spring-modulith-test" (groupId "org.springframework.modulith") and add "spring-modulith-starter-test" with scope "test" so the project uses the bundled test toolkit (which includes spring-modulith-test, spring-modulith-docs, spring-boot-starter-test, and awaitility); ensure no conflicting duplicate test dependencies remain and keep the JVM test scope as "test".src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java (1)
34-35: OAuth attribute names are GitHub-specific.The attributes
"login"and"name"are GitHub-specific. If this application ever supports other OAuth providers (Google, Azure AD, etc.), this will need adaptation. Consider documenting this coupling or extracting provider-specific attribute mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java` around lines 34 - 35, The code in CustomOAuth2UserService uses GitHub-specific attribute names ("login" and "name") via oAuth2User.getAttribute, which will break for other OAuth providers; update the service to branch on the provider (use OAuth2UserRequest.getClientRegistration().getRegistrationId() inside loadUser) and map provider-specific attributes through a small mapper method or class (e.g., mapAttributesForProvider or ProviderAttributeMapper) that returns the canonical fields (login, fullName, email), or fall back to standard attribute names when available; alternatively add a clear comment documenting the GitHub coupling if you intentionally want to keep it GitHub-only.src/main/java/org/example/projektarendehantering/identity/domain/AccountEntity.java (1)
12-23: Consider adding column constraints for data integrity.The entity lacks explicit column definitions that could improve schema clarity and data integrity:
createdAtshould likely be non-updatable- Fields like
displayNameandgithubUsernamemay neednullable = falseconstraints♻️ Suggested column annotations
`@Id` private UUID id; - `@Column`(unique = true) + `@Column`(unique = true, nullable = false) private String githubUsername; - private String displayName; - + `@Column`(nullable = false) + private String displayName; + `@Enumerated`(EnumType.STRING) + `@Column`(nullable = false) private Role role; - private Instant createdAt; + `@Column`(nullable = false, updatable = false) + private Instant createdAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/identity/domain/AccountEntity.java` around lines 12 - 23, AccountEntity is missing explicit column constraints: mark githubUsername and displayName as non-nullable by adding `@Column`(nullable = false) to both (githubUsername can keep its unique = true), and mark createdAt as non-updatable by adding `@Column`(updatable = false) (also consider nullable = false for createdAt). Update the AccountEntity class annotations and, if createdAt must be set automatically, add a `@PrePersist` method to initialize createdAt; ensure imports for javax.persistence.Column and javax.persistence.PrePersist are present.src/main/java/org/example/projektarendehantering/identity/domain/PatientProfileEntity.java (1)
13-16: Remove publicsetId()and derive identifier solely fromaccountrelationship.Line 35 exposes a public
setId(UUID id)setter, while line 38'ssetAccount(AccountEntity account)does not synchronize the identifier. With@MapsId, theidis a derived identifier that should be read-only and sourced only from the associatedaccount. This design prevents in-memory inconsistency whereidandaccount.idcould diverge. Drop the publicidsetter and move identifier synchronization intosetAccount(). Also update the constructor at line 26 to usesetAccount(...)instead of direct field assignment.♻️ Suggested refactor
public PatientProfileEntity(AccountEntity account, String firstName, String lastName, String personalIdentityNumber) { - this.account = account; - // Do NOT manually set this.id when using `@MapsId` + setAccount(account); this.firstName = firstName; this.lastName = lastName; this.personalIdentityNumber = personalIdentityNumber; } public UUID getId() { return id; } - public void setId(UUID id) { this.id = id; } public AccountEntity getAccount() { return account; } - public void setAccount(AccountEntity account) { this.account = account; } + public void setAccount(AccountEntity account) { + this.account = account; + this.id = account != null ? account.getId() : null; + }Also applies to: 26-28, 34-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/identity/domain/PatientProfileEntity.java` around lines 13 - 16, Remove the public setId(UUID) on PatientProfileEntity so the derived identifier is read-only; update setAccount(AccountEntity account) to synchronize the id by setting this.id = account.getId() (or null if account is null) and use setAccount(...) inside the constructor instead of assigning the account field directly; ensure no other code relies on the public setId (make it private or delete it) so the `@MapsId` relationship remains the single source of truth for the identifier.src/main/java/org/example/projektarendehantering/audit/application/AuditService.java (1)
13-13: Prefer an exported read port over another module's repository.
audit.applicationnow depends oncases.domain.CaseRepositoryfor authorization checks. That couples audit to the cases module's persistence API and makes the new module boundary harder to enforce. An exported query interface from the cases module would keep this dependency directional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/audit/application/AuditService.java` at line 13, AuditService currently depends directly on cases.domain.CaseRepository which couples the audit module to the cases persistence API; replace that concrete dependency with an exported read-only port interface from the cases module (e.g., CasesReadPort or CaseQueryService) and use that interface in AuditService (constructor injection of the new port) for authorization checks; update any usages inside AuditService (methods referencing CaseRepository) to call the port's read/query methods and remove the direct import of CaseRepository so the audit module only depends on the cases module's exported read port.src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java (1)
21-23: This assertion is effectively tautological.
count()cannot be negative, so this only distinguishes “threw” vs “didn't throw” and will still pass against an empty or mis-seeded database. Either make this an explicit no-throw smoke test or assert a known condition that this refactor should preserve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java` around lines 21 - 23, The test repositoryIsAccessible() currently asserts that auditEventRepository.count() is >= 0 which is tautological; change it to either an explicit no-throw smoke test (e.g. use JUnit's assertDoesNotThrow or AssertJ's assertThatCode(...).doesNotThrowAnyException() around auditEventRepository.count()) or assert a concrete expected value that the refactor should preserve (compare auditEventRepository.count() to a known expectedCount). Update the assertion accordingly and keep the test name repositoryIsAccessible() consistent.src/main/java/org/example/projektarendehantering/audit/presentation/AuditController.java (1)
5-5: Avoid importing another module's infrastructure adapter here.Depending on
identity.infrastructure.SecurityActorAdaptermakes the audit module reach into identity's implementation layer directly, which undercuts the modularization this PR is aiming for. Prefer a small application/shared interface for current-actor lookup and keep the adapter internal to the identity module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/audit/presentation/AuditController.java` at line 5, AuditController currently imports the identity module's SecurityActorAdapter which creates an unwanted implementation dependency; instead define a small shared interface (e.g., CurrentActorProvider with a method like getCurrentActor() or getCurrentActorId()) in the application/shared API, change AuditController to depend on that interface (constructor/DI injection) and remove the SecurityActorAdapter import, and keep SecurityActorAdapter implementing that new interface inside the identity module and bound in your DI configuration so the adapter remains internal to identity while AuditController uses only the shared interface.src/main/java/org/example/projektarendehantering/identity/domain/EmployeeProfileEntity.java (1)
27-31: Remove the publicsetIdon this@MapsIdentity.Here
idis derived fromaccount. Keeping a public setter allows callers to put the entity into an impossible state where the primary key and the associatedAccountEntitydisagree. While no direct calls tosetId()on profile entities were found, the constructor comment already warns against manual ID assignment, making the public setter contradictory and a source of confusion. The same issue exists inPatientProfileEntity—both should remove this method to enforce the invariant.♻️ Proposed fix
public UUID getId() { return id; } - public void setId(UUID id) { this.id = id; } public AccountEntity getAccount() { return account; } public void setAccount(AccountEntity account) { this.account = account; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/identity/domain/EmployeeProfileEntity.java` around lines 27 - 31, Remove the public setId(UUID) method from EmployeeProfileEntity (the `@MapsId-mapped` id derived from AccountEntity) so callers cannot violate the primary-key/account invariant; delete the setId method (or make it private if used internally) and ensure no external code relies on EmployeeProfileEntity.setId, and apply the same change to PatientProfileEntity to prevent manual ID assignment that would desynchronize the mapped AccountEntity and the entity primary key.
🤖 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/cases/application/CaseService.java`:
- Around line 134-138: The role-check helpers (canCreate, isManager, isDoctor,
isNurse, isPatient) currently dereference actor and cause NPEs when called from
createCase, getCase, getAllCases, and getCasesForPatient for anonymous users;
update each helper to first guard against null (e.g., if actor == null) and
return false so anonymous access yields the existing NotAuthorizedException path
rather than a 500, keeping the rest of each predicate logic unchanged.
- Around line 44-51: The addNote method currently only null-checks the actor and
then saves a note, allowing any authenticated actor to append notes; update
addNote to enforce authorization by invoking requireCanRead(actor, caseEntity)
after loading caseEntity (or a stricter write-specific check method if you want
to distinguish read vs write) before creating/saving the CaseNoteEntity so only
authorized actors can add notes; preserve existing logic using
caseRepository.findById(...), then call requireCanRead(actor, caseEntity) (or
requireCanWrite(actor, caseEntity)) before mapping with caseNoteMapper and
saving with caseNoteRepository.
- Around line 59-66: CaseMapper.toEntity currently copies client-provided
workflow fields (status, createdAt) allowing tampering; modify create flow to
ensure server-controlled values by either introducing a dedicated creation
mapper (e.g., CaseMapper.toEntityForCreate/CaseCreateDTO that omits status and
createdAt) or, more simply, always overwrite those fields after mapping in
CaseService (replace the null-checks with unconditional sets): set
entity.setStatus("OPEN") and entity.setCreatedAt(Instant.now()) regardless of
incoming values, and continue to set owner via the existing
isDoctor/isManager(actor) check; reference CaseMapper.toEntity, CaseService
(create case block), entity.setStatus and entity.setCreatedAt to locate where to
apply the change.
In
`@src/main/java/org/example/projektarendehantering/cases/domain/CaseEntity.java`:
- Around line 25-27: The association change on CaseEntity.subject (the
patient_id join column) now points to PatientProfileEntity.id (which is a shared
account/profile UUID via `@MapsId`) and will break historical links; add a
Flyway/Liquibase migration that: 1) creates PatientProfileEntity rows for each
legacy patient/account missing a profile (populate required fields and link to
the existing account UUID), and 2) updates every cases.patient_id to the newly
created PatientProfileEntity.id for matching legacy patients before deploying
the entity change so existing CaseEntity.subject links remain intact. Ensure the
migration references the patient_id column and the PatientProfileEntity id
mapping (`@MapsId`) and runs prior to enabling the new association.
In
`@src/main/java/org/example/projektarendehantering/identity/application/EmployeeService.java`:
- Around line 54-65: Update EmployeeService to remove contradictory profile rows
when role changes: when dto.getRole() == Role.PATIENT, ensure you create a
PatientProfileEntity if missing and also delete any existing employee profile
for userId via employeeProfileRepository.deleteById(userId) (or
find-and-delete); when dto.getRole() != Role.PENDING (staff), create
EmployeeProfileEntity if missing and delete any existing patient profile via
patientProfileRepository.deleteById(userId); additionally, when role becomes
Role.PENDING remove both patient and employee profiles to keep state consistent;
use the existing patientProfileRepository, employeeProfileRepository,
savedAccount and userId identifiers to locate and update profiles.
- Around line 37-45: The code currently derives userId with
UUID.nameUUIDFromBytes(dto.getGithubUsername().getBytes(...)) which ties
identity to a mutable login; instead, use a stable provider subject/ID from the
DTO or OAuth response (e.g., dto.getGithubId() or dto.getOauthSubject()) as the
account key when creating/finding AccountEntity via accountRepository.findById,
and continue to store dto.getGithubUsername() only as mutable profile data on
AccountEntity; if the DTO/API does not provide a stable subject, obtain it from
the OAuth token exchange before creating the AccountEntity so renames/case
changes don’t create new accounts.
In
`@src/main/java/org/example/projektarendehantering/identity/application/PatientService.java`:
- Around line 32-47: The current read-before-write check in PatientService using
dto.getPersonalIdentityNumber() is TOCTOU and you also persist the raw value;
trim/normalize the personal identity number immediately (e.g.,
canonicalPersonalId = dto.getPersonalIdentityNumber() != null ?
dto.getPersonalIdentityNumber().trim() : null), use that canonical value for the
pre-check
(patientProfileRepository.findByPersonalIdentityNumber(canonicalPersonalId)) and
for creating PatientProfileEntity, and add a DB unique constraint/index on the
personalIdentityNumber column so the database enforces uniqueness; finally, wrap
the account/profile save block to catch the integrity exception (e.g.,
DataIntegrityViolationException or PersistenceException thrown by
accountRepository.save/profile save) and translate it to a
ResponseStatusException with HttpStatus.CONFLICT to return 409 when the DB
constraint is violated.
In
`@src/main/java/org/example/projektarendehantering/identity/domain/PatientProfileRepository.java`:
- Around line 7-8: The repository currently exposes raw PII via
PatientProfileRepository.findByPersonalIdentityNumber and the
PatientProfileEntity.personalIdentityNumber field; change the model to store
only an encryptedPersonalIdentity (ciphertext) plus a deterministic
blind-index/hmac column (e.g., personalIdentityHmac) and update the repository
to provide a lookup method like findByPersonalIdentityHmac(String
personalIdentityHmac) instead of findByPersonalIdentityNumber. Ensure callers
compute the HMAC/blind-index using a server-side secret before querying (use a
secure HMAC algorithm and key management) and persist the HMAC when
creating/updating PatientProfileEntity; remove or stop using plain
personalIdentityNumber in queries and storage.
In
`@src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java`:
- Around line 39-68: The code silently proceeds when login is null; update
CustomOAuth2UserService to handle this explicitly by checking if (login == null)
at the start of the method and then either (a) log a warning via the class
logger (e.g., logger.warn("OAuth2 login is null for provider ...")) and throw a
runtime/authentication exception (e.g., OAuth2AuthenticationException or
IllegalStateException) to abort authentication, or (b) if you prefer rejecting
gracefully, log the warning and add a safe fallback authority like new
SimpleGrantedAuthority("ROLE_ANONYMOUS") before returning; ensure this check
occurs before UUID.nameUUIDFromBytes(login.getBytes(...)) to avoid NPE and
mention accountRepository, AccountEntity and authorities when updating behavior.
- Around line 30-69: The loadUser method in CustomOAuth2UserService performs
multiple DB operations (find, update, create) without a transaction; annotate
the method (or the CustomOAuth2UserService class) with `@Transactional` so the
account lookup/update/create in loadUser is executed atomically; ensure the
Spring transaction annotation is imported
(org.springframework.transaction.annotation.Transactional) and apply it to the
loadUser(OAuth2UserRequest) method (or the class) so that
accountRepository.findById(...), accountRepository.save(...) calls are wrapped
in a transaction.
In `@src/main/resources/data.sql`:
- Around line 2-10: The seed in data.sql inserts privileged OAuth-linked
accounts into the accounts table using real github_username values
('admin','alice','bob') which will be reapplied on every startup due to
spring.sql.init.mode=always; move the privileged rows (those with role
'MANAGER','DOCTOR','NURSE' and the github_username values 'admin','alice','bob')
out of src/main/resources/data.sql into a test-only seed file (e.g.,
src/main/resources/data-test.sql) that is only loaded by test profiles, or
replace their github_username values with non-matching placeholders so they
cannot collide with real GitHub identities; ensure the ON CONFLICT behavior
remains and that only non-privileged or placeholder rows remain in data.sql.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/audit/infrastructure/AuditInterceptor.java`:
- Around line 73-81: The current extractCaseId(HttpServletRequest) method falls
back from "caseId" to "id" and thus can capture non-case resources; modify
extractCaseId to only treat the "id" template variable as a caseId when the
matched route/pattern indicates a case endpoint (use
HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE or request.getRequestURI() to
inspect the route), e.g., check that the pattern contains "case" or matches
"/api/cases/{id}" before using vars.get("id"), otherwise return null; keep using
HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE to read vars and only apply the
"id" fallback when the route check passes.
In
`@src/main/java/org/example/projektarendehantering/identity/infrastructure/SecurityConfig.java`:
- Around line 47-55: The code unconditionally registers a hardcoded in-memory
admin user in userDetailsService (User.builder / InMemoryUserDetailsManager)
with "{noop}password" and leaves formLogin enabled; remove or externalize this
before production by either deleting the userDetailsService bean and relying on
OAuth2, or making it conditional (use `@Profile`("dev") or a conditional bean) and
reading username/password from configuration/env vars (avoid {noop} in prod);
also wrap the formLogin() call behind the same dev-only condition or disable it
for non-dev profiles so form-based auth is not enabled in production.
---
Nitpick comments:
In `@pom.xml`:
- Around line 55-59: Replace the direct modulith test dependency with the
recommended starter: remove the dependency whose artifactId is
"spring-modulith-test" (groupId "org.springframework.modulith") and add
"spring-modulith-starter-test" with scope "test" so the project uses the bundled
test toolkit (which includes spring-modulith-test, spring-modulith-docs,
spring-boot-starter-test, and awaitility); ensure no conflicting duplicate test
dependencies remain and keep the JVM test scope as "test".
In
`@src/main/java/org/example/projektarendehantering/audit/application/AuditService.java`:
- Line 13: AuditService currently depends directly on
cases.domain.CaseRepository which couples the audit module to the cases
persistence API; replace that concrete dependency with an exported read-only
port interface from the cases module (e.g., CasesReadPort or CaseQueryService)
and use that interface in AuditService (constructor injection of the new port)
for authorization checks; update any usages inside AuditService (methods
referencing CaseRepository) to call the port's read/query methods and remove the
direct import of CaseRepository so the audit module only depends on the cases
module's exported read port.
In
`@src/main/java/org/example/projektarendehantering/audit/presentation/AuditController.java`:
- Line 5: AuditController currently imports the identity module's
SecurityActorAdapter which creates an unwanted implementation dependency;
instead define a small shared interface (e.g., CurrentActorProvider with a
method like getCurrentActor() or getCurrentActorId()) in the application/shared
API, change AuditController to depend on that interface (constructor/DI
injection) and remove the SecurityActorAdapter import, and keep
SecurityActorAdapter implementing that new interface inside the identity module
and bound in your DI configuration so the adapter remains internal to identity
while AuditController uses only the shared interface.
In
`@src/main/java/org/example/projektarendehantering/identity/domain/AccountEntity.java`:
- Around line 12-23: AccountEntity is missing explicit column constraints: mark
githubUsername and displayName as non-nullable by adding `@Column`(nullable =
false) to both (githubUsername can keep its unique = true), and mark createdAt
as non-updatable by adding `@Column`(updatable = false) (also consider nullable =
false for createdAt). Update the AccountEntity class annotations and, if
createdAt must be set automatically, add a `@PrePersist` method to initialize
createdAt; ensure imports for javax.persistence.Column and
javax.persistence.PrePersist are present.
In
`@src/main/java/org/example/projektarendehantering/identity/domain/EmployeeProfileEntity.java`:
- Around line 27-31: Remove the public setId(UUID) method from
EmployeeProfileEntity (the `@MapsId-mapped` id derived from AccountEntity) so
callers cannot violate the primary-key/account invariant; delete the setId
method (or make it private if used internally) and ensure no external code
relies on EmployeeProfileEntity.setId, and apply the same change to
PatientProfileEntity to prevent manual ID assignment that would desynchronize
the mapped AccountEntity and the entity primary key.
In
`@src/main/java/org/example/projektarendehantering/identity/domain/PatientProfileEntity.java`:
- Around line 13-16: Remove the public setId(UUID) on PatientProfileEntity so
the derived identifier is read-only; update setAccount(AccountEntity account) to
synchronize the id by setting this.id = account.getId() (or null if account is
null) and use setAccount(...) inside the constructor instead of assigning the
account field directly; ensure no other code relies on the public setId (make it
private or delete it) so the `@MapsId` relationship remains the single source of
truth for the identifier.
In
`@src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java`:
- Around line 34-35: The code in CustomOAuth2UserService uses GitHub-specific
attribute names ("login" and "name") via oAuth2User.getAttribute, which will
break for other OAuth providers; update the service to branch on the provider
(use OAuth2UserRequest.getClientRegistration().getRegistrationId() inside
loadUser) and map provider-specific attributes through a small mapper method or
class (e.g., mapAttributesForProvider or ProviderAttributeMapper) that returns
the canonical fields (login, fullName, email), or fall back to standard
attribute names when available; alternatively add a clear comment documenting
the GitHub coupling if you intentionally want to keep it GitHub-only.
In
`@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java`:
- Around line 21-23: The test repositoryIsAccessible() currently asserts that
auditEventRepository.count() is >= 0 which is tautological; change it to either
an explicit no-throw smoke test (e.g. use JUnit's assertDoesNotThrow or
AssertJ's assertThatCode(...).doesNotThrowAnyException() around
auditEventRepository.count()) or assert a concrete expected value that the
refactor should preserve (compare auditEventRepository.count() to a known
expectedCount). Update the assertion accordingly and keep the test name
repositoryIsAccessible() consistent.
🪄 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: dc292a5d-d6f3-4dad-b9bc-54bfb84bae6f
📒 Files selected for processing (59)
pom.xmlsrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/application/service/PatientService.javasrc/main/java/org/example/projektarendehantering/audit/application/AuditEventDTO.javasrc/main/java/org/example/projektarendehantering/audit/application/AuditEventMapper.javasrc/main/java/org/example/projektarendehantering/audit/application/AuditService.javasrc/main/java/org/example/projektarendehantering/audit/domain/AuditEventEntity.javasrc/main/java/org/example/projektarendehantering/audit/domain/AuditEventRepository.javasrc/main/java/org/example/projektarendehantering/audit/infrastructure/AuditInterceptor.javasrc/main/java/org/example/projektarendehantering/audit/infrastructure/AuditWebMvcConfig.javasrc/main/java/org/example/projektarendehantering/audit/presentation/AuditController.javasrc/main/java/org/example/projektarendehantering/audit/presentation/AuditUiController.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseAssignmentDTO.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseDTO.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseMapper.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseNoteDTO.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseNoteMapper.javasrc/main/java/org/example/projektarendehantering/cases/application/CaseService.javasrc/main/java/org/example/projektarendehantering/cases/application/CreateCaseForm.javasrc/main/java/org/example/projektarendehantering/cases/domain/CaseEntity.javasrc/main/java/org/example/projektarendehantering/cases/domain/CaseNoteEntity.javasrc/main/java/org/example/projektarendehantering/cases/domain/CaseNoteRepository.javasrc/main/java/org/example/projektarendehantering/cases/domain/CaseRepository.javasrc/main/java/org/example/projektarendehantering/cases/presentation/CaseController.javasrc/main/java/org/example/projektarendehantering/cases/presentation/GlobalControllerAdvice.javasrc/main/java/org/example/projektarendehantering/cases/presentation/UiController.javasrc/main/java/org/example/projektarendehantering/common/Role.javasrc/main/java/org/example/projektarendehantering/identity/application/EmployeeCreateDTO.javasrc/main/java/org/example/projektarendehantering/identity/application/EmployeeDTO.javasrc/main/java/org/example/projektarendehantering/identity/application/EmployeeMapper.javasrc/main/java/org/example/projektarendehantering/identity/application/EmployeeService.javasrc/main/java/org/example/projektarendehantering/identity/application/PatientCreateDTO.javasrc/main/java/org/example/projektarendehantering/identity/application/PatientDTO.javasrc/main/java/org/example/projektarendehantering/identity/application/PatientMapper.javasrc/main/java/org/example/projektarendehantering/identity/application/PatientService.javasrc/main/java/org/example/projektarendehantering/identity/domain/AccountEntity.javasrc/main/java/org/example/projektarendehantering/identity/domain/AccountRepository.javasrc/main/java/org/example/projektarendehantering/identity/domain/EmployeeProfileEntity.javasrc/main/java/org/example/projektarendehantering/identity/domain/EmployeeProfileRepository.javasrc/main/java/org/example/projektarendehantering/identity/domain/PatientProfileEntity.javasrc/main/java/org/example/projektarendehantering/identity/domain/PatientProfileRepository.javasrc/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.javasrc/main/java/org/example/projektarendehantering/identity/infrastructure/SecurityActorAdapter.javasrc/main/java/org/example/projektarendehantering/identity/infrastructure/SecurityConfig.javasrc/main/java/org/example/projektarendehantering/identity/presentation/EmployeeController.javasrc/main/java/org/example/projektarendehantering/identity/presentation/EmployeeUiController.javasrc/main/java/org/example/projektarendehantering/identity/presentation/PatientController.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/PatientRepository.javasrc/main/java/org/example/projektarendehantering/shared/Actor.javasrc/main/java/org/example/projektarendehantering/shared/AppException.javasrc/main/java/org/example/projektarendehantering/shared/BadRequestException.javasrc/main/java/org/example/projektarendehantering/shared/NotAuthorizedException.javasrc/main/java/org/example/projektarendehantering/shared/Role.javasrc/main/resources/data.sqlsrc/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.javasrc/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.javasrc/test/resources/data-test.sql
💤 Files with no reviewable changes (6)
- src/main/java/org/example/projektarendehantering/common/Role.java
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.java
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/PatientRepository.java
- src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java
- src/main/java/org/example/projektarendehantering/application/service/PatientService.java
- src/main/java/org/example/projektarendehantering/application/service/CaseService.java
Comment on lines
+44
to
+51
| public void addNote(UUID caseId, String content, Actor actor) { | ||
| if (actor == null) throw new NotAuthorizedException("Not allowed to add notes"); | ||
| CaseEntity caseEntity = caseRepository.findById(caseId) | ||
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found")); | ||
|
|
||
| CaseNoteEntity note = caseNoteMapper.toEntity(actor, content); | ||
| note.setCaseEntity(caseEntity); | ||
| caseNoteRepository.save(note); |
There was a problem hiding this comment.
Adding notes currently bypasses case-level authorization.
After the null check on Line 45, any authenticated actor who knows a case id can append notes. Reuse requireCanRead(actor, caseEntity) here at minimum, or add a stricter write-specific check if patients should be read-only.
🔒 Suggested fix
public void addNote(UUID caseId, String content, Actor actor) {
if (actor == null) throw new NotAuthorizedException("Not allowed to add notes");
CaseEntity caseEntity = caseRepository.findById(caseId)
.orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found"));
+ requireCanRead(actor, caseEntity);
CaseNoteEntity note = caseNoteMapper.toEntity(actor, content);
note.setCaseEntity(caseEntity);
caseNoteRepository.save(note);
}📝 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 void addNote(UUID caseId, String content, Actor actor) { | |
| if (actor == null) throw new NotAuthorizedException("Not allowed to add notes"); | |
| CaseEntity caseEntity = caseRepository.findById(caseId) | |
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found")); | |
| CaseNoteEntity note = caseNoteMapper.toEntity(actor, content); | |
| note.setCaseEntity(caseEntity); | |
| caseNoteRepository.save(note); | |
| public void addNote(UUID caseId, String content, Actor actor) { | |
| if (actor == null) throw new NotAuthorizedException("Not allowed to add notes"); | |
| CaseEntity caseEntity = caseRepository.findById(caseId) | |
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found")); | |
| requireCanRead(actor, caseEntity); | |
| CaseNoteEntity note = caseNoteMapper.toEntity(actor, content); | |
| note.setCaseEntity(caseEntity); | |
| 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/cases/application/CaseService.java`
around lines 44 - 51, The addNote method currently only null-checks the actor
and then saves a note, allowing any authenticated actor to append notes; update
addNote to enforce authorization by invoking requireCanRead(actor, caseEntity)
after loading caseEntity (or a stricter write-specific check method if you want
to distinguish read vs write) before creating/saving the CaseNoteEntity so only
authorized actors can add notes; preserve existing logic using
caseRepository.findById(...), then call requireCanRead(actor, caseEntity) (or
requireCanWrite(actor, caseEntity)) before mapping with caseNoteMapper and
saving with caseNoteRepository.
Comment on lines
+59
to
+66
| CaseEntity entity = caseMapper.toEntity(caseDTO); | ||
| PatientProfileEntity subject = patientProfileRepository.findById(caseDTO.getPatientId()) | ||
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Patient Profile not found")); | ||
| entity.setSubject(subject); | ||
|
|
||
| if (isDoctor(actor) || isManager(actor)) entity.setOwnerId(actor.userId()); | ||
| if (entity.getStatus() == null) entity.setStatus("OPEN"); | ||
| if (entity.getCreatedAt() == null) entity.setCreatedAt(Instant.now()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected result: either CaseDTO does not expose workflow fields,
# or CaseMapper.toEntity(...) does not copy them from the request.
find src/main/java \( -name 'CaseDTO.java' -o -name 'CaseMapper.java' \) -print
rg -n -C3 --iglob 'CaseDTO.java' '\b(status|createdAt|ownerId|handlerId|otherId|patientId)\b' src/main/java
rg -n -C8 --iglob 'CaseMapper.java' '\btoEntity\s*\(' src/main/java
rg -n -C2 --iglob 'CaseMapper.java' '\b(setStatus|setCreatedAt|setOwnerId|setHandlerId|setOtherId)\b' src/main/javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 8068
CaseMapper.toEntity() copies client-provided workflow fields without restriction.
The issue is confirmed. CaseDTO exposes status and createdAt fields, and CaseMapper.toEntity() (line 40 and 43) copies them directly to the entity. The null-checks in CaseService.createCase() (lines 65–66) only provide defaults if these fields are absent—clients sending status="CLOSED" or backdated timestamps will have those values persisted. Use a dedicated create DTO that excludes these fields, or explicitly overwrite them regardless of null state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/cases/application/CaseService.java`
around lines 59 - 66, CaseMapper.toEntity currently copies client-provided
workflow fields (status, createdAt) allowing tampering; modify create flow to
ensure server-controlled values by either introducing a dedicated creation
mapper (e.g., CaseMapper.toEntityForCreate/CaseCreateDTO that omits status and
createdAt) or, more simply, always overwrite those fields after mapping in
CaseService (replace the null-checks with unconditional sets): set
entity.setStatus("OPEN") and entity.setCreatedAt(Instant.now()) regardless of
incoming values, and continue to set owner via the existing
isDoctor/isManager(actor) check; reference CaseMapper.toEntity, CaseService
(create case block), entity.setStatus and entity.setCreatedAt to locate where to
apply the change.
Comment on lines
+134
to
+138
| private boolean canCreate(Actor actor) { return isManager(actor) || isDoctor(actor); } | ||
| private boolean isManager(Actor actor) { return actor.role() == Role.MANAGER; } | ||
| private boolean isDoctor(Actor actor) { return actor.role() == Role.DOCTOR; } | ||
| private boolean isNurse(Actor actor) { return actor.role() == Role.NURSE; } | ||
| private boolean isPatient(Actor actor) { return actor.role() == Role.PATIENT; } |
There was a problem hiding this comment.
Null actors still turn into 500s in the role helpers.
Lines 135-138 dereference actor directly. createCase, getCase, getAllCases, and getCasesForPatient all call these predicates before any null guard, so anonymous access becomes an NPE instead of NotAuthorizedException.
🛡 Suggested fix
- private boolean canCreate(Actor actor) { return isManager(actor) || isDoctor(actor); }
- private boolean isManager(Actor actor) { return actor.role() == Role.MANAGER; }
- private boolean isDoctor(Actor actor) { return actor.role() == Role.DOCTOR; }
- private boolean isNurse(Actor actor) { return actor.role() == Role.NURSE; }
- private boolean isPatient(Actor actor) { return actor.role() == Role.PATIENT; }
+ private boolean canCreate(Actor actor) { return isManager(actor) || isDoctor(actor); }
+ private boolean isManager(Actor actor) { return actor != null && actor.role() == Role.MANAGER; }
+ private boolean isDoctor(Actor actor) { return actor != null && actor.role() == Role.DOCTOR; }
+ private boolean isNurse(Actor actor) { return actor != null && actor.role() == Role.NURSE; }
+ private boolean isPatient(Actor actor) { return actor != null && actor.role() == Role.PATIENT; }📝 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
| private boolean canCreate(Actor actor) { return isManager(actor) || isDoctor(actor); } | |
| private boolean isManager(Actor actor) { return actor.role() == Role.MANAGER; } | |
| private boolean isDoctor(Actor actor) { return actor.role() == Role.DOCTOR; } | |
| private boolean isNurse(Actor actor) { return actor.role() == Role.NURSE; } | |
| private boolean isPatient(Actor actor) { return actor.role() == Role.PATIENT; } | |
| private boolean canCreate(Actor actor) { return isManager(actor) || isDoctor(actor); } | |
| private boolean isManager(Actor actor) { return actor != null && actor.role() == Role.MANAGER; } | |
| private boolean isDoctor(Actor actor) { return actor != null && actor.role() == Role.DOCTOR; } | |
| private boolean isNurse(Actor actor) { return actor != null && actor.role() == Role.NURSE; } | |
| private boolean isPatient(Actor actor) { return actor != null && actor.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/cases/application/CaseService.java`
around lines 134 - 138, The role-check helpers (canCreate, isManager, isDoctor,
isNurse, isPatient) currently dereference actor and cause NPEs when called from
createCase, getCase, getAllCases, and getCasesForPatient for anonymous users;
update each helper to first guard against null (e.g., if actor == null) and
return false so anonymous access yields the existing NotAuthorizedException path
rather than a 500, keeping the rest of each predicate logic unchanged.
Comment on lines
25
to
+27
| @ManyToOne(optional = true) | ||
| @JoinColumn(name = "patient_id", nullable = true) // Optional because the patient can be null | ||
| private PatientEntity patient; | ||
| @JoinColumn(name = "patient_id", nullable = true) | ||
| private PatientProfileEntity subject; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI --glob 'V*__*.sql'
fd -HI --glob '*changelog*'
rg -n -C2 --ignore-case \
'patient_profiles|patient_id|alter table .*cases|update .*cases.*patient_id|foreign key.*patient_id|PatientProfileEntity|PatientEntity|@MapsId' \
. || trueRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 14674
Backfill cases.patient_id before switching this association.
patient_id now resolves against PatientProfileEntity.id, which is a shared account/profile UUID via @MapsId. Without a schema/data migration to rewrite existing cases.patient_id values, historical cases will lose their subject link after deploy. A search of the codebase found no migration files (Flyway or Liquibase) that perform this transformation.
Expected result: a schema/data migration that creates patient_profiles for all legacy patients and rewrites cases.patient_id rows to the matching account/profile IDs before this association is activated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/cases/domain/CaseEntity.java`
around lines 25 - 27, The association change on CaseEntity.subject (the
patient_id join column) now points to PatientProfileEntity.id (which is a shared
account/profile UUID via `@MapsId`) and will break historical links; add a
Flyway/Liquibase migration that: 1) creates PatientProfileEntity rows for each
legacy patient/account missing a profile (populate required fields and link to
the existing account UUID), and 2) updates every cases.patient_id to the newly
created PatientProfileEntity.id for matching legacy patients before deploying
the entity change so existing CaseEntity.subject links remain intact. Ensure the
migration references the patient_id column and the PatientProfileEntity id
mapping (`@MapsId`) and runs prior to enabling the new association.
Comment on lines
+37
to
+45
| UUID userId = UUID.nameUUIDFromBytes(dto.getGithubUsername().getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| // 1. Create or Update Account | ||
| AccountEntity account = accountRepository.findById(userId).orElseGet(() -> { | ||
| AccountEntity newAccount = new AccountEntity(); | ||
| newAccount.setId(userId); | ||
| newAccount.setGithubUsername(dto.getGithubUsername()); | ||
| newAccount.setCreatedAt(Instant.now()); | ||
| return newAccount; |
There was a problem hiding this comment.
Don't derive the account id from githubUsername.
Line 37 makes identity depend on a mutable login. Renames, case-only changes, or stray whitespace will generate a different UUID and split the same person into a new account. Key the account off the stable OAuth subject/id and store the GitHub username separately as mutable profile data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/identity/application/EmployeeService.java`
around lines 37 - 45, The code currently derives userId with
UUID.nameUUIDFromBytes(dto.getGithubUsername().getBytes(...)) which ties
identity to a mutable login; instead, use a stable provider subject/ID from the
DTO or OAuth response (e.g., dto.getGithubId() or dto.getOauthSubject()) as the
account key when creating/finding AccountEntity via accountRepository.findById,
and continue to store dto.getGithubUsername() only as mutable profile data on
AccountEntity; if the DTO/API does not provide a stable subject, obtain it from
the OAuth token exchange before creating the AccountEntity so renames/case
changes don’t create new accounts.
Comment on lines
+32
to
+47
| if (dto.getPersonalIdentityNumber() != null && !dto.getPersonalIdentityNumber().isBlank()) { | ||
| patientProfileRepository.findByPersonalIdentityNumber(dto.getPersonalIdentityNumber()) | ||
| .ifPresent(existing -> { | ||
| throw new ResponseStatusException(HttpStatus.CONFLICT, "Patient with personalIdentityNumber already exists"); | ||
| }); | ||
| } | ||
|
|
||
| UUID id = UUID.randomUUID(); | ||
| String displayName = (dto.getFirstName() + " " + dto.getLastName()).trim(); | ||
|
|
||
| // 1. Create central account (Manual Patients don't have GitHub login) | ||
| AccountEntity account = new AccountEntity(id, null, displayName, Role.PATIENT, Instant.now()); | ||
| accountRepository.save(account); | ||
|
|
||
| // 2. Create profile | ||
| PatientProfileEntity profile = new PatientProfileEntity(account, dto.getFirstName(), dto.getLastName(), dto.getPersonalIdentityNumber()); |
There was a problem hiding this comment.
Make personalIdentityNumber uniqueness database-backed and canonicalized.
The read-before-write check on Lines 32-36 is a TOCTOU race. Two concurrent requests can both pass it; without a unique constraint you'll store duplicates, and with one the loser currently bubbles as a 500 instead of the intended 409. Also, Line 47 persists the raw value, so whitespace variants can bypass the pre-check unless you trim/normalize once before lookup and save.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/identity/application/PatientService.java`
around lines 32 - 47, The current read-before-write check in PatientService
using dto.getPersonalIdentityNumber() is TOCTOU and you also persist the raw
value; trim/normalize the personal identity number immediately (e.g.,
canonicalPersonalId = dto.getPersonalIdentityNumber() != null ?
dto.getPersonalIdentityNumber().trim() : null), use that canonical value for the
pre-check
(patientProfileRepository.findByPersonalIdentityNumber(canonicalPersonalId)) and
for creating PatientProfileEntity, and add a DB unique constraint/index on the
personalIdentityNumber column so the database enforces uniqueness; finally, wrap
the account/profile save block to catch the integrity exception (e.g.,
DataIntegrityViolationException or PersistenceException thrown by
accountRepository.save/profile save) and translate it to a
ResponseStatusException with HttpStatus.CONFLICT to return 409 when the DB
constraint is violated.
Comment on lines
+7
to
+8
| public interface PatientProfileRepository extends JpaRepository<PatientProfileEntity, UUID> { | ||
| Optional<PatientProfileEntity> findByPersonalIdentityNumber(String personalIdentityNumber); |
There was a problem hiding this comment.
Don't expose raw personal identity numbers as a first-class lookup key.
Combined with the plain String mapping on PatientProfileEntity.personalIdentityNumber, this method means the full identifier is stored and queried as-is. For national-ID style PII, prefer an encrypted value plus a deterministic blind-index/HMAC column for uniqueness and lookup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/identity/domain/PatientProfileRepository.java`
around lines 7 - 8, The repository currently exposes raw PII via
PatientProfileRepository.findByPersonalIdentityNumber and the
PatientProfileEntity.personalIdentityNumber field; change the model to store
only an encryptedPersonalIdentity (ciphertext) plus a deterministic
blind-index/hmac column (e.g., personalIdentityHmac) and update the repository
to provide a lookup method like findByPersonalIdentityHmac(String
personalIdentityHmac) instead of findByPersonalIdentityNumber. Ensure callers
compute the HMAC/blind-index using a server-side secret before querying (use a
secure HMAC algorithm and key management) and persist the HMAC when
creating/updating PatientProfileEntity; remove or stop using plain
personalIdentityNumber in queries and storage.
Comment on lines
+30
to
+69
| @Override | ||
| public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException { | ||
| OAuth2User oAuth2User = super.loadUser(userRequest); | ||
|
|
||
| String login = oAuth2User.getAttribute("login"); | ||
| String fullName = oAuth2User.getAttribute("name"); | ||
|
|
||
| Set<GrantedAuthority> authorities = new HashSet<>(oAuth2User.getAuthorities()); | ||
|
|
||
| if (login != null) { | ||
| UUID userId = UUID.nameUUIDFromBytes(login.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| // Check if account already exists | ||
| var accountOpt = accountRepository.findById(userId); | ||
|
|
||
| if (accountOpt.isPresent()) { | ||
| AccountEntity account = accountOpt.get(); | ||
| // Update display name if it changed on GitHub | ||
| if (fullName != null && !fullName.equals(account.getDisplayName())) { | ||
| account.setDisplayName(fullName); | ||
| accountRepository.save(account); | ||
| } | ||
|
|
||
| // Add the role from DB to authorities | ||
| if (account.getRole() != null) { | ||
| authorities.add(new SimpleGrantedAuthority("ROLE_" + account.getRole().name())); | ||
| } | ||
| } else { | ||
| // FIRST LOGIN: Create central account with PENDING role | ||
| AccountEntity newAccount = new AccountEntity( | ||
| userId, | ||
| login, | ||
| fullName != null ? fullName : login, | ||
| Role.PENDING, | ||
| Instant.now() | ||
| ); | ||
| accountRepository.save(newAccount); | ||
| authorities.add(new SimpleGrantedAuthority("ROLE_PENDING")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add @Transactional to ensure atomic account creation/update.
The loadUser method performs multiple database operations (lookup, potential update, potential create) without a transaction boundary. If an error occurs mid-flow, this could leave the database in an inconsistent state.
🔒 Proposed fix
+import org.springframework.transaction.annotation.Transactional;
+
`@Service`
public class CustomOAuth2UserService extends DefaultOAuth2UserService {
// ...
`@Override`
+ `@Transactional`
public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java`
around lines 30 - 69, The loadUser method in CustomOAuth2UserService performs
multiple DB operations (find, update, create) without a transaction; annotate
the method (or the CustomOAuth2UserService class) with `@Transactional` so the
account lookup/update/create in loadUser is executed atomically; ensure the
Spring transaction annotation is imported
(org.springframework.transaction.annotation.Transactional) and apply it to the
loadUser(OAuth2UserRequest) method (or the class) so that
accountRepository.findById(...), accountRepository.save(...) calls are wrapped
in a transaction.
Comment on lines
+39
to
+68
| if (login != null) { | ||
| UUID userId = UUID.nameUUIDFromBytes(login.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| // Check if account already exists | ||
| var accountOpt = accountRepository.findById(userId); | ||
|
|
||
| if (accountOpt.isPresent()) { | ||
| AccountEntity account = accountOpt.get(); | ||
| // Update display name if it changed on GitHub | ||
| if (fullName != null && !fullName.equals(account.getDisplayName())) { | ||
| account.setDisplayName(fullName); | ||
| accountRepository.save(account); | ||
| } | ||
|
|
||
| // Add the role from DB to authorities | ||
| if (account.getRole() != null) { | ||
| authorities.add(new SimpleGrantedAuthority("ROLE_" + account.getRole().name())); | ||
| } | ||
| } else { | ||
| // FIRST LOGIN: Create central account with PENDING role | ||
| AccountEntity newAccount = new AccountEntity( | ||
| userId, | ||
| login, | ||
| fullName != null ? fullName : login, | ||
| Role.PENDING, | ||
| Instant.now() | ||
| ); | ||
| accountRepository.save(newAccount); | ||
| authorities.add(new SimpleGrantedAuthority("ROLE_PENDING")); | ||
| } |
There was a problem hiding this comment.
Handle the case where login is null more explicitly.
If login is null, the method silently returns a user without any role-based authorities from the database. Consider logging a warning or throwing an exception for this unexpected scenario.
🛡️ Suggested handling
if (login != null) {
// existing logic...
+ } else {
+ // Log warning - OAuth provider didn't return expected 'login' attribute
+ // This could indicate misconfiguration or unsupported provider
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/identity/infrastructure/CustomOAuth2UserService.java`
around lines 39 - 68, The code silently proceeds when login is null; update
CustomOAuth2UserService to handle this explicitly by checking if (login == null)
at the start of the method and then either (a) log a warning via the class
logger (e.g., logger.warn("OAuth2 login is null for provider ...")) and throw a
runtime/authentication exception (e.g., OAuth2AuthenticationException or
IllegalStateException) to abort authentication, or (b) if you prefer rejecting
gracefully, log the warning and add a safe fallback authority like new
SimpleGrantedAuthority("ROLE_ANONYMOUS") before returning; ensure this check
occurs before UUID.nameUUIDFromBytes(login.getBytes(...)) to avoid NPE and
mention accountRepository, AccountEntity and authorities when updating behavior.
Comment on lines
+2
to
10
| INSERT INTO accounts (id, github_username, display_name, role, created_at) VALUES | ||
| ('550e8400-e29b-41d4-a716-446655440000', NULL, 'John Doe', 'PATIENT', CURRENT_TIMESTAMP), | ||
| ('550e8400-e29b-41d4-a716-446655440001', NULL, 'Jane Smith', 'PATIENT', CURRENT_TIMESTAMP), | ||
| ('550e8400-e29b-41d4-a716-446655440002', NULL, 'Anders Andersson', 'PATIENT', CURRENT_TIMESTAMP), | ||
| ('770e8400-e29b-41d4-a716-446655440000', 'admin', 'Admin User', 'MANAGER', CURRENT_TIMESTAMP), | ||
| ('770e8400-e29b-41d4-a716-446655440001', 'alice', 'Dr. Alice Roberts', 'DOCTOR', CURRENT_TIMESTAMP), | ||
| ('770e8400-e29b-41d4-a716-446655440002', 'bob', 'Nurse Bob Jones', 'NURSE', CURRENT_TIMESTAMP), | ||
| ('880e8400-e29b-41d4-a716-446655440000', 'newuser', 'New GitHub User', 'PENDING', CURRENT_TIMESTAMP) | ||
| ON CONFLICT (id) DO NOTHING; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf "== SQL init configuration ==\n"
fd -0 -a 'application.*' . | xargs -0 -r rg -n -C2 'spring\.sql\.init|spring\.profiles|defer-datasource-initialization'
printf "\n== Seed SQL files ==\n"
fd -a 'data*.sql' .
printf "\n== Privileged seeded GitHub usernames ==\n"
rg -n -C1 "github_username|'MANAGER'|'DOCTOR'|'NURSE'" src/main/resources/data.sqlRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 2186
Move privileged OAuth seed accounts out of main data.sql.
github_username is now an authentication key. In src/main/resources/application.properties, spring.sql.init.mode=always causes data.sql to execute on every startup. This means real GitHub users named alice, bob, or admin will be automatically mapped to these pre-seeded DOCTOR/NURSE/MANAGER accounts in any environment using default configuration.
Keep these rows in a test-only seed file (data-test.sql) referenced only via test profiles, or use non-matching placeholder values that cannot map to external identities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/data.sql` around lines 2 - 10, The seed in data.sql
inserts privileged OAuth-linked accounts into the accounts table using real
github_username values ('admin','alice','bob') which will be reapplied on every
startup due to spring.sql.init.mode=always; move the privileged rows (those with
role 'MANAGER','DOCTOR','NURSE' and the github_username values
'admin','alice','bob') out of src/main/resources/data.sql into a test-only seed
file (e.g., src/main/resources/data-test.sql) that is only loaded by test
profiles, or replace their github_username values with non-matching placeholders
so they cannot collide with real GitHub identities; ensure the ON CONFLICT
behavior remains and that only non-privileged or placeholder rows remain in
data.sql.
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.
Summary by CodeRabbit
Chores
Refactor