From 1b129307c3327c02459168dadf99737ef144acb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Tue, 23 May 2023 08:59:30 +0200 Subject: [PATCH 1/2] Add admin_vulnerability to custom roles Changelog: added EE: true --- ...add_admin_vulnerability_to_member_roles.rb | 7 +++ db/schema_migrations/20230522210320 | 1 + db/structure.sql | 3 +- ee/app/models/members/member_role.rb | 25 ++++++++- ee/app/policies/ee/project_policy.rb | 10 ++++ .../lib/ee/api/entities/member_role_spec.rb | 1 + ee/spec/models/members/member_role_spec.rb | 26 +++++++++ ...member_roles_in_projects_preloader_spec.rb | 55 +++++++++---------- ee/spec/policies/project_policy_spec.rb | 28 ++++++++++ ee/spec/requests/api/member_roles_spec.rb | 2 + locale/gitlab.pot | 3 + 11 files changed, 130 insertions(+), 31 deletions(-) create mode 100644 db/migrate/20230522210320_add_admin_vulnerability_to_member_roles.rb create mode 100644 db/schema_migrations/20230522210320 diff --git a/db/migrate/20230522210320_add_admin_vulnerability_to_member_roles.rb b/db/migrate/20230522210320_add_admin_vulnerability_to_member_roles.rb new file mode 100644 index 00000000000000..f3f68f007dd75e --- /dev/null +++ b/db/migrate/20230522210320_add_admin_vulnerability_to_member_roles.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddAdminVulnerabilityToMemberRoles < Gitlab::Database::Migration[2.1] + def change + add_column :member_roles, :admin_vulnerability, :boolean, default: false, null: false + end +end diff --git a/db/schema_migrations/20230522210320 b/db/schema_migrations/20230522210320 new file mode 100644 index 00000000000000..74a32da1d1a572 --- /dev/null +++ b/db/schema_migrations/20230522210320 @@ -0,0 +1 @@ +6c287edd3102743f83a94b3f961074eb2d85c63c7f7789c63cb51a11ef4c04c1 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 3dfcb3ef38ed1a..e761bcb557504c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17843,7 +17843,8 @@ 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 NOT NULL + read_vulnerability boolean DEFAULT false NOT NULL, + admin_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 f2fcb40310eb53..3edc3c22b70a78 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -9,7 +9,12 @@ 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::GUEST } + { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::GUEST }, + admin_vulnerability: { + descripition: 'Permission to read vulnerability', + minimal_level: Gitlab::Access::GUEST, + requirement: :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 @@ -26,6 +31,9 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass validate :validate_minimal_base_access_level, if: ->(member_role) do Feature.enabled?(:custom_roles_vulnerability, member_role.namespace&.root_ancestor) end + validate :validate_requirements, if: ->(member_role) do + Feature.enabled?(:custom_roles_vulnerability, member_role.namespace&.root_ancestor) + end validates_associated :members @@ -85,6 +93,21 @@ def validate_minimal_base_access_level end end + def validate_requirements + ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission, params| + requirement = params[:requirement] + + next unless self[permission] # skipping permissions not set for the object + next unless requirement # skipping permissions that have no requirement + next if self[requirement] # the requierement is met + + errors.add(permission, + format(s_("MemberRole|%{requirement} has to be enabled in order to enable %{permission}."), + requirement: requirement, permission: permission) + ) + end + end + def attributes_locked_after_member_associated return unless members.present? diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 6dda1d176ee5d4..21caa86bea7c11 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -225,6 +225,13 @@ module ProjectPolicy @user.custom_permission_for?(project, :read_vulnerability) end + desc "Custom role on project that enables admin vulnerability" + condition(:role_enables_admin_vulnerability) do + next unless @user.is_a?(User) + + @user.custom_permission_for?(project, :admin_vulnerability) + end + with_scope :subject condition(:suggested_reviewers_available) do @subject.can_suggest_reviewers? @@ -565,6 +572,9 @@ module ProjectPolicy enable :read_security_resource enable :create_vulnerability_export end + rule { custom_roles_allowed & custom_roles_vulnerabilities_allowed & role_enables_admin_vulnerability }.policy do + enable :admin_vulnerability + end rule { can?(:create_issue) & okrs_enabled }.policy do enable :create_objective 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 87bae88d765950..8972656071cba3 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -17,6 +17,7 @@ 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[:admin_vulnerability]).to eq member_role.admin_vulnerability expect(subject[:group_id]).to eq(member_role.namespace.id) end end diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb index e28df6c1bfb8a7..c30d6eb1967439 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -126,6 +126,32 @@ end end end + + context 'when requirement is not met' do + context 'when custom_roles_vulnerability FF is enabled' do + it 'creates a validation error' do + member_role.base_access_level = Gitlab::Access::GUEST + member_role.admin_vulnerability = true + + expect(member_role).not_to be_valid + expect(member_role.errors[:admin_vulnerability]) + .to include(s_("MemberRole|read_vulnerability has to be enabled in order to enable admin_vulnerability.")) + end + end + + 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 = Gitlab::Access::GUEST + member_role.admin_vulnerability = true + + expect(member_role).to be_valid + end + 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 0db84fb0101a3d..93fe43a4326cb4 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,15 +11,27 @@ subject(:result) { described_class.new(projects: project_list, user: user).execute } + def ability_requirement(ability) + ability_definition = MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS[ability] + ability_definition[:requirement] + end + + def create_member_role(ability, member) + create(:member_role, :guest, namespace: project.group).tap do |record| + record[ability] = true + record[ability_requirement(ability)] = true if ability_requirement(ability) + record.save! + record.members << member + end + end + shared_examples 'custom roles' do |ability| + let(:expected_abilities) { [ability, ability_requirement(ability)].compact } + 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 + create_member_role(ability, project_member) expect(result).to eq({}) end @@ -32,17 +44,13 @@ context 'when project has custom role' do let_it_be(:member_role) do - create(:member_role, :guest, namespace: project.group).tap do |record| - record[ability] = true - record.save! - record.members << project_member - end + create_member_role(ability, project_member) end 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] }) + expect(result[project.id]).to match_array(expected_abilities) end end @@ -50,7 +58,7 @@ let(:project_list) { Project.where(id: project.id) } it 'returns the project_id with a value array that includes the ability' do - expect(result).to eq({ project.id => [ability] }) + expect(result[project.id]).to match_array(expected_abilities) end end end @@ -59,15 +67,11 @@ 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, :guest, namespace: project.group).tap do |record| - record[ability] = true - record.save! - record.members << group_member - end + create_member_role(ability, group_member) end it 'returns the project_id with a value array that includes the ability' do - expect(result).to eq({ project.id => [ability] }) + expect(result[project.id]).to match_array(expected_abilities) end end @@ -75,18 +79,14 @@ let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } it 'project value array includes the ability' do + create_member_role(ability, group_member) 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 - expect(result[project.id]).to match_array([ability]) + expect(result[project.id]).to match_array(expected_abilities) end end @@ -123,11 +123,7 @@ # 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 + create_member_role(ability, subgroup_member) expect(result).to eq({ project.id => [] }) end @@ -137,4 +133,5 @@ it_behaves_like 'custom roles', :read_code it_behaves_like 'custom roles', :read_vulnerability + it_behaves_like 'custom roles', :admin_vulnerability end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 51f46747700b57..79f9c25d89037c 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2545,6 +2545,10 @@ def create_member_role(member, abilities = member_role_abilities) end it_behaves_like 'custom roles abilities' + + it 'does not enable to admin_vulnerability' do + expect(subject).to be_disallowed(:admin_vulnerability) + end end context 'with custom_roles_vulnerability FF disabled' do @@ -2560,6 +2564,30 @@ def create_member_role(member, abilities = member_role_abilities) it { is_expected.to be_disallowed(*disallowed_abilities) } end end + + context 'for a member role with admin_vulnerability true' do + context 'with custom_roles_vulnerability FF enabled' do + before do + stub_feature_flags(custom_roles_vulnerability: [project.group]) + end + + let(:member_role_abilities) { { read_vulnerability: true, admin_vulnerability: true } } + let(:allowed_abilities) { [:read_vulnerability, :admin_vulnerability] } + + it_behaves_like 'custom roles abilities' + end + + context 'with custom_roles_vulnerability FF disabled' do + before do + stub_feature_flags(custom_roles_vulnerability: false) + create_member_role(group_member_guest) + end + + let(:disallowed_abilities) { [:admin_vulnerability] } + + it { is_expected.to be_disallowed(*disallowed_abilities) } + end + end end describe 'permissions for suggested reviewers bot', :saas do diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index bb255c8a8eb640..8d1b6a99408b69 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -96,6 +96,7 @@ "base_access_level" => ::Gitlab::Access::REPORTER, "read_code" => false, "read_vulnerability" => true, + "admin_vulnerability" => false, "group_id" => group_id }, { @@ -103,6 +104,7 @@ "base_access_level" => ::Gitlab::Access::REPORTER, "read_code" => true, "read_vulnerability" => false, + "admin_vulnerability" => false, "group_id" => group_id } ] diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8ec3ee9bfe7519..dde0745e898772 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27911,6 +27911,9 @@ msgstr "" msgid "MemberInviteEmail|Invitation to join the %{project_or_group} %{project_or_group_name}" msgstr "" +msgid "MemberRole|%{requirement} has to be enabled in order to enable %{permission}." +msgstr "" + msgid "MemberRole|%{role} - custom" msgstr "" -- GitLab From 3c052adcb4edb772be8dd40add8f942954d9de45 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Mon, 5 Jun 2023 11:18:38 +0000 Subject: [PATCH 2/2] Fix the permission description --- ee/app/models/members/member_role.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 3edc3c22b70a78..05b454e71e1d31 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::GUEST }, admin_vulnerability: { - descripition: 'Permission to read vulnerability', + descripition: 'Permission to admin vulnerability', minimal_level: Gitlab::Access::GUEST, requirement: :read_vulnerability } -- GitLab