Skip to content
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

oy2-25705 - allow download of attachments only if passed virus scan #1451

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bflynn-cms
Copy link
Collaborator

Story: https://qmacbis.atlassian.net/browse/OY2-25075
Endpoint: See github-actions bot comment

Details

If an attachment hasnt passed virus scan then do not display as link. Additionally due to this the download all button behavior was changed.

Changes

  • When processing signed urls for attachment, first get the attachment tags and determine virus scan outcome, if there is an error getting tags or tag does not yet exist then treat as scan pending. Only generate signed urls for attachments with a CLEAN virus scan.
  • on the UI side display as normal for CLEAN attachments (have a url) and as plain text without a link for attachments that did not pass virus scan (null url)
  • for the download all button in each section, though it wasnt in the AC, i decided it odd that a section with no downloadable attachments (all pending or failed) would have a button that downloaded an empty zip file and therefore I decided to conditionally display the button only if there is at least one downloadable file. Also had to change the server side logic as well to return a null object for each attachment without a url and filter out before attempting to combine to a zip file.

Implementation Notes

  • Could have gone about this 2 ways, 1 was this way to always interrogate the tag status via aws api. This does result in more network traffic between the lambda and DB but its negligible since its all within the VPC and records will only have 10s of attachments not 1000s so the number of calls is small and quick.
    The alternative would be to keep the virusScanStatus along with the package in the DB. I felt that was far more intrusive and would have required deeper code edits but worse of all would directly couple the attachments service to the DB as it would have to update a DB record or call package builder or something that would have an effect to get the package rebuilt everytime an attachment scan completed and I didnt really think that would be a good approach.

  • Note that alot of our attachments in feature branches with staged data dont actually point to the real branch s3 bucket and therefore most the staged data does not function properly (I did change a few just to verify it was on the up and up). To fully test you will have to submit new attachments. I expect this to only be an issue on feature branches where we load test data that has erroneous url values in the package.

Test Plan

  1. Submit new package of any type
  2. Verify directly after submittal that the attachments are not available for download and no "Download All" button appears (unitl at least one attachment is available)
  3. Refresh after a minute or two (about the time virus scan takes) and verify attachments are available.
  4. Alter tags in s3 to make one attachment have virusScanStatus = INFECTED and verify that status translates to the UI and the file is undownloadable. Also verify Download ALL will download the remaining attachments but not the fake infected attachment.

Copy link

Endpoint URL - https://d22tvuxyzefp9s.cloudfront.net

Copy link
Collaborator

@kristin-at-theta kristin-at-theta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the one hand, I prefer to have the database be the place where all the "truths" are kept... BUT, at the same time, by looking at/for the actual attachment, it is doing an extra verification that the file hasn't gotten lost somewhere from something that didn't update the database. So, now I like this way better :)

updates look good, tests clean, Approved! Great work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants