From 00c8f520e90f35dfed9f8769c99b4ab14dc0fc0d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 6 Nov 2020 10:33:19 +1000 Subject: [PATCH] FIX: Do not enable published page if secure media enabled (#11131) There are issues around displaying images on published pages when secure media is enabled. This PR temporarily makes it appear as if published pages are enabled if secure media is also enabled. --- app/controllers/published_pages_controller.rb | 4 +++- app/serializers/topic_view_serializer.rb | 5 ++++- config/locales/server.en.yml | 1 + lib/guardian.rb | 3 ++- lib/site_settings/validations.rb | 4 ++++ spec/components/guardian_spec.rb | 14 ++++++++++++++ spec/lib/site_settings/validations_spec.rb | 19 +++++++++++++++++++ .../published_pages_controller_spec.rb | 12 ++++++++++++ .../serializers/topic_view_serializer_spec.rb | 12 ++++++++++++ spec/support/uploads_helpers.rb | 5 +++++ 10 files changed, 76 insertions(+), 3 deletions(-) diff --git a/app/controllers/published_pages_controller.rb b/app/controllers/published_pages_controller.rb index a51776ada6c..9c3f4ccdd81 100644 --- a/app/controllers/published_pages_controller.rb +++ b/app/controllers/published_pages_controller.rb @@ -92,7 +92,9 @@ private end def ensure_publish_enabled - raise Discourse::NotFound unless SiteSetting.enable_page_publishing? + if !SiteSetting.enable_page_publishing? || SiteSetting.secure_media + raise Discourse::NotFound + end end def enforce_login_required! diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index d897b34ec85..5be4ccc4c05 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -275,7 +275,10 @@ class TopicViewSerializer < ApplicationSerializer end def include_published_page? - SiteSetting.enable_page_publishing? && scope.is_staff? && object.published_page.present? + SiteSetting.enable_page_publishing? && + scope.is_staff? && + object.published_page.present? && + !SiteSetting.secure_media end def thumbnails diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5cc3a78e476..ededb92aaf7 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -190,6 +190,7 @@ en: default_tags_already_selected: "You cannot select a tag used in another list." s3_upload_bucket_is_required: "You cannot enable uploads to S3 unless you've provided the 's3_upload_bucket'." enable_s3_uploads_is_required: "You cannot enable inventory to S3 unless you've enabled the S3 uploads." + page_publishing_requirements: "Page publishing cannot be enabled if secure media is enabled." 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." secure_media_requirements: "S3 uploads must be enabled before enabling secure media." diff --git a/lib/guardian.rb b/lib/guardian.rb index fd594077aff..ef8319e7a34 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -523,7 +523,8 @@ class Guardian end def can_publish_page?(topic) - return false unless SiteSetting.enable_page_publishing? + return false if !SiteSetting.enable_page_publishing? + return false if SiteSetting.secure_media? return false if topic.blank? return false if topic.private_message? return false unless can_see_topic?(topic) diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index 6d132195f8d..81090bfe1c1 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -144,6 +144,10 @@ module SiteSettings::Validations validate_error :secure_media_requirements if new_val == "t" && !SiteSetting.Upload.enable_s3_uploads end + def validate_enable_page_publishing(new_val) + validate_error :page_publishing_requirements if new_val == "t" && SiteSetting.secure_media? + end + def validate_share_quote_buttons(new_val) validate_error :share_quote_facebook_requirements if new_val.include?("facebook") && SiteSetting.facebook_app_id.blank? end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 1d54d690b27..cf94a4b1a7d 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -3756,6 +3756,20 @@ describe Guardian do post = Fabricate(:private_message_post, user: admin) expect(Guardian.new(admin).can_publish_page?(post.topic)).to eq(false) end + + context "when secure_media is also enabled" do + before do + setup_s3 + SiteSetting.secure_media = true + end + + it "is false for everyone" do + expect(Guardian.new(moderator).can_publish_page?(topic)).to eq(false) + expect(Guardian.new(user).can_publish_page?(topic)).to eq(false) + expect(Guardian.new.can_publish_page?(topic)).to eq(false) + expect(Guardian.new(admin).can_publish_page?(topic)).to eq(false) + end + end end end end diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb index 87ee9b99b6c..4f8358ff958 100644 --- a/spec/lib/site_settings/validations_spec.rb +++ b/spec/lib/site_settings/validations_spec.rb @@ -205,6 +205,25 @@ describe SiteSettings::Validations do end end + describe "#validate_enable_page_publishing" do + context "when the new value is true" do + it "is ok" do + expect { subject.validate_enable_page_publishing("t") }.not_to raise_error + end + + context "if secure media is enabled" do + let(:error_message) { I18n.t("errors.site_settings.page_publishing_requirements") } + before do + enable_secure_media + end + + it "is not ok" do + expect { subject.validate_enable_page_publishing("t") }.to raise_error(Discourse::InvalidParameters, error_message) + end + end + end + end + describe "#validate_secure_media" do let(:error_message) { I18n.t("errors.site_settings.secure_media_requirements") } diff --git a/spec/requests/published_pages_controller_spec.rb b/spec/requests/published_pages_controller_spec.rb index 5aa14705155..2dde8b94148 100644 --- a/spec/requests/published_pages_controller_spec.rb +++ b/spec/requests/published_pages_controller_spec.rb @@ -93,6 +93,18 @@ RSpec.describe PublishedPagesController do published_page.topic.tags = [Fabricate(:tag, name: "recipes")] end + context "when secure media is enabled" do + before do + setup_s3 + SiteSetting.secure_media = true + end + + it "returns 404" do + get published_page.path + expect(response.status).to eq(404) + end + end + it "returns 200" do get published_page.path expect(response.status).to eq(200) diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 10a433142c3..421b65ce371 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -399,6 +399,18 @@ describe TopicViewSerializer do expect(json[:published_page]).to be_present expect(json[:published_page][:slug]).to eq(published_page.slug) end + + context "secure media is enabled" do + before do + setup_s3 + SiteSetting.secure_media = true + end + + it "doesn't return the published page" do + json = serialize_topic(topic, admin) + expect(json[:published_page]).to be_blank + end + end end end end diff --git a/spec/support/uploads_helpers.rb b/spec/support/uploads_helpers.rb index 583c26e0c6d..35c57fc940b 100644 --- a/spec/support/uploads_helpers.rb +++ b/spec/support/uploads_helpers.rb @@ -13,6 +13,11 @@ module UploadsHelpers stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/") end + def enable_secure_media + setup_s3 + SiteSetting.secure_media = true + end + def stub_upload(upload) url = "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/original/1X/#{upload.sha1}.#{upload.extension}?acl" stub_request(:put, url)