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 735e650
Author: James Mead <james@floehopper.org>
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 86ba9d2
Author: James Mead <james@floehopper.org>
Date:   Tue Jul 25 12:30:23 2017 +0100

    Rename spec/support/s3_storage.rb -> cloud_storage.rb

    To better reflect its purpose.

commit fa9d9f5
Author: James Mead <james@floehopper.org>
Date:   Tue Jul 25 11:33:30 2017 +0100

    Extract MediaController#stream_from_s3? method

    To make the code more readable.

commit 13ba282
Author: James Mead <james@floehopper.org>
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]: rubocop/rubocop@833aad6

commit fd0a75b
Author: James Mead <james@floehopper.org>
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 985ce25
Author: James Mead <james@floehopper.org>
Date:   Mon Jul 24 17:43:48 2017 +0100

    Add blank lines to some specs to improve readability

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