From 7490606a56eae99a197dd164dc74b8ab4fc72252 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Fri, 9 Jun 2023 14:55:09 -0400 Subject: [PATCH 1/3] Create services to modify custom attributes --- app/models/user.rb | 4 ++++ app/models/user_custom_attribute.rb | 1 + app/services/spam/spam_verdict_service.rb | 2 +- .../users/allow_possible_spam_service.rb | 18 ++++++++++++++++++ .../users/disallow_possible_spam_service.rb | 13 +++++++++++++ spec/models/user_spec.rb | 10 ++++++++++ 6 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 app/services/users/allow_possible_spam_service.rb create mode 100644 app/services/users/disallow_possible_spam_service.rb diff --git a/app/models/user.rb b/app/models/user.rb index 3ac58b758797d6..1053293ec337e0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2596,6 +2596,10 @@ def ci_namespace_mirrors_for_group_members(level) Ci::NamespaceMirror.contains_traversal_ids(traversal_ids) end + + def allow_possible_spam? + custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).exists? + end end User.prepend_mod_with('User') diff --git a/app/models/user_custom_attribute.rb b/app/models/user_custom_attribute.rb index 63a5ee9770fd80..0ba58a132371a2 100644 --- a/app/models/user_custom_attribute.rb +++ b/app/models/user_custom_attribute.rb @@ -15,6 +15,7 @@ class UserCustomAttribute < ApplicationRecord UNBLOCKED_BY = 'unblocked_by' ARKOSE_RISK_BAND = 'arkose_risk_band' AUTO_BANNED_BY_ABUSE_REPORT_ID = 'auto_banned_by_abuse_report_id' + ALLOW_POSSIBLE_SPAM = 'allow_possible_spam' class << self def upsert_custom_attributes(custom_attributes) diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 2ecd431fd91473..160ebb81b39724 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -85,7 +85,7 @@ def override_via_allow_possible_spam?(verdict:) # than the override verdict's priority value), then we don't need to override it. return false if SUPPORTED_VERDICTS[verdict][:priority] > SUPPORTED_VERDICTS[OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM][:priority] - target.allow_possible_spam? + target.allow_possible_spam? || user.allow_possible_spam? end def spamcheck_client diff --git a/app/services/users/allow_possible_spam_service.rb b/app/services/users/allow_possible_spam_service.rb new file mode 100644 index 00000000000000..d9273fe0fc1cf4 --- /dev/null +++ b/app/services/users/allow_possible_spam_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Users + class AllowPossibleSpamService < BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(user) + custom_attribute = { + user_id: user.id, + key: UserCustomAttribute::ALLOW_POSSIBLE_SPAM, + value: "#{current_user.username}/#{current_user.id}+#{Time.current}" + } + UserCustomAttribute.upsert_custom_attributes([custom_attribute]) + end + end +end diff --git a/app/services/users/disallow_possible_spam_service.rb b/app/services/users/disallow_possible_spam_service.rb new file mode 100644 index 00000000000000..96d62b42704e4d --- /dev/null +++ b/app/services/users/disallow_possible_spam_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Users + class DisallowPossibleSpamService < BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(user) + UserCustomAttribute.by_user(user).by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).delete_all + end + end +end \ No newline at end of file diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 768198bc8267e4..fb822b5e4e26ae 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6216,6 +6216,16 @@ def add_user(access) expect { user.delete_async(deleted_by: deleted_by) }.not_to change { user.note } end end + + context 'when user is allowed to create spam' do + before do + user.custom_attributes.upsert_custom_attributes({UserCustomAttribute::ALLOW_POSSIBLE_SPAM: {}}) + end + + it '#allow_possible_spam? is true' do + expect(user.allow_possible_spam?).to be_truthy + end + end end end -- GitLab From e9f3e0353880cb52c12782c03341109d385cb316 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Mon, 12 Jun 2023 15:20:15 -0400 Subject: [PATCH 2/3] Add allow/disallow services and specs Added helper method to `User`. Updated `SpamVerdictService` to query user's `allow_possible_spam` status. --- app/models/user.rb | 8 ++--- .../users/disallow_possible_spam_service.rb | 4 +-- spec/models/user_spec.rb | 7 +++- .../spam/spam_verdict_service_spec.rb | 32 +++++++++++++++++ .../users/allow_possible_spam_service_spec.rb | 24 +++++++++++++ .../disallow_possible_spam_service_spec.rb | 34 +++++++++++++++++++ 6 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 spec/services/users/allow_possible_spam_service_spec.rb create mode 100644 spec/services/users/disallow_possible_spam_service_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index 1053293ec337e0..cd24cd74ef3b44 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2302,6 +2302,10 @@ def abuse_metadata } end + def allow_possible_spam? + custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).exists? + end + def namespace_commit_email_for_namespace(namespace) return if namespace.nil? @@ -2596,10 +2600,6 @@ def ci_namespace_mirrors_for_group_members(level) Ci::NamespaceMirror.contains_traversal_ids(traversal_ids) end - - def allow_possible_spam? - custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).exists? - end end User.prepend_mod_with('User') diff --git a/app/services/users/disallow_possible_spam_service.rb b/app/services/users/disallow_possible_spam_service.rb index 96d62b42704e4d..e31ba7ddff02a4 100644 --- a/app/services/users/disallow_possible_spam_service.rb +++ b/app/services/users/disallow_possible_spam_service.rb @@ -7,7 +7,7 @@ def initialize(current_user) end def execute(user) - UserCustomAttribute.by_user(user).by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).delete_all + user.custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).delete_all end end -end \ No newline at end of file +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fb822b5e4e26ae..6e040501674a03 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6219,7 +6219,12 @@ def add_user(access) context 'when user is allowed to create spam' do before do - user.custom_attributes.upsert_custom_attributes({UserCustomAttribute::ALLOW_POSSIBLE_SPAM: {}}) + user.custom_attributes.upsert_custom_attributes( + [{ + user_id: user.id, + key: UserCustomAttribute::ALLOW_POSSIBLE_SPAM, + value: "test" + }]) end it '#allow_possible_spam? is true' do diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 6b14cf33041bec..f54981a7ac2d12 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -136,6 +136,38 @@ end end + context 'if allow_possible_spam user custom attribute is set' do + before do + UserCustomAttribute.upsert_custom_attributes( + [{ + user_id: user.id, + key: 'allow_possible_spam', + value: 'does not matter' + }] + ) + end + + context 'and a service returns a verdict that should be overridden' do + before do + allow(service).to receive(:get_spamcheck_verdict).and_return(BLOCK_USER) + end + + it 'overrides and renders the override verdict' do + is_expected.to eq OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM + end + end + + context 'and a service returns a verdict that does not need to be overridden' do + before do + allow(service).to receive(:get_spamcheck_verdict).and_return(ALLOW) + end + + it 'does not override and renders the original verdict' do + is_expected.to eq ALLOW + end + end + end + context 'records metrics' do let(:histogram) { instance_double(Prometheus::Client::Histogram) } diff --git a/spec/services/users/allow_possible_spam_service_spec.rb b/spec/services/users/allow_possible_spam_service_spec.rb new file mode 100644 index 00000000000000..53618f0c8e9614 --- /dev/null +++ b/spec/services/users/allow_possible_spam_service_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::AllowPossibleSpamService, feature_category: :user_management do + let_it_be(:current_user) { create(:admin) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + let(:user) { create(:user) } + + subject(:operation) { service.execute(user) } + + it 'updates the custom attributes', :aggregate_failures do + expect(user.custom_attributes).to be_empty + + operation + user.reload + + expect(user.custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM)).to be_present + end + end +end diff --git a/spec/services/users/disallow_possible_spam_service_spec.rb b/spec/services/users/disallow_possible_spam_service_spec.rb new file mode 100644 index 00000000000000..32a47e05525a7b --- /dev/null +++ b/spec/services/users/disallow_possible_spam_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::DisallowPossibleSpamService, feature_category: :user_management do + let_it_be(:current_user) { create(:admin) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + let(:user) { create(:user) } + + subject(:operation) { service.execute(user) } + + before do + UserCustomAttribute.upsert_custom_attributes( + [{ + user_id: user.id, + key: :allow_possible_spam, + value: 'not important' + }] + ) + end + + it 'updates the custom attributes', :aggregate_failures do + expect(user.custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM)).to be_present + + operation + user.reload + + expect(user.custom_attributes).to be_empty + end + end +end -- GitLab From 9c6da7b2d9baf6652f3a28be407f1180cb3bcaa6 Mon Sep 17 00:00:00 2001 From: Ethan Urie Date: Fri, 16 Jun 2023 10:34:42 -0400 Subject: [PATCH 3/3] Refactor user specs --- app/models/user.rb | 2 +- spec/models/user_spec.rb | 28 ++++++++++++++++++---------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index cd24cd74ef3b44..16174dce14d56d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2303,7 +2303,7 @@ def abuse_metadata end def allow_possible_spam? - custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).exists? + custom_attributes.by_key(UserCustomAttribute::ALLOW_POSSIBLE_SPAM).exists? end def namespace_commit_email_for_namespace(namespace) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6e040501674a03..3244b60e317ca2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6217,18 +6217,26 @@ def add_user(access) end end - context 'when user is allowed to create spam' do - before do - user.custom_attributes.upsert_custom_attributes( - [{ - user_id: user.id, - key: UserCustomAttribute::ALLOW_POSSIBLE_SPAM, - value: "test" - }]) + describe '#allow_possible_spam?' do + context 'when no custom attribute is set' do + it 'is false' do + expect(user.allow_possible_spam?).to be_falsey + end end - it '#allow_possible_spam? is true' do - expect(user.allow_possible_spam?).to be_truthy + context 'when the custom attribute is set' do + before do + user.custom_attributes.upsert_custom_attributes( + [{ + user_id: user.id, + key: UserCustomAttribute::ALLOW_POSSIBLE_SPAM, + value: "test" + }]) + end + + it '#allow_possible_spam? is true' do + expect(user.allow_possible_spam?).to be_truthy + end end end end -- GitLab