Merge pull request #1193 from alphagov/fix-static-url-for-assets-preview

Fix static URL for assets (local dev env)

app/controllers/media_controller.rb

@@ -97,6 +97,9 @@ def proxy_to_s3_via_nginx(asset)
 
     if request.fresh?(response)
       head :not_modified
+    elsif AssetManager.s3.fake?
+      url = Services.cloud_storage.presigned_url_for(asset, http_method: request.request_method)
+      redirect_to url
     else
       url = Services.cloud_storage.presigned_url_for(asset, http_method: request.request_method)
       headers["X-Accel-Redirect"] = "/cloud-storage-proxy/#{url}"

spec/controllers/media_controller_spec.rb

@@ -26,6 +26,7 @@ def download
       let(:presigned_url) { "https://s3-host.test/presigned-url" }
       let(:last_modified) { Time.zone.parse("2017-01-01 00:00") }
       let(:content_disposition) { instance_double(ContentDispositionConfiguration) }
+      let(:s3) { S3Configuration.build }
       let(:http_method) { "GET" }
 
       before do
@@ -38,142 +39,167 @@ def download
         allow(asset).to receive(:last_modified).and_return(last_modified)
         allow(AssetManager).to receive(:content_disposition).and_return(content_disposition)
         allow(content_disposition).to receive(:header_for).with(asset).and_return("content-disposition")
+        allow(AssetManager).to receive(:s3).and_return(s3)
       end
 
-      shared_examples "a download response" do
-        it "instructs Nginx to proxy the request to S3" do
-          get :download, params: { id: asset.id }
+      context "when using real s3 in non-local environment" do
+        before do
+          allow(s3).to receive(:fake?).and_return(false)
+        end
+
+        shared_examples "a download response" do
+          it "instructs Nginx to proxy the request to S3" do
+            get :download, params: { id: asset.id }
+
+            expect(response.headers["X-Accel-Redirect"]).to match("/cloud-storage-proxy/#{presigned_url}")
+          end
+
+          it "returns an ok response" do
+            get :download, params: { id: asset.id }
 
-          expect(response.headers["X-Accel-Redirect"]).to match("/cloud-storage-proxy/#{presigned_url}")
+            expect(response).to have_http_status(:ok)
+          end
         end
 
-        it "returns an ok response" do
-          get :download, params: { id: asset.id }
+        shared_examples "a not modified response" do
+          it "does not instruct Nginx to proxy the request to S3" do
+            get :download, params: { id: asset.id }
+
+            expect(response.headers).not_to include("X-Accel-Redirect")
+          end
+
+          it "returns a not modified response" do
+            get :download, params: { id: asset.id }
 
-          expect(response).to have_http_status(:ok)
+            expect(response).to have_http_status(:not_modified)
+          end
         end
-      end
 
-      shared_examples "a not modified response" do
-        it "does not instruct Nginx to proxy the request to S3" do
+        it "sends ETag response header with quoted value" do
           get :download, params: { id: asset.id }
 
-          expect(response.headers).not_to include("X-Accel-Redirect")
+          expect(response.headers["ETag"]).to eq(%("599ffda8-e169"))
         end
 
-        it "returns a not modified response" do
+        it "sends Last-Modified response header in HTTP time format" do
           get :download, params: { id: asset.id }
 
-          expect(response).to have_http_status(:not_modified)
+          expect(response.headers["Last-Modified"]).to eq("Sun, 01 Jan 2017 00:00:00 GMT")
         end
-      end
-
-      it "sends ETag response header with quoted value" do
-        get :download, params: { id: asset.id }
 
-        expect(response.headers["ETag"]).to eq(%("599ffda8-e169"))
-      end
+        it "sends Content-Disposition response header based on asset filename" do
+          get :download, params: { id: asset.id }
 
-      it "sends Last-Modified response header in HTTP time format" do
-        get :download, params: { id: asset.id }
+          expect(response.headers["Content-Disposition"]).to eq("content-disposition")
+        end
 
-        expect(response.headers["Last-Modified"]).to eq("Sun, 01 Jan 2017 00:00:00 GMT")
-      end
+        it "sends an Asset's content_type when one is set" do
+          allow(asset).to receive(:content_type).and_return("image/jpeg")
+          get :download, params: { id: asset.id }
 
-      it "sends Content-Disposition response header based on asset filename" do
-        get :download, params: { id: asset.id }
+          expect(response.headers["Content-Type"]).to eq("image/jpeg")
+        end
 
-        expect(response.headers["Content-Disposition"]).to eq("content-disposition")
-      end
+        it "determines an Asset's content_type by filename when it is not set" do
+          allow(asset).to receive(:content_type).and_return(nil)
+          allow(asset).to receive(:filename).and_return("file.pdf")
+          get :download, params: { id: asset.id }
 
-      it "sends an Asset's content_type when one is set" do
-        allow(asset).to receive(:content_type).and_return("image/jpeg")
-        get :download, params: { id: asset.id }
+          expect(response.headers["Content-Type"]).to eq("application/pdf")
+        end
 
-        expect(response.headers["Content-Type"]).to eq("image/jpeg")
-      end
+        context "when there aren't conditional headers" do
+          it_behaves_like "a download response"
+        end
 
-      it "determines an Asset's content_type by filename when it is not set" do
-        allow(asset).to receive(:content_type).and_return(nil)
-        allow(asset).to receive(:filename).and_return("file.pdf")
-        get :download, params: { id: asset.id }
+        context "when a conditional request is made using an ETag that matches the asset ETag" do
+          before { request.headers["If-None-Match"] = %("#{asset.etag}") }
 
-        expect(response.headers["Content-Type"]).to eq("application/pdf")
-      end
+          it_behaves_like "a not modified response"
+        end
 
-      context "when there aren't conditional headers" do
-        it_behaves_like "a download response"
-      end
+        context "when conditional request is made using an ETag that does not match the asset ETag" do
+          before { request.headers["If-None-Match"] = %("made-up-etag") }
 
-      context "when a conditional request is made using an ETag that matches the asset ETag" do
-        before { request.headers["If-None-Match"] = %("#{asset.etag}") }
+          it_behaves_like "a download response"
+        end
 
-        it_behaves_like "a not modified response"
-      end
+        context "when a conditional request is made using a timestamp that matches the asset timestamp" do
+          before do
+            request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          end
 
-      context "when conditional request is made using an ETag that does not match the asset ETag" do
-        before { request.headers["If-None-Match"] = %("made-up-etag") }
+          it_behaves_like "a not modified response"
+        end
 
-        it_behaves_like "a download response"
-      end
+        context "when a conditional request is made using a timestamp that is earlier than the asset timestamp" do
+          before do
+            request.headers["If-Modified-Since"] = (asset.last_modified - 1.day).httpdate
+          end
 
-      context "when a conditional request is made using a timestamp that matches the asset timestamp" do
-        before do
-          request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          it_behaves_like "a download response"
         end
 
-        it_behaves_like "a not modified response"
-      end
+        context "when a conditional request is made using a timestamp that is later than the asset timestamp" do
+          before do
+            request.headers["If-Modified-Since"] = (asset.last_modified + 1.day).httpdate
+          end
 
-      context "when a conditional request is made using a timestamp that is earlier than the asset timestamp" do
-        before do
-          request.headers["If-Modified-Since"] = (asset.last_modified - 1.day).httpdate
+          it_behaves_like "a not modified response"
         end
 
-        it_behaves_like "a download response"
-      end
+        context "when a conditional request is made using an Etag and timestamp that match the asset" do
+          before do
+            request.headers["If-None-Match"] = %("#{asset.etag}")
+            request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          end
 
-      context "when a conditional request is made using a timestamp that is later than the asset timestamp" do
-        before do
-          request.headers["If-Modified-Since"] = (asset.last_modified + 1.day).httpdate
+          it_behaves_like "a not modified response"
         end
 
-        it_behaves_like "a not modified response"
-      end
+        context "when a conditional request is made using an Etag that matches and timestamp that does not match the asset" do
+          before do
+            request.headers["If-None-Match"] = %("#{asset.etag}")
+            request.headers["If-Modified-Since"] = (asset.last_modified - 1.day).httpdate
+          end
 
-      context "when a conditional request is made using an Etag and timestamp that match the asset" do
-        before do
-          request.headers["If-None-Match"] = %("#{asset.etag}")
-          request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          it_behaves_like "a download response"
         end
 
-        it_behaves_like "a not modified response"
-      end
+        context "when a conditional request is made using an Etag that does not match and a timestamp that matches the asset" do
+          before do
+            request.headers["If-None-Match"] = "made-up-etag"
+            request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          end
 
-      context "when a conditional request is made using an Etag that matches and timestamp that does not match the asset" do
-        before do
-          request.headers["If-None-Match"] = %("#{asset.etag}")
-          request.headers["If-Modified-Since"] = (asset.last_modified - 1.day).httpdate
+          it_behaves_like "a download response"
         end
-
-        it_behaves_like "a download response"
       end
 
-      context "when a conditional request is made using an Etag that does not match and a timestamp that matches the asset" do
+      context "when using fake s3 in local environment" do
         before do
-          request.headers["If-None-Match"] = "made-up-etag"
-          request.headers["If-Modified-Since"] = asset.last_modified.httpdate
+          allow(s3).to receive(:fake?).and_return(true)
         end
 
-        it_behaves_like "a download response"
+        it "redirects to presigned fake s3 url directly instead of Nginx proxy" do
+          get :download, params: { id: asset.id }
+
+          expected_url = presigned_url
+          expect(controller).to redirect_to expected_url
+        end
       end
     end
   end
 
   describe "GET 'download'" do
-    before { not_logged_in }
+    before do
+      allow(AssetManager).to receive(:s3).and_return(s3)
+      allow(s3).to receive(:fake?).and_return(false)
+      not_logged_in
+    end
 
     let(:params) { { params: { id: asset, filename: asset.filename } } }
+    let(:s3) { S3Configuration.build }
 
     context "with a valid uploaded file" do
       let(:asset) { FactoryBot.create(:uploaded_asset) }

spec/lib/s3_storage/fake_spec.rb

@@ -42,7 +42,11 @@
   end
 
   describe "#presigned_url_for" do
+    let(:s3) { S3Configuration.build }
+
     before do
+      allow(AssetManager).to receive(:s3).and_return(s3)
+      allow(s3).to receive(:fake?).and_return(false)
       storage.upload(asset)
     end
 

spec/requests/access_limited_assets_spec.rb

@@ -6,8 +6,11 @@
   let(:user_3) { FactoryBot.create(:user, uid: "user-3-id", organisation_content_id: "org-a") }
   let(:user_4) { FactoryBot.create(:user, uid: "user-4-id", organisation_content_id: "org-b") }
   let(:asset) { FactoryBot.create(:uploaded_asset, draft: true, access_limited: ["user-1-id"], access_limited_organisation_ids: %w[org-a]) }
+  let(:s3) { S3Configuration.build }
 
   before do
+    allow(AssetManager).to receive(:s3).and_return(s3)
+    allow(s3).to receive(:fake?).and_return(false)
     host! AssetManager.govuk.draft_assets_host
   end
 

spec/requests/access_limited_whitehall_assets_spec.rb

@@ -4,8 +4,11 @@
   let(:user_1) { FactoryBot.create(:user, uid: "user-1-id") }
   let(:user_2) { FactoryBot.create(:user, uid: "user-2-id") }
   let(:asset) { FactoryBot.create(:uploaded_whitehall_asset, draft: true, access_limited: ["user-1-id"]) }
+  let(:s3) { S3Configuration.build }
 
   before do
+    allow(AssetManager).to receive(:s3).and_return(s3)
+    allow(s3).to receive(:fake?).and_return(false)
     host! AssetManager.govuk.draft_assets_host
   end
 

spec/requests/media_requests_spec.rb

@@ -1,7 +1,11 @@
 require "rails_helper"
 
 RSpec.describe "Media requests", type: :request do
+  let(:s3) { S3Configuration.build }
+
   before do
+    allow(AssetManager).to receive(:s3).and_return(s3)
+    allow(s3).to receive(:fake?).and_return(false)
     not_logged_in
     # create a user that can be used automatically with GDS SSO mock
     stub_user

spec/requests/virus_scanning_spec.rb

@@ -1,7 +1,11 @@
 require "rails_helper"
 
 RSpec.describe "Virus scanning of uploaded images", type: :request, disable_cloud_storage_stub: true do
+  let(:s3) { S3Configuration.build }
+
   before do
+    allow(AssetManager).to receive(:s3).and_return(s3)
+    allow(s3).to receive(:fake?).and_return(true)
     login_as_stub_user
   end
 
@@ -26,11 +30,10 @@
     SaveToCloudStorageWorker.drain
 
     get download_media_path(id: asset, filename: "lorem.txt")
-    expect(response).to have_http_status(:success)
+    expect(response).to have_http_status(:found)
 
-    redirect_url = headers["X-Accel-Redirect"]
-    cloud_url = redirect_url.match(%r{^/cloud-storage-proxy/(.*)$})[1]
-    get cloud_url
+    redirect_url = headers["location"]
+    get redirect_url
     expect(response).to have_http_status(:success)
   end
 end

spec/requests/whitehall_media_requests_spec.rb

@@ -10,11 +10,13 @@
         state:,
       )
     end
+    let(:s3) { S3Configuration.build }
 
     before do
       allow(cloud_storage).to receive(:presigned_url_for)
                                 .with(asset, http_method:).and_return(presigned_url)
-
+      allow(AssetManager).to receive(:s3).and_return(s3)
+      allow(s3).to receive(:fake?).and_return(false)
       get path
     end
 
@@ -75,11 +77,13 @@
   describe "request for an uploaded asset" do
     let(:path) { "/government/uploads/asset.png" }
     let(:asset) { FactoryBot.create(:uploaded_whitehall_asset, legacy_url_path: path) }
+    let(:s3) { S3Configuration.build }
 
     before do
       allow(cloud_storage).to receive(:presigned_url_for)
                                 .with(asset, http_method:).and_return(presigned_url)
-
+      allow(AssetManager).to receive(:s3).and_return(s3)
+      allow(s3).to receive(:fake?).and_return(false)
       get path,
           headers: {
             "HTTP_X_SENDFILE_TYPE" => "X-Accel-Redirect",
@@ -142,6 +146,12 @@
           legacy_url_path: path,
         )
       end
+      let(:s3) { S3Configuration.build }
+
+      before do
+        allow(AssetManager).to receive(:s3).and_return(s3)
+        allow(s3).to receive(:fake?).and_return(false)
+      end
 
       it "serves the asset without a valid token" do
         get path
@@ -187,6 +197,12 @@
           access_limited: %w[some-other-user],
         )
       end
+      let(:s3) { S3Configuration.build }
+
+      before do
+        allow(AssetManager).to receive(:s3).and_return(s3)
+        allow(s3).to receive(:fake?).and_return(false)
+      end
 
       it "does not serve the asset without a valid token" do
         get path