diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 51fbcc0235a..ea615b65554 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -169,6 +169,7 @@ en: default_categories_already_selected: "You cannot select a category used in another list." s3_upload_bucket_is_required: "You cannot enable uploads to S3 unless you've provided the 's3_upload_bucket'." s3_backup_requires_s3_settings: "You cannot use S3 as backup location unless you've provided the '%{setting_name}'." + s3_bucket_reused: "You cannot use the same bucket for 's3_upload_bucket' and 's3_backup_bucket'. Choose a different bucket or use a different path for each bucket." 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: @@ -190,7 +191,7 @@ en: error: "There was an error uploading that file. Please try again later." topic_invite: - failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." + failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}." user_exists: "Sorry, that user has already been invited. You may only invite a user to a topic once." backup: diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index 0d50f488b3d..2d03fefc3cf 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -62,4 +62,33 @@ module SiteSettings::Validations validate_error(:s3_backup_requires_s3_settings, setting_name: "s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? end end + + def validate_s3_upload_bucket(new_val) + validate_bucket_setting("s3_upload_bucket", new_val, SiteSetting.s3_backup_bucket) + end + + def validate_s3_backup_bucket(new_val) + validate_bucket_setting("s3_backup_bucket", SiteSetting.s3_upload_bucket, new_val) + end + + private + + def validate_bucket_setting(setting_name, upload_bucket, backup_bucket) + return if upload_bucket.blank? || backup_bucket.blank? + + backup_bucket_name, backup_prefix = split_s3_bucket(backup_bucket) + upload_bucket_name, upload_prefix = split_s3_bucket(upload_bucket) + + return if backup_bucket_name != upload_bucket_name + + if backup_prefix == upload_prefix || backup_prefix.blank? || upload_prefix&.start_with?(backup_prefix) + validate_error(:s3_bucket_reused, setting_name: setting_name) + end + end + + def split_s3_bucket(s3_bucket) + bucket_name, prefix = s3_bucket.downcase.split("/", 2) + prefix&.chomp!("/") + [bucket_name, prefix] + end end diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb new file mode 100644 index 00000000000..252099b4010 --- /dev/null +++ b/spec/lib/site_settings/validations_spec.rb @@ -0,0 +1,80 @@ +require 'rails_helper' +require 'site_settings/validations' + +describe SiteSettings::Validations do + subject { Class.new.include(described_class).new } + + context "s3 buckets reusage" do + let(:error_message) { I18n.t("errors.site_settings.s3_bucket_reused") } + + shared_examples "s3 bucket validation" do + def change_bucket_value(value) + SiteSetting.public_send("#{other_setting_name}=", value) + end + + it "shouldn't raise an error when both buckets are blank" do + change_bucket_value("") + expect { validate("") }.to_not raise_error + end + + it "shouldn't raise an error when only one bucket is set" do + change_bucket_value("") + expect { validate("my-awesome-bucket") }.to_not raise_error + end + + it "shouldn't raise an error when both buckets are equal, but use a different path" do + change_bucket_value("my-awesome-bucket/foo") + expect { validate("my-awesome-bucket/bar") }.to_not raise_error + end + + it "should raise an error when both buckets are equal" do + change_bucket_value("my-awesome-bucket") + expect { validate("my-awesome-bucket") }.to raise_error(Discourse::InvalidParameters, error_message) + end + + it "should raise an error when both buckets are equal except for a trailing slash" do + change_bucket_value("my-awesome-bucket/") + expect { validate("my-awesome-bucket") }.to raise_error(Discourse::InvalidParameters, error_message) + + change_bucket_value("my-awesome-bucket") + expect { validate("my-awesome-bucket/") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + + describe "#validate_s3_backup_bucket" do + let(:other_setting_name) { "s3_upload_bucket" } + + def validate(new_value) + subject.validate_s3_backup_bucket(new_value) + end + + it_behaves_like "s3 bucket validation" + + it "shouldn't raise an error when the 's3_backup_bucket' is a subdirectory of 's3_upload_bucket'" do + SiteSetting.s3_upload_bucket = "my-awesome-bucket" + expect { validate("my-awesome-bucket/backups") }.to_not raise_error + + SiteSetting.s3_upload_bucket = "my-awesome-bucket/foo" + expect { validate("my-awesome-bucket/foo/backups") }.to_not raise_error + end + end + + describe "#validate_s3_upload_bucket" do + let(:other_setting_name) { "s3_backup_bucket" } + + def validate(new_value) + subject.validate_s3_upload_bucket(new_value) + end + + it_behaves_like "s3 bucket validation" + + it "should raise an error when the 's3_upload_bucket' is a subdirectory of 's3_backup_bucket'" do + SiteSetting.s3_backup_bucket = "my-awesome-bucket" + expect { validate("my-awesome-bucket/uploads") }.to raise_error(Discourse::InvalidParameters, error_message) + + SiteSetting.s3_backup_bucket = "my-awesome-bucket/foo" + expect { validate("my-awesome-bucket/foo/uploads") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + end +end