-
Notifications
You must be signed in to change notification settings - Fork 4
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
Recover from virus detected during Transfer process #718
Conversation
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.
Almost all of this looks fine - I've confirmed that the tests pass, the email gets sent as expected, and the feedback to the user is shown when appropriate.
I see that coverage goes down, but I'm not sure what the right testing strategy would be here for covering the rescue clause in the transfer controller.
The only thing I think is needed is a comma in app.json
after the new entry - I'm not sure what leaving it off will do, so if this is optional somehow let me know and I'm happy to approve as is.
app.json
Outdated
@@ -27,6 +27,9 @@ | |||
"LANG": { | |||
"required": true | |||
}, | |||
"MAINTAINER_EMAIL": { | |||
"required": true | |||
} |
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 like there's a comma missing on this line?
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.
Good catch. All future PR builds were likely to blow up lol
app.json
Outdated
@@ -77,4 +80,4 @@ | |||
"url": "heroku/ruby" | |||
} | |||
] | |||
} | |||
} |
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.
I don't know that this matters, but I see a symbol here that there's no trailing carriage return on this file. Ignore if you like, I'm happy to merge without this.
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.
ugh. yeah my editor seems to remove that railing line in json files for some reason when I save. Since I need to fix the json anyway for the above catch, I'll see if I can get this resolved as well.
Why are these changes being introduced: * Most versions of ruby seem to include this gem bundled, but for some reason this is not happening on my latest laptop. This experimental branch is to mostly confirm adding this gem manually doesn't negatively affect our various environments. Document any side effects to this change: We're explicitly including a gem that seems to often already be included. This may allow us to keep it up to date more than relying on ruby-included versions that may be installed in various environments.
Why are these changes being introduced: * the virus scanner removes access to files when a virus is detected * Rails tries to access each file being added to ActiveStorage when the record is saved * files with viruses will throw a error * Handling the specific error allows us to signal to the user a problem was detected * The Transfer record and all files associated with it remain in a fully valid state with only the virus infected files being inacessible Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-349 How does this address that need: * We recover from the specific S3 access denied message during Transfer saves, modify the message slightly to the user, then continue with the rest of the process Document any side effects to this change: * If there is any other reason the S3 file access would be denied during this save, it may not be what we expect.
Why are these changes being introduced: * knowing when this error is handled and being able to confirm it is due to what we expected and not some other underlying problem with the transfer will be 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.
Why are these changes being introduced:
record is saved
was detected
valid state with only the virus infected files being inacessible
Relevant ticket(s):
How does this address that need:
saves, modify the message slightly to the user, then continue with the
rest of the process
Document any side effects to this change:
this save, it may not be what we expect.
json
, but for somereason this is not happening on my latest laptop. There doesn't seem to be any
harm in including it in the
Gemfile
and it makes my life way easierDeveloper
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
YES