From 02cb01406e6498588dddb5d2fc7acdca8d7bdcc4 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 20 Nov 2019 07:46:44 +1000 Subject: [PATCH] FIX: Allow secure uploads if global s3 setting active and enable_s3_uploads validations (#8373) The secure media functionality relied on `SiteSetting.enable_s3_uploads?` which, as we found in dev, did not take into account global S3 settings via `GlobalSetting.use_s3?`. We now use `SiteSetting.Upload.enable_s3_uploads` instead to be more consistent. Also, we now validate `enable_s3_uploads` changes, because if `GlobalSetting.use_s3?` is true users should NOT be enabling S3 uploads manually. --- .../regular/update_private_uploads_acl.rb | 2 +- config/locales/server.en.yml | 1 + lib/site_settings/validations.rb | 6 +- .../update_private_uploads_acl_spec.rb | 37 ++++++++ spec/lib/site_settings/validations_spec.rb | 90 ++++++++++++++++++- 5 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 spec/jobs/regular/update_private_uploads_acl_spec.rb diff --git a/app/jobs/regular/update_private_uploads_acl.rb b/app/jobs/regular/update_private_uploads_acl.rb index 4a95f5a5e40..661f84437c1 100644 --- a/app/jobs/regular/update_private_uploads_acl.rb +++ b/app/jobs/regular/update_private_uploads_acl.rb @@ -4,7 +4,7 @@ module Jobs class UpdatePrivateUploadsAcl < ::Jobs::Base # only runs when SiteSetting.prevent_anons_from_downloading_files is updated def execute(args) - return if !SiteSetting.enable_s3_uploads + return if !SiteSetting.Upload.enable_s3_uploads Upload.find_each do |upload| if !FileHelper.is_supported_media?(upload.original_filename) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 777895b7dd5..73eb81b5e99 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -208,6 +208,7 @@ en: secure_media_requirements: "S3 uploads must be enabled before enabling secure media." second_factor_cannot_be_enforced_with_disabled_local_login: "You cannot enforce 2FA if local logins are disabled." local_login_cannot_be_disabled_if_second_factor_enforced: "You cannot disable local login if 2FA is enforced. Disable enforced 2FA before disabling local logins." + cannot_enable_s3_uploads_when_s3_enabled_globally: "You cannot enable S3 uploads because S3 uploads are already globally enabled, and enabling this site-level could cause critical issues with uploads" conflicting_google_user_id: 'The Google Account ID for this account has changed; staff intervention is required for security reasons. Please contact staff and point them to
https://meta.discourse.org/t/76575' activemodel: diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index 65534e0c90f..a24da43db58 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -118,11 +118,13 @@ module SiteSettings::Validations end def validate_enable_s3_uploads(new_val) - validate_error :s3_upload_bucket_is_required if new_val == "t" && SiteSetting.s3_upload_bucket.blank? + return if new_val == "f" + validate_error :cannot_enable_s3_uploads_when_s3_enabled_globally if GlobalSetting.use_s3? + validate_error :s3_upload_bucket_is_required if SiteSetting.s3_upload_bucket.blank? end def validate_secure_media(new_val) - validate_error :secure_media_requirements if new_val == "t" && !SiteSetting.enable_s3_uploads? + validate_error :secure_media_requirements if new_val == "t" && !SiteSetting.Upload.enable_s3_uploads end def validate_enable_s3_inventory(new_val) diff --git a/spec/jobs/regular/update_private_uploads_acl_spec.rb b/spec/jobs/regular/update_private_uploads_acl_spec.rb new file mode 100644 index 00000000000..d80713d6323 --- /dev/null +++ b/spec/jobs/regular/update_private_uploads_acl_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::UpdatePrivateUploadsAcl do + let(:args) { [] } + + before do + SiteSetting.authorized_extensions = "pdf" + end + + describe '#execute' do + context "if not SiteSetting.Upload.enable_s3_uploads" do + before do + SiteSetting.Upload.stubs(:enable_s3_uploads).returns(false) + end + it "returns early and changes no uploads" do + Upload.expects(:find_each).never + subject.execute(args) + end + end + context "if SiteSetting.Upload.enable_s3_uploads" do + let!(:upload) { Fabricate(:upload_s3, extension: 'pdf', original_filename: "watchmen.pdf", secure: false) } + before do + SiteSetting.login_required = true + SiteSetting.prevent_anons_from_downloading_files = true + SiteSetting::Upload.stubs(:enable_s3_uploads).returns(true) + Discourse.stubs(:store).returns(stub(external?: false)) + end + + it "changes the upload to secure" do + subject.execute(args) + expect(upload.reload.secure).to eq(true) + end + end + end +end diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb index 6660872806d..7f346d4d5cb 100644 --- a/spec/lib/site_settings/validations_spec.rb +++ b/spec/lib/site_settings/validations_spec.rb @@ -125,7 +125,7 @@ describe SiteSettings::Validations do end it "should be ok" do - expect { subject.validate_enforce_second_factor("t") }.not_to raise_error(Discourse::InvalidParameters, error_message) + expect { subject.validate_enforce_second_factor("t") }.not_to raise_error end end end @@ -150,14 +150,98 @@ describe SiteSettings::Validations do end it "should be ok" do - expect { subject.validate_enable_local_logins("f") }.not_to raise_error(Discourse::InvalidParameters, error_message) + expect { subject.validate_enable_local_logins("f") }.not_to raise_error end end end context "when the new value is true" do it "should be ok" do - expect { subject.validate_enable_local_logins("t") }.not_to raise_error(Discourse::InvalidParameters, error_message) + expect { subject.validate_enable_local_logins("t") }.not_to raise_error + end + end + end + + describe "#validate_secure_media" do + let(:error_message) { I18n.t("errors.site_settings.secure_media_requirements") } + + context "when the new value is true" do + context 'if site setting for enable_s3_uploads is enabled' do + before do + SiteSetting.enable_s3_uploads = true + end + + it "should be ok" do + expect { subject.validate_secure_media("t") }.not_to raise_error + end + end + + context 'if site setting for enable_s3_uploads is not enabled' do + before do + SiteSetting.enable_s3_uploads = false + end + + it "is not ok" do + expect { subject.validate_secure_media("t") }.to raise_error(Discourse::InvalidParameters, error_message) + end + + context "if global s3 setting is enabled" do + before do + GlobalSetting.stubs(:use_s3?).returns(true) + end + + it "should be ok" do + expect { subject.validate_secure_media("t") }.not_to raise_error + end + end + end + end + end + + describe "#validate_enable_s3_uploads" do + let(:error_message) { I18n.t("errors.site_settings.cannot_enable_s3_uploads_when_s3_enabled_globally") } + + context "when the new value is true" do + context "when s3 uploads are already globally enabled" do + before do + GlobalSetting.stubs(:use_s3?).returns(true) + end + + it "is not ok" do + expect { subject.validate_enable_s3_uploads("t") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + + context "when s3 uploads are not already globally enabled" do + before do + GlobalSetting.stubs(:use_s3?).returns(false) + end + + it "should be ok" do + expect { subject.validate_enable_s3_uploads("t") }.not_to raise_error + end + end + + context "when the s3_upload_bucket is blank" do + let(:error_message) { I18n.t("errors.site_settings.s3_upload_bucket_is_required") } + + before do + SiteSetting.s3_upload_bucket = nil + end + + it "is not ok" do + expect { subject.validate_enable_s3_uploads("t") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + + context "when the s3_upload_bucket is not blank" do + before do + SiteSetting.s3_upload_bucket = "some-bucket" + end + + it "should be ok" do + expect { subject.validate_enable_s3_uploads("t") }.not_to raise_error + end end end end