diff --git a/ee/app/controllers/ee/registrations_controller.rb b/ee/app/controllers/ee/registrations_controller.rb index 56ec85196235a3459a71c24e55e5568b1d081fc6..9b8d9ec7f4f8bc1f78ddab0fdbfc9e5d390d7f36 100644 --- a/ee/app/controllers/ee/registrations_controller.rb +++ b/ee/app/controllers/ee/registrations_controller.rb @@ -4,6 +4,7 @@ module EE module RegistrationsController extend ActiveSupport::Concern extend ::Gitlab::Utils::Override + include ::Gitlab::Utils::StrongMemoize prepended do include ArkoseLabsCSP @@ -35,6 +36,7 @@ def after_request_hook(user) super log_audit_event(user) + record_arkose_data(user) end override :set_blocked_pending_approval? @@ -63,10 +65,25 @@ def log_audit_event(user) def verify_arkose_labs_token return true unless ::Feature.enabled?(:arkose_labs_signup_challenge) + return false unless params[:arkose_labs_token].present? - # TODO: verify the token with Arkose Labs Verify API - # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96243 - params[:arkose_labs_token].present? + arkose_labs_verify_response.present? + end + + def arkose_labs_verify_response + result = Arkose::TokenVerificationService.new(session_token: params[:arkose_labs_token]).execute + result.success? ? result.payload[:response] : nil + end + strong_memoize_attr :arkose_labs_verify_response + + def record_arkose_data(user) + return unless ::Feature.enabled?(:arkose_labs_signup_challenge) + return unless arkose_labs_verify_response + + Arkose::RecordUserDataService.new( + response: arkose_labs_verify_response, + user: user + ).execute end end end diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb index 2b97df00d03b3b4da76e7a1af19b6d949fbbedd3..e730e714742bf8cfa94560549e2a1c5120e9672b 100644 --- a/ee/app/services/arkose/token_verification_service.rb +++ b/ee/app/services/arkose/token_verification_service.rb @@ -23,7 +23,11 @@ def execute RecordUserDataService.new(response: response, user: user).execute if response.allowlisted? || response.challenge_solved? - ServiceResponse.success(payload: { low_risk: response.allowlisted? || response.low_risk? }) + payload = { + low_risk: response.allowlisted? || response.low_risk?, + response: response + } + ServiceResponse.success(payload: payload) else ServiceResponse.error(message: 'Captcha was not solved') end diff --git a/ee/spec/requests/registrations_controller_spec.rb b/ee/spec/requests/registrations_controller_spec.rb index 498ef566cb8f06f2016353cb281de69b03b2cd44..3d2ba1766cd6ed42573adfec7e9d7582307e6df2 100644 --- a/ee/spec/requests/registrations_controller_spec.rb +++ b/ee/spec/requests/registrations_controller_spec.rb @@ -4,25 +4,27 @@ RSpec.describe RegistrationsController, type: :request do describe 'POST #create' do - let_it_be(:base_user_params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) } + let_it_be(:user_attrs) { build(:user).slice(:first_name, :last_name, :username, :email, :password) } let(:arkose_labs_params) { { arkose_labs_token: 'arkose-labs-token' } } - let(:user_params) { { user: base_user_params }.merge(arkose_labs_params) } + let(:user_params) { { user: user_attrs }.merge(arkose_labs_params) } + + let(:json_response) do + Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json'))) + end subject(:request) { post user_registration_path, params: user_params } - context 'when arkose_labs_token verification succeeds' do - it 'does not render new action', :aggregate_failures do + shared_examples 'creates the user' do + it 'creates the user' do request - expect(flash[:alert]).to be_nil - expect(response).not_to render_template(:new) + created_user = User.find_by(email: user_attrs[:email]) + expect(created_user).not_to be_nil end end - context 'when arkose_labs_token verification fails' do - let(:arkose_labs_params) { {} } - + shared_examples 'renders new action with an alert flash' do it 'renders new action with an alert flash', :aggregate_failures do request @@ -30,5 +32,65 @@ expect(response).to render_template(:new) end end + + context 'when arkose labs session token verification is needed' do + let(:verify_response) { Arkose::VerifyResponse.new(json_response) } + let(:service_response) { ServiceResponse.success(payload: { response: verify_response }) } + + before do + allow_next_instance_of(Arkose::TokenVerificationService) do |instance| + allow(instance).to receive(:execute).and_return(service_response) + end + end + + context 'when arkose_labs_token verification succeeds' do + it_behaves_like 'creates the user' + + it "records the user's data from Arkose Labs" do + expect { request }.to change(UserCustomAttribute, :count).from(0) + end + end + + context 'when verification fails' do + let(:service_response) { ServiceResponse.error(message: 'Captcha was not solved') } + + it_behaves_like 'renders new action with an alert flash' + + it "does not record the user's data from Arkose Labs" do + expect(Arkose::RecordUserDataService).not_to receive(:new) + + request + end + end + end + + context 'when arkose labs session token verification is skipped' do + shared_examples 'skips verification and data recording' do + it 'skips verification and data recording', :aggregate_failures do + expect(Arkose::TokenVerificationService).not_to receive(:new) + expect(Arkose::RecordUserDataService).not_to receive(:new) + + request + end + end + + context 'when :arkose_labs_signup_challenge feature flag is disabled' do + before do + stub_feature_flags(arkose_labs_signup_challenge: false) + end + + it_behaves_like 'creates the user' + + it_behaves_like 'skips verification and data recording' + end + + context 'when arkose_labs_token param is not present' do + let(:arkose_labs_params) { {} } + + it_behaves_like 'renders new action with an alert flash' + + it_behaves_like 'skips verification and data recording' + end + end end end diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb index e650249060291ec633475a6ed522711afdefad9d..a889ca88b7789d7e5adcb1a0370f2719bcf2016e 100644 --- a/ee/spec/services/arkose/token_verification_service_spec.rb +++ b/ee/spec/services/arkose/token_verification_service_spec.rb @@ -44,8 +44,30 @@ end context 'when feature arkose_labs_prevent_login is enabled' do + shared_examples 'returns success response with the correct payload' do + let(:mock_response) { Arkose::VerifyResponse.new(arkose_ec_response) } + + before do + allow(Arkose::VerifyResponse).to receive(:new).with(arkose_ec_response).and_return(mock_response) + end + + it 'returns a success response' do + expect(subject).to be_success + end + + it "returns payload with correct :low_risk value" do + expect(subject.payload[:low_risk]).to eq is_low_risk + end + + it 'includes the json response in the payload' do + expect(subject.payload[:response]).to eq mock_response + end + end + context 'when the user solved the challenge' do context 'when the risk score is low' do + let(:is_low_risk) { true } + let(:arkose_ec_response) do Gitlab::Json.parse( File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')) @@ -58,13 +80,7 @@ expect(WebMock).to have_requested(:post, verify_api_url) end - it 'returns a success response' do - expect(subject).to be_success - end - - it 'returns { low_risk: true } payload' do - expect(subject.payload[:low_risk]).to eq true - end + it_behaves_like 'returns success response with the correct payload' it 'logs Arkose verify response' do allow(Gitlab::AppLogger).to receive(:info) @@ -101,6 +117,8 @@ end context 'when the session is allowlisted' do + let(:is_low_risk) { true } + let(:arkose_ec_response) do json = Gitlab::Json.parse( File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_high_risk.json')) @@ -109,35 +127,27 @@ json end - it 'returns a success response' do - expect(subject).to be_success - end - - it 'returns { low_risk: true } payload' do - expect(subject.payload[:low_risk]).to eq true - end + it_behaves_like 'returns success response with the correct payload' end context 'when the risk score is high' do + let(:is_low_risk) { false } + let(:arkose_ec_response) do Gitlab::Json.parse( File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_high_risk.json')) ) end - it 'returns a success response' do - expect(subject).to be_success - end - - it 'returns { low_risk: false } payload' do - expect(subject.payload[:low_risk]).to eq false - end + it_behaves_like 'returns success response with the correct payload' end end end context 'when the response does not include the risk session' do context 'when the user solved the challenge' do + let(:is_low_risk) { true } + let(:arkose_ec_response) do Gitlab::Json.parse( File.read( @@ -146,13 +156,7 @@ ) end - it 'returns a success response' do - expect(subject).to be_success - end - - it 'returns { low_risk: true } payload' do - expect(subject.payload[:low_risk]).to eq true - end + it_behaves_like 'returns success response with the correct payload' end context 'when the user did not solve the challenge' do @@ -173,11 +177,7 @@ end end - context 'when response from Arkose is not what we expect' do - # For example: https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/54 - - let(:arkose_ec_response) { 'unexpected_from_arkose' } - + shared_examples 'returns success response with correct payload and logs the error' do it 'returns a success response' do expect(subject).to be_success end @@ -186,6 +186,10 @@ expect(subject.payload[:low_risk]).to eq true end + it 'does not include the json response in the payload' do + expect(subject.payload[:response]).to be_nil + end + it 'logs the error' do expect(Gitlab::AppLogger).to receive(:error).with(a_string_matching(/Error verifying user on Arkose: /)) @@ -193,6 +197,14 @@ end end + context 'when response from Arkose is not what we expect' do + # For example: https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/54 + + let(:arkose_ec_response) { 'unexpected_from_arkose' } + + it_behaves_like 'returns success response with correct payload and logs the error' + end + context 'when an error occurs during the Arkose request' do let(:arkose_ec_response) { {} } @@ -200,19 +212,7 @@ allow(Gitlab::HTTP).to receive(:perform_request).and_raise(Errno::ECONNREFUSED.new('bad connection')) end - it 'returns a success response' do - expect(subject).to be_success - end - - it 'returns { low_risk: true } payload' do - expect(subject.payload[:low_risk]).to eq true - end - - it 'logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with(a_string_matching(/Error verifying user on Arkose: /)) - - subject - end + it_behaves_like 'returns success response with correct payload and logs the error' end context 'when user is nil' do @@ -245,7 +245,7 @@ end end - context 'when Arkose is configured using application settings' do + context 'when arkose_labs_prevent_login feature flag is enabled' do before do stub_application_setting(arkose_labs_private_api_key: arkose_labs_private_api_key) stub_application_setting(arkose_labs_namespace: "gitlab") @@ -254,14 +254,6 @@ it_behaves_like 'interacting with Arkose verify API', "https://gitlab-verify.arkoselabs.com/api/v4/verify/" end - context 'when Arkose application settings are not present, fallback to environment variables' do - before do - stub_env('ARKOSE_LABS_PRIVATE_KEY': arkose_labs_private_api_key) - end - - it_behaves_like 'interacting with Arkose verify API', "https://verify-api.arkoselabs.com/api/v4/verify/" - end - context 'when feature arkose_labs_prevent_login is disabled' do before do stub_feature_flags(arkose_labs_prevent_login: false)