From 473594f1920966cc40d749141d7ca44b740eea78 Mon Sep 17 00:00:00 2001 From: Anna Buianova Date: Wed, 27 Dec 2023 23:12:50 +0300 Subject: [PATCH] Match spam role with existing suspended actions (#20477) * Started matching spam and suspended roles * Match spam and suspended roles in most cases * Fixed check_suspended for unauthenticated users --- app/controllers/api/v0/api_controller.rb | 4 ++-- app/controllers/api/v1/api_controller.rb | 6 +++--- app/controllers/application_controller.rb | 2 +- app/controllers/stories_controller.rb | 2 +- app/decorators/user_decorator.rb | 2 +- app/models/user.rb | 1 + app/models/users/suspended_username.rb | 3 ++- app/policies/api_secret_policy.rb | 2 +- app/policies/application_policy.rb | 2 +- app/policies/authorizer.rb | 4 ++++ app/policies/comment_policy.rb | 4 ++-- app/policies/discussion_lock_policy.rb | 2 +- app/policies/github_repo_policy.rb | 2 +- app/policies/image_upload_policy.rb | 2 +- app/policies/message_policy.rb | 2 +- app/policies/organization_policy.rb | 2 +- app/policies/rating_vote_policy.rb | 2 +- app/policies/stripe_active_card_policy.rb | 2 +- app/policies/stripe_subscription_policy.rb | 2 +- app/policies/user_block_policy.rb | 2 +- app/policies/user_policy.rb | 6 +++--- app/services/authentication/authenticator.rb | 6 +++--- app/services/re_captcha/check_enabled.rb | 2 +- app/services/tag_moderators/add_trusted_role.rb | 2 +- app/services/users/delete.rb | 2 +- app/services/users/update.rb | 2 +- .../admin/feedback_messages/_abuse_reports.html.erb | 2 +- app/views/admin/users/show/profile/_status.html.erb | 2 ++ app/views/users/show.html.erb | 2 +- spec/models/user_spec.rb | 2 ++ spec/models/users/suspended_username_spec.rb | 7 +++++++ spec/policies/api_secret_policy_spec.rb | 7 +++++++ spec/policies/response_template_policy_spec.rb | 2 +- spec/services/re_captcha/check_enabled_spec.rb | 7 +++++++ spec/services/users/delete_spec.rb | 9 +++++++++ 35 files changed, 75 insertions(+), 35 deletions(-) diff --git a/app/controllers/api/v0/api_controller.rb b/app/controllers/api/v0/api_controller.rb index 753f7b4a0..c289a4c17 100644 --- a/app/controllers/api/v0/api_controller.rb +++ b/app/controllers/api/v0/api_controller.rb @@ -44,7 +44,7 @@ module Api def authenticate! user = authenticate_with_api_key_or_current_user return error_unauthorized unless user - return error_unauthorized if @user.suspended? + return error_unauthorized if @user.spam_or_suspended? true end @@ -120,7 +120,7 @@ module Api # guard against timing attacks # see secure_secret = ActiveSupport::SecurityUtils.secure_compare(api_secret.secret, api_key) - return api_secret.user if secure_secret + api_secret.user if secure_secret end end end diff --git a/app/controllers/api/v1/api_controller.rb b/app/controllers/api/v1/api_controller.rb index b94300de9..c4dcd558a 100644 --- a/app/controllers/api/v1/api_controller.rb +++ b/app/controllers/api/v1/api_controller.rb @@ -50,7 +50,7 @@ module Api def authenticate_with_api_key! @user ||= authenticate_with_api_key return error_unauthorized unless @user - return error_unauthorized if @user.suspended? + return error_unauthorized if @user.spam_or_suspended? true end @@ -60,7 +60,7 @@ module Api def authenticate_with_api_key_or_current_user! @user ||= authenticate_with_api_key_or_current_user return error_unauthorized unless @user - return error_unauthorized if @user.suspended? + return error_unauthorized if @user.spam_or_suspended? true end @@ -116,7 +116,7 @@ module Api # guard against timing attacks # see secure_secret = ActiveSupport::SecurityUtils.secure_compare(api_secret.secret, api_key) - return api_secret.user if secure_secret + api_secret.user if secure_secret end end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7235ce41d..c2ba92307 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -205,7 +205,7 @@ class ApplicationController < ActionController::Base # @deprecated This is a policy related question and should be part of an ApplicationPolicy def check_suspended - return unless current_user&.suspended? + return unless current_user&.spam_or_suspended? respond_with_user_suspended end diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 57699104f..c7268a8dd 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -184,7 +184,7 @@ class StoriesController < ApplicationController end not_found if @user.username.include?("spam_") && @user.decorate.fully_banished? not_found unless @user.registered - if !user_signed_in? && (@user.suspended? && @user.has_no_published_content?) + if !user_signed_in? && (@user.spam_or_suspended? && @user.has_no_published_content?) not_found end assign_user_comments diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index fb52372d5..300db9045 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -119,7 +119,7 @@ class UserDecorator < ApplicationDecorator # returns true if the user has been suspended and has no content def fully_banished? - articles_count.zero? && comments_count.zero? && suspended? + articles_count.zero? && comments_count.zero? && spam_or_suspended? end def considered_new? diff --git a/app/models/user.rb b/app/models/user.rb index af8938a7a..c159167a9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -448,6 +448,7 @@ class User < ApplicationRecord :support_admin?, :suspended?, :spam?, + :spam_or_suspended?, :tag_moderator?, :tech_admin?, :trusted?, diff --git a/app/models/users/suspended_username.rb b/app/models/users/suspended_username.rb index 89908a99c..30b71dd39 100644 --- a/app/models/users/suspended_username.rb +++ b/app/models/users/suspended_username.rb @@ -8,11 +8,12 @@ module Users Digest::SHA256.hexdigest(username) end + # suspended or assigned spam role def self.previously_suspended?(username) where(username_hash: hash_username(username)).any? end - # Convenience method for easily adding a suspended user + # Convenience method for easily adding a suspended/spam user def self.create_from_user(user) create!(username_hash: hash_username(user.username)) end diff --git a/app/policies/api_secret_policy.rb b/app/policies/api_secret_policy.rb index 7d9b6c68a..0c6e7aead 100644 --- a/app/policies/api_secret_policy.rb +++ b/app/policies/api_secret_policy.rb @@ -1,6 +1,6 @@ class ApiSecretPolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end def destroy? diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 6bdbea0f8..a63178972 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -70,7 +70,7 @@ class ApplicationPolicy def self.require_user_in_good_standing!(user:) require_user!(user: user) - return true unless user.suspended? + return true unless user.spam_or_suspended? raise ApplicationPolicy::UserSuspendedError, I18n.t("policies.application_policy.your_account_is_suspended") end diff --git a/app/policies/authorizer.rb b/app/policies/authorizer.rb index c079829e3..d4843f452 100644 --- a/app/policies/authorizer.rb +++ b/app/policies/authorizer.rb @@ -137,6 +137,10 @@ module Authorizer has_role?(:spam) end + def spam_or_suspended? + has_any_role?(:spam, :suspended) + end + def tag_moderator?(tag: nil) # Note a fan of "peeking" into the roles table, which in a way # circumvents the rolify gem. But this was the past implementation. diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb index 4822ef32b..0db69b55e 100644 --- a/app/policies/comment_policy.rb +++ b/app/policies/comment_policy.rb @@ -1,6 +1,6 @@ class CommentPolicy < ApplicationPolicy def edit? - return false if user_suspended? + return false if user.spam_or_suspended? user_author? end @@ -10,7 +10,7 @@ class CommentPolicy < ApplicationPolicy end def create? - !user_suspended? && !user.comment_suspended? + !user.spam_or_suspended? && !user.comment_suspended? end alias new? create? diff --git a/app/policies/discussion_lock_policy.rb b/app/policies/discussion_lock_policy.rb index 1445fe305..8c084eaa5 100644 --- a/app/policies/discussion_lock_policy.rb +++ b/app/policies/discussion_lock_policy.rb @@ -2,7 +2,7 @@ class DiscussionLockPolicy < ApplicationPolicy PERMITTED_ATTRIBUTES = %i[article_id notes reason].freeze def create? - (user_author? || user_any_admin?) && !user_suspended? + (user_author? || user_any_admin?) && !user.spam_or_suspended? end alias destroy? create? diff --git a/app/policies/github_repo_policy.rb b/app/policies/github_repo_policy.rb index 59f8e3ef0..52f18dc99 100644 --- a/app/policies/github_repo_policy.rb +++ b/app/policies/github_repo_policy.rb @@ -1,6 +1,6 @@ class GithubRepoPolicy < ApplicationPolicy def index? - !user_suspended? && user.authenticated_through?(:github) + !user.spam_or_suspended? && user.authenticated_through?(:github) end alias update_or_create? index? diff --git a/app/policies/image_upload_policy.rb b/app/policies/image_upload_policy.rb index 737344119..bfa99e305 100644 --- a/app/policies/image_upload_policy.rb +++ b/app/policies/image_upload_policy.rb @@ -1,5 +1,5 @@ class ImageUploadPolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end end diff --git a/app/policies/message_policy.rb b/app/policies/message_policy.rb index 05d95c2e2..aeb73205a 100644 --- a/app/policies/message_policy.rb +++ b/app/policies/message_policy.rb @@ -1,6 +1,6 @@ class MessagePolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end def destroy? diff --git a/app/policies/organization_policy.rb b/app/policies/organization_policy.rb index 7d43b8159..3e8186eac 100644 --- a/app/policies/organization_policy.rb +++ b/app/policies/organization_policy.rb @@ -1,6 +1,6 @@ class OrganizationPolicy < ApplicationPolicy def create? - !user.suspended? + !user.spam_or_suspended? end def update? diff --git a/app/policies/rating_vote_policy.rb b/app/policies/rating_vote_policy.rb index 008aa91a5..020ea2560 100644 --- a/app/policies/rating_vote_policy.rb +++ b/app/policies/rating_vote_policy.rb @@ -1,6 +1,6 @@ class RatingVotePolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end def permitted_attributes diff --git a/app/policies/stripe_active_card_policy.rb b/app/policies/stripe_active_card_policy.rb index ace29be70..7ec5fe191 100644 --- a/app/policies/stripe_active_card_policy.rb +++ b/app/policies/stripe_active_card_policy.rb @@ -1,6 +1,6 @@ class StripeActiveCardPolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end alias update? create? diff --git a/app/policies/stripe_subscription_policy.rb b/app/policies/stripe_subscription_policy.rb index a1be09c5c..1dda42486 100644 --- a/app/policies/stripe_subscription_policy.rb +++ b/app/policies/stripe_subscription_policy.rb @@ -1,6 +1,6 @@ class StripeSubscriptionPolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end alias update? create? diff --git a/app/policies/user_block_policy.rb b/app/policies/user_block_policy.rb index 4e9e4c77d..8516b4542 100644 --- a/app/policies/user_block_policy.rb +++ b/app/policies/user_block_policy.rb @@ -1,6 +1,6 @@ class UserBlockPolicy < ApplicationPolicy def create? - !user_suspended? + !user.spam_or_suspended? end alias destroy? create? diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 17f969472..4e338222b 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -66,7 +66,7 @@ class UserPolicy < ApplicationPolicy alias onboarding_notifications_checkbox_update? onboarding_update? def update? - edit? && !user_suspended? + edit? && !user.spam_or_suspended? end alias destroy? edit? @@ -78,7 +78,7 @@ class UserPolicy < ApplicationPolicy alias request_destroy? edit? def join_org? - !user_suspended? + !user.spam_or_suspended? end def leave_org? @@ -99,7 +99,7 @@ class UserPolicy < ApplicationPolicy alias search_by_email? elevated_user? def moderation_routes? - (user.has_trusted_role? || elevated_user?) && !user.suspended? + (user.has_trusted_role? || elevated_user?) && !user.spam_or_suspended? end def permitted_attributes diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb index 6ebd60206..8a1109c3a 100644 --- a/app/services/authentication/authenticator.rb +++ b/app/services/authentication/authenticator.rb @@ -119,9 +119,9 @@ module Authentication suspended_user = Users::SuspendedUsername.previously_suspended?(username) raise ::Authentication::Errors::PreviouslySuspended if suspended_user - existing_user = User.where( + existing_user = User.find_by( provider.user_username_field => username, - ).take + ) return existing_user if existing_user User.new.tap do |user| @@ -149,7 +149,7 @@ module Authentication end def update_user(user) - return user if user.suspended? + return user if user.spam_or_suspended? user.tap do |model| model.unlock_access! if model.access_locked? diff --git a/app/services/re_captcha/check_enabled.rb b/app/services/re_captcha/check_enabled.rb index 12e554f84..9f4977c71 100644 --- a/app/services/re_captcha/check_enabled.rb +++ b/app/services/re_captcha/check_enabled.rb @@ -22,7 +22,7 @@ module ReCaptcha # recaptcha will not be enabled for tag moderator/trusted/admin users return false if @user.tag_moderator? || @user.trusted? || @user.any_admin? # recaptcha will be enabled if the user has been suspended - return true if @user.suspended? + return true if @user.spam_or_suspended? # recaptcha will be enabled if the user has a vomit or is too recent @user.vomited_on? || @user.created_at.after?(1.month.ago) diff --git a/app/services/tag_moderators/add_trusted_role.rb b/app/services/tag_moderators/add_trusted_role.rb index 02bfd541f..d351514f6 100644 --- a/app/services/tag_moderators/add_trusted_role.rb +++ b/app/services/tag_moderators/add_trusted_role.rb @@ -1,7 +1,7 @@ module TagModerators class AddTrustedRole def self.call(user) - return if user.has_trusted_role? || user.suspended? + return if user.has_trusted_role? || user.spam_or_suspended? user.add_role(:trusted) user.notification_setting.update(email_community_mod_newsletter: true) diff --git a/app/services/users/delete.rb b/app/services/users/delete.rb index 13e0da86b..5f699f5f9 100644 --- a/app/services/users/delete.rb +++ b/app/services/users/delete.rb @@ -15,7 +15,7 @@ module Users delete_user_activity user.remove_from_mailchimp_newsletters EdgeCache::Bust.call("/#{user.username}") - Users::SuspendedUsername.create_from_user(user) if user.suspended? + Users::SuspendedUsername.create_from_user(user) if user.spam_or_suspended? user.destroy Rails.cache.delete("user-destroy-token-#{user.id}") end diff --git a/app/services/users/update.rb b/app/services/users/update.rb index 50e9d4595..915b60867 100644 --- a/app/services/users/update.rb +++ b/app/services/users/update.rb @@ -115,7 +115,7 @@ module Users end def conditionally_resave_articles - return unless resave_articles? && !@user.suspended? + return unless resave_articles? && !@user.spam_or_suspended? Users::ResaveArticlesWorker.perform_async(@user.id) end diff --git a/app/views/admin/feedback_messages/_abuse_reports.html.erb b/app/views/admin/feedback_messages/_abuse_reports.html.erb index b7f3b033f..5d0ec68bc 100644 --- a/app/views/admin/feedback_messages/_abuse_reports.html.erb +++ b/app/views/admin/feedback_messages/_abuse_reports.html.erb @@ -36,7 +36,7 @@ <%= reaction.reactable_type %>: <%= reaction.reactable_type == "User" ? reaction.reactable.username : reaction.reactable.title %> - <% if reaction.reactable_type == "User" && reaction.reactable.suspended? %> + <% if reaction.reactable_type == "User" && reaction.reactable.spam_or_suspended? %> Suspended <% end %> <% if reaction.reactable_type == "User" && reaction.reactable.vomited_on? %> diff --git a/app/views/admin/users/show/profile/_status.html.erb b/app/views/admin/users/show/profile/_status.html.erb index c77155e02..1921854f7 100644 --- a/app/views/admin/users/show/profile/_status.html.erb +++ b/app/views/admin/users/show/profile/_status.html.erb @@ -2,6 +2,8 @@ <%= t("views.admin.users.status_reader") %> <% if user.suspended? %> <%= t("views.admin.users.statuses.Suspended") %> + <% elsif user.spam? %> + <%= t("views.admin.users.statuses.Spam") %> <% elsif user.warned? %> <%= t("views.admin.users.statuses.Warned") %> <% elsif user.comment_suspended? %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 623b1124f..5a1e04396 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -5,7 +5,7 @@ <%= content_for :page_meta do %> <%= render "users/meta" %> - <% if @user.score.negative? || @user.suspended? %> + <% if @user.score.negative? || @user.spam_or_suspended? %> <% end %> diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f9c7e2f27..c7148b9db 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -51,6 +51,8 @@ RSpec.describe User do it { is_expected.to delegate_method(:super_admin?).to(:authorizer) } it { is_expected.to delegate_method(:support_admin?).to(:authorizer) } it { is_expected.to delegate_method(:suspended?).to(:authorizer) } + it { is_expected.to delegate_method(:spam?).to(:authorizer) } + it { is_expected.to delegate_method(:spam_or_suspended?).to(:authorizer) } it { is_expected.to delegate_method(:tag_moderator?).to(:authorizer) } it { is_expected.to delegate_method(:tech_admin?).to(:authorizer) } it { is_expected.to delegate_method(:trusted?).to(:authorizer) } diff --git a/spec/models/users/suspended_username_spec.rb b/spec/models/users/suspended_username_spec.rb index 2c9dedde7..0e863ccbd 100644 --- a/spec/models/users/suspended_username_spec.rb +++ b/spec/models/users/suspended_username_spec.rb @@ -16,6 +16,13 @@ RSpec.describe Users::SuspendedUsername do expect(described_class.previously_suspended?(user.username)).to be true end + it "returns true if the user has been previously assigned a spam roles" do + user = create(:user, :spam) + described_class.create_from_user(user) + + expect(described_class.previously_suspended?(user.username)).to be true + end + it "returns false if the user has not been previously_suspended" do user = create(:user) diff --git a/spec/policies/api_secret_policy_spec.rb b/spec/policies/api_secret_policy_spec.rb index a9dd27505..bbb9ac17b 100644 --- a/spec/policies/api_secret_policy_spec.rb +++ b/spec/policies/api_secret_policy_spec.rb @@ -35,4 +35,11 @@ RSpec.describe ApiSecretPolicy, type: :policy do it { is_expected.to forbid_actions %i[create] } end + + context "when the user has a spam role" do + let(:user) { create(:user, :spam) } + let(:api_secret) { build_stubbed(:api_secret) } + + it { is_expected.to forbid_actions %i[create] } + end end diff --git a/spec/policies/response_template_policy_spec.rb b/spec/policies/response_template_policy_spec.rb index 62a93db2e..d672a2d85 100644 --- a/spec/policies/response_template_policy_spec.rb +++ b/spec/policies/response_template_policy_spec.rb @@ -62,6 +62,6 @@ RSpec.describe ResponseTemplatePolicy, type: :policy do let(:response_template) { create(:response_template, type_of: "mod_comment", user: nil) } it { is_expected.to permit_actions(%i[moderator_index moderator_create]) } - it { is_expected.not_to permit_actions(%i[create update destroy]) } + it { is_expected.to forbid_actions(%i[admin_create update destroy]) } end end diff --git a/spec/services/re_captcha/check_enabled_spec.rb b/spec/services/re_captcha/check_enabled_spec.rb index 9672b5278..0205f4653 100644 --- a/spec/services/re_captcha/check_enabled_spec.rb +++ b/spec/services/re_captcha/check_enabled_spec.rb @@ -57,6 +57,13 @@ RSpec.describe ReCaptcha::CheckEnabled, type: :request do expect(described_class.call(older_user)).to be(true) older_user.remove_role(:suspended) end + + it "marks ReCaptcha as enabled when a spam user is logged in" do + older_user.add_role(:spam) + sign_in older_user + expect(described_class.call(older_user)).to be(true) + older_user.remove_role(:spam) + end end end end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 778f1df74..7c8b58b62 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -169,4 +169,13 @@ RSpec.describe Users::Delete, type: :service do end.to change(Users::SuspendedUsername, :count).by(1) end end + + context "when the user was a spammer" do + it "stores a hash of the username so the user can't sign up again" do + user = create(:user, :spam) + expect do + described_class.call(user) + end.to change(Users::SuspendedUsername, :count).by(1) + end + end end