diff --git a/app/forms/register_user_email_form.rb b/app/forms/register_user_email_form.rb index 1a57677eebc..85706e9900a 100644 --- a/app/forms/register_user_email_form.rb +++ b/app/forms/register_user_email_form.rb @@ -25,10 +25,26 @@ def user @user ||= User.new end + def rate_limited? + @rate_limited + end + def email email_address&.email end + def email_fingerprint + email_address&.email_fingerprint + end + + def normalized_email + @normalized_email ||= EmailNormalizer.new(email).normalized_email + end + + def digested_base_email + @digested_base_email ||= OpenSSL::Digest::SHA256.hexdigest(normalized_email) + end + def validate_terms_accepted return if @terms_accepted || email_address_record&.user&.accepted_terms_at.present? @@ -93,13 +109,7 @@ def process_successful_submission(request_id, instructions) elsif email_taken? send_sign_up_confirmed_email else - user.accepted_terms_at = Time.zone.now - user.save! - SendSignUpEmailConfirmation.new(user).call( - request_id: email_request_id(request_id), - instructions: instructions, - password_reset_requested: password_reset_requested?, - ) + send_sign_up_email(request_id, instructions) end end @@ -114,16 +124,46 @@ def extra_analytics_attributes email_already_exists: email_taken?, user_id: user.uuid || existing_user.uuid, domain_name: email&.split('@')&.last, - rate_limited: @rate_limited, + rate_limited: rate_limited?, } end - def send_sign_up_unconfirmed_email(request_id) - rate_limiter = RateLimiter.new(user: existing_user, rate_limit_type: :reg_unconfirmed_email) + def rate_limit!(rate_limit_type) + rate_limiter = RateLimiter.new( + target: digested_base_email, + rate_limit_type: rate_limit_type, + ) + rate_limiter.increment! @rate_limited = rate_limiter.limited? + end + + def send_sign_up_email(request_id, instructions) + rate_limit!(:reg_unconfirmed_email) + + if rate_limited? + @analytics.rate_limit_reached( + limiter_type: :reg_unconfirmed_email, + ) + @attempts_tracker.user_registration_email_submission_rate_limited( + email: email, email_already_registered: false, + ) + else + user.accepted_terms_at = Time.zone.now + user.save! + + SendSignUpEmailConfirmation.new(user).call( + request_id: email_request_id(request_id), + instructions: instructions, + password_reset_requested: password_reset_requested?, + ) + end + end - if @rate_limited + def send_sign_up_unconfirmed_email(request_id) + rate_limit!(:reg_unconfirmed_email) + + if rate_limited? @analytics.rate_limit_reached( limiter_type: :reg_unconfirmed_email, ) @@ -136,11 +176,9 @@ def send_sign_up_unconfirmed_email(request_id) end def send_sign_up_confirmed_email - rate_limiter = RateLimiter.new(user: existing_user, rate_limit_type: :reg_confirmed_email) - rate_limiter.increment! - @rate_limited = rate_limiter.limited? + rate_limit!(:reg_confirmed_email) - if @rate_limited + if rate_limited? @analytics.rate_limit_reached( limiter_type: :reg_confirmed_email, ) @@ -166,8 +204,8 @@ def user_unconfirmed? def email_address_record return @email_address_record if defined?(@email_address_record) + @email_address_record = EmailAddress.find_with_email(email) - @email_address_record end def existing_user @@ -180,6 +218,7 @@ def email_request_id(request_id) def blocked_email_address return @blocked_email_address if defined?(@blocked_email_address) - @blocked_email_address = SuspendedEmail.find_with_email(email) + + @blocked_email_address = SuspendedEmail.find_with_email_digest(digested_base_email) end end diff --git a/app/models/suspended_email.rb b/app/models/suspended_email.rb index 5a1011bc884..ac46081b80f 100644 --- a/app/models/suspended_email.rb +++ b/app/models/suspended_email.rb @@ -18,5 +18,9 @@ def create_from_email_address!(email_address) def find_with_email(email) find_by(digested_base_email: generate_email_digest(email))&.email_address end + + def find_with_email_digest(digest) + find_by(digested_base_email: digest)&.email_address + end end end diff --git a/app/services/email_normalizer.rb b/app/services/email_normalizer.rb index acfac75488a..4c298cbe8cb 100644 --- a/app/services/email_normalizer.rb +++ b/app/services/email_normalizer.rb @@ -28,6 +28,7 @@ def gmail? def google_mx_record? return false if ENV['RAILS_OFFLINE'] + return false if email.domain.blank? || !email.domain.to_s.ascii_only? mx_records(email.domain).any? { |domain| domain.end_with?('google.com') } end diff --git a/spec/features/visitors/sign_up_with_email_spec.rb b/spec/features/visitors/sign_up_with_email_spec.rb index ee559e7d763..409b324821f 100644 --- a/spec/features/visitors/sign_up_with_email_spec.rb +++ b/spec/features/visitors/sign_up_with_email_spec.rb @@ -72,14 +72,16 @@ sign_up_with(email) starting_count = unread_emails_for(email).size - max_attempts = IdentityConfig.store.reg_unconfirmed_email_max_attempts - (max_attempts - 1).times do |i| + remaining_attempts = IdentityConfig.store. + reg_unconfirmed_email_max_attempts - 1 - starting_count + + 1.upto(remaining_attempts) do |i| sign_up_with(email) - expect(unread_emails_for(email).size).to eq(starting_count + i + 1) + expect(unread_emails_for(email).size).to eq(starting_count + i) end - expect(unread_emails_for(email).size).to eq(starting_count + max_attempts - 1) + expect(unread_emails_for(email).size).to eq(starting_count + remaining_attempts) sign_up_with(email) - expect(unread_emails_for(email).size).to eq(starting_count + max_attempts - 1) + expect(unread_emails_for(email).size).to eq(starting_count + remaining_attempts) end end diff --git a/spec/forms/register_user_email_form_spec.rb b/spec/forms/register_user_email_form_spec.rb index 60fa95cbb96..495751d73a8 100644 --- a/spec/forms/register_user_email_form_spec.rb +++ b/spec/forms/register_user_email_form_spec.rb @@ -8,8 +8,10 @@ it_behaves_like 'email validation' describe '#submit' do + let(:anonymous_uuid) { 'anonymous-uuid' } let(:email_domain) { 'gmail.com' } let(:registered_email_address) { 'taken@' + email_domain } + let(:variation_of_preexisting_email) { 'TAKEN@' + email_domain } let(:unregistered_email_address) { 'not_taken@' + email_domain } let(:registered_and_confirmed_user) do [:user, :fully_registered, **{ email: registered_email_address }] @@ -94,7 +96,6 @@ 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) } let(:extra_params) do @@ -191,6 +192,30 @@ limiter_type: :reg_unconfirmed_email, ) end + + context 'with the same normalized email address' do + let(:rate_limit) { IdentityConfig.store.reg_unconfirmed_email_max_attempts } + + it 'creates rate_limiter events after reaching rate_limiter limit' do + expect(attempts_tracker).to receive( + :user_registration_email_submission_rate_limited, + ).with( + email: "taken+#{rate_limit}@gmail.com", email_already_registered: false, + ) + + 1.upto(rate_limit) do |i| + RegisterUserEmailForm.new(analytics: analytics, attempts_tracker: attempts_tracker). + submit( + email: "taken+#{i}@gmail.com", terms_accepted: '1', + ) + end + + expect(analytics).to have_logged_event( + 'Rate Limit Reached', + limiter_type: :reg_unconfirmed_email, + ) + end + end end context 'when the email exists but is unconfirmed and on a confirmed user' do @@ -240,9 +265,32 @@ expect(User.find_with_email(unregistered_email_address).email_language).to eq('fr') end + + context 'with the same normalized email address' do + let(:rate_limit) { IdentityConfig.store.reg_unconfirmed_email_max_attempts } + + it 'creates rate_limiter events after reaching rate_limiter limit' do + expect(attempts_tracker).to receive( + :user_registration_email_submission_rate_limited, + ).with( + email: "taken+#{rate_limit}@gmail.com", email_already_registered: false, + ) + + 1.upto(rate_limit) do |i| + RegisterUserEmailForm.new(analytics: analytics, attempts_tracker: attempts_tracker). + submit( + email: "taken+#{i}@gmail.com", terms_accepted: '1', + ) + end + + expect(analytics).to have_logged_event( + 'Rate Limit Reached', + limiter_type: :reg_unconfirmed_email, + ) + end + end end - let(:anonymous_uuid) { 'anonymous-uuid' } context 'when email is invalid' do it 'returns false and adds errors to the form object' do invalid_email = 'invalid_email' diff --git a/spec/models/suspended_email_spec.rb b/spec/models/suspended_email_spec.rb index bfb06ec4002..c03d1587298 100644 --- a/spec/models/suspended_email_spec.rb +++ b/spec/models/suspended_email_spec.rb @@ -41,4 +41,29 @@ end end end + + describe '.find_with_email_digest' do + context 'when the email is not blocked' do + it 'returns nil' do + email = 'not_blocked@example.com' + digested_base_email = Digest::SHA256.hexdigest(email) + + expect(SuspendedEmail.find_with_email_digest(digested_base_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 = Digest::SHA256.hexdigest('blocked@example.com') + FactoryBot.create( + :suspended_email, + digested_base_email: digested_base_email, + email_address: blocked_email, + ) + + expect(SuspendedEmail.find_with_email_digest(digested_base_email)).to eq(blocked_email) + end + end + end end diff --git a/spec/services/email_normalizer_spec.rb b/spec/services/email_normalizer_spec.rb index 0afa8b6c529..53c20e7a286 100644 --- a/spec/services/email_normalizer_spec.rb +++ b/spec/services/email_normalizer_spec.rb @@ -6,6 +6,22 @@ describe '#normalized_email' do subject(:normalized_email) { normalizer.normalized_email } + context 'when email is nil' do + let(:email) { nil } + + it 'is an empty string' do + expect(normalized_email).to eq(email.to_s) + end + end + + context 'with an invalid email' do + let(:email) { 'invalid_email' } + + it 'is the same string' do + expect(normalized_email).to eq(email) + end + end + context 'with a non-gmail domain' do let(:email) { 'foobar+123@example.com' } @@ -57,5 +73,16 @@ end end end + + context 'with an internationalized domain name' do + let(:email) { 'test+1@รงร .com' } + let(:rails_offline) { false } + + before { stub_const('ENV', 'RAILS_OFFLINE' => (rails_offline ? 'TRUE' : nil)) } + + it 'is the same email' do + expect(normalized_email).to eq(email) + end + end end end