Prevent suspended users from self-deleting and returning (#12503)

* Add new table and model

* Store banned user username hash on delete

* Prevent previously banned user from signing up again

* Update method name

* Refactor code and add more specs

* Test improvements

* Don't override existing global flash

* Fix typo

* Update spec description

* Update schema.rb

* More schema.rb fixes

* Simplify spec

* Update migration

* Clean up migration

* Rename method

* Add DataDog counter

* Revisit error handling

* Remove spurious empty line

* Make model name more explicit

app/controllers/concerns/verify_setup_completed.rb

@@ -33,6 +33,9 @@ module VerifySetupCompleted
   end
 
   def verify_setup_completed
+    # This is the only flash in our application layout, don't override it if
+    # there's already another message.
+    return if flash[:global_notice].present?
     return if config_path? || setup_completed? || SiteConfig.waiting_on_first_user
 
     link = helpers.tag.a("the configuration page", href: admin_config_path, data: { "no-instant" => true })

app/controllers/omniauth_callbacks_controller.rb

@@ -95,6 +95,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       flash[:alert] = user_errors
       redirect_to new_user_registration_url
     end
+  rescue ::Authentication::Errors::PreviouslyBanned => e
+    flash[:global_notice] = e.message
+    redirect_to root_path
   rescue StandardError => e
     Honeybadger.notify(e)
 

app/errors/authentication/errors.rb

@@ -0,0 +1,26 @@
+module Authentication
+  module Errors
+    PREVIOUSLY_BANNED_MESSAGE = "It appears that your previous %<community_name>s " \
+      "account was suspended. As such, we've taken measures to prevent you from " \
+      "creating a new account with %<community_name>s and its community. If you " \
+      "think that there has been a mistake, please email us at %<community_email>s, " \
+      "and we will take another look.".freeze
+
+    class Error < StandardError
+    end
+
+    class ProviderNotFound < Error
+    end
+
+    class ProviderNotEnabled < Error
+    end
+
+    class PreviouslyBanned < Error
+      def message
+        format(PREVIOUSLY_BANNED_MESSAGE,
+               community_name: SiteConfig.community_name,
+               community_email: SiteConfig.email_addresses[:contact])
+      end
+    end
+  end
+end

app/models/users/suspended_username.rb

@@ -0,0 +1,20 @@
+module Users
+  class SuspendedUsername < ApplicationRecord
+    self.table_name_prefix = "users_"
+
+    validates :username_hash, presence: true, uniqueness: true
+
+    def self.hash_username(username)
+      Digest::SHA256.hexdigest(username)
+    end
+
+    def self.previously_banned?(username)
+      where(username_hash: hash_username(username)).any?
+    end
+
+    # Convenience method for easily adding a suspended user
+    def self.create_from_user(user)
+      create!(username_hash: hash_username(user.username))
+    end
+  end
+end

app/services/authentication/authenticator.rb

@@ -96,8 +96,12 @@ module Authentication
     end
 
     def find_or_create_user!
+      username = provider.user_nickname
+      banned_user = Users::SuspendedUsername.previously_banned?(username)
+      raise ::Authentication::Errors::PreviouslyBanned if banned_user
+
       existing_user = User.where(
-        provider.user_username_field => provider.user_nickname,
+        provider.user_username_field => username,
       ).take
       return existing_user if existing_user
 

app/services/authentication/errors.rb

@@ -1,12 +0,0 @@
-module Authentication
-  module Errors
-    class Error < StandardError
-    end
-
-    class ProviderNotFound < Error
-    end
-
-    class ProviderNotEnabled < Error
-    end
-  end
-end

app/services/users/delete.rb

@@ -10,6 +10,7 @@ module Users
       delete_user_activity
       user.unsubscribe_from_newsletters
       EdgeCache::Bust.call("/#{user.username}")
+      Users::SuspendedUsername.create_from_user(user) if user.has_role?(:banned)
       user.destroy
       Rails.cache.delete("user-destroy-token-#{user.id}")
     end

db/migrate/20210201055410_create_users_suspended_usernames.rb

@@ -0,0 +1,9 @@
+class CreateUsersSuspendedUsernames < ActiveRecord::Migration[6.0]
+  def change
+    create_table :users_suspended_usernames, id: false do |t|
+      t.string :username_hash, primary_key: true
+
+      t.timestamps
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_01_31_000458) do
+ActiveRecord::Schema.define(version: 2021_02_01_055410) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -1370,6 +1370,11 @@ ActiveRecord::Schema.define(version: 2021_01_31_000458) do
     t.index ["user_id", "role_id"], name: "index_users_roles_on_user_id_and_role_id"
   end
 
+  create_table "users_suspended_usernames", primary_key: "username_hash", id: :string, force: :cascade do |t|
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+  end
+
   create_table "webhook_endpoints", force: :cascade do |t|
     t.datetime "created_at", null: false
     t.string "events", null: false, array: true

spec/factories/suspended_usernames.rb

@@ -0,0 +1,11 @@
+FactoryBot.define do
+  factory :suspended_username, class: "Users::SuspendedUsername" do
+    transient do
+      username { Faker::Internet.unique.username }
+    end
+
+    after(:build) do |user, evaluator|
+      user.username_hash = Users::SuspendedUsername.hash_username(evaluator.username)
+    end
+  end
+end

spec/models/users/suspended_username_spec.rb

@@ -0,0 +1,33 @@
+require "rails_helper"
+
+RSpec.describe Users::SuspendedUsername, type: :model do
+  describe "validations" do
+    subject { create(:suspended_username) }
+
+    it { is_expected.to validate_presence_of(:username_hash) }
+    it { is_expected.to validate_uniqueness_of(:username_hash) }
+  end
+
+  describe ".previously_banned?" do
+    it "returns true if the user has been previously banned" do
+      user = create(:user, :banned)
+      described_class.create_from_user(user)
+
+      expect(described_class.previously_banned?(user.username)).to be true
+    end
+
+    it "returns false if the user has not been previously_banned" do
+      user = create(:user)
+
+      expect(described_class.previously_banned?(user.username)).to be false
+    end
+  end
+
+  describe ".create_from_user" do
+    it "records a hash of the username in the database" do
+      expect do
+        described_class.create_from_user(create(:user, :banned))
+      end.to change(described_class, :count).by(1)
+    end
+  end
+end

spec/services/users/delete_spec.rb

@@ -176,4 +176,13 @@ RSpec.describe Users::Delete, type: :service do
       expect(ChatChannel.find_by(id: chat_channel.id)).not_to be_nil
     end
   end
+
+  context "when the user was banned" do
+    it "stores a hash of the username so the user can't sign up again" do
+      user = create(:user, :banned)
+      expect do
+        described_class.call(user)
+      end.to change(Users::SuspendedUsername, :count).by(1)
+    end
+  end
 end

spec/system/authentication/user_with_suspended_username_spec.rb

@@ -0,0 +1,34 @@
+require "rails_helper"
+
+RSpec.describe "User with suspended username tried to sign up via OAuth" do
+  before do
+    omniauth_mock_twitter_payload
+
+    allow(SiteConfig)
+      .to receive(:authentication_providers)
+      .and_return(Authentication::Providers.available)
+
+    allow(ForemStatsClient).to receive(:increment)
+  end
+
+  context "when a user has been previously banned", :aggregate_failures do
+    it "displays an error message" do
+      username = OmniAuth.config.mock_auth[:twitter].extra.raw_info.username
+      create(:suspended_username, username: username)
+
+      visit sign_up_path
+      click_on("Continue with Twitter", match: :first)
+      expect(page).to have_current_path(root_path)
+
+      expected_message = ::Authentication::Errors::PreviouslyBanned.new.message
+      expect(page).to have_content(expected_message)
+      expect(ForemStatsClient)
+        .to have_received(:increment)
+        .with("identity.errors",
+              tags: [
+                "error:Authentication::Errors::PreviouslyBanned",
+                "message:#{expected_message}",
+              ])
+    end
+  end
+end