From 9f0f801ae35162bf71933101b0818d464ac72754 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Tue, 16 Feb 2021 12:34:03 +1000
Subject: [PATCH] FIX: Do not mark group_flair images as secure on upload
 (#12081)

See https://meta.discourse.org/t/secure-media-uploads-breaks-group-flair-image/173671/4

Group flair image uploads definitely do not need to be secure.
---
 .../components/group-flair-inputs.hbs         |  8 ++---
 ...p_flair_avatar_upload_security_and_acls.rb | 34 +++++++++++++++++++
 lib/upload_security.rb                        |  1 +
 spec/lib/upload_security_spec.rb              |  6 ++++
 4 files changed, 45 insertions(+), 4 deletions(-)
 create mode 100644 db/post_migrate/20210215231312_fix_group_flair_avatar_upload_security_and_acls.rb

diff --git a/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs b/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
index f198b7bb636..94f140f845b 100644
--- a/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
+++ b/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
@@ -2,13 +2,13 @@
   <label class="control-label" for="flair_url">{{i18n "groups.flair_url"}}</label>
 
   <div class="radios">
-    <label class="radio-label" for="avatar-flair">
-      {{radio-button name="avatar-flair" value="icon" selection=model.flair_type}}
+    <label class="radio-label" for="avatar-flair-icon">
+      {{radio-button name="avatar-flair-icon" id="avatar-flair-icon" value="icon" selection=model.flair_type}}
       <b>{{i18n "groups.flair_type.icon"}}</b>
     </label>
 
-    <label class="radio-label" for="avatar-flair">
-      {{radio-button name="avatar-flair" value="image" selection=model.flair_type}}
+    <label class="radio-label" for="avatar-flair-image">
+      {{radio-button name="avatar-flair-image" id="avatar-flair-image" value="image" selection=model.flair_type}}
       <b>{{i18n "groups.flair_type.image"}}</b>
     </label>
   </div>
diff --git a/db/post_migrate/20210215231312_fix_group_flair_avatar_upload_security_and_acls.rb b/db/post_migrate/20210215231312_fix_group_flair_avatar_upload_security_and_acls.rb
new file mode 100644
index 00000000000..3232bacb6fb
--- /dev/null
+++ b/db/post_migrate/20210215231312_fix_group_flair_avatar_upload_security_and_acls.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+class FixGroupFlairAvatarUploadSecurityAndAcls < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  def up
+    upload_ids = DB.query_single(<<~SQL
+      SELECT flair_upload_id
+      FROM groups
+      WHERE flair_upload_id IS NOT NULL
+     SQL
+    )
+
+    if upload_ids.any?
+      reason = "group_flair fixup migration"
+      DB.exec(<<~SQL, upload_ids: upload_ids, reason: reason, now: Time.zone.now)
+        UPDATE uploads SET secure = false, security_last_changed_at = :now, updated_at = :now, security_last_changed_reason = :reason
+        WHERE id IN (:upload_ids) AND uploads.secure
+      SQL
+
+      if Discourse.store.external?
+        uploads = Upload.where(id: upload_ids, secure: false).where("updated_at = security_last_changed_at")
+        uploads.each do |upload|
+          Discourse.store.update_upload_ACL(upload)
+          upload.touch
+        end
+      end
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index 9af270de917..6ec2bbd5bd2 100644
--- a/lib/upload_security.rb
+++ b/lib/upload_security.rb
@@ -25,6 +25,7 @@ class UploadSecurity
     card_background
     category_logo
     category_background
+    group_flair
   ]
 
   def self.register_custom_public_type(type)
diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb
index 12fced51630..07e2b544022 100644
--- a/spec/lib/upload_security_spec.rb
+++ b/spec/lib/upload_security_spec.rb
@@ -25,6 +25,12 @@ RSpec.describe UploadSecurity do
       end
 
       context "when uploading in public context" do
+        describe "for a public type group_flair" do
+          let(:type) { 'group_flair' }
+          it "returns false" do
+            expect(subject.should_be_secure?).to eq(false)
+          end
+        end
         describe "for a public type avatar" do
           let(:type) { 'avatar' }
           it "returns false" do