Merge branch '16240-fix-project-bot-captcha' into 'master'

Resolve "Project bots can get CAPTCHA even on a private project"

See merge request !65661

app/models/issue.rb

@@ -426,8 +426,15 @@ def visible_to_user?(user = nil)
   end
 
   def check_for_spam?
-    publicly_visible? &&
-      (title_changed? || description_changed? || confidential_changed?)
+    # content created via support bots is always checked for spam, EVEN if
+    # the issue is not publicly visible and/or confidential
+    return true if author.support_bot? && spammable_attribute_changed?
+
+    # Only check for spam on issues which are publicly visible (and thus indexed in search engines)
+    return false unless publicly_visible?
+
+    # Only check for spam if certain attributes have changed
+    spammable_attribute_changed?
   end
 
   def as_json(options = {})
@@ -515,6 +522,14 @@ def issue_assignee_user_ids
 
   private
 
+  def spammable_attribute_changed?
+    title_changed? ||
+      description_changed? ||
+      # NOTE: We need to check them for spam when issues are made non-confidential, because spam
+      # may have been added while they were confidential and thus not being checked for spam.
+      confidential_changed?(from: true, to: false)
+  end
+
   # Ensure that the metrics association is safely created and respecting the unique constraint on issue_id
   override :ensure_metrics
   def ensure_metrics

ee/app/models/ee/issue.rb

@@ -91,11 +91,6 @@ def use_separate_indices?
       end
     end
 
-    # override
-    def check_for_spam?
-      author.bot? && (title_changed? || description_changed? || confidential_changed?) || super
-    end
-
     # override
     def allows_multiple_assignees?
       project.feature_available?(:multiple_issue_assignees)

ee/spec/models/issue_spec.rb

@@ -453,38 +453,6 @@
     end
   end
 
-  describe '#check_for_spam?' do
-    using RSpec::Parameterized::TableSyntax
-    let_it_be(:reusable_project) { create(:project) }
-    let_it_be(:author) { ::User.support_bot }
-
-    where(:visibility_level, :confidential, :new_attributes, :check_for_spam?) do
-      Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | true  | { confidential: false } | true
-      Gitlab::VisibilityLevel::PUBLIC   | true  | { description: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'woo', confidential: true } | true
-      Gitlab::VisibilityLevel::INTERNAL | false | { description: 'woo' } | true
-      Gitlab::VisibilityLevel::PRIVATE  | true  | { description: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'original description' } | false
-      Gitlab::VisibilityLevel::PRIVATE  | true  | { weight: 3 } | false
-    end
-
-    with_them do
-      context 'when author is a bot' do
-        it 'only checks for spam when description, title, or confidential status is updated' do
-          project = reusable_project
-          project.update(visibility_level: visibility_level)
-          issue = create(:issue, project: project, confidential: confidential, description: 'original description', author: author)
-
-          issue.assign_attributes(new_attributes)
-
-          expect(issue.check_for_spam?).to eq(check_for_spam?)
-        end
-      end
-    end
-  end
-
   describe '#weight' do
     where(:license_value, :database_value, :expected) do
       true  | 5   | 5

spec/models/issue_spec.rb

@@ -1051,23 +1051,53 @@
 
   describe '#check_for_spam?' do
     using RSpec::Parameterized::TableSyntax
-
-    where(:visibility_level, :confidential, :new_attributes, :check_for_spam?) do
-      Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'woo' } | true
-      Gitlab::VisibilityLevel::PUBLIC   | true  | { confidential: false } | true
-      Gitlab::VisibilityLevel::PUBLIC   | true  | { description: 'woo' } | false
-      Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'woo', confidential: true } | false
-      Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'original description' } | false
-      Gitlab::VisibilityLevel::INTERNAL | false | { description: 'woo' } | false
-      Gitlab::VisibilityLevel::PRIVATE  | false | { description: 'woo' } | false
+    let_it_be(:support_bot) { ::User.support_bot }
+
+    where(:support_bot?, :visibility_level, :confidential, :new_attributes, :check_for_spam?) do
+      ### non-support-bot cases
+      # spammable attributes changing
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'new' } | true
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'new' } | true
+      # confidential to non-confidential
+      false | Gitlab::VisibilityLevel::PUBLIC   | true  | { confidential: false } | true
+      # non-confidential to confidential
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { confidential: true } | false
+      # spammable attributes changing on confidential
+      false | Gitlab::VisibilityLevel::PUBLIC   | true  | { description: 'new' } | false
+      # spammable attributes changing while changing to confidential
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'new', confidential: true } | false
+      # spammable attribute not changing
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'original description' } | false
+      # non-spammable attribute changing
+      false | Gitlab::VisibilityLevel::PUBLIC   | false | { weight: 3 } | false
+      # spammable attributes changing on non-public
+      false | Gitlab::VisibilityLevel::INTERNAL | false | { description: 'new' } | false
+      false | Gitlab::VisibilityLevel::PRIVATE  | false | { description: 'new' } | false
+
+      ### support-bot cases
+      # confidential to non-confidential
+      true | Gitlab::VisibilityLevel::PUBLIC    | true  | { confidential: false } | true
+      # non-confidential to confidential
+      true | Gitlab::VisibilityLevel::PUBLIC    | false | { confidential: true } | false
+      # spammable attributes changing on confidential
+      true  | Gitlab::VisibilityLevel::PUBLIC   | true  | { description: 'new' } | true
+      # spammable attributes changing while changing to confidential
+      true  | Gitlab::VisibilityLevel::PUBLIC   | false | { title: 'new', confidential: true } | true
+      # spammable attributes changing on non-public
+      true  | Gitlab::VisibilityLevel::INTERNAL | false | { description: 'new' } | true
+      true  | Gitlab::VisibilityLevel::PRIVATE  | false | { title: 'new' } | true
+      # spammable attribute not changing
+      true  | Gitlab::VisibilityLevel::PUBLIC   | false | { description: 'original description' } | false
+      # non-spammable attribute changing
+      true  | Gitlab::VisibilityLevel::PRIVATE  | true  | { weight: 3 } | false
     end
 
     with_them do
-      it 'checks for spam on issues that can be seen anonymously' do
+      it 'checks for spam when necessary' do
+        author = support_bot? ? support_bot : user
         project = reusable_project
         project.update!(visibility_level: visibility_level)
-        issue = create(:issue, project: project, confidential: confidential, description: 'original description')
+        issue = create(:issue, project: project, confidential: confidential, description: 'original description', author: author)
 
         issue.assign_attributes(new_attributes)