diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index a4c8a0d0..2096f7a0 100644 --- a/app/controllers/media_controller.rb +++ b/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}" diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index 49df427b..685c6534 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/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) } diff --git a/spec/lib/s3_storage/fake_spec.rb b/spec/lib/s3_storage/fake_spec.rb index 494fa379..8e174554 100644 --- a/spec/lib/s3_storage/fake_spec.rb +++ b/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 diff --git a/spec/requests/access_limited_assets_spec.rb b/spec/requests/access_limited_assets_spec.rb index ede74962..9b903a67 100644 --- a/spec/requests/access_limited_assets_spec.rb +++ b/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 diff --git a/spec/requests/access_limited_whitehall_assets_spec.rb b/spec/requests/access_limited_whitehall_assets_spec.rb index f1d6483b..7b05301c 100644 --- a/spec/requests/access_limited_whitehall_assets_spec.rb +++ b/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 diff --git a/spec/requests/media_requests_spec.rb b/spec/requests/media_requests_spec.rb index 1a2b04ae..3dc0de35 100644 --- a/spec/requests/media_requests_spec.rb +++ b/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 diff --git a/spec/requests/virus_scanning_spec.rb b/spec/requests/virus_scanning_spec.rb index 45eb4d1f..04a27518 100644 --- a/spec/requests/virus_scanning_spec.rb +++ b/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 diff --git a/spec/requests/whitehall_media_requests_spec.rb b/spec/requests/whitehall_media_requests_spec.rb index 8796d9de..83b6eb56 100644 --- a/spec/requests/whitehall_media_requests_spec.rb +++ b/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 diff --git a/spec/service_consumers/pact_helper.rb b/spec/service_consumers/pact_helper.rb index 4f471bb9..7bd95377 100644 --- a/spec/service_consumers/pact_helper.rb +++ b/spec/service_consumers/pact_helper.rb @@ -37,6 +37,8 @@ def url_encode(str) WebMock.reset! DatabaseCleaner.clean_with :truncation GDS::SSO.test_user = create(:user) + AssetManager.s3 = S3Configuration.build + allow(AssetManager.s3).to receive(:fake?).and_return(false) end tear_down do