From 96b28e258e3f480e7969dd9b507dbff5161629cc Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 25 Jul 2017 16:06:28 +0100 Subject: [PATCH] TODO: Remove this commit Squashed commits from the following branch: respond-with-error-if-asset-requested-from-s3-but-s3-not-configured See PR #84. Squashed commit of the following: commit 735e650bc8f82ffe8e52ea256dde1bfbbb193aa3 Author: James Mead Date: Tue Jul 25 13:07:23 2017 +0100 Error if asset requested from S3, but S3 not configured If the AWS S3 bucket is not configured and a request is made to stream an asset from S3, then the server now responds with a "500 Internal server error" instead of a "200 OK" and an empty file. * I decided to introduce a `CloudStorage` class as a namespace for the base class of the `NotConfiguredError` exception to keep the `ApplicationController` agnostic of the underlying cloud storage implementation. * The "500 Internal server error" status seemed the most appropriate. I also considered the following: * "501 Not implemented", but the functionality *is* implemented, it's just not correctly configured. * "503 Service unavailable", but this error supposed to only be temporary and that doesn't seem quite right in this case. * "404 Not found", but the problem seems more serious than that. * I decided to capitalise "Internal" in the error status, because that seems like a more common approach. I wanted to capitalise the "not found" status for the 404 error, but I was concerned there might be clients depending on it, so I left it unchanged. * I decided to append the exception message to the error status string, because I think it's useful to make as much information available as possible. I tried to see whether there was any standard way of doing this for other GOV.UK APIs, but couldn't find any consistent approach. * I introduced the `disable_cloud_storage_stub: true` option for specs so that I could write an example where the exception was raised. Fixes #78. commit 86ba9d248b2923460c59ed4e3207b0ca55e12e18 Author: James Mead Date: Tue Jul 25 12:30:23 2017 +0100 Rename spec/support/s3_storage.rb -> cloud_storage.rb To better reflect its purpose. commit fa9d9f54b90d10732bb1979a9d30e512b94cb831 Author: James Mead Date: Tue Jul 25 11:33:30 2017 +0100 Extract MediaController#stream_from_s3? method To make the code more readable. commit 13ba282a4d3a268a3dee2a6ead2505f48ed0c110 Author: James Mead Date: Tue Jul 25 11:31:34 2017 +0100 Extract AWS_S3_BUCKET_NAME env var into app config So it's easier to stub. Unfortunately I've had to disable a Rubocop rule around one line. This violation is due to the digit `3` in the variable name. It looks as if the constraints for this rule have been relaxed [1] in Rubocop v0.44.0, but we're currently using v0.43.0 and upgrading means first upgrading the constraints on the Rubocop version in the specification for the `govuk-lint` gem and then upgrading that gem in this project. I'm going to do that separately and I'll make a note to remove this disabling of the rule when I do that. [1]: bbatsov/rubocop@833aad6ccb35a16c6196830e936f4ef34384590b commit fd0a75bdcb6ae43262837433aac8d112ab804a7e Author: James Mead Date: Mon Jul 24 18:00:13 2017 +0100 Use have_http_status matcher & status code symbols To make controller & request specs more expressive. Also: * Reworded some example names to make them more consistent. * `be_successful` -> `be_success` for consistency. * In one example: `be_success` -> `have_http_status(:success)` to contrast with example immediately preceding it. commit 985ce25b958beef924a2b1edc7c00a1c0484b67e Author: James Mead Date: Mon Jul 24 17:43:48 2017 +0100 Add blank lines to some specs to improve readability --- app/controllers/application_controller.rb | 7 ++++ app/controllers/media_controller.rb | 6 +++- config/initializers/aws.rb | 2 ++ lib/cloud_storage.rb | 3 ++ lib/s3_storage.rb | 6 +++- lib/services.rb | 5 ++- spec/controllers/assets_controller_spec.rb | 32 ++++++++++--------- spec/controllers/media_controller_spec.rb | 14 ++++---- spec/lib/s3_storage_spec.rb | 12 +++---- spec/requests/asset_requests_spec.rb | 22 ++++++------- spec/requests/healthcheck_spec.rb | 2 +- spec/requests/media_requests_spec.rb | 27 ++++++++++++++-- spec/requests/virus_scanning_spec.rb | 12 +++---- .../{s3_storage.rb => cloud_storage.rb} | 2 +- 14 files changed, 98 insertions(+), 54 deletions(-) create mode 100644 lib/cloud_storage.rb rename spec/support/{s3_storage.rb => cloud_storage.rb} (75%) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3ba7af81..8fe51a5b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,3 +1,5 @@ +require 'cloud_storage' + class ApplicationController < ActionController::Base # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. @@ -8,6 +10,7 @@ class ApplicationController < ActionController::Base before_filter :require_signin_permission! rescue_from Mongoid::Errors::DocumentNotFound, with: :error_404 + rescue_from CloudStorage::NotConfiguredError, with: :error_500 private @@ -15,6 +18,10 @@ def error_404 error 404, "not found" end + def error_500(e) + error 500, "Internal server error: #{e.message}" + end + def error(code, message) render json: { _response_info: { status: message } }, status: code end diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index bbe43522..f145ab01 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -16,7 +16,7 @@ def download respond_to do |format| format.any do set_expiry(24.hours) - if params[:stream_from_s3].present? + if stream_from_s3? body = Services.cloud_storage.load(asset) send_data(body.read, filename: File.basename(asset.file.path), disposition: 'inline') else @@ -28,6 +28,10 @@ def download protected + def stream_from_s3? + params[:stream_from_s3].present? + end + def filename_current? asset.filename == params[:filename] end diff --git a/config/initializers/aws.rb b/config/initializers/aws.rb index d34ad101..3d5eef69 100644 --- a/config/initializers/aws.rb +++ b/config/initializers/aws.rb @@ -1,3 +1,5 @@ +AssetManager::Application.config.aws_s3_bucket_name = ENV['AWS_S3_BUCKET_NAME'] + Aws.config.update( logger: Rails.logger ) diff --git a/lib/cloud_storage.rb b/lib/cloud_storage.rb new file mode 100644 index 00000000..f10c6269 --- /dev/null +++ b/lib/cloud_storage.rb @@ -0,0 +1,3 @@ +class CloudStorage + NotConfiguredError = Class.new(StandardError) +end diff --git a/lib/s3_storage.rb b/lib/s3_storage.rb index 844da145..c96214dc 100644 --- a/lib/s3_storage.rb +++ b/lib/s3_storage.rb @@ -1,10 +1,14 @@ +require 'cloud_storage' + class S3Storage + NotConfiguredError = Class.new(CloudStorage::NotConfiguredError) + class Null def save(_asset) end def load(_asset) - StringIO.new + raise NotConfiguredError.new('AWS S3 bucket not correctly configured') end end diff --git a/lib/services.rb b/lib/services.rb index 770d975c..031fc647 100644 --- a/lib/services.rb +++ b/lib/services.rb @@ -2,6 +2,9 @@ module Services def self.cloud_storage - @cloud_storage ||= S3Storage.build(ENV['AWS_S3_BUCKET_NAME']) + # rubocop:disable Style/VariableNumber + aws_s3_bucket_name = AssetManager::Application.config.aws_s3_bucket_name + # rubocop:enable Style/VariableNumber + @cloud_storage ||= S3Storage.build(aws_s3_bucket_name) end end diff --git a/spec/controllers/assets_controller_spec.rb b/spec/controllers/assets_controller_spec.rb index e79ccdf9..0a1cd957 100644 --- a/spec/controllers/assets_controller_spec.rb +++ b/spec/controllers/assets_controller_spec.rb @@ -17,11 +17,13 @@ expect(assigns(:asset)).to be_persisted expect(assigns(:asset).file.current_path).to match(/asset\.png$/) end + it "returns a created status" do post :create, asset: attributes - expect(response.status).to eq(201) + expect(response).to have_http_status(:created) end + it "returns the location and details of the new asset" do post :create, asset: attributes @@ -45,10 +47,10 @@ expect(assigns(:asset).file.current_path).to be_nil end - it "returns an unprocessable status" do + it "returns an unprocessable entity status" do post :create, asset: attributes - expect(response.status).to eq(422) + expect(response).to have_http_status(:unprocessable_entity) end end end @@ -68,7 +70,7 @@ it "returns a success status" do put :update, id: asset.id, asset: attributes - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) end it "returns the location and details of the new asset" do @@ -94,10 +96,10 @@ expect(assigns(:asset).file.current_path).to be_nil end - it "returns an unprocessable status" do + it "returns an unprocessable entity status" do post :create, asset: attributes - expect(response.status).to eq(422) + expect(response).to have_http_status(:unprocessable_entity) end end end @@ -115,14 +117,14 @@ it "returns a success status" do delete :destroy, id: asset.id - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) end end context "an asset that doesn't exist" do - it "responds with 404" do + it "responds with not found status" do delete :destroy, id: "12345" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end end @@ -137,8 +139,8 @@ delete :destroy, id: asset.id end - it "responds with 422" do - expect(response.status).to eq(422) + it "responds with unprocessable entity status" do + expect(response).to have_http_status(:unprocessable_entity) end it "returns the asset errors" do @@ -169,7 +171,7 @@ it "returns a not found status" do get :show, id: "some-gif-or-other" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end it "returns a not found message" do @@ -189,7 +191,7 @@ end it "is a successful request" do - expect(response).to be_successful + expect(response).to be_success end it "assigns the asset" do @@ -209,8 +211,8 @@ post :restore, id: asset.id end - it "responds with 422" do - expect(response.status).to be(422) + it "responds with unprocessable entity status" do + expect(response).to have_http_status(:unprocessable_entity) end it "responds with an error message" do diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index cab6cf96..ed44cff5 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -124,14 +124,14 @@ def do_get(extra_params = {}) let(:restricted_asset) { FactoryGirl.create(:access_limited_asset, organisation_slug: 'example-slug') } let(:unrestricted_asset) { FactoryGirl.create(:clean_asset) } - it "404s requests to access limited documents" do + it "responds with not found status for access-limited documents" do get :download, id: restricted_asset.id.to_s, filename: 'asset.png' - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end it "permits access to unrestricted documents" do get :download, id: unrestricted_asset.id.to_s, filename: 'asset.png' - expect(response).to be_success + expect(response).to have_http_status(:success) end end @@ -148,13 +148,13 @@ def do_get(extra_params = {}) get :download, id: asset.id.to_s, filename: 'asset.png' end - it "404s requests to access limited documents if the user has the wrong organisation" do + it "responds with not found status for access-limited documents if the user has the wrong organisation" do user = FactoryGirl.create(:user, organisation_slug: 'incorrect-organisation-slug') login_as(user) get :download, id: asset.id.to_s, filename: 'asset.png' - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end it "permits access to access limited documents if the user has the right organisation" do @@ -174,8 +174,8 @@ def do_get(extra_params = {}) get :download, id: asset.id.to_s, filename: asset.file.file.identifier end - it "response should be 404" do - expect(response.status).to eq(404) + it "responds with not found status" do + expect(response).to have_http_status(:not_found) end end end diff --git a/spec/lib/s3_storage_spec.rb b/spec/lib/s3_storage_spec.rb index 85b5fbab..33d8a826 100644 --- a/spec/lib/s3_storage_spec.rb +++ b/spec/lib/s3_storage_spec.rb @@ -47,14 +47,10 @@ context 'when bucket name is blank' do let(:bucket_name) { '' } - it 'does not download file from S3 bucket' do - expect(Aws::S3::Object).not_to receive(:new) - - subject.load(asset) - end - - it 'returns empty StringIO' do - expect(subject.load(asset).read).to eq('') + it 'raises NotConfiguredError exception' do + expect { + subject.load(asset) + }.to raise_error(S3Storage::NotConfiguredError, 'AWS S3 bucket not correctly configured') end end end diff --git a/spec/requests/asset_requests_spec.rb b/spec/requests/asset_requests_spec.rb index 4e178f92..60168676 100644 --- a/spec/requests/asset_requests_spec.rb +++ b/spec/requests/asset_requests_spec.rb @@ -11,7 +11,7 @@ post "/assets", asset: { file: load_fixture_file("asset.png") } body = JSON.parse(response.body) - expect(response.status).to eq(201) + expect(response).to have_http_status(:created) expect(body["_response_info"]["status"]).to eq("created") expect(body["id"]).to match(%r{http://www.example.com/assets/[a-z0-9]+}) @@ -24,7 +24,7 @@ post "/assets", asset: { file: nil } body = JSON.parse(response.body) - expect(response.status).to eq(422) + expect(response).to have_http_status(:unprocessable_entity) expect(body["_response_info"]["status"]).to eq(["File can't be blank"]) end end @@ -40,7 +40,7 @@ put "/assets/#{asset_id}", asset: { file: load_fixture_file("asset2.jpg") } body = JSON.parse(response.body) - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(body["_response_info"]["status"]).to eq("success") expect(body["id"]).to end_with(asset_id) @@ -53,7 +53,7 @@ post "/assets", asset: { file: nil } body = JSON.parse(response.body) - expect(response.status).to eq(422) + expect(response).to have_http_status(:unprocessable_entity) expect(body["_response_info"]["status"]).to eq(["File can't be blank"]) end end @@ -65,7 +65,7 @@ get "/assets/#{asset.id}" body = JSON.parse(response.body) - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(body["_response_info"]["status"]).to eq("ok") expect(body["id"]).to eq("http://www.example.com/assets/#{asset.id}") @@ -81,7 +81,7 @@ get "/assets/#{asset.id}" body = JSON.parse(response.body) - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(body["_response_info"]["status"]).to eq("ok") expect(body["id"]).to eq("http://www.example.com/assets/#{asset.id}") @@ -95,7 +95,7 @@ get "/assets/blah" body = JSON.parse(response.body) - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) expect(body["_response_info"]["status"]).to eq("not found") end end @@ -107,7 +107,7 @@ delete "/assets/#{asset.id}" body = JSON.parse(response.body) - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(body["_response_info"]["status"]).to eq("success") expect(body["id"]).to eq("http://www.example.com/assets/#{asset.id}") expect(body["name"]).to eq("asset.png") @@ -117,7 +117,7 @@ get "/assets/#{asset.id}" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end end @@ -129,7 +129,7 @@ body = JSON.parse(response.body) - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(body["_response_info"]["status"]).to eq("success") expect(body["id"]).to eq("http://www.example.com/assets/#{asset.id}") expect(body["name"]).to eq("asset.png") @@ -139,7 +139,7 @@ get "/assets/#{asset.id}" - expect(response).to be_successful + expect(response).to be_success end end end diff --git a/spec/requests/healthcheck_spec.rb b/spec/requests/healthcheck_spec.rb index 08bb2c8b..6d69c7f3 100644 --- a/spec/requests/healthcheck_spec.rb +++ b/spec/requests/healthcheck_spec.rb @@ -3,7 +3,7 @@ RSpec.describe "Healthcheck", type: :request do it "should respond with success on the healthehcek path" do get "/healthcheck" - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expect(response.content_type).to eq("text/plain") expect(response.body).to eq("OK") end diff --git a/spec/requests/media_requests_spec.rb b/spec/requests/media_requests_spec.rb index e936c3ba..e3180aea 100644 --- a/spec/requests/media_requests_spec.rb +++ b/spec/requests/media_requests_spec.rb @@ -2,14 +2,37 @@ RSpec.describe "Media requests", type: :request do describe "requesting an asset that doesn't exist" do - it "should respond with file not found" do + it "should respond with not found status" do get "/media/34/test.jpg" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) + end + end + + describe "request an asset to be streamed from S3", disable_cloud_storage_stub: true do + context "when bucket not configured" do + let(:asset) { FactoryGirl.create(:clean_asset) } + + before do + allow(AssetManager::Application.config).to receive(:aws_s3_bucket_name).and_return(nil) + end + + it "should respond with internal server error status" do + get "/media/#{asset.id}/asset.png?stream_from_s3=true" + expect(response).to have_http_status(:internal_server_error) + end + + it "should include error message in JSON response" do + get "/media/#{asset.id}/asset.png?stream_from_s3=true" + json = JSON.parse(response.body) + status = json['_response_info']['status'] + expect(status).to eq('Internal server error: AWS S3 bucket not correctly configured') + end end end describe "request an asset that does exist" do let(:asset) { FactoryGirl.create(:clean_asset) } + before do get "/media/#{asset.id}/asset.png", nil, "HTTP_X_SENDFILE_TYPE" => "X-Accel-Redirect", "HTTP_X_ACCEL_MAPPING" => "#{Rails.root}/tmp/test_uploads/assets/=/raw/" diff --git a/spec/requests/virus_scanning_spec.rb b/spec/requests/virus_scanning_spec.rb index fa284b44..85e91760 100644 --- a/spec/requests/virus_scanning_spec.rb +++ b/spec/requests/virus_scanning_spec.rb @@ -7,7 +7,7 @@ specify "uploading a clean asset, and seeing it available after virus scanning" do post "/assets", asset: { file: load_fixture_file("lorem.txt") } - expect(response.status).to eq(201) + expect(response).to have_http_status(:created) asset = Asset.last @@ -15,12 +15,12 @@ expect(asset_details["id"]).to match(%r{http://www.example.com/assets/#{asset.id}}) get "/media/#{asset.id}/lorem.txt" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) run_all_delayed_jobs get "/media/#{asset.id}/lorem.txt" - expect(response.status).to eq(200) + expect(response).to have_http_status(:success) expected = File.read(fixture_file_path("lorem.txt")) expect(response.body).to eq(expected) @@ -46,7 +46,7 @@ def initialize specify "uploading an infected asset, and not seeing it available after virus scanning" do post "/assets", asset: { file: UploadedVirus.new } - expect(response.status).to eq(201) + expect(response).to have_http_status(:created) asset = Asset.last @@ -54,11 +54,11 @@ def initialize expect(asset_details["id"]).to match(%r{http://www.example.com/assets/#{asset.id}}) get "/media/#{asset.id}/eicar.com" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) run_all_delayed_jobs get "/media/#{asset.id}/eicar.com" - expect(response.status).to eq(404) + expect(response).to have_http_status(:not_found) end end diff --git a/spec/support/s3_storage.rb b/spec/support/cloud_storage.rb similarity index 75% rename from spec/support/s3_storage.rb rename to spec/support/cloud_storage.rb index b61d1223..72a77be4 100644 --- a/spec/support/s3_storage.rb +++ b/spec/support/cloud_storage.rb @@ -1,7 +1,7 @@ require 'services' RSpec.configure do |config| - config.before do + config.before(:each, disable_cloud_storage_stub: false) do cloud_storage = double(:cloud_storage).as_null_object allow(Services).to receive(:cloud_storage).and_return(cloud_storage) end