From 12f02dccb9a9ee828271b1cf71f8a9a98f97fc46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Thu, 11 May 2023 12:05:24 +0200 Subject: [PATCH] Revert "Merge branch '388156-read-vuln-custom-role' into 'master'" Changelog: other --- .../security_training_urls_resolver.rb | 2 +- ee/app/models/ee/user.rb | 5 - ee/app/models/members/member_role.rb | 23 +-- ...user_member_roles_in_projects_preloader.rb | 19 +- ee/app/policies/ee/project_policy.rb | 16 -- .../instance_security_dashboard_policy.rb | 1 - .../custom_roles_vulnerability.yml | 8 - ee/lib/api/member_roles.rb | 4 +- ee/spec/factories/member_roles.rb | 1 - .../security_training_urls_resolver_spec.rb | 4 - ...rability_severities_count_resolver_spec.rb | 2 +- .../lib/ee/api/entities/member_role_spec.rb | 1 - ee/spec/models/ee/user_spec.rb | 55 +----- ee/spec/models/members/member_role_spec.rb | 26 --- ...member_roles_in_projects_preloader_spec.rb | 174 ++++++++---------- ee/spec/policies/project_policy_spec.rb | 157 +++------------- ee/spec/requests/api/member_roles_spec.rb | 12 +- locale/gitlab.pot | 3 - 18 files changed, 117 insertions(+), 396 deletions(-) delete mode 100644 ee/config/feature_flags/development/custom_roles_vulnerability.yml diff --git a/ee/app/graphql/resolvers/security_training_urls_resolver.rb b/ee/app/graphql/resolvers/security_training_urls_resolver.rb index f4141d4f312100..30a7438fed2c26 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 :read_security_resource + authorize :access_security_and_compliance authorizes_object! argument :identifier_external_ids, diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 846fe3921c6e3c..2b34aa5e5bda8a 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -508,11 +508,6 @@ 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 cf4a99e6f4cea3..4ac27dd3c5364a 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -5,12 +5,7 @@ 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: - { description: 'Permission to read code', minimal_level: Gitlab::Access::GUEST }, - read_vulnerability: - { descripition: 'Permission to read vulnerability', minimal_level: Gitlab::Access::GUEST } - }.freeze + ALL_CUSTOMIZABLE_PERMISSIONS = { read_code: 'Permission to read code' }.freeze CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code].freeze NON_PERMISSION_COLUMNS = [:id, :namespace_id, :created_at, :updated_at, :base_access_level].freeze @@ -23,9 +18,6 @@ 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, if: ->(member_role) do - Feature.enabled?(:custom_roles_vulnerability, member_role.namespace&.root_ancestor) - end validates_associated :members @@ -71,19 +63,6 @@ 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| - min_level = params[:minimal_level] - next unless self[permission] - next if base_access_level >= min_level - - 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 - 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 fd8681383cf0a3..96c81198ce0030 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,31 +26,29 @@ def execute value_list = Arel::Nodes::ValuesList.new(sql_values_array) sql = <<~SQL - 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), + SELECT project_ids.project_id, custom_permissions.read_code FROM (#{value_list.to_sql}) AS project_ids (project_id, namespace_ids), LATERAL ( ( - #{Member.select('read_code, read_vulnerability') + #{Member.select('read_code') .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 or member_roles.read_vulnerability = true') + .where(member_roles: { read_code: true }) .limit(1).to_sql} ) UNION ALL ( - #{Member.select('read_code, read_vulnerability') + #{Member.select('read_code') .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 or member_roles.read_vulnerability = true') + .where(member_roles: { read_code: true }) .limit(1).to_sql} ) UNION ALL ( - SELECT false AS read_code, false AS read_vulnerability + SELECT false AS read_code ) 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| @@ -60,11 +58,6 @@ 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 681e0401edc166..6208ae4415589f 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -207,10 +207,6 @@ 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) @@ -218,13 +214,6 @@ 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? @@ -554,11 +543,6 @@ 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 }.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/app/policies/instance_security_dashboard_policy.rb b/ee/app/policies/instance_security_dashboard_policy.rb index 22e950fbafab6f..89dda82a86287f 100644 --- a/ee/app/policies/instance_security_dashboard_policy.rb +++ b/ee/app/policies/instance_security_dashboard_policy.rb @@ -14,7 +14,6 @@ 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/config/feature_flags/development/custom_roles_vulnerability.yml b/ee/config/feature_flags/development/custom_roles_vulnerability.yml deleted file mode 100644 index c6a3317a26e4f6..00000000000000 --- a/ee/config/feature_flags/development/custom_roles_vulnerability.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: custom_roles_vulnerability -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114734 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/409387 -milestone: '16.0' -type: development -group: group::authentication and authorization -default_enabled: false diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb index 512dd61f9c652c..e0234ad599e60a 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_params| - optional permission_name.to_s, type: Boolean, desc: permission_params[:description], default: false + ::MemberRole::ALL_CUSTOMIZABLE_PERMISSIONS.each do |permission_name, permission_description| + optional permission_name.to_s, type: Boolean, desc: permission_description end end diff --git a/ee/spec/factories/member_roles.rb b/ee/spec/factories/member_roles.rb index 1c6f9806ee902c..503438d2521193 100644 --- a/ee/spec/factories/member_roles.rb +++ b/ee/spec/factories/member_roles.rb @@ -6,7 +6,6 @@ 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/security_training_urls_resolver_spec.rb b/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb index 31454b579635e2..6e8866429f1bdd 100644 --- a/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/security_training_urls_resolver_spec.rb @@ -6,10 +6,6 @@ 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) } 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 18cfe5f97c9552..01ee1440cb6360 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, :security_and_compliance_enabled) } + let_it_be(:project) { create(:project) } 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 87bae88d765950..9eba9e7ce204a5 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -16,7 +16,6 @@ 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 2066014b212944..df1c0f655ffc4e 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" @@ -2128,53 +2124,6 @@ end end - 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) - 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 - - 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 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 b8732ee895d7cf..07149eac613092 100644 --- a/ee/spec/models/members/member_role_spec.rb +++ b/ee/spec/models/members/member_role_spec.rb @@ -100,32 +100,6 @@ expect(member_role.errors[:namespace]).to include(s_("MemberRole|can't be changed")) end end - - context 'when base_access_level is too low' do - 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 - - 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 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..e9b5a44071ee4b 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,130 +11,108 @@ subject(:result) { described_class.new(projects: project_list, user: user).execute } - 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 + 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) - expect(result).to eq({}) - end + expect(result).to eq({}) end + end - context 'when custom_roles license is enabled on project root ancestor' do - before do - stub_licensed_features(custom_roles: true) + 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 + let_it_be(:member_role) do + create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) end - 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 + context 'when custom role has read_code: true' do + 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] }) end 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] }) - 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 the ability' do - expect(result).to eq({ project.id => [ability] }) - end + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_code] }) end end end + end - 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 - 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 - it 'returns the project_id with a value array that includes the ability' do - expect(result).to eq({ project.id => [ability] }) - end + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_code] }) end + 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) } + 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 ability' 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 + 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) - expect(result[project.id]).to match_array([ability]) - end + expect(result[project.id]).to match_array([:read_code]) 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 no custom role' do + let_it_be(:project) { create(:project, :private, :in_group) } - it 'returns project id with empty value array' do - expect(result).to eq(project.id => []) - end + it 'returns project id with empty value array' do + expect(result).to eq(project.id => []) 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, namespace: project.group).tap do |record| - record[ability] = false - record.save! - record.members << project_without_custom_permission_member - end - - expect(result).to eq(project.id => []) - 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 - - expect(result).to eq({ project.id => [] }) - 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 => [] }) end end end - - it_behaves_like 'custom roles', :read_code - it_behaves_like 'custom roles', :read_vulnerability end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 9b6d3f83496fe7..d20d2932177aae 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2425,51 +2425,27 @@ def expect_private_project_permissions_as_if_non_member end context 'custom role' do - 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(:current_user) { create(:user) } let_it_be(:project) { private_project_in_group } - let_it_be(:group_member_guest) do + let_it_be(:group_member) do create( :group_member, - user: guest, + user: current_user, source: project.group, access_level: Gitlab::Access::GUEST ) end - 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 + let_it_be(:project_member) do create( :project_member, :guest, - user: guest, + user: current_user, 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, @@ -2488,147 +2464,62 @@ def custom_role_allowed?(user, ability) ) 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 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 + context 'custom role allows read code' do + before do + member_role_read_code_true.members << group_member end - 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 + it { is_expected.to be_allowed(:read_code) } end - 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 + context 'custom role disallows read code' do + before do + member_role_read_code_false.members << group_member end - 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 + it { is_expected.to be_disallowed(:read_code) } end end context 'custom role on project membership' do - 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 - - context 'when custom_roles_vulnerability FF is enabled' do - before do - stub_feature_flags(custom_roles_vulnerability: [project.group]) - end - - 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 + context 'custom role allows read code' do + before do + member_role_read_code_true.members << project_member end - context 'when custom_roles_vulnerability FF is disabled' 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 + it { is_expected.to be_allowed(:read_code) } end - 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 + context 'custom role disallows read code' do + before do + member_role_read_code_false.members << project_member end - 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 + it { is_expected.to be_disallowed(:read_code) } 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_guest - member_role_read_code_false.members << group_member_guest + member_role_read_code_true.members << project_member + member_role_read_code_false.members << group_member 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 '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 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_guest + member_role_read_code_true.members << project_member 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 bb255c8a8eb640..5cff4002c3b8b5 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -22,8 +22,7 @@ :member_role, namespace: group_with_member_roles, base_access_level: ::Gitlab::Access::REPORTER, - read_code: false, - read_vulnerability: true + read_code: false ) end @@ -32,8 +31,7 @@ :member_role, namespace: group_with_member_roles, base_access_level: ::Gitlab::Access::REPORTER, - read_code: true, - read_vulnerability: false + read_code: true ) end @@ -95,14 +93,12 @@ "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 } ] @@ -133,7 +129,7 @@ describe "POST /groups/:id/member_roles" do let_it_be(:params) do - { base_access_level: ::Gitlab::Access::GUEST, read_code: true } + { base_access_level: ::Gitlab::Access::MAINTAINER, read_code: true } end subject { post api("/groups/#{group_id}/member_roles", current_user), params: params } @@ -175,7 +171,7 @@ aggregate_failures "testing response" do expect(response).to have_gitlab_http_status(:created) - expect(json_response['base_access_level']).to eq(::Gitlab::Access::GUEST) + expect(json_response['base_access_level']).to eq(::Gitlab::Access::MAINTAINER) expect(json_response['read_code']).to eq(true) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 07c53fe6f03933..a365dcdea61211 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27554,9 +27554,6 @@ 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 must be %{min_access_level}." -msgstr "" - msgid "MemberRole|must be top-level namespace" msgstr "" -- GitLab