From ae5eff8c38d04ca77819c45ff52ad05a8c3a8310 Mon Sep 17 00:00:00 2001 From: Jannik Lehmann Date: Thu, 16 Feb 2023 15:49:27 +0100 Subject: [PATCH 01/22] Enhance Security & Compliance Error Handlings This commit solves: https://gitlab.com/gitlab-org/gitlab/-/issues/386854 It enhances the error handling for the Security & Compliance Configuration to make the distinction between user-facing and non-user facing errors. To achieve this behaviour this commit introduces a Backend and a Frontend util to make User facing errors distinguishable by adding and parsing a prefix. Changelog: changed EE: true --- .../sast/components/app.vue | 20 ++++++++----- .../sast/components/app_spec.js | 29 +++++++++++++++++-- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue index 7d6191ff4aa6d3..18cb6840e9ada1 100644 --- a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue +++ b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue @@ -3,6 +3,7 @@ import { GlAlert, GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui'; import { __, s__ } from '~/locale'; import DismissibleFeedbackAlert from '~/vue_shared/components/dismissible_feedback_alert.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import SafeHtml from '~/vue_shared/directives/safe_html'; import ConfigurationPageLayout from '../../components/configuration_page_layout.vue'; import sastCiConfigurationQuery from '../graphql/sast_ci_configuration.query.graphql'; import ConfigurationForm from './configuration_form.vue'; @@ -17,6 +18,7 @@ export default { GlLoadingIcon, GlSprintf, }, + directives: { SafeHtml }, mixins: [glFeatureFlagsMixin()], inject: { sastDocumentationPath: { @@ -39,13 +41,13 @@ export default { update({ project }) { return project?.sastCiConfiguration; }, - result({ loading }) { + result({ loading, error }) { if (!loading && !this.sastCiConfiguration) { - this.onError(); + this.onError(error); } }, - error() { - this.onError(); + error(error) { + this.onError(error); }, }, }, @@ -53,11 +55,13 @@ export default { return { sastCiConfiguration: null, hasLoadingError: false, + specificErrorText: undefined, }; }, methods: { - onError() { + onError(message) { this.hasLoadingError = true; + this.specificErrorText = message; }, }, i18n: { @@ -70,7 +74,7 @@ export default { GitLab and are excluded from updates. For details of more advanced configuration options, see the %{linkStart}GitLab SAST documentation%{linkEnd}.`, ), - loadingErrorText: s__( + genericErrorText: s__( `SecurityConfiguration|Could not retrieve configuration data. Please refresh the page, or try again later.`, ), @@ -111,8 +115,10 @@ export default { variant="danger" :dismissible="false" data-testid="error-alert" - >{{ $options.i18n.loadingErrorText }} + + {{ $options.i18n.genericErrorText }} + diff --git a/ee/spec/frontend/security_configuration/sast/components/app_spec.js b/ee/spec/frontend/security_configuration/sast/components/app_spec.js index aa46dda52284cc..25cc9fc5b0fa45 100644 --- a/ee/spec/frontend/security_configuration/sast/components/app_spec.js +++ b/ee/spec/frontend/security_configuration/sast/components/app_spec.js @@ -2,6 +2,7 @@ import { GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui'; import { merge } from 'lodash'; import Vue from 'vue'; import VueApollo from 'vue-apollo'; +import { __ } from '~/locale'; import ConfigurationPageLayout from 'ee/security_configuration/components/configuration_page_layout.vue'; import SASTConfigurationApp from 'ee/security_configuration/sast/components/app.vue'; import ConfigurationForm from 'ee/security_configuration/sast/components/configuration_form.vue'; @@ -21,7 +22,8 @@ describe('SAST Configuration App', () => { const pendingHandler = () => new Promise(() => {}); const successHandler = async () => sastCiConfigurationQueryResponse; - const failureHandler = async () => ({ errors: [{ message: 'some error' }] }); + const failureHandlerSpecific = async () => ({ errors: [{ message: 'some error' }] }); + const failureHandlerGeneric = async () => ({}); const createMockApolloProvider = (handler) => createMockApollo([[sastCiConfigurationQuery, handler]]); @@ -108,10 +110,10 @@ describe('SAST Configuration App', () => { }); }); - describe('when loading failed', () => { + describe('when loading failed with specific Error Message', () => { beforeEach(() => { createComponent({ - apolloProvider: createMockApolloProvider(failureHandler), + apolloProvider: createMockApolloProvider(failureHandlerSpecific), }); return waitForPromises(); }); @@ -127,6 +129,27 @@ describe('SAST Configuration App', () => { it('displays an alert message', () => { expect(findErrorAlert().exists()).toBe(true); }); + + it('shows specific error message when defined', () => { + expect(findErrorAlert().exists()).toBe(true); + expect(findErrorAlert().text()).toContain('Error: some error'); + }); + }); + + describe('when loading failed with generic Error Message', () => { + beforeEach(() => { + createComponent({ + apolloProvider: createMockApolloProvider(failureHandlerGeneric), + }); + return waitForPromises(); + }); + + it('shows generic error message when no specific message is defined', () => { + expect(findErrorAlert().exists()).toBe(true); + expect(findErrorAlert().text()).toContain( + __(`Could not retrieve configuration data. Please refresh the page, or try again later.`), + ); + }); }); describe('when loaded', () => { -- GitLab From 38acca33b7071128d59b71a8cb86dec4d99b4d44 Mon Sep 17 00:00:00 2001 From: Jannik Lehmann Date: Tue, 21 Feb 2023 14:28:38 +0100 Subject: [PATCH 02/22] Implement distinction between user-facing errors and non-user-facing ones --- .../security_configuration/components/app.vue | 11 ++++- .../components/constants.js | 6 +++ app/graphql/types/project_type.rb | 2 +- .../ci_configuration/base_create_service.rb | 2 +- .../sast/components/app.vue | 47 +++++++++++-------- .../sast/components/app_spec.js | 23 ++++----- .../components/app_spec.js | 15 +++++- 7 files changed, 70 insertions(+), 36 deletions(-) diff --git a/app/assets/javascripts/security_configuration/components/app.vue b/app/assets/javascripts/security_configuration/components/app.vue index e96f71981e5e20..d287bde6d2683d 100644 --- a/app/assets/javascripts/security_configuration/components/app.vue +++ b/app/assets/javascripts/security_configuration/components/app.vue @@ -7,7 +7,7 @@ import SectionLayout from '~/vue_shared/security_configuration/components/sectio import SafeHtml from '~/vue_shared/directives/safe_html'; import AutoDevOpsAlert from './auto_dev_ops_alert.vue'; import AutoDevOpsEnabledAlert from './auto_dev_ops_enabled_alert.vue'; -import { AUTO_DEVOPS_ENABLED_ALERT_DISMISSED_STORAGE_KEY } from './constants'; +import { AUTO_DEVOPS_ENABLED_ALERT_DISMISSED_STORAGE_KEY, USERFACING_KEYWORD } from './constants'; import FeatureCard from './feature_card.vue'; import TrainingProviderList from './training_provider_list.vue'; import UpgradeBanner from './upgrade_banner.vue'; @@ -33,6 +33,9 @@ export const i18n = { 'SecurityConfiguration|Enable security training to help your developers learn how to fix vulnerabilities. Developers can view security training from selected educational providers, relevant to the detected vulnerability.', ), securityTrainingDoc: s__('SecurityConfiguration|Learn more about vulnerability training'), + genericErrorText: s__( + `SecurityConfiguration|Something went wrong. Please refresh the page, or try again later.`, + ), }; export default { @@ -125,7 +128,11 @@ export default { this.autoDevopsEnabledAlertDismissedProjects = Array.from(dismissedProjects); }, onError(message) { - this.errorMessage = message; + if (message.includes(USERFACING_KEYWORD)) { + this.errorMessage = message.replace(USERFACING_KEYWORD, '').trim(); + } else { + this.errorMessage = i18n.genericErrorText; + } }, dismissAlert() { this.errorMessage = ''; diff --git a/app/assets/javascripts/security_configuration/components/constants.js b/app/assets/javascripts/security_configuration/components/constants.js index 6beb6cd4d34883..00052fb1e6c362 100644 --- a/app/assets/javascripts/security_configuration/components/constants.js +++ b/app/assets/javascripts/security_configuration/components/constants.js @@ -21,6 +21,12 @@ import configureSastMutation from '../graphql/configure_sast.mutation.graphql'; import configureSastIacMutation from '../graphql/configure_iac.mutation.graphql'; import configureSecretDetectionMutation from '../graphql/configure_secret_detection.mutation.graphql'; +/** + * Keyword to distinguish userfacing error messages from non-userfacing ones + */ + +export const USERFACING_KEYWORD = 'USERFACING'; + /** * Translations & helpPagePaths for Security Configuration Page * Make sure to add new scanner translations to the SCANNER_NAMES_MAP below. diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index 4593f5e5925c52..2254d388dd8001 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -665,7 +665,7 @@ def sast_ci_configuration if project.repository.empty? raise Gitlab::Graphql::Errors::MutationError, - _(format('You must %s before using Security features.', add_file_docs_link.html_safe)).html_safe + _(format('USERFACING You must %s before using Security features.', add_file_docs_link.html_safe)).html_safe end ::Security::CiConfiguration::SastParserService.new(object).configuration diff --git a/app/services/security/ci_configuration/base_create_service.rb b/app/services/security/ci_configuration/base_create_service.rb index c5fbabf487c493..0a2fdba2bc4e20 100644 --- a/app/services/security/ci_configuration/base_create_service.rb +++ b/app/services/security/ci_configuration/base_create_service.rb @@ -19,7 +19,7 @@ def execute target: '_blank', rel: 'noopener noreferrer' raise Gitlab::Graphql::Errors::MutationError, - _(format('You must %s before using Security features.', docs_link.html_safe)).html_safe + _(format('USERFACING You must %s before using Security features.', docs_link.html_safe)).html_safe end project.repository.add_branch(current_user, branch_name, project.default_branch) diff --git a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue index 18cb6840e9ada1..efc5315c81953a 100644 --- a/ee/app/assets/javascripts/security_configuration/sast/components/app.vue +++ b/ee/app/assets/javascripts/security_configuration/sast/components/app.vue @@ -4,11 +4,29 @@ import { __, s__ } from '~/locale'; import DismissibleFeedbackAlert from '~/vue_shared/components/dismissible_feedback_alert.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import SafeHtml from '~/vue_shared/directives/safe_html'; +import { USERFACING_KEYWORD } from '~/security_configuration/components/constants'; import ConfigurationPageLayout from '../../components/configuration_page_layout.vue'; import sastCiConfigurationQuery from '../graphql/sast_ci_configuration.query.graphql'; import ConfigurationForm from './configuration_form.vue'; +export const i18n = { + feedbackAlertMessage: __(` + As we continue to build more features for SAST, we'd love your feedback + on the SAST configuration feature in %{linkStart}this issue%{linkEnd}.`), + helpText: s__( + `SecurityConfiguration|Customize common SAST settings to suit your + requirements. Configuration changes made here override those provided by + GitLab and are excluded from updates. For details of more advanced + configuration options, see the %{linkStart}GitLab SAST documentation%{linkEnd}.`, + ), + genericErrorText: s__( + `SecurityConfiguration|Could not retrieve configuration data. Please + refresh the page, or try again later.`, + ), +}; + export default { + i18n, components: { ConfigurationForm, ConfigurationPageLayout, @@ -43,11 +61,11 @@ export default { }, result({ loading, error }) { if (!loading && !this.sastCiConfiguration) { - this.onError(error); + this.onError(error.message); } }, error(error) { - this.onError(error); + this.onError(error.message); }, }, }, @@ -61,24 +79,15 @@ export default { methods: { onError(message) { this.hasLoadingError = true; - this.specificErrorText = message; + // stringify errormessage object + if ( + message && + JSON.stringify(message, Object.getOwnPropertyNames(message)).includes(USERFACING_KEYWORD) + ) { + this.specificErrorText = message.replace(USERFACING_KEYWORD, '').trim(); + } }, }, - i18n: { - feedbackAlertMessage: __(` - As we continue to build more features for SAST, we'd love your feedback - on the SAST configuration feature in %{linkStart}this issue%{linkEnd}.`), - helpText: s__( - `SecurityConfiguration|Customize common SAST settings to suit your - requirements. Configuration changes made here override those provided by - GitLab and are excluded from updates. For details of more advanced - configuration options, see the %{linkStart}GitLab SAST documentation%{linkEnd}.`, - ), - genericErrorText: s__( - `SecurityConfiguration|Could not retrieve configuration data. Please - refresh the page, or try again later.`, - ), - }, feedbackIssue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/225991', }; @@ -117,7 +126,7 @@ export default { data-testid="error-alert" > - {{ $options.i18n.genericErrorText }} + {{ $options.i18n.genericErrorText }} diff --git a/ee/spec/frontend/security_configuration/sast/components/app_spec.js b/ee/spec/frontend/security_configuration/sast/components/app_spec.js index 25cc9fc5b0fa45..79bfa80aef546c 100644 --- a/ee/spec/frontend/security_configuration/sast/components/app_spec.js +++ b/ee/spec/frontend/security_configuration/sast/components/app_spec.js @@ -2,9 +2,8 @@ import { GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui'; import { merge } from 'lodash'; import Vue from 'vue'; import VueApollo from 'vue-apollo'; -import { __ } from '~/locale'; import ConfigurationPageLayout from 'ee/security_configuration/components/configuration_page_layout.vue'; -import SASTConfigurationApp from 'ee/security_configuration/sast/components/app.vue'; +import SASTConfigurationApp, { i18n } from 'ee/security_configuration/sast/components/app.vue'; import ConfigurationForm from 'ee/security_configuration/sast/components/configuration_form.vue'; import sastCiConfigurationQuery from 'ee/security_configuration/sast/graphql/sast_ci_configuration.query.graphql'; import createMockApollo from 'helpers/mock_apollo_helper'; @@ -22,8 +21,12 @@ describe('SAST Configuration App', () => { const pendingHandler = () => new Promise(() => {}); const successHandler = async () => sastCiConfigurationQueryResponse; - const failureHandlerSpecific = async () => ({ errors: [{ message: 'some error' }] }); - const failureHandlerGeneric = async () => ({}); + const failureHandlerSpecific = async () => ({ + errors: [{ message: 'USERFACING some specific error' }], + }); + const failureHandlerGeneric = async () => ({ + errors: [{ message: 'some non-userfacing technical error' }], + }); const createMockApolloProvider = (handler) => createMockApollo([[sastCiConfigurationQuery, handler]]); @@ -110,7 +113,7 @@ describe('SAST Configuration App', () => { }); }); - describe('when loading failed with specific Error Message', () => { + describe('when loading failed with Error Message including user facing keyword', () => { beforeEach(() => { createComponent({ apolloProvider: createMockApolloProvider(failureHandlerSpecific), @@ -130,13 +133,13 @@ describe('SAST Configuration App', () => { expect(findErrorAlert().exists()).toBe(true); }); - it('shows specific error message when defined', () => { + it('shows specific error message without keyword when defined', () => { expect(findErrorAlert().exists()).toBe(true); - expect(findErrorAlert().text()).toContain('Error: some error'); + expect(findErrorAlert().text()).toContain('some specific error'); }); }); - describe('when loading failed with generic Error Message', () => { + describe('when loading failed with Error Message without user facing keyword', () => { beforeEach(() => { createComponent({ apolloProvider: createMockApolloProvider(failureHandlerGeneric), @@ -146,9 +149,7 @@ describe('SAST Configuration App', () => { it('shows generic error message when no specific message is defined', () => { expect(findErrorAlert().exists()).toBe(true); - expect(findErrorAlert().text()).toContain( - __(`Could not retrieve configuration data. Please refresh the page, or try again later.`), - ); + expect(findErrorAlert().text()).toContain(i18n.genericErrorText); }); }); diff --git a/spec/frontend/security_configuration/components/app_spec.js b/spec/frontend/security_configuration/components/app_spec.js index 5ef387adf39aa6..3655b0596e297e 100644 --- a/spec/frontend/security_configuration/components/app_spec.js +++ b/spec/frontend/security_configuration/components/app_spec.js @@ -200,10 +200,10 @@ describe('App component', () => { }); }); - describe('when error occurs', () => { + describe('when Userfacing error occurs', () => { it('should show Alert with error Message', async () => { expect(findManageViaMRErrorAlert().exists()).toBe(false); - findFeatureCards().at(1).vm.$emit('error', 'There was a manage via MR error'); + findFeatureCards().at(1).vm.$emit('error', 'USERFACING There was a manage via MR error'); await nextTick(); expect(findManageViaMRErrorAlert().exists()).toBe(true); @@ -221,6 +221,17 @@ describe('App component', () => { expect(findManageViaMRErrorAlert().exists()).toBe(false); }); }); + + describe('when non-userfacing error occurs', () => { + it('should show Alert with generic error Message', async () => { + expect(findManageViaMRErrorAlert().exists()).toBe(false); + findFeatureCards().at(1).vm.$emit('error', 'There was a manage via MR error'); + + await nextTick(); + expect(findManageViaMRErrorAlert().exists()).toBe(true); + expect(findManageViaMRErrorAlert().text()).toEqual(i18n.genericErrorText); + }); + }); }); describe('Auto DevOps hint alert', () => { -- GitLab From 314f525a1031926be7f2c748d0ac81c188588edb Mon Sep 17 00:00:00 2001 From: Jannik Lehmann Date: Tue, 21 Feb 2023 21:33:04 +0100 Subject: [PATCH 03/22] Introduce UserFacing Error utils --- .../javascripts/lib/utils/error_message.js | 36 ++++++++++++ .../security_configuration/components/app.vue | 11 ++-- .../components/constants.js | 6 -- app/graphql/types/project_type.rb | 2 +- .../ci_configuration/base_create_service.rb | 3 +- .../sast/components/app.vue | 18 +++--- .../sast/components/app_spec.js | 3 +- lib/gitlab/utils/error_message.rb | 11 ++++ locale/gitlab.pot | 3 + spec/frontend/lib/utils/error_message_spec.js | 58 +++++++++++++++++++ .../components/app_spec.js | 3 +- spec/lib/gitlab/utils/error_message_spec.rb | 23 ++++++++ 12 files changed, 153 insertions(+), 24 deletions(-) create mode 100644 app/assets/javascripts/lib/utils/error_message.js create mode 100644 lib/gitlab/utils/error_message.rb create mode 100644 spec/frontend/lib/utils/error_message_spec.js create mode 100644 spec/lib/gitlab/utils/error_message_spec.rb diff --git a/app/assets/javascripts/lib/utils/error_message.js b/app/assets/javascripts/lib/utils/error_message.js new file mode 100644 index 00000000000000..83a0206f3f2bf4 --- /dev/null +++ b/app/assets/javascripts/lib/utils/error_message.js @@ -0,0 +1,36 @@ +import _ from 'lodash'; + +const USERFACINGINDICATOR = /^UF:/; + +const getMessageFromError = (error) => { + if (_.isString(error)) { + return error; + } + if (_.isObject(error) && error.message) { + return error.message; + } + + return ''; +}; + +export const parseErrorMessage = (error) => { + if (!error) { + return { + message: '', + userFacing: false, + }; + } + + const messageString = getMessageFromError(error); + + if (messageString.match(USERFACINGINDICATOR)) { + return { + message: messageString.replace(USERFACINGINDICATOR, '').trim(), + userFacing: true, + }; + } + return { + message: messageString, + userFacing: false, + }; +}; diff --git a/app/assets/javascripts/security_configuration/components/app.vue b/app/assets/javascripts/security_configuration/components/app.vue index d287bde6d2683d..9ff577cf7dc095 100644 --- a/app/assets/javascripts/security_configuration/components/app.vue +++ b/app/assets/javascripts/security_configuration/components/app.vue @@ -1,13 +1,14 @@