Commit 3e05629d authored by Victor Zagorodny's avatar Victor Zagorodny Committed by James Lopez

Ensure Vulnerabilities to be created as confidential - backstage

parent 7f7a6d2d
......@@ -151,16 +151,17 @@ 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
rule { can?(:read_project_security_dashboard) & can?(:developer_access) }.policy do
enable :read_vulnerability
enable :create_vulnerability
enable :resolve_vulnerability
enable :dismiss_vulnerability
end
rule { can?(:read_project) & (can?(:read_merge_request) | can?(:read_build)) }.enable :read_vulnerability_feedback
......@@ -205,9 +206,10 @@ module EE
enable :read_deployment
enable :read_pages
enable :read_project_security_dashboard
enable :create_vulnerability
enable :resolve_vulnerability
enable :dismiss_vulnerability
end
rule { auditor & can?(:read_project_security_dashboard) }.policy do
enable :read_vulnerability
end
rule { auditor & ~guest }.policy do
......
......@@ -19,14 +19,6 @@ 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
......@@ -80,7 +72,7 @@ module API
use :pagination
end
get ':id/vulnerabilities' do
authorize_can_read!
authorize! :read_vulnerability, user_project
vulnerabilities = paginate(
vulnerabilities_by(user_project)
......@@ -96,7 +88,7 @@ module API
requires :finding_id, type: Integer, desc: 'The id of confirmed vulnerability finding'
end
post ':id/vulnerabilities' do
authorize_can_create!
authorize! :create_vulnerability, user_project
vulnerability = ::Vulnerabilities::CreateService.new(
user_project, current_user, finding_id: params[:finding_id]
......
......@@ -33,7 +33,7 @@ describe ProjectPolicy do
let(:additional_developer_permissions) do
%i[
admin_vulnerability_feedback read_project_security_dashboard read_feature_flag
create_vulnerability resolve_vulnerability dismiss_vulnerability
read_vulnerability create_vulnerability resolve_vulnerability dismiss_vulnerability
]
end
let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
......@@ -46,8 +46,7 @@ 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
create_vulnerability resolve_vulnerability dismiss_vulnerability
read_project_security_dashboard read_vulnerability
read_vulnerability_feedback read_software_license_policy
]
end
......
......@@ -3,6 +3,8 @@
require 'spec_helper'
describe API::Vulnerabilities do
include AccessMatchersForRequest
before do
stub_licensed_features(security_dashboard: true)
end
......@@ -39,7 +41,7 @@ describe API::Vulnerabilities do
describe 'GET /projects/:id/vulnerabilities' do
let_it_be(:project) { create(:project, :with_vulnerabilities) }
subject { get api(project_vulnerabilities_path, user) }
subject(:get_vulnerabilities) { get api(project_vulnerabilities_path, user) }
context 'with an authorized user with proper permissions' do
before do
......@@ -47,7 +49,7 @@ describe API::Vulnerabilities do
end
it 'returns all vulnerabilities of a project' do
subject
get_vulnerabilities
expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers
......@@ -59,7 +61,7 @@ describe API::Vulnerabilities do
let(:project_vulnerabilities_path) { "#{super()}?page=2&per_page=1" }
it 'paginates the vulnerabilities according to the pagination params' do
subject
get_vulnerabilities
expect(response).to have_gitlab_http_status(200)
expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id)
......@@ -69,8 +71,17 @@ describe API::Vulnerabilities do
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'
describe 'permissions' do
it { expect { get_vulnerabilities }.to be_allowed_for(:admin) }
it { expect { get_vulnerabilities }.to be_allowed_for(:owner).of(project) }
it { expect { get_vulnerabilities }.to be_allowed_for(:maintainer).of(project) }
it { expect { get_vulnerabilities }.to be_allowed_for(:developer).of(project) }
it { expect { get_vulnerabilities }.to be_allowed_for(:auditor) }
it { expect { get_vulnerabilities }.to be_denied_for(:reporter).of(project) }
it { expect { get_vulnerabilities }.to be_denied_for(:guest).of(project) }
it { expect { get_vulnerabilities }.to be_denied_for(:anonymous) }
end
end
describe 'POST /projects/:id/vulnerabilities' do
......@@ -79,9 +90,7 @@ describe API::Vulnerabilities do
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
subject(:create_vulnerability) { post api(project_vulnerabilities_path, user), params: { finding_id: finding_id } }
context 'with an authorized user with proper permissions' do
before do
......@@ -133,19 +142,20 @@ describe API::Vulnerabilities do
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
describe 'permissions' do
it { expect { create_vulnerability }.to be_allowed_for(:admin) }
it { expect { create_vulnerability }.to be_allowed_for(:owner).of(project) }
it { expect { create_vulnerability }.to be_allowed_for(:maintainer).of(project) }
it { expect { create_vulnerability }.to be_allowed_for(:developer).of(project) }
expect(response).to have_gitlab_http_status(403)
it { expect { create_vulnerability }.to be_denied_for(:auditor) }
it { expect { create_vulnerability }.to be_denied_for(:reporter).of(project) }
it { expect { create_vulnerability }.to be_denied_for(:guest).of(project) }
it { expect { create_vulnerability }.to be_denied_for(:anonymous) }
end
end
describe "POST /vulnerabilities:id/dismiss" do
describe 'POST /vulnerabilities:id/dismiss' do
before do
create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project)
end
......@@ -153,7 +163,7 @@ describe API::Vulnerabilities do
let_it_be(:project) { create(:project, :with_vulnerabilities) }
let(:vulnerability) { project.vulnerabilities.first }
subject { post api("/vulnerabilities/#{vulnerability.id}/dismiss", user) }
subject(:dismiss_vulnerability) { post api("/vulnerabilities/#{vulnerability.id}/dismiss", user) }
context 'with an authorized user with proper permissions' do
before do
......@@ -162,7 +172,7 @@ describe API::Vulnerabilities do
it 'dismisses a vulnerability and its associated findings' do
Timecop.freeze do
subject
dismiss_vulnerability
expect(response).to have_gitlab_http_status(201)
expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
......@@ -196,7 +206,7 @@ describe API::Vulnerabilities do
end
it 'responds with error' do
subject
dismiss_vulnerability
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']).to eq('base' => ['something went wrong'])
......@@ -207,7 +217,7 @@ describe API::Vulnerabilities do
let(:vulnerability) { create(:vulnerability, :closed, project: project) }
it 'responds with 304 Not Modified' do
subject
dismiss_vulnerability
expect(response).to have_gitlab_http_status(304)
end
......@@ -216,11 +226,20 @@ 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 access level'
it_behaves_like 'prevents working with vulnerabilities for anonymous users'
describe 'permissions' do
it { expect { dismiss_vulnerability }.to be_allowed_for(:admin) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:owner).of(project) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:maintainer).of(project) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:developer).of(project) }
it { expect { dismiss_vulnerability }.to be_denied_for(:auditor) }
it { expect { dismiss_vulnerability }.to be_denied_for(:reporter).of(project) }
it { expect { dismiss_vulnerability }.to be_denied_for(:guest).of(project) }
it { expect { dismiss_vulnerability }.to be_denied_for(:anonymous) }
end
end
describe "POST /vulnerabilities:id/resolve" do
describe 'POST /vulnerabilities:id/resolve' do
before do
create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
end
......@@ -228,7 +247,7 @@ describe API::Vulnerabilities do
let_it_be(:project) { create(:project, :with_vulnerabilities) }
let(:vulnerability) { project.vulnerabilities.first }
subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
subject(:resolve_vulnerability) { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
context 'with an authorized user with proper permissions' do
before do
......@@ -237,7 +256,7 @@ describe API::Vulnerabilities do
it 'resolves a vulnerability and its associated findings' do
Timecop.freeze do
subject
resolve_vulnerability
expect(response).to have_gitlab_http_status(201)
expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
......@@ -252,7 +271,7 @@ describe API::Vulnerabilities do
let(:vulnerability) { create(:vulnerability, :closed, project: project) }
it 'responds with 304 Not Modified response' do
subject
resolve_vulnerability
expect(response).to have_gitlab_http_status(304)
end
......@@ -261,7 +280,16 @@ 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 access level'
it_behaves_like 'prevents working with vulnerabilities for anonymous users'
describe 'permissions' do
it { expect { resolve_vulnerability }.to be_allowed_for(:admin) }
it { expect { resolve_vulnerability }.to be_allowed_for(:owner).of(project) }
it { expect { resolve_vulnerability }.to be_allowed_for(:maintainer).of(project) }
it { expect { resolve_vulnerability }.to be_allowed_for(:developer).of(project) }
it { expect { resolve_vulnerability }.to be_denied_for(:auditor) }
it { expect { resolve_vulnerability }.to be_denied_for(:reporter).of(project) }
it { expect { resolve_vulnerability }.to be_denied_for(:guest).of(project) }
it { expect { resolve_vulnerability }.to be_denied_for(:anonymous) }
end
end
end
......@@ -3,6 +3,8 @@
require 'spec_helper'
describe API::VulnerabilityFindings do
include AccessMatchersForRequest
let_it_be(:project) { create(:project, :public) }
let_it_be(:user) { create(:user) }
......@@ -175,14 +177,32 @@ describe API::VulnerabilityFindings do
end
end
end
end
it_behaves_like 'responds with "not found" when there is no access to the project' do
subject { get api(project_vulnerability_findings_path, user) }
context 'when security dashboard feature is not available' do
before do
stub_licensed_features(security_dashboard: false)
end
it 'responds with 403 Forbidden' do
get api(project_vulnerability_findings_path, user)
expect(response).to have_gitlab_http_status(403)
end
end
end
it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level' do
subject { get api(project_vulnerability_findings_path, user) }
describe 'permissions' do
subject(:get_vulnerability_findings) { get api(project_vulnerability_findings_path, user) }
it { expect { get_vulnerability_findings }.to be_allowed_for(:admin) }
it { expect { get_vulnerability_findings }.to be_allowed_for(:owner).of(project) }
it { expect { get_vulnerability_findings }.to be_allowed_for(:maintainer).of(project) }
it { expect { get_vulnerability_findings }.to be_allowed_for(:developer).of(project) }
it { expect { get_vulnerability_findings }.to be_allowed_for(:auditor) }
it { expect { get_vulnerability_findings }.to be_denied_for(:reporter).of(project) }
it { expect { get_vulnerability_findings }.to be_denied_for(:guest).of(project) }
it { expect { get_vulnerability_findings }.to be_denied_for(:anonymous) }
end
end
end
......@@ -3,6 +3,8 @@
require 'spec_helper'
describe Vulnerabilities::DismissService do
include AccessMatchersGeneric
before do
stub_licensed_features(security_dashboard: true)
end
......@@ -12,7 +14,7 @@ describe Vulnerabilities::DismissService do
let(:vulnerability) { create(:vulnerability, :with_findings, project: project) }
let(:service) { described_class.new(user, vulnerability) }
subject { service.execute }
subject(:dismiss_vulnerability) { service.execute }
context 'with an authorized user with proper permissions' do
before do
......@@ -21,7 +23,7 @@ describe Vulnerabilities::DismissService do
it 'dismisses a vulnerability and its associated findings' do
Timecop.freeze do
subject
dismiss_vulnerability
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
......@@ -38,7 +40,7 @@ describe Vulnerabilities::DismissService do
let(:broken_finding) { vulnerability.findings.first }
it 'responds with error' do
expect(subject.errors.messages).to eq(
expect(dismiss_vulnerability.errors.messages).to eq(
base: ["failed to dismiss associated finding(id=#{broken_finding.id}): something went wrong"])
end
end
......@@ -49,18 +51,20 @@ describe Vulnerabilities::DismissService do
end
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
expect { dismiss_vulnerability }.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
describe 'permissions' do
it { expect { dismiss_vulnerability }.to be_allowed_for(:admin) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:owner).of(project) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:maintainer).of(project) }
it { expect { dismiss_vulnerability }.to be_allowed_for(:developer).of(project) }
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
end
it { expect { dismiss_vulnerability }.to be_denied_for(:auditor) }
it { expect { dismiss_vulnerability }.to be_denied_for(:reporter).of(project) }
it { expect { dismiss_vulnerability }.to be_denied_for(:guest).of(project) }
it { expect { dismiss_vulnerability }.to be_denied_for(:anonymous) }
end
end
......@@ -3,6 +3,8 @@
require 'spec_helper'
describe Vulnerabilities::ResolveService do
include AccessMatchersGeneric
before do
stub_licensed_features(security_dashboard: true)
end
......@@ -12,7 +14,7 @@ describe Vulnerabilities::ResolveService do
let(:vulnerability) { create(:vulnerability, project: project) }
let(:service) { described_class.new(user, vulnerability) }
subject { service.execute }
subject(:resolve_vulnerability) { service.execute }
context 'with an authorized user with proper permissions' do
before do
......@@ -21,7 +23,7 @@ describe Vulnerabilities::ResolveService do
it 'resolves a vulnerability' do
Timecop.freeze do
subject
resolve_vulnerability
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
......@@ -34,18 +36,20 @@ describe Vulnerabilities::ResolveService do
end
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
expect { resolve_vulnerability }.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
describe 'permissions' do
it { expect { resolve_vulnerability }.to be_allowed_for(:admin) }
it { expect { resolve_vulnerability }.to be_allowed_for(:owner).of(project) }
it { expect { resolve_vulnerability }.to be_allowed_for(:maintainer).of(project) }
it { expect { resolve_vulnerability }.to be_allowed_for(:developer).of(project) }
it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
end
it { expect { resolve_vulnerability }.to be_denied_for(:auditor) }
it { expect { resolve_vulnerability }.to be_denied_for(:reporter).of(project) }
it { expect { resolve_vulnerability }.to be_denied_for(:guest).of(project) }
it { expect { resolve_vulnerability }.to be_denied_for(:anonymous) }
end
end
# frozen_string_literal: true
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
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
shared_examples 'responds with "not found" when there is no access to the project' do
context 'with no project access' do
let(:project) { create(:project) }
it 'responds with 404 Not Found' do
subject
expect(response).to have_gitlab_http_status(404)
end
end
context 'with unknown project' do
before do
project.id = 0
end
let(:project) { build(:project) }
it 'responds with 404 Not Found' do
subject
expect(response).to have_gitlab_http_status(404)
end
end
end
# frozen_string_literal: true
module AccessMatchersHelpers
USER_ACCESSOR_METHOD_NAME = 'user'
def provide_user(role, membership = nil)
case role
when :admin
create(:admin)
when :auditor
create(:user, :auditor)
when :user
create(:user)
when :external
create(:user, :external)
when :visitor, :anonymous
nil
when User
role
when *Gitlab::Access.sym_options_with_owner.keys # owner, maintainer, developer, reporter, guest
raise ArgumentError, "cannot provide #{role} when membership reference is blank" unless membership
provide_user_by_membership(role, membership)
else
raise ArgumentError, "cannot provide user of an unknown role #{role}"
end
end
def provide_user_by_membership(role, membership)
if role == :owner && membership.owner
membership.owner
else
create(:user).tap do |user|
membership.public_send(:"add_#{role}", user)
end
end
end
def raise_if_non_block_expectation!(actual)
raise ArgumentError, 'This matcher supports block expectations only.' unless actual.is_a?(Proc)
end
def update_owner(objects, user)
return unless objects
objects.each do |object|
if object.respond_to?(:owner)
object.update_attribute(:owner, user)
elsif object.respond_to?(:user)
object.update_attribute(:user, user)
else
raise ArgumentError, "cannot own this object #{object}"
end
end
end
def patch_example_group(user)
return if user.nil? # for anonymous users
# This call is evaluated in context of ExampleGroup instance in which the matcher is called. Overrides the `user`
# (or defined by `method_name`) method generated by `let` definition in example group before it's used by `subject`.
# This override is per concrete example only because the example group class gets re-created for each example.
instance_eval(<<~CODE, __FILE__, __LINE__ + 1)
if instance_variable_get(:@__#{USER_ACCESSOR_METHOD_NAME}_patched)
raise ArgumentError, 'An access matcher be_allowed_for/be_denied_for can be used only once per example (`it` block)'
end
instance_variable_set(:@__#{USER_ACCESSOR_METHOD_NAME}_patched, true)
def #{USER_ACCESSOR_METHOD_NAME}
@#{USER_ACCESSOR_METHOD_NAME} ||= User.find(#{user.id})
end
CODE
end
def prepare_matcher_environment(role, membership, owned_objects)
user = provide_user(role, membership)
if user
update_owner(owned_objects, user)
patch_example_group(user)
end
end
def run_matcher(action, role, membership, owned_objects)
raise_if_non_block_expectation!(action)
prepare_matcher_environment(role, membership, owned_objects)
if block_given?
yield action
else
action.call
end
end
end
# frozen_string_literal: true
# AccessMatchersForRequest
#
# Matchers to test the access permissions for requests specs (most useful for API tests).
module AccessMatchersForRequest
extend RSpec::Matchers::DSL
include AccessMatchersHelpers
EXPECTED_STATUS_CODES_ALLOWED = [200, 201, 204, 302, 304].freeze
EXPECTED_STATUS_CODES_DENIED = [401, 403, 404].freeze
def description_for(role, type, expected, result)
"be #{type} for #{role} role. Expected status code: any of #{expected.join(', ')} Got: #{result}"
end
matcher :be_allowed_for do |role|
match do |action|
# methods called in this and negated block are being run in context of ExampleGroup
# (not matcher) instance so we have to pass data via local vars
run_matcher(action, role, @membership, @owned_objects)
EXPECTED_STATUS_CODES_ALLOWED.include?(response.status)
end
match_when_negated do |action|
run_matcher(action, role, @membership, @owned_objects)
EXPECTED_STATUS_CODES_DENIED.include?(response.status)
end
chain :of do |membership|
@membership = membership
end
chain :own do |*owned_objects|
@owned_objects = owned_objects
end
failure_message do
"expected this action to #{description_for(role, 'allowed', EXPECTED_STATUS_CODES_ALLOWED, response.status)}"
end
failure_message_when_negated do
"expected this action to #{description_for(role, 'denied', EXPECTED_STATUS_CODES_DENIED, response.status)}"
end
supports_block_expectations
end
RSpec::Matchers.define_negated_matcher :be_denied_for, :be_allowed_for
end
# frozen_string_literal: true
# AccessMatchersGeneric
#
# Matchers to test the access permissions for service classes or other generic pieces of business logic.
module AccessMatchersGeneric
extend RSpec::Matchers::DSL
include AccessMatchersHelpers
ERROR_CLASS = Gitlab::Access::AccessDeniedError
def error_message(error)
str = error.class.name
str += ": #{error.message}" if error.message != error.class.name
str
end
def error_expectation_message(allowed, error)
if allowed
"Expected to raise nothing but #{error_message(error)} was raised."
else
"Expected to raise #{ERROR_CLASS} but nothing was raised."
end
end
def description_for(role, type, error)
allowed = type == 'allowed'
"be #{type} for #{role} role. #{error_expectation_message(allowed, error)}"
end
matcher :be_allowed_for do |role|
match do |action|
# methods called in this and negated block are being run in context of ExampleGroup
# (not matcher) instance so we have to pass data via local vars
run_matcher(action, role, @membership, @owned_objects) do |action|
action.call
rescue => e
@error = e
raise unless e.is_a?(ERROR_CLASS)
end
@error.nil?
end
chain :of do |membership|
@membership = membership
end
chain :own do |*owned_objects|
@owned_objects = owned_objects
end
failure_message do
"expected this action to #{description_for(role, 'allowed', @error)}"
end
failure_message_when_negated do
"expected this action to #{description_for(role, 'denied', @error)}"
end
supports_block_expectations
end
RSpec::Matchers.define_negated_matcher :be_denied_for, :be_allowed_for
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