Merge branch '329527-prevent-unknown-deployment_types-in-jiratrackerdata' into 'master'

URL-based deployment_types resolutions for JiraTrackerData

See merge request gitlab-org/gitlab!62040

app/models/integrations/jira.rb

@@ -10,12 +10,7 @@ module Integrations
     include Gitlab::Utils::StrongMemoize
 
     PROJECTS_PER_PAGE = 50
-
-    # TODO: use jira_service.deployment_type enum when https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37003 is merged
-    DEPLOYMENT_TYPES = {
-      server: 'SERVER',
-      cloud: 'CLOUD'
-    }.freeze
+    JIRA_CLOUD_HOST = '.atlassian.net'
 
     validates :url, public_url: true, presence: true, if: :activated?
     validates :api_url, public_url: true, allow_blank: true
@@ -517,15 +512,38 @@ module Integrations
     def update_deployment_type
       clear_memoization(:server_info) # ensure we run the request when we try to update deployment type
       results = server_info
-      return data_fields.deployment_unknown! unless results.present?
 
-      case results['deploymentType']
-      when 'Server'
+      unless results.present?
+        Gitlab::AppLogger.warn(message: "Jira API returned no ServerInfo, setting deployment_type from URL", server_info: results, url: client_url)
+
+        return set_deployment_type_from_url
+      end
+
+      if jira_cloud?
+        data_fields.deployment_cloud!
+      else
         data_fields.deployment_server!
-      when 'Cloud'
+      end
+    end
+
+    def jira_cloud?
+      server_info['deploymentType'] == 'Cloud' || URI(client_url).hostname.end_with?(JIRA_CLOUD_HOST)
+    end
+
+    def set_deployment_type_from_url
+      # This shouldn't happen but of course it will happen when an integration is removed.
+      # Instead of deleting the integration we set all fields to null
+      # and mark it as inactive
+      return data_fields.deployment_unknown! unless client_url
+
+      # If API-based detection methods fail here then
+      # we can only assume it's either Cloud or Server
+      # based on the URL being *.atlassian.net
+
+      if URI(client_url).hostname.end_with?(JIRA_CLOUD_HOST)
         data_fields.deployment_cloud!
       else
-        data_fields.deployment_unknown!
+        data_fields.deployment_server!
       end
     end
 

app/services/jira_import/users_importer.rb

@@ -31,21 +31,10 @@ module JiraImport
       @users_mapper_service ||= user_mapper_service_factory
     end
 
-    def deployment_type
-      # TODO: use project.jira_service.deployment_type value when https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37003 is merged
-      @deployment_type ||= client.ServerInfo.all.deploymentType
-    end
-
-    def client
-      @client ||= project.jira_service.client
-    end
-
     def user_mapper_service_factory
-      # TODO: use deployment_type enum from jira service when https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37003 is merged
-      case deployment_type.upcase
-      when Integrations::Jira::DEPLOYMENT_TYPES[:server]
+      if project.jira_service.data_fields.deployment_server?
         ServerUsersMapperService.new(user, project, start_at)
-      when Integrations::Jira::DEPLOYMENT_TYPES[:cloud]
+      elsif project.jira_service.data_fields.deployment_cloud?
         CloudUsersMapperService.new(user, project, start_at)
       else
         raise ArgumentError

spec/models/integrations/jira_spec.rb

@@ -117,7 +117,8 @@ RSpec.describe Integrations::Jira do
     let(:params) do
       {
         project: project,
-        url: url, api_url: api_url,
+        url: url,
+        api_url: api_url,
         username: username, password: password,
         jira_issue_transition_id: transition_id
       }
@@ -141,9 +142,9 @@ RSpec.describe Integrations::Jira do
     end
 
     context 'when loading serverInfo' do
-      let!(:jira_service) { subject }
+      let(:jira_service) { subject }
 
-      context 'Cloud instance' do
+      context 'from a Cloud instance' do
         let(:server_info_results) { { 'deploymentType' => 'Cloud' } }
 
         it 'is detected' do
@@ -151,7 +152,7 @@ RSpec.describe Integrations::Jira do
         end
       end
 
-      context 'Server instance' do
+      context 'from a Server instance' do
         let(:server_info_results) { { 'deploymentType' => 'Server' } }
 
         it 'is detected' do
@@ -159,11 +160,45 @@ RSpec.describe Integrations::Jira do
         end
       end
 
-      context 'Unknown instance' do
+      context 'from an Unknown instance' do
         let(:server_info_results) { { 'deploymentType' => 'FutureCloud' } }
 
-        it 'is detected' do
-          expect(jira_service.jira_tracker_data.deployment_unknown?).to be_truthy
+        context 'and URL ends in .atlassian.net' do
+          let(:api_url) { 'http://example-api.atlassian.net' }
+
+          it 'deployment_type is set to cloud' do
+            expect(jira_service.jira_tracker_data.deployment_cloud?).to be_truthy
+          end
+        end
+
+        context 'and URL is something else' do
+          let(:api_url) { 'http://my-jira-api.someserver.com' }
+
+          it 'deployment_type is set to server' do
+            expect(jira_service.jira_tracker_data.deployment_server?).to be_truthy
+          end
+        end
+      end
+
+      context 'and no ServerInfo response is received' do
+        let(:server_info_results) { {} }
+
+        context 'and URL ends in .atlassian.net' do
+          let(:api_url) { 'http://example-api.atlassian.net' }
+
+          it 'deployment_type is set to cloud' do
+            expect(Gitlab::AppLogger).to receive(:warn).with(message: "Jira API returned no ServerInfo, setting deployment_type from URL", server_info: server_info_results, url: api_url)
+            expect(jira_service.jira_tracker_data.deployment_cloud?).to be_truthy
+          end
+        end
+
+        context 'and URL is something else' do
+          let(:api_url) { 'http://my-jira-api.someserver.com' }
+
+          it 'deployment_type is set to server' do
+            expect(Gitlab::AppLogger).to receive(:warn).with(message: "Jira API returned no ServerInfo, setting deployment_type from URL", server_info: server_info_results, url: api_url)
+            expect(jira_service.jira_tracker_data.deployment_server?).to be_truthy
+          end
         end
       end
     end
@@ -229,17 +264,31 @@ RSpec.describe Integrations::Jira do
         end
 
         context 'when updating the url, api_url, username, or password' do
-          it 'updates deployment type' do
-            service.update!(url: 'http://first.url')
-            service.jira_tracker_data.update!(deployment_type: 'server')
+          context 'when updating the integration' do
+            it 'updates deployment type' do
+              service.update!(url: 'http://first.url')
+              service.jira_tracker_data.update!(deployment_type: 'server')
 
-            expect(service.jira_tracker_data.deployment_server?).to be_truthy
+              expect(service.jira_tracker_data.deployment_server?).to be_truthy
 
-            service.update!(api_url: 'http://another.url')
-            service.jira_tracker_data.reload
+              service.update!(api_url: 'http://another.url')
+              service.jira_tracker_data.reload
 
-            expect(service.jira_tracker_data.deployment_cloud?).to be_truthy
-            expect(WebMock).to have_requested(:get, /serverInfo/).twice
+              expect(service.jira_tracker_data.deployment_cloud?).to be_truthy
+              expect(WebMock).to have_requested(:get, /serverInfo/).twice
+            end
+          end
+
+          context 'when removing the integration' do
+            let(:server_info_results) { {} }
+
+            it 'updates deployment type' do
+              service.update!(url: nil, api_url: nil, active: false)
+
+              service.jira_tracker_data.reload
+
+              expect(service.jira_tracker_data.deployment_unknown?).to be_truthy
+            end
           end
 
           it 'calls serverInfo for url' do

spec/services/jira_import/users_importer_spec.rb

@@ -43,39 +43,37 @@ RSpec.describe JiraImport::UsersImporter do
       end
     end
 
-    RSpec.shared_examples 'maps jira users to gitlab users' do
+    RSpec.shared_examples 'maps Jira users to GitLab users' do |users_mapper_service:|
       context 'when Jira import is configured correctly' do
-        let_it_be(:jira_service) { create(:jira_service, project: project, active: true) }
-        let(:client) { double }
+        let_it_be(:jira_service) { create(:jira_service, project: project, active: true, url: "http://jira.example.net") }
 
-        before do
-          expect(importer).to receive(:client).at_least(1).and_return(client)
-          allow(client).to receive_message_chain(:ServerInfo, :all, :deploymentType).and_return(deployment_type)
-        end
-
-        context 'when jira client raises an error' do
+        context 'when users mapper service raises an error' do
           let(:error) { Timeout::Error.new }
 
           it 'returns an error response' do
-            expect(client).to receive(:get).and_raise(error)
-            expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, project_id: project.id)
+            expect_next_instance_of(users_mapper_service) do |instance|
+              expect(instance).to receive(:execute).and_raise(error)
+            end
 
+            expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, project_id: project.id)
             expect(subject.error?).to be_truthy
             expect(subject.message).to include('There was an error when communicating to Jira')
           end
         end
 
-        context 'when jira client returns result' do
-          context 'when jira client returns an empty array' do
-            let(:jira_users) { [] }
-
+        context 'when users mapper service returns result' do
+          context 'when users mapper service returns an empty array' do
             it 'returns nil payload' do
+              expect_next_instance_of(users_mapper_service) do |instance|
+                expect(instance).to receive(:execute).and_return([])
+              end
+
               expect(subject.success?).to be_truthy
               expect(subject.payload).to be_empty
             end
           end
 
-          context 'when jira client returns an results' do
+          context 'when Jira client returns any users' do
             let_it_be(:project_member) { create(:user, email: 'sample@jira.com') }
             let_it_be(:group_member) { create(:user, name: 'user-name2') }
             let_it_be(:other_user) { create(:user) }
@@ -86,6 +84,10 @@ RSpec.describe JiraImport::UsersImporter do
             end
 
             it 'returns the mapped users' do
+              expect_next_instance_of(users_mapper_service) do |instance|
+                expect(instance).to receive(:execute).and_return(mapped_users)
+              end
+
               expect(subject.success?).to be_truthy
               expect(subject.payload).to eq(mapped_users)
             end
@@ -95,43 +97,23 @@ RSpec.describe JiraImport::UsersImporter do
     end
 
     context 'when Jira instance is of Server deployment type' do
-      let(:deployment_type) { 'Server' }
-      let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" }
-      let(:jira_users) do
-        [
-          { 'key' => 'acc1', 'name' => 'user-name1', 'emailAddress' => 'sample@jira.com' },
-          { 'key' => 'acc2', 'name' => 'user-name2' }
-        ]
-      end
-
       before do
-        allow_next_instance_of(JiraImport::ServerUsersMapperService) do |instance|
-          allow(instance).to receive(:client).and_return(client)
-          allow(client).to receive(:get).with(url).and_return(jira_users)
-        end
+        allow(project).to receive(:jira_service).and_return(jira_service)
+
+        jira_service.data_fields.deployment_server!
       end
 
-      it_behaves_like 'maps jira users to gitlab users'
+      it_behaves_like 'maps Jira users to GitLab users', users_mapper_service: JiraImport::ServerUsersMapperService
     end
 
-    context 'when Jira instance is of Cloud deploymet type' do
-      let(:deployment_type) { 'Cloud' }
-      let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" }
-      let(:jira_users) do
-        [
-          { 'accountId' => 'acc1', 'displayName' => 'user-name1', 'emailAddress' => 'sample@jira.com' },
-          { 'accountId' => 'acc2', 'displayName' => 'user-name2' }
-        ]
-      end
-
+    context 'when Jira instance is of Cloud deployment type' do
       before do
-        allow_next_instance_of(JiraImport::CloudUsersMapperService) do |instance|
-          allow(instance).to receive(:client).and_return(client)
-          allow(client).to receive(:get).with(url).and_return(jira_users)
-        end
+        allow(project).to receive(:jira_service).and_return(jira_service)
+
+        jira_service.data_fields.deployment_cloud!
       end
 
-      it_behaves_like 'maps jira users to gitlab users'
+      it_behaves_like 'maps Jira users to GitLab users', users_mapper_service: JiraImport::CloudUsersMapperService
     end
   end
 end