Enhancement/encrypt ssn#80
Merged
Merged
Conversation
…n for secure data handling
…hods leveraging TextEncryptor for secure data processing.
…ce: encrypt SSNs before saving and decrypt/mask SSNs in response data; update controller methods to include requester role in data handling.
…: implement EncryptionService in DataInitializer, modify EmploymentFormService to encrypt SSNs, and adjust EmploymentFormController to use Staff for authentication. Update tests for ssn-encryption. Update DataInitializer.java for ADMIN-startup Remove flyweight-classes containing insert into Staff.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughField-level SSN encryption added (EncryptionConfig/EncryptionService); SSNs are encrypted before persistence and decrypted or masked on retrieval based on requester role. Controllers/services now accept authenticated Staff principals. SSN format validation removed; seeds, migrations, tests, and application properties updated for encryption and env-driven config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Service as Service
participant Encrypt as EncryptionService
participant Repo as Repository/DB
Client->>Controller: POST /forms (SSN plaintext)
Controller->>Service: createForm(dto, requester)
Service->>Encrypt: encrypt(plainSSN)
Encrypt-->>Service: encryptedSSN
Service->>Repo: save(entity with encryptedSSN)
Repo-->>Service: saved entity
Service-->>Controller: created response
Controller-->>Client: 201 Created
Client->>Controller: GET /staff/{id}
Controller->>Service: getStaffById(id, requester)
Service->>Repo: findById(id)
Repo-->>Service: staff entity (encryptedSSN)
alt requester is ADMIN or HR
Service->>Encrypt: decrypt(encryptedSSN)
Encrypt-->>Service: plainSSN
Service-->>Controller: dto with plainSSN
else
Service->>Encrypt: maskLastFour(encryptedSSN)
Encrypt-->>Service: maskedSSN
Service-->>Controller: dto with maskedSSN
end
Controller-->>Client: 200 OK (dto)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/org/example/cyberwatch/config/DataInitializer.java (2)
29-38:⚠️ Potential issue | 🟠 MajorHardcoded ADMIN seed accounts with weak passwords run in every profile.
DataInitializeris a plain@Componentwith no@Profileguard, so on any fresh deployment (including prod) this seeds four ADMIN accounts with predictable passwords liketestPass123and real-looking personal Gmail/Hotmail addresses. A few concerns:
- Security: on an empty prod DB this creates four admin logins with known credentials — anyone with access to this source tree can authenticate as admin.
- Privacy: committing personal-looking names/emails/phone numbers, even if made up, is poor practice and will surface in git history regardless of later edits.
- Role appropriateness: e.g. a staff member in the
HRdepartment is seeded withRole.ADMIN; check if this was intended or ifRole.HRwas meant.Suggest gating on
@Profile({"dev","test"})(or an explicitapp.seed.enabledproperty) and sourcing credentials from env vars rather than literals. At minimum the password should be read from configuration.🔒 Suggested guard
`@Component` +@Profile({"dev", "test"}) public class DataInitializer implements CommandLineRunner {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java` around lines 29 - 38, DataInitializer currently seeds four ADMIN accounts unguarded (class DataInitializer, method that calls staffRepository.saveAll with createStaff(...)), which leaks credentials and personal-looking data; change it to run only in non-prod by adding a guard (e.g., annotate DataInitializer with `@Profile`({"dev","test"}) or check an app.seed.enabled property), move all secret values (passwords) out of literals and load them from configuration/environment properties, replace real-looking names/emails/phones with clearly synthetic placeholders, and verify the intended Role for each createStaff(...) call (e.g., ensure Role.HR vs Role.ADMIN is correct) so seeding is safe and configurable.
41-52:⚠️ Potential issue | 🟡 MinorSSN is encrypted, but consider idempotency on restart.
encryptionService.encrypt(ssn)is called every timerunexecutes and is only guarded bystaffRepository.count() > 0. If you ever seed partial data or lose rows, re-running will encrypt with a new (non‑deterministic) ciphertext, and any lookups viaStaffRepository.existsBySocialSecurityNumber(...)against these SSNs will miss. This is a manifestation of the encryption-determinism issue flagged inEncryptionConfig.java; fixing that resolves this too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java` around lines 41 - 52, The SSN is being encrypted non-deterministically in createStaff via encryptionService.encrypt(ssn), which breaks idempotent seeding and StaffRepository.existsBySocialSecurityNumber(...) lookups; change the implementation to store a deterministic lookup key (either by calling a deterministic encryption method from EncryptionConfig/encryptionService or by computing and storing a deterministic HMAC/fingerprint alongside the non-deterministic ciphertext) and update Staff to hold both fields (e.g., encryptedSsn and ssnFingerprint) so lookups use ssnFingerprint while the ciphertext remains secure; ensure createStaff sets both values and that existsBySocialSecurityNumber queries the deterministic field.src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
117-126:⚠️ Potential issue | 🔴 CriticalCritical: update path breaks SSN encryption and duplicate check.
Two problems in the update flow:
- Line 118 compares
existingForm.getSocialSecurityNumber()(ciphertext on disk) withupdatedForm.getSocialSecurityNumber()(plaintext from the DTO). They will virtually always differ, causingvalidateSsnNotExiststo run on every update — and, as flagged separately, that validator itself is currently broken.- Line 122 calls
employmentMapper.updateEntity(updatedForm, existingForm)which (per the mapper pattern) directly overwritesexistingForm.socialSecurityNumberwith the DTO's plaintext value. The form ends up persisted unencrypted, silently bypassing the encryption guarantee thatcreateFormestablishes.Please decrypt
existingForm's SSN (or compare encrypted values consistently) for the equality check, and re-encrypt the SSN on the entity afterupdateEntity(...):🔒️ Suggested fix
- // Check for duplicate SSN if it's changed - if (!existingForm.getSocialSecurityNumber().equals(updatedForm.getSocialSecurityNumber())) { - validateSsnNotExists(updatedForm.getSocialSecurityNumber()); - } - - employmentMapper.updateEntity(updatedForm, existingForm); + // Check for duplicate SSN if it's changed (compare plaintext to plaintext) + String currentSsnPlain = encryptionService.decrypt(existingForm.getSocialSecurityNumber()); + if (!currentSsnPlain.equals(updatedForm.getSocialSecurityNumber())) { + validateSsnNotExists(updatedForm.getSocialSecurityNumber()); + } + + employmentMapper.updateEntity(updatedForm, existingForm); + existingForm.setSocialSecurityNumber( + encryptionService.encrypt(updatedForm.getSocialSecurityNumber()) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 117 - 126, The update flow currently compares and copies SSNs as plaintext, breaking encryption and causing unnecessary duplicate checks; to fix, when comparing SSNs use a decrypted value from existingForm (decrypt existingForm.getSocialSecurityNumber() via your SSN encryption utility) or compare using a stable encrypted form so validateSsnNotExists(updatedForm.getSocialSecurityNumber()) only runs when the actual plaintext changed, and after calling employmentMapper.updateEntity(updatedForm, existingForm) re-encrypt the SSN on existingForm (replace the plaintext value copied by updateEntity with the encrypted result from your encrypt method) before calling employmentFormRepository.save(existingForm), ensuring validateSsnNotExists is invoked on the correct value and persisted SSNs remain encrypted.
227-236:⚠️ Potential issue | 🔴 CriticalCritical:
validateSsnNotExistschecks plaintext SSN against encrypted records and non-deterministic encryption breaksexistsBySocialSecurityNumberqueries.
createFormpersistssocialSecurityNumberviaencryptionService.encrypt(...)(lines 58-60), storing ciphertext. Line 228 passes rawssntoemploymentFormRepository.existsBySocialSecurityNumber(ssn), comparing plaintext against encrypted data—the check will never match and duplicate forms slip through.Additionally,
EncryptionConfigusesEncryptors.text(encryptionKey, salt)(line 20 of EncryptionConfig.java), which generates a random IV on each call and produces different ciphertext for the same input. This meansexistsBySocialSecurityNumber(encryptedSsn)on line 233 is unreliable even when passed encrypted data: the stored value will not match the newly encrypted value, so the staff table check fails silently.Critical:
updateFormBeforeApprovalsaves plaintext SSN to the database.Line 118 compares
existingForm.getSocialSecurityNumber()(encrypted) withupdatedForm.getSocialSecurityNumber()(plaintext), which always differs. Line 122 callsemploymentMapper.updateEntity(updatedForm, existingForm), which at line 52 ofEmploymentMapper.javasetsentity.setSocialSecurityNumber(dto.getSocialSecurityNumber())directly—writing plaintext SSN to the database, breaking encryption consistency.Fixes required:
- Line 228: Encrypt plaintext SSN before querying the form repository
- Migrate to
Encryptors.queryableText(...)inEncryptionConfigto make encryption deterministic for equality checks, or refactor to an alternative uniqueness validation approach- Line 122: Ensure SSN is encrypted before persisting; modify the update flow to handle encryption explicitly (do not rely on
employmentMapper.updateEntityto preserve encrypted state when SSN changes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 227 - 236, validateSsnNotExists currently compares plaintext SSNs to encrypted DB values and re-encrypts with a non-deterministic cipher, so uniqueness checks always fail; fix by encrypting the input SSN with a deterministic/queryable encryptor before any existsBySocialSecurityNumber call (update validateSsnNotExists to call encryptionService.encryptQueryable(ssn) or equivalent and use that for both employmentFormRepository.existsBySocialSecurityNumber(...) and staffRepository.existsBySocialSecurityNumber(...)), update EncryptionConfig to use Encryptors.queryableText(...) (or provide a deterministic encrypt/decrypt pair) so equality checks are stable, and change updateFormBeforeApproval / employmentMapper.updateEntity flow so the DTO-to-entity path never writes plaintext SSN—ensure updateFormBeforeApproval encrypts the SSN (using the same deterministic method) before calling employmentMapper.updateEntity or adjust EmploymentMapper.setSocialSecurityNumber to accept an already-encrypted value.
🧹 Nitpick comments (5)
src/main/java/org/example/cyberwatch/config/security/EncryptionService.java (1)
13-19: Consider null-safe wrappers.
encrypt(null)/decrypt(null)will NPE inside Spring'sTextEncryptor. If any upstream callers may pass a nullable SSN (e.g. optional fields on DTOs), consider returningnullfast to avoid leaking a confusing Spring exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/security/EncryptionService.java` around lines 13 - 19, The encrypt/decrypt methods in EncryptionService should be null-safe: if callers pass null the current calls to textEncryptor.encrypt/decrypt will NPE. Update the EncryptionService.encrypt(String data) and EncryptionService.decrypt(String data) to return null immediately when data is null (or use Objects.isNull), before calling textEncryptor.encrypt(...) or textEncryptor.decrypt(...), preserving behavior for non-null inputs; reference the existing textEncryptor field in the class when adding the guard.src/test/java/org/example/cyberwatch/SecurityIntegrationTests.java (1)
37-57: LGTM — constructing a realStaffprincipal withUsernamePasswordAuthenticationToken+ROLE_HRmatches the production flow inOAuth2SuccessHandlerand is what@AuthenticationPrincipal Staffinjection requires.@WithMockUserwould no longer work here since it installs aStringprincipal.Minor nit: consider extracting this Staff+auth helper into a small test utility — the same setup will likely be needed in the other tests as they migrate off
@WithMockUser(e.g.hrCannotApproveForm,hrCanCreateForm, which currently still rely on@WithMockUserand will exercise@AuthenticationPrincipal Staffpaths asnull).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/SecurityIntegrationTests.java` around lines 37 - 57, Tests are repeatedly constructing a Staff principal and UsernamePasswordAuthenticationToken with ROLE_HR; extract that logic into a small test utility method/class (e.g., TestAuthUtils.createStaffAuth or TestPrincipalFactory.createHrAuthentication) so other tests (hrCanAccessStaff, hrCannotApproveForm, hrCanCreateForm) can reuse it; the utility should build a Staff object, setFirstName/LastName/Email/Role, create a UsernamePasswordAuthenticationToken with that Staff as principal, null credentials and a List.of(new SimpleGrantedAuthority("ROLE_HR")), and tests should call .with(authentication(TestAuthUtils.createHrAuthentication())) instead of duplicating setup.src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java (1)
53-58: Remove commented-out dead code.The commented block references the old
getStaffByRoleOrDepartment(null, null)signature which no longer compiles. Either delete or, if you want to preserve a convenience endpoint, restore it using the current 3-arg signature.♻️ Suggested cleanup
- /* `@GetMapping` - public ResponseEntity<List<StaffDTO>> getAllStaff() { - return ResponseEntity.ok(staffService.getStaffByRoleOrDepartment(null, null)); - } - - */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java` around lines 53 - 58, Remove the commented-out dead code block containing getAllStaff and the obsolete call to getStaffByRoleOrDepartment(null, null); delete that commented code, or if you want to keep a convenience endpoint, restore a proper getAllStaff method that calls the current three-argument service method (getStaffByRoleOrDepartment) with appropriate default values and returns ResponseEntity<List<StaffDTO>>; ensure the method signature is annotated with `@GetMapping` and uses the correct StaffService method name getStaffByRoleOrDepartment.src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
94-94: Misleading parameter names after theString email → Staffrefactor.
loggedInHrEmail,loggedInManagementEmail, andloggedInEmailare now fullStaffobjects, not email strings. The stale names confuse readers and future maintainers (the variables are used for role/id comparisons, never email). Suggest renaming tologgedInHr,loggedInManagement, andloggedInStaffrespectively to match the types and the existingcreateForm/approveAndFinalizeEmploymentnaming.Also applies to: 131-131, 160-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` at line 94, Rename misleading parameters and local variables that still end with "Email" to match their Staff type: change loggedInHrEmail → loggedInHr, loggedInManagementEmail → loggedInManagement, and loggedInEmail → loggedInStaff across the methods updateFormBeforeApproval, createForm, and approveAndFinalizeEmployment (and any local usages within those methods) so all comparisons and role/id accesses use the new names; update method signatures, callers, and any Javadoc/comments to reflect the new parameter names to keep names consistent with the Staff type and avoid compilation errors from mismatched identifiers.src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (1)
36-52: Optional: DRY up the admin-vs-non-admin SSN exposure block.The
if (requesterRole == Role.ADMIN) { decrypt } else { maskLastFour }block is duplicated betweengetStaffByIdand the stream mapper ingetStaffByRoleOrDepartment. Extracting a small private helper (e.g.,applySsnVisibility(Staff, StaffDTO, Role)) would keep visibility policy in a single place and make future role changes safer.♻️ Suggested refactor
+ private void applySsnVisibility(Staff source, StaffDTO dto, Role requesterRole) { + String stored = source.getSocialSecurityNumber(); + dto.setSocialSecurityNumber( + requesterRole == Role.ADMIN + ? encryptionService.decrypt(stored) + : encryptionService.maskLastFour(stored) + ); + }Then call
applySsnVisibility(staff, dto, requesterRole);in both methods.Also consider defensively treating
nullrequesterRoleexplicitly rather than relying on theelsefall-through, so intent is obvious.Also applies to: 90-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java` around lines 36 - 52, The SSN exposure logic in getStaffById and the stream mapper in getStaffByRoleOrDepartment is duplicated; extract a private helper method applySsnVisibility(Staff staff, StaffDTO dto, Role requesterRole) that encapsulates the logic currently using encryptionService.decrypt(...) and encryptionService.maskLastFour(...), and call that helper from both getStaffById and getStaffByRoleOrDepartment; ensure the helper treats a null requesterRole explicitly (e.g., treat null as non-admin) and documents the behavior so the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/cyberwatch/config/EncryptionConfig.java`:
- Around line 18-21: The TextEncryptor bean using Encryptors.text(...) is
non-deterministic and breaks existence checks; replace/augment the current
approach by introducing a deterministic HMAC lookup value for SSNs: add a method
(e.g., encryptionService.computeSsnLookup(ssn) or computeHmacSsn) that returns a
keyed HMAC-SHA256 of the plaintext SSN, persist that HMAC into a new
Staff.ssnLookup (or ssn_lookup) column, and change
EmploymentFormService.validateSsnNotExists to query
staffRepository.existsBySsnLookup(encryptionService.computeSsnLookup(ssn))
instead of
staffRepository.existsBySocialSecurityNumber(encryptionService.encrypt(ssn));
likewise update updateFormBeforeApproval to compare existingForm.getSsnLookup()
with encryptionService.computeSsnLookup(updatedForm.getSocialSecurityNumber())
(or normalize to compare plaintext-to-plaintext before encryption), and update
tests to stub/verify computeSsnLookup rather than encrypt. Ensure the HMAC key
is securely configured and rotated strategy is documented.
In `@src/main/java/org/example/cyberwatch/config/security/EncryptionService.java`:
- Around line 21-24: In maskLastFour, defend against null and short values and
handle decrypt failures: if the input encrypted is null return null; wrap the
call to textEncryptor.decrypt(encrypted) in a try/catch (catch
RuntimeException/Exception from decrypt) and on failure return a safe masked
placeholder like "****"; after successful decrypt, if decrypted.length() <= 4
return "****" (avoid substring errors), otherwise return decrypted.substring(0,
decrypted.length() - 4) + "****"; also add a short comment above maskLastFour
explaining that this method intentionally preserves the prefix and masks the
last four characters (or change to mask all-but-last-four if you prefer the more
common convention).
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 167-171: The error message in deleteForm is stale: the guard
checks isAdmin (Role.ADMIN) and isCreator but the thrown message mentions
"management"; update either the logic to include management roles (e.g., check
Role.CEO/Role.CTO or a isManagement predicate) or change the exception text to
accurately state "Only the HR staff who created this form or an admin can delete
it". Locate the check using the isAdmin, isCreator locals and the deleteForm
method to apply the change so message and policy match.
- Line 63: In EmploymentFormService, replace logging of the full Staff/JPA
entity (e.g., logger.info(..., loggedInHr)) with a stable identifier to avoid
dumping entity fields; update all logger calls that pass Staff objects (the
occurrences around lines referenced) to use loggedInHr.getId() or
loggedInHr.getEmail() (or the appropriate getter for the variable names used,
e.g., loggedInRequester.getId()) so logs record only the safe identifier rather
than the entire Staff object.
In
`@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Around line 27-32: Remove the dead commented-out getAllStaff() method from the
StaffRestController class: locate the commented block referencing getAllStaff
(the commented method inside StaffRestController) and delete those commented
lines entirely so no unused/commented code remains in the file; keep the
existing getStaffById(...) and other active methods unchanged.
In `@src/main/resources/application.properties`:
- Around line 20-21: The app.properties now requires ENCRYPTION_KEY and
ENCRYPTION_SALT but lacks documentation and a key-rotation plan; add an
.env.example and README entries describing all required env vars (DATABASE_URL,
DATABASE_USER, DATABASE_PASSWORD, S3_ENDPOINT, S3_ACCESS_KEY, S3_SECRET_KEY,
ENCRYPTION_KEY, ENCRYPTION_SALT), explicitly state ENCRYPTION_SALT must be
hex-encoded (min 16 hex chars / 8 bytes) as used by Encryptors.text(password,
salt), and document deployment notes; implement a key-rotation strategy by
adding key-version metadata for encrypted records and a Flyway migration (or
service migration job) that rewraps existing ciphertexts when ENCRYPTION_KEY
changes (or provide instructions to run the rewrap job), and ensure startup
validates presence/format of app.encryption.key and app.encryption.salt with a
clear error message referencing those property names.
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 129-145: The test createForm_DuplicateSsnInStaff_ThrowsException
is relying on a Mockito stub for encryptionService.encrypt which makes the
ciphertext deterministic and so falsely triggers
staffRepository.existsBySocialSecurityNumber; instead add an integration-style
test that uses the real EncryptionService (the same Encryptors.text-based
implementation used in production) and the real/embedded repository so that
service.createForm observes the true non-deterministic encryption behavior and a
duplicate SSN path can be validated end-to-end, or alternatively adjust the test
to simulate realistic encrypted-value checks by storing the actual encrypted
value returned from the real EncryptionService before invoking
staffRepository.existsBySocialSecurityNumber; reference
createForm_DuplicateSsnInStaff_ThrowsException, encryptionService.encrypt,
staffRepository.existsBySocialSecurityNumber, Encryptors.text, and
service.createForm when making the change.
- Around line 62-87: The test currently asserts encryptionService.encrypt is
called twice which pins a known inefficiency; update the test in
EmploymentFormServiceTest (createForm_Success) to expect a single encryption
call by changing verify(encryptionService, times(2)).encrypt("19900101-1234") to
verify(encryptionService, times(1)).encrypt("19900101-1234") and adjust any
related mocks so the single encrypt() invocation supplies the value used both
for the staff existence check and for persisting the EmploymentForm (aligning
with createForm and validateSsnNotExists behavior that should reuse
EncryptionService.encrypt).
---
Outside diff comments:
In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java`:
- Around line 29-38: DataInitializer currently seeds four ADMIN accounts
unguarded (class DataInitializer, method that calls staffRepository.saveAll with
createStaff(...)), which leaks credentials and personal-looking data; change it
to run only in non-prod by adding a guard (e.g., annotate DataInitializer with
`@Profile`({"dev","test"}) or check an app.seed.enabled property), move all secret
values (passwords) out of literals and load them from configuration/environment
properties, replace real-looking names/emails/phones with clearly synthetic
placeholders, and verify the intended Role for each createStaff(...) call (e.g.,
ensure Role.HR vs Role.ADMIN is correct) so seeding is safe and configurable.
- Around line 41-52: The SSN is being encrypted non-deterministically in
createStaff via encryptionService.encrypt(ssn), which breaks idempotent seeding
and StaffRepository.existsBySocialSecurityNumber(...) lookups; change the
implementation to store a deterministic lookup key (either by calling a
deterministic encryption method from EncryptionConfig/encryptionService or by
computing and storing a deterministic HMAC/fingerprint alongside the
non-deterministic ciphertext) and update Staff to hold both fields (e.g.,
encryptedSsn and ssnFingerprint) so lookups use ssnFingerprint while the
ciphertext remains secure; ensure createStaff sets both values and that
existsBySocialSecurityNumber queries the deterministic field.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 117-126: The update flow currently compares and copies SSNs as
plaintext, breaking encryption and causing unnecessary duplicate checks; to fix,
when comparing SSNs use a decrypted value from existingForm (decrypt
existingForm.getSocialSecurityNumber() via your SSN encryption utility) or
compare using a stable encrypted form so
validateSsnNotExists(updatedForm.getSocialSecurityNumber()) only runs when the
actual plaintext changed, and after calling
employmentMapper.updateEntity(updatedForm, existingForm) re-encrypt the SSN on
existingForm (replace the plaintext value copied by updateEntity with the
encrypted result from your encrypt method) before calling
employmentFormRepository.save(existingForm), ensuring validateSsnNotExists is
invoked on the correct value and persisted SSNs remain encrypted.
- Around line 227-236: validateSsnNotExists currently compares plaintext SSNs to
encrypted DB values and re-encrypts with a non-deterministic cipher, so
uniqueness checks always fail; fix by encrypting the input SSN with a
deterministic/queryable encryptor before any existsBySocialSecurityNumber call
(update validateSsnNotExists to call encryptionService.encryptQueryable(ssn) or
equivalent and use that for both
employmentFormRepository.existsBySocialSecurityNumber(...) and
staffRepository.existsBySocialSecurityNumber(...)), update EncryptionConfig to
use Encryptors.queryableText(...) (or provide a deterministic encrypt/decrypt
pair) so equality checks are stable, and change updateFormBeforeApproval /
employmentMapper.updateEntity flow so the DTO-to-entity path never writes
plaintext SSN—ensure updateFormBeforeApproval encrypts the SSN (using the same
deterministic method) before calling employmentMapper.updateEntity or adjust
EmploymentMapper.setSocialSecurityNumber to accept an already-encrypted value.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/config/security/EncryptionService.java`:
- Around line 13-19: The encrypt/decrypt methods in EncryptionService should be
null-safe: if callers pass null the current calls to
textEncryptor.encrypt/decrypt will NPE. Update the
EncryptionService.encrypt(String data) and EncryptionService.decrypt(String
data) to return null immediately when data is null (or use Objects.isNull),
before calling textEncryptor.encrypt(...) or textEncryptor.decrypt(...),
preserving behavior for non-null inputs; reference the existing textEncryptor
field in the class when adding the guard.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Line 94: Rename misleading parameters and local variables that still end with
"Email" to match their Staff type: change loggedInHrEmail → loggedInHr,
loggedInManagementEmail → loggedInManagement, and loggedInEmail → loggedInStaff
across the methods updateFormBeforeApproval, createForm, and
approveAndFinalizeEmployment (and any local usages within those methods) so all
comparisons and role/id accesses use the new names; update method signatures,
callers, and any Javadoc/comments to reflect the new parameter names to keep
names consistent with the Staff type and avoid compilation errors from
mismatched identifiers.
In
`@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Around line 53-58: Remove the commented-out dead code block containing
getAllStaff and the obsolete call to getStaffByRoleOrDepartment(null, null);
delete that commented code, or if you want to keep a convenience endpoint,
restore a proper getAllStaff method that calls the current three-argument
service method (getStaffByRoleOrDepartment) with appropriate default values and
returns ResponseEntity<List<StaffDTO>>; ensure the method signature is annotated
with `@GetMapping` and uses the correct StaffService method name
getStaffByRoleOrDepartment.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 36-52: The SSN exposure logic in getStaffById and the stream
mapper in getStaffByRoleOrDepartment is duplicated; extract a private helper
method applySsnVisibility(Staff staff, StaffDTO dto, Role requesterRole) that
encapsulates the logic currently using encryptionService.decrypt(...) and
encryptionService.maskLastFour(...), and call that helper from both getStaffById
and getStaffByRoleOrDepartment; ensure the helper treats a null requesterRole
explicitly (e.g., treat null as non-admin) and documents the behavior so the
intent is clear.
In `@src/test/java/org/example/cyberwatch/SecurityIntegrationTests.java`:
- Around line 37-57: Tests are repeatedly constructing a Staff principal and
UsernamePasswordAuthenticationToken with ROLE_HR; extract that logic into a
small test utility method/class (e.g., TestAuthUtils.createStaffAuth or
TestPrincipalFactory.createHrAuthentication) so other tests (hrCanAccessStaff,
hrCannotApproveForm, hrCanCreateForm) can reuse it; the utility should build a
Staff object, setFirstName/LastName/Email/Role, create a
UsernamePasswordAuthenticationToken with that Staff as principal, null
credentials and a List.of(new SimpleGrantedAuthority("ROLE_HR")), and tests
should call .with(authentication(TestAuthUtils.createHrAuthentication()))
instead of duplicating setup.
🪄 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: b47b1c15-e4e0-4f32-9658-ccf7a14edfb7
📒 Files selected for processing (19)
src/main/java/org/example/cyberwatch/config/DataInitializer.javasrc/main/java/org/example/cyberwatch/config/EncryptionConfig.javasrc/main/java/org/example/cyberwatch/config/security/EncryptionService.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/application.propertiessrc/main/resources/db/migration/V6__add_test_user.sqlsrc/main/resources/db/migration/V6__add_ticket_assignments_many_to_many.sqlsrc/main/resources/db/migration/V7__remove_unused_entities.sqlsrc/main/resources/db/migration/V8__add_admin_user.sqlsrc/test/java/org/example/cyberwatch/SecurityIntegrationTests.javasrc/test/java/org/example/cyberwatch/TicketAttachmentSecurityTest.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.javasrc/test/resources/application-test.properties
💤 Files with no reviewable changes (4)
- src/main/resources/db/migration/V6__add_test_user.sql
- src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
- src/main/resources/db/migration/V8__add_admin_user.sql
- src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java
# Conflicts: # src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java # src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java # src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java # src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java # src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
…ity and privacy by masking SSNs for non-admin users and adjusting method signatures for clarity.
…ity and privacy by masking SSNs for non-admin users and adjusting method signatures for clarity.
…ity and privacy by masking SSNs for non-admin users and adjusting method signatures for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
235-241:⚠️ Potential issue | 🔴 CriticalDuplicate SSN validation is completely broken due to encryption non-determinism.
Line 236 searches for raw SSN in a column storing encrypted values—plaintext will never match ciphertext. Line 240–241 is worse: Spring Security's
Encryptors.text()uses a random initialization vector on each call, so re-encrypting the same SSN produces different ciphertext each time. Both checks will always fail silently, allowing duplicate SSNs through validation (only caught later by the database unique constraint, not by the intended user-facing error).Use a deterministic keyed hash or HMAC-based blind index column for uniqueness checks, keeping the encrypted SSN for decryption:
Suggested model-level direction
private void validateSsnNotExists(String ssn) { - if (employmentFormRepository.existsBySocialSecurityNumber(ssn)) { + String ssnFingerprint = encryptionService.fingerprint(ssn); + if (employmentFormRepository.existsBySocialSecurityNumberFingerprint(ssnFingerprint)) { throw new IllegalStateException("An application with this SSN already exists."); } - // Kryptera innan sökning i staff-tabellen - String encryptedSsn = encryptionService.encrypt(ssn); - if (staffRepository.existsBySocialSecurityNumber(encryptedSsn)) { + if (staffRepository.existsBySocialSecurityNumberFingerprint(ssnFingerprint)) { throw new IllegalStateException("An employee with this SSN already exists."); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 235 - 241, The current validateSsnNotExists method is broken because it compares plaintext or re-encrypted SSNs against non-deterministic ciphertext; replace these checks with deterministic blind-index/HMAC lookups: add a new deterministic hash field/column (e.g., socialSecurityNumberHash) on the EmploymentForm and Staff models, compute the HMAC/HASH using a keyed secret (implement in a new HashService or extend EncryptionService with a hmac(String) method), persist that hash whenever records are saved, add repository methods like existsBySocialSecurityNumberHash(String) on employmentFormRepository and staffRepository, and change validateSsnNotExists to compute the hash from the incoming ssn and call existsBySocialSecurityNumberHash(...) (remove the plaintext and re-encryption checks). Also add a DB migration to backfill the hash column for existing rows and ensure the secret key is provided via configuration.
122-128:⚠️ Potential issue | 🟠 MajorFix SSN encryption handling in update flow to match create flow behavior.
The
updateFormBeforeApprovalmethod has a critical encryption inconsistency: the create flow explicitly encrypts SSN before saving (line 63-66), but the update flow does not. AfteremploymentMapper.updateEntity(updatedForm, existingForm), the raw SSN from the DTO overwrites the encrypted SSN in the entity, persisting unencrypted sensitive data to the database.Additionally, the SSN comparison at line 122 is flawed: it compares the encrypted SSN loaded from the database with the raw SSN from the DTO, which will always differ unless the input happens to be the ciphertext itself.
- Decrypt
existingForm.getSocialSecurityNumber()before comparison- Re-encrypt
updatedForm.getSocialSecurityNumber()after the mapper call and beforesave()Proposed fix
- // Check for duplicate SSN if it's changed - if (!existingForm.getSocialSecurityNumber().equals(updatedForm.getSocialSecurityNumber())) { - validateSsnNotExists(updatedForm.getSocialSecurityNumber()); - } - - employmentMapper.updateEntity(updatedForm, existingForm); + String updatedSsn = updatedForm.getSocialSecurityNumber(); + String currentEncryptedSsn = existingForm.getSocialSecurityNumber(); + + if (updatedSsn != null) { + String currentSsn = encryptionService.decrypt(currentEncryptedSsn); + if (!Objects.equals(currentSsn, updatedSsn)) { + validateSsnNotExists(updatedSsn); + } + } + + employmentMapper.updateEntity(updatedForm, existingForm); + if (updatedSsn != null) { + existingForm.setSocialSecurityNumber(encryptionService.encrypt(updatedSsn)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 122 - 128, In updateFormBeforeApproval the SSN handling is inconsistent: decrypt existingForm.getSocialSecurityNumber() before comparing to updatedForm.getSocialSecurityNumber() (so the validateSsnNotExists check compares plaintext values), then after employmentMapper.updateEntity(updatedForm, existingForm) re-encrypt updatedForm.getSocialSecurityNumber() (so the entity isn't left with raw SSN) using the same encryptor.encrypt(...) used in create flow, and only then call save() (or employmentRepository.save) to persist; keep validateSsnNotExists usage but operate on the plaintext SSN.
🧹 Nitpick comments (1)
src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java (1)
219-259: Remove the commented-out duplicate tests.These stale tests are now superseded by the active cases above and make the suite harder to scan.
🧹 Proposed cleanup
-// `@Test` -// `@DisplayName`("Should throw exception if SSN already exists in form repository") -// void createForm_DuplicateSsnInForms_ThrowsException() { -// //arrange -// Staff creator = new Staff(); -// creator.setEmail("owner@cyberwatch.local"); -// CreateEmploymentDTO dto = new CreateEmploymentDTO( -// "19900101-1234", "Alice", "Andersson", -// "alice@test.com", "070", Role.HR, Department.BACKEND, null, null, null -// ); -// -// //act -// when(formRepository.existsBySocialSecurityNumber("19900101-1234")).thenReturn(true); -// -// //assert -// assertThrows(IllegalStateException.class, () -> -// service.createForm(dto, creator) -// ); -// verify(formRepository, never()).save(any()); -// } - - -// `@Test` -// `@DisplayName`("Should throw exception if SSN already exists in staff repository") -// void createForm_DuplicateSsnInStaff_ThrowsException() { -// //arrange -// Staff creator = new Staff(); -// creator.setEmail("owner@cyberwatch.local"); -// CreateEmploymentDTO dto = new CreateEmploymentDTO( -// "19900101-1234", "Alice", "Andersson", -// "alice@test.com", "070", Role.HR, Department.BACKEND, null, null, null -// ); -// -// //act -// when(staffRepository.existsBySocialSecurityNumber("19900101-1234")).thenReturn(true); -// -// //assert -// assertThrows(IllegalStateException.class, () -> -// service.createForm(dto, creator) -// ); -// verify(formRepository, never()).save(any()); -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java` around lines 219 - 259, Delete the two commented-out test blocks createForm_DuplicateSsnInForms_ThrowsException and createForm_DuplicateSsnInStaff_ThrowsException from EmploymentFormServiceTest; these stale, commented tests reference createForm and mocks formRepository.existsBySocialSecurityNumber / staffRepository.existsBySocialSecurityNumber and should be removed so only the active duplicate-SSN test cases remain, keeping the file clean and easier to scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 58-65: getUserStaff currently returns the raw encrypted SSN
because it just calls staffMapper.toDto(staff); update it to apply the same
decrypt-and-mask policy used in getStaffById and getStaffByRoleOrDepartment:
obtain the StaffDTO (or create it) and replace its socialSecurityNumber with the
result of the shared decrypt+mask logic (e.g., call the existing utility/method
used by getStaffById/getStaffByRoleOrDepartment or replicate that logic: decrypt
staff.getSocialSecurityNumber(), mask to show only last 4 digits), so the DTO
returned by getUserStaff never contains ciphertext but the masked SSN.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 235-241: The current validateSsnNotExists method is broken because
it compares plaintext or re-encrypted SSNs against non-deterministic ciphertext;
replace these checks with deterministic blind-index/HMAC lookups: add a new
deterministic hash field/column (e.g., socialSecurityNumberHash) on the
EmploymentForm and Staff models, compute the HMAC/HASH using a keyed secret
(implement in a new HashService or extend EncryptionService with a hmac(String)
method), persist that hash whenever records are saved, add repository methods
like existsBySocialSecurityNumberHash(String) on employmentFormRepository and
staffRepository, and change validateSsnNotExists to compute the hash from the
incoming ssn and call existsBySocialSecurityNumberHash(...) (remove the
plaintext and re-encryption checks). Also add a DB migration to backfill the
hash column for existing rows and ensure the secret key is provided via
configuration.
- Around line 122-128: In updateFormBeforeApproval the SSN handling is
inconsistent: decrypt existingForm.getSocialSecurityNumber() before comparing to
updatedForm.getSocialSecurityNumber() (so the validateSsnNotExists check
compares plaintext values), then after
employmentMapper.updateEntity(updatedForm, existingForm) re-encrypt
updatedForm.getSocialSecurityNumber() (so the entity isn't left with raw SSN)
using the same encryptor.encrypt(...) used in create flow, and only then call
save() (or employmentRepository.save) to persist; keep validateSsnNotExists
usage but operate on the plaintext SSN.
---
Nitpick comments:
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 219-259: Delete the two commented-out test blocks
createForm_DuplicateSsnInForms_ThrowsException and
createForm_DuplicateSsnInStaff_ThrowsException from EmploymentFormServiceTest;
these stale, commented tests reference createForm and mocks
formRepository.existsBySocialSecurityNumber /
staffRepository.existsBySocialSecurityNumber and should be removed so only the
active duplicate-SSN test cases remain, keeping the file clean and easier to
scan.
🪄 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: 20ce1255-f06a-4730-b5ad-3a1785b08804
⛔ Files ignored due to path filters (1)
logs/cyberwatch.logis excluded by!**/*.log
📒 Files selected for processing (9)
src/main/java/org/example/cyberwatch/config/security/EncryptionService.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/application.propertiessrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/application.properties
- src/main/java/org/example/cyberwatch/config/security/EncryptionService.java
- src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java
…thods to ensure SSNs are encrypted and improve security checks for duplicates.
…thods to ensure SSNs are encrypted and improve security checks for duplicates.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
121-130:⚠️ Potential issue | 🔴 CriticalMove the mapper call before SSN encryption to prevent plaintext overwrite.
The
EmploymentMapper.updateEntity()method (line 52) copiesdto.getSocialSecurityNumber()directly to the entity. Since this is called afterexistingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain))at line 126, the mapper overwrites the encrypted SSN with plaintext data. This breaks SSN-at-rest encryption and stores plaintext values in the database.Reorder the operations to call the mapper first, then handle SSN encryption separately to prevent the mapper from overwriting encrypted data.
Example fix approach
+ employmentMapper.updateEntity(updatedForm, existingForm); + String existingSsnPlain = encryptionService.decrypt(existingForm.getSocialSecurityNumber()); String newSsnPlain = updatedForm.getSocialSecurityNumber(); // Check for duplicate SSN if it's changed if (!existingSsnPlain.equals(newSsnPlain)) { validateSsnNotExists(newSsnPlain); existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)); } - - employmentMapper.updateEntity(updatedForm, existingForm);Alternatively, exclude
socialSecurityNumberfrom the mapper's field copies since it requires special encryption handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java` around lines 121 - 130, Move the employmentMapper.updateEntity(updatedForm, existingForm) call before any SSN encryption so the mapper doesn't overwrite the encrypted value; call employmentMapper.updateEntity(updatedForm, existingForm) first, then retrieve plaintext SSNs via encryptionService.decrypt(existingForm.getSocialSecurityNumber()) and updatedForm.getSocialSecurityNumber(), compare and if changed call validateSsnNotExists(newSsnPlain) and set the encrypted value on the entity with existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)); alternatively, if you prefer not to reorder, modify employmentMapper.updateEntity to skip copying socialSecurityNumber so that EmploymentFormService alone handles encryption logic.src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java (1)
126-136:⚠️ Potential issue | 🟡 MinorAdd
throws Exceptionto this test method for consistency with Jackson 3 API.
rejectForm_Success()at line 126 stubsobjectMapper.writeValueAsString(any()), which throws a checked exception in Jackson 3 (JacksonException). Although the mock masks this at runtime, the method signature should match the pattern used inapproveAndFinalize_Success()for consistency and defensive coding.Proposed fix
- void rejectForm_Success() { + void rejectForm_Success() throws Exception {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java` around lines 126 - 136, The test method rejectForm_Success should declare throws Exception to match Jackson 3's checked exception behavior and be consistent with approveAndFinalize_Success; update the method signature of rejectForm_Success() to include "throws Exception" (this aligns with the stubbing of objectMapper.writeValueAsString(any()) and avoids compilation issues when Jackson's JacksonException is treated as checked).
🧹 Nitpick comments (2)
src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java (1)
26-32: Remove the stale implementation comment.
getUserStaffnow exists, so//skapa en ny metod i serviceis no longer useful.🧹 Proposed cleanup
`@GetMapping`("/me") - //skapa en ny metod i service public ResponseEntity<StaffDTO> getCurrentUser(`@AuthenticationPrincipal` Staff staff) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java` around lines 26 - 32, Remove the stale inline comment in StaffRestController's getCurrentUser method; delete the line "//skapa en ny metod i service" since getUserStaff already exists and is used to return StaffDTO for the authenticated Staff (method names: getCurrentUser, getUserStaff, class: StaffRestController, return type: ResponseEntity<StaffDTO>, mapping: `@GetMapping`("/me")).src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java (1)
41-43: Remove the duplicate TODO comment.The return-type note is present twice around the approve endpoint; keep one clear TODO or move it to an issue.
🧹 Proposed cleanup
- //Change returntype when emailservice is implemented `@PostMapping`("/{id}/approve") - //Change returntype when emailservice is implemented + // TODO: Change return type when email service is implemented. public ResponseEntity<String> approveForm(`@PathVariable` Long id, Authentication authentication) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java` around lines 41 - 43, In EmploymentFormController, remove the duplicated comment lines "//Change returntype when emailservice is implemented" surrounding the `@PostMapping`("/{id}/approve") endpoint so only a single TODO remains (or move the note to an issue tracker); update the comment placement near the approve method to keep one clear TODO and delete the redundant duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 237-257: validateSsnNotExists currently decrypts every row which
allows race conditions; instead add a normalized SSN hash column (e.g., ssnHash)
with a DB unique constraint, create a migration to populate existing rows by
computing the normalized (digits-only) SHA-256/HMAC of socialSecurityNumber, and
update create/update flows to compute and persist ssnHash via encryptionService
(or a new hashing utility) before saving; then change validateSsnNotExists to
query repositories (add methods like
employmentFormRepository.existsBySsnHash(...) and
staffRepository.existsBySsnHash(...)) to check uniqueness, and ensure save is
protected by the DB constraint and translate integrity exceptions (e.g.,
DataIntegrityViolationException) into the existing IllegalStateException to
handle concurrent inserts.
- Around line 67-70: The EmploymentMapper.toDTO(...) is returning encrypted SSNs
directly to clients; add a single DTO sanitization helper in
EmploymentFormService (e.g., a private toSafeDto(EmploymentForm) that calls
employmentMapper.toDTO(form) then replaces the SSN via
encryptionService.maskLastFour(...) or similar) and use that helper instead of
returning employmentMapper.toDTO(...) in createForm, getAllForms, getFormById
and updateForm so all outgoing EmploymentFormDTOs have masked SSNs following the
role-based pattern used in StaffService.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 46-63: The updateStaff and updateStatus endpoints currently return
DTOs with unsanitized SSNs because they call staffMapper.toDto(...) directly;
centralize SSN handling by adding helper methods (e.g., toDtoWithSsnPolicy(Staff
staff, Staff requester) and toMaskedDto(Staff staff)) that wrap
staffMapper.toDto(...) and set the socialSecurityNumber using
encryptionService.decrypt(...) for ADMIN/HR or
encryptionService.maskLastFour(...) otherwise, then replace direct
staffMapper.toDto(...) returns in updateStaff and updateStatus with calls to the
new helper(s); ensure existing callers like getStaffById, getUserStaff, and
getStaffByRoleOrDepartment use or are migrated to the same helpers so SSN policy
is consistent.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 121-130: Move the employmentMapper.updateEntity(updatedForm,
existingForm) call before any SSN encryption so the mapper doesn't overwrite the
encrypted value; call employmentMapper.updateEntity(updatedForm, existingForm)
first, then retrieve plaintext SSNs via
encryptionService.decrypt(existingForm.getSocialSecurityNumber()) and
updatedForm.getSocialSecurityNumber(), compare and if changed call
validateSsnNotExists(newSsnPlain) and set the encrypted value on the entity with
existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain));
alternatively, if you prefer not to reorder, modify
employmentMapper.updateEntity to skip copying socialSecurityNumber so that
EmploymentFormService alone handles encryption logic.
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 126-136: The test method rejectForm_Success should declare throws
Exception to match Jackson 3's checked exception behavior and be consistent with
approveAndFinalize_Success; update the method signature of rejectForm_Success()
to include "throws Exception" (this aligns with the stubbing of
objectMapper.writeValueAsString(any()) and avoids compilation issues when
Jackson's JacksonException is treated as checked).
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 41-43: In EmploymentFormController, remove the duplicated comment
lines "//Change returntype when emailservice is implemented" surrounding the
`@PostMapping`("/{id}/approve") endpoint so only a single TODO remains (or move
the note to an issue tracker); update the comment placement near the approve
method to keep one clear TODO and delete the redundant duplicate.
In
`@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Around line 26-32: Remove the stale inline comment in StaffRestController's
getCurrentUser method; delete the line "//skapa en ny metod i service" since
getUserStaff already exists and is used to return StaffDTO for the authenticated
Staff (method names: getCurrentUser, getUserStaff, class: StaffRestController,
return type: ResponseEntity<StaffDTO>, mapping: `@GetMapping`("/me")).
🪄 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: fd7ba16e-2570-491a-a130-3161af7ad310
⛔ Files ignored due to path filters (1)
logs/cyberwatch.logis excluded by!**/*.log
📒 Files selected for processing (5)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
# Conflicts: # src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
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/cyberwatch/config/DataInitializer.java (1)
14-38:⚠️ Potential issue | 🔴 CriticalGate seed admin accounts and remove real-looking PII from committed defaults.
Because this initializer is a global
@Component, an empty production database would receive fourADMINaccounts with predictable passwords and committed SSN/email/phone values. Please restrict this to local/test seeding or an explicit opt-in flag, and use non-routable dummy data or environment-provided credentials.🛡️ Suggested guard for seed data
import org.example.cyberwatch.shared.model.enums.Role; import org.springframework.boot.CommandLineRunner; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Component; @@ `@Component` +@ConditionalOnProperty(name = "app.seed-data.enabled", havingValue = "true") public class DataInitializer implements CommandLineRunner {Then enable
app.seed-data.enabled=trueonly in local/test profiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java` around lines 14 - 38, DataInitializer currently seeds four ADMIN users unconditionally in run(...) using staffRepository.saveAll(...) with real-looking PII and predictable passwords; guard this so seeding only happens when explicitly enabled (e.g., check a configuration property app.seed-data.enabled or active profile is local/test) before calling createStaff(...) and saveAll, and replace the committed SSN/email/phone/password values with non-routable/dummy placeholders or read credentials from environment variables; update the DataInitializer constructor to accept the configuration/property (or use `@Value/`@ConfigurationProperties) and ensure createStaff(...) and run(...) use that gate to prevent automatic production seeding.
🧹 Nitpick comments (3)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java (2)
42-44: Duplicate comment.The same
//Change returntype when emailservice is implementednote appears both above and below the@PostMappingannotation. Keep only one.🧹 Proposed cleanup
- //Change returntype when emailservice is implemented `@PostMapping`("/{id}/approve") //Change returntype when emailservice is implemented public ResponseEntity<String> approveForm(`@PathVariable` Long id, Authentication authentication) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java` around lines 42 - 44, Remove the duplicated comment surrounding the mapping in EmploymentFormController: keep a single "//Change returntype when emailservice is implemented" and delete the redundant copy so the comment appears only once adjacent to the `@PostMapping`("/{id}/approve") annotation; update the EmploymentFormController method block (the approve endpoint) to have just one instance of that comment.
61-101: InconsistentStaffreference style.Now that
org.example.cyberwatch.features.staff.model.Staffis imported (line 8), the remaining methods (updateFormL61,rejectFormL87,deleteFormL101) still use the fully-qualified name. Consider using the short name for consistency withcreateEmploymentFormandapproveForm.Additionally, the
principal instanceof Staffcheck is repeated 5 times in this controller — extracting a smallgetAuthenticatedStaff(Authentication)helper (mirroring the pattern already used inTicketController#getAuthenticatedStaff) would remove the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java` around lines 61 - 101, The controller uses fully-qualified org.example.cyberwatch.features.staff.model.Staff in updateForm, rejectForm, and deleteForm even though Staff is imported; replace those fully-qualified references with the short Staff type to match createEmploymentForm/approveForm and keep style consistent, and extract the repeated principal instanceof Staff check into a private helper getAuthenticatedStaff(Authentication) (follow the same signature/pattern as TicketController#getAuthenticatedStaff) that throws AccessDeniedException when the principal is not a Staff, then update updateForm, rejectForm, deleteForm (and any other duplicated checks) to call this helper and use the returned Staff.src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java (1)
85-113: Separate requester fixtures from returned staff and assert list SSN visibility.Several tests pass
newStaffas both the requester and the repository result, so an implementation that accidentally checks the target staff role could still pass. Use a distinct requester fixture, and add SSN assertions/verifications in the collection tests that currently only check list size.🧪 Example tightening for one masked-list case
`@DisplayName`("Should filter by role and mask SSN for non-admin or hr") void getStaffByRoleOrDepartment_FilterByRole_MaskedSsn() { - newStaff.setRole(Role.CEO); + Staff requester = new Staff(); + requester.setRole(Role.CEO); + newStaff.setRole(Role.HR); when(staffRepository.findByRole(Role.HR)).thenReturn(List.of(newStaff)); when(staffMapper.toDto(newStaff)).thenReturn(newStaffDTO); when(encryptionService.maskLastFour("krypterat-ssn")).thenReturn("19900101-****"); - List<StaffDTO> result = staffService.getStaffByRoleOrDepartment(Role.HR, null, newStaff); + List<StaffDTO> result = staffService.getStaffByRoleOrDepartment(Role.HR, null, requester); assertThat(result).hasSize(1); + assertThat(result.get(0).getSocialSecurityNumber()).isEqualTo("19900101-****"); verify(staffRepository, times(1)).findByRole(Role.HR); verify(staffRepository, never()).findByDepartment(any()); + verify(encryptionService).maskLastFour("krypterat-ssn"); + verify(encryptionService, never()).decrypt(any()); }Also applies to: 176-200, 204-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java` around lines 85 - 113, The tests use the same fixture (newStaff) as both the requester and the repository result which masks a bug where the code checks the target staff's role instead of the requester's; change tests like getStaffById_NonAdminOrHr_ReturnsMaskedSsn and getStaffById_Admin_ReturnsDecryptedSsn to pass a distinct requester fixture (e.g., requesterStaff) to staffService.getStaffById while keeping the repository result as a separate targetStaff/newStaff, set requesterStaff.setRole(...) appropriately, and update the collection/list tests referenced (around lines 176-200 and 204-245) to assert the SSN visibility for each returned StaffDTO and to verify encryptionService.maskLastFour(...) or decrypt(...) calls accordingly so the behavior is validated against the requester role rather than the target staff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Line 50: The log message in EmploymentFormController.approveForm is misleading
("Creating employment form with approval from staff: {}"); update the
logger.info call in the approveForm method to reflect that the existing form is
being approved/finalized (for example "Approving/finalizing employment form
approved by staff: {}" or "Finalizing employment form approved by staff: {}")
and keep the staff.getEmail() parameter intact so logs correctly show the
approver's email.
---
Outside diff comments:
In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java`:
- Around line 14-38: DataInitializer currently seeds four ADMIN users
unconditionally in run(...) using staffRepository.saveAll(...) with real-looking
PII and predictable passwords; guard this so seeding only happens when
explicitly enabled (e.g., check a configuration property app.seed-data.enabled
or active profile is local/test) before calling createStaff(...) and saveAll,
and replace the committed SSN/email/phone/password values with
non-routable/dummy placeholders or read credentials from environment variables;
update the DataInitializer constructor to accept the configuration/property (or
use `@Value/`@ConfigurationProperties) and ensure createStaff(...) and run(...)
use that gate to prevent automatic production seeding.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 42-44: Remove the duplicated comment surrounding the mapping in
EmploymentFormController: keep a single "//Change returntype when emailservice
is implemented" and delete the redundant copy so the comment appears only once
adjacent to the `@PostMapping`("/{id}/approve") annotation; update the
EmploymentFormController method block (the approve endpoint) to have just one
instance of that comment.
- Around line 61-101: The controller uses fully-qualified
org.example.cyberwatch.features.staff.model.Staff in updateForm, rejectForm, and
deleteForm even though Staff is imported; replace those fully-qualified
references with the short Staff type to match createEmploymentForm/approveForm
and keep style consistent, and extract the repeated principal instanceof Staff
check into a private helper getAuthenticatedStaff(Authentication) (follow the
same signature/pattern as TicketController#getAuthenticatedStaff) that throws
AccessDeniedException when the principal is not a Staff, then update updateForm,
rejectForm, deleteForm (and any other duplicated checks) to call this helper and
use the returned Staff.
In
`@src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java`:
- Around line 85-113: The tests use the same fixture (newStaff) as both the
requester and the repository result which masks a bug where the code checks the
target staff's role instead of the requester's; change tests like
getStaffById_NonAdminOrHr_ReturnsMaskedSsn and
getStaffById_Admin_ReturnsDecryptedSsn to pass a distinct requester fixture
(e.g., requesterStaff) to staffService.getStaffById while keeping the repository
result as a separate targetStaff/newStaff, set requesterStaff.setRole(...)
appropriately, and update the collection/list tests referenced (around lines
176-200 and 204-245) to assert the SSN visibility for each returned StaffDTO and
to verify encryptionService.maskLastFour(...) or decrypt(...) calls accordingly
so the behavior is validated against the requester role rather than the target
staff.
🪄 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: 222712e6-1d9f-4991-b9c7-7d12ead27caa
📒 Files selected for processing (4)
src/main/java/org/example/cyberwatch/config/DataInitializer.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
7 tasks
…tionPrincipal for staff authentication; enhance SSN handling with masking and decryption policies
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java (1)
26-33: Remove stale Swedish TODO comment.
//skapa en ny metod i serviceis a leftover note ("create a new method in service") that no longer applies —getUserStaff(Staff)has already been added.`@GetMapping`("/me") - //skapa en ny metod i service public ResponseEntity<StaffDTO> getCurrentUser(`@AuthenticationPrincipal` Staff staff) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java` around lines 26 - 33, Remove the stale Swedish TODO comment in StaffRestController above the getCurrentUser method; specifically delete the line "//skapa en ny metod i service" so the controller only contains the `@GetMapping` and the method implementation that calls staffService.getUserStaff(Staff) without the outdated note.src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (1)
75-78: Stale comments contradict the actual SSN policy.Both comments state
// Returnerar maskat SSNbuttoDtoWithSsnPolicyreturns a decrypted SSN when the requester isADMIN/HRand only masks otherwise. Update or drop the comments to avoid misleading readers about the privacy guarantees of the response.- return toDtoWithSsnPolicy(savedStaff, requester); // Returnerar maskat SSN efter uppdatering + return toDtoWithSsnPolicy(savedStaff, requester); // Decrypted for ADMIN/HR, masked for others- return toDtoWithSsnPolicy(savedStaff, requester); // Returnerar maskat SSN efter statusuppdatering + return toDtoWithSsnPolicy(savedStaff, requester); // Decrypted for ADMIN/HR, masked for othersAlso applies to: 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java` around lines 75 - 78, Update the misleading inline comments that say "// Returnerar maskat SSN" near the call to toDtoWithSsnPolicy after saving staff (where savedStaff is returned and logger.info("Staff {} updated", staffId) is logged) to correctly reflect the behavior of toDtoWithSsnPolicy: state that it returns a decrypted SSN for ADMIN/HR requesters and a masked SSN for others, or simply remove the comment; apply the same fix to the other occurrence around lines 121-125 where toDtoWithSsnPolicy is used.src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java (1)
225-248: Strengthen no-filter tests to actually assert SSN policy.
getStaffByRoleOrDepartment_NoFilterandgetStaffByRoleOrDepartment_NoFilter_MaskedFouronly assert list size — they don't verify whichEncryptionServicepath was taken or what SSN value the returned DTO carries. Assertingresult.getFirst().getSocialSecurityNumber()andverify(encryptionService).maskLastFour(...)/verify(..., never()).decrypt(...)(mirroringgetStaffById_NonAdminOrHr_ReturnsMaskedSsn) would prevent regressions where the SSN policy silently breaks for the no-filter list path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java` around lines 225 - 248, Update the two tests getStaffByRoleOrDepartment_NoFilter and getStaffByRoleOrDepartment_NoFilter_MaskedFour to assert the returned DTO's SSN value and verify which EncryptionService method was used: call staffService.getStaffByRoleOrDepartment(...) as before, then assert result.get(0).getSocialSecurityNumber() equals the expected masked or unmasked value and add verify(encryptionService).maskLastFour("krypterat-ssn") in the masked test (and verify(encryptionService, never()).decrypt(...) for the masked case), and for the non-masked test assert the DTO SSN equals the original and verify(encryptionService).decrypt(...) was called or maskLastFour was never called as appropriate; locate these changes in the existing test methods getStaffByRoleOrDepartment_NoFilter and getStaffByRoleOrDepartment_NoFilter_MaskedFour.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 260-265: The call to encryptionService.decrypt(...) in toSafeDto
is redundant and discards its result, so remove that invocation and rely on
encryptionService.maskLastFour(form.getSocialSecurityNumber()) to both decrypt
and mask; update EmploymentFormService.toSafeDto to set
dto.setSocialSecurityNumber(encryptionService.maskLastFour(form.getSocialSecurityNumber()))
without the prior decrypt call.
- Around line 121-127: The current code in EmploymentFormService calls
encryptionService.decrypt(existingForm.getSocialSecurityNumber()) which will
throw on null/blank persisted SSNs; guard against that by checking whether
existingForm.getSocialSecurityNumber() is non-null and non-blank before calling
encryptionService.decrypt, e.g., compute existingSsnPlain only when
StringUtils.hasText(existingForm.getSocialSecurityNumber()) (or similar) and
treat a null/blank persisted SSN as different so you still call
validateSsnNotExists(updatedForm.getSocialSecurityNumber()) and then set
existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain));
alternatively, update EncryptionService.decrypt to safely return null/empty for
null/blank inputs—apply the approach in the block around validateSsnNotExists,
encrypt(), and setSocialSecurityNumber to avoid 500s on legacy rows.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 49-54: The null-check in getUserStaff incorrectly states "Staff ID
cannot be null" while the parameter is a Staff object; update the
IllegalArgumentException message in getUserStaff to accurately reflect the
parameter (e.g., "Staff cannot be null" or "user cannot be null") so
callers/logs are not misled, leaving the rest of the method (return
toMaskedDto(user)) unchanged.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.java`:
- Around line 26-33: Remove the stale Swedish TODO comment in
StaffRestController above the getCurrentUser method; specifically delete the
line "//skapa en ny metod i service" so the controller only contains the
`@GetMapping` and the method implementation that calls
staffService.getUserStaff(Staff) without the outdated note.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 75-78: Update the misleading inline comments that say "//
Returnerar maskat SSN" near the call to toDtoWithSsnPolicy after saving staff
(where savedStaff is returned and logger.info("Staff {} updated", staffId) is
logged) to correctly reflect the behavior of toDtoWithSsnPolicy: state that it
returns a decrypted SSN for ADMIN/HR requesters and a masked SSN for others, or
simply remove the comment; apply the same fix to the other occurrence around
lines 121-125 where toDtoWithSsnPolicy is used.
In
`@src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java`:
- Around line 225-248: Update the two tests getStaffByRoleOrDepartment_NoFilter
and getStaffByRoleOrDepartment_NoFilter_MaskedFour to assert the returned DTO's
SSN value and verify which EncryptionService method was used: call
staffService.getStaffByRoleOrDepartment(...) as before, then assert
result.get(0).getSocialSecurityNumber() equals the expected masked or unmasked
value and add verify(encryptionService).maskLastFour("krypterat-ssn") in the
masked test (and verify(encryptionService, never()).decrypt(...) for the masked
case), and for the non-masked test assert the DTO SSN equals the original and
verify(encryptionService).decrypt(...) was called or maskLastFour was never
called as appropriate; locate these changes in the existing test methods
getStaffByRoleOrDepartment_NoFilter and
getStaffByRoleOrDepartment_NoFilter_MaskedFour.
🪄 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: 791e5b97-7053-4fc2-8143-b0a84fc04288
📒 Files selected for processing (5)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffRestController.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
…e in approval tests
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.
This pull request introduces significant improvements to the handling of sensitive data (specifically social security numbers), streamlines authentication and authorization by leveraging the authenticated
Staffprincipal, and refactors controller and service methods for better security and maintainability. The most important changes are summarized below.Sensitive Data Encryption
EncryptionServiceandEncryptionConfigto handle encryption and decryption of sensitive fields, specifically social security numbers (SSN). All SSNs are now encrypted before being stored or queried in the database. (EncryptionService.java,EncryptionConfig.java,DataInitializer.java,EmploymentFormService.java) [1] [2] [3] [4] [5] [6] [7] [8]Authentication and Authorization Refactor
Controller methods now use
@AuthenticationPrincipal Staffto inject the authenticated user directly, replacing manual extraction of usernames/emails fromAuthentication. This change is reflected in both employment form and staff controllers. (EmploymentFormController.java,StaffRestController.java) [1] [2] [3] [4] [5]Service methods in
EmploymentFormServicenow acceptStaffobjects instead of emails/usernames, improving type safety and reducing redundant lookups. Authorization checks are performed using theStaffobject's role and ID. (EmploymentFormService.java) [1] [2] [3] [4] [5] [6] [7]Data Initialization Updates
DataInitializerto use real names/emails and to encrypt SSNs upon creation. (DataInitializer.java)Validation and Model Simplification
EmploymentFormhas been removed, as SSNs are now always stored encrypted and cannot be validated via regex at this layer. (EmploymentForm.java)Access Control Improvements
Staffobject’s role and ID, improving security and clarity in permission checks for update, approve, reject, and delete operations. (EmploymentFormService.java) [1] [2] [3]These changes collectively enhance security (especially regarding SSN handling), improve code maintainability, and align the authentication flow with Spring Security best practices.
Summary by CodeRabbit
New Features
Security & Access Control
Behavior Changes
Tests
Chores / Data