Skip to content
Snippets Groups Projects
Commit b68fc67b authored by Lucas Charles's avatar Lucas Charles :speech_balloon:
Browse files

Create Merge Request From Vulnerability Solution

Fixes https://gitlab.com/gitlab-org/gitlab-ee/issues/9224

 - Add `MergeRequests::CreateFromVulnerabilityDataService`
 - Add new Vulnerabilities::Feedback#feedback_type of 'merge_request'
parent fa0068e4
No related branches found
No related tags found
1 merge request!9326Create Merge Request From Remediated Vulnerability Solution
Showing
with 383 additions and 25 deletions
......@@ -3181,8 +3181,10 @@
t.integer "pipeline_id"
t.integer "issue_id"
t.string "project_fingerprint", limit: 40, null: false
t.integer "merge_request_id"
t.index ["author_id"], name: "index_vulnerability_feedback_on_author_id", using: :btree
t.index ["issue_id"], name: "index_vulnerability_feedback_on_issue_id", using: :btree
t.index ["merge_request_id"], name: "index_vulnerability_feedback_on_merge_request_id", using: :btree
t.index ["pipeline_id"], name: "index_vulnerability_feedback_on_pipeline_id", using: :btree
t.index ["project_id", "category", "feedback_type", "project_fingerprint"], name: "vulnerability_feedback_unique_idx", unique: true, using: :btree
end
......@@ -3600,6 +3602,7 @@
add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade
add_foreign_key "vulnerability_feedback", "ci_pipelines", column: "pipeline_id", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "issues", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "merge_requests", on_delete: :cascade
add_foreign_key "vulnerability_feedback", "projects", on_delete: :cascade
add_foreign_key "vulnerability_feedback", "users", column: "author_id", on_delete: :cascade
add_foreign_key "vulnerability_identifiers", "projects", on_delete: :cascade
......
......@@ -129,6 +129,9 @@ def vulnerability_data_params_attributes
links: %i[
name
url
],
remediation: %i[
diff
]
]
end
......
......@@ -7,25 +7,27 @@ class Feedback < ActiveRecord::Base
belongs_to :project
belongs_to :author, class_name: "User"
belongs_to :issue
belongs_to :merge_request
belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
attr_accessor :vulnerability_data
enum feedback_type: { dismissal: 0, issue: 1 }
enum feedback_type: { dismissal: 0, issue: 1, merge_request: 2 }
enum category: { sast: 0, dependency_scanning: 1, container_scanning: 2, dast: 3 }
validates :project, presence: true
validates :author, presence: true
validates :issue, presence: true, if: :issue?
validates :vulnerability_data, presence: true, if: :issue?
validates :merge_request, presence: true, if: :merge_request?
validates :vulnerability_data, presence: true, unless: :dismissal?
validates :feedback_type, presence: true
validates :category, presence: true
validates :project_fingerprint, presence: true, uniqueness: { scope: [:project_id, :category, :feedback_type] }
scope :with_associations, -> { includes(:pipeline, :issue, :author) }
scope :with_associations, -> { includes(:pipeline, :issue, :merge_request, :author) }
scope :all_preloaded, -> do
preload(:author, :project, :issue, :pipeline)
preload(:author, :project, :issue, :merge_request, :pipeline)
end
end
end
......@@ -138,6 +138,10 @@ def issue_feedback
feedback(feedback_type: 'issue')
end
def merge_request_feedback
feedback(feedback_type: 'merge_request')
end
def metadata
strong_memoize(:metadata) do
begin
......
......@@ -18,13 +18,21 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
end
expose :issue_iid, if: -> (feedback, _) { feedback.issue? } do |feedback|
feedback.issue&.iid
feedback.issue.iid
end
expose :issue_url, if: -> (feedback, _) { feedback.issue? } do |feedback|
project_issue_url(feedback.project, feedback.issue)
end
expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
feedback.merge_request.iid
end
expose :merge_request_url, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
project_merge_request_url(feedback.project, feedback.merge_request)
end
expose :category
expose :feedback_type
expose :branch do |feedback|
......
......@@ -8,10 +8,12 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity
expose :identifiers, using: Vulnerabilities::IdentifierEntity
expose :project_fingerprint
expose :vulnerability_feedback_path, as: :vulnerability_feedback_issue_path, if: ->(_, _) { can_admin_vulnerability_feedback? && can_create_issue? }
expose :vulnerability_feedback_path, as: :vulnerability_feedback_merge_request_path, if: ->(_, _) { can_admin_vulnerability_feedback? && can_create_merge_request? }
expose :vulnerability_feedback_path, as: :vulnerability_feedback_dismissal_path, if: ->(_, _) { can_admin_vulnerability_feedback? }
expose :project, using: ::ProjectEntity
expose :dismissal_feedback, using: Vulnerabilities::FeedbackEntity
expose :issue_feedback, using: Vulnerabilities::FeedbackEntity
expose :merge_request_feedback, using: Vulnerabilities::FeedbackEntity
expose :metadata, merge: true, if: ->(occurrence, _) { occurrence.raw_metadata } do
expose :description
......@@ -35,4 +37,8 @@ def can_admin_vulnerability_feedback?
def can_create_issue?
can?(request.current_user, :create_issue, occurrence.project)
end
def can_create_merge_request?
can?(request.current_user, :create_merge_request_in, occurrence.project)
end
end
# frozen_string_literal: true
module MergeRequests
class CreateFromVulnerabilityDataService < ::BaseService
def execute
vulnerability = create_vulnerability
return error('Invalid vulnerability category') unless vulnerability
title_slug = Gitlab::Utils.slugify(vulnerability.title)
source_branch = "remediate/%s-%s" % [
title_slug[0..74],
Time.now.strftime("D%Y%m%dT%H%M%S")
]
target_branch = @project.default_branch
return error("Can't create merge_request") unless can_create_merge_request?(source_branch)
return error("Can't create merge request") unless vulnerability.remediation
patch_result = create_patch(vulnerability, source_branch, target_branch)
return error('Unable to apply patch') unless patch_result[:status] == :success
merge_request_params = {
title: "Resolve vulnerability: #{vulnerability.title}",
description: render_description(vulnerability),
source_branch: source_branch,
target_branch: target_branch,
force_remove_source_branch: "1"
}
merge_request = MergeRequests::CreateService.new(@project, @current_user, merge_request_params).execute
if merge_request.valid?
success(merge_request)
else
error(merge_request.errors)
end
end
private
def create_vulnerability
case @params[:category]
when 'sast', 'dependency_scanning', 'dast'
Gitlab::Vulnerabilities::StandardVulnerability.new(@params)
when 'container_scanning'
Gitlab::Vulnerabilities::ContainerScanningVulnerability.new(@params)
end
end
def create_patch(vulnerability, source_branch, target_branch)
diff = Base64.decode64(vulnerability.data.dig(:remediation, :diff))
head_commit = project.repository.find_branch(target_branch).dereferenced_target
new_commit = render_commit(diff, head_commit, vulnerability)
commit_patch_params = {
branch_name: source_branch,
patches: [
new_commit
]
}
Commits::CommitPatchService.new(@project, @current_user, commit_patch_params).execute
end
def success(merge_request)
super(merge_request: merge_request)
end
def render_commit(diff, head_commit, vulnerability)
git_user = Gitlab::Git::User.from_gitlab(@current_user)
render_template(
file: 'vulnerabilities/remediation.patch.erb',
locals: {
diff: diff,
head_commit: head_commit,
user: git_user,
vulnerability: vulnerability
}
)
end
def render_description(vulnerability)
render_template(
file: 'vulnerabilities/merge_request_description.md.erb',
locals: { vulnerability: vulnerability }
)
end
def render_template(file:, locals:)
view = ActionView::Base.new(ActionController::Base.view_paths, {})
view.render(file: file, locals: locals)
end
def can_create_merge_request?(source_branch)
can?(@current_user, :create_merge_request_in, @project) &&
can?(@current_user, :create_merge_request_from, @project) &&
::Gitlab::UserAccess.new(@current_user, project: @project).can_push_to_branch?(source_branch)
end
end
end
......@@ -6,31 +6,23 @@ def execute
vulnerability_feedback = @project.vulnerability_feedback.new(@params)
vulnerability_feedback.author = @current_user
# Wrap Feedback and Issue creation in the same transaction
ActiveRecord::Base.transaction do
if vulnerability_feedback.issue? && # (feedback_type == 'issue')
!vulnerability_feedback.vulnerability_data.blank?
if vulnerability_feedback.issue? &&
!vulnerability_feedback.vulnerability_data.blank?
result = Issues::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
create_issue_for(vulnerability_feedback)
elsif vulnerability_feedback.merge_request? &&
!vulnerability_feedback.vulnerability_data.blank?
if result[:status] == :error
vulnerability_feedback.errors[:issue] << result[:message]
raise ActiveRecord::Rollback
end
issue = result[:issue]
vulnerability_feedback.issue = issue
end
# Ensure created issue is rolled back if feedback can't be saved
raise ActiveRecord::Rollback unless vulnerability_feedback.save
create_merge_request_for(vulnerability_feedback)
else
vulnerability_feedback.save
end
if vulnerability_feedback.persisted?
success(vulnerability_feedback)
else
rollback_merge_request(vulnerability_feedback.merge_request) if vulnerability_feedback.merge_request
error(vulnerability_feedback.errors)
end
......@@ -44,5 +36,49 @@ def execute
def success(vulnerability_feedback)
super().merge(vulnerability_feedback: vulnerability_feedback)
end
def create_issue_for(vulnerability_feedback)
# Wrap Feedback and Issue creation in the same transaction
ActiveRecord::Base.transaction do
result = Issues::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
if result[:status] == :error
vulnerability_feedback.errors[:issue] << result[:message]
raise ActiveRecord::Rollback
end
issue = result[:issue]
vulnerability_feedback.issue = issue
# Ensure created association is rolled back if feedback can't be saved
raise ActiveRecord::Rollback unless vulnerability_feedback.save
end
end
def create_merge_request_for(vulnerability_feedback)
result = MergeRequests::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
if result[:status] == :success
merge_request = result[:merge_request]
vulnerability_feedback.merge_request = merge_request
vulnerability_feedback.save
else
vulnerability_feedback.errors[:merge_request] << result[:message]
end
end
# Gitaly RPCs cannot occur within a transaction so we must manually
# rollback MR and branch creation
def rollback_merge_request(merge_request)
branch_name = merge_request.source_branch
merge_request&.destroy &&
DeleteBranchService.new(project, current_user).execute(branch_name)
end
end
end
### Description:
<%= vulnerability.description %>
<% if vulnerability.severity.present? %>
* Severity: <%= vulnerability.severity %>
<% end %>
<% if vulnerability.confidence.present? %>
* Confidence: <%= vulnerability.confidence %>
<% end %>
<% if defined?(vulnerability.file) && vulnerability.file.present? %>
* Location: [<%= vulnerability.location_text %>](<%= vulnerability.location_link %>)
<% end %>
<% if vulnerability.solution.present? %>
### Solution:
<%= vulnerability.solution %>
<% end %>
<% if vulnerability.identifiers.present? %>
### Identifiers:
<% vulnerability.identifiers.each do |identifier| %>
<% if identifier[:url].present? %>
* [<%= identifier[:name] %>](<%= identifier[:url] %>)
<% else %>
* <%= identifier[:name] %>
<% end %>
<% end %>
<% end %>
<% if vulnerability.links.present? %>
### Links:
<% vulnerability.links.each do |link| %>
<% if link[:name].present? %>
* [<%= link[:name] %>](<%= link[:url] %>)
<% else %>
* <%= link[:url] %>
<% end %>
<% end %>
<% end %>
From <%= head_commit.sha %> <%= head_commit.date.strftime("%a %b %e %H:%M:%S %Y") %>
From: <%= user.name %> <<%= user.email %>>
Date: <%= Time.now.strftime("%a, %e %b %Y %H:%M:%S %z") %>
Subject: Fix Vulnerability - <%= vulnerability.title %>
<%= diff %>
--
2.19.1
\ No newline at end of file
---
title: Create MR from Vulnerability Solution
merge_request: 9326
author:
type: added
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddMergeRequestIdToVulnerabilityFeedback < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_reference :vulnerability_feedback, :merge_request, null: true, index: true, foreign_key: { on_delete: :nullify }
end
end
......@@ -32,6 +32,10 @@ def line
@data[:line].presence || @data.dig(:location, :start_line)
end
def remediation
@data[:remediation].presence
end
def location_text
return file unless line
......
......@@ -15,11 +15,13 @@
let(:pipeline_2) { create(:ci_pipeline, project: project) }
let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let!(:vuln_feedback_1) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_1) }
let!(:vuln_feedback_2) { create(:vulnerability_feedback, :issue, :sast, project: project, author: user, pipeline: pipeline_1, issue: issue) }
let!(:vuln_feedback_3) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_2) }
let!(:vuln_feedback_4) { create(:vulnerability_feedback, :dismissal, :dependency_scanning, project: project, author: user, pipeline: pipeline_2) }
let!(:vuln_feedback_5) { create(:vulnerability_feedback, :merge_request, :dependency_scanning, project: project, author: user, pipeline: pipeline_1, merge_request: merge_request) }
context '@vulnerability_feedback' do
it 'returns a successful 200 response' do
......@@ -32,7 +34,7 @@
list_feedbacks
expect(response).to match_response_schema('vulnerability_feedback_list', dir: 'ee')
expect(json_response.length).to eq 4
expect(json_response.length).to eq 5
end
context 'with filter params' do
......
......@@ -9,6 +9,7 @@
project
author
issue nil
merge_request nil
association :pipeline, factory: :ci_pipeline
feedback_type 'dismissal'
category 'sast'
......@@ -23,6 +24,11 @@
feedback_type 'issue'
end
trait :merge_request do
feedback_type 'merge_request'
merge_request { create(:merge_request, source_project: project) }
end
trait :sast do
category 'sast'
end
......
......@@ -13,6 +13,7 @@
"name" : { "type": "string" },
"project_fingerprint": { "type": "string" },
"vulnerability_feedback_issue_path": { "type": "string" },
"vulnerability_feedback_merge_request_path": { "type": "string" },
"vulnerability_feedback_dismissal_path": { "type": "string" },
"confidence" : {
"type": "string",
......@@ -46,6 +47,10 @@
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
]},
"merge_request_feedback" : { "oneOf": [
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
]},
"dismissal_feedback" : { "oneOf": [
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
......
......@@ -18,9 +18,11 @@
},
"issue_iid": { "type": ["integer", "null"] },
"issue_url": { "type": ["string", "null"] },
"merge_request_iid": { "type": ["integer", "null"] },
"merge_request_url": { "type": ["string", "null"] },
"feedback_type": {
"type": "string",
"enum": ["dismissal", "issue"]
"enum": ["dismissal", "issue", "merge_request"]
},
"category": {
"type": "string",
......
......@@ -10,6 +10,7 @@
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to belong_to(:issue) }
it { is_expected.to belong_to(:merge_request) }
it { is_expected.to belong_to(:pipeline).class_name('Ci::Pipeline').with_foreign_key('pipeline_id') }
end
......
......@@ -230,4 +230,81 @@
is_expected.to eq({ 4 => 1, 5 => 2, 6 => 3 })
end
end
describe 'feedback' do
set(:project) { create(:project) }
let(:occurrence) do
create(
:vulnerabilities_occurrence,
report_type: :dependency_scanning,
project: project
)
end
describe '#issue_feedback' do
let(:issue) { create(:issue, project: project) }
let!(:issue_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:issue,
issue: issue,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.issue_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'issue'
expect(feedback[:issue_id]).to eq issue.id
end
end
describe '#dismissal_feedback' do
let!(:dismissal_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:dismissal,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.dismissal_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'dismissal'
end
end
describe '#merge_request_feedback' do
let(:merge_request) { create(:merge_request, source_project: project) }
let!(:merge_request_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:merge_request,
merge_request: merge_request,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.merge_request_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'merge_request'
expect(feedback[:merge_request_id]).to eq merge_request.id
end
end
end
end
......@@ -63,6 +63,7 @@
it 'does not contain vulnerability feedback paths' do
expect(subject).not_to include(:vulnerability_feedback_issue_path)
expect(subject).not_to include(:vulnerability_feedback_merge_request_path)
expect(subject).not_to include(:vulnerability_feedback_dismissal_path)
end
end
......@@ -80,6 +81,10 @@
expect(subject).to include(:vulnerability_feedback_issue_path)
end
it 'contains vulnerability feedback merge_request path' do
expect(subject).to include(:vulnerability_feedback_merge_request_path)
end
context 'when disallowed to create issue' do
let(:project) { create(:project, issues_access_level: ProjectFeature::DISABLED) }
......@@ -90,6 +95,26 @@
it 'contains vulnerability feedback dismissal path' do
expect(subject).to include(:vulnerability_feedback_dismissal_path)
end
it 'contains vulnerability feedback merge_request path' do
expect(subject).to include(:vulnerability_feedback_merge_request_path)
end
end
context 'when disallowed to create merge_request' do
let(:project) { create(:project, merge_requests_access_level: ProjectFeature::DISABLED) }
it 'does not contain vulnerability feedback merge_request path' do
expect(subject).not_to include(:vulnerability_feedback_merge_request_path)
end
it 'contains vulnerability feedback issue path' do
expect(subject).to include(:vulnerability_feedback_issue_path)
end
it 'contains vulnerability feedback dismissal path' do
expect(subject).to include(:vulnerability_feedback_dismissal_path)
end
end
end
end
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment