diff --git a/config/feature_flags/wip/opt_in_identity_verification.yml b/config/feature_flags/wip/opt_in_identity_verification.yml new file mode 100644 index 0000000000000000000000000000000000000000..cea057f1cb2eb1dfb1b80e0810f785c25ac2a024 --- /dev/null +++ b/config/feature_flags/wip/opt_in_identity_verification.yml @@ -0,0 +1,9 @@ +--- +name: opt_in_identity_verification +feature_issue_url: https://gitlab.com/groups/gitlab-org/modelops/anti-abuse/-/epics/32 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147068 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/455481 +milestone: '17.0' +group: group::anti-abuse +type: wip +default_enabled: false diff --git a/ee/app/assets/javascripts/pages/users/identity_verification/show/index.js b/ee/app/assets/javascripts/pages/users/identity_verification/show/index.js new file mode 100644 index 0000000000000000000000000000000000000000..1b22113c17569d00bbe1eb39c7f5d456808c16ed --- /dev/null +++ b/ee/app/assets/javascripts/pages/users/identity_verification/show/index.js @@ -0,0 +1,3 @@ +import { initIdentityVerification } from 'ee/users/identity_verification'; + +initIdentityVerification(); diff --git a/ee/app/controllers/users/base_identity_verification_controller.rb b/ee/app/controllers/users/base_identity_verification_controller.rb index f41e07bfb5fe1d3cb80e88ee72dc7374f386d439..36a0c4be7f8fe1e29c548632aa6c3b412ce7d104 100644 --- a/ee/app/controllers/users/base_identity_verification_controller.rb +++ b/ee/app/controllers/users/base_identity_verification_controller.rb @@ -139,13 +139,7 @@ def require_verification_user! redirect_to root_path end - def find_verification_user - return unless session[:verification_user_id] - - verification_user_id = session[:verification_user_id] - load_balancer_stick_request(::User, :user, verification_user_id) - User.find_by_id(verification_user_id) - end + def find_verification_user; end def redirect_banned_user return unless @user.banned? diff --git a/ee/app/controllers/users/identity_verification_controller.rb b/ee/app/controllers/users/identity_verification_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..10f7a74badfcc1d0140e5f4c879a8e6076a90cec --- /dev/null +++ b/ee/app/controllers/users/identity_verification_controller.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Users + class IdentityVerificationController < BaseIdentityVerificationController + before_action :ensure_feature_enabled + + def show; end + + private + + def ensure_feature_enabled + not_found unless ::Feature.enabled?(:opt_in_identity_verification, @user, type: :wip) + end + end +end diff --git a/ee/app/controllers/users/registrations_identity_verification_controller.rb b/ee/app/controllers/users/registrations_identity_verification_controller.rb index 7953ebec7192d780ca58b332f134816e7271fb18..e9bab84cf371ad5a5427b77ebbd38194bca21407 100644 --- a/ee/app/controllers/users/registrations_identity_verification_controller.rb +++ b/ee/app/controllers/users/registrations_identity_verification_controller.rb @@ -71,6 +71,14 @@ def require_unverified_user! redirect_to success_signup_identity_verification_path if @user.identity_verified? end + def find_verification_user + return unless session[:verification_user_id] + + verification_user_id = session[:verification_user_id] + load_balancer_stick_request(::User, :user, verification_user_id) + User.find_by_id(verification_user_id) + end + def require_arkose_verification! return unless arkose_labs_enabled? return unless @user.identities.any? diff --git a/ee/app/helpers/users/identity_verification_helper.rb b/ee/app/helpers/users/identity_verification_helper.rb index 73dc6d2b201733339719790fe65b29b42c90299d..c5c0cff09f2d72e806062c4a4fd7ab64dd49e8d3 100644 --- a/ee/app/helpers/users/identity_verification_helper.rb +++ b/ee/app/helpers/users/identity_verification_helper.rb @@ -6,7 +6,7 @@ module IdentityVerificationHelper RESTRICTED_COUNTRY_CODES = %w[CN HK MO].freeze - def identity_verification_data(user) + def signup_identity_verification_data(user) { data: { verification_state_path: verification_state_signup_identity_verification_path, @@ -21,6 +21,21 @@ def identity_verification_data(user) } end + def identity_verification_data(user) + { + data: { + verification_state_path: verification_state_identity_verification_path, + offer_phone_number_exemption: false, + phone_exemption_path: toggle_phone_exemption_signup_identity_verification_path, + credit_card: credit_card_verification_data(user), + phone_number: phone_number_verification_data(user), + email: email_verification_data(user), + arkose: arkose_labs_data, + successful_verification_path: success_signup_identity_verification_path + }.to_json + } + end + def user_banned_error_message if ::Gitlab.com? format( diff --git a/ee/app/models/concerns/identity_verifiable.rb b/ee/app/models/concerns/identity_verifiable.rb index 67967f8b26190493cd158efe3617c8d5f217292d..9283a7bde7491407e055c3ea207c88f20045d9a4 100644 --- a/ee/app/models/concerns/identity_verifiable.rb +++ b/ee/app/models/concerns/identity_verifiable.rb @@ -11,12 +11,13 @@ module IdentityVerifiable EMAIL: 'email' }.freeze - IDENTITY_VERIFICATION_EXEMPT_METHODS = %w[email].freeze + SIGNUP_IDENTITY_VERIFICATION_EXEMPT_METHODS = %w[email].freeze PHONE_NUMBER_EXEMPT_METHODS = %w[email credit_card].freeze ASSUMED_HIGH_RISK_USER_METHODS = %w[email credit_card phone].freeze HIGH_RISK_USER_METHODS = %w[email phone credit_card].freeze MEDIUM_RISK_USER_METHODS = %w[email phone].freeze LOW_RISK_USER_METHODS = %w[email].freeze + ACTIVE_USER_METHODS = %w[phone].freeze def identity_verification_enabled? return false unless ::Gitlab::Saas.feature_available?(:identity_verification) @@ -48,7 +49,7 @@ def identity_verified? # 4. User signs out and signs in again # 5. User is redirected to Identity Verification which requires them to # verify their credit card - return email_verified? if last_sign_in_at.present? + return email_verified? if active_user? identity_verification_state.values.all? end @@ -110,7 +111,7 @@ def destroy_identity_verification_exemption identity_verification_exemption_attribute&.destroy end - def exempt_from_identity_verification? + def signup_identity_verification_exempt? return true if identity_verification_exemption_attribute.present? return true if enterprise_user? return true if belongs_to_paid_namespace?(exclude_trials: true) @@ -118,18 +119,6 @@ def exempt_from_identity_verification? false end - def verification_method_enabled?(method) - case method - when 'phone' - Feature.enabled?(:identity_verification_phone_number, self) && - !PhoneVerification::Users::RateLimitService.daily_transaction_hard_limit_exceeded? - when 'credit_card' - Feature.enabled?(:identity_verification_credit_card, self) - else - true - end - end - def offer_phone_number_exemption? return false unless verification_method_enabled?('credit_card') return false unless verification_method_enabled?('phone') @@ -167,6 +156,22 @@ def verification_method_allowed?(method:) private + def verification_method_enabled?(method) + case method + when 'phone' + Feature.enabled?(:identity_verification_phone_number, self) && + !PhoneVerification::Users::RateLimitService.daily_transaction_hard_limit_exceeded? + when 'credit_card' + Feature.enabled?(:identity_verification_credit_card, self) + when 'email' + !active_user? + end + end + + def active_user? + last_sign_in_at.present? + end + def risk_profile @risk_profile ||= IdentityVerification::UserRiskProfile.new(self) end @@ -192,7 +197,22 @@ def phone_number_verification_experiment_candidate? end def determine_required_methods - return IDENTITY_VERIFICATION_EXEMPT_METHODS if exempt_from_identity_verification? + if active_user? + active_user_required_methods + else + new_user_required_methods + end + end + + def active_user_required_methods + return PHONE_NUMBER_EXEMPT_METHODS if exempt_from_phone_number_verification? + return ASSUMED_HIGH_RISK_USER_METHODS if assumed_high_risk? || affected_by_phone_verifications_limit? + + ACTIVE_USER_METHODS + end + + def new_user_required_methods + return SIGNUP_IDENTITY_VERIFICATION_EXEMPT_METHODS if signup_identity_verification_exempt? return PHONE_NUMBER_EXEMPT_METHODS if exempt_from_phone_number_verification? return ASSUMED_HIGH_RISK_USER_METHODS if assumed_high_risk? || affected_by_phone_verifications_limit? return HIGH_RISK_USER_METHODS if high_risk? diff --git a/ee/app/views/admin/users/_custom_attributes.html.haml b/ee/app/views/admin/users/_custom_attributes.html.haml index 9cd96ad6a88dd17790429968664c017b825fcefd..03fca48cd2c2545d22a16a81e376221d38fed1e6 100644 --- a/ee/app/views/admin/users/_custom_attributes.html.haml +++ b/ee/app/views/admin/users/_custom_attributes.html.haml @@ -16,7 +16,7 @@ .gl-form-group{ role: 'group' } = render Pajamas::AlertComponent.new(variant: :info, dismissible: false, alert_options: { class: 'gl-mb-5' }, title: _('Identity verification exemption')) do |c| - c.with_body do - - if @user.exempt_from_identity_verification? + - if @user.signup_identity_verification_exempt? %p= s_('This user is currently exempt from identity verification. Remove the exemption using the button below.') = render Pajamas::ButtonComponent.new(variant: :danger, href: destroy_identity_verification_exemption_admin_user_path(@user), method: :delete) do = s_('Remove identity verification exemption') diff --git a/ee/app/views/users/identity_verification/show.html.haml b/ee/app/views/users/identity_verification/show.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..20c4b1fe254298613d41e0f911322cb6c1c3488a --- /dev/null +++ b/ee/app/views/users/identity_verification/show.html.haml @@ -0,0 +1,4 @@ +- page_title s_('IdentityVerification|Help us keep GitLab secure') + +%div + #js-identity-verification{ data: identity_verification_data(@user) } diff --git a/ee/app/views/users/registrations_identity_verification/show.html.haml b/ee/app/views/users/registrations_identity_verification/show.html.haml index 0164af347f2248fd111ab0e242ef55786f338ac3..5a94a4aef6546f0145da9fecdeb0860d506a468c 100644 --- a/ee/app/views/users/registrations_identity_verification/show.html.haml +++ b/ee/app/views/users/registrations_identity_verification/show.html.haml @@ -3,4 +3,4 @@ = render_if_exists 'devise/shared/delete_unconfirmed_users_flash' %div{ data: { track_action: 'render', track_label: onboarding_status.tracking_label } } - #js-identity-verification{ data: identity_verification_data(@user) } + #js-identity-verification{ data: signup_identity_verification_data(@user) } diff --git a/ee/config/routes/identity_verification.rb b/ee/config/routes/identity_verification.rb index f069a21d0727d1066fa40da2acfa169e050a6ec9..40c6216d55822490302f508a0bba400a576eebb4 100644 --- a/ee/config/routes/identity_verification.rb +++ b/ee/config/routes/identity_verification.rb @@ -16,3 +16,9 @@ get :restricted end end + +scope '-', module: :users do + resource :identity_verification, controller: :identity_verification, only: :show do + get :verification_state + end +end diff --git a/ee/spec/helpers/users/identity_verification_helper_spec.rb b/ee/spec/helpers/users/identity_verification_helper_spec.rb index ecbbdcdaa4133b998c68b5742511db581fe6e744..1d3c308c7760335bf69a3c363efbb6e80df4853f 100644 --- a/ee/spec/helpers/users/identity_verification_helper_spec.rb +++ b/ee/spec/helpers/users/identity_verification_helper_spec.rb @@ -7,7 +7,7 @@ let_it_be_with_reload(:user) { create(:user) } - describe '#identity_verification_data' do + describe '#signup_identity_verification_data' do let(:mock_required_identity_verification_methods) { ['email'] } let(:mock_offer_phone_number_exemption) { true } @@ -30,7 +30,7 @@ stub_feature_flags(arkose_labs_phone_verification_challenge: false) end - subject(:data) { helper.identity_verification_data(user) } + subject(:data) { helper.signup_identity_verification_data(user) } context 'when no phone number for user exists' do it 'returns the expected data' do @@ -245,6 +245,17 @@ def expected_data end end + describe '#identity_verification_data' do + subject(:data) { helper.identity_verification_data(user) } + + it 'returns the expected data' do + expect(Gitlab::Json.parse(data[:data])).to include( + "verification_state_path" => verification_state_identity_verification_path, + "offer_phone_number_exemption" => false + ) + end + end + describe '#user_banned_error_message' do subject(:user_banned_error_message) { helper.user_banned_error_message } diff --git a/ee/spec/models/concerns/identity_verifiable_spec.rb b/ee/spec/models/concerns/identity_verifiable_spec.rb index c9d342329e0e24dbc277dc26d63e3618ffe9f424..65b8feb805e97f90a8f1a44e3698bb7d13e39b73 100644 --- a/ee/spec/models/concerns/identity_verifiable_spec.rb +++ b/ee/spec/models/concerns/identity_verifiable_spec.rb @@ -20,6 +20,10 @@ def add_identity_verification_exemption create(:user_custom_attribute, key: UserCustomAttribute::IDENTITY_VERIFICATION_EXEMPT, value: true, user: user) end + def assume_high_risk(user) + create(:user_custom_attribute, :assumed_high_risk_reason, user: user) + end + describe('#identity_verification_enabled?') do where( identity_verification: [true, false], @@ -179,6 +183,30 @@ def add_identity_verification_exemption it { is_expected.to eq(result) } end + context 'when user is already active i.e. signed in at least once' do + let(:user) { create(:user, last_sign_in_at: Time.zone.now) } + + where(:phone_exempt, :assumed_high_risk, :affected_by_phone_verifications_limit, :result) do + false | false | false | %w[phone] + true | false | false | %w[credit_card] + false | true | false | %w[credit_card phone] + false | false | true | %w[credit_card] + end + + with_them do + before do + add_phone_exemption if phone_exempt + assume_high_risk(user) if assumed_high_risk + + # Disables phone number verification method + allow(PhoneVerification::Users::RateLimitService) + .to receive(:daily_transaction_hard_limit_exceeded?).and_return(affected_by_phone_verifications_limit) + end + + it { is_expected.to eq(result) } + end + end + context 'when flag is enabled for a specific user' do let_it_be(:another_user) { create(:user) } @@ -338,7 +366,7 @@ def add_identity_verification_exemption with_them do before do - create(:user_custom_attribute, :assumed_high_risk_reason, user: user) + assume_high_risk(user) add_user_risk_band(risk_band) if risk_band add_phone_exemption if phone_exempt @@ -529,8 +557,8 @@ def add_identity_verification_exemption end end - describe '#exempt_from_identity_verification?', :saas do - subject(:exempt_from_identity_verification) { user.exempt_from_identity_verification? } + describe '#signup_identity_verification_exempt?', :saas do + subject(:signup_identity_verification_exempt) { user.signup_identity_verification_exempt? } let(:user) { create(:user) } diff --git a/ee/spec/requests/users/identity_verification_controller_spec.rb b/ee/spec/requests/users/identity_verification_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..115cdceca35223e87b7cfdb572404d22ef1aac0b --- /dev/null +++ b/ee/spec/requests/users/identity_verification_controller_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::IdentityVerificationController, :clean_gitlab_redis_sessions, + :clean_gitlab_redis_rate_limiting, feature_category: :instance_resiliency do + include SessionHelpers + + let_it_be(:user) { create(:user, :low_risk) } + + before do + allow(::Gitlab::ApplicationRateLimiter).to receive(:peek).and_call_original + allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_call_original + + allow(user).to receive(:verification_method_allowed?).and_return(true) + + login_as(user) + end + + shared_examples 'it returns 404 when opt_in_identity_verification feature flag is disabled' do + before do + stub_feature_flags(opt_in_identity_verification: false) + end + + it 'returns 404' do + do_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe 'GET show' do + subject(:do_request) { get identity_verification_path } + + it_behaves_like 'it requires a signed in user' + it_behaves_like 'it returns 404 when opt_in_identity_verification feature flag is disabled' + it_behaves_like 'it loads reCAPTCHA' + + it 'renders show template with minimal layout' do + do_request + + expect(response).to render_template('show', layout: 'minimal') + end + end + + describe 'GET verification_state' do + subject(:do_request) { get verification_state_identity_verification_path } + + it_behaves_like 'it requires a signed in user' + it_behaves_like 'it returns 404 when opt_in_identity_verification feature flag is disabled' + it_behaves_like 'it sets poll interval header' + + it 'returns verification methods and state' do + do_request + + expect(json_response).to eq({ + 'verification_methods' => ["phone"], + 'verification_state' => { "phone" => false } + }) + end + end +end diff --git a/ee/spec/requests/users/registrations_identity_verification_controller_spec.rb b/ee/spec/requests/users/registrations_identity_verification_controller_spec.rb index 5a3dc9f78eb633d1e2a69b997a7756c6b668fd3c..56c635ecf1c7b8f3fd5671cf9a2555eadcec48eb 100644 --- a/ee/spec/requests/users/registrations_identity_verification_controller_spec.rb +++ b/ee/spec/requests/users/registrations_identity_verification_controller_spec.rb @@ -95,22 +95,7 @@ end end - context 'when session is empty but a confirmed user is logged in' do - before do - stub_session(session_data: { verification_user_id: nil }) - sign_in confirmed_user - - do_request - end - - it 'sets the user instance variable' do - expect(assigns(:user)).to eq(confirmed_user) - end - - it 'does not redirect to root path' do - expect(response).not_to redirect_to(root_path) - end - end + it_behaves_like 'it requires a signed in user' end shared_examples 'it requires an unconfirmed user' do |expected_response_code| @@ -298,57 +283,6 @@ end end - shared_examples 'it loads reCAPTCHA' do - before do - stub_feature_flags(arkose_labs_phone_verification_challenge: false) - stub_session(session_data: { verification_user_id: unconfirmed_user.id }) - end - - context 'when reCAPTCHA is disabled' do - before do - allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(false) - end - - it 'does not load recaptcha configuration' do - expect(Gitlab::Recaptcha).not_to receive(:load_configurations!) - - do_request - end - end - - context 'when reCAPTCHA is enabled but daily limit has not been exceeded' do - before do - allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true) - allow(::Gitlab::ApplicationRateLimiter) - .to receive(:peek) - .with(:soft_phone_verification_transactions_limit, scope: nil) - .and_return(false) - end - - it 'does not load reCAPTCHA configuration' do - expect(Gitlab::Recaptcha).not_to receive(:load_configurations!) - - do_request - end - end - - context 'when reCAPTCHA is enabled and daily limit has been exceeded' do - before do - allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true) - allow(::Gitlab::ApplicationRateLimiter) - .to receive(:peek) - .with(:soft_phone_verification_transactions_limit, scope: nil) - .and_return(true) - end - - it 'loads reCAPTCHA configuration' do - expect(Gitlab::Recaptcha).to receive(:load_configurations!) - - do_request - end - end - end - shared_examples 'it verifies reCAPTCHA response' do before do stub_feature_flags(arkose_labs_phone_verification_challenge: false) @@ -445,14 +379,16 @@ describe 'GET show' do subject(:do_request) { get signup_identity_verification_path } + before do + stub_session(session_data: { verification_user_id: unconfirmed_user.id }) + end + it_behaves_like 'it requires a valid verification_user_id' it_behaves_like 'it requires an unconfirmed user' it_behaves_like 'it requires oauth users to go through ArkoseLabs challenge' it_behaves_like 'it loads reCAPTCHA' it 'renders template show with layout minimal' do - stub_session(session_data: { verification_user_id: unconfirmed_user.id }) - do_request expect(response).to render_template('show', layout: 'minimal') @@ -539,13 +475,7 @@ }) end - describe 'poll interval header' do - it 'is added' do - do_request - - expect(response.headers.to_h).to include(Gitlab::PollingInterval::HEADER_NAME => '10000') - end - end + it_behaves_like 'it sets poll interval header' end context 'with a verified user' do diff --git a/ee/spec/support/shared_examples/requests/identity_verification_shared_examples.rb b/ee/spec/support/shared_examples/requests/identity_verification_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..d2482b19193323dab0f9f76fe297bf9fb04328e9 --- /dev/null +++ b/ee/spec/support/shared_examples/requests/identity_verification_shared_examples.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'it requires a signed in user' do + let_it_be(:confirmed_user) { create(:user) } + + before do + stub_session(session_data: { verification_user_id: nil }) + sign_in confirmed_user + + do_request + end + + it 'sets the user instance variable' do + expect(assigns(:user)).to eq(confirmed_user) + end + + it 'does not redirect to root path' do + expect(response).not_to redirect_to(root_path) + end +end + +RSpec.shared_examples 'it loads reCAPTCHA' do + before do + stub_feature_flags(arkose_labs_phone_verification_challenge: false) + end + + context 'when reCAPTCHA is disabled' do + before do + allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(false) + end + + it 'does not load recaptcha configuration' do + expect(Gitlab::Recaptcha).not_to receive(:load_configurations!) + + do_request + end + end + + context 'when reCAPTCHA is enabled but daily limit has not been exceeded' do + before do + allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true) + allow(::Gitlab::ApplicationRateLimiter) + .to receive(:peek) + .with(:soft_phone_verification_transactions_limit, scope: nil) + .and_return(false) + end + + it 'does not load reCAPTCHA configuration' do + expect(Gitlab::Recaptcha).not_to receive(:load_configurations!) + + do_request + end + end + + context 'when reCAPTCHA is enabled and daily limit has been exceeded' do + before do + allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true) + allow(::Gitlab::ApplicationRateLimiter) + .to receive(:peek) + .with(:soft_phone_verification_transactions_limit, scope: nil) + .and_return(true) + end + + it 'loads reCAPTCHA configuration' do + expect(Gitlab::Recaptcha).to receive(:load_configurations!) + + do_request + end + end +end + +# GET verification_state +RSpec.shared_examples 'it sets poll interval header' do + it 'sets poll interval header' do + do_request + + expect(response.headers.to_h).to include(Gitlab::PollingInterval::HEADER_NAME => '10000') + end +end