From df7401f71c18f936eb4ee7a8ea5a51935e255a5a Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Wed, 21 May 2025 09:15:39 +0530 Subject: [PATCH] fix: account email validation during signup (#11307) - Refactor email validation logic to be a service - Use the service for both email/pass signup and Google SSO - fix account email validation during signup - Use `blocked_domain` setting for both email/pass signup and Google Sign In [`BLOCKED_DOMAIN` via GlobalConfig] - add specs for `account_builder` - add specs for the new service --------- Co-authored-by: Sojan Jose --- app/builders/account_builder.rb | 26 +------ .../omniauth_callbacks_controller.rb | 10 ++- .../sign_up_email_validation_service.rb | 36 +++++++++ spec/builders/account_builder_spec.rb | 57 ++++++++++++++ .../omniauth_callbacks_controller_spec.rb | 65 ++++++++++------ .../sign_up_email_validation_service_spec.rb | 74 +++++++++++++++++++ 6 files changed, 215 insertions(+), 53 deletions(-) create mode 100644 app/services/account/sign_up_email_validation_service.rb create mode 100644 spec/builders/account_builder_spec.rb create mode 100644 spec/services/account/sign_up_email_validation_service_spec.rb diff --git a/app/builders/account_builder.rb b/app/builders/account_builder.rb index 6f7980586..532487a1b 100644 --- a/app/builders/account_builder.rb +++ b/app/builders/account_builder.rb @@ -32,14 +32,7 @@ class AccountBuilder end def validate_email - raise InvalidEmail.new({ domain_blocked: domain_blocked }) if domain_blocked? - - address = ValidEmail2::Address.new(@email) - if address.valid? && !address.disposable? - true - else - raise InvalidEmail.new({ valid: address.valid?, disposable: address.disposable? }) - end + Account::SignUpEmailValidationService.new(@email).perform end def validate_user @@ -81,21 +74,4 @@ class AccountBuilder @user.confirm if @confirmed @user.save! end - - def domain_blocked? - domain = @email.split('@').last - - blocked_domains.each do |blocked_domain| - return true if domain.match?(blocked_domain) - end - - false - end - - def blocked_domains - domains = GlobalConfigService.load('BLOCKED_EMAIL_DOMAINS', '') - return [] if domains.blank? - - domains.split("\n").map(&:strip) - end end diff --git a/app/controllers/devise_overrides/omniauth_callbacks_controller.rb b/app/controllers/devise_overrides/omniauth_callbacks_controller.rb index 2b3ea9067..db312e94f 100644 --- a/app/controllers/devise_overrides/omniauth_callbacks_controller.rb +++ b/app/controllers/devise_overrides/omniauth_callbacks_controller.rb @@ -21,7 +21,7 @@ class DeviseOverrides::OmniauthCallbacksController < DeviseTokenAuth::OmniauthCa def sign_up_user return redirect_to login_page_url(error: 'no-account-found') unless account_signup_allowed? - return redirect_to login_page_url(error: 'business-account-only') unless validate_business_account? + return redirect_to login_page_url(error: 'business-account-only') unless validate_signup_email_is_business_domain? create_account_for_user token = @resource.send(:set_reset_password_token) @@ -53,9 +53,11 @@ class DeviseOverrides::OmniauthCallbacksController < DeviseTokenAuth::OmniauthCa ).first end - def validate_business_account? - # return true if the user is a business account, false if it is a gmail account - auth_hash['info']['email'].downcase.exclude?('@gmail.com') + def validate_signup_email_is_business_domain? + # return true if the user is a business account, false if it is a blocked domain account + Account::SignUpEmailValidationService.new(auth_hash['info']['email']).perform + rescue CustomExceptions::Account::InvalidEmail + false end def create_account_for_user diff --git a/app/services/account/sign_up_email_validation_service.rb b/app/services/account/sign_up_email_validation_service.rb new file mode 100644 index 000000000..8889145b2 --- /dev/null +++ b/app/services/account/sign_up_email_validation_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +class Account::SignUpEmailValidationService + include CustomExceptions::Account + attr_reader :email + + def initialize(email) + @email = email + end + + def perform + address = ValidEmail2::Address.new(email) + + raise InvalidEmail.new({ valid: false, disposable: nil }) unless address.valid? + + raise InvalidEmail.new({ domain_blocked: true }) if domain_blocked? + + raise InvalidEmail.new({ valid: true, disposable: true }) if address.disposable? + + true + end + + private + + def domain_blocked? + domain = email.split('@').last&.downcase + blocked_domains.any? { |blocked_domain| domain.match?(blocked_domain.downcase) } + end + + def blocked_domains + domains = GlobalConfigService.load('BLOCKED_EMAIL_DOMAINS', '') + return [] if domains.blank? + + domains.split("\n").map(&:strip) + end +end diff --git a/spec/builders/account_builder_spec.rb b/spec/builders/account_builder_spec.rb new file mode 100644 index 000000000..86cb4cf73 --- /dev/null +++ b/spec/builders/account_builder_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe AccountBuilder do + let(:email) { 'user@example.com' } + let(:user_password) { 'Password123!' } + let(:account_name) { 'Test Account' } + let(:user_full_name) { 'Test User' } + let(:validation_service) { instance_double(Account::SignUpEmailValidationService, perform: true) } + let(:account_builder) do + described_class.new( + account_name: account_name, + email: email, + user_full_name: user_full_name, + user_password: user_password, + confirmed: true + ) + end + + # Mock the email validation service + before do + allow(Account::SignUpEmailValidationService).to receive(:new).with(email).and_return(validation_service) + end + + describe '#perform' do + context 'when valid params are passed' do + it 'creates a new account with correct name' do + _user, account = account_builder.perform + expect(account).to be_an(Account) + expect(account.name).to eq(account_name) + end + + it 'creates a new confirmed user with correct details' do + user, _account = account_builder.perform + expect(user).to be_a(User) + expect(user.email).to eq(email) + expect(user.name).to eq(user_full_name) + expect(user.confirmed?).to be(true) + end + + it 'links user to account as administrator' do + user, account = account_builder.perform + expect(user.account_users.first.role).to eq('administrator') + expect(user.accounts.first).to eq(account) + end + + it 'increments the counts of models' do + expect do + account_builder.perform + end.to change(Account, :count).by(1) + .and change(User, :count).by(1) + .and change(AccountUser, :count).by(1) + end + end + end +end diff --git a/spec/controllers/devise/omniauth_callbacks_controller_spec.rb b/spec/controllers/devise/omniauth_callbacks_controller_spec.rb index 3513b9c34..0a9c82f53 100644 --- a/spec/controllers/devise/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/devise/omniauth_callbacks_controller_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do let(:account_builder) { double } let(:user_double) { object_double(:user) } + let(:email_validation_service) { instance_double(Account::SignUpEmailValidationService) } def set_omniauth_config(for_email = 'test@example.com') OmniAuth.config.test_mode = true @@ -17,13 +18,18 @@ RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do ) end + before do + allow(Account::SignUpEmailValidationService).to receive(:new).and_return(email_validation_service) + end + describe '#omniauth_sucess' do it 'allows signup' do - with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true', FRONTEND_URL: 'http://www.example.com' do set_omniauth_config('test_not_preset@example.com') allow(AccountBuilder).to receive(:new).and_return(account_builder) allow(account_builder).to receive(:perform).and_return(user_double) allow(Avatar::AvatarFromUrlJob).to receive(:perform_later).and_return(true) + allow(email_validation_service).to receive(:perform).and_return(true) get '/omniauth/google_oauth2/callback' @@ -43,8 +49,10 @@ RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do end it 'blocks personal accounts signup' do - with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true', FRONTEND_URL: 'http://www.example.com' do set_omniauth_config('personal@gmail.com') + allow(email_validation_service).to receive(:perform).and_raise(CustomExceptions::Account::InvalidEmail.new({ valid: false, disposable: nil })) + get '/omniauth/google_oauth2/callback' # expect a 302 redirect to auth/google_oauth2/callback @@ -57,10 +65,13 @@ RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do end it 'blocks personal accounts signup with different Gmail case variations' do - with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'true', FRONTEND_URL: 'http://www.example.com' do # Test different case variations of Gmail ['personal@Gmail.com', 'personal@GMAIL.com', 'personal@Gmail.COM'].each do |email| set_omniauth_config(email) + allow(email_validation_service).to receive(:perform).and_raise(CustomExceptions::Account::InvalidEmail.new({ valid: false, + disposable: nil })) + get '/omniauth/google_oauth2/callback' # expect a 302 redirect to auth/google_oauth2/callback @@ -76,8 +87,10 @@ RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do # This test does not affect line coverage, but it is important to ensure that the logic # does not allow any signup if the ENV explicitly disables it it 'blocks signup if ENV disabled' do - with_modified_env ENABLE_ACCOUNT_SIGNUP: 'false' do + with_modified_env ENABLE_ACCOUNT_SIGNUP: 'false', FRONTEND_URL: 'http://www.example.com' do set_omniauth_config('does-not-exist-for-sure@example.com') + allow(email_validation_service).to receive(:perform).and_return(true) + get '/omniauth/google_oauth2/callback' # expect a 302 redirect to auth/google_oauth2/callback @@ -90,38 +103,42 @@ RSpec.describe 'DeviseOverrides::OmniauthCallbacksController', type: :request do end it 'allows login' do - create(:user, email: 'test@example.com') - set_omniauth_config('test@example.com') + with_modified_env FRONTEND_URL: 'http://www.example.com' do + create(:user, email: 'test@example.com') + set_omniauth_config('test@example.com') - get '/omniauth/google_oauth2/callback' - # expect a 302 redirect to auth/google_oauth2/callback - expect(response).to redirect_to('http://www.example.com/auth/google_oauth2/callback') + get '/omniauth/google_oauth2/callback' + # expect a 302 redirect to auth/google_oauth2/callback + expect(response).to redirect_to('http://www.example.com/auth/google_oauth2/callback') - follow_redirect! - expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) + follow_redirect! + expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) - # expect app/login page to respond with 200 and render - follow_redirect! - expect(response).to have_http_status(:ok) + # expect app/login page to respond with 200 and render + follow_redirect! + expect(response).to have_http_status(:ok) + end end # from a line coverage point of view this may seem redundant # but to ensure that the logic allows for existing users even if they have a gmail account # we need to test this explicitly it 'allows personal account login' do - create(:user, email: 'personal-existing@gmail.com') - set_omniauth_config('personal-existing@gmail.com') + with_modified_env FRONTEND_URL: 'http://www.example.com' do + create(:user, email: 'personal-existing@gmail.com') + set_omniauth_config('personal-existing@gmail.com') - get '/omniauth/google_oauth2/callback' - # expect a 302 redirect to auth/google_oauth2/callback - expect(response).to redirect_to('http://www.example.com/auth/google_oauth2/callback') + get '/omniauth/google_oauth2/callback' + # expect a 302 redirect to auth/google_oauth2/callback + expect(response).to redirect_to('http://www.example.com/auth/google_oauth2/callback') - follow_redirect! - expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) + follow_redirect! + expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) - # expect app/login page to respond with 200 and render - follow_redirect! - expect(response).to have_http_status(:ok) + # expect app/login page to respond with 200 and render + follow_redirect! + expect(response).to have_http_status(:ok) + end end end end diff --git a/spec/services/account/sign_up_email_validation_service_spec.rb b/spec/services/account/sign_up_email_validation_service_spec.rb new file mode 100644 index 000000000..3f907f02c --- /dev/null +++ b/spec/services/account/sign_up_email_validation_service_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Account::SignUpEmailValidationService, type: :service do + let(:service) { described_class.new(email) } + let(:blocked_domains) { "gmail.com\noutlook.com" } + let(:valid_email_address) { instance_double(ValidEmail2::Address, valid?: true, disposable?: false) } + let(:disposable_email_address) { instance_double(ValidEmail2::Address, valid?: true, disposable?: true) } + let(:invalid_email_address) { instance_double(ValidEmail2::Address, valid?: false) } + + before do + allow(GlobalConfigService).to receive(:load).with('BLOCKED_EMAIL_DOMAINS', '').and_return(blocked_domains) + end + + describe '#perform' do + context 'when email is invalid format' do + let(:email) { 'invalid-email' } + + it 'raises InvalidEmail with invalid message' do + allow(ValidEmail2::Address).to receive(:new).with(email).and_return(invalid_email_address) + expect { service.perform }.to raise_error do |error| + expect(error).to be_a(CustomExceptions::Account::InvalidEmail) + expect(error.message).to eq(I18n.t('errors.signup.invalid_email')) + end + end + end + + context 'when domain is blocked' do + let(:email) { 'test@gmail.com' } + + it 'raises InvalidEmail with blocked domain message' do + allow(ValidEmail2::Address).to receive(:new).with(email).and_return(valid_email_address) + expect { service.perform }.to raise_error do |error| + expect(error).to be_a(CustomExceptions::Account::InvalidEmail) + expect(error.message).to eq(I18n.t('errors.signup.blocked_domain')) + end + end + end + + context 'when domain is blocked (case insensitive)' do + let(:email) { 'test@GMAIL.COM' } + + it 'raises InvalidEmail with blocked domain message' do + allow(ValidEmail2::Address).to receive(:new).with(email).and_return(valid_email_address) + expect { service.perform }.to raise_error do |error| + expect(error).to be_a(CustomExceptions::Account::InvalidEmail) + expect(error.message).to eq(I18n.t('errors.signup.blocked_domain')) + end + end + end + + context 'when email is from disposable provider' do + let(:email) { 'test@mailinator.com' } + + it 'raises InvalidEmail with disposable message' do + allow(ValidEmail2::Address).to receive(:new).with(email).and_return(disposable_email_address) + expect { service.perform }.to raise_error do |error| + expect(error).to be_a(CustomExceptions::Account::InvalidEmail) + expect(error.message).to eq(I18n.t('errors.signup.disposable_email')) + end + end + end + + context 'when email is valid business email' do + let(:email) { 'test@example.com' } + + it 'returns true' do + allow(ValidEmail2::Address).to receive(:new).with(email).and_return(valid_email_address) + expect(service.perform).to be(true) + end + end + end +end