From 8327889c1c261697917ac64c86ea648836a50076 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 7 Jun 2021 15:00:35 -0400 Subject: [PATCH 1/4] Adds json gem 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. --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 31c55d2b..a46276cf 100644 --- a/Gemfile +++ b/Gemfile @@ -13,6 +13,7 @@ gem 'cancancan' gem 'cocoon' gem 'delayed_job_active_record' gem 'devise' +gem 'json' gem 'kaminari' gem 'lograge' gem 'mitlibraries-theme' diff --git a/Gemfile.lock b/Gemfile.lock index 51fb00c2..66969ab5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -141,6 +141,7 @@ GEM rails-dom-testing (>= 1, < 3) railties (>= 4.2.0) thor (>= 0.14, < 2.0) + json (2.5.1) kaminari (1.2.1) activesupport (>= 4.1.0) kaminari-actionview (= 1.2.1) @@ -333,6 +334,7 @@ DEPENDENCIES delayed_job_active_record devise dotenv-rails + json kaminari listen lograge From 200db914e7eaeee8be3d76623b0b73efc2f3fd51 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Wed, 9 Jun 2021 13:12:12 -0400 Subject: [PATCH 2/4] Recover when virus is detected during Transfer 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. --- app/controllers/transfer_controller.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/controllers/transfer_controller.rb b/app/controllers/transfer_controller.rb index b4325d5e..a9270d13 100644 --- a/app/controllers/transfer_controller.rb +++ b/app/controllers/transfer_controller.rb @@ -24,6 +24,15 @@ def create flash[:error] = "Error saving transfer: #{@transfer.errors.full_messages}" render 'new' end + + # A virus detected prior to the Transfer being saved throws this error but + # our application and the files in s3 are in a state where it is safe to + # continue and investigate which file was problematic asynchronously + rescue Aws::S3::Errors::AccessDenied + flash[:error] = "We detected a potential problem with a file in your upload. Library staff will contact you with details when we have more details." + ReceiptMailer.transfer_receipt_email(@transfer, current_user).deliver_later + redirect_to transfer_confirm_path + end def files From 9d5995d2ef7a7a6610b15afb078bfa8f41bbca15 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Wed, 9 Jun 2021 13:29:47 -0400 Subject: [PATCH 3/4] Send email to maintainers when virus detected 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 --- README.md | 1 + app.json | 5 ++++- app/controllers/transfer_controller.rb | 2 ++ app/mailers/receipt_mailer.rb | 9 +++++++++ app/views/receipt_mailer/virus_detected_email.html.erb | 9 +++++++++ 5 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 app/views/receipt_mailer/virus_detected_email.html.erb diff --git a/README.md b/README.md index 2891d186..a5bb6d84 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,7 @@ this automatically. It is often nice in development as well. ### Development +`MAINTAINER_EMAIL` - used for `to` field of virus detected emails. `THESIS_ADMIN_EMAIL` - used for `from` field of receipt emails. ### Production diff --git a/app.json b/app.json index 02fe64fc..263def5a 100644 --- a/app.json +++ b/app.json @@ -27,6 +27,9 @@ "LANG": { "required": true }, + "MAINTAINER_EMAIL": { + "required": true + } "RACK_ENV": { "required": true }, @@ -77,4 +80,4 @@ "url": "heroku/ruby" } ] -} +} \ No newline at end of file diff --git a/app/controllers/transfer_controller.rb b/app/controllers/transfer_controller.rb index a9270d13..75190981 100644 --- a/app/controllers/transfer_controller.rb +++ b/app/controllers/transfer_controller.rb @@ -31,6 +31,8 @@ def create rescue Aws::S3::Errors::AccessDenied flash[:error] = "We detected a potential problem with a file in your upload. Library staff will contact you with details when we have more details." ReceiptMailer.transfer_receipt_email(@transfer, current_user).deliver_later + + ReceiptMailer.virus_detected_email(@transfer).deliver_later redirect_to transfer_confirm_path end diff --git a/app/mailers/receipt_mailer.rb b/app/mailers/receipt_mailer.rb index 327912fa..35440f47 100644 --- a/app/mailers/receipt_mailer.rb +++ b/app/mailers/receipt_mailer.rb @@ -17,4 +17,13 @@ def transfer_receipt_email(transfer, user) cc: ENV['THESIS_ADMIN_EMAIL'], subject: 'Thesis files transferred') end + + def virus_detected_email(transfer) + return unless ENV.fetch('DISABLE_ALL_EMAIL', 'true') == 'false' # allows PR builds to disable emails + @transfer = transfer + mail(from: "MIT Libraries <#{ENV['THESIS_ADMIN_EMAIL']}>", + to: ENV['MAINTAINER_EMAIL'], + cc: ENV['THESIS_ADMIN_EMAIL'], + subject: 'Thesis files with virus detected') + end end diff --git a/app/views/receipt_mailer/virus_detected_email.html.erb b/app/views/receipt_mailer/virus_detected_email.html.erb new file mode 100644 index 00000000..e7dbead1 --- /dev/null +++ b/app/views/receipt_mailer/virus_detected_email.html.erb @@ -0,0 +1,9 @@ +

A file was flagged with a virus during Transfer <%= @transfer.created_at.in_time_zone("America/New_York").strftime('%b %-d, %Y at %l:%M %p %Z') %>.

+ +

Files transferred:

+ +
    + <% @transfer.files.each do |file| %> +
  1. <%= file.filename %>
    2. + <% end %> +
From fc5ed3d47207fe4ad864c7bc60dc6d90890a24e6 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Thu, 10 Jun 2021 09:20:01 -0400 Subject: [PATCH 4/4] fix json syntax in app.json --- app.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app.json b/app.json index 263def5a..14e641e3 100644 --- a/app.json +++ b/app.json @@ -29,7 +29,7 @@ }, "MAINTAINER_EMAIL": { "required": true - } + }, "RACK_ENV": { "required": true }, @@ -80,4 +80,4 @@ "url": "heroku/ruby" } ] -} \ No newline at end of file +}