Merge branch '323304-improve-user-facing-jira-errors' into 'master'

Improve user errors during Jira API requests

See merge request gitlab-org/gitlab!66987

app/services/jira/requests/base.rb

@@ -6,6 +6,17 @@ module Jira
       include ProjectServicesLoggable
 
       JIRA_API_VERSION = 2
+      # Limit the size of the JSON error message we will attempt to parse, as the JSON is external input.
+      JIRA_ERROR_JSON_SIZE_LIMIT = 5_000
+
+      ERRORS = {
+        connection: [Errno::ECONNRESET, Errno::ECONNREFUSED],
+        jira_ruby:  JIRA::HTTPError,
+        ssl:        OpenSSL::SSL::SSLError,
+        timeout:    [Timeout::Error, Errno::ETIMEDOUT],
+        uri:        [URI::InvalidURIError, SocketError]
+      }.freeze
+      ALL_ERRORS = ERRORS.values.flatten.freeze
 
       def initialize(jira_integration, params = {})
         @project = jira_integration&.project
@@ -43,15 +54,66 @@ module Jira
       def request
         response = client.get(url)
         build_service_response(response)
-      rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET, Errno::ECONNREFUSED, URI::InvalidURIError, JIRA::HTTPError, OpenSSL::SSL::SSLError => error
-        error_message = "Jira request error: #{error.message}"
-        log_error("Error sending message", client_url: client.options[:site],
-                  error: {
-                    exception_class: error.class.name,
-                    exception_message: error.message,
-                    exception_backtrace: Gitlab::BacktraceCleaner.clean_backtrace(error.backtrace)
-                  })
-        ServiceResponse.error(message: error_message)
+      rescue *ALL_ERRORS => e
+        log_error('Error sending message',
+          client_url: client.options[:site],
+          error: {
+            exception_class: e.class.name,
+            exception_message: e.message,
+            exception_backtrace: Gitlab::BacktraceCleaner.clean_backtrace(e.backtrace)
+          }
+        )
+
+        ServiceResponse.error(message: error_message(e))
+      end
+
+      def error_message(error)
+        reportable_error_message(error) ||
+          s_('JiraRequest|An error occurred while requesting data from Jira. Check your Jira integration configuration and try again.')
+      end
+
+      # Returns a user-facing error message if possible, otherwise `nil`.
+      def reportable_error_message(error)
+        case error
+        when ERRORS[:jira_ruby]
+          reportable_jira_ruby_error_message(error)
+        when ERRORS[:ssl]
+          s_('JiraRequest|An SSL error occurred while connecting to Jira: %{message}. Try your request again.') % { message: error.message }
+        when *ERRORS[:uri]
+          s_('JiraRequest|The Jira API URL for connecting to Jira is not valid. Check your Jira integration API URL and try again.')
+        when *ERRORS[:timeout]
+          s_('JiraRequest|A timeout error occurred while connecting to Jira. Try your request again.')
+        when *ERRORS[:connection]
+          s_('JiraRequest|A connection error occurred while connecting to Jira. Try your request again.')
+        end
+      end
+
+      # Returns a user-facing error message for a `JIRA::HTTPError` if possible,
+      # otherwise `nil`.
+      def reportable_jira_ruby_error_message(error)
+        case error.message
+        when 'Unauthorized'
+          s_('JiraRequest|The credentials for accessing Jira are not valid. Check your Jira integration credentials and try again.')
+        when 'Forbidden'
+          s_('JiraRequest|The credentials for accessing Jira are not allowed to access the data. Check your Jira integration credentials and try again.')
+        when 'Bad Request'
+          s_('JiraRequest|An error occurred while requesting data from Jira. Check your Jira integration configuration and try again.')
+        when /errorMessages/
+          jira_ruby_json_error_message(error.message)
+        end
+      end
+
+      def jira_ruby_json_error_message(error_message)
+        return if error_message.length > JIRA_ERROR_JSON_SIZE_LIMIT
+
+        begin
+          messages = Gitlab::Json.parse(error_message)['errorMessages']&.to_sentence
+          messages = Rails::Html::FullSanitizer.new.sanitize(messages).presence
+          return unless messages
+
+          s_('JiraRequest|An error occurred while requesting data from Jira: %{messages}. Check your Jira integration configuration and try again.') % { messages: messages }
+        rescue JSON::ParserError
+        end
       end
 
       def url

ee/app/controllers/projects/integrations/jira/issues_controller.rb

@@ -18,8 +18,7 @@ module Projects
           push_frontend_feature_flag(:jira_issue_details_edit_labels, project, default_enabled: :yaml)
         end
 
-        rescue_from ::Projects::Integrations::Jira::IssuesFinder::IntegrationError, with: :render_integration_error
-        rescue_from ::Projects::Integrations::Jira::IssuesFinder::RequestError, with: :render_request_error
+        rescue_from ::Projects::Integrations::Jira::IssuesFinder::Error, with: :render_error
 
         feature_category :integrations
 
@@ -104,19 +103,11 @@ module Projects
           return render_404 unless project.jira_issues_integration_available? && project.jira_integration.issues_enabled
         end
 
-        # Return the informational message to the user
-        def render_integration_error(exception)
+        def render_error(exception)
           log_exception(exception)
 
           render json: { errors: [exception.message] }, status: :bad_request
         end
-
-        # Log the specific request error details and return generic message
-        def render_request_error(exception)
-          log_exception(exception)
-
-          render json: { errors: [_('An error occurred while requesting data from the Jira service.')] }, status: :bad_request
-        end
       end
     end
   end

ee/app/finders/projects/integrations/jira/issues_finder.rb

@@ -4,8 +4,9 @@ module Projects
   module Integrations
     module Jira
       class IssuesFinder
-        IntegrationError = Class.new(StandardError)
-        RequestError = Class.new(StandardError)
+        Error = Class.new(StandardError)
+        IntegrationError = Class.new(Error)
+        RequestError = Class.new(Error)
 
         attr_reader :issues, :total_count, :per_page
 
@@ -46,12 +47,10 @@ module Projects
                        .new(jira_integration, { jql: jql, page: page, per_page: per_page })
                        .execute
 
-          if response.success?
-            @total_count = response.payload[:total_count]
-            @issues = response.payload[:issues]
-          else
-            raise RequestError, response.message
-          end
+          raise RequestError, response.message if response.error?
+
+          @total_count = response.payload[:total_count]
+          @issues = response.payload[:issues]
         end
         # rubocop: enable CodeReuse/ServiceClass
 

ee/spec/controllers/projects/integrations/jira/issues_controller_spec.rb

@@ -97,7 +97,7 @@ RSpec.describe Projects::Integrations::Jira::IssuesController do
         get :index, params: { namespace_id: project.namespace, project_id: project }, format: :json
 
         expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response['errors']).to eq ['An error occurred while requesting data from the Jira service.']
+        expect(json_response['errors']).to eq ['Request error']
       end
 
       it 'sets pagination headers' do

ee/spec/services/jira/requests/issues/list_service_spec.rb

@@ -44,16 +44,7 @@ RSpec.describe Jira::Requests::Issues::ListService do
           stub_request(:get, expected_url_pattern).to_return(status: 200, body: response_body, headers: response_headers)
         end
 
-        context 'when the request to Jira returns an error' do
-          before do
-            expect_next(JIRA::Client).to receive(:get).and_raise(Timeout::Error)
-          end
-
-          it 'returns an error response' do
-            expect(subject.error?).to be_truthy
-            expect(subject.message).to eq('Jira request error: Timeout::Error')
-          end
-        end
+        it_behaves_like 'a service that handles Jira API errors'
 
         context 'when jira runs on a subpath' do
           let(:jira_integration) { create(:jira_integration, url: 'http://jira.example.com/jira') }

locale/gitlab.pot

@@ -3756,9 +3756,6 @@ msgstr ""
 msgid "An error occurred while reordering issues."
 msgstr ""
 
-msgid "An error occurred while requesting data from the Jira service."
-msgstr ""
-
 msgid "An error occurred while retrieving calendar activity"
 msgstr ""
 
@@ -18824,6 +18821,30 @@ msgstr ""
 msgid "JiraConnect|You can now close this window and return to Jira."
 msgstr ""
 
+msgid "JiraRequest|A connection error occurred while connecting to Jira. Try your request again."
+msgstr ""
+
+msgid "JiraRequest|A timeout error occurred while connecting to Jira. Try your request again."
+msgstr ""
+
+msgid "JiraRequest|An SSL error occurred while connecting to Jira: %{message}. Try your request again."
+msgstr ""
+
+msgid "JiraRequest|An error occurred while requesting data from Jira. Check your Jira integration configuration and try again."
+msgstr ""
+
+msgid "JiraRequest|An error occurred while requesting data from Jira: %{messages}. Check your Jira integration configuration and try again."
+msgstr ""
+
+msgid "JiraRequest|The Jira API URL for connecting to Jira is not valid. Check your Jira integration API URL and try again."
+msgstr ""
+
+msgid "JiraRequest|The credentials for accessing Jira are not allowed to access the data. Check your Jira integration credentials and try again."
+msgstr ""
+
+msgid "JiraRequest|The credentials for accessing Jira are not valid. Check your Jira integration credentials and try again."
+msgstr ""
+
 msgid "JiraService| on branch %{branch_link}"
 msgstr ""
 

spec/graphql/resolvers/projects/jira_projects_resolver_spec.rb

@@ -86,11 +86,11 @@ RSpec.describe Resolvers::Projects::JiraProjectsResolver do
         context 'when Jira connection is not valid' do
           before do
             WebMock.stub_request(:get, 'https://jira.example.com/rest/api/2/project')
-              .to_raise(JIRA::HTTPError.new(double(message: 'Some failure.')))
+              .to_raise(JIRA::HTTPError.new(double(message: '{"errorMessages":["Some failure"]}')))
           end
 
           it 'raises failure error' do
-            expect { resolve_jira_projects }.to raise_error('Jira request error: Some failure.')
+            expect { resolve_jira_projects }.to raise_error('An error occurred while requesting data from Jira: Some failure. Check your Jira integration configuration and try again.')
           end
         end
       end

spec/services/jira/requests/projects/list_service_spec.rb

@@ -43,20 +43,7 @@ RSpec.describe Jira::Requests::Projects::ListService do
           stub_request(:get, expected_url_pattern).to_return(status: 200, body: response_body, headers: response_headers)
         end
 
-        context 'when the request to Jira returns an error' do
-          before do
-            expect_next(JIRA::Client).to receive(:get).and_raise(Timeout::Error)
-          end
-
-          it 'returns an error response' do
-            expect(Gitlab::ProjectServiceLogger).to receive(:error).with(
-              hash_including(
-                error: hash_including(:exception_class, :exception_message, :exception_backtrace)))
-              .and_call_original
-            expect(subject.error?).to be_truthy
-            expect(subject.message).to eq('Jira request error: Timeout::Error')
-          end
-        end
+        it_behaves_like 'a service that handles Jira API errors'
 
         context 'when jira runs on a subpath' do
           let(:jira_integration) { create(:jira_integration, url: 'http://jira.example.com/jira') }

spec/support/shared_examples/services/jira/requests/base_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'a service that handles Jira API errors' do
+  include AfterNextHelpers
+  using RSpec::Parameterized::TableSyntax
+
+  where(:exception_class, :exception_message, :expected_message) do
+    Errno::ECONNRESET | ''    | 'A connection error occurred'
+    Errno::ECONNREFUSED | ''  | 'A connection error occurred'
+    Errno::ETIMEDOUT | ''     | 'A timeout error occurred'
+    Timeout::Error | ''       | 'A timeout error occurred'
+    URI::InvalidURIError | '' | 'The Jira API URL'
+    SocketError | ''          | 'The Jira API URL'
+    OpenSSL::SSL::SSLError | 'foo'   | 'An SSL error occurred while connecting to Jira: foo'
+    JIRA::HTTPError | 'Unauthorized' | 'The credentials for accessing Jira are not valid'
+    JIRA::HTTPError | 'Forbidden'    | 'The credentials for accessing Jira are not allowed'
+    JIRA::HTTPError | 'Bad Request'  | 'An error occurred while requesting data from Jira'
+    JIRA::HTTPError | 'Foo'          | 'An error occurred while requesting data from Jira.'
+    JIRA::HTTPError | '{"errorMessages":["foo","bar"]}' | 'An error occurred while requesting data from Jira: foo and bar'
+    JIRA::HTTPError | '{"errorMessages":[""]}'          | 'An error occurred while requesting data from Jira.'
+  end
+
+  with_them do
+    it 'handles the error' do
+      stub_client_and_raise(exception_class, exception_message)
+
+      expect(subject).to be_a(ServiceResponse)
+      expect(subject).to be_error
+      expect(subject.message).to include(expected_message)
+    end
+  end
+
+  context 'when the JSON in JIRA::HTTPError is unsafe' do
+    before do
+      stub_client_and_raise(JIRA::HTTPError, error)
+    end
+
+    context 'when JSON is malformed' do
+      let(:error) { '{"errorMessages":' }
+
+      it 'returns the default error message' do
+        expect(subject.message).to eq('An error occurred while requesting data from Jira. Check your Jira integration configuration and try again.')
+      end
+    end
+
+    context 'when JSON contains tags' do
+      let(:error) { '{"errorMessages":["<script>alert(true)</script>foo"]}' }
+
+      it 'sanitizes it' do
+        expect(subject.message).to eq('An error occurred while requesting data from Jira: foo. Check your Jira integration configuration and try again.')
+      end
+    end
+  end
+
+  it 'allows unknown exception classes to bubble' do
+    stub_client_and_raise(StandardError)
+
+    expect { subject }.to raise_exception(StandardError)
+  end
+
+  it 'logs the error' do
+    stub_client_and_raise(Timeout::Error, 'foo')
+
+    expect(Gitlab::ProjectServiceLogger).to receive(:error).with(
+      hash_including(
+        client_url: be_present,
+        message: 'Error sending message',
+        service_class: described_class.name,
+        error: hash_including(
+          exception_class: Timeout::Error.name,
+          exception_message: 'foo',
+          exception_backtrace: be_present
+        )
+      )
+    )
+    expect(subject).to be_error
+  end
+
+  def stub_client_and_raise(exception_class, message = '')
+    # `JIRA::HTTPError` classes take a response from the JIRA API, rather than a `String`.
+    message = double(body: message) if exception_class == JIRA::HTTPError
+
+    allow_next(JIRA::Client).to receive(:get).and_raise(exception_class, message)
+  end
+end