diff --git a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue index 8bee8fad9bda3bb0d8ef0f0142589a76ee7490bd..0c144e93f9f4776eff9b2f4a4ecab9111cb42cff 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue @@ -42,7 +42,9 @@ export default { return Boolean(this.vulnerability.dismissal_feedback); }, hasIssue() { - return Boolean(this.vulnerability.issue_feedback); + return Boolean( + this.vulnerability.issue_feedback && this.vulnerability.issue_feedback.issue_iid, + ); }, canDismissVulnerability() { const path = this.vulnerability.vulnerability_feedback_dismissal_path; diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js index 0200dc05423e62d78fd460d77e69981380fcaaf8..1ba221a970ff607ddc94580faebdc37c917e3322 100644 --- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js +++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js @@ -93,11 +93,17 @@ export default { Vue.set(state.modal.data.reportType, 'value', vulnerability.report_type); Vue.set(state.modal.data.confidence, 'value', vulnerability.confidence); Vue.set(state.modal, 'vulnerability', vulnerability); - Vue.set(state.modal.vulnerability, 'hasIssue', Boolean(vulnerability.issue_feedback)); + Vue.set( + state.modal.vulnerability, + 'hasIssue', + Boolean(vulnerability.issue_feedback && vulnerability.issue_feedback.issue_iid), + ); Vue.set( state.modal.vulnerability, 'hasMergeRequest', - Boolean(vulnerability.merge_request_feedback), + Boolean( + vulnerability.merge_request_feedback && vulnerability.merge_request.merge_request_iid, + ), ); Vue.set(state.modal.vulnerability, 'isDismissed', Boolean(vulnerability.dismissal_feedback)); Vue.set(state.modal, 'error', null); diff --git a/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js b/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js index a01964b16bea9d203d34f803eb366bea798344d5..1d942fa3fda3c5f70e1d05d90cc085b659d8c8ac 100644 --- a/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js +++ b/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js @@ -53,13 +53,13 @@ function enrichVulnerabilityWithfeedback(vulnerability, feedback = []) { isDismissed: true, dismissalFeedback: fb, }; - } else if (fb.feedback_type === 'issue') { + } else if (fb.feedback_type === 'issue' && fb.issue_iid) { return { ...vuln, hasIssue: true, issue_feedback: fb, }; - } else if (fb.feedback_type === 'merge_request') { + } else if (fb.feedback_type === 'merge_request' && fb.merge_request_iid) { return { ...vuln, hasMergeRequest: true, diff --git a/ee/app/controllers/projects/vulnerability_feedback_controller.rb b/ee/app/controllers/projects/vulnerability_feedback_controller.rb index 3bc09f3e4c634dde1cec558937c4e308221edc10..57064a125f91ac68baf97f33e16e0692c56f7b24 100644 --- a/ee/app/controllers/projects/vulnerability_feedback_controller.rb +++ b/ee/app/controllers/projects/vulnerability_feedback_controller.rb @@ -12,6 +12,7 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle def index # TODO: Move to finder or list service @vulnerability_feedback = @project.vulnerability_feedback.with_associations + if params[:category].present? @vulnerability_feedback = @vulnerability_feedback .where(category: Vulnerabilities::Feedback.categories[params[:category]]) diff --git a/ee/app/models/vulnerabilities/feedback.rb b/ee/app/models/vulnerabilities/feedback.rb index 019f9ed04f87b81459b91cbb7195df754ebd75ef..6bf91d6fe6014382eda1ad87ed250d5f8cff038b 100644 --- a/ee/app/models/vulnerabilities/feedback.rb +++ b/ee/app/models/vulnerabilities/feedback.rb @@ -29,5 +29,26 @@ class Feedback < ActiveRecord::Base scope :all_preloaded, -> do preload(:author, :project, :issue, :merge_request, :pipeline) end + + def self.find_or_init_for(feedback_params) + validate_enums(feedback_params) + + record = find_or_initialize_by(feedback_params.slice(:category, :feedback_type, :project_fingerprint)) + record.assign_attributes(feedback_params) + record + end + + # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by. + # This method, and calls to it can be removed when we are on Rails 5.2. + def self.validate_enums(feedback_params) + unless feedback_types.include?(feedback_params[:feedback_type]) + + raise ArgumentError.new("'#{feedback_params[:feedback_type]}' is not a valid feedback_type") + end + + unless categories.include?(feedback_params[:category]) + raise ArgumentError.new("'#{feedback_params[:category]}' is not a valid category") + end + end end end diff --git a/ee/app/serializers/vulnerabilities/feedback_entity.rb b/ee/app/serializers/vulnerabilities/feedback_entity.rb index 18239da5c287cfbccde7d0c4c130c368e6d83535..131698ba58c08be4ae882d769bb2c07340520226 100644 --- a/ee/app/serializers/vulnerabilities/feedback_entity.rb +++ b/ee/app/serializers/vulnerabilities/feedback_entity.rb @@ -17,19 +17,19 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity end end - expose :issue_iid, if: -> (feedback, _) { feedback.issue? } do |feedback| + expose :issue_iid, if: -> (feedback, _) { feedback.issue.present? } do |feedback| feedback.issue.iid end - expose :issue_url, if: -> (feedback, _) { feedback.issue? } do |feedback| + expose :issue_url, if: -> (feedback, _) { feedback.issue.present? } do |feedback| project_issue_url(feedback.project, feedback.issue) end - expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? } do |feedback| + expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback| feedback.merge_request.iid end - expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? } do |feedback| + expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback| project_merge_request_path(feedback.project, feedback.merge_request) end diff --git a/ee/app/services/vulnerability_feedback_module/create_service.rb b/ee/app/services/vulnerability_feedback_module/create_service.rb index 63667e912bb27ec3b16b465f753d5129aa7b252e..b55dcc3958d1cd10377e89478fd4fb96c7d65be9 100644 --- a/ee/app/services/vulnerability_feedback_module/create_service.rb +++ b/ee/app/services/vulnerability_feedback_module/create_service.rb @@ -3,7 +3,7 @@ module VulnerabilityFeedbackModule class CreateService < ::BaseService def execute - vulnerability_feedback = @project.vulnerability_feedback.new(@params) + vulnerability_feedback = @project.vulnerability_feedback.find_or_init_for(@params) vulnerability_feedback.author = @current_user if vulnerability_feedback.issue? && diff --git a/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml b/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml new file mode 100644 index 0000000000000000000000000000000000000000..ecd8baffd50a739281973e434ab43d4c04d4f1e3 --- /dev/null +++ b/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml @@ -0,0 +1,6 @@ +--- +title: Resolve Deletion of vulnerability-associated issuables prevents security report + from loading +merge_request: 10016 +author: +type: fixed diff --git a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js index 9e83f27f4a74d3744dbd76338fa1bc4224ad1e29..08d97dcbe506582ba36ac6ff61e8580876a21bd6 100644 --- a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js +++ b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js @@ -108,4 +108,55 @@ describe('Security Dashboard Table Row', () => { expect(vm.$el.textContent).toContain('DISMISSED'); }); }); + + describe('with valid issue feedback', () => { + const vulnerability = mockDataVulnerabilities[3]; + + beforeEach(() => { + props = { vulnerability }; + vm = mountComponentWithStore(Component, { store, props }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should have a `ic-issue-created` class', () => { + expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(1); + }); + }); + + describe('with invalid issue feedback', () => { + const vulnerability = mockDataVulnerabilities[6]; + + beforeEach(() => { + props = { vulnerability }; + vm = mountComponentWithStore(Component, { store, props }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should not have a `ic-issue-created` class', () => { + expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0); + }); + }); + + describe('with no issue feedback', () => { + const vulnerability = mockDataVulnerabilities[0]; + + beforeEach(() => { + props = { vulnerability }; + vm = mountComponentWithStore(Component, { store, props }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should not have a `ic-issue-created` class', () => { + expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0); + }); + }); }); diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json index 44841e366f35b43a8ea862bb090eef0a51727b6f..bfdd894d7fc660b24000a39ee4602ed8554631aa 100644 --- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json +++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json @@ -406,5 +406,79 @@ "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first" } ] + }, + { + "id": 7, + "report_type": "sast", + "name": "Insecure variable usage", + "severity": "high", + "confidence": "low", + "scanner": { + "external_id": "find_sec_bugs", + "name": "Find Security Bugs" + }, + "identifiers": [ + { + "external_type": "CVE", + "external_id": "CVE-2018-1234", + "name": "CVE-2018-1234", + "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234" + }, + { + "external_type": "CVE", + "external_id": "CVE-2018-1234", + "name": "CVE-2018-1234", + "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234" + } + ], + "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4", + "project": { + "id": 1, + "name": "project1", + "full_path": "/namespace1/project1", + "full_name": "Gitab.org / security-products / codequality" + }, + "dismissal_feedback": null, + "issue_feedback": { + "id": 7, + "project_id": 1, + "author": { + "id": 8, + "name": "John Doe9", + "username": "user8", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/51798cfc94af924ac2dffb7083baa6f4?s=80&d=identicon", + "web_url": "http://localhost/user8", + "status_tooltip_html": null, + "path": "/user8" + }, + "issue_iid": null, + "pipeline": { + "id": 3, + "path": "/namespace6/project6/pipelines/3" + }, + "issue_url": null, + "category": "sast", + "feedback_type": "issue", + "branch": "master", + "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4" + }, + "vulnerability_feedback_issue_path": "https://example.com/vulnerability_feedback", + "vulnerability_feedback_dismissal_path": "https://example.com/vulnerability_feedback", + "description": "The cipher does not provide data integrity update 1", + "solution": "GCM mode introduces an HMAC into the resulting encrypted data, providing integrity of the result.", + "location": { + "file": "maven/src/main/java/com/gitlab/security_products/tests/App.java", + "start_line": 29, + "end_line": 29, + "class": "com.gitlab.security_products.tests.App", + "method": "insecureCypher" + }, + "links": [ + { + "name": "Cipher does not check for integrity first?", + "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first" + } + ] } -] \ No newline at end of file +] diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js index e5a41f8e76c989ca690b39d2b105ceeca2d5a4cb..e1561b35e065a810da630b2802bdf6d248741e72 100644 --- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js +++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js @@ -314,12 +314,33 @@ describe('vulnerabilities module mutations', () => { }); it('should set hasIssue when the vulnerabilitiy has a related issue', () => { - const payload = { vulnerability: { ...vulnerability, issue_feedback: 'I am an issue' } }; + const payload = { + vulnerability: { + ...vulnerability, + issue_feedback: { + issue_iid: 123, + }, + }, + }; mutations[types.SET_MODAL_DATA](state, payload); expect(state.modal.vulnerability.hasIssue).toEqual(true); }); + it('should not set hasIssue when the issue_iid in null', () => { + const payload = { + vulnerability: { + ...vulnerability, + issue_feedback: { + issue_iid: null, + }, + }, + }; + mutations[types.SET_MODAL_DATA](state, payload); + + expect(state.modal.vulnerability.hasIssue).toEqual(false); + }); + it('should nullify the modal links', () => { const payload = { vulnerability: { ...vulnerability, links: [] } }; mutations[types.SET_MODAL_DATA](state, payload); diff --git a/ee/spec/javascripts/vue_shared/security_reports/mock_data.js b/ee/spec/javascripts/vue_shared/security_reports/mock_data.js index ec29476dc39a49475f0eacbb7cd8018eb96dedac..81645ee158f3107dbca5908d55d616f0054f567f 100644 --- a/ee/spec/javascripts/vue_shared/security_reports/mock_data.js +++ b/ee/spec/javascripts/vue_shared/security_reports/mock_data.js @@ -1196,7 +1196,7 @@ export const sastFeedbacks = [ id: 3, project_id: 17, author_id: 1, - issue_id: null, + issue_iid: null, pipeline_id: 132, category: 'sast', feedback_type: 'dismissal', @@ -1207,7 +1207,7 @@ export const sastFeedbacks = [ id: 4, project_id: 17, author_id: 1, - issue_id: 123, + issue_iid: 123, pipeline_id: 132, category: 'sast', feedback_type: 'issue', @@ -1221,7 +1221,7 @@ export const dependencyScanningFeedbacks = [ id: 3, project_id: 17, author_id: 1, - issue_id: null, + issue_iid: null, pipeline_id: 132, category: 'dependency_scanning', feedback_type: 'dismissal', @@ -1232,7 +1232,7 @@ export const dependencyScanningFeedbacks = [ id: 4, project_id: 17, author_id: 1, - issue_id: 123, + issue_iid: 123, pipeline_id: 132, category: 'dependency_scanning', feedback_type: 'issue', @@ -1246,7 +1246,7 @@ export const dastFeedbacks = [ id: 3, project_id: 17, author_id: 1, - issue_id: null, + issue_iid: null, pipeline_id: 132, category: 'container_scanning', feedback_type: 'dismissal', @@ -1257,7 +1257,7 @@ export const dastFeedbacks = [ id: 4, project_id: 17, author_id: 1, - issue_id: 123, + issue_iid: 123, pipeline_id: 132, category: 'container_scanning', feedback_type: 'issue', @@ -1271,7 +1271,7 @@ export const containerScanningFeedbacks = [ id: 3, project_id: 17, author_id: 1, - issue_id: null, + issue_iid: null, pipeline_id: 132, category: 'container_scanning', feedback_type: 'dismissal', @@ -1282,7 +1282,7 @@ export const containerScanningFeedbacks = [ id: 4, project_id: 17, author_id: 1, - issue_id: 123, + issue_iid: 123, pipeline_id: 132, category: 'container_scanning', feedback_type: 'issue', diff --git a/ee/spec/models/vulnerabilities/feedback_spec.rb b/ee/spec/models/vulnerabilities/feedback_spec.rb index 3214a04f7b41baf99ccf17ecf0bf7b6f3e57a06a..7d6fec4aa619f8de5ee6457acd7581209d01371a 100644 --- a/ee/spec/models/vulnerabilities/feedback_spec.rb +++ b/ee/spec/models/vulnerabilities/feedback_spec.rb @@ -21,4 +21,74 @@ it { is_expected.to validate_presence_of(:category) } it { is_expected.to validate_presence_of(:project_fingerprint) } end + + describe '#find_or_init_for' do + let(:group) { create(:group) } + let(:project) { create(:project, :public, :repository, namespace: group) } + let(:user) { create(:user) } + let(:pipeline) { create(:ci_pipeline, project: project) } + + let(:feedback_params) do + { + feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast', + project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8', + vulnerability_data: { + category: 'sast', + priority: 'Low', line: '41', + file: 'subdir/src/main/java/com/gitlab/security_products/tests/App.java', + cve: '818bf5dacb291e15d9e6dc3c5ac32178:PREDICTABLE_RANDOM', + name: 'Predictable pseudorandom number generator', + description: 'Description of Predictable pseudorandom number generator', + tool: 'find_sec_bugs' + } + } + end + + context 'when params are valid' do + subject(:feedback) { described_class.find_or_init_for(feedback_params) } + + before do + feedback.author = user + feedback.project = project + end + + it 'inits the feedback' do + is_expected.to be_new_record + end + + it 'finds the existing feedback' do + feedback.save! + + existing_feedback = described_class.find_or_init_for(feedback_params) + + expect(existing_feedback).to eq(feedback) + end + + context 'when attempting to save duplicate' do + it 'raises ActiveRecord::RecordInvalid' do + duplicate = described_class.find_or_init_for(feedback_params) + duplicate.author = user + duplicate.project = project + + feedback.save! + + expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end + + context 'when params are invalid' do + it 'raises ArgumentError when given a bad feedback_type value' do + feedback_params[:feedback_type] = 'foo' + + expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /feedback_type/) + end + + it 'raises ArgumentError when given a bad category value' do + feedback_params[:category] = 'foo' + + expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /category/) + end + end + end end diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb index 983745cd009f57fc2a61dbd27cef386748ed75b4..dd2f22e48f47f84799e507d187f691dd7182aa8d 100644 --- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb +++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb @@ -12,4 +12,52 @@ it { is_expected.to include(:project_id, :author, :category, :feedback_type) } end + + context 'when issue is present' do + let(:feedback) { build(:vulnerability_feedback, :issue ) } + let(:entity) { described_class.represent(feedback) } + + subject { entity.as_json } + + it 'exposes issue information' do + is_expected.to include(:issue_iid) + is_expected.to include(:issue_url) + end + end + + context 'when issue is not present' do + let(:feedback) { build(:vulnerability_feedback, feedback_type: 'issue', issue: nil) } + let(:entity) { described_class.represent(feedback) } + + subject { entity.as_json } + + it 'does not expose issue information' do + is_expected.not_to include(:issue_iid) + is_expected.not_to include(:issue_url) + end + end + + context 'when merge request is present' do + let(:feedback) { build(:vulnerability_feedback, :merge_request ) } + let(:entity) { described_class.represent(feedback) } + + subject { entity.as_json } + + it 'exposes merge request information' do + is_expected.to include(:merge_request_iid) + is_expected.to include(:merge_request_path) + end + end + + context 'when merge request is not present' do + let(:feedback) { build(:vulnerability_feedback, feedback_type: 'merge_request', merge_request: nil) } + let(:entity) { described_class.represent(feedback) } + + subject { entity.as_json } + + it 'does not expose merge request information' do + is_expected.not_to include(:merge_request_iid) + is_expected.not_to include(:merge_request_path) + end + end end diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb index 419887154e11a337be3bad3a84ddaba16db6dd66..a3ec58147cc8ce59e9c95f32d5c4904a3371374c 100644 --- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb +++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb @@ -74,6 +74,19 @@ expect(feedback.merge_request).to be_nil end + it 'updates the feedback when it already exists' do + result + + expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count) + end + + it 'creates a new issue when feedback already exists and issue has been deleted' do + result + + expect { result[:vulnerability_feedback].issue.destroy}.to change(Issue, :count).by(-1) + expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1) + end + it 'delegates the Issue creation to CreateFromVulnerabilityDataService' do expect_any_instance_of(Issues::CreateFromVulnerabilityDataService) .to receive(:execute).once.and_call_original @@ -191,5 +204,21 @@ expect(result[:message]).to eq("'foo' is not a valid feedback_type") end end + + context 'when category is invalid' do + let(:feedback_params) do + { + feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'foo', + project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8' + } + end + + let(:result) { described_class.new(project, user, feedback_params).execute } + + it 'returns error with correct message' do + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("'foo' is not a valid category") + end + end end end