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

@@ -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 <https://www.slideshare.net/NickMalcolm/timing-attacks-and-ruby-on-rails>
         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

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 <https://www.slideshare.net/NickMalcolm/timing-attacks-and-ruby-on-rails>
         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

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

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

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?

app/models/user.rb

@@ -448,6 +448,7 @@ class User < ApplicationRecord
     :support_admin?,
     :suspended?,
     :spam?,
+    :spam_or_suspended?,
     :tag_moderator?,
     :tech_admin?,
     :trusted?,

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

app/policies/api_secret_policy.rb

@@ -1,6 +1,6 @@
 class ApiSecretPolicy < ApplicationPolicy
   def create?
-    !user_suspended?
+    !user.spam_or_suspended?
   end
 
   def destroy?

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

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.

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?

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?

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?

app/policies/image_upload_policy.rb

@@ -1,5 +1,5 @@
 class ImageUploadPolicy < ApplicationPolicy
   def create?
-    !user_suspended?
+    !user.spam_or_suspended?
   end
 end

app/policies/message_policy.rb

@@ -1,6 +1,6 @@
 class MessagePolicy < ApplicationPolicy
   def create?
-    !user_suspended?
+    !user.spam_or_suspended?
   end
 
   def destroy?

app/policies/organization_policy.rb

@@ -1,6 +1,6 @@
 class OrganizationPolicy < ApplicationPolicy
   def create?
-    !user.suspended?
+    !user.spam_or_suspended?
   end
 
   def update?

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

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?

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?

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?

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

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?

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)

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)

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

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

app/views/admin/feedback_messages/_abuse_reports.html.erb

@@ -36,7 +36,7 @@
               <span>
                 <strong><%= reaction.reactable_type %>:</strong>
                 <a href="<%= reaction.reactable.path %>" target="_blank" rel="noopener"><%= reaction.reactable_type == "User" ? reaction.reactable.username : reaction.reactable.title %></a>
-                <% if reaction.reactable_type == "User" && reaction.reactable.suspended? %>
+                <% if reaction.reactable_type == "User" && reaction.reactable.spam_or_suspended? %>
                   <span class="c-indicator c-indicator--danger">Suspended</span>
                 <% end %>
                 <% if reaction.reactable_type == "User" && reaction.reactable.vomited_on? %>

app/views/admin/users/show/profile/_status.html.erb

@@ -2,6 +2,8 @@
   <span class="screen-reader-only"><%= t("views.admin.users.status_reader") %></span>
   <% if user.suspended? %>
     <span data-testid="user-status" class="c-indicator c-indicator--danger c-indicator--relaxed"><%= t("views.admin.users.statuses.Suspended") %></span>
+  <% elsif user.spam? %>
+    <span data-testid="user-status" class="c-indicator c-indicator--danger c-indicator--relaxed"><%= t("views.admin.users.statuses.Spam") %></span>
   <% elsif user.warned? %>
     <span data-testid="user-status" class="c-indicator c-indicator--warning c-indicator--relaxed"><%= t("views.admin.users.statuses.Warned") %></span>
   <% elsif user.comment_suspended? %>

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? %>
     <meta name="robots" content="noindex">
     <meta name="robots" content="nofollow">
   <% end %>

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) }

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)
 

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

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

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

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