diff --git a/app/controllers/concerns/verify_setup_completed.rb b/app/controllers/concerns/verify_setup_completed.rb index 2565260fc..c1a442b46 100644 --- a/app/controllers/concerns/verify_setup_completed.rb +++ b/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 }) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 1ed892228..2da288848 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/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) diff --git a/app/errors/authentication/errors.rb b/app/errors/authentication/errors.rb new file mode 100644 index 000000000..98069f835 --- /dev/null +++ b/app/errors/authentication/errors.rb @@ -0,0 +1,26 @@ +module Authentication + module Errors + PREVIOUSLY_BANNED_MESSAGE = "It appears that your previous %s " \ + "account was suspended. As such, we've taken measures to prevent you from " \ + "creating a new account with %s and its community. If you " \ + "think that there has been a mistake, please email us at %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 diff --git a/app/models/users/suspended_username.rb b/app/models/users/suspended_username.rb new file mode 100644 index 000000000..6f6d29cf1 --- /dev/null +++ b/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 diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb index ad8b813a2..113921fd3 100644 --- a/app/services/authentication/authenticator.rb +++ b/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 diff --git a/app/services/authentication/errors.rb b/app/services/authentication/errors.rb deleted file mode 100644 index 2c4311a83..000000000 --- a/app/services/authentication/errors.rb +++ /dev/null @@ -1,12 +0,0 @@ -module Authentication - module Errors - class Error < StandardError - end - - class ProviderNotFound < Error - end - - class ProviderNotEnabled < Error - end - end -end diff --git a/app/services/users/delete.rb b/app/services/users/delete.rb index f4bb6da42..ba09e97d7 100644 --- a/app/services/users/delete.rb +++ b/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 diff --git a/db/migrate/20210201055410_create_users_suspended_usernames.rb b/db/migrate/20210201055410_create_users_suspended_usernames.rb new file mode 100644 index 000000000..ddd87d280 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 97bb009da..edea010c6 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/factories/suspended_usernames.rb b/spec/factories/suspended_usernames.rb new file mode 100644 index 000000000..f520acb4f --- /dev/null +++ b/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 diff --git a/spec/models/users/suspended_username_spec.rb b/spec/models/users/suspended_username_spec.rb new file mode 100644 index 000000000..e212e513c --- /dev/null +++ b/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 diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 287b85752..f3294afff 100644 --- a/spec/services/users/delete_spec.rb +++ b/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 diff --git a/spec/system/authentication/user_with_suspended_username_spec.rb b/spec/system/authentication/user_with_suspended_username_spec.rb new file mode 100644 index 000000000..ce4c2a999 --- /dev/null +++ b/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