From 3a38c0721133175b2bd0073ec1e7a84b9c6e178c Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Tue, 28 Apr 2020 17:16:47 -0400 Subject: [PATCH] Revert "Set a public ACL for files uploaded to a public GCS service" This reverts commit 43503bdfecb86ed7386eecc54a75ccf3744b5dc2. --- activestorage/CHANGELOG.md | 15 ------ .../lib/active_storage/service/gcs_service.rb | 10 ++-- .../test/service/gcs_public_service_test.rb | 28 ----------- .../test/service/gcs_service_test.rb | 47 ------------------- guides/source/active_storage_overview.md | 20 +------- 5 files changed, 4 insertions(+), 116 deletions(-) diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index cffb2b26ce..8e43acc97e 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,18 +1,3 @@ -* Add support for `upload` options with `GCSService`. - - For example, to add `Cache-Control` headers to uploaded files, modify - `config/storage.yml` with the `upload` key and corresponding Hash: - - ```yml - google: - service: GCS - ... - upload: - cache_control: "public, max-age=60" - ``` - - *Brendan Abbott* - * Add `config.active_storage.web_image_content_types` to allow applications to add content types (like `image/webp`) in which variants can be processed, instead of letting those images be converted to the fallback PNG format. diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb index ec63a55a44..f166aee07e 100644 --- a/activestorage/lib/active_storage/service/gcs_service.rb +++ b/activestorage/lib/active_storage/service/gcs_service.rb @@ -7,13 +7,9 @@ module ActiveStorage # Wraps the Google Cloud Storage as an Active Storage service. See ActiveStorage::Service for the generic API # documentation that applies to all services. class Service::GCSService < Service - attr_reader :upload_options - - def initialize(public: false, upload: {}, **config) + def initialize(public: false, **config) @config = config @public = public - @upload_options = upload - @upload_options[:acl] = "public_read" if public? end def upload(key, io, checksum: nil, content_type: nil, disposition: nil, filename: nil) @@ -23,7 +19,7 @@ def upload(key, io, checksum: nil, content_type: nil, disposition: nil, filename # binary and attachment when the file's content type requires it. The only way to force them is to # store them as object's metadata. content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename - bucket.create_file(io, key, md5: checksum, content_type: content_type, content_disposition: content_disposition, **upload_options) + bucket.create_file(io, key, md5: checksum, content_type: content_type, content_disposition: content_disposition) rescue Google::Cloud::InvalidArgumentError raise ActiveStorage::IntegrityError end @@ -88,7 +84,7 @@ def exist?(key) def url_for_direct_upload(key, expires_in:, checksum:, **) instrument :url, key: key do |payload| - generated_url = bucket.signed_url key, method: "PUT", expires: expires_in, content_md5: checksum, **upload_options + generated_url = bucket.signed_url key, method: "PUT", expires: expires_in, content_md5: checksum payload[:url] = generated_url diff --git a/activestorage/test/service/gcs_public_service_test.rb b/activestorage/test/service/gcs_public_service_test.rb index 759546b37d..9ab440122d 100644 --- a/activestorage/test/service/gcs_public_service_test.rb +++ b/activestorage/test/service/gcs_public_service_test.rb @@ -9,34 +9,6 @@ class ActiveStorage::Service::GCSPublicServiceTest < ActiveSupport::TestCase include ActiveStorage::Service::SharedServiceTests - test "public acl options" do - assert_equal "public_read", @service.upload_options[:acl] - end - - test "uploaded file is accessible by all users" do - assert_includes @service.bucket.find_file(@key).acl.readers, "allUsers" - end - - test "direct upload file is accessible by all users" do - key = SecureRandom.base58(24) - data = "Something else entirely!" - checksum = Digest::MD5.base64digest(data) - url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size, checksum: checksum) - - uri = URI.parse url - request = Net::HTTP::Put.new uri.request_uri - request.body = data - request.add_field "Content-Type", "" - request.add_field "Content-MD5", checksum - Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http| - http.request request - end - - assert_includes @service.bucket.find_file(key).acl.readers, "allUsers" - ensure - @service.delete key - end - test "public URL generation" do url = @service.url(@key, filename: ActiveStorage::Filename.new("avatar.png")) diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb index 57fb8f68c1..4a98237277 100644 --- a/activestorage/test/service/gcs_service_test.rb +++ b/activestorage/test/service/gcs_service_test.rb @@ -57,33 +57,6 @@ class ActiveStorage::Service::GCSServiceTest < ActiveSupport::TestCase @service.delete key end - test "direct upload with custom upload options" do - cache_control = "public, max-age=60" - service = build_service(upload: { cache_control: cache_control }) - - key = SecureRandom.base58(24) - data = "Something else entirely!" - checksum = Digest::MD5.base64digest(data) - url = service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size, checksum: checksum) - - uri = URI.parse url - request = Net::HTTP::Put.new uri.request_uri - request.body = data - service.headers_for_direct_upload(key, checksum: checksum, filename: ActiveStorage::Filename.new("test.txt"), disposition: :attachment).each do |k, v| - request.add_field k, v - end - request.add_field "Content-Type", "" - Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http| - http.request request - end - - url = service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) - response = Net::HTTP.get_response(URI(url)) - assert_equal(cache_control, response["Cache-Control"]) - ensure - service.delete key - end - test "upload with content_type and content_disposition" do key = SecureRandom.base58(24) data = "Something else entirely!" @@ -112,21 +85,6 @@ class ActiveStorage::Service::GCSServiceTest < ActiveSupport::TestCase @service.delete key end - test "upload with custom upload options" do - key = SecureRandom.base58(24) - data = "Something else entirely!" - cache_control = "public, max-age=60" - service = build_service(upload: { cache_control: cache_control }) - - begin - service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") - - assert_equal cache_control, service.bucket.find_file(key).cache_control - ensure - service.delete key - end - end - test "update metadata" do key = SecureRandom.base58(24) data = "Something else entirely!" @@ -146,11 +104,6 @@ class ActiveStorage::Service::GCSServiceTest < ActiveSupport::TestCase assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/, @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) end - - private - def build_service(configuration) - ActiveStorage::Service.configure(:gcs, SERVICE_CONFIGURATIONS.deep_merge(gcs: configuration)) - end end else puts "Skipping GCS Service tests because no GCS configuration was supplied" diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 29e002fe81..f5d2521778 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -118,7 +118,6 @@ amazon: secret_access_key: "" region: "" bucket: "" - public: false ``` Optionally provide a Hash of upload options: @@ -130,7 +129,7 @@ amazon: secret_access_key: "" region: "" bucket: "" - upload: + upload: server_side_encryption: "" # 'aws:kms' or 'AES256' ``` @@ -177,7 +176,6 @@ google: credentials: <%= Rails.root.join("path/to/keyfile.json") %> project: "" bucket: "" - public: false ``` Optionally provide a Hash of credentials instead of a keyfile path: @@ -200,22 +198,6 @@ google: bucket: "" ``` -Optionally provide a Hash of upload options: - -```yaml -google: - service: GCS - credentials: <%= Rails.root.join("path/to/keyfile.json") %> - project: "" - bucket: "" - upload: - acl: "" # will be set to `public_read` on public buckets - cache_control: "" - storage_class: "" -``` - -The [Google Cloud Storage SDK docs](https://googleapis.dev/ruby/google-cloud-storage/latest/Google/Cloud/Storage/Bucket.html#create_file-instance_method) detail other possible upload options. - Add the [`google-cloud-storage`](https://github.com/GoogleCloudPlatform/google-cloud-ruby/tree/master/google-cloud-storage) gem to your `Gemfile`: ```ruby