Enhancement/refactor encryption#91
Merged
Merged
Conversation
…ndexes; include `commons-codec` dependency for hashing
…ng with `hmacKey` for search, update encryption configuration and schema migration
… access, and update schema constraints
…date validation logic, and streamline encryption handling
… HMAC-based SSN hashing, and enhance SSN access control in services
…ggling, update EmploymentForm and Staff services to apply SSN policy based on user roles, and improve styling for SSN elements
… and pending employment forms; update DataInitializer with enhanced staff and employment form creation logic.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughIntroduces HMAC-based blind-indexing for SSNs (ssnHash): adds hmac support, switches encryption config to password+pepper with Encryptors.delux, updates entities/repositories/migration to store ssnHash, refactors services to use existsBySsnHash checks, seeds data, updates UI and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as StaffService/EmploymentFormService
participant Encryption as EncryptionService
participant Repo as Staff/EmploymentForm Repository
participant DB as Database
Client->>Service: submit SSN (plain)
Service->>Encryption: hmac(plainSSN)
Encryption-->>Service: ssnHash
Service->>Repo: existsBySsnHash(ssnHash)?
Repo->>DB: SELECT WHERE ssn_hash=?
DB-->>Repo: exists? true/false
Repo-->>Service: result
alt exists
Service-->>Client: error (duplicate SSN)
else not exists
Service->>Encryption: encrypt(plainSSN)
Encryption-->>Service: encryptedSSN
Service->>Repo: save(entity with encryptedSSN + ssnHash)
Repo->>DB: INSERT/UPDATE
DB-->>Repo: persisted
Repo-->>Service: saved entity
Service-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/db/migration/V12__add_ssn_hash.sql (1)
1-13:⚠️ Potential issue | 🟠 MajorAdd backfill and
NOT NULLconstraint, or document the nullable assumption.The migration adds
ssn_hashas nullable and creates unique indexes but does not backfill existing rows. PostgreSQL allows multipleNULLvalues in unique indexes, so:
- Any pre-existing rows will have
ssn_hash = NULLand won't be covered by the uniqueness constraint.existsBySsnHash(...)queries inStaffService.validateSsnNotExists()andEmploymentFormService.validateSsnNotExists()will not find these NULL rows, bypassing duplicate-SSN checks for legacy data.While the codebase correctly populates
ssnHashfor all new/updated records (viaDataInitializer, service layer), the column definition in bothStaff.javaandEmploymentForm.javalacksnullable = false, creating an implicit assumption that the database starts fresh. Either:
- Add a backfill migration that computes and populates
ssn_hashfor existing rows, then addNOT NULLto both columns, or- Explicitly document that this migration assumes a clean database (e.g., in a comment or migration notes).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V12__add_ssn_hash.sql` around lines 1 - 13, The migration V12__add_ssn_hash.sql adds nullable ssn_hash and unique indexes but doesn't backfill existing rows, which allows multiple NULLs to bypass uniqueness and the existsBySsnHash checks used by StaffService.validateSsnNotExists and EmploymentFormService.validateSsnNotExists; either add a follow-up migration that computes and populates ssn_hash for all existing staff and employment_form rows (using the same hashing logic as DataInitializer/service layer), then ALTER TABLE ... SET NOT NULL for ssn_hash and keep/create the unique indexes (idx_staff_ssn_hash, idx_form_ssn_hash), or document in the migration header that the DB must be clean and update Staff.java and EmploymentForm.java column mappings to nullable=false only after the backfill migration is applied.src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
118-125:⚠️ Potential issue | 🟡 MinorSame null-SSN risk as
StaffService.updateStaff.If
updatedForm.getSocialSecurityNumber()is null (partial update),existingSsnPlain.equals(newSsnPlain)returnsfalse, thenvalidateSsnNotExists(null)andencryptionService.encrypt(null)are invoked — neither tolerates null gracefully. Guard the SSN block on a non-null/non-blanknewSsnPlain(and consider whetherUpdateEmploymentDTOshould disallow null here at the validation layer).🛡️ Suggested guard
- 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)); - existingForm.setSsnHash(encryptionService.hmac(newSsnPlain)); - } + String newSsnPlain = updatedForm.getSocialSecurityNumber(); + if (newSsnPlain != null && !newSsnPlain.isBlank()) { + String existingSsnPlain = encryptionService.decrypt(existingForm.getSocialSecurityNumber()); + if (!Objects.equals(existingSsnPlain, newSsnPlain)) { + validateSsnNotExists(newSsnPlain); + existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)); + existingForm.setSsnHash(encryptionService.hmac(newSsnPlain)); + } + }🤖 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 118 - 125, The SSN update block in EmploymentFormService (where you call encryptionService.decrypt(existingForm.getSocialSecurityNumber()), updatedForm.getSocialSecurityNumber(), validateSsnNotExists(...), encryptionService.encrypt(...), and encryptionService.hmac(...)) must first guard that newSsnPlain is non-null and non-blank before comparing or passing it to validate/encrypt/hmac; change the if to check newSsnPlain != null && !newSsnPlain.isBlank() (or use a utility like StringUtils.hasText) and only then compare with existingSsnPlain and perform validateSsnNotExists(newSsnPlain), existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)), and existingForm.setSsnHash(encryptionService.hmac(newSsnPlain)).
🧹 Nitpick comments (5)
src/main/resources/static/css/styles.css (1)
218-273: LGTM — selectors line up with the structure produced bygetInitialSsnHtml/toggleSsnView/resetSsnViewinsrc/main/resources/static/js/employment.js. Purely additive styling.One minor nit: the
:hoverand.activestates for.btn-ssn-actionuse the sameborder-color:#94a3b8`` and very similar backgrounds (#f8fafcvs `#f1f5f9`), so the toggled-on state is visually almost indistinguishable from a hover. Consider a stronger affordance (e.g., a colored border or filled background) for the active/revealed state since it gates a sensitive disclosure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/styles.css` around lines 218 - 273, The active state for the SSN toggle is too similar to the hover state; update the CSS for .btn-ssn-action.active (and consider coordinating .btn-ssn-action:hover) to provide a stronger affordance such as a distinct filled background color (e.g., a subtle tint like `#e0f2fe`), a more pronounced border color (e.g., `#0284c7`), or increased box-shadow to clearly differentiate the revealed/active state from hover; change only the styles in the .btn-ssn-action.active rule so the active/revealed state is visually unambiguous compared to .btn-ssn-action:hover.src/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.java (1)
14-16: RemoveexistsBySocialSecurityNumberfrom bothStaffRepositoryandEmploymentFormRepositoryinterfaces — it is unused dead code.The method cannot serve a valid purpose after the move to blind-indexed lookups via
existsBySsnHash, since SSN values are encrypted at rest. No callers reference it anywhere in the codebase.🤖 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/repository/StaffRepository.java` around lines 14 - 16, Remove the dead method declaration existsBySocialSecurityNumber from the StaffRepository and EmploymentFormRepository interfaces: locate the interfaces (symbols StaffRepository and EmploymentFormRepository) and delete the existsBySocialSecurityNumber(String socialSecurityNumber) line, leaving the blind-indexed lookup method existsBySsnHash(String ssnHash) intact; after removal, run a compile and tests to ensure no references remain and clean up any now-unused imports or javadoc mentioning existsBySocialSecurityNumber.src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java (1)
14-15: Remove dead-code methodexistsBySocialSecurityNumber.The method has no callers in the codebase (only
existsBySsnHashis used throughout). Since SSN is encrypted at rest, this method cannot work correctly and should be removed.🤖 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/repository/EmploymentFormRepository.java` around lines 14 - 15, Remove the dead repository method existsBySocialSecurityNumber from the EmploymentFormRepository interface because SSNs are stored encrypted and this method is unused; keep existsBySsnHash (which is the correct lookup) and delete the existsBySocialSecurityNumber declaration and any related Javadoc or comments referencing it, ensuring no callers remain and compile imports/IDE warnings are cleaned up.src/main/java/org/example/cyberwatch/config/EncryptionConfig.java (1)
12-23: Optional: Clarify naming and documentation for security-sensitive code.Spring's
Encryptors.deluxdocuments its second argument as a "hex-encoded, random, site-global salt value" and internally uses AES-256-GCM with per-encryption random IVs. Functionally sound, but two minor clarity improvements worth considering:
Naming: The variable
pepperis semantically incorrect. A pepper is a separate, externally-stored secret applied in addition to a per-record salt. Here, it is the Spring "salt" parameter — the fixed, site-global value used with PBKDF2 to derive the key. In crypto code, misleading names tend to persist.Documentation: The Swedish comment conflates the fixed salt with the random IV. The salt is fixed; the IV is random per call.
Suggested rename + comment clarification
- `@Value`("${app.encryption.password}") - private String encryptionPass; - - `@Value`("${app.encryption.pepper}") - private String pepper; - - //Skapar en AES-256 krypteringsnyckel, pass + pepper = key, - //delux lägger till ett randomiserat salt-värde(Nonce/IV) vid varje anrop av encrypt + `@Value`("${app.encryption.password}") + private String encryptionPassword; + + // Hex-encoded, site-global salt used by PBKDF2 to derive the AES key. + `@Value`("${app.encryption.pepper}") + private String encryptionSalt; + + // delux() = AES-256-GCM. Key is derived once from (password, salt) via PBKDF2; + // a fresh random 16-byte IV is generated per encrypt() call. `@Bean` public TextEncryptor textEncryptor() { - return Encryptors.delux(encryptionPass, pepper); + return Encryptors.delux(encryptionPassword, encryptionSalt); }(The property key
app.encryption.peppercan stay if preferred — only the Java field and comment need updating.)🤖 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/EncryptionConfig.java` around lines 12 - 23, Rename and clarify the semantics for the site-global salt field and comment: change the field currently named "pepper" to a more accurate name (e.g., "siteSalt" or "keySalt") and update the JavaDoc/comment above textEncryptor() to state that the second argument to Encryptors.delux is a hex-encoded, site-global salt used for PBKDF2 key derivation while AES-256-GCM uses a random IV per encryption; keep the external property name (app.encryption.pepper) if desired but map it to the renamed field (encryptionPass and siteSalt referenced in textEncryptor() and Encryptors.delux).src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (1)
138-145: Dup-check inconsistent withEmploymentFormService.validateSsnNotExists.
EmploymentFormService.validateSsnNotExistschecks bothemploymentFormRepository.existsBySsnHashandstaffRepository.existsBySsnHash, but the version here only checksstaffRepository. As a result, updating a Staff SSN to a value that matches a PENDINGEmploymentFormwill succeed and later cause a conflict at approval time (or, in the worst case, duplicate the same hash across tables).♻️ Suggested fix to mirror cross-table uniqueness
private void validateSsnNotExists(String ssn) { String ssnHash = encryptionService.hmac(ssn); + if (employmentFormRepository.existsBySsnHash(ssnHash)) { + throw new IllegalStateException("An application with this SSN already exists."); + } + // Kontrollera om SSN redan finns i Staff if (staffRepository.existsBySsnHash(ssnHash)) { throw new IllegalStateException("An employee with this SSN already exists."); } }(requires injecting
EmploymentFormRepositoryintoStaffService.)🤖 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 138 - 145, The StaffService.validateSsnNotExists currently only checks staffRepository.existsBySsnHash; mirror EmploymentFormService.validateSsnNotExists by also checking employmentFormRepository.existsBySsnHash to avoid cross-table duplicates: inject EmploymentFormRepository into StaffService, compute ssnHash via encryptionService.hmac(ssn) as before, and throw the same IllegalStateException if either staffRepository.existsBySsnHash(ssnHash) or employmentFormRepository.existsBySsnHash(ssnHash) returns true so pending EmploymentForm records block Staff SSN reuse.
🤖 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/DataInitializer.java`:
- Around line 63-69: The comment and variable naming around hrStaff =
admins.get(0) is misleading because the seeded admin (Eric) is Role.ADMIN,
Department.BACKEND; update the comment/variable to reflect that (e.g., rename
hrStaff to adminStaff or change the comment to "Eric (Admin/Backend)") or
alternatively change the seeded admin's role/department to HR if you intended an
HR user; also remove the redundant manual timestamp assignment
(form.setCreatedDate(LocalDateTime.now())) since EmploymentForm.createdDate is
managed by `@CreationTimestamp` so let the JPA provider populate it when
persisting via employmentFormRepository.saveAll and createEmploymentForm.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 249-261: applySsnPolicy currently decrypts per-row then masks in
an else branch that is never reached because callers getFormsByFilterApproval
and getFormById are annotated
`@PreAuthorize`("hasAnyRole('HR','CEO','CTO','ADMIN')"); either make the masking
reachable by widening those `@PreAuthorize` annotations to include the roles that
should receive masked SSNs (e.g., CONSULTANT, EMPLOYEE) so applySsnPolicy's else
branch executes, or remove the else branch and avoid unnecessary decrypts by
always setting the decrypted value in applySsnPolicy; update the annotations on
getFormsByFilterApproval and getFormById (and any other callers like
create/update flows if needed) when choosing the first option and keep changes
confined to applySsnPolicy and the authorization annotations.
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 26-27: The ssnHash field is currently nullable which allows
multiple NULLs to bypass the UNIQUE constraint; update the JPA entities
(Staff.ss nHash and EmploymentForm.ss nHash) to add nullable = false on the
`@Column` and annotate the fields with `@NotBlank` to match socialSecurityNumber and
enforce the blind-index invariant, ensure any service-layer code still
backfills/populates ssnHash before persist, and update the migration
V12__add_ssn_hash.sql to alter/add the column with NOT NULL (only after
backfilling existing rows) so the DB constraint matches the entity.
In
`@src/main/java/org/example/cyberwatch/features/staff/model/UpdateStaffDTO.java`:
- Around line 25-27: The current `@Pattern` on UpdateStaffDTO.socialSecurityNumber
only enforces digit counts but not valid date ranges; update the validation to
either use a tighter regex (e.g. replace the Pattern with one that enforces
year/month/day ranges like
^(19|20)\\d{2}(0[1-9]|1[0-2])(0[1-9]|[12]\\d|3[01])-\\d{4}$) and change the
message to "Social security number must match format YYYYMMDD-NNNN with valid
month/day", or implement a custom ConstraintValidator used on the
socialSecurityNumber field that parses YYYYMMDD and validates the date portion
(keeping StaffService.updateStaff() behavior unchanged). Refer to
UpdateStaffDTO.socialSecurityNumber and, if adding a custom validator, the new
annotation and validator class names you create.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 74-84: Add a null-safety check before calling
encryptionService.decrypt on existingStaff.getSocialSecurityNumber() in
StaffService so a null SSN does not cause an NPE: read
existingStaff.getSocialSecurityNumber() into a variable, if it's non-null call
encryptionService.decrypt(...) else set existingSsnPlain to null (or use
Objects.equals when comparing); then use Objects.equals(existingSsnPlain,
dto.getSocialSecurityNumber()) for the comparison, and keep the existing calls
to validateSsnNotExists(...), existingStaff.setSocialSecurityNumber(...),
existingStaff.setSsnHash(...), and staffMapper.updateEntity(...) unchanged.
In `@src/main/resources/static/js/employment.js`:
- Around line 150-193: toggleSsnView currently starts an auto-mask via
setTimeout but never stores the timer id, causing stale timers to re-mask SSNs
after a manual hide/re-show; modify toggleSsnView to save the timeout handle per
card (e.g., in a module-level Map keyed by id or by element) and ensure
resetSsnView clears any existing timeout for that card before restoring the
initial HTML; also ensure toggleSsnView clears any prior timeout before setting
a new one so repeated shows don't leave old timers active, and keep references
to the keys using the same id/type logic used for element lookup (ssn/form-ssn)
so getInitialSsnHtml can be called safely.
In `@src/test/resources/application-test.properties`:
- Around line 1-3: The test properties file uses mixed Spring property and
env-placeholder names so Spring fails to resolve placeholders in
EncryptionConfig; update src/test/resources/application-test.properties to
define the environment-style keys that match the placeholders used by
EncryptionConfig and application.properties—replace app.encryption.hmac-key with
APP_ENCRYPTION_HMAC_KEY and ensure ENCRYPTION_PASSWORD and ENCRYPTION_PEPPER
remain as uppercase env-style keys (matching ${ENCRYPTION_PASSWORD},
${ENCRYPTION_PEPPER}, ${APP_ENCRYPTION_HMAC_KEY}) so the test values are picked
up at runtime.
---
Outside diff comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 118-125: The SSN update block in EmploymentFormService (where you
call encryptionService.decrypt(existingForm.getSocialSecurityNumber()),
updatedForm.getSocialSecurityNumber(), validateSsnNotExists(...),
encryptionService.encrypt(...), and encryptionService.hmac(...)) must first
guard that newSsnPlain is non-null and non-blank before comparing or passing it
to validate/encrypt/hmac; change the if to check newSsnPlain != null &&
!newSsnPlain.isBlank() (or use a utility like StringUtils.hasText) and only then
compare with existingSsnPlain and perform validateSsnNotExists(newSsnPlain),
existingForm.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)),
and existingForm.setSsnHash(encryptionService.hmac(newSsnPlain)).
In `@src/main/resources/db/migration/V12__add_ssn_hash.sql`:
- Around line 1-13: The migration V12__add_ssn_hash.sql adds nullable ssn_hash
and unique indexes but doesn't backfill existing rows, which allows multiple
NULLs to bypass uniqueness and the existsBySsnHash checks used by
StaffService.validateSsnNotExists and
EmploymentFormService.validateSsnNotExists; either add a follow-up migration
that computes and populates ssn_hash for all existing staff and employment_form
rows (using the same hashing logic as DataInitializer/service layer), then ALTER
TABLE ... SET NOT NULL for ssn_hash and keep/create the unique indexes
(idx_staff_ssn_hash, idx_form_ssn_hash), or document in the migration header
that the DB must be clean and update Staff.java and EmploymentForm.java column
mappings to nullable=false only after the backfill migration is applied.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/config/EncryptionConfig.java`:
- Around line 12-23: Rename and clarify the semantics for the site-global salt
field and comment: change the field currently named "pepper" to a more accurate
name (e.g., "siteSalt" or "keySalt") and update the JavaDoc/comment above
textEncryptor() to state that the second argument to Encryptors.delux is a
hex-encoded, site-global salt used for PBKDF2 key derivation while AES-256-GCM
uses a random IV per encryption; keep the external property name
(app.encryption.pepper) if desired but map it to the renamed field
(encryptionPass and siteSalt referenced in textEncryptor() and
Encryptors.delux).
In
`@src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java`:
- Around line 14-15: Remove the dead repository method
existsBySocialSecurityNumber from the EmploymentFormRepository interface because
SSNs are stored encrypted and this method is unused; keep existsBySsnHash (which
is the correct lookup) and delete the existsBySocialSecurityNumber declaration
and any related Javadoc or comments referencing it, ensuring no callers remain
and compile imports/IDE warnings are cleaned up.
In
`@src/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.java`:
- Around line 14-16: Remove the dead method declaration
existsBySocialSecurityNumber from the StaffRepository and
EmploymentFormRepository interfaces: locate the interfaces (symbols
StaffRepository and EmploymentFormRepository) and delete the
existsBySocialSecurityNumber(String socialSecurityNumber) line, leaving the
blind-indexed lookup method existsBySsnHash(String ssnHash) intact; after
removal, run a compile and tests to ensure no references remain and clean up any
now-unused imports or javadoc mentioning existsBySocialSecurityNumber.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 138-145: The StaffService.validateSsnNotExists currently only
checks staffRepository.existsBySsnHash; mirror
EmploymentFormService.validateSsnNotExists by also checking
employmentFormRepository.existsBySsnHash to avoid cross-table duplicates: inject
EmploymentFormRepository into StaffService, compute ssnHash via
encryptionService.hmac(ssn) as before, and throw the same IllegalStateException
if either staffRepository.existsBySsnHash(ssnHash) or
employmentFormRepository.existsBySsnHash(ssnHash) returns true so pending
EmploymentForm records block Staff SSN reuse.
In `@src/main/resources/static/css/styles.css`:
- Around line 218-273: The active state for the SSN toggle is too similar to the
hover state; update the CSS for .btn-ssn-action.active (and consider
coordinating .btn-ssn-action:hover) to provide a stronger affordance such as a
distinct filled background color (e.g., a subtle tint like `#e0f2fe`), a more
pronounced border color (e.g., `#0284c7`), or increased box-shadow to clearly
differentiate the revealed/active state from hover; change only the styles in
the .btn-ssn-action.active rule so the active/revealed state is visually
unambiguous compared to .btn-ssn-action:hover.
🪄 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: 23598140-2705-4137-a112-ad2a6626954b
📒 Files selected for processing (21)
pom.xmlsrc/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/mapper/EmploymentMapper.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.javasrc/main/java/org/example/cyberwatch/features/staff/model/UpdateStaffDTO.javasrc/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/main/resources/application.propertiessrc/main/resources/db/migration/V12__add_ssn_hash.sqlsrc/main/resources/static/css/styles.csssrc/main/resources/static/js/employment.jssrc/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
Comment on lines
+74
to
84
| // Hantera SSN separat för kryptering och hashning | ||
| String existingSsnPlain = encryptionService.decrypt(existingStaff.getSocialSecurityNumber()); | ||
| String newSsnPlain = dto.getSocialSecurityNumber(); | ||
|
|
||
| if (!existingSsnPlain.equals(newSsnPlain)) { | ||
| validateSsnNotExists(newSsnPlain); | ||
| existingStaff.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain)); | ||
| existingStaff.setSsnHash(encryptionService.hmac(newSsnPlain)); | ||
| } | ||
|
|
||
| staffMapper.updateEntity(dto, existingStaff); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that StaffMapper.updateEntity does not overwrite socialSecurityNumber/ssnHash from the DTO,
# which would clobber the values just set in StaffService.updateStaff.
fd -i 'StaffMapper.java' --exec cat {}
echo '---'
# Also check whether UpdateStaffDTO carries a socialSecurityNumber field.
fd -i 'UpdateStaffDTO.java' --exec cat {}Repository: ithsjava25/project-backend-super-cool-team
Length of output: 2837
Add null-safety check before decrypting existing SSN.
If existingStaff.getSocialSecurityNumber() is unexpectedly null (corrupted state), the decrypt() call on line 75 will NPE. Add a defensive null check:
String existingSsnPlain = existingStaff.getSocialSecurityNumber() != null
? encryptionService.decrypt(existingStaff.getSocialSecurityNumber())
: null;
String newSsnPlain = dto.getSocialSecurityNumber();
if (!Objects.equals(existingSsnPlain, newSsnPlain)) {
validateSsnNotExists(newSsnPlain);
existingStaff.setSocialSecurityNumber(encryptionService.encrypt(newSsnPlain));
existingStaff.setSsnHash(encryptionService.hmac(newSsnPlain));
}Note: StaffMapper.updateEntity() explicitly excludes socialSecurityNumber from its field copying, so no clobbering risk 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/staff/service/StaffService.java`
around lines 74 - 84, Add a null-safety check before calling
encryptionService.decrypt on existingStaff.getSocialSecurityNumber() in
StaffService so a null SSN does not cause an NPE: read
existingStaff.getSocialSecurityNumber() into a variable, if it's non-null call
encryptionService.decrypt(...) else set existingSsnPlain to null (or use
Objects.equals when comparing); then use Objects.equals(existingSsnPlain,
dto.getSocialSecurityNumber()) for the comparison, and keep the existing calls
to validateSsnNotExists(...), existingStaff.setSocialSecurityNumber(...),
existingStaff.setSsnHash(...), and staffMapper.updateEntity(...) unchanged.
Comment on lines
+150
to
+193
| async function toggleSsnView(id, type) { | ||
| // Bestäm prefix och endpoint baserat på typen | ||
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | ||
| const endpoint = (type === 'staff') ? 'staff' : 'forms'; | ||
|
|
||
| const container = document.getElementById(`${prefix}-container-${id}`); | ||
| if (!container) return; | ||
|
|
||
| container.innerHTML = "<em>Laddar...</em>"; | ||
|
|
||
| try { | ||
| const res = await apiFetch(`/${endpoint}/${id}`); | ||
| if (!res.ok) throw new Error("Behörighet saknas"); | ||
|
|
||
| const data = await res.json(); | ||
|
|
||
| container.innerHTML = ` | ||
| <div class="ssn-display-row"> | ||
| <span class="ssn-label">SSN:</span> | ||
| <div class="ssn-wrapper"> | ||
| <strong class="ssn-value">${escapeHtml(data.socialSecurityNumber)}</strong> | ||
| <button class="btn-ssn-action active" onclick="resetSsnView(${id}, '${type}')"> | ||
| DÖLJ | ||
| </button> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| // Maskera automatiskt efter 10 sekunder | ||
| setTimeout(() => resetSsnView(id, type), 10000); | ||
|
|
||
| } catch (e) { | ||
| container.innerHTML = "<span class='text-danger'>Kunde inte hämta data.</span>"; | ||
| console.error(e); | ||
| } | ||
| } | ||
|
|
||
| function resetSsnView(id, type) { | ||
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | ||
| const container = document.getElementById(`${prefix}-container-${id}`); | ||
| if (container) { | ||
| container.innerHTML = getInitialSsnHtml(id, type); | ||
| } | ||
| } |
There was a problem hiding this comment.
Track and clear the auto-mask timer to avoid premature reset on re-toggle.
setTimeout(...) on Line 179 is fired but its handle is never stored. If the user manually clicks DÖLJ within 10 s and then clicks VISA again on the same card before the original 10 s elapses, the first timer still fires and prematurely re-masks the freshly-revealed SSN. resetSsnView should also cancel any pending timer for that card.
🛠️ Suggested change
+const ssnTimers = new Map(); // key: `${type}-${id}`, value: timeoutId
+
async function toggleSsnView(id, type) {
// Bestäm prefix och endpoint baserat på typen
const prefix = (type === 'staff') ? 'ssn' : 'form-ssn';
const endpoint = (type === 'staff') ? 'staff' : 'forms';
+ const timerKey = `${type}-${id}`;
+
+ // Cancel any pending auto-mask for this card
+ if (ssnTimers.has(timerKey)) {
+ clearTimeout(ssnTimers.get(timerKey));
+ ssnTimers.delete(timerKey);
+ }
@@
- // Maskera automatiskt efter 10 sekunder
- setTimeout(() => resetSsnView(id, type), 10000);
+ // Maskera automatiskt efter 10 sekunder
+ ssnTimers.set(timerKey, setTimeout(() => {
+ ssnTimers.delete(timerKey);
+ resetSsnView(id, type);
+ }, 10000));
@@
function resetSsnView(id, type) {
const prefix = (type === 'staff') ? 'ssn' : 'form-ssn';
+ const timerKey = `${type}-${id}`;
+ if (ssnTimers.has(timerKey)) {
+ clearTimeout(ssnTimers.get(timerKey));
+ ssnTimers.delete(timerKey);
+ }
const container = document.getElementById(`${prefix}-container-${id}`);📝 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
| async function toggleSsnView(id, type) { | |
| // Bestäm prefix och endpoint baserat på typen | |
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | |
| const endpoint = (type === 'staff') ? 'staff' : 'forms'; | |
| const container = document.getElementById(`${prefix}-container-${id}`); | |
| if (!container) return; | |
| container.innerHTML = "<em>Laddar...</em>"; | |
| try { | |
| const res = await apiFetch(`/${endpoint}/${id}`); | |
| if (!res.ok) throw new Error("Behörighet saknas"); | |
| const data = await res.json(); | |
| container.innerHTML = ` | |
| <div class="ssn-display-row"> | |
| <span class="ssn-label">SSN:</span> | |
| <div class="ssn-wrapper"> | |
| <strong class="ssn-value">${escapeHtml(data.socialSecurityNumber)}</strong> | |
| <button class="btn-ssn-action active" onclick="resetSsnView(${id}, '${type}')"> | |
| DÖLJ | |
| </button> | |
| </div> | |
| </div> | |
| `; | |
| // Maskera automatiskt efter 10 sekunder | |
| setTimeout(() => resetSsnView(id, type), 10000); | |
| } catch (e) { | |
| container.innerHTML = "<span class='text-danger'>Kunde inte hämta data.</span>"; | |
| console.error(e); | |
| } | |
| } | |
| function resetSsnView(id, type) { | |
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | |
| const container = document.getElementById(`${prefix}-container-${id}`); | |
| if (container) { | |
| container.innerHTML = getInitialSsnHtml(id, type); | |
| } | |
| } | |
| const ssnTimers = new Map(); // key: `${type}-${id}`, value: timeoutId | |
| async function toggleSsnView(id, type) { | |
| // Bestäm prefix och endpoint baserat på typen | |
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | |
| const endpoint = (type === 'staff') ? 'staff' : 'forms'; | |
| const timerKey = `${type}-${id}`; | |
| // Cancel any pending auto-mask for this card | |
| if (ssnTimers.has(timerKey)) { | |
| clearTimeout(ssnTimers.get(timerKey)); | |
| ssnTimers.delete(timerKey); | |
| } | |
| const container = document.getElementById(`${prefix}-container-${id}`); | |
| if (!container) return; | |
| container.innerHTML = "<em>Laddar...</em>"; | |
| try { | |
| const res = await apiFetch(`/${endpoint}/${id}`); | |
| if (!res.ok) throw new Error("Behörighet saknas"); | |
| const data = await res.json(); | |
| container.innerHTML = ` | |
| <div class="ssn-display-row"> | |
| <span class="ssn-label">SSN:</span> | |
| <div class="ssn-wrapper"> | |
| <strong class="ssn-value">${escapeHtml(data.socialSecurityNumber)}</strong> | |
| <button class="btn-ssn-action active" onclick="resetSsnView(${id}, '${type}')"> | |
| DÖLJ | |
| </button> | |
| </div> | |
| </div> | |
| `; | |
| // Maskera automatiskt efter 10 sekunder | |
| ssnTimers.set(timerKey, setTimeout(() => { | |
| ssnTimers.delete(timerKey); | |
| resetSsnView(id, type); | |
| }, 10000)); | |
| } catch (e) { | |
| container.innerHTML = "<span class='text-danger'>Kunde inte hämta data.</span>"; | |
| console.error(e); | |
| } | |
| } | |
| function resetSsnView(id, type) { | |
| const prefix = (type === 'staff') ? 'ssn' : 'form-ssn'; | |
| const timerKey = `${type}-${id}`; | |
| if (ssnTimers.has(timerKey)) { | |
| clearTimeout(ssnTimers.get(timerKey)); | |
| ssnTimers.delete(timerKey); | |
| } | |
| const container = document.getElementById(`${prefix}-container-${id}`); | |
| if (container) { | |
| container.innerHTML = getInitialSsnHtml(id, type); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/js/employment.js` around lines 150 - 193,
toggleSsnView currently starts an auto-mask via setTimeout but never stores the
timer id, causing stale timers to re-mask SSNs after a manual hide/re-show;
modify toggleSsnView to save the timeout handle per card (e.g., in a
module-level Map keyed by id or by element) and ensure resetSsnView clears any
existing timeout for that card before restoring the initial HTML; also ensure
toggleSsnView clears any prior timeout before setting a new one so repeated
shows don't leave old timers active, and keep references to the keys using the
same id/type logic used for element lookup (ssn/form-ssn) so getInitialSsnHtml
can be called safely.
…tion, add non-null constraints for `ssn_hash`, remove role-based SSN masking policy, and streamline encryption handling; update database seeding with ticket creation.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
249-254:⚠️ Potential issue | 🟡 Minor
applySsnPolicyno longer applies any policy — name is misleading and PR objective is not delivered.The method now unconditionally decrypts, so there's no "policy" — it's just a decrypting DTO mapper. Two concerns:
- The PR description states "ADMIN, HR, CEO, and CTO see full SSNs; other roles receive masked values." As implemented, masking is unreachable: every caller of
applySsnPolicyis already@PreAuthorize("hasAnyRole('HR','CEO','CTO','ADMIN')")(lines 47, 71, 88, 95), and the body has no role check. So "other roles receive masked values" is not delivered — they're simply denied access.- The name
applySsnPolicyadvertises behavior that doesn't exist. Either:
- Widen
@PreAuthorizeongetFormsByFilterApproval/getFormByIdto include the roles that should see masked values, and reintroduce a role-based branch insideapplySsnPolicy, or- Rename to
toDtoWithDecryptedSsn(or similar) to honestly describe the behavior.🤖 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 249 - 254, applySsnPolicy currently always decrypts and the name is misleading; change behavior so callers that should see masked SSNs can call this method and it will return either a decrypted SSN for privileged roles or a masked SSN for others: widen the `@PreAuthorize` on getFormsByFilterApproval/getFormById to include the additional roles that must be able to view masked results (so they can call these endpoints), then update applySsnPolicy to check the current principal roles (via SecurityContextHolder or an injected AuthenticationFacade) and branch: if user hasAnyRole("HR","CEO","CTO","ADMIN") set dto.setSocialSecurityNumber(decryption), else set dto.setSocialSecurityNumber(maskingFunction(decryptedOrEncryptedValue)); keep the EmploymentFormDTO mapping via employmentMapper.toDTO(form) as before and ensure maskingFunction preserves format and redacts appropriately; alternatively, if you intend only to ever decrypt here, rename applySsnPolicy to toDtoWithDecryptedSsn and leave authorization as-is.
🧹 Nitpick comments (2)
src/main/java/org/example/cyberwatch/features/form/dto/CreateEmploymentDTO.java (1)
21-22: Tightened regex looks good; consider extracting the shared SSN pattern.The same regex now appears in
UpdateStaffDTO(line 25). To avoid drift, consider either a shared constant or a custom@ValidSwedishSsnannotation backed by aConstraintValidator(the latter would also let you reject impossible day-of-month combinations like20240230-xxxx, which the current regex still accepts).🤖 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/dto/CreateEmploymentDTO.java` around lines 21 - 22, The SSN regex used in CreateEmploymentDTO (the `@Pattern` on the field) is duplicated in UpdateStaffDTO; extract it into a shared constant (e.g., a public static String SWEDISH_SSN_REGEX) and reference that constant from both DTOs, or better: create a custom constraint annotation (e.g., `@ValidSwedishSsn`) with an accompanying ConstraintValidator implementation that centralizes validation logic and can enforce calendar-valid dates (rejecting impossible dates like 20240230); update CreateEmploymentDTO and UpdateStaffDTO to use the shared constant or the new `@ValidSwedishSsn` annotation instead of repeating the regex.src/main/java/org/example/cyberwatch/config/DataInitializer.java (1)
72-85: Brittle ticket-to-staff assignment via positional indices onfindAll().
staffRepository.findAll()does not guarantee any ordering (JPA leaves it implementation-defined; in Hibernate it's typically PK order but not contractual). HardcodingallStaff.get(4)..allStaff.get(10)to mean "the first dummy", "the second dummy", etc. silently couples the ticket fixtures to insertion order and to the exact count/order of precedingsaveAllcalls. A future re-order or extra seeded admin will reassign every ticket to the wrong person without any test failure.Suggest capturing the second
saveAllresult (similar toadmins) and referencing staff by name/role instead of by index.♻️ Proposed refactor
- staffRepository.saveAll(List.of( + List<Staff> dummies = staffRepository.saveAll(List.of( createStaff("19850212-1212", "Johan", "Andersson", ...), ... createStaff("19960820-1122", "Lucas", "Karlsson", ...) )); @@ - // Skapa 10 tickets - List<Staff> allStaff = staffRepository.findAll(); + // Skapa 10 tickets — index by named handles, not by position from findAll() + Staff johan = dummies.get(0); + Staff sofia = dummies.get(1); + Staff marcus = dummies.get(2); + Staff linnea = dummies.get(3); + Staff niklas = dummies.get(4); + Staff elena = dummies.get(5); + Staff lucas = dummies.get(8); ticketRepository.saveAll(List.of( - createTicket("TICKET-1001", ..., allStaff.get(4), List.of(allStaff.get(7))), + createTicket("TICKET-1001", ..., johan, List.of(linnea)), ... ));🤖 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 72 - 85, The ticket seed is brittle because it uses positional indices from staffRepository.findAll() (allStaff.get(...)) which is order-undefined; instead capture the saved staff list from the staff save call (like the savedAdmins pattern) and look up staff by a stable attribute (e.g., username, email, or role) when building tickets; update the DataInitializer to store the return of staffRepository.saveAll(...) into a variable (e.g., savedStaff), implement a small lookup helper or use streams to findByUsername/role for each ticket owner/assignee, and pass those Staff instances into createTicket(...) before calling ticketRepository.saveAll(...)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 249-254: applySsnPolicy currently always decrypts and the name is
misleading; change behavior so callers that should see masked SSNs can call this
method and it will return either a decrypted SSN for privileged roles or a
masked SSN for others: widen the `@PreAuthorize` on
getFormsByFilterApproval/getFormById to include the additional roles that must
be able to view masked results (so they can call these endpoints), then update
applySsnPolicy to check the current principal roles (via SecurityContextHolder
or an injected AuthenticationFacade) and branch: if user
hasAnyRole("HR","CEO","CTO","ADMIN") set
dto.setSocialSecurityNumber(decryption), else set
dto.setSocialSecurityNumber(maskingFunction(decryptedOrEncryptedValue)); keep
the EmploymentFormDTO mapping via employmentMapper.toDTO(form) as before and
ensure maskingFunction preserves format and redacts appropriately;
alternatively, if you intend only to ever decrypt here, rename applySsnPolicy to
toDtoWithDecryptedSsn and leave authorization as-is.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/config/DataInitializer.java`:
- Around line 72-85: The ticket seed is brittle because it uses positional
indices from staffRepository.findAll() (allStaff.get(...)) which is
order-undefined; instead capture the saved staff list from the staff save call
(like the savedAdmins pattern) and look up staff by a stable attribute (e.g.,
username, email, or role) when building tickets; update the DataInitializer to
store the return of staffRepository.saveAll(...) into a variable (e.g.,
savedStaff), implement a small lookup helper or use streams to
findByUsername/role for each ticket owner/assignee, and pass those Staff
instances into createTicket(...) before calling ticketRepository.saveAll(...)
In
`@src/main/java/org/example/cyberwatch/features/form/dto/CreateEmploymentDTO.java`:
- Around line 21-22: The SSN regex used in CreateEmploymentDTO (the `@Pattern` on
the field) is duplicated in UpdateStaffDTO; extract it into a shared constant
(e.g., a public static String SWEDISH_SSN_REGEX) and reference that constant
from both DTOs, or better: create a custom constraint annotation (e.g.,
`@ValidSwedishSsn`) with an accompanying ConstraintValidator implementation that
centralizes validation logic and can enforce calendar-valid dates (rejecting
impossible dates like 20240230); update CreateEmploymentDTO and UpdateStaffDTO
to use the shared constant or the new `@ValidSwedishSsn` annotation instead of
repeating the regex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6106ffd-c123-49a6-8383-8fd4f4f0f582
📒 Files selected for processing (8)
src/main/java/org/example/cyberwatch/config/DataInitializer.javasrc/main/java/org/example/cyberwatch/features/form/dto/CreateEmploymentDTO.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/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/UpdateStaffDTO.javasrc/main/resources/db/migration/V12__add_ssn_hash.sqlsrc/test/resources/application-test.properties
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
- src/main/resources/db/migration/V12__add_ssn_hash.sql
- src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java
…eration, clarify encryption and decryption processes, and add unique index for SSN hash in database
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/cyberwatch/config/security/EncryptionService.java`:
- Around line 15-40: The hmac(String data) method uses hmacKey without
validation; add a guard that fails fast when hmacKey is null or blank/empty
(e.g., check StringUtils.hasText(hmacKey) or hmacKey == null ||
hmacKey.trim().isEmpty()) and throw an IllegalStateException (or similar runtime
exception) with a clear message like "HMAC key is missing or empty" before
computing the HMAC with new HmacUtils(HmacAlgorithms.HMAC_SHA_256, hmacKey).
Ensure the check is performed in hmac() (or in class initialization/constructor)
so no HMAC is computed with an invalid key.
🪄 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: f9bb4dfd-8587-4c6f-9118-b4d643500419
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/config/EncryptionConfig.javasrc/main/java/org/example/cyberwatch/config/security/EncryptionService.javasrc/main/resources/db/migration/V12__add_ssn_hash.sql
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V12__add_ssn_hash.sql
…ion and HMAC keys to ensure strong security at startup
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 personal data, particularly social security numbers (SSNs), by adding deterministic hashing (blind indexing) for secure lookups, updating encryption configuration, and refining access control for viewing SSNs. It also updates the data initialization logic and enhances the employment form creation and retrieval processes for better security and usability.
Security and Encryption Improvements:
ssnHash) for SSNs in bothEmploymentFormandStaff, enabling secure, efficient lookups and duplicate checks without decrypting sensitive data. The repository and service layers now usessnHashfor existence checks, replacing previous approaches that required decrypting all records. [1] [2] [3] [4] [5]Encryptors.deluxfor improved cryptographic practices.Access Control and Data Exposure:
Data Initialization and Test Data:
DataInitializerto populate the database with a richer set of staff and employment form test data, including proper encryption and hashing of SSNs. Also, now creates three pending employment forms linked to an admin/HR staff member for testing. [1] [2]Codebase and Mapping Adjustments:
EmploymentMapperto clarify that encryption and hashing of SSNs are handled in the service layer, not in the mapper itself. Also, ensures that the SSN and its hash are correctly copied when converting an approved form to a staff entity. [1] [2] [3]Other Notable Changes:
These changes collectively improve the security, maintainability, and clarity of how sensitive personal data is handled throughout the application.
Summary by CodeRabbit
New Features
UI/UX Improvements
Data
Security
Database Migration
Tests