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