From 0fd18b78559e718c95277939d0a6b92140a382eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 15 Mar 2023 14:21:41 +0100 Subject: [PATCH 01/10] Add ability to read_vulnerability to custom roles - add migration adding the colum - change the query preloading the permissions - change model and policy classes Changelog: added --- ..._add_read_vulnerability_to_member_roles.rb | 7 + db/schema_migrations/20230424094117 | 1 + db/structure.sql | 3 +- ...vulnerability_severities_count_resolver.rb | 2 +- ee/app/models/ee/user.rb | 5 + ee/app/models/members/member_role.rb | 17 ++- ...user_member_roles_in_projects_preloader.rb | 19 ++- ee/app/policies/ee/project_policy.rb | 8 ++ .../instance_security_dashboard_policy.rb | 1 + ee/lib/api/member_roles.rb | 4 +- ee/lib/api/vulnerability_issue_links.rb | 2 +- ee/spec/factories/member_roles.rb | 1 + ...rability_severities_count_resolver_spec.rb | 2 +- .../lib/ee/api/entities/member_role_spec.rb | 1 + ee/spec/models/ee/user_spec.rb | 48 ++++++- ee/spec/models/members/member_role_spec.rb | 11 ++ ...member_roles_in_projects_preloader_spec.rb | 31 ++++- ee/spec/policies/project_policy_spec.rb | 129 ++++++++++++++---- ee/spec/requests/api/member_roles_spec.rb | 37 +++-- locale/gitlab.pot | 3 + 20 files changed, 280 insertions(+), 52 deletions(-) create mode 100644 db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb create mode 100644 db/schema_migrations/20230424094117 diff --git a/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb b/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb new file mode 100644 index 00000000000000..a39ea1b91785ac --- /dev/null +++ b/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddReadVulnerabilityToMemberRoles < Gitlab::Database::Migration[2.1] + def change + add_column :member_roles, :read_vulnerability, :boolean, default: false + end +end diff --git a/db/schema_migrations/20230424094117 b/db/schema_migrations/20230424094117 new file mode 100644 index 00000000000000..3e0002ddd6b58b --- /dev/null +++ b/db/schema_migrations/20230424094117 @@ -0,0 +1 @@ +e69855d9b788edb799158a839917e5320461891be3a0bd1799e46827e36bdaab \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 94c36a22fd46d4..78b74488d5c8d4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17762,7 +17762,8 @@ CREATE TABLE member_roles ( created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, base_access_level integer NOT NULL, - read_code boolean DEFAULT false + read_code boolean DEFAULT false, + read_vulnerability boolean DEFAULT false ); CREATE SEQUENCE member_roles_id_seq diff --git a/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb b/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb index baa37149cad14d..f8ca8d88779263 100644 --- a/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb +++ b/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb @@ -6,7 +6,7 @@ class VulnerabilitySeveritiesCountResolver < VulnerabilitiesBaseResolver include Gitlab::Graphql::Authorize::AuthorizeResource type Types::VulnerabilitySeveritiesCountType, null: true - authorize :read_security_resource + authorize :read_vulnerability authorizes_object! argument :project_id, [GraphQL::Types::ID], diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 7aae0e2c7cd7f7..4991d5ccbd5ad4 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -508,6 +508,11 @@ def read_code_for?(project) roles&.include?(:read_code) end + def read_vulnerability_for?(project) + roles = preloaded_member_roles_for_projects([project])[project.id] + roles&.include?(:read_vulnerability) + end + override :preloaded_member_roles_for_projects def preloaded_member_roles_for_projects(projects) resource_key = "member_roles_in_projects:#{self.class}:#{self.id}" diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 4ac27dd3c5364a..12904742933fa8 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -5,7 +5,12 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass ignore_column :download_code, remove_with: '15.9', remove_after: '2023-01-22' MAX_COUNT_PER_GROUP_HIERARCHY = 10 - ALL_CUSTOMIZABLE_PERMISSIONS = { read_code: 'Permission to read code' }.freeze + ALL_CUSTOMIZABLE_PERMISSIONS = { + read_code: + { description: 'Permission to read code', minimal_level: Gitlab::Access::GUEST }, + read_vulnerability: + { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::REPORTER } + }.freeze CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code].freeze NON_PERMISSION_COLUMNS = [:id, :namespace_id, :created_at, :updated_at, :base_access_level].freeze @@ -18,6 +23,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass validate :max_count_per_group_hierarchy, on: :create validate :validate_namespace_locked, on: :update validate :attributes_locked_after_member_associated, on: :update + validate :validate_minimal_base_access_level validates_associated :members @@ -63,6 +69,15 @@ def validate_namespace_locked errors.add(:namespace, s_("MemberRole|can't be changed")) end + def validate_minimal_base_access_level + ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission, params| + next unless self[permission] + next if base_access_level >= params[:minimal_level] + + errors.add(:base_access_level, s_("MemberRole|minimal base access level is too low")) + end + end + def attributes_locked_after_member_associated return unless members.present? diff --git a/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb b/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb index 96c81198ce0030..fd8681383cf0a3 100644 --- a/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb +++ b/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb @@ -26,29 +26,31 @@ def execute value_list = Arel::Nodes::ValuesList.new(sql_values_array) sql = <<~SQL - SELECT project_ids.project_id, custom_permissions.read_code FROM (#{value_list.to_sql}) AS project_ids (project_id, namespace_ids), + SELECT project_ids.project_id, bool_or(custom_permissions.read_code) as read_code, bool_or(custom_permissions.read_vulnerability) as read_vulnerability + FROM (#{value_list.to_sql}) AS project_ids (project_id, namespace_ids), LATERAL ( ( - #{Member.select('read_code') + #{Member.select('read_code, read_vulnerability') .left_outer_joins(:member_role) .where("members.source_type = 'Project' AND members.source_id = project_ids.project_id") .with_user(user) - .where(member_roles: { read_code: true }) + .where('member_roles.read_code = true or member_roles.read_vulnerability = true') .limit(1).to_sql} ) UNION ALL ( - #{Member.select('read_code') + #{Member.select('read_code, read_vulnerability') .left_outer_joins(:member_role) .where("members.source_type = 'Namespace' AND members.source_id IN (SELECT UNNEST(project_ids.namespace_ids) as ids)") .with_user(user) - .where(member_roles: { read_code: true }) + .where('member_roles.read_code = true or member_roles.read_vulnerability = true') .limit(1).to_sql} ) UNION ALL ( - SELECT false AS read_code + SELECT false AS read_code, false AS read_vulnerability ) LIMIT 1 ) AS custom_permissions + GROUP BY project_ids.project_id; SQL grouped_by_project = ApplicationRecord.connection.execute(sql).to_a.group_by do |h| @@ -58,6 +60,11 @@ def execute grouped_by_project.transform_values do |value| custom_attributes = [] custom_attributes << :read_code if value.find { |custom_role| custom_role["read_code"] == true } + + if value.find { |custom_role| custom_role["read_vulnerability"] == true } + custom_attributes << :read_vulnerability + end + custom_attributes end end diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 6208ae4415589f..981d4347b14e58 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -214,6 +214,13 @@ module ProjectPolicy @user.read_code_for?(project) end + desc "Custom role on project that enables read vulnerability" + condition(:role_enables_read_vulnerability) do + next unless @user.is_a?(User) + + @user.read_vulnerability_for?(project) + end + with_scope :subject condition(:suggested_reviewers_available) do @subject.can_suggest_reviewers? @@ -543,6 +550,7 @@ module ProjectPolicy end rule { custom_roles_allowed & role_enables_read_code }.enable :read_code + rule { custom_roles_allowed & role_enables_read_vulnerability }.enable :read_vulnerability rule { can?(:create_issue) & okrs_enabled }.policy do enable :create_objective diff --git a/ee/app/policies/instance_security_dashboard_policy.rb b/ee/app/policies/instance_security_dashboard_policy.rb index 89dda82a86287f..22e950fbafab6f 100644 --- a/ee/app/policies/instance_security_dashboard_policy.rb +++ b/ee/app/policies/instance_security_dashboard_policy.rb @@ -14,6 +14,7 @@ class InstanceSecurityDashboardPolicy < BasePolicy rule { ~anonymous }.policy do enable :read_instance_security_dashboard enable :read_security_resource + enable :read_vulnerability end rule { security_dashboard_enabled & can?(:read_instance_security_dashboard) }.policy do diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb index e0234ad599e60a..a8abb5a15b99f4 100644 --- a/ee/lib/api/member_roles.rb +++ b/ee/lib/api/member_roles.rb @@ -42,8 +42,8 @@ class MemberRoles < ::API::Base documentation: { example: 10 } ) - ::MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission_name, permission_description| - optional permission_name.to_s, type: Boolean, desc: permission_description + ::MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission_name, permission_params| + optional permission_name.to_s, type: Boolean, desc: permission_params[:descrption] end end diff --git a/ee/lib/api/vulnerability_issue_links.rb b/ee/lib/api/vulnerability_issue_links.rb index 8c3e50b32ab7aa..b6387fce9ee24f 100644 --- a/ee/lib/api/vulnerability_issue_links.rb +++ b/ee/lib/api/vulnerability_issue_links.rb @@ -36,7 +36,7 @@ def find_issue_link! is_array true end get ':id/issue_links' do - vulnerability = find_and_authorize_vulnerability!(:read_security_resource) + vulnerability = find_and_authorize_vulnerability!(:read_vulnerability) related_issues = vulnerability.related_issues.with_api_entity_associations.with_vulnerability_links present Ability.issues_readable_by_user(related_issues, current_user), with: EE::API::Entities::VulnerabilityRelatedIssue diff --git a/ee/spec/factories/member_roles.rb b/ee/spec/factories/member_roles.rb index 503438d2521193..1c6f9806ee902c 100644 --- a/ee/spec/factories/member_roles.rb +++ b/ee/spec/factories/member_roles.rb @@ -6,6 +6,7 @@ base_access_level { Gitlab::Access::DEVELOPER } trait(:developer) { base_access_level { Gitlab::Access::DEVELOPER } } + trait(:reporter) { base_access_level { Gitlab::Access::REPORTER } } trait(:guest) { base_access_level { Gitlab::Access::GUEST } } end end diff --git a/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb b/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb index 01ee1440cb6360..18cfe5f97c9552 100644 --- a/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb @@ -8,7 +8,7 @@ describe '#resolve' do subject { resolve(described_class, obj: vulnerable, args: filters, ctx: { current_user: current_user }) } - let_it_be(:project) { create(:project) } + let_it_be(:project) { create(:project, :security_and_compliance_enabled) } let_it_be(:cluster_agent) { create(:cluster_agent, project: project) } let_it_be(:user) { create(:user, security_dashboard_projects: [project]) } diff --git a/ee/spec/lib/ee/api/entities/member_role_spec.rb b/ee/spec/lib/ee/api/entities/member_role_spec.rb index 9eba9e7ce204a5..87bae88d765950 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -16,6 +16,7 @@ expect(subject[:id]).to eq member_role.id expect(subject[:base_access_level]).to eq member_role.base_access_level expect(subject[:read_code]).to eq member_role.read_code + expect(subject[:read_vulnerability]).to eq member_role.read_vulnerability expect(subject[:group_id]).to eq(member_role.namespace.id) end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index ecbae299a08506..ddc47452f4e03f 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -757,8 +757,12 @@ ("users"."user_type" = 0 OR "users"."user_type" IS NULL OR "users"."user_type" IN (0, 4, 5)) AND (EXISTS (SELECT 1 FROM ((SELECT "members".* FROM "members" - WHERE (members.access_level > 10))) members - WHERE "members"."user_id" = "users"."id")) + WHERE (members.access_level > 10)) + UNION + (SELECT "members".* FROM "members" + INNER JOIN "member_roles" ON "member_roles"."id" = "members"."member_role_id" + WHERE "members"."access_level" = 10 AND (read_vulnerability = true))) members + WHERE "members"."user_id" = "users"."id")) SQL expect(users.to_sql.squish).to eq(expected_sql.squish), "query was changed. Please ensure query is covered with an index and adjust this test case" @@ -2124,6 +2128,46 @@ end end + describe '#read_vulnerability_for?', :request_store do + let_it_be(:project) { create(:project, :in_group) } + let_it_be(:user) { create(:user) } + + before do + stub_licensed_features(custom_roles: true) + end + + before_all do + project_member = create(:project_member, :reporter, user: user, source: project) + create( + :member_role, + :reporter, + read_vulnerability: true, + members: [project_member], + namespace: project.group + ) + end + + context 'when read_vulnerability present in preloaded custom roles' do + before do + user.read_vulnerability_for?(project) + end + + it 'returns true' do + expect(user.read_vulnerability_for?(project)).to be true + end + + it 'does not perform extra queries when asked for projects have already been preloaded' do + expect { user.read_vulnerability_for?(project) }.not_to exceed_query_limit(0) + end + end + + context 'when project not present in preloaded custom roles' do + it 'loads the custom role' do + expect(user.read_vulnerability_for?(project)).to be true + end + end + end + describe '#can_group_owner_disable_two_factor?' do let_it_be(:group) { create(:group) } let_it_be(:owner) { create(:user) } diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index 07149eac613092..b792461e9282f3 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -100,6 +100,17 @@ expect(member_role.errors[:namespace]).to include(s_("MemberRole|can't be changed")) end end + + context 'when base_access_level is too low' do + it 'creates a validation error' do + member_role.base_access_level = 10 + member_role.read_vulnerability = true + + expect(member_role).not_to be_valid + expect(member_role.errors[:base_access_level]) + .to include(s_("MemberRole|minimal base access level is too low")) + end + end end end diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index e9b5a44071ee4b..e3394a6ec91322 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -26,11 +26,11 @@ end context 'when project has custom role' do - let_it_be(:member_role) do - create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) - end - context 'when custom role has read_code: true' do + let_it_be(:member_role) do + create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) + end + context 'when Array of project passed' do it 'returns the project_id with a value array that includes :read_code' do expect(result).to eq({ project.id => [:read_code] }) @@ -45,6 +45,29 @@ end end end + + context 'when custom role has read_vulnerability: true' do + let_it_be(:user) { create(:user) } + let_it_be(:developer_member) { create(:project_member, :developer, user: user, source: project) } + let_it_be(:member_role) do + create(:member_role, :developer, members: [developer_member], + namespace: project.group, read_vulnerability: true) + end + + context 'when Array of project passed' do + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_vulnerability] }) + end + end + + context 'when ActiveRecord::Relation of projects passed' do + let(:project_list) { Project.where(id: project.id) } + + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_vulnerability] }) + end + end + end end context 'when project namespace has a custom role with read_code: true' do diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index d20d2932177aae..a068cd94b7d58e 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2425,27 +2425,51 @@ def expect_private_project_permissions_as_if_non_member end context 'custom role' do - let_it_be(:current_user) { create(:user) } + def custom_role_allowed?(user, ability) + described_class.new(user, project).allowed?(ability) + end + + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } let_it_be(:project) { private_project_in_group } - let_it_be(:group_member) do + let_it_be(:group_member_guest) do create( :group_member, - user: current_user, + user: guest, source: project.group, access_level: Gitlab::Access::GUEST ) end - let_it_be(:project_member) do + let_it_be(:group_member_reporter) do + create( + :group_member, + user: reporter, + source: project.group, + access_level: Gitlab::Access::REPORTER + ) + end + + let_it_be(:project_member_guest) do create( :project_member, :guest, - user: current_user, + user: guest, project: project, access_level: Gitlab::Access::GUEST ) end + let_it_be(:project_member_reporter) do + create( + :project_member, + :guest, + user: reporter, + project: project, + access_level: Gitlab::Access::REPORTER + ) + end + let_it_be(:member_role_read_code_true) do create( :member_role, @@ -2464,62 +2488,119 @@ def expect_private_project_permissions_as_if_non_member ) end + let_it_be(:member_role_read_vulnerability_true) do + create( + :member_role, + :reporter, + namespace: project.group, + read_vulnerability: true + ) + end + + let_it_be(:member_role_read_vulnerability_false) do + create( + :member_role, + :reporter, + namespace: project.group, + read_vulnerability: false + ) + end + context 'custom_roles license enabled' do before do stub_licensed_features(custom_roles: true) end context 'custom role for parent group' do - context 'custom role allows read code' do - before do - member_role_read_code_true.members << group_member + context 'custom role allows abilities' do + it 'allows guest to read code' do + member_role_read_code_true.members << group_member_guest + + expect(custom_role_allowed?(guest, :read_code)).to be_truthy end - it { is_expected.to be_allowed(:read_code) } + it 'allows reporter to read vulnerability' do + member_role_read_vulnerability_true.members << group_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + end end - context 'custom role disallows read code' do - before do - member_role_read_code_false.members << group_member + context 'custom role disallows abilities' do + it 'does not allow guest to read code' do + member_role_read_code_false.members << group_member_guest + + expect(custom_role_allowed?(guest, :read_code)).to be_falsey end - it { is_expected.to be_disallowed(:read_code) } + it 'does not allow reporter to read vulnerability' do + member_role_read_vulnerability_false.members << group_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + end end end context 'custom role on project membership' do - context 'custom role allows read code' do - before do - member_role_read_code_true.members << project_member + context 'custom role allows abilities' do + it 'allows guest to read code' do + member_role_read_code_true.members << project_member_guest + + expect(custom_role_allowed?(guest, :read_code)).to be_truthy end - it { is_expected.to be_allowed(:read_code) } + it 'allows reporter to read vulnerability' do + member_role_read_vulnerability_true.members << project_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + end end - context 'custom role disallows read code' do - before do - member_role_read_code_false.members << project_member + context 'custom role disallows abilities' do + it 'does not allow guest to read code' do + member_role_read_code_false.members << project_member_guest + + expect(custom_role_allowed?(guest, :read_code)).to be_falsey end - it { is_expected.to be_disallowed(:read_code) } + it 'does not allow reporter to read vulnerability' do + member_role_read_vulnerability_false.members << project_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + end end end context 'multiple custom roles in hierarchy with different read_code values' do + let(:current_user) { guest } + before do - member_role_read_code_true.members << project_member - member_role_read_code_false.members << group_member + member_role_read_code_true.members << project_member_guest + member_role_read_code_false.members << group_member_guest end # allows read code if any of the custom roles allow it it { is_expected.to be_allowed(:read_code) } end + + context 'multiple custom roles in hierarchy with different read_vulnerability values' do + let(:current_user) { reporter } + + before do + member_role_read_vulnerability_true.members << project_member_reporter + member_role_read_vulnerability_false.members << group_member_reporter + end + + it { is_expected.to be_allowed(:read_vulnerability) } + end end context 'without custom_roles license enabled' do + let(:current_user) { guest } + before do stub_licensed_features(custom_roles: false) - member_role_read_code_true.members << project_member + member_role_read_code_true.members << project_member_guest end it { is_expected.to be_disallowed(:read_code) } diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index 5cff4002c3b8b5..9293308d7e58c8 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -22,7 +22,8 @@ :member_role, namespace: group_with_member_roles, base_access_level: ::Gitlab::Access::REPORTER, - read_code: false + read_code: false, + read_vulnerability: true ) end @@ -31,7 +32,8 @@ :member_role, namespace: group_with_member_roles, base_access_level: ::Gitlab::Access::REPORTER, - read_code: true + read_code: true, + read_vulnerability: false ) end @@ -93,12 +95,14 @@ "id" => member_role_1.id, "base_access_level" => ::Gitlab::Access::REPORTER, "read_code" => false, + "read_vulnerability" => true, "group_id" => group_id }, { "id" => member_role_2.id, "base_access_level" => ::Gitlab::Access::REPORTER, "read_code" => true, + "read_vulnerability" => false, "group_id" => group_id } ] @@ -129,7 +133,7 @@ describe "POST /groups/:id/member_roles" do let_it_be(:params) do - { base_access_level: ::Gitlab::Access::MAINTAINER, read_code: true } + { base_access_level: ::Gitlab::Access::GUEST, read_code: true } end subject { post api("/groups/#{group_id}/member_roles", current_user), params: params } @@ -171,7 +175,7 @@ aggregate_failures "testing response" do expect(response).to have_gitlab_http_status(:created) - expect(json_response['base_access_level']).to eq(::Gitlab::Access::MAINTAINER) + expect(json_response['base_access_level']).to eq(::Gitlab::Access::GUEST) expect(json_response['read_code']).to eq(true) end end @@ -188,13 +192,28 @@ end context "when params are invalid" do - let(:params) { { base_access_level: 1 } } + context "when base_access level is invalid" do + let_it_be(:params) do + { base_access_level: ::Gitlab::Access::GUEST, read_vulnerability: true } + end - it "returns a 400 error when params are invalid" do - subject + it "returns a 400 error when params are invalid" do + subject - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq('base_access_level does not have a valid value') + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to include('minimal base access level is too low') + end + end + + context "when base_access level is too low for the ability" do + let(:params) { { base_access_level: 1 } } + + it "returns a 400 error when params are invalid" do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('base_access_level does not have a valid value') + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3b426ca7f89a6b..bce28114f41ac0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27548,6 +27548,9 @@ msgstr "" msgid "MemberRole|maximum number of Member Roles are already in use by the group hierarchy. Please delete an existing Member Role." msgstr "" +msgid "MemberRole|minimal base access level is too low" +msgstr "" + msgid "MemberRole|must be top-level namespace" msgstr "" -- GitLab From ebd9fbbcdc6b0e80263a590fb047e65d9d3b75ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Thu, 27 Apr 2023 10:39:39 +0200 Subject: [PATCH 02/10] Apply code review suggestions --- ee/app/models/members/member_role.rb | 10 +++++++--- ee/lib/api/member_roles.rb | 2 +- ee/spec/models/ee/user_spec.rb | 8 ++------ ee/spec/models/members/member_role_spec.rb | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 12904742933fa8..36a7ae04c7f87a 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -11,7 +11,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass read_vulnerability: { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::REPORTER } }.freeze - CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code].freeze + CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code, :read_vulnerability].freeze NON_PERMISSION_COLUMNS = [:id, :namespace_id, :created_at, :updated_at, :base_access_level].freeze has_many :members @@ -71,10 +71,14 @@ def validate_namespace_locked def validate_minimal_base_access_level ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission, params| + min_level = params[:minimal_level] next unless self[permission] - next if base_access_level >= params[:minimal_level] + next if base_access_level >= min_level - errors.add(:base_access_level, s_("MemberRole|minimal base access level is too low")) + errors.add(:base_access_level, + format(s_("MemberRole|minimal base access level must be %{min_access_level}."), + min_access_level: "#{Gitlab::Access.options_with_owner.key(min_level)} (#{min_level})") + ) end end diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb index a8abb5a15b99f4..20b1fbd3604ae2 100644 --- a/ee/lib/api/member_roles.rb +++ b/ee/lib/api/member_roles.rb @@ -43,7 +43,7 @@ class MemberRoles < ::API::Base ) ::MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission_name, permission_params| - optional permission_name.to_s, type: Boolean, desc: permission_params[:descrption] + optional permission_name.to_s, type: Boolean, desc: permission_params[:description] end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index ddc47452f4e03f..bd03804fcc667d 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -757,12 +757,8 @@ ("users"."user_type" = 0 OR "users"."user_type" IS NULL OR "users"."user_type" IN (0, 4, 5)) AND (EXISTS (SELECT 1 FROM ((SELECT "members".* FROM "members" - WHERE (members.access_level > 10)) - UNION - (SELECT "members".* FROM "members" - INNER JOIN "member_roles" ON "member_roles"."id" = "members"."member_role_id" - WHERE "members"."access_level" = 10 AND (read_vulnerability = true))) members - WHERE "members"."user_id" = "users"."id")) + WHERE (members.access_level > 10))) members + WHERE "members"."user_id" = "users"."id")) SQL expect(users.to_sql.squish).to eq(expected_sql.squish), "query was changed. Please ensure query is covered with an index and adjust this test case" diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index b792461e9282f3..7665e2b91b0c5f 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -108,7 +108,7 @@ expect(member_role).not_to be_valid expect(member_role.errors[:base_access_level]) - .to include(s_("MemberRole|minimal base access level is too low")) + .to include(s_("MemberRole|minimal base access level must be Reporter (20).")) end end end -- GitLab From 6771f98a24e8aa3143eabe1c52a2c8d6bb9b1b94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Fri, 28 Apr 2023 06:16:48 +0200 Subject: [PATCH 03/10] fix failing builds --- ee/spec/requests/api/member_roles_spec.rb | 2 +- locale/gitlab.pot | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index 9293308d7e58c8..86edac876171cd 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -201,7 +201,7 @@ subject expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include('minimal base access level is too low') + expect(json_response['message']).to include('minimal base access level must be Reporter (20)') end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bce28114f41ac0..486e124b76ba6c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27548,7 +27548,7 @@ msgstr "" msgid "MemberRole|maximum number of Member Roles are already in use by the group hierarchy. Please delete an existing Member Role." msgstr "" -msgid "MemberRole|minimal base access level is too low" +msgid "MemberRole|minimal base access level must be %{min_access_level}." msgstr "" msgid "MemberRole|must be top-level namespace" -- GitLab From c351d9db652177804b3ecccdf77bee0d99a4eb5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Mon, 1 May 2023 09:10:22 +0200 Subject: [PATCH 04/10] Add custom roles vulnerability abilities under FF --- ee/app/policies/ee/project_policy.rb | 6 +++++- .../custom_roles_vulnerability.yml | 8 ++++++++ ee/spec/policies/project_policy_spec.rb | 20 ++++++++++++++++--- 3 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 ee/config/feature_flags/development/custom_roles_vulnerability.yml diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 981d4347b14e58..0b8eb7a5e39fe7 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -207,6 +207,10 @@ module ProjectPolicy @subject.custom_roles_enabled? end + condition(:custom_roles_vulnerabilities_allowed) do + ::Feature.enabled?(:custom_roles_vulnerability, @subject.root_ancestor) + end + desc "Custom role on project that enables read code" condition(:role_enables_read_code) do next unless @user.is_a?(User) @@ -550,7 +554,7 @@ module ProjectPolicy end rule { custom_roles_allowed & role_enables_read_code }.enable :read_code - rule { custom_roles_allowed & role_enables_read_vulnerability }.enable :read_vulnerability + rule { custom_roles_allowed & custom_roles_vulnerabilities_allowed & role_enables_read_vulnerability }.enable :read_vulnerability rule { can?(:create_issue) & okrs_enabled }.policy do enable :create_objective diff --git a/ee/config/feature_flags/development/custom_roles_vulnerability.yml b/ee/config/feature_flags/development/custom_roles_vulnerability.yml new file mode 100644 index 00000000000000..d130b9ba2863a0 --- /dev/null +++ b/ee/config/feature_flags/development/custom_roles_vulnerability.yml @@ -0,0 +1,8 @@ +--- +name: custom_roles_vulnerability +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114734 +rollout_issue_url: +milestone: '16.0' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index a068cd94b7d58e..86d5b60d798678 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2549,10 +2549,24 @@ def custom_role_allowed?(user, ability) expect(custom_role_allowed?(guest, :read_code)).to be_truthy end - it 'allows reporter to read vulnerability' do - member_role_read_vulnerability_true.members << project_member_reporter + context 'when custom_roles_vulnerability FF is enabled' do + before do + stub_feature_flags(custom_roles_vulnerability: [project.group]) + end - expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + it 'allows reporter to read vulnerability' do + member_role_read_vulnerability_true.members << project_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + end + end + + context 'when custom_roles_vulnerability FF is disabled' do + it 'disallows reporter to read vulnerability' do + member_role_read_vulnerability_true.members << project_member_reporter + + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + end end end -- GitLab From c16c414c975aebbd18429587fffd0f958bd3c119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Mon, 1 May 2023 22:44:07 +0200 Subject: [PATCH 05/10] Add specs and set minimal level to Guest for vuln --- ee/app/models/members/member_role.rb | 4 +- ee/spec/models/ee/user_spec.rb | 14 +- ee/spec/models/members/member_role_spec.rb | 2 +- ...member_roles_in_projects_preloader_spec.rb | 185 +++++++++--------- 4 files changed, 107 insertions(+), 98 deletions(-) diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 36a7ae04c7f87a..128d3e26ed0f89 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -9,9 +9,9 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass read_code: { description: 'Permission to read code', minimal_level: Gitlab::Access::GUEST }, read_vulnerability: - { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::REPORTER } + { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::GUEST } }.freeze - CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code, :read_vulnerability].freeze + CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code].freeze NON_PERMISSION_COLUMNS = [:id, :namespace_id, :created_at, :updated_at, :base_access_level].freeze has_many :members diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index bd03804fcc667d..dfda121b50aebe 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -757,8 +757,12 @@ ("users"."user_type" = 0 OR "users"."user_type" IS NULL OR "users"."user_type" IN (0, 4, 5)) AND (EXISTS (SELECT 1 FROM ((SELECT "members".* FROM "members" - WHERE (members.access_level > 10))) members - WHERE "members"."user_id" = "users"."id")) + WHERE (members.access_level > 10)) + UNION + (SELECT "members".* FROM "members" INNER JOIN "member_roles" ON "member_roles"."id" = "members"."member_role_id" + WHERE "members"."access_level" = 10 + AND + (read_vulnerability = true))) members WHERE "members"."user_id" = "users"."id")) SQL expect(users.to_sql.squish).to eq(expected_sql.squish), "query was changed. Please ensure query is covered with an index and adjust this test case" @@ -2127,6 +2131,7 @@ describe '#read_vulnerability_for?', :request_store do let_it_be(:project) { create(:project, :in_group) } let_it_be(:user) { create(:user) } + let_it_be(:another_user) { create(:user) } before do stub_licensed_features(custom_roles: true) @@ -2146,12 +2151,17 @@ context 'when read_vulnerability present in preloaded custom roles' do before do user.read_vulnerability_for?(project) + another_user.read_vulnerability_for?(project) end it 'returns true' do expect(user.read_vulnerability_for?(project)).to be true end + it 'returns false for a user not assigned to the custom role' do + expect(another_user.read_vulnerability_for?(project)).to be false + end + it 'does not perform extra queries when asked for projects have already been preloaded' do expect { user.read_vulnerability_for?(project) }.not_to exceed_query_limit(0) end diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index 7665e2b91b0c5f..5c6e08914a7a32 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -103,7 +103,7 @@ context 'when base_access_level is too low' do it 'creates a validation error' do - member_role.base_access_level = 10 + member_role.base_access_level = 0 member_role.read_vulnerability = true expect(member_role).not_to be_valid diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index e3394a6ec91322..1a870aa0f8c36c 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -11,131 +11,130 @@ subject(:result) { described_class.new(projects: project_list, user: user).execute } - context 'when custom_roles license is not enabled on project root ancestor' do - it 'skips preload' do - stub_licensed_features(custom_roles: false) - create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) + shared_examples 'custom roles' do |ability| + context 'when custom_roles license is not enabled on project root ancestor' do + it 'skips preload' do + stub_licensed_features(custom_roles: false) + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = true + record.save! + record.members << project_member + end - expect(result).to eq({}) + expect(result).to eq({}) + end end - end - context 'when custom_roles license is enabled on project root ancestor' do - before do - stub_licensed_features(custom_roles: true) - end + context 'when custom_roles license is enabled on project root ancestor' do + before do + stub_licensed_features(custom_roles: true) + end - context 'when project has custom role' do - context 'when custom role has read_code: true' do + context 'when project has custom role' do let_it_be(:member_role) do - create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = true + record.save! + record.members << project_member + end end - context 'when Array of project passed' do - it 'returns the project_id with a value array that includes :read_code' do - expect(result).to eq({ project.id => [:read_code] }) + context 'when custom role has ability: true' do + context 'when Array of project passed' do + it 'returns the project_id with a value array that includes the ability' do + expect(result).to eq({ project.id => [ability] }) + end end - end - context 'when ActiveRecord::Relation of projects passed' do - let(:project_list) { Project.where(id: project.id) } + context 'when ActiveRecord::Relation of projects passed' do + let(:project_list) { Project.where(id: project.id) } - it 'returns the project_id with a value array that includes :read_code' do - expect(result).to eq({ project.id => [:read_code] }) + it 'returns the project_id with a value array that includes the ability' do + expect(result).to eq({ project.id => [ability] }) + end end end end - context 'when custom role has read_vulnerability: true' do - let_it_be(:user) { create(:user) } - let_it_be(:developer_member) { create(:project_member, :developer, user: user, source: project) } + context 'when project namespace has a custom role with ability: true' do + let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.namespace) } let_it_be(:member_role) do - create(:member_role, :developer, members: [developer_member], - namespace: project.group, read_vulnerability: true) - end - - context 'when Array of project passed' do - it 'returns the project_id with a value array that includes :read_code' do - expect(result).to eq({ project.id => [:read_vulnerability] }) + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = true + record.save! + record.members << group_member end end - context 'when ActiveRecord::Relation of projects passed' do - let(:project_list) { Project.where(id: project.id) } - - it 'returns the project_id with a value array that includes :read_code' do - expect(result).to eq({ project.id => [:read_vulnerability] }) - end + it 'returns the project_id with a value array that includes the ability' do + expect(result).to eq({ project.id => [ability] }) end end - end - context 'when project namespace has a custom role with read_code: true' do - let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.namespace) } - let_it_be(:member_role) do - create(:member_role, :guest, read_code: true, members: [group_member], namespace: project.group) - end + context 'when user is a member of the project in multiple ways' do + let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } - it 'returns the project_id with a value array that includes :read_code' do - expect(result).to eq({ project.id => [:read_code] }) - end - end + it 'project value array includes the if any custom roles enable them' do + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = false + record.save! + record.members << project_member + end + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = true + record.save! + record.members << project_member + end - context 'when user is a member of the project in multiple ways' do - let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } + expect(result[project.id]).to match_array([ability]) + end + end - it 'project value array includes :read_code if any custom roles enable them' do - create(:member_role, :guest, read_code: false, members: [project_member], namespace: project.group) - create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) + context 'when project membership has no custom role' do + let_it_be(:project) { create(:project, :private, :in_group) } - expect(result[project.id]).to match_array([:read_code]) + it 'returns project id with empty value array' do + expect(result).to eq(project.id => []) + end end - end - context 'when project membership has no custom role' do - let_it_be(:project) { create(:project, :private, :in_group) } + context 'when project membership has custom role that does not enable custom permission' do + let_it_be(:project) { create(:project, :private, :in_group) } + + it 'returns project id with empty value array' do + project_without_custom_permission_member = create( + :project_member, + :guest, + user: user, + source: project + ) + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = false + record.save! + record.members << project_without_custom_permission_member + end - it 'returns project id with empty value array' do - expect(result).to eq(project.id => []) + expect(result).to eq(project.id => []) + end end - end - context 'when project membership has custom role that does not enable custom permission' do - let_it_be(:project) { create(:project, :private, :in_group) } - - it 'returns project id with empty value array' do - project_without_custom_permission_member = create( - :project_member, - :guest, - user: user, - source: project - ) - create( - :member_role, - :guest, - read_code: false, - members: [project_without_custom_permission_member], - namespace: project.group - ) - - expect(result).to eq(project.id => []) - end - end + context 'when user has custom role that enables custom permission outside of project hierarchy' do + it 'ignores custom role outside of project hierarchy' do + # subgroup is within parent group of project but not above project + subgroup = create(:group, parent: project.group) + subgroup_member = create(:group_member, :guest, user: user, source: subgroup) + _custom_role_outside_hierarchy = create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = false + record.save! + record.members << subgroup_member + end - context 'when user has custom role that enables custom permission outside of project hierarchy' do - it 'ignores custom role outside of project hierarchy' do - # subgroup is within parent group of project but not above project - subgroup = create(:group, parent: project.group) - subgroup_member = create(:group_member, :guest, user: user, source: subgroup) - _custom_role_outside_hierarchy = create( - :member_role, :guest, - members: [subgroup_member], - read_code: true, - namespace: project.group - ) - - expect(result).to eq({ project.id => [] }) + expect(result).to eq({ project.id => [] }) + end end end end + + it_behaves_like 'custom roles', :read_code + it_behaves_like 'custom roles', :read_vulnerability end -- GitLab From 6f83842ecde4a2de24c0362361d22f8c38ba4789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Mon, 1 May 2023 23:42:50 +0200 Subject: [PATCH 06/10] Fix specs --- ee/spec/models/members/member_role_spec.rb | 4 ++-- ee/spec/requests/api/member_roles_spec.rb | 25 +++++----------------- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index 5c6e08914a7a32..a1615f7c405a51 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -103,12 +103,12 @@ context 'when base_access_level is too low' do it 'creates a validation error' do - member_role.base_access_level = 0 + member_role.base_access_level = 5 member_role.read_vulnerability = true expect(member_role).not_to be_valid expect(member_role.errors[:base_access_level]) - .to include(s_("MemberRole|minimal base access level must be Reporter (20).")) + .to include(s_("MemberRole|minimal base access level must be Guest (10).")) end end end diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index 86edac876171cd..bb255c8a8eb640 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -192,28 +192,13 @@ end context "when params are invalid" do - context "when base_access level is invalid" do - let_it_be(:params) do - { base_access_level: ::Gitlab::Access::GUEST, read_vulnerability: true } - end - - it "returns a 400 error when params are invalid" do - subject - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include('minimal base access level must be Reporter (20)') - end - end + let(:params) { { base_access_level: 1 } } - context "when base_access level is too low for the ability" do - let(:params) { { base_access_level: 1 } } - - it "returns a 400 error when params are invalid" do - subject + it "returns a 400 error when params are invalid" do + subject - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq('base_access_level does not have a valid value') - end + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('base_access_level does not have a valid value') end end -- GitLab From a3be62b783ecb8b6742e6c3072395f2be43f3ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 3 May 2023 21:34:18 +0200 Subject: [PATCH 07/10] Small fixes - db migration default value - validation behind FF - specs organisation --- ..._add_read_vulnerability_to_member_roles.rb | 2 +- db/structure.sql | 2 +- ee/app/models/members/member_role.rb | 4 ++- .../custom_roles_vulnerability.yml | 2 +- ee/lib/api/member_roles.rb | 2 +- ee/spec/models/ee/user_spec.rb | 10 ++++--- ee/spec/models/members/member_role_spec.rb | 27 ++++++++++++++----- 7 files changed, 34 insertions(+), 15 deletions(-) diff --git a/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb b/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb index a39ea1b91785ac..4ae4a7f6bffb78 100644 --- a/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb +++ b/db/migrate/20230424094117_add_read_vulnerability_to_member_roles.rb @@ -2,6 +2,6 @@ class AddReadVulnerabilityToMemberRoles < Gitlab::Database::Migration[2.1] def change - add_column :member_roles, :read_vulnerability, :boolean, default: false + add_column :member_roles, :read_vulnerability, :boolean, default: false, null: false end end diff --git a/db/structure.sql b/db/structure.sql index 78b74488d5c8d4..833e83c61dfae1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17763,7 +17763,7 @@ CREATE TABLE member_roles ( updated_at timestamp with time zone NOT NULL, base_access_level integer NOT NULL, read_code boolean DEFAULT false, - read_vulnerability boolean DEFAULT false + read_vulnerability boolean DEFAULT false NOT NULL ); CREATE SEQUENCE member_roles_id_seq diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 128d3e26ed0f89..cf4a99e6f4cea3 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -23,7 +23,9 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass validate :max_count_per_group_hierarchy, on: :create validate :validate_namespace_locked, on: :update validate :attributes_locked_after_member_associated, on: :update - validate :validate_minimal_base_access_level + validate :validate_minimal_base_access_level, if: ->(member_role) do + Feature.enabled?(:custom_roles_vulnerability, member_role.namespace&.root_ancestor) + end validates_associated :members diff --git a/ee/config/feature_flags/development/custom_roles_vulnerability.yml b/ee/config/feature_flags/development/custom_roles_vulnerability.yml index d130b9ba2863a0..c6a3317a26e4f6 100644 --- a/ee/config/feature_flags/development/custom_roles_vulnerability.yml +++ b/ee/config/feature_flags/development/custom_roles_vulnerability.yml @@ -1,7 +1,7 @@ --- name: custom_roles_vulnerability introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114734 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/409387 milestone: '16.0' type: development group: group::authentication and authorization diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb index 20b1fbd3604ae2..512dd61f9c652c 100644 --- a/ee/lib/api/member_roles.rb +++ b/ee/lib/api/member_roles.rb @@ -43,7 +43,7 @@ class MemberRoles < ::API::Base ) ::MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission_name, permission_params| - optional permission_name.to_s, type: Boolean, desc: permission_params[:description] + optional permission_name.to_s, type: Boolean, desc: permission_params[:description], default: false end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index dfda121b50aebe..6d25e99f27fb94 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -2158,10 +2158,6 @@ expect(user.read_vulnerability_for?(project)).to be true end - it 'returns false for a user not assigned to the custom role' do - expect(another_user.read_vulnerability_for?(project)).to be false - end - it 'does not perform extra queries when asked for projects have already been preloaded' do expect { user.read_vulnerability_for?(project) }.not_to exceed_query_limit(0) end @@ -2172,6 +2168,12 @@ expect(user.read_vulnerability_for?(project)).to be true end end + + context 'when a user is not assigned to the custom role' do + it 'returns false' do + expect(another_user.read_vulnerability_for?(project)).to be false + end + end end describe '#can_group_owner_disable_two_factor?' do diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index a1615f7c405a51..b8732ee895d7cf 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -102,13 +102,28 @@ end context 'when base_access_level is too low' do - it 'creates a validation error' do - member_role.base_access_level = 5 - member_role.read_vulnerability = true + context 'when custom_roles_vulnerability FF is enabled' do + it 'creates a validation error' do + member_role.base_access_level = 5 + member_role.read_vulnerability = true + + expect(member_role).not_to be_valid + expect(member_role.errors[:base_access_level]) + .to include(s_("MemberRole|minimal base access level must be Guest (10).")) + end + end - expect(member_role).not_to be_valid - expect(member_role.errors[:base_access_level]) - .to include(s_("MemberRole|minimal base access level must be Guest (10).")) + context 'when custom_roles_vulnerability FF is disabled' do + before do + stub_feature_flags(custom_roles_vulnerability: false) + end + + it 'is valid' do + member_role.base_access_level = 5 + member_role.read_vulnerability = true + + expect(member_role).to be_valid + end end end end -- GitLab From 99a56cb5250f79f8aa442e35b0b38865a5cf2e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Mon, 8 May 2023 12:44:18 +0200 Subject: [PATCH 08/10] Fix specs --- ee/spec/models/ee/user_spec.rb | 1 - .../preloaders/user_member_roles_in_projects_preloader_spec.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 6d25e99f27fb94..da36e3f9ab9ddd 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -2151,7 +2151,6 @@ context 'when read_vulnerability present in preloaded custom roles' do before do user.read_vulnerability_for?(project) - another_user.read_vulnerability_for?(project) end it 'returns true' do diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index 1a870aa0f8c36c..0db84fb0101a3d 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -74,7 +74,7 @@ context 'when user is a member of the project in multiple ways' do let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } - it 'project value array includes the if any custom roles enable them' do + it 'project value array includes the ability' do create(:member_role, :guest, namespace: project.group).tap do |record| record[ability] = false record.save! -- GitLab From d2318028d0cd8ddf0b503bbb9d3f275758f9af26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Tue, 9 May 2023 20:28:30 +0200 Subject: [PATCH 09/10] Fix read vulnerability custom role ability --- ee/app/policies/ee/project_policy.rb | 6 +++++- ee/lib/api/vulnerability_issue_links.rb | 2 +- ee/spec/policies/project_policy_spec.rb | 26 +++++++++++++++++++------ 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 0b8eb7a5e39fe7..681e0401edc166 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -554,7 +554,11 @@ module ProjectPolicy end rule { custom_roles_allowed & role_enables_read_code }.enable :read_code - rule { custom_roles_allowed & custom_roles_vulnerabilities_allowed & role_enables_read_vulnerability }.enable :read_vulnerability + rule { custom_roles_allowed & custom_roles_vulnerabilities_allowed & role_enables_read_vulnerability }.policy do + enable :read_vulnerability + enable :read_security_resource + enable :create_vulnerability_export + end rule { can?(:create_issue) & okrs_enabled }.policy do enable :create_objective diff --git a/ee/lib/api/vulnerability_issue_links.rb b/ee/lib/api/vulnerability_issue_links.rb index b6387fce9ee24f..8c3e50b32ab7aa 100644 --- a/ee/lib/api/vulnerability_issue_links.rb +++ b/ee/lib/api/vulnerability_issue_links.rb @@ -36,7 +36,7 @@ def find_issue_link! is_array true end get ':id/issue_links' do - vulnerability = find_and_authorize_vulnerability!(:read_vulnerability) + vulnerability = find_and_authorize_vulnerability!(:read_security_resource) related_issues = vulnerability.related_issues.with_api_entity_associations.with_vulnerability_links present Ability.issues_readable_by_user(related_issues, current_user), with: EE::API::Entities::VulnerabilityRelatedIssue diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 86d5b60d798678..9b6d3f83496fe7 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2519,10 +2519,12 @@ def custom_role_allowed?(user, ability) expect(custom_role_allowed?(guest, :read_code)).to be_truthy end - it 'allows reporter to read vulnerability' do + it 'allows reporter to read security related resources' do member_role_read_vulnerability_true.members << group_member_reporter expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_truthy + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_truthy end end @@ -2533,10 +2535,12 @@ def custom_role_allowed?(user, ability) expect(custom_role_allowed?(guest, :read_code)).to be_falsey end - it 'does not allow reporter to read vulnerability' do + it 'does not allow reporter to read security related resources' do member_role_read_vulnerability_false.members << group_member_reporter expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_falsey + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_falsey end end end @@ -2554,18 +2558,22 @@ def custom_role_allowed?(user, ability) stub_feature_flags(custom_roles_vulnerability: [project.group]) end - it 'allows reporter to read vulnerability' do + it 'allows reporter to read security related resources' do member_role_read_vulnerability_true.members << project_member_reporter expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_truthy + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_truthy end end context 'when custom_roles_vulnerability FF is disabled' do - it 'disallows reporter to read vulnerability' do + it 'does not allow reporter to read security related resources' do member_role_read_vulnerability_true.members << project_member_reporter expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_falsey + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_falsey end end end @@ -2577,10 +2585,12 @@ def custom_role_allowed?(user, ability) expect(custom_role_allowed?(guest, :read_code)).to be_falsey end - it 'does not allow reporter to read vulnerability' do + it 'does not allow reporter to read security related resources' do member_role_read_vulnerability_false.members << project_member_reporter expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_falsey + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_falsey + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_falsey end end end @@ -2605,7 +2615,11 @@ def custom_role_allowed?(user, ability) member_role_read_vulnerability_false.members << group_member_reporter end - it { is_expected.to be_allowed(:read_vulnerability) } + it 'allows reporter to read security related resources' do + expect(custom_role_allowed?(reporter, :read_vulnerability)).to be_truthy + expect(custom_role_allowed?(reporter, :read_security_resource)).to be_truthy + expect(custom_role_allowed?(reporter, :create_vulnerability_export)).to be_truthy + end end end -- GitLab From b8fb0155fcf7305ec37b6917c18a487ddf8468dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 10 May 2023 12:02:15 +0200 Subject: [PATCH 10/10] Change permission checks to read_security_resource --- ee/app/graphql/resolvers/security_training_urls_resolver.rb | 2 +- .../resolvers/vulnerability_severities_count_resolver.rb | 2 +- .../graphql/resolvers/security_training_urls_resolver_spec.rb | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ee/app/graphql/resolvers/security_training_urls_resolver.rb b/ee/app/graphql/resolvers/security_training_urls_resolver.rb index 30a7438fed2c26..f4141d4f312100 100644 --- a/ee/app/graphql/resolvers/security_training_urls_resolver.rb +++ b/ee/app/graphql/resolvers/security_training_urls_resolver.rb @@ -6,7 +6,7 @@ class SecurityTrainingUrlsResolver < BaseResolver type [::Types::Security::TrainingUrlType], null: true - authorize :access_security_and_compliance + authorize :read_security_resource authorizes_object! argument :identifier_external_ids, diff --git a/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb b/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb index f8ca8d88779263..baa37149cad14d 100644 --- a/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb +++ b/ee/app/graphql/resolvers/vulnerability_severities_count_resolver.rb @@ -6,7 +6,7 @@ class VulnerabilitySeveritiesCountResolver < VulnerabilitiesBaseResolver include Gitlab::Graphql::Authorize::AuthorizeResource type Types::VulnerabilitySeveritiesCountType, null: true - authorize :read_vulnerability + authorize :read_security_resource authorizes_object! argument :project_id, [GraphQL::Types::ID], diff --git a/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb b/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb index 6e8866429f1bdd..31454b579635e2 100644 --- a/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb @@ -6,6 +6,10 @@ include GraphqlHelpers describe '#resolve' do + before do + stub_licensed_features(security_dashboard: true) + end + subject { resolve(described_class, obj: project, ctx: { current_user: user }) } let_it_be(:user) { create(:user) } -- GitLab