From dbc64b861c3e7c29197ca5d55fc94f62d1928b08 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 18:36:46 +0800
Subject: [PATCH 1/4] Verify Arkose Labs session token and record user data
 during sign up

Verify the ARkose Labs session token passed from the frontend before
creating a new user. If verification succeeds, the new user is created
and the Arkose Labs data for that user is persisted. Otherwise, the sign
up is prevented from proceeding.
---
 .../ee/registrations_controller.rb            |  19 +++-
 .../arkose/token_verification_service.rb      |   6 +-
 .../requests/registrations_controller_spec.rb |  79 ++++++++++++--
 .../arkose/token_verification_service_spec.rb | 102 ++++++++----------
 4 files changed, 138 insertions(+), 68 deletions(-)

diff --git a/ee/app/controllers/ee/registrations_controller.rb b/ee/app/controllers/ee/registrations_controller.rb
index 56ec85196235a3..d27f7d69af288a 100644
--- a/ee/app/controllers/ee/registrations_controller.rb
+++ b/ee/app/controllers/ee/registrations_controller.rb
@@ -35,6 +35,7 @@ def after_request_hook(user)
       super
 
       log_audit_event(user)
+      record_arkose_data(user)
     end
 
     override :set_blocked_pending_approval?
@@ -63,10 +64,22 @@ 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?
+      result = Arkose::TokenVerificationService.new(session_token: params[:arkose_labs_token]).execute
+      @arkose_verify_response = result.payload[:response] # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
+      result.success?
+    end
+
+    def record_arkose_data(user)
+      return unless ::Feature.enabled?(:arkose_labs_signup_challenge)
+      return unless @arkose_verify_response # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
+      Arkose::RecordUserDataService.new(
+        response: @arkose_verify_response, # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        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 2b97df00d03b3b..e730e714742bf8 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 498ef566cb8f06..484e54c80986df 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,64 @@
         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)
+        end
+      end
+    end
+
+    context 'when arkose labs session token verification is skipped' do
+      context 'when :arkose_labs_signup_challenge feature flag is disabled' do
+        before do
+          stub_feature_flags(arkose_labs_signup_challenge: false)
+
+          request
+        end
+
+        it_behaves_like 'creates the user'
+
+        it 'skips verification' do
+          expect(Arkose::TokenVerificationService).not_to receive(:new)
+        end
+
+        it "does not record the user's data from Arkose Labs" do
+          expect(Arkose::RecordUserDataService).not_to receive(:new)
+        end
+      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 "does not record the user's data from Arkose Labs" do
+          expect(Arkose::RecordUserDataService).not_to receive(:new)
+        end
+      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 e650249060291e..a889ca88b7789d 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)
-- 
GitLab


From 9c1b5a233c167be4e913f44372a8b3235a761a11 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 7 Sep 2022 11:46:41 +0800
Subject: [PATCH 2/4] Fix false positive specs

---
 ee/spec/requests/registrations_controller_spec.rb | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ee/spec/requests/registrations_controller_spec.rb b/ee/spec/requests/registrations_controller_spec.rb
index 484e54c80986df..74e59240b0a1e6 100644
--- a/ee/spec/requests/registrations_controller_spec.rb
+++ b/ee/spec/requests/registrations_controller_spec.rb
@@ -58,6 +58,8 @@
 
         it "does not record the user's data from Arkose Labs" do
           expect(Arkose::RecordUserDataService).not_to receive(:new)
+
+          request
         end
       end
     end
@@ -66,18 +68,20 @@
       context 'when :arkose_labs_signup_challenge feature flag is disabled' do
         before do
           stub_feature_flags(arkose_labs_signup_challenge: false)
-
-          request
         end
 
         it_behaves_like 'creates the user'
 
         it 'skips verification' do
           expect(Arkose::TokenVerificationService).not_to receive(:new)
+
+          request
         end
 
         it "does not record the user's data from Arkose Labs" do
           expect(Arkose::RecordUserDataService).not_to receive(:new)
+
+          request
         end
       end
 
@@ -88,6 +92,8 @@
 
         it "does not record the user's data from Arkose Labs" do
           expect(Arkose::RecordUserDataService).not_to receive(:new)
+
+          request
         end
       end
     end
-- 
GitLab


From b49a0b1df9f787fdaa83f45e7fe57b6131f9e522 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 7 Sep 2022 11:49:18 +0800
Subject: [PATCH 3/4] Improve specs for case when verification is skipped

---
 .../requests/registrations_controller_spec.rb | 27 ++++++++-----------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/ee/spec/requests/registrations_controller_spec.rb b/ee/spec/requests/registrations_controller_spec.rb
index 74e59240b0a1e6..3d2ba1766cd6ed 100644
--- a/ee/spec/requests/registrations_controller_spec.rb
+++ b/ee/spec/requests/registrations_controller_spec.rb
@@ -65,6 +65,15 @@
     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)
@@ -72,17 +81,7 @@
 
         it_behaves_like 'creates the user'
 
-        it 'skips verification' do
-          expect(Arkose::TokenVerificationService).not_to receive(:new)
-
-          request
-        end
-
-        it "does not record the user's data from Arkose Labs" do
-          expect(Arkose::RecordUserDataService).not_to receive(:new)
-
-          request
-        end
+        it_behaves_like 'skips verification and data recording'
       end
 
       context 'when arkose_labs_token param is not present' do
@@ -90,11 +89,7 @@
 
         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
+        it_behaves_like 'skips verification and data recording'
       end
     end
   end
-- 
GitLab


From 39a7051b5412d6743b0a46d79eaa8aba74ac1205 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 8 Sep 2022 11:14:03 +0800
Subject: [PATCH 4/4] Use strong memoize instead of an instance variable

---
 ee/app/controllers/ee/registrations_controller.rb | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ee/app/controllers/ee/registrations_controller.rb b/ee/app/controllers/ee/registrations_controller.rb
index d27f7d69af288a..9b8d9ec7f4f8bc 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
@@ -66,18 +67,21 @@ def verify_arkose_labs_token
       return true unless ::Feature.enabled?(:arkose_labs_signup_challenge)
       return false unless params[:arkose_labs_token].present?
 
-      result = Arkose::TokenVerificationService.new(session_token: params[:arkose_labs_token]).execute
-      @arkose_verify_response = result.payload[:response] # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      arkose_labs_verify_response.present?
+    end
 
-      result.success?
+    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_verify_response # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      return unless arkose_labs_verify_response
 
       Arkose::RecordUserDataService.new(
-        response: @arkose_verify_response, # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        response: arkose_labs_verify_response,
         user: user
       ).execute
     end
-- 
GitLab