UFAL/Run file preview as authenticated user #936
Conversation
|
""" WalkthroughThe changes introduce mandatory user authentication to the FilePreview script in DSpace. Two new command-line options, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FilePreview Script
participant DSpace Context
participant EPerson (User Record)
User->>FilePreview Script: Run script with -e (email) and -p (password)
FilePreview Script->>DSpace Context: Initialize context
FilePreview Script->>EPerson (User Record): Lookup by email
alt EPerson found and password correct
FilePreview Script->>DSpace Context: Set authenticated user
FilePreview Script->>FilePreview Script: Generate file previews
FilePreview Script->>DSpace Context: Commit and complete context
else Authentication fails
FilePreview Script->>User: Log error and abort
FilePreview Script->>DSpace Context: Abort context
end
Possibly related issues
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)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java (4)
114-114: Fix incorrect comment.The comment indicates that there should be no errors or warnings, but the test actually expects one error message about the missing password.
- // There should be no errors or warnings + // There should be exactly one error message about missing password
103-103: Use assertion message for better failure diagnostics.Add a descriptive message to the assertion to provide better context when the test fails.
- assertEquals(testDSpaceRunnableHandler.getErrorMessages().size(), 1); + assertEquals("Should have exactly one error message", 1, testDSpaceRunnableHandler.getErrorMessages().size());
104-104: Use assertion message for better failure diagnostics.Add a descriptive message to the assertion to provide better context when the test fails.
- assertEquals(testDSpaceRunnableHandler.getErrorMessages().get(0), "Email is required for authentication."); + assertEquals("Wrong error message", "Email is required for authentication.", testDSpaceRunnableHandler.getErrorMessages().get(0));
115-116: Use assertion message for better failure diagnostics.Add descriptive messages to the assertions to provide better context when the test fails.
- assertEquals(testDSpaceRunnableHandler.getErrorMessages().size(), 1); - assertEquals(testDSpaceRunnableHandler.getErrorMessages().get(0), "Password is required for authentication."); + assertEquals("Should have exactly one error message", 1, testDSpaceRunnableHandler.getErrorMessages().size()); + assertEquals("Wrong error message", "Password is required for authentication.", testDSpaceRunnableHandler.getErrorMessages().get(0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java(7 hunks)dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
⏰ 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
🔇 Additional comments (5)
dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java (5)
95-105: Good test for verifying email requirement.This test effectively verifies that the script requires an email for authentication, which aligns with the PR objective of removing anonymous context for file preview generation.
107-117: Good test for verifying password requirement.This test effectively verifies that the script requires a password when an email is provided, ensuring proper authentication.
123-124: Good update to provide authentication credentials.The test has been properly updated to include the new required authentication parameters.
141-141: Good update to provide authentication credentials.The test has been properly updated to include the new required authentication parameters.
85-93:Details
❌ Incorrect review comment
Consider updating testWhenNoFilesRun to include authentication.
This test doesn't include the new authentication parameters, which would likely cause it to fail with the current implementation. Consider updating it to include the email and password parameters.
🏁 Script executed:
#!/bin/bash # Verify if the FilePreview script implementation requires authentication even when no specific operation is performed # Check if the FilePreview class has authentication checks that would affect this test rg -A 5 "email.*required|password.*required" dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.javaLength of output: 119
Final Verification Outcome: Authentication not required
Our review of the FilePreview implementation via the grep search indicates that there are no authentication checks (or email/password requirements) in the current version of
FilePreview.java. Consequently, the integration testtestWhenNoFilesRunis valid as-is, and updating it to include email and password parameters isn’t necessary at this time.Likely an incorrect or invalid review comment.
dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java
Outdated
Show resolved
Hide resolved
…g, improved errors check for unauthorized access
dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java
Outdated
Show resolved
Hide resolved
milanmajchrak
changed the title
* Enhanced file preview feature - Run file preview as authenticated user (#936), Property to allow composing file previews (#935), Do not create temp file during generating preview (#921) * Use the handle url instead of the items url with the UUID (#938) * Items file metadata not correctly updated (#940) * Remove duplicate dependency element with identical content (warnings in build) * Anonymous users should be able to use shortener * Fixed internal server error, when context.commit() was called multiple times in search request * Catch DSpaceBadRequestException rather than BadRequestException * Fixed security issue for downloading file with non anonymous license * Fixed Clarinuserregistration nullpoint exception (#941)
pulling changes from latest (lindat-2025.04.14600125143) dataquest release This contains the following changes: - UFAL/Run file preview as authenticated user (dataquest-dev#936) - Property to allow composing file previews (dataquest-dev#935) - File preview is not loaded properly for big zip file (dataquest-dev#921) - UFAL/Clarinuserregistration nullpoint exception (dataquest-dev#941) - UFAL/Use the handle url instead of the items url with the UUID (dataquest-dev#938) - UFAL/Items file metadata not correctly updated (dataquest-dev#940) - Synchronize ufal and dataquest main branches (dataquest-dev#939)
Problem description
When the file preview is generated via the command line, an anonymous context is used. We want to authenticate an admin user before running the script.
Summary by CodeRabbit