From c5a024f8dfcd0611f3661b59b0b7c8d5bc72fac8 Mon Sep 17 00:00:00 2001
From: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
Date: Mon, 30 Sep 2024 09:17:19 +1000
Subject: [PATCH] FIX: custom flag name should be unique (#28869)

Validation to ensure that the custom flag name is unique.
---
 app/controllers/admin/config/flags_controller.rb | 2 ++
 app/services/flags/create_flag.rb                | 5 +++++
 app/services/flags/update_flag.rb                | 5 +++++
 config/locales/server.en.yml                     | 1 +
 spec/services/flags/create_flag_spec.rb          | 9 +++++++++
 spec/services/flags/update_flag_spec.rb          | 9 +++++++++
 6 files changed, 31 insertions(+)

diff --git a/app/controllers/admin/config/flags_controller.rb b/app/controllers/admin/config/flags_controller.rb
index 2389be1127b..7799e186c26 100644
--- a/app/controllers/admin/config/flags_controller.rb
+++ b/app/controllers/admin/config/flags_controller.rb
@@ -33,6 +33,7 @@ class Admin::Config::FlagsController < Admin::AdminController
       end
       on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_policy(:unique_name) { render_json_error(I18n.t("flags.errors.unique_name")) }
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
       end
@@ -50,6 +51,7 @@ class Admin::Config::FlagsController < Admin::AdminController
       on_failed_policy(:not_system) { render_json_error(I18n.t("flags.errors.system")) }
       on_failed_policy(:not_used) { render_json_error(I18n.t("flags.errors.used")) }
       on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_policy(:unique_name) { render_json_error(I18n.t("flags.errors.unique_name")) }
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
       end
diff --git a/app/services/flags/create_flag.rb b/app/services/flags/create_flag.rb
index dab3e8c13b8..09addd884ec 100644
--- a/app/services/flags/create_flag.rb
+++ b/app/services/flags/create_flag.rb
@@ -5,6 +5,7 @@ class Flags::CreateFlag
 
   contract
   policy :invalid_access
+  policy :unique_name
   model :flag, :instantiate_flag
 
   transaction do
@@ -27,6 +28,10 @@ class Flags::CreateFlag
 
   private
 
+  def unique_name(name:)
+    !Flag.custom.where(name: name).exists?
+  end
+
   def instantiate_flag(name:, description:, applies_to:, require_message:, enabled:)
     Flag.new(
       name: name,
diff --git a/app/services/flags/update_flag.rb b/app/services/flags/update_flag.rb
index b04e0e99b2e..eaaee969d15 100644
--- a/app/services/flags/update_flag.rb
+++ b/app/services/flags/update_flag.rb
@@ -8,6 +8,7 @@ class Flags::UpdateFlag
   policy :not_system
   policy :not_used
   policy :invalid_access
+  policy :unique_name
 
   transaction do
     step :update
@@ -29,6 +30,10 @@ class Flags::UpdateFlag
 
   private
 
+  def unique_name(id:, name:)
+    !Flag.custom.where(name: name).where.not(id: id).exists?
+  end
+
   def fetch_flag(id:)
     Flag.find(id)
   end
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index e9d931077f3..4db03475fa0 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1270,6 +1270,7 @@ en:
       wrong_move: "Flag cannot be moved"
       system: "System flag cannot be updated or deleted."
       used: "Flag cannot be updated or deleted because has already been used."
+      unique_name: "Flag name must be unique"
   reports:
     default:
       labels:
diff --git a/spec/services/flags/create_flag_spec.rb b/spec/services/flags/create_flag_spec.rb
index 620cd9bbce6..effeeba3e8b 100644
--- a/spec/services/flags/create_flag_spec.rb
+++ b/spec/services/flags/create_flag_spec.rb
@@ -24,6 +24,15 @@ RSpec.describe(Flags::CreateFlag) do
     it { is_expected.to fail_a_policy(:invalid_access) }
   end
 
+  context "when title is not unique" do
+    fab!(:current_user) { Fabricate(:admin) }
+    fab!(:flag) { Fabricate(:flag, name: "custom flag name") }
+
+    it { is_expected.to fail_a_policy(:unique_name) }
+
+    after { Flag.destroy_by(name: "custom flag name") }
+  end
+
   context "when applies to is invalid" do
     fab!(:current_user) { Fabricate(:admin) }
     let(:applies_to) { ["User"] }
diff --git a/spec/services/flags/update_flag_spec.rb b/spec/services/flags/update_flag_spec.rb
index 3fb7faacf15..f5434277038 100644
--- a/spec/services/flags/update_flag_spec.rb
+++ b/spec/services/flags/update_flag_spec.rb
@@ -43,6 +43,15 @@ RSpec.describe(Flags::UpdateFlag) do
     it { is_expected.to fail_a_contract }
   end
 
+  context "when title is not unique" do
+    fab!(:current_user) { Fabricate(:admin) }
+    fab!(:flag_2) { Fabricate(:flag, name: "edited custom flag name") }
+
+    it { is_expected.to fail_a_policy(:unique_name) }
+
+    after { Flag.destroy_by(name: "edited custom flag name") }
+  end
+
   context "when title is too long" do
     fab!(:current_user) { Fabricate(:admin) }
     let(:name) { "a" * 201 }