-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add middleware class #27
Conversation
Looked just at the schema - looks good to me! Two questions:
|
I assumed that the outputs would remain the same but now that you mention it there could be a scenario where a script modifies a file in place, which means the input path might appear in the output. I'll check for that as well thanks. I was thinking of making |
Reviewer's Guide by SourceryThis pull request introduces a new File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @athith-g - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using Path objects consistently throughout the class for more robust path handling, especially in methods like _change_executor_paths and _change_output_paths.
- The middleware currently uses hardcoded values for the image name and output directory. Consider making these configurable through environment variables or constructor parameters for increased flexibility.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me and I think we can merge.
However, one comment for consideration for a possible change in the future: Could VOLUME_PATH
be set randomly (e.g., via the tempfile
module) to something less guessable/obvious? Someone who gains access to the filesystem attached to the TES service might have a slightly harder finding interesting data if decrypted data isn't always written to the same place.
@uniqueg Yeah that's a good idea I can do that in this pull request. |
I created an outline of what the middleware class might look like. Are there any problems/is there anything I should keep in mind before hashing the details out? @jvkersch @uniqueg
Summary by Sourcery
Add a CryptMiddleware class to manage Crypt4GH file inputs, including setting original input paths, checking volumes, modifying executor and output paths, and adding a decryption executor to the request.
New Features: