From 5fb2b97275fbe6689b9d6ec086bc43d6be6e0397 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 14 Jan 2025 08:14:54 +0000 Subject: [PATCH] Optionally use redirects and signed cookies for blobs --- .env.example | 1 + .env.test | 1 + Gemfile | 1 + Gemfile.lock | 4 + app/models/document.rb | 26 +++++ app/models/local_authority.rb | 1 + config/application.yml | 1 + .../bops_uploads/application_controller.rb | 18 +++ .../bops_uploads/blobs_controller.rb | 52 +++++++++ .../bops_uploads/files_controller.rb | 63 ++++------- .../bops_uploads/redirects_controller.rb | 19 ---- engines/bops_uploads/config/routes.rb | 11 +- engines/bops_uploads/lib/bops_uploads.rb | 4 + .../bops_uploads/lib/bops_uploads/engine.rb | 20 ++++ .../bops_uploads/spec/bops_uploads_helper.rb | 6 - .../spec/helpers/url_helpers_spec.rb | 35 ++++++ .../bops_uploads/spec/requests/files_spec.rb | 106 +++++++++++++++--- spec/requests/api/document_show_spec.rb | 2 +- spec/requests/api/oas3_spec.rb | 2 +- .../api/planning_application_show_spec.rb | 2 +- ...cement_document_validation_request_spec.rb | 10 +- 21 files changed, 293 insertions(+), 92 deletions(-) create mode 100644 engines/bops_uploads/app/controllers/bops_uploads/blobs_controller.rb delete mode 100644 engines/bops_uploads/app/controllers/bops_uploads/redirects_controller.rb create mode 100644 engines/bops_uploads/spec/helpers/url_helpers_spec.rb diff --git a/.env.example b/.env.example index 5faf122168..588d8941e4 100644 --- a/.env.example +++ b/.env.example @@ -7,3 +7,4 @@ GROUPED_PROPOSAL_DETAILS_FEATURE=enabled GOOGLE_TAG_MANAGER_ID=GTM-XXXXXXXX UPLOADS_HOSTNAME=uploads.bops.localhost UPLOADS_BASE_URL=http://uploads.bops.localhost:3000 +USE_SIGNED_URLS=false diff --git a/.env.test b/.env.test index 8c37e8cb7e..f895d54bc2 100644 --- a/.env.test +++ b/.env.test @@ -5,3 +5,4 @@ OS_VECTOR_TILES_API_KEY="testtest" NOTIFY_LETTER_API_KEY='testtest' UPLOADS_HOSTNAME=uploads.example.com UPLOADS_BASE_URL=http://uploads.example.com +USE_SIGNED_URLS=false diff --git a/Gemfile b/Gemfile index ed5d1ddf91..57e910c221 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ gem "aasm" gem "activerecord-postgis-adapter", "~> 10.0" gem "acts_as_list" gem "appsignal" +gem "aws-sdk-cloudfront", require: false gem "aws-sdk-s3", require: false gem "bootsnap", ">= 1.4.2", require: false gem "business_time" diff --git a/Gemfile.lock b/Gemfile.lock index 687994b6e8..797247e39f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -123,6 +123,9 @@ GEM ast (2.4.2) aws-eventstream (1.1.1) aws-partitions (1.489.0) + aws-sdk-cloudfront (1.55.0) + aws-sdk-core (~> 3, >= 3.119.0) + aws-sigv4 (~> 1.1) aws-sdk-core (3.119.1) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.239.0) @@ -664,6 +667,7 @@ DEPENDENCIES activerecord-postgis-adapter (~> 10.0) acts_as_list appsignal + aws-sdk-cloudfront aws-sdk-s3 bootsnap (>= 1.4.2) bops_admin! diff --git a/app/models/document.rb b/app/models/document.rb index db83fa5807..ec52f0cd75 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -263,7 +263,33 @@ class NotArchiveableError < StandardError; end self.user ||= Current.user end + has_many :file_variant_records, through: :file_blob, source: :variant_records + has_many :file_variant_attachments, through: :file_variant_records, source: :image_attachment + has_many :file_variant_blobs, through: :file_variant_attachments, source: :blob + + has_one :file_preview_image_attachment, through: :file_blob, source: :preview_image_attachment + has_one :file_preview_blob, through: :file_preview_image_attachment, source: :blob + has_many :file_preview_variant_records, through: :file_preview_blob, source: :variant_records + has_many :file_preview_variant_attachments, through: :file_preview_variant_records, source: :image_attachment + has_many :file_preview_variant_blobs, through: :file_preview_variant_attachments, source: :blob + class << self + def find_by_blob!(key:) + blob_associations = %i[ + file_blob + file_preview_blob + file_variant_blobs + file_preview_variant_blobs + ] + + left_joins(*blob_associations) + .where(file_blob: {key:}) + .or(where(file_variant_blobs: {key:})) + .or(where(file_preview_blob: {key:})) + .or(where(file_preview_variant_blobs: {key:})) + .distinct.sole + end + def tags(key) case key.to_s when "plans" diff --git a/app/models/local_authority.rb b/app/models/local_authority.rb index 4c9a1e5766..c0fbee0195 100644 --- a/app/models/local_authority.rb +++ b/app/models/local_authority.rb @@ -23,6 +23,7 @@ class LocalAuthority < ApplicationRecord with_options through: :planning_applications do has_many :audits + has_many :documents has_many :validation_requests end diff --git a/config/application.yml b/config/application.yml index 939da79115..d0b960d13c 100644 --- a/config/application.yml +++ b/config/application.yml @@ -13,3 +13,4 @@ shared: uploads_hostname: <%= ENV.fetch("UPLOADS_HOSTNAME", "uploads.bops.localhost:3000") %> uploads_base_url: <%= ENV.fetch("UPLOADS_BASE_URL", "http://uploads.bops.localhost:3000") %> domain: <%= ENV.fetch("DOMAIN", "bops.localhost:3000") %> + use_signed_urls: <%= ENV.fetch("USE_SIGNED_URLS", "false") == "true" %> diff --git a/engines/bops_uploads/app/controllers/bops_uploads/application_controller.rb b/engines/bops_uploads/app/controllers/bops_uploads/application_controller.rb index 7472e75fc9..c9f4407cd0 100644 --- a/engines/bops_uploads/app/controllers/bops_uploads/application_controller.rb +++ b/engines/bops_uploads/app/controllers/bops_uploads/application_controller.rb @@ -2,5 +2,23 @@ module BopsUploads class ApplicationController < ActionController::Base + include BopsCore::ApplicationController + + with_options to: :BopsUploads do + delegate :key_pair_id, :private_key, :cookie_signer + end + + before_action :set_service + before_action :set_blob + + private + + def set_service + @service = ActiveStorage::Blob.service + end + + def set_blob + @blob = ActiveStorage::Blob.find_by!(key: params[:key]) + end end end diff --git a/engines/bops_uploads/app/controllers/bops_uploads/blobs_controller.rb b/engines/bops_uploads/app/controllers/bops_uploads/blobs_controller.rb new file mode 100644 index 0000000000..8b41a956a6 --- /dev/null +++ b/engines/bops_uploads/app/controllers/bops_uploads/blobs_controller.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module BopsUploads + class BlobsController < ApplicationController + def show + serve_file(blob_path, content_type:, disposition:) + rescue Errno::ENOENT + head :not_found + end + + private + + def blob_path + @service.path_for(@blob.key) + end + + def forcibly_serve_as_binary? + ActiveStorage.content_types_to_serve_as_binary.include?(@blob.content_type) + end + + def allowed_inline? + ActiveStorage.content_types_allowed_inline.include?(@blob.content_type) + end + + def content_type + forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : @blob.content_type + end + + def disposition + if forcibly_serve_as_binary? || !allowed_inline? + :attachment + else + :inline + end + end + + def serve_file(path, content_type:, disposition:) + ::Rack::Files.new(nil).serving(request, path).tap do |(status, headers, body)| + self.status = status + self.response_body = body + + headers.each do |name, value| + response.headers[name] = value + end + + response.headers.except!("X-Cascade", "x-cascade") if status == 416 + response.headers["Content-Type"] = content_type || DEFAULT_SEND_FILE_TYPE + response.headers["Content-Disposition"] = disposition || DEFAULT_SEND_FILE_DISPOSITION + end + end + end +end diff --git a/engines/bops_uploads/app/controllers/bops_uploads/files_controller.rb b/engines/bops_uploads/app/controllers/bops_uploads/files_controller.rb index 6f166dd015..21697e8abc 100644 --- a/engines/bops_uploads/app/controllers/bops_uploads/files_controller.rb +++ b/engines/bops_uploads/app/controllers/bops_uploads/files_controller.rb @@ -2,62 +2,45 @@ module BopsUploads class FilesController < ApplicationController - before_action :set_service - before_action :set_blob + before_action :set_document + before_action :set_planning_application def show - serve_file(blob_path, content_type:, disposition:) - rescue Errno::ENOENT - head :not_found - end - - private + signed_cookies.each do |key, value| + cookies[key] = { + value: value, + path: blob_path(@blob.key), + expires: expiry_time + } + end - def set_service - @service = ActiveStorage::Blob.service + redirect_to blob_url(@blob.key) end - def set_blob - @blob = ActiveStorage::Blob.find_by!(key: params[:key]) - end + private - def blob_path - @service.path_for(@blob.key) + def set_document + @document = current_local_authority.documents.find_by_blob!(key: @blob.key) end - def forcibly_serve_as_binary? - ActiveStorage.content_types_to_serve_as_binary.include?(@blob.content_type) + def set_planning_application + @planning_application = @document.planning_application end - def allowed_inline? - ActiveStorage.content_types_allowed_inline.include?(@blob.content_type) + def signed_cookies + cookie_signer.signed_cookie(url_to_be_signed, signing_options) end - def content_type - forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : @blob.content_type + def url_to_be_signed + blob_url(@blob.key) end - def disposition - if forcibly_serve_as_binary? || !allowed_inline? - :attachment - else - :inline - end + def signing_options + { expires: expiry_time } end - def serve_file(path, content_type:, disposition:) - ::Rack::Files.new(nil).serving(request, path).tap do |(status, headers, body)| - self.status = status - self.response_body = body - - headers.each do |name, value| - response.headers[name] = value - end - - response.headers.except!("X-Cascade", "x-cascade") if status == 416 - response.headers["Content-Type"] = content_type || DEFAULT_SEND_FILE_TYPE - response.headers["Content-Disposition"] = disposition || DEFAULT_SEND_FILE_DISPOSITION - end + def expiry_time + 10.minutes.from_now end end end diff --git a/engines/bops_uploads/app/controllers/bops_uploads/redirects_controller.rb b/engines/bops_uploads/app/controllers/bops_uploads/redirects_controller.rb deleted file mode 100644 index 58722d8cbf..0000000000 --- a/engines/bops_uploads/app/controllers/bops_uploads/redirects_controller.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -module BopsUploads - class RedirectsController < ApplicationController - def show - redirect_to redirect_url, allow_other_host: true - end - - private - - def redirect_url - file_url(params[:key], host: uploads_base_url) - end - - def uploads_base_url - Rails.configuration.uploads_base_url - end - end -end diff --git a/engines/bops_uploads/config/routes.rb b/engines/bops_uploads/config/routes.rb index 41805142ad..926a54363d 100644 --- a/engines/bops_uploads/config/routes.rb +++ b/engines/bops_uploads/config/routes.rb @@ -4,11 +4,12 @@ extend BopsCore::Routing local_authority_subdomain do - get "/files/:key", to: "redirects#show", as: "redirect" + get "/blobs/:key", to: "blobs#show", as: "blob" + get "/files/:key", to: "files#show", as: "file" end uploads_subdomain do - get "/:key", to: "files#show", as: "file" + get "/:key", to: "blobs#show", as: "upload" end end @@ -16,7 +17,11 @@ direct :uploaded_file do |blob, options| next "" if blob.blank? - bops_uploads.redirect_url(blob.key) + if Rails.configuration.use_signed_urls + bops_uploads.file_url(blob.key) + else + bops_uploads.upload_url(blob.key, host: Rails.configuration.uploads_base_url) + end end resolve("ActiveStorage::Attachment") { |attachment, options| route_for(:uploaded_file, attachment.blob, options) } diff --git a/engines/bops_uploads/lib/bops_uploads.rb b/engines/bops_uploads/lib/bops_uploads.rb index 658e16cdb4..79b5fdbc05 100644 --- a/engines/bops_uploads/lib/bops_uploads.rb +++ b/engines/bops_uploads/lib/bops_uploads.rb @@ -3,6 +3,10 @@ require "bops_uploads/engine" module BopsUploads + with_options instance_accessor: false do + mattr_accessor :key_pair_id, :private_key, :cookie_signer + end + class << self def env ActiveSupport::StringInquirer.new(ENV.fetch("BOPS_ENVIRONMENT", "development")) diff --git a/engines/bops_uploads/lib/bops_uploads/engine.rb b/engines/bops_uploads/lib/bops_uploads/engine.rb index b97e1c7e6e..130ff2e2f6 100644 --- a/engines/bops_uploads/lib/bops_uploads/engine.rb +++ b/engines/bops_uploads/lib/bops_uploads/engine.rb @@ -1,7 +1,27 @@ # frozen_string_literal: true +require "aws-sdk-cloudfront" + module BopsUploads class Engine < ::Rails::Engine isolate_namespace BopsUploads + + initializer "bops_uploads.cookie_signer" do |app| + config.after_initialize do + key_pair_id = ENV.fetch("UPLOADS_PUBLIC_KEY_ID") do + SecureRandom.alphanumeric(14).upcase + end + + private_key = ENV.fetch("UPLOADS_PRIVATE_KEY") do + OpenSSL::PKey::RSA.generate(2048).to_pem + end + + cookie_signer = Aws::CloudFront::CookieSigner.new(key_pair_id:, private_key:) + + BopsUploads.key_pair_id = key_pair_id + BopsUploads.private_key = OpenSSL::PKey::RSA.new(private_key) + BopsUploads.cookie_signer = cookie_signer + end + end end end diff --git a/engines/bops_uploads/spec/bops_uploads_helper.rb b/engines/bops_uploads/spec/bops_uploads_helper.rb index 32b68168e0..2329421d64 100644 --- a/engines/bops_uploads/spec/bops_uploads_helper.rb +++ b/engines/bops_uploads/spec/bops_uploads_helper.rb @@ -1,9 +1,3 @@ # frozen_string_literal: true require "rails_helper" - -RSpec.configure do |config| - config.before type: :request do - host!("uploads.bops.services") - end -end diff --git a/engines/bops_uploads/spec/helpers/url_helpers_spec.rb b/engines/bops_uploads/spec/helpers/url_helpers_spec.rb new file mode 100644 index 0000000000..086cb05ffd --- /dev/null +++ b/engines/bops_uploads/spec/helpers/url_helpers_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require "bops_uploads_helper" + +RSpec.describe "Generating urls" do + let(:config) { Rails.configuration } + let(:document) { create(:document) } + let(:blob) { document.file } + let(:key) { blob.key } + + before do + request.headers["HTTP_HOST"] = "southwark.bops.services" + allow(config).to receive(:uploads_base_url).and_return("http://uploads.bops.services") + end + + context "when use_signed_urls is false" do + before do + allow(config).to receive(:use_signed_urls).and_return(false) + end + + it "generates urls with the uploads subdomain" do + expect(helper.uploaded_file_url(blob)).to eq("http://uploads.bops.services/#{key}") + end + end + + context "when use_signed_urls is true" do + before do + allow(config).to receive(:use_signed_urls).and_return(true) + end + + it "generates urls with the local authority subdomain" do + expect(helper.uploaded_file_url(blob)).to eq("http://southwark.bops.services/files/#{key}") + end + end +end diff --git a/engines/bops_uploads/spec/requests/files_spec.rb b/engines/bops_uploads/spec/requests/files_spec.rb index a78d4e2fd2..26e8b5319d 100644 --- a/engines/bops_uploads/spec/requests/files_spec.rb +++ b/engines/bops_uploads/spec/requests/files_spec.rb @@ -3,36 +3,110 @@ require "bops_uploads_helper" RSpec.describe "Downloading files", show_exceptions: true do - context "when a blob exists" do - let(:document) { create(:document) } + context "on the uploads subdomain" do + before do + host!("uploads.bops.services") + end + + context "when a blob exists" do + let(:document) { create(:document) } + let(:blob) { document.file } + let(:key) { blob.key } + let(:path) { blob.service.path_for(blob.key) } + + it "returns 200 OK" do + get "/#{key}" + expect(response).to have_http_status(:ok) + expect(response.content_type).to eq("image/png") + end + + context "but the file is missing" do + before do + File.unlink(path) + end + + it "returns 404 Not Found" do + get "/#{key}" + expect(response).to have_http_status(:not_found) + end + end + end + + context "when a blob doesn't exist" do + let(:key) { SecureRandom.base36(28) } + + it "returns 404 Not Found" do + get "/#{key}" + expect(response).to have_http_status(:not_found) + end + end + end + + context "on the local authority subdomain" do + let!(:local_authority) { create(:local_authority, :southwark) } + let!(:subdomain) { local_authority.subdomain } + + let(:document) { create(:document, planning_application: planning_application) } let(:blob) { document.file } let(:key) { blob.key } - let(:path) { blob.service.path_for(blob.key) } - it "returns 200 OK" do - get "/#{key}" - expect(response).to have_http_status(:ok) - expect(response.content_type).to eq("image/png") + before do + host!("#{subdomain}.bops.services") + end + + context "when a blob doesn't exist" do + let(:key) { SecureRandom.base36(28) } + + before do + get "/files/#{key}" + end + + it "returns 404 Not Found" do + expect(response).to have_http_status(:not_found) + end end - context "but the file is missing" do + context "when a blob exists but for another local authority" do + let(:other_authority) { create(:local_authority, :lambeth) } + let(:planning_application) { create(:planning_application, local_authority: other_authority) } + before do - File.unlink(path) + get "/files/#{key}" end it "returns 404 Not Found" do - get "/#{key}" expect(response).to have_http_status(:not_found) end end - end - context "when a blob doesn't exist" do - let(:key) { SecureRandom.base36(28) } + context "when a blob exists for the local authority" do + let(:planning_application) { create(:planning_application, local_authority: local_authority) } + + before do + get "/files/#{key}" + end + + it "returns 302 Found" do + expect(response).to have_http_status(:found) + expect(response).to redirect_to("http://southwark.bops.services/blobs/#{key}") + end + + it "sets signed cookies" do + expect(cookies["CloudFront-Expires"]).to be_present + expect(cookies["CloudFront-Key-Pair-Id"]).to be_present + expect(cookies["CloudFront-Signature"]).to be_present + end - it "returns 404 Not Found" do - get "/#{key}" - expect(response).to have_http_status(:not_found) + context "and the redirect is followed" do + before do + follow_redirect! + end + + it "returns 200 OK" do + expect(response).to have_http_status(:ok) + expect(response.content_type).to eq("image/png") + end + end end end end diff --git a/spec/requests/api/document_show_spec.rb b/spec/requests/api/document_show_spec.rb index 6824d85306..40b1423043 100644 --- a/spec/requests/api/document_show_spec.rb +++ b/spec/requests/api/document_show_spec.rb @@ -45,7 +45,7 @@ it "redirects to blob url" do get "/api/v1/planning_applications/#{planning_application.reference}/documents/#{document.id}" - expect(response).to redirect_to("http://planx.example.com/files/#{document.blob.key}") + expect(response).to redirect_to("http://uploads.example.com/#{document.blob.key}") end end end diff --git a/spec/requests/api/oas3_spec.rb b/spec/requests/api/oas3_spec.rb index 987caabd97..bf4cc5846c 100644 --- a/spec/requests/api/oas3_spec.rb +++ b/spec/requests/api/oas3_spec.rb @@ -61,7 +61,7 @@ def example_response_hash_for(*) expected_response = example_response_hash_for("/api/v1/planning_applications/{id}", "get", 200, "ldc_proposed") expected_response["status"] = "in_assessment" expected_response["documents"].first["url"] = "http://planx.example.com/api/v1/planning_applications/#{planning_application.reference}/documents/#{planning_application_document.id}" - expected_response["documents"].first["blob_url"] = "http://planx.example.com/files/#{planning_application_document.representation.key}" + expected_response["documents"].first["blob_url"] = "http://uploads.example.com/#{planning_application_document.representation.key}" get("/api/v1/planning_applications/#{planning_application_hash["id"]}", headers: {"CONTENT-TYPE": "application/json", Authorization: "Bearer #{api_user.token}"}) expect(JSON.parse(response.body)).to eq(expected_response) diff --git a/spec/requests/api/planning_application_show_spec.rb b/spec/requests/api/planning_application_show_spec.rb index 18be534d4f..97e6f36307 100644 --- a/spec/requests/api/planning_application_show_spec.rb +++ b/spec/requests/api/planning_application_show_spec.rb @@ -145,7 +145,7 @@ planning_application_json["documents"].first.tap do |document_json| expect(document_json["url"]).to eq("http://planx.example.com/api/v1/planning_applications/#{planning_application.reference}/documents/#{document_with_number.id}") - expect(document_json["blob_url"]).to eq("http://planx.example.com/files/#{document_with_number.representation.key}") + expect(document_json["blob_url"]).to eq("http://uploads.example.com/#{document_with_number.representation.key}") expect(document_json["created_at"]).to eq(json_time_format(document_with_number.created_at)) expect(document_json["archived_at"]).to eq(json_time_format(document_with_number.archived_at)) expect(document_json["archive_reason"]).to eq(document_with_number.archive_reason) diff --git a/spec/requests/api/replacement_document_validation_request_spec.rb b/spec/requests/api/replacement_document_validation_request_spec.rb index 1ba3db9eaa..4a1ee1c657 100644 --- a/spec/requests/api/replacement_document_validation_request_spec.rb +++ b/spec/requests/api/replacement_document_validation_request_spec.rb @@ -83,7 +83,7 @@ old_document: { name: "proposed-floorplan.png", invalid_document_reason: "Document is invalid", - url: "http://planx.example.com/files/#{old_document.representation.key}" + url: "http://uploads.example.com/#{old_document.representation.key}" } }.deep_stringify_keys, { @@ -96,11 +96,11 @@ old_document: { name: "proposed-floorplan.png", invalid_document_reason: "Document is invalid", - url: "http://planx.example.com/files/#{old_document2.representation.key}" + url: "http://uploads.example.com/#{old_document2.representation.key}" }, new_document: { name: "proposed-floorplan.png", - url: "http://planx.example.com/files/#{new_document.representation.key}" + url: "http://uploads.example.com/#{new_document.representation.key}" } }.deep_stringify_keys, { @@ -113,7 +113,7 @@ old_document: { name: "proposed-floorplan.png", invalid_document_reason: nil, - url: "http://planx.example.com/files/#{old_document3.representation.key}" + url: "http://uploads.example.com/#{old_document3.representation.key}" } }.deep_stringify_keys ) @@ -172,7 +172,7 @@ old_document: { name: "proposed-floorplan.png", invalid_document_reason: "Document is invalid", - url: "http://planx.example.com/files/#{old_document.representation.key}" + url: "http://uploads.example.com/#{old_document.representation.key}" } }.deep_stringify_keys )