Commit 427224db authored by Stan Hu's avatar Stan Hu

Fix spam checking to consider updater of issue instead of author

When an issue or some other issauble is updated, we previously would
check whether the original author was the support bot instead of the
user who updated this issue. As a result, the spam check would be
triggered unnecessarily and causing spam logs to be attributed to the
support bot.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/335971

Changelog: fixed
parent dcd8e7ab
...@@ -111,7 +111,7 @@ module Spammable ...@@ -111,7 +111,7 @@ module Spammable
end end
# Override in Spammable if further checks are necessary # Override in Spammable if further checks are necessary
def check_for_spam? def check_for_spam?(user:)
true true
end end
......
...@@ -437,10 +437,10 @@ class Issue < ApplicationRecord ...@@ -437,10 +437,10 @@ class Issue < ApplicationRecord
user, project.external_authorization_classification_label) user, project.external_authorization_classification_label)
end end
def check_for_spam? def check_for_spam?(user:)
# content created via support bots is always checked for spam, EVEN if # content created via support bots is always checked for spam, EVEN if
# the issue is not publicly visible and/or confidential # the issue is not publicly visible and/or confidential
return true if author.support_bot? && spammable_attribute_changed? return true if user.support_bot? && spammable_attribute_changed?
# Only check for spam on issues which are publicly visible (and thus indexed in search engines) # Only check for spam on issues which are publicly visible (and thus indexed in search engines)
return false unless publicly_visible? return false unless publicly_visible?
......
...@@ -246,7 +246,7 @@ class Snippet < ApplicationRecord ...@@ -246,7 +246,7 @@ class Snippet < ApplicationRecord
notes.includes(:author) notes.includes(:author)
end end
def check_for_spam? def check_for_spam?(user:)
visibility_level_changed?(to: Snippet::PUBLIC) || visibility_level_changed?(to: Snippet::PUBLIC) ||
(public? && (title_changed? || content_changed?)) (public? && (title_changed? || content_changed?))
end end
......
...@@ -28,7 +28,7 @@ module Spam ...@@ -28,7 +28,7 @@ module Spam
ServiceResponse.success(message: "CAPTCHA successfully verified") ServiceResponse.success(message: "CAPTCHA successfully verified")
else else
return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user) return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user)
return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam? return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam?(user: user)
perform_spam_service_check perform_spam_service_check
ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement") ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement")
...@@ -94,7 +94,7 @@ module Spam ...@@ -94,7 +94,7 @@ module Spam
def create_spam_log def create_spam_log
@spam_log = SpamLog.create!( @spam_log = SpamLog.create!(
{ {
user_id: target.author_id, user_id: user.id,
title: target.spam_title, title: target.spam_title,
description: target.spam_description, description: target.spam_description,
source_ip: spam_params.ip_address, source_ip: spam_params.ip_address,
......
...@@ -28,11 +28,11 @@ RSpec.describe Spammable do ...@@ -28,11 +28,11 @@ RSpec.describe Spammable do
it 'returns true for public project' do it 'returns true for public project' do
issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
expect(issue.check_for_spam?).to eq(true) expect(issue.check_for_spam?(user: issue.author)).to eq(true)
end end
it 'returns false for other visibility levels' do it 'returns false for other visibility levels' do
expect(issue.check_for_spam?).to eq(false) expect(issue.check_for_spam?(user: issue.author)).to eq(false)
end end
end end
......
...@@ -1112,14 +1112,14 @@ RSpec.describe Issue do ...@@ -1112,14 +1112,14 @@ RSpec.describe Issue do
with_them do with_them do
it 'checks for spam when necessary' do it 'checks for spam when necessary' do
author = support_bot? ? support_bot : user active_user = support_bot? ? support_bot : user
project = reusable_project project = reusable_project
project.update!(visibility_level: visibility_level) project.update!(visibility_level: visibility_level)
issue = create(:issue, project: project, confidential: confidential, description: 'original description', author: author) issue = create(:issue, project: project, confidential: confidential, description: 'original description', author: support_bot)
issue.assign_attributes(new_attributes) issue.assign_attributes(new_attributes)
expect(issue.check_for_spam?).to eq(check_for_spam?) expect(issue.check_for_spam?(user: active_user)).to eq(check_for_spam?)
end end
end end
end end
......
...@@ -431,7 +431,7 @@ RSpec.describe Snippet do ...@@ -431,7 +431,7 @@ RSpec.describe Snippet do
subject do subject do
snippet.assign_attributes(title: title) snippet.assign_attributes(title: title)
snippet.check_for_spam? snippet.check_for_spam?(user: snippet.author)
end end
context 'when public and spammable attributes changed' do context 'when public and spammable attributes changed' do
...@@ -455,7 +455,7 @@ RSpec.describe Snippet do ...@@ -455,7 +455,7 @@ RSpec.describe Snippet do
snippet.save! snippet.save!
snippet.visibility_level = Snippet::PUBLIC snippet.visibility_level = Snippet::PUBLIC
expect(snippet.check_for_spam?).to be_truthy expect(snippet.check_for_spam?(user: snippet.author)).to be_truthy
end end
end end
......
...@@ -5,7 +5,7 @@ require 'spec_helper' ...@@ -5,7 +5,7 @@ require 'spec_helper'
RSpec.describe Spam::SpamActionService do RSpec.describe Spam::SpamActionService do
include_context 'includes Spam constants' include_context 'includes Spam constants'
let(:issue) { create(:issue, project: project, author: user) } let(:issue) { create(:issue, project: project, author: author) }
let(:fake_ip) { '1.2.3.4' } let(:fake_ip) { '1.2.3.4' }
let(:fake_user_agent) { 'fake-user-agent' } let(:fake_user_agent) { 'fake-user-agent' }
let(:fake_referer) { 'fake-http-referer' } let(:fake_referer) { 'fake-http-referer' }
...@@ -23,6 +23,7 @@ RSpec.describe Spam::SpamActionService do ...@@ -23,6 +23,7 @@ RSpec.describe Spam::SpamActionService do
let_it_be(:project) { create(:project, :public) } let_it_be(:project) { create(:project, :public) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:author) { create(:user) }
before do before do
issue.spam = false issue.spam = false
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment