Commit a5bc331c authored by Victor Zagorodny's avatar Victor Zagorodny Committed by Dmitriy Zaporozhets

Implement Create Vulnerability from Finding

Make title_html field optional in order to be
controlled by CacheMarkdownField logic. Add
POST /projects/:id/vulnerabilities API call.
Add create_vulnerability ability for User.
Add Vulnerabilities::CreateService and
model methods and scopes required for its
functioning. Add new factories and traits.
parent 57f016b4
# frozen_string_literal: true
class AddCachedMarkdownVersionToVulnerabilities < ActiveRecord::Migration[5.2]
DOWNTIME = false
def change
add_column :vulnerabilities, :cached_markdown_version, :integer
end
end
# frozen_string_literal: true
class ChangeVulnerabilitiesTitleHtmlToNullable < ActiveRecord::Migration[5.2]
DOWNTIME = false
def change
change_column_null :vulnerabilities, :title_html, true
end
end
......@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 2019_11_11_115431) do
ActiveRecord::Schema.define(version: 2019_11_12_115317) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_trgm"
......@@ -3928,7 +3928,7 @@ ActiveRecord::Schema.define(version: 2019_11_11_115431) do
t.datetime_with_timezone "created_at", null: false
t.datetime_with_timezone "updated_at", null: false
t.string "title", limit: 255, null: false
t.text "title_html", null: false
t.text "title_html"
t.text "description"
t.text "description_html"
t.bigint "start_date_sourcing_milestone_id"
......@@ -3941,6 +3941,7 @@ ActiveRecord::Schema.define(version: 2019_11_11_115431) do
t.integer "confidence", limit: 2, null: false
t.boolean "confidence_overridden", default: false
t.integer "report_type", limit: 2, null: false
t.integer "cached_markdown_version"
t.index ["author_id"], name: "index_vulnerabilities_on_author_id"
t.index ["closed_by_id"], name: "index_vulnerabilities_on_closed_by_id"
t.index ["due_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_due_date_sourcing_milestone_id"
......
......@@ -67,7 +67,11 @@ module EE
# https://gitlab.com/gitlab-org/gitlab/issues/10252#terminology
has_many :vulnerabilities
has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback'
has_many :vulnerability_findings, class_name: 'Vulnerabilities::Occurrence'
has_many :vulnerability_findings, class_name: 'Vulnerabilities::Occurrence' do
def lock_for_confirmation!(id)
where(vulnerability_id: nil).lock.find(id)
end
end
has_many :vulnerability_identifiers, class_name: 'Vulnerabilities::Identifier'
has_many :vulnerability_scanners, class_name: 'Vulnerabilities::Scanner'
......
# frozen_string_literal: true
class Vulnerability < ApplicationRecord
belongs_to :project
include CacheMarkdownField
include Redactable
include StripAttribute
cache_markdown_field :title, pipeline: :single_line
cache_markdown_field :description, issuable_state_filter_enabled: true
strip_attributes :title
redact_field :description
belongs_to :project # keep this association named 'project' for correct work of markdown cache
belongs_to :milestone
belongs_to :epic
belongs_to :author, class_name: 'User'
belongs_to :author, class_name: 'User' # keep this association named 'author' for correct work of markdown cache
belongs_to :updated_by, class_name: 'User'
belongs_to :last_edited_by, class_name: 'User'
belongs_to :closed_by, class_name: 'User'
......@@ -17,7 +28,7 @@ class Vulnerability < ApplicationRecord
enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
enum report_type: Vulnerabilities::Occurrence::REPORT_TYPES
validates :project, :author, :title, :title_html, :severity, :confidence, :report_type, presence: true
validates :project, :author, :title, :severity, :confidence, :report_type, presence: true
# at this stage Vulnerability is not an Issuable, has some important attributes (and their constraints) in common
validates :title, length: { maximum: Issuable::TITLE_LENGTH_MAX }
......
......@@ -151,12 +151,14 @@ module EE
rule { can?(:developer_access) }.policy do
enable :read_project_security_dashboard
enable :create_vulnerability
enable :resolve_vulnerability
enable :dismiss_vulnerability
end
rule { security_dashboard_feature_disabled }.policy do
prevent :read_project_security_dashboard
prevent :create_vulnerability
prevent :resolve_vulnerability
prevent :dismiss_vulnerability
end
......@@ -203,6 +205,7 @@ module EE
enable :read_deployment
enable :read_pages
enable :read_project_security_dashboard
enable :create_vulnerability
enable :resolve_vulnerability
enable :dismiss_vulnerability
end
......
# frozen_string_literal: true
module Vulnerabilities
class CreateService
include Gitlab::Allowable
def initialize(project, author, finding_id:)
@project = project
@author = author
@finding_id = finding_id
end
def execute
raise Gitlab::Access::AccessDeniedError unless can?(@author, :create_vulnerability, @project)
vulnerability = Vulnerability.new
Vulnerabilities::Occurrence.transaction(requires_new: true) do
# we're using `lock` instead of `with_lock` to avoid extra call to `find` under the hood
finding = @project.vulnerability_findings.lock_for_confirmation!(@finding_id)
save_vulnerability(vulnerability, finding)
rescue ActiveRecord::RecordNotFound
vulnerability.errors.add(:base, _('finding is not found or is already attached to a vulnerability'))
raise ActiveRecord::Rollback
end
vulnerability
end
private
def save_vulnerability(vulnerability, finding)
vulnerability.assign_attributes(
author: @author,
project: @project,
title: finding.name,
state: :opened,
severity: finding.severity,
severity_overridden: false,
confidence: finding.confidence,
confidence_overridden: false,
report_type: finding.report_type
)
vulnerability.save && vulnerability.findings << finding
end
end
end
---
title: Added autogenerated Markdown support for Vulnerability title and description
merge_request: 18283
author:
type: other
......@@ -19,6 +19,14 @@ module API
end
end
def authorize_can_read!
authorize! :read_project_security_dashboard, user_project
end
def authorize_can_create!
authorize! :create_vulnerability, user_project
end
def render_vulnerability(vulnerability)
if vulnerability.valid?
present vulnerability, with: EE::API::Entities::Vulnerability
......@@ -68,8 +76,11 @@ module API
desc 'Get a list of project vulnerabilities' do
success EE::API::Entities::Vulnerability
end
params do
use :pagination
end
get ':id/vulnerabilities' do
authorize! :read_project_security_dashboard, user_project
authorize_can_read!
vulnerabilities = paginate(
vulnerabilities_by(user_project)
......@@ -77,6 +88,26 @@ module API
present vulnerabilities, with: EE::API::Entities::Vulnerability
end
desc 'Create a new Vulnerability (from a confirmed Finding)' do
success EE::API::Entities::Vulnerability
end
params do
requires :finding_id, type: Integer, desc: 'The id of confirmed vulnerability finding'
end
post ':id/vulnerabilities' do
authorize_can_create!
vulnerability = ::Vulnerabilities::CreateService.new(
user_project, current_user, finding_id: params[:finding_id]
).execute
if vulnerability.persisted?
present vulnerability, with: EE::API::Entities::Vulnerability
else
render_validation_error!(vulnerability)
end
end
end
end
end
......@@ -48,6 +48,8 @@ describe Vulnerability do
it { is_expected.to belong_to(:updated_by).class_name('User') }
it { is_expected.to belong_to(:last_edited_by).class_name('User') }
it { is_expected.to belong_to(:closed_by).class_name('User') }
it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').dependent(false) }
end
describe 'validations' do
......@@ -56,7 +58,6 @@ describe Vulnerability do
it { is_expected.to validate_presence_of(:project) }
it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_presence_of(:title_html) }
it { is_expected.to validate_presence_of(:severity) }
it { is_expected.to validate_presence_of(:confidence) }
it { is_expected.to validate_presence_of(:report_type) }
......@@ -66,4 +67,29 @@ describe Vulnerability do
it { is_expected.to validate_length_of(:description).is_at_most(::Issuable::DESCRIPTION_LENGTH_MAX).allow_nil }
it { is_expected.to validate_length_of(:description_html).is_at_most(::Issuable::DESCRIPTION_HTML_LENGTH_MAX).allow_nil }
end
describe 'text fields' do
subject { create(:vulnerability, title: '_My title_ ', description: '**Hello `world`**') }
it 'has proper markdown for title field' do
expect(subject.title_html).to eq('_My title_') # no markdown rendering because it's a single line field
end
it 'has proper markdown for title field' do
expect(subject.description_html).to(
eq('<p data-sourcepos="1:1-1:17" dir="auto"><strong>Hello <code>world</code></strong></p>')
)
end
context 'redactable fields' do
before do
stub_commonmark_sourcepos_disabled
end
it_behaves_like 'model with redactable field' do
let(:model) { create(:vulnerability) }
let(:field) { :description }
end
end
end
end
......@@ -33,7 +33,7 @@ describe ProjectPolicy do
let(:additional_developer_permissions) do
%i[
admin_vulnerability_feedback read_project_security_dashboard read_feature_flag
resolve_vulnerability dismiss_vulnerability
create_vulnerability resolve_vulnerability dismiss_vulnerability
]
end
let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
......@@ -46,7 +46,8 @@ describe ProjectPolicy do
read_pipeline read_build read_commit_status read_container_image
read_environment read_deployment read_merge_request read_pages
create_merge_request_in award_emoji
read_project_security_dashboard resolve_vulnerability dismiss_vulnerability
read_project_security_dashboard
create_vulnerability resolve_vulnerability dismiss_vulnerability
read_vulnerability_feedback read_software_license_policy
]
end
......@@ -494,6 +495,7 @@ describe ProjectPolicy do
include_context 'when security dashboard feature is not available'
it { is_expected.to be_disallowed(:create_vulnerability) }
it { is_expected.to be_disallowed(:resolve_vulnerability) }
it { is_expected.to be_disallowed(:dismiss_vulnerability) }
end
......
......@@ -7,8 +7,8 @@ describe API::Vulnerabilities do
stub_licensed_features(security_dashboard: true)
end
let_it_be(:project) { create(:project, :with_vulnerabilities) }
let_it_be(:user) { create(:user) }
let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" }
shared_examples 'forbids actions on vulnerability in case of disabled features' do
context 'when "first-class vulnerabilities" feature is disabled' do
......@@ -37,7 +37,7 @@ describe API::Vulnerabilities do
end
describe 'GET /projects/:id/vulnerabilities' do
let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" }
let_it_be(:project) { create(:project, :with_vulnerabilities) }
subject { get api(project_vulnerabilities_path, user) }
......@@ -70,7 +70,79 @@ describe API::Vulnerabilities do
end
it_behaves_like 'responds with "not found" when there is no access to the project'
it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
end
describe 'POST /projects/:id/vulnerabilities' do
let_it_be(:project) { create(:project) }
let(:finding) { create(:vulnerabilities_occurrence, project: project) }
let(:finding_id) { finding.id }
let(:expected_error_messages) { { 'base' => ['finding is not found or is already attached to a vulnerability'] } }
subject do
post api(project_vulnerabilities_path, user), params: { finding_id: finding_id }
end
context 'with an authorized user with proper permissions' do
before do
project.add_developer(user)
end
it 'creates a vulnerability from finding and attaches it to the vulnerability' do
expect { subject }.to change { project.vulnerabilities.count }.by(1)
expect(project.vulnerabilities.last).to(
have_attributes(
author: user,
title: finding.name,
state: 'opened',
severity: finding.severity,
severity_overridden: false,
confidence: finding.confidence,
confidence_overridden: false,
report_type: finding.report_type
))
expect(response).to have_gitlab_http_status(201)
expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
end
context 'when finding id is unknown' do
let(:finding_id) { 0 }
it 'responds with expected error' do
subject
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']).to eq(expected_error_messages)
end
end
context 'when a vulnerability already exists for a specific finding' do
before do
create(:vulnerability, findings: [finding], project: finding.project)
end
it 'rejects creation of a new vulnerability from this finding' do
subject
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']).to eq(expected_error_messages)
end
end
it_behaves_like 'forbids actions on vulnerability in case of disabled features'
end
it_behaves_like 'responds with "not found" when there is no access to the project'
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
end
shared_examples 'prevents working with vulnerabilities for anonymous users' do
it do
subject
expect(response).to have_gitlab_http_status(403)
end
end
describe "POST /vulnerabilities:id/dismiss" do
......@@ -78,6 +150,7 @@ describe API::Vulnerabilities do
create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project)
end
let_it_be(:project) { create(:project, :with_vulnerabilities) }
let(:vulnerability) { project.vulnerabilities.first }
subject { post api("/vulnerabilities/#{vulnerability.id}/dismiss", user) }
......@@ -143,7 +216,8 @@ describe API::Vulnerabilities do
it_behaves_like 'forbids actions on vulnerability in case of disabled features'
end
it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
it_behaves_like 'prevents working with vulnerabilities for anonymous users'
end
describe "POST /vulnerabilities:id/resolve" do
......@@ -151,6 +225,7 @@ describe API::Vulnerabilities do
create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
end
let_it_be(:project) { create(:project, :with_vulnerabilities) }
let(:vulnerability) { project.vulnerabilities.first }
subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
......@@ -186,6 +261,7 @@ describe API::Vulnerabilities do
it_behaves_like 'forbids actions on vulnerability in case of disabled features'
end
it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
it_behaves_like 'prevents working with vulnerabilities for anonymous users'
end
end
......@@ -181,7 +181,7 @@ describe API::VulnerabilityFindings do
subject { get api(project_vulnerability_findings_path, user) }
end
it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges' do
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level' do
subject { get api(project_vulnerability_findings_path, user) }
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe Vulnerabilities::CreateService do
before do
stub_licensed_features(security_dashboard: true)
end
let_it_be(:user) { create(:user) }
let(:project) { create(:project) } # cannot use let_it_be here: caching causes problems with permission-related tests
let(:finding) { create(:vulnerabilities_occurrence, project: project) }
let(:finding_id) { finding.id }
let(:expected_error_messages) { { base: ['finding is not found or is already attached to a vulnerability'] } }
subject { described_class.new(project, user, finding_id: finding_id).execute }
context 'with an authorized user with proper permissions' do
before do
project.add_developer(user)
end
it 'creates a vulnerability from finding and attaches it to the vulnerability' do
expect { subject }.to change { project.vulnerabilities.count }.by(1)
expect(project.vulnerabilities.last).to(
have_attributes(
author: user,
title: finding.name,
state: 'opened',
severity: finding.severity,
severity_overridden: false,
confidence: finding.confidence,
confidence_overridden: false,
report_type: finding.report_type
))
end
it 'starts a new transaction for the create sequence' do
allow(Vulnerabilities::Occurrence).to receive(:transaction).and_call_original
subject
expect(Vulnerabilities::Occurrence).to have_received(:transaction).with(requires_new: true).once
end
context 'when finding id is unknown' do
let(:finding_id) { 0 }
it 'adds expected error to the response' do
expect(subject.errors.messages).to eq(expected_error_messages)
end
end
context 'when finding does not belong to the vulnerability project' do
let(:finding) { create(:vulnerabilities_occurrence) }
it 'adds expected error to the response' do
expect(subject.errors.messages).to eq(expected_error_messages)
end
end
context 'when a vulnerability already exists for a specific finding' do
before do
create(:vulnerability, findings: [finding], project: finding.project)
end
it 'rejects creation of a new vulnerability from this finding' do
expect(subject.errors.messages).to eq(expected_error_messages)
end
end
context 'when security dashboard feature is disabled' do
before do
stub_licensed_features(security_dashboard: false)
end
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
end
context 'when user does not have rights to dismiss a vulnerability' do
before do
project.add_reporter(user)
end
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
end
# frozen_string_literal: true
shared_examples 'prevents working with vulnerabilities in case of insufficient privileges' do
context 'with lesser access level than required' do
it 'responds with 403 Forbidden' do
project.add_reporter(user)
shared_examples 'prevents working with vulnerabilities in case of insufficient access level' do
it 'responds 403 Forbidden when accessed by reporter' do
project.add_reporter(user)
subject
subject
expect(response).to have_gitlab_http_status(403)
end
expect(response).to have_gitlab_http_status(403)
end
it 'responds 403 Forbidden when accessed by guest' do
project.add_guest(user)
subject
expect(response).to have_gitlab_http_status(403)
end
end
......
......@@ -20349,6 +20349,9 @@ msgstr ""
msgid "failed to dismiss associated finding(id=%{finding_id}): %{message}"
msgstr ""
msgid "finding is not found or is already attached to a vulnerability"
msgstr ""
msgid "for %{link_to_merge_request} with %{link_to_merge_request_source_branch}"
msgstr ""
......
......@@ -7,44 +7,6 @@ describe Redactable do
stub_commonmark_sourcepos_disabled
end
shared_examples 'model with redactable field' do
it 'redacts unsubscribe token' do
model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
model.save!
expect(model[field]).to eq 'some text /sent_notifications/REDACTED/unsubscribe more text'
end
it 'ignores not hexadecimal tokens' do
text = 'some text /sent_notifications/token/unsubscribe more text'
model[field] = text
model.save!
expect(model[field]).to eq text
end
it 'ignores not matching texts' do
text = 'some text /sent_notifications/.*/unsubscribe more text'
model[field] = text
model.save!
expect(model[field]).to eq text
end
it 'redacts the field when saving the model before creating markdown cache' do
model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
model.save!
expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
expect(model[field]).to eq expected
expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
end
end
context 'when model is an issue' do
it_behaves_like 'model with redactable field' do
let(:model) { create(:issue) }
......
# frozen_string_literal: true
shared_examples 'model with redactable field' do
it 'redacts unsubscribe token' do
model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
model.save!
expect(model[field]).to eq 'some text /sent_notifications/REDACTED/unsubscribe more text'
end
it 'ignores not hexadecimal tokens' do
text = 'some text /sent_notifications/token/unsubscribe more text'
model[field] = text
model.save!
expect(model[field]).to eq text
end
it 'ignores not matching texts' do
text = 'some text /sent_notifications/.*/unsubscribe more text'
model[field] = text
model.save!
expect(model[field]).to eq text
end
it 'redacts the field when saving the model before creating markdown cache' do
model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
model.save!
expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
expect(model[field]).to eq expected
expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment