From 932aad2c4c8163e92a61de2e6736d16fda90ee57 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Mon, 30 May 2022 16:36:49 +0800
Subject: [PATCH 01/10] Split Arkose::UserVerificationService into two services

Split Arkose::UserVerificationService into two services as follows:

1. Arkose::TokenVerificationService - responsible for sending a request
to Arkose Labs Verify API endpoint to perform verification of the
session token passed from the frontend
2. Arkose::RecordUserDataService - responsible for persisting the user's
data from Arkose Labs into the database. Currently, data persisted are
`arkose_session` (id), `arkose_risk_band`, `arkose_global_score`,
`arkose_custom_score`.

This update is required so we can reuse the resulting two services to
verify session token received and persist user data during registration.
TokenVerificationService will executed before a user is created and
RecordUserDataService is executed after the user is created.
---
 ee/app/controllers/ee/sessions_controller.rb  |   4 +-
 .../arkose/record_user_data_service.rb        |  40 +++
 ...rvice.rb => token_verification_service.rb} |  57 ++--
 .../ee/sessions_controller_spec.rb            |  30 ++-
 .../arkose/record_user_data_service_spec.rb   |  54 ++++
 .../arkose/token_verification_service_spec.rb | 245 ++++++++++++++++++
 .../arkose/user_verification_service_spec.rb  | 166 ------------
 7 files changed, 388 insertions(+), 208 deletions(-)
 create mode 100644 ee/app/services/arkose/record_user_data_service.rb
 rename ee/app/services/arkose/{user_verification_service.rb => token_verification_service.rb} (54%)
 create mode 100644 ee/spec/services/arkose/record_user_data_service_spec.rb
 create mode 100644 ee/spec/services/arkose/token_verification_service_spec.rb
 delete mode 100644 ee/spec/services/arkose/user_verification_service_spec.rb

diff --git a/ee/app/controllers/ee/sessions_controller.rb b/ee/app/controllers/ee/sessions_controller.rb
index 49f86271a66041..f33527408e39f0 100644
--- a/ee/app/controllers/ee/sessions_controller.rb
+++ b/ee/app/controllers/ee/sessions_controller.rb
@@ -103,7 +103,9 @@ def check_arkose_captcha
     end
 
     def verify_arkose_token(user)
-      if Arkose::UserVerificationService.new(session_token: params[:arkose_labs_token], user: user).execute
+      result = Arkose::TokenVerificationService.new(session_token: params[:arkose_labs_token], user: user).execute
+
+      if result.success? && result.payload[:low_risk]
         increment_successful_login_captcha_counter
       else
         failed_login_captcha
diff --git a/ee/app/services/arkose/record_user_data_service.rb b/ee/app/services/arkose/record_user_data_service.rb
new file mode 100644
index 00000000000000..260229489e2cce
--- /dev/null
+++ b/ee/app/services/arkose/record_user_data_service.rb
@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module Arkose
+  class RecordUserDataService
+    attr_reader :response, :user
+
+    def initialize(arkose_verify_response:, user:)
+      @response = VerifyResponse.new(arkose_verify_response)
+      @user = user
+    end
+
+    def execute
+      return ServiceResponse.error(message: 'user is required') unless user.present?
+      return ServiceResponse.error(message: 'Invalid Arkose Labs token') if response.invalid_token?
+
+      add_or_update_arkose_attributes
+
+      ServiceResponse.success
+    end
+
+    private
+
+    def add_or_update_arkose_attributes
+      return if Gitlab::Database.read_only?
+
+      UserCustomAttribute.upsert_custom_attributes(custom_attributes)
+    end
+
+    def custom_attributes
+      custom_attributes = []
+      custom_attributes.push({ key: 'arkose_session', value: response.session_id })
+      custom_attributes.push({ key: 'arkose_risk_band', value: response.risk_band })
+      custom_attributes.push({ key: 'arkose_global_score', value: response.global_score })
+      custom_attributes.push({ key: 'arkose_custom_score', value: response.custom_score })
+
+      custom_attributes.map! { |custom_attribute| custom_attribute.merge({ user_id: user.id }) }
+      custom_attributes
+    end
+  end
+end
diff --git a/ee/app/services/arkose/user_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
similarity index 54%
rename from ee/app/services/arkose/user_verification_service.rb
rename to ee/app/services/arkose/token_verification_service.rb
index fbb777131c60f9..fe965157947666 100644
--- a/ee/app/services/arkose/user_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -1,78 +1,65 @@
 # frozen_string_literal: true
+
 module Arkose
-  class UserVerificationService
-    attr_reader :url, :session_token, :user, :response
+  class TokenVerificationService
+    attr_reader :session_token, :user
 
     ARKOSE_LABS_DEFAULT_NAMESPACE = 'client'
     ARKOSE_LABS_DEFAULT_SUBDOMAIN = 'verify-api'
 
-    def initialize(session_token:, user:)
+    def initialize(session_token:, user: nil)
       @session_token = session_token
       @user = user
     end
 
     def execute
       json_response = Gitlab::HTTP.perform_request(Net::HTTP::Post, arkose_verify_url, body: body).parsed_response
-      @response = VerifyResponse.new(json_response)
 
-      logger.info(build_message)
+      response = VerifyResponse.new(json_response)
+
+      logger.info(build_message(response))
 
-      return false if response.invalid_token?
+      return ServiceResponse.error(message: response.error) if response.invalid_token?
 
-      add_or_update_arkose_attributes
+      RecordUserDataService.new(arkose_verify_response: json_response, user: user).execute
 
-      response.allowlisted? || (response.challenge_solved? && response.low_risk?)
+      if response.allowlisted? || response.challenge_solved?
+        ServiceResponse.success(payload: { low_risk: response.allowlisted? || response.low_risk? })
+      else
+        ServiceResponse.error(message: 'Captcha was not solved')
+      end
     rescue StandardError => error
-      payload = { session_token: session_token, log_data: user.id }
+      payload = { session_token: session_token, log_data: user&.id }.compact
       Gitlab::ExceptionLogFormatter.format!(error, payload)
       Gitlab::ErrorTracking.track_exception(error)
-      logger.error("Error verifying user on Arkose: #{payload}")
 
-      true
+      ServiceResponse.success(payload: payload)
     end
 
     private
 
-    def add_or_update_arkose_attributes
-      return if Gitlab::Database.read_only?
-
-      UserCustomAttribute.upsert_custom_attributes(custom_attributes)
-    end
-
-    def custom_attributes
-      custom_attributes = []
-      custom_attributes.push({ key: 'arkose_session', value: response.session_id })
-      custom_attributes.push({ key: 'arkose_risk_band', value: response.risk_band })
-      custom_attributes.push({ key: 'arkose_global_score', value: response.global_score })
-      custom_attributes.push({ key: 'arkose_custom_score', value: response.custom_score })
-
-      custom_attributes.map! { |custom_attribute| custom_attribute.merge({ user_id: user.id }) }
-      custom_attributes
-    end
-
     def body
       {
         private_key: Settings.arkose_private_api_key,
         session_token: session_token,
-        log_data: user.id
-      }
+        log_data: user&.id
+      }.compact
     end
 
     def logger
       Gitlab::AppLogger
     end
 
-    def build_message
+    def build_message(response)
       Gitlab::ApplicationContext.current.symbolize_keys.merge(
         {
           message: 'Arkose verify response',
           response: response.response,
-          username: user.username
-        }.merge(arkose_payload)
-      )
+          username: user&.username
+        }.compact).merge(arkose_payload(response))
     end
 
-    def arkose_payload
+    def arkose_payload(response)
       {
         'arkose.session_id': response.session_id,
         'arkose.global_score': response.global_score,
diff --git a/ee/spec/controllers/ee/sessions_controller_spec.rb b/ee/spec/controllers/ee/sessions_controller_spec.rb
index 24c08377a8e480..6e06f9988ed878 100644
--- a/ee/spec/controllers/ee/sessions_controller_spec.rb
+++ b/ee/spec/controllers/ee/sessions_controller_spec.rb
@@ -175,20 +175,38 @@ def authenticate_2fa(otp_user_id: user.id, **user_params)
       end
 
       context 'when the user was verified by Arkose' do
-        it 'successfully logs in a user when reCAPTCHA is solved' do
-          allow_next_instance_of(Arkose::UserVerificationService) do |instance|
-            allow(instance).to receive(:execute).and_return(true)
+        before do
+          allow_next_instance_of(Arkose::TokenVerificationService) do |instance|
+            response = ServiceResponse.success(payload: { low_risk: low_risk })
+            allow(instance).to receive(:execute).and_return(response)
           end
+
           post(:create, params: params, session: {})
+        end
 
-          expect(subject.current_user).to eq user
+        context 'when user is low risk' do
+          let(:low_risk) { true }
+
+          it 'successfully logs in the user' do
+            expect(subject.current_user).to eq user
+          end
+        end
+
+        context 'when user is NOT low risk' do
+          let(:low_risk) { false }
+
+          it 'prevents the user from logging in' do
+            expect(response).to render_template(:new)
+            expect(flash[:alert]).to include 'Login failed. Please retry from your primary device and network'
+            expect(subject.current_user).to be_nil
+          end
         end
       end
 
       context 'when the user was not verified by Arkose' do
         before do
-          allow_next_instance_of(Arkose::UserVerificationService) do |instance|
-            allow(instance).to receive(:execute).and_return(false)
+          allow_next_instance_of(Arkose::TokenVerificationService) do |instance|
+            allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'error'))
           end
         end
 
diff --git a/ee/spec/services/arkose/record_user_data_service_spec.rb b/ee/spec/services/arkose/record_user_data_service_spec.rb
new file mode 100644
index 00000000000000..1ab25f06137136
--- /dev/null
+++ b/ee/spec/services/arkose/record_user_data_service_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Arkose::RecordUserDataService do
+  let(:user) { create(:user) }
+
+  let(:arkose_verify_response) do
+    Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
+  end
+
+  let(:service) { described_class.new(arkose_verify_response: arkose_verify_response, user: user) }
+
+  describe '#execute' do
+    it 'adds new custom attributes to the user' do
+      expect { service.execute }.to change { user.custom_attributes.count }.from(0).to(4)
+    end
+
+    it 'adds arkose data to custom attributes' do
+      service.execute
+
+      expect(user.custom_attributes.find_by(key: 'arkose_session').value).to eq('22612c147bb418c8.2570749403')
+      expect(user.custom_attributes.find_by(key: 'arkose_risk_band').value).to eq('Low')
+      expect(user.custom_attributes.find_by(key: 'arkose_global_score').value).to eq('0')
+      expect(user.custom_attributes.find_by(key: 'arkose_custom_score').value).to eq('0')
+    end
+
+    it 'returns a success response' do
+      expect(service.execute).to be_success
+    end
+
+    context 'when response is from failed verification' do
+      let(:arkose_verify_response) do
+        Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/invalid_token.json')))
+      end
+
+      it 'does not add any custom attributes' do
+        expect { service.execute }.not_to change { user.custom_attributes.count }
+      end
+
+      it 'returns an error response' do
+        expect(service.execute).to be_error
+      end
+    end
+
+    context 'when user is nil' do
+      let(:user) { nil }
+
+      it 'returns an error response' do
+        expect(service.execute).to be_error
+      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
new file mode 100644
index 00000000000000..b5c2056a40b14c
--- /dev/null
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -0,0 +1,245 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Arkose::TokenVerificationService do
+  let(:user) { create(:user) }
+  let(:session_token) { '22612c147bb418c8.2570749403' }
+  let(:service) { described_class.new(session_token: session_token, user: user) }
+  let(:verify_api_url) { "https://verify-api.arkoselabs.com/api/v4/verify/" }
+  let(:arkose_labs_private_api_key) { 'foo' }
+
+  subject { service.execute }
+
+  before do
+    stub_request(:post, verify_api_url)
+      .with(
+        body: /.*/,
+        headers: {
+          'Accept' => '*/*'
+        }
+      ).to_return(
+        status: 200,
+        body: arkose_ec_response.to_json,
+        headers: { 'Content-Type' => 'application/json' }
+      )
+  end
+
+  describe '#execute' do
+    shared_examples_for 'interacting with Arkose verify API' do |url|
+      let(:verify_api_url) { url }
+
+      context 'when the user did not solve the challenge' do
+        let(:arkose_ec_response) do
+          Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/failed_ec_response.json')))
+        end
+
+        it 'returns an error response' do
+          expect(subject).to be_error
+        end
+      end
+
+      describe 'user data recording' do
+        let(:arkose_ec_response) do
+          Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
+        end
+
+        it 'executes Arkose::RecordUserDataService with the response and user' do
+          init_args = { arkose_verify_response: arkose_ec_response, user: user }
+          expect_next_instance_of(Arkose::RecordUserDataService, init_args) do |service|
+            expect(service).to receive(:execute)
+          end
+
+          subject
+        end
+      end
+
+      context 'when feature arkose_labs_prevent_login is enabled' do
+        context 'when the user solved the challenge' do
+          context 'when the risk score is low' do
+            let(:arkose_ec_response) do
+              Gitlab::Json.parse(
+                File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json'))
+              )
+            end
+
+            it 'makes a request to the Verify API' do
+              subject
+
+              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 'logs Arkose verify response' do
+              allow(Gitlab::AppLogger).to receive(:info)
+              allow(Gitlab::ApplicationContext).to receive(:current).and_return(
+                { 'correlation_id': 'be025cf83013ac4f52ffd2bf712b11a2' }
+              )
+
+              subject
+
+              expect(Gitlab::AppLogger).to have_received(:info).with(
+                correlation_id: 'be025cf83013ac4f52ffd2bf712b11a2',
+                message: 'Arkose verify response',
+                response: arkose_ec_response,
+                username: user.username,
+                'arkose.session_id': '22612c147bb418c8.2570749403',
+                'arkose.global_score': '0',
+                'arkose.global_telltale_list': [],
+                'arkose.custom_score': '0',
+                'arkose.custom_telltale_list': [],
+                'arkose.risk_band': 'Low',
+                'arkose.risk_category': 'NO-THREAT'
+              )
+            end
+
+            context 'when the session is allowlisted' do
+              let(:arkose_ec_response) do
+                json = Gitlab::Json.parse(
+                  File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json'))
+                )
+                json['session_details']['telltale_list'].push(Arkose::VerifyResponse::ALLOWLIST_TELLTALE)
+                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
+            end
+
+            context 'when the risk score is high' do
+              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
+            end
+          end
+        end
+
+        context 'when the response does not include the risk session' do
+          context 'when the user solved the challenge' do
+            let(:arkose_ec_response) do
+              Gitlab::Json.parse(
+                File.read(
+                  Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_without_session_risk.json')
+                )
+              )
+            end
+
+            it 'returns a success response' do
+              expect(subject).to be_success
+            end
+          end
+
+          context 'when the user did not solve the challenge' do
+            let(:arkose_ec_response) do
+              Gitlab::Json.parse(
+                File.read(Rails.root.join('ee/spec/fixtures/arkose/failed_ec_response_without_risk_session.json'))
+              )
+            end
+
+            it 'returns an error response' do
+              expect(subject).to be_error
+            end
+          end
+        end
+      end
+
+      context 'when an error occurs during the Arkose request' do
+        let(:arkose_ec_response) { {} }
+
+        before do
+          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
+      end
+
+      context 'when user is nil' do
+        let(:user) { nil }
+        let(:arkose_ec_response) do
+          Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
+        end
+
+        it 'logs Arkose verify response without username' do
+          allow(Gitlab::AppLogger).to receive(:info)
+          allow(Gitlab::ApplicationContext).to receive(:current).and_return(
+            { 'correlation_id': 'be025cf83013ac4f52ffd2bf712b11a2' }
+          )
+
+          subject
+
+          expect(Gitlab::AppLogger).to have_received(:info).with(
+            correlation_id: 'be025cf83013ac4f52ffd2bf712b11a2',
+            message: 'Arkose verify response',
+            response: arkose_ec_response,
+            'arkose.session_id': '22612c147bb418c8.2570749403',
+            'arkose.global_score': '0',
+            'arkose.global_telltale_list': [],
+            'arkose.custom_score': '0',
+            'arkose.custom_telltale_list': [],
+            'arkose.risk_band': 'Low',
+            'arkose.risk_category': 'NO-THREAT'
+          )
+        end
+      end
+    end
+
+    context 'when Arkose is configured using application settings' do
+      before do
+        stub_application_setting(arkose_labs_private_api_key: arkose_labs_private_api_key)
+        stub_application_setting(arkose_labs_namespace: "gitlab")
+      end
+
+      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)
+      end
+
+      context 'when the risk score is high' do
+        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 repsonse' do
+          expect(subject).to be_success
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/services/arkose/user_verification_service_spec.rb b/ee/spec/services/arkose/user_verification_service_spec.rb
deleted file mode 100644
index fbd4d9475e0ca4..00000000000000
--- a/ee/spec/services/arkose/user_verification_service_spec.rb
+++ /dev/null
@@ -1,166 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Arkose::UserVerificationService do
-  let(:session_token) { '22612c147bb418c8.2570749403' }
-  let_it_be_with_reload(:user) { create(:user, id: '1999') }
-
-  let(:service) { Arkose::UserVerificationService.new(session_token: session_token, user: user) }
-  let(:verify_api_url) { "https://verify-api.arkoselabs.com/api/v4/verify/" }
-  let(:response_status_code) { 200 }
-  let(:arkose_labs_private_api_key) { 'foo' }
-
-  subject { service.execute }
-
-  before do
-    stub_request(:post, verify_api_url)
-      .with(
-        body: /.*/,
-        headers: {
-          'Accept' => '*/*'
-        }
-      ).to_return(status: response_status_code, body: arkose_ec_response.to_json, headers: {
-        'Content-Type' => 'application/json'
-      })
-  end
-
-  describe '#execute' do
-    shared_examples_for 'interacting with Arkose verify API' do |url|
-      let(:verify_api_url) { url }
-
-      context 'when the user did not solve the challenge' do
-        let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/failed_ec_response.json'))) }
-
-        it 'returns false' do
-          expect(subject).to be_falsey
-        end
-      end
-
-      context 'when feature arkose_labs_prevent_login is enabled' do
-        context 'when the user solved the challenge' do
-          context 'when the risk score is not high' do
-            let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json'))) }
-
-            it 'makes a request to the Verify API' do
-              subject
-
-              expect(WebMock).to have_requested(:post, verify_api_url)
-            end
-
-            it 'returns true' do
-              expect(subject).to be_truthy
-            end
-
-            it 'adds arkose data to custom attributes' do
-              subject
-              expect(user.custom_attributes.count).to eq(4)
-
-              expect(user.custom_attributes.find_by(key: 'arkose_session').value).to eq('22612c147bb418c8.2570749403')
-              expect(user.custom_attributes.find_by(key: 'arkose_risk_band').value).to eq('Low')
-              expect(user.custom_attributes.find_by(key: 'arkose_global_score').value).to eq('0')
-              expect(user.custom_attributes.find_by(key: 'arkose_custom_score').value).to eq('0')
-            end
-
-            it 'logs Arkose verify response' do
-              allow(Gitlab::AppLogger).to receive(:info)
-              allow(Gitlab::ApplicationContext).to receive(:current).and_return({ 'correlation_id': 'be025cf83013ac4f52ffd2bf712b11a2' })
-
-              subject
-
-              expect(Gitlab::AppLogger).to have_received(:info).with(correlation_id: 'be025cf83013ac4f52ffd2bf712b11a2',
-                                                                     message: 'Arkose verify response',
-                                                                     response: arkose_ec_response,
-                                                                     username: user.username,
-                                                                     'arkose.session_id': '22612c147bb418c8.2570749403',
-                                                                     'arkose.global_score': '0',
-                                                                     'arkose.global_telltale_list': [],
-                                                                     'arkose.custom_score': '0',
-                                                                     'arkose.custom_telltale_list': [],
-                                                                     'arkose.risk_band': 'Low',
-                                                                     'arkose.risk_category': 'NO-THREAT')
-            end
-
-            context 'when the risk score is high' do
-              let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_high_risk.json'))) }
-
-              it 'returns false' do
-                expect(subject).to be_falsey
-              end
-
-              context 'when the session is allowlisted' do
-                let(:arkose_ec_response) do
-                  json = Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
-                  json['session_details']['telltale_list'].push(Arkose::VerifyResponse::ALLOWLIST_TELLTALE)
-                  json
-                end
-
-                it 'returns true' do
-                  expect(subject).to be_truthy
-                end
-              end
-            end
-          end
-        end
-
-        context 'when the response does not include the risk session' do
-          context 'when the user solved the challenge' do
-            let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_without_session_risk.json'))) }
-
-            it 'returns true' do
-              expect(subject).to be_truthy
-            end
-          end
-
-          context 'when the user did not solve the challenge' do
-            let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/failed_ec_response_without_risk_session.json'))) }
-
-            it 'returns false' do
-              expect(subject).to be_falsey
-            end
-          end
-        end
-      end
-
-      context 'when an error occurs during the Arkose request' do
-        let(:arkose_ec_response) { {} }
-        let(:response_status_code) { 500 }
-
-        it 'returns true' do
-          expect(subject).to be_truthy
-        end
-      end
-    end
-
-    context 'when Arkose is configured using application settings' do
-      before do
-        stub_application_setting(arkose_labs_private_api_key: arkose_labs_private_api_key)
-        stub_application_setting(arkose_labs_namespace: "gitlab")
-      end
-
-      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)
-      end
-
-      context 'when the risk score is high' do
-        let(:arkose_ec_response) { Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_high_risk.json'))) }
-
-        it 'returns true' do
-          expect(subject).to be_truthy
-        end
-      end
-    end
-  end
-end
-- 
GitLab


From fcc0e812c8fabcbe9a19b152bcf8ca26a043741f Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Fri, 26 Aug 2022 13:27:48 +0800
Subject: [PATCH 02/10] Use json response with high risk score to avoid false
 positive test

---
 ee/spec/services/arkose/token_verification_service_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index b5c2056a40b14c..753bb325a9ad37 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -103,7 +103,7 @@
             context 'when the session is allowlisted' do
               let(:arkose_ec_response) do
                 json = Gitlab::Json.parse(
-                  File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json'))
+                  File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response_high_risk.json'))
                 )
                 json['session_details']['telltale_list'].push(Arkose::VerifyResponse::ALLOWLIST_TELLTALE)
                 json
-- 
GitLab


From 22085cacafc719e90b6f2dbf64be30ba590c87a0 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 14:53:54 +0800
Subject: [PATCH 03/10] Add spec for case when response from Arkose is
 unexpected

---
 .../services/arkose/token_verification_service_spec.rb | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index 753bb325a9ad37..d42481c25deec0 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -165,6 +165,16 @@
         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 'returns a success response' do
+          expect(subject).to be_success
+        end
+      end
+
       context 'when an error occurs during the Arkose request' do
         let(:arkose_ec_response) { {} }
 
-- 
GitLab


From 26ba4ff34a05cf865c172171b99adb0c06d6c3d5 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 15:10:35 +0800
Subject: [PATCH 04/10] Restore accidentally removed error logging

---
 ee/app/services/arkose/token_verification_service.rb |  1 +
 .../arkose/token_verification_service_spec.rb        | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
index fe965157947666..fffa94f444cb77 100644
--- a/ee/app/services/arkose/token_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -32,6 +32,7 @@ def execute
       payload = { session_token: session_token, log_data: user&.id }.compact
       Gitlab::ExceptionLogFormatter.format!(error, payload)
       Gitlab::ErrorTracking.track_exception(error)
+      logger.error("Error verifying user on Arkose: #{payload}")
 
       ServiceResponse.success(payload: payload)
     end
diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index d42481c25deec0..dc42fd43636d9c 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -173,6 +173,12 @@
         it 'returns a success response' do
           expect(subject).to be_success
         end
+
+        it 'logs the error' do
+          expect(Gitlab::AppLogger).to receive(:error).with(a_string_matching(/Error verifying user on Arkose: /))
+
+          subject
+        end
       end
 
       context 'when an error occurs during the Arkose request' do
@@ -185,6 +191,12 @@
         it 'returns a success response' do
           expect(subject).to be_success
         end
+
+        it 'logs the error' do
+          expect(Gitlab::AppLogger).to receive(:error).with(a_string_matching(/Error verifying user on Arkose: /))
+
+          subject
+        end
       end
 
       context 'when user is nil' do
-- 
GitLab


From b1ccb737dcaf5eec5737aa8d236905d9c9055ed1 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 15:18:23 +0800
Subject: [PATCH 05/10] Re-arrange specs

---
 .../arkose/token_verification_service_spec.rb | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index dc42fd43636d9c..fef12e51160ff7 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -39,21 +39,6 @@
         end
       end
 
-      describe 'user data recording' do
-        let(:arkose_ec_response) do
-          Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
-        end
-
-        it 'executes Arkose::RecordUserDataService with the response and user' do
-          init_args = { arkose_verify_response: arkose_ec_response, user: user }
-          expect_next_instance_of(Arkose::RecordUserDataService, init_args) do |service|
-            expect(service).to receive(:execute)
-          end
-
-          subject
-        end
-      end
-
       context 'when feature arkose_labs_prevent_login is enabled' do
         context 'when the user solved the challenge' do
           context 'when the risk score is low' do
@@ -100,6 +85,15 @@
               )
             end
 
+            it "records user's Arkose data" do
+              init_args = { arkose_verify_response: arkose_ec_response, user: user }
+              expect_next_instance_of(Arkose::RecordUserDataService, init_args) do |service|
+                expect(service).to receive(:execute)
+              end
+
+              subject
+            end
+
             context 'when the session is allowlisted' do
               let(:arkose_ec_response) do
                 json = Gitlab::Json.parse(
-- 
GitLab


From e7a82c8b334f4a38170c343c6ae3745f1f1dc50e Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 15:40:18 +0800
Subject: [PATCH 06/10] Ensure returned payload are correct

---
 .../arkose/token_verification_service.rb      |  8 +++++-
 .../arkose/token_verification_service_spec.rb | 26 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
index fffa94f444cb77..d4a01f146b66e1 100644
--- a/ee/app/services/arkose/token_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -29,7 +29,13 @@ def execute
         ServiceResponse.error(message: 'Captcha was not solved')
       end
     rescue StandardError => error
-      payload = { session_token: session_token, log_data: user&.id }.compact
+      payload = {
+        # Allow user to proceed when we can't verify the token for some
+        # unexpected reason (e.g. ArkoseLabs is down)
+        low_risk: true,
+        session_token: session_token,
+        log_data: user&.id
+      }.compact
       Gitlab::ExceptionLogFormatter.format!(error, payload)
       Gitlab::ErrorTracking.track_exception(error)
       logger.error("Error verifying user on Arkose: #{payload}")
diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index fef12e51160ff7..0ebabdbde0e638 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -37,6 +37,10 @@
         it 'returns an error response' do
           expect(subject).to be_error
         end
+
+        it 'returns an error message' do
+          expect(subject.message).to eq 'Captcha was not solved'
+        end
       end
 
       context 'when feature arkose_labs_prevent_login is enabled' do
@@ -143,6 +147,10 @@
             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
           end
 
           context 'when the user did not solve the challenge' do
@@ -155,6 +163,10 @@
             it 'returns an error response' do
               expect(subject).to be_error
             end
+
+            it 'returns an error message' do
+              expect(subject.message).to eq 'Captcha was not solved'
+            end
           end
         end
       end
@@ -168,6 +180,10 @@
           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: /))
 
@@ -186,6 +202,10 @@
           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: /))
 
@@ -252,9 +272,13 @@
           )
         end
 
-        it 'returns a success repsonse' do
+        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
       end
     end
   end
-- 
GitLab


From ba1563f898522638971dce5c77186eeef426fbe5 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Wed, 31 Aug 2022 15:40:39 +0800
Subject: [PATCH 07/10] Use error message that Arkose::TokenVerificationService
 returns

---
 ee/spec/controllers/ee/sessions_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/spec/controllers/ee/sessions_controller_spec.rb b/ee/spec/controllers/ee/sessions_controller_spec.rb
index 6e06f9988ed878..abf0c7826b40de 100644
--- a/ee/spec/controllers/ee/sessions_controller_spec.rb
+++ b/ee/spec/controllers/ee/sessions_controller_spec.rb
@@ -206,7 +206,7 @@ def authenticate_2fa(otp_user_id: user.id, **user_params)
       context 'when the user was not verified by Arkose' do
         before do
           allow_next_instance_of(Arkose::TokenVerificationService) do |instance|
-            allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'error'))
+            allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'Captcha was not solved'))
           end
         end
 
-- 
GitLab


From f26ea14abb825c3967cea90050066a48d8d23e53 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 1 Sep 2022 11:04:35 +0800
Subject: [PATCH 08/10] Update Arkose::RecordUserDataService to accept
 Arkose::VerifyResponse

---
 ee/app/services/arkose/record_user_data_service.rb         | 4 ++--
 ee/app/services/arkose/token_verification_service.rb       | 2 +-
 ee/spec/services/arkose/record_user_data_service_spec.rb   | 3 ++-
 ee/spec/services/arkose/token_verification_service_spec.rb | 6 ++++--
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/ee/app/services/arkose/record_user_data_service.rb b/ee/app/services/arkose/record_user_data_service.rb
index 260229489e2cce..5861927bcbc95e 100644
--- a/ee/app/services/arkose/record_user_data_service.rb
+++ b/ee/app/services/arkose/record_user_data_service.rb
@@ -4,8 +4,8 @@ module Arkose
   class RecordUserDataService
     attr_reader :response, :user
 
-    def initialize(arkose_verify_response:, user:)
-      @response = VerifyResponse.new(arkose_verify_response)
+    def initialize(response:, user:)
+      @response = response
       @user = user
     end
 
diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
index d4a01f146b66e1..91327b46b2a8d0 100644
--- a/ee/app/services/arkose/token_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -21,7 +21,7 @@ def execute
 
       return ServiceResponse.error(message: response.error) if response.invalid_token?
 
-      RecordUserDataService.new(arkose_verify_response: json_response, user: user).execute
+      RecordUserDataService.new(response: response, user: user).execute
 
       if response.allowlisted? || response.challenge_solved?
         ServiceResponse.success(payload: { low_risk: response.allowlisted? || response.low_risk? })
diff --git a/ee/spec/services/arkose/record_user_data_service_spec.rb b/ee/spec/services/arkose/record_user_data_service_spec.rb
index 1ab25f06137136..3153fc69517399 100644
--- a/ee/spec/services/arkose/record_user_data_service_spec.rb
+++ b/ee/spec/services/arkose/record_user_data_service_spec.rb
@@ -9,7 +9,8 @@
     Gitlab::Json.parse(File.read(Rails.root.join('ee/spec/fixtures/arkose/successfully_solved_ec_response.json')))
   end
 
-  let(:service) { described_class.new(arkose_verify_response: arkose_verify_response, user: user) }
+  let(:response) { Arkose::VerifyResponse.new(arkose_verify_response) }
+  let(:service) { described_class.new(response: response, user: user) }
 
   describe '#execute' do
     it 'adds new custom attributes to the user' do
diff --git a/ee/spec/services/arkose/token_verification_service_spec.rb b/ee/spec/services/arkose/token_verification_service_spec.rb
index 0ebabdbde0e638..e650249060291e 100644
--- a/ee/spec/services/arkose/token_verification_service_spec.rb
+++ b/ee/spec/services/arkose/token_verification_service_spec.rb
@@ -90,8 +90,10 @@
             end
 
             it "records user's Arkose data" do
-              init_args = { arkose_verify_response: arkose_ec_response, user: user }
-              expect_next_instance_of(Arkose::RecordUserDataService, init_args) do |service|
+              mock_response = Arkose::VerifyResponse.new(arkose_ec_response)
+              expect(Arkose::VerifyResponse).to receive(:new).with(arkose_ec_response).and_return(mock_response)
+
+              expect_next_instance_of(Arkose::RecordUserDataService, response: mock_response, user: user) do |service|
                 expect(service).to receive(:execute)
               end
 
-- 
GitLab


From 83fbae663b7bb8928432d132d77483b4f15b4aa1 Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 1 Sep 2022 11:07:24 +0800
Subject: [PATCH 09/10] Use ::Arkose::VerifyResponse to avoid confusion with
 ServiceResponse

---
 ee/app/services/arkose/token_verification_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
index 91327b46b2a8d0..ae0e033f495ff2 100644
--- a/ee/app/services/arkose/token_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -15,7 +15,7 @@ def initialize(session_token:, user: nil)
     def execute
       json_response = Gitlab::HTTP.perform_request(Net::HTTP::Post, arkose_verify_url, body: body).parsed_response
 
-      response = VerifyResponse.new(json_response)
+      response = ::Arkose::VerifyResponse.new(json_response)
 
       logger.info(build_message(response))
 
-- 
GitLab


From 46055c44d6567133ab45b29765f33f0c883d84fa Mon Sep 17 00:00:00 2001
From: eugielimpin <elimpin@gitlab.com>
Date: Thu, 1 Sep 2022 14:20:10 +0800
Subject: [PATCH 10/10] Ensure that the parsed response is a Hash or raise an
 error

---
 .../arkose/token_verification_service.rb        |  5 ++---
 ee/lib/arkose/verify_response.rb                |  6 ++++++
 ee/spec/lib/arkose/verify_response_spec.rb      | 17 +++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/ee/app/services/arkose/token_verification_service.rb b/ee/app/services/arkose/token_verification_service.rb
index ae0e033f495ff2..2b97df00d03b3b 100644
--- a/ee/app/services/arkose/token_verification_service.rb
+++ b/ee/app/services/arkose/token_verification_service.rb
@@ -13,9 +13,8 @@ def initialize(session_token:, user: nil)
     end
 
     def execute
-      json_response = Gitlab::HTTP.perform_request(Net::HTTP::Post, arkose_verify_url, body: body).parsed_response
-
-      response = ::Arkose::VerifyResponse.new(json_response)
+      response = Gitlab::HTTP.perform_request(Net::HTTP::Post, arkose_verify_url, body: body).parsed_response
+      response = ::Arkose::VerifyResponse.new(response)
 
       logger.info(build_message(response))
 
diff --git a/ee/lib/arkose/verify_response.rb b/ee/lib/arkose/verify_response.rb
index 60e7fc05d9ac67..f5c4748b157249 100644
--- a/ee/lib/arkose/verify_response.rb
+++ b/ee/lib/arkose/verify_response.rb
@@ -4,9 +4,15 @@ module Arkose
   class VerifyResponse
     attr_reader :response
 
+    InvalidResponseFormatError = Class.new(StandardError)
+
     ALLOWLIST_TELLTALE = 'gitlab1-whitelist-qa-team'
 
     def initialize(response)
+      unless response.is_a? Hash
+        raise InvalidResponseFormatError, "Arkose Labs Verify API returned a #{response.class} instead of of an object"
+      end
+
       @response = response
     end
 
diff --git a/ee/spec/lib/arkose/verify_response_spec.rb b/ee/spec/lib/arkose/verify_response_spec.rb
index 42814c422f22a3..1aa1cdb315190d 100644
--- a/ee/spec/lib/arkose/verify_response_spec.rb
+++ b/ee/spec/lib/arkose/verify_response_spec.rb
@@ -27,6 +27,23 @@ def parse_json(file_path)
     parse_json('ee/spec/fixtures/arkose/allowlisted_response.json')
   end
 
+  describe '.new' do
+    context 'when response is not a Hash' do
+      it 'raises an InvalidResponseFormatError error' do
+        expect { described_class.new('a_string') }.to raise_error(
+          described_class::InvalidResponseFormatError,
+          "Arkose Labs Verify API returned a String instead of of an object"
+        )
+      end
+    end
+
+    context 'when response is a Hash' do
+      it 'does not raise an InvalidResponseFormatError error' do
+        expect { described_class.new({}) }.not_to raise_error
+      end
+    end
+  end
+
   describe '#invalid_token?' do
     subject { described_class.new(json_response).invalid_token? }
 
-- 
GitLab