From 68699a2826468a251c2a40f3d4e4e5ad66e7bd79 Mon Sep 17 00:00:00 2001 From: Osman Latif Date: Wed, 26 Jul 2023 10:59:50 -0500 Subject: [PATCH 1/4] LG-10313: Blocking associated emails for Gmail when suspended changelog: Internal, User suspension, Add code to block associated emails for user suspension --- app/forms/register_user_email_form.rb | 6 +++ app/models/email_address.rb | 1 + app/models/suspended_email.rb | 14 ++++++ app/models/user.rb | 11 +++++ ...720183509_create_suspended_emails_table.rb | 9 ++++ db/schema.rb | 11 ++++- spec/factories/suspended_emails.rb | 6 +++ spec/forms/register_user_email_form_spec.rb | 28 +++++++++++- spec/models/suspended_email_spec.rb | 44 +++++++++++++++++++ spec/models/user_spec.rb | 26 ++++++++++- 10 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 app/models/suspended_email.rb create mode 100644 db/primary_migrate/20230720183509_create_suspended_emails_table.rb create mode 100644 spec/factories/suspended_emails.rb create mode 100644 spec/models/suspended_email_spec.rb diff --git a/app/forms/register_user_email_form.rb b/app/forms/register_user_email_form.rb index 17493594957..85502fc0043 100644 --- a/app/forms/register_user_email_form.rb +++ b/app/forms/register_user_email_form.rb @@ -85,6 +85,8 @@ def process_successful_submission(request_id, instructions) # already taken and if so, we act as if the user registration was successful. if email_address_record&.user&.suspended? send_suspended_user_email(email_address_record) + elsif blocked_email_address + send_suspended_user_email(blocked_email_address) elsif email_taken? && user_unconfirmed? update_user_language_preference send_sign_up_unconfirmed_email(request_id) @@ -175,4 +177,8 @@ def existing_user def email_request_id(request_id) request_id if request_id.present? && ServiceProviderRequestProxy.find_by(uuid: request_id) end + + def blocked_email_address + @blocked_email_address ||= SuspendedEmail.blocked_email_address(email) + end end diff --git a/app/models/email_address.rb b/app/models/email_address.rb index 967c5fe9011..e8d93fee412 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -6,6 +6,7 @@ class EmailAddress < ApplicationRecord belongs_to :user, inverse_of: :email_addresses validates :encrypted_email, presence: true validates :email_fingerprint, presence: true + has_many :suspended_emails, inverse_of: :email_address, dependent: :destroy scope :confirmed, -> { where('confirmed_at IS NOT NULL') } diff --git a/app/models/suspended_email.rb b/app/models/suspended_email.rb new file mode 100644 index 00000000000..e5049fad5bf --- /dev/null +++ b/app/models/suspended_email.rb @@ -0,0 +1,14 @@ +class SuspendedEmail < ApplicationRecord + belongs_to :email_address, inverse_of: :suspended_emails + validates :digested_base_email, presence: true + + def self.generate_email_digest(email) + normalized_email = EmailNormalizer.new(email).normalized_email + OpenSSL::Digest::SHA256.hexdigest(normalized_email) + end + + def self.blocked_email_address(email) + digested_base_email = generate_email_digest(email) + find_by(digested_base_email: digested_base_email)&.email_address + end +end diff --git a/app/models/user.rb b/app/models/user.rb index d132e59ef93..a1ab0b817e0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -122,6 +122,13 @@ def suspend! OutOfBandSessionAccessor.new(unique_session_id).destroy if unique_session_id update!(suspended_at: Time.zone.now, unique_session_id: nil) analytics.user_suspended(success: true) + email_addresses.flat_map do |email_address| + digested_base_email = SuspendedEmail.generate_email_digest(email_address.email) + SuspendedEmail.create!( + digested_base_email: digested_base_email, + email_address: email_address, + ) + end end def reinstate! @@ -131,6 +138,10 @@ def reinstate! end update!(reinstated_at: Time.zone.now) analytics.user_reinstated(success: true) + email_addresses.flat_map do |email_address| + digested_base_email = SuspendedEmail.generate_email_digest(email_address.email) + SuspendedEmail.find_by(digested_base_email: digested_base_email)&.destroy + end end def pending_profile diff --git a/db/primary_migrate/20230720183509_create_suspended_emails_table.rb b/db/primary_migrate/20230720183509_create_suspended_emails_table.rb new file mode 100644 index 00000000000..494cce27e04 --- /dev/null +++ b/db/primary_migrate/20230720183509_create_suspended_emails_table.rb @@ -0,0 +1,9 @@ +class CreateSuspendedEmailsTable < ActiveRecord::Migration[7.0] + def change + create_table :suspended_emails do |t| + t.references :email_address, null: false + t.string :digested_base_email, null: false, index: true + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index bcf0914a68b..bfb90ef358f 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[7.0].define(version: 2023_07_07_144310) do +ActiveRecord::Schema[7.0].define(version: 2023_07_20_183509) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "pgcrypto" @@ -572,6 +572,15 @@ t.index ["request_id"], name: "index_sp_return_logs_on_request_id", unique: true end + create_table "suspended_emails", force: :cascade do |t| + t.bigint "email_address_id", null: false + t.string "digested_base_email", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["digested_base_email"], name: "index_suspended_emails_on_digested_base_email" + t.index ["email_address_id"], name: "index_suspended_emails_on_email_address_id" + end + create_table "users", id: :serial, force: :cascade do |t| t.string "reset_password_token", limit: 255 t.datetime "reset_password_sent_at", precision: nil diff --git a/spec/factories/suspended_emails.rb b/spec/factories/suspended_emails.rb new file mode 100644 index 00000000000..a018f738036 --- /dev/null +++ b/spec/factories/suspended_emails.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :suspended_email do + digested_base_email { 'test_digest' } + association :email_address + end +end diff --git a/spec/forms/register_user_email_form_spec.rb b/spec/forms/register_user_email_form_spec.rb index ddbbdca081c..8f25d7c68b6 100644 --- a/spec/forms/register_user_email_form_spec.rb +++ b/spec/forms/register_user_email_form_spec.rb @@ -8,7 +8,7 @@ it_behaves_like 'email validation' describe '#submit' do - let(:email_domain) { 'test.com' } + let(:email_domain) { 'gmail.com' } let(:registered_email_address) { 'taken@' + email_domain } let(:unregistered_email_address) { 'not_taken@' + email_domain } let(:registered_and_confirmed_user) do @@ -68,6 +68,32 @@ end end + context 'email submission with special characters' do + context 'mx record are gmail' do + shared_examples 'blocked email address' do |email_address| + it 'sends the email with error code' do + user = create(*registered_and_confirmed_user) + user.suspend! + + subject.submit(email: email_address, terms_accepted: '1') + + expect_delivered_email_count(1) + expect_delivered_email( + to: [registered_email_address], + subject: t('user_mailer.suspended_create_account.subject'), + ) + expect(subject.send(:blocked_email_address).user).to eq(user) + end + end + context 'when email contains a plus sign' do + it_behaves_like 'blocked email address', 'taken+1@gmail.com' + end + context 'when email contains a dot' do + it_behaves_like 'blocked email address', 'tak.en@gmail.com' + end + end + end + let(:variation_of_preexisting_email) { 'TAKEN@' + email_domain } context 'when email is already taken' do let!(:existing_user) { create(*registered_and_confirmed_user) } diff --git a/spec/models/suspended_email_spec.rb b/spec/models/suspended_email_spec.rb new file mode 100644 index 00000000000..cc9e867bcef --- /dev/null +++ b/spec/models/suspended_email_spec.rb @@ -0,0 +1,44 @@ +require 'rails_helper' + +RSpec.describe SuspendedEmail, type: :model do + describe 'associations' do + it { should belong_to(:email_address).class_name('EmailAddress') } + end + + describe 'validations' do + it { should validate_presence_of(:digested_base_email) } + end + + describe '.generate_email_digest' do + it 'generates the correct digest for a given email' do + email = 'test@example.com' + expected_digest = OpenSSL::Digest.hexdigest('SHA256', 'test@example.com') + + expect(SuspendedEmail.generate_email_digest(email)).to eq(expected_digest) + end + end + + describe '.blocked_email_address' do + context 'when the email is not blocked' do + it 'returns nil' do + email = 'not_blocked@example.com' + + expect(SuspendedEmail.blocked_email_address(email)).to be_nil + end + end + + context 'when the email is blocked' do + it 'returns the original email address' do + blocked_email = FactoryBot.create(:email_address, email: 'blocked@example.com') + digested_base_email = SuspendedEmail.generate_email_digest('blocked@example.com') + FactoryBot.create( + :suspended_email, + digested_base_email: digested_base_email, + email_address: blocked_email, + ) + + expect(SuspendedEmail.blocked_email_address('blocked@example.com')).to eq(blocked_email) + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4f488a917b6..bcacca6147e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -689,7 +689,7 @@ end describe 'user suspension' do - let(:user) { User.new } + let(:user) { create(:user, email: 'foo@gmail.com') } let(:cannot_reinstate_message) { :user_is_not_suspended } let(:cannot_suspend_message) { :user_already_suspended } @@ -768,6 +768,17 @@ UpdateUser.new(user: user, attributes: { unique_session_id: mock_session_id }).call end + it 'creates SuspendedEmail records for each email address' do + user_email_address = user.email_addresses.last + expect(SuspendedEmail).to receive(:generate_email_digest).with(user_email_address.email). + and_return('digest1') + expect(SuspendedEmail).to receive(:create!).with( + digested_base_email: 'digest1', + email_address: user_email_address, + ) + user.suspend! + end + it 'updates the suspended_at attribute with the current time' do expect do user.suspend! @@ -825,6 +836,19 @@ user.suspended_at = Time.zone.now user.reinstated_at = nil end + + it 'destroys SuspendedEmail records for each email address' do + suspended_email = double + expect(SuspendedEmail).to receive(:generate_email_digest). + with(user.email_addresses.last.email). + and_return('digest1') + expect(SuspendedEmail).to receive(:find_by).with( + digested_base_email: 'digest1', + ).and_return(suspended_email) + expect(suspended_email).to receive(:destroy) + user.reinstate! + end + it 'updates the reinstated_at attribute with the current time' do expect do user.reinstate! From 2a49b2256878b4a30dfb2eba2c8c02510c235aad Mon Sep 17 00:00:00 2001 From: Osman Latif Date: Wed, 26 Jul 2023 14:12:39 -0500 Subject: [PATCH 2/4] feedback --- app/forms/register_user_email_form.rb | 3 ++- app/models/email_address.rb | 2 +- app/models/suspended_email.rb | 24 ++++++++++++++++-------- app/models/user.rb | 13 ++++--------- spec/models/suspended_email_spec.rb | 6 +++--- spec/models/user_spec.rb | 24 ++++++------------------ 6 files changed, 32 insertions(+), 40 deletions(-) diff --git a/app/forms/register_user_email_form.rb b/app/forms/register_user_email_form.rb index 85502fc0043..a0d1efc3ffd 100644 --- a/app/forms/register_user_email_form.rb +++ b/app/forms/register_user_email_form.rb @@ -179,6 +179,7 @@ def email_request_id(request_id) end def blocked_email_address - @blocked_email_address ||= SuspendedEmail.blocked_email_address(email) + return @blocked_email_address if defined?(@blocked_email_address) + @blocked_email_address = SuspendedEmail.find_with_email(email) end end diff --git a/app/models/email_address.rb b/app/models/email_address.rb index e8d93fee412..6d626ba2128 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -6,7 +6,7 @@ class EmailAddress < ApplicationRecord belongs_to :user, inverse_of: :email_addresses validates :encrypted_email, presence: true validates :email_fingerprint, presence: true - has_many :suspended_emails, inverse_of: :email_address, dependent: :destroy + has_one :suspended_email, dependent: :destroy scope :confirmed, -> { where('confirmed_at IS NOT NULL') } diff --git a/app/models/suspended_email.rb b/app/models/suspended_email.rb index e5049fad5bf..2e0005c4c7e 100644 --- a/app/models/suspended_email.rb +++ b/app/models/suspended_email.rb @@ -1,14 +1,22 @@ class SuspendedEmail < ApplicationRecord - belongs_to :email_address, inverse_of: :suspended_emails + belongs_to :email_address validates :digested_base_email, presence: true - def self.generate_email_digest(email) - normalized_email = EmailNormalizer.new(email).normalized_email - OpenSSL::Digest::SHA256.hexdigest(normalized_email) - end + class << self + def generate_email_digest(email) + normalized_email = EmailNormalizer.new(email).normalized_email + OpenSSL::Digest::SHA256.hexdigest(normalized_email) + end + + def create_from_email_adddress!(email_address) + create!( + digested_base_email: generate_email_digest(email_address.email), + email_address: email_address, + ) + end - def self.blocked_email_address(email) - digested_base_email = generate_email_digest(email) - find_by(digested_base_email: digested_base_email)&.email_address + def find_with_email(email) + find_by(digested_base_email: generate_email_digest(email))&.email_address + end end end diff --git a/app/models/user.rb b/app/models/user.rb index a1ab0b817e0..5db2d427d9b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -122,12 +122,8 @@ def suspend! OutOfBandSessionAccessor.new(unique_session_id).destroy if unique_session_id update!(suspended_at: Time.zone.now, unique_session_id: nil) analytics.user_suspended(success: true) - email_addresses.flat_map do |email_address| - digested_base_email = SuspendedEmail.generate_email_digest(email_address.email) - SuspendedEmail.create!( - digested_base_email: digested_base_email, - email_address: email_address, - ) + email_addresses.map do |email_address| + SuspendedEmail.create_from_email_adddress!(email_address) end end @@ -138,9 +134,8 @@ def reinstate! end update!(reinstated_at: Time.zone.now) analytics.user_reinstated(success: true) - email_addresses.flat_map do |email_address| - digested_base_email = SuspendedEmail.generate_email_digest(email_address.email) - SuspendedEmail.find_by(digested_base_email: digested_base_email)&.destroy + email_addresses.map do |email_address| + SuspendedEmail.find_with_email(email_address.email)&.destroy end end diff --git a/spec/models/suspended_email_spec.rb b/spec/models/suspended_email_spec.rb index cc9e867bcef..bfb06ec4002 100644 --- a/spec/models/suspended_email_spec.rb +++ b/spec/models/suspended_email_spec.rb @@ -12,7 +12,7 @@ describe '.generate_email_digest' do it 'generates the correct digest for a given email' do email = 'test@example.com' - expected_digest = OpenSSL::Digest.hexdigest('SHA256', 'test@example.com') + expected_digest = Digest::SHA256.hexdigest('test@example.com') expect(SuspendedEmail.generate_email_digest(email)).to eq(expected_digest) end @@ -23,7 +23,7 @@ it 'returns nil' do email = 'not_blocked@example.com' - expect(SuspendedEmail.blocked_email_address(email)).to be_nil + expect(SuspendedEmail.find_with_email(email)).to be_nil end end @@ -37,7 +37,7 @@ email_address: blocked_email, ) - expect(SuspendedEmail.blocked_email_address('blocked@example.com')).to eq(blocked_email) + expect(SuspendedEmail.find_with_email('blocked@example.com')).to eq(blocked_email) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bcacca6147e..2b5a776569c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -769,14 +769,7 @@ end it 'creates SuspendedEmail records for each email address' do - user_email_address = user.email_addresses.last - expect(SuspendedEmail).to receive(:generate_email_digest).with(user_email_address.email). - and_return('digest1') - expect(SuspendedEmail).to receive(:create!).with( - digested_base_email: 'digest1', - email_address: user_email_address, - ) - user.suspend! + expect { user.suspend! }.to(change { SuspendedEmail.count }.by(1)) end it 'updates the suspended_at attribute with the current time' do @@ -833,20 +826,15 @@ describe '#reinstate!' do before do - user.suspended_at = Time.zone.now + user.suspend! user.reinstated_at = nil end it 'destroys SuspendedEmail records for each email address' do - suspended_email = double - expect(SuspendedEmail).to receive(:generate_email_digest). - with(user.email_addresses.last.email). - and_return('digest1') - expect(SuspendedEmail).to receive(:find_by).with( - digested_base_email: 'digest1', - ).and_return(suspended_email) - expect(suspended_email).to receive(:destroy) - user.reinstate! + email_address = user.email_addresses.last + expect { user.reinstate! }.to change { + SuspendedEmail.find_with_email(email_address.email) + }.from(email_address).to(nil) end it 'updates the reinstated_at attribute with the current time' do From b309dfb19500adc3df8a286b019755d8c559dd13 Mon Sep 17 00:00:00 2001 From: Osman Latif Date: Wed, 26 Jul 2023 14:45:43 -0500 Subject: [PATCH 3/4] feedack --- spec/models/user_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2b5a776569c..0ce1af29431 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -689,7 +689,7 @@ end describe 'user suspension' do - let(:user) { create(:user, email: 'foo@gmail.com') } + let(:user) { create(:user) } let(:cannot_reinstate_message) { :user_is_not_suspended } let(:cannot_suspend_message) { :user_already_suspended } @@ -832,9 +832,9 @@ it 'destroys SuspendedEmail records for each email address' do email_address = user.email_addresses.last - expect { user.reinstate! }.to change { - SuspendedEmail.find_with_email(email_address.email) - }.from(email_address).to(nil) + expect { user.reinstate! }. + to(change { SuspendedEmail.find_with_email(email_address.email) }. + from(email_address).to(nil)) end it 'updates the reinstated_at attribute with the current time' do From 5fb3d0146507059a7853cd25af67ba9800f2491e Mon Sep 17 00:00:00 2001 From: Osman Latif Date: Wed, 26 Jul 2023 15:03:01 -0500 Subject: [PATCH 4/4] removing dependent destroy --- app/models/email_address.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/email_address.rb b/app/models/email_address.rb index 6d626ba2128..b451f4afe04 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -6,7 +6,9 @@ class EmailAddress < ApplicationRecord belongs_to :user, inverse_of: :email_addresses validates :encrypted_email, presence: true validates :email_fingerprint, presence: true - has_one :suspended_email, dependent: :destroy + # rubocop:disable Rails/HasManyOrHasOneDependent + has_one :suspended_email + # rubocop:enable Rails/HasManyOrHasOneDependent scope :confirmed, -> { where('confirmed_at IS NOT NULL') }