Merge pull request #84 from alphagov/respond-with-error-if-asset-requ…

…ested-from-s3-but-s3-not-configured

Respond with error if asset requested from S3, but S3 not configured

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,13 +10,18 @@ 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
 
   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

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

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
 )

lib/cloud_storage.rb

@@ -0,0 +1,3 @@
+class CloudStorage
+  NotConfiguredError = Class.new(StandardError)
+end

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
 

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

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

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

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

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

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