Enable reading custom roles for more users

- instance level roles can be read by all registered users
- group level roles can be read by all members of a group

ee/app/finders/member_roles/roles_finder.rb

@@ -71,6 +71,8 @@ def sort(items)
     def for_instance(items)
       return items if gitlab_com_subscription?
 
+      return MemberRole.none unless allowed_read_member_role?
+
       items.for_instance
     end
 
@@ -78,7 +80,7 @@ def allowed_namespace_ids(items)
       items.select { |item| allowed_read_member_role?(item.namespace, item) }.map(&:namespace_id)
     end
 
-    def allowed_read_member_role?(group, member_role = nil)
+    def allowed_read_member_role?(group = nil, member_role = nil)
       return Ability.allowed?(current_user, :read_member_role, group) if group
 
       return Ability.allowed?(current_user, :read_member_role, member_role) if member_role

ee/app/policies/ee/global_policy.rb

@@ -135,6 +135,9 @@ module GlobalPolicy
 
       rule { admin & custom_roles_allowed }.policy do
         enable :admin_member_role
+      end
+
+      rule { ~anonymous & custom_roles_allowed }.policy do
         enable :read_member_role
       end
 

ee/app/policies/ee/group_policy.rb

@@ -527,12 +527,14 @@ module GroupPolicy
         enable :manage_resource_access_tokens
       end
 
+      rule { custom_roles_allowed & guest }.policy do
+        enable :read_member_role
+      end
+
       rule { custom_roles_allowed & owner }.policy do
         enable :admin_member_role
       end
 
-      rule { can?(:admin_member_role) }.enable :read_member_role
-
       rule { custom_roles_allowed & can?(:admin_group_member) }.policy do
         enable :admin_member_role
       end

ee/app/policies/ee/project_policy.rb

@@ -391,7 +391,7 @@ module ProjectPolicy
         enable :admin_iteration
       end
 
-      rule { custom_roles_allowed & can?(:admin_project_member) }.policy do
+      rule { custom_roles_allowed & (guest | admin) }.policy do
         enable :read_member_role
       end
 

ee/app/policies/members/member_role_policy.rb

@@ -8,16 +8,11 @@ class MemberRolePolicy < BasePolicy
       ::License.feature_available?(:custom_roles)
     end
 
-    with_options scope: :user, score: 10
-    condition(:user_is_owner_of_at_least_one_group) do
-      GroupsFinder.new(@user, { min_access_level: Gitlab::Access::OWNER }).execute.any?
-    end
-
     condition(:is_instance_member_role, scope: :subject) do
       @subject.namespace.nil?
     end
 
-    rule { is_instance_member_role & user_is_owner_of_at_least_one_group & custom_roles_allowed }.policy do
+    rule { is_instance_member_role & custom_roles_allowed }.policy do
       enable :read_member_role
     end
 

ee/spec/policies/global_policy_spec.rb

@@ -278,28 +278,64 @@
     end
   end
 
-  describe 'custom roles administration' do
-    let(:permissions) { [:read_member_role, :admin_member_role] }
+  describe 'custom roles' do
+    describe 'admin_member_role' do
+      let(:permissions) { [:admin_member_role] }
 
-    context 'when custom_roles feature is enabled' do
-      before do
-        stub_licensed_features(custom_roles: true)
-      end
+      context 'when custom_roles feature is enabled' do
+        before do
+          stub_licensed_features(custom_roles: true)
+        end
 
-      it { is_expected.to be_disallowed(*permissions) }
+        it { is_expected.to be_disallowed(*permissions) }
 
-      context 'when admin mode enabled', :enable_admin_mode do
-        it { expect(described_class.new(admin, [user])).to be_allowed(*permissions) }
+        context 'when admin mode enabled', :enable_admin_mode do
+          let(:current_user) { admin }
+
+          it { is_expected.to be_allowed(*permissions) }
+        end
+
+        context 'when admin mode disabled' do
+          let(:current_user) { admin }
+
+          it { is_expected.to be_disallowed(*permissions) }
+        end
       end
 
-      context 'when admin mode disabled' do
-        it { expect(described_class.new(admin, [user])).to be_disallowed(*permissions) }
+      context 'when custom_roles feature is disabled' do
+        let(:current_user) { admin }
+
+        context 'when admin mode enabled', :enable_admin_mode do
+          it { is_expected.to be_disallowed(*permissions) }
+        end
       end
     end
 
-    context 'when custom_roles feature is disabled' do
-      context 'when admin mode enabled', :enable_admin_mode do
-        it { expect(described_class.new(admin, [user])).to be_disallowed(*permissions) }
+    describe 'read_member_role' do
+      let(:permissions) { [:read_member_role] }
+
+      context 'when custom_roles feature is enabled' do
+        before do
+          stub_licensed_features(custom_roles: true)
+        end
+
+        context 'for anynomous user' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(*permissions) }
+        end
+
+        context 'for registeres user' do
+          let(:current_user) { user }
+
+          it { is_expected.to be_allowed(*permissions) }
+        end
+      end
+
+      context 'when custom_roles feature is disabled' do
+        context 'when admin mode enabled', :enable_admin_mode do
+          it { expect(described_class.new(admin, [user])).to be_disallowed(*permissions) }
+        end
       end
     end
   end
@@ -576,9 +612,9 @@
         stub_licensed_features(code_suggestions: code_suggestions_licensed)
         code_suggestions_service_data = instance_double(CloudConnector::BaseAvailableServiceData)
         allow(CloudConnector::AvailableServices).to receive(:find_by_name).with(:code_suggestions)
-          .and_return(code_suggestions_service_data)
+                                                                          .and_return(code_suggestions_service_data)
         allow(code_suggestions_service_data).to receive(:allowed_for?).with(current_user)
-          .and_return(duo_pro_seat_assigned)
+                                                                      .and_return(duo_pro_seat_assigned)
       end
 
       it { is_expected.to code_suggestions_enabled_for_user }
@@ -608,10 +644,10 @@
           allow(current_user).to receive(:any_group_with_ai_chat_available?).and_return(group_with_ai_membership)
           duo_chat_service_data = instance_double(CloudConnector::SelfManaged::AvailableServiceData)
           allow(CloudConnector::AvailableServices).to receive(:find_by_name).with(:duo_chat)
-            .and_return(duo_chat_service_data)
+                                                                            .and_return(duo_chat_service_data)
           allow(duo_chat_service_data).to receive(:allowed_for?).with(current_user).and_return(duo_pro_seat_assigned)
           allow(current_user).to receive(:belongs_to_group_requires_licensed_seat_for_chat?)
-            .and_return(requires_licensed_seat)
+                                   .and_return(requires_licensed_seat)
         end
 
         it { is_expected.to duo_chat_enabled_for_user }
@@ -648,7 +684,7 @@
           duo_chat_service_data = CloudConnector::SelfManaged::AvailableServiceData.new(:duo_chat,
             duo_chat_cut_off_date, %w[duo_pro])
           allow(CloudConnector::AvailableServices).to receive(:find_by_name)
-            .with(:duo_chat).and_return(duo_chat_service_data)
+                                                        .with(:duo_chat).and_return(duo_chat_service_data)
           allow(duo_chat_service_data).to receive(:allowed_for?).with(current_user).and_return(duo_pro_seat_assigned)
         end
 

ee/spec/policies/group_policy_spec.rb

@@ -2399,52 +2399,104 @@ def set_access_level(access_level)
       end
     end
 
-    describe 'custom roles administration' do
-      using RSpec::Parameterized::TableSyntax
+    describe 'custom roles' do
+      describe ':admin_member_role' do
+        using RSpec::Parameterized::TableSyntax
 
-      let(:permissions) { [:read_member_role, :admin_member_role] }
+        let(:permissions) { [:admin_member_role] }
 
-      where(:role, :allowed) do
-        :guest      | false
-        :reporter   | false
-        :developer  | false
-        :maintainer | false
-        :auditor    | false
-        :owner      | true
-        :admin      | true
-      end
-
-      with_them do
-        let(:current_user) { public_send(role) }
-
-        before do
-          enable_admin_mode!(current_user) if role == :admin
+        where(:role, :allowed) do
+          :guest      | false
+          :reporter   | false
+          :developer  | false
+          :maintainer | false
+          :auditor    | false
+          :owner      | true
+          :admin      | true
         end
 
-        context 'when custom_roles feature is enabled' do
+        with_them do
+          let(:current_user) { public_send(role) }
+
           before do
-            stub_licensed_features(custom_roles: true)
+            enable_admin_mode!(current_user) if role == :admin
           end
 
-          it { is_expected.to(allowed ? be_allowed(*permissions) : be_disallowed(*permissions)) }
-
-          context 'when memberships are locked to LDAP' do
+          context 'when custom_roles feature is enabled' do
             before do
-              allow(group).to receive(:ldap_synced?).and_return(true)
-              stub_application_setting(allow_group_owners_to_manage_ldap: true)
-              stub_application_setting(lock_memberships_to_ldap: true)
+              stub_licensed_features(custom_roles: true)
             end
 
             it { is_expected.to(allowed ? be_allowed(*permissions) : be_disallowed(*permissions)) }
+
+            context 'when memberships are locked to LDAP' do
+              before do
+                allow(group).to receive(:ldap_synced?).and_return(true)
+                stub_application_setting(allow_group_owners_to_manage_ldap: true)
+                stub_application_setting(lock_memberships_to_ldap: true)
+              end
+
+              it { is_expected.to(allowed ? be_allowed(*permissions) : be_disallowed(*permissions)) }
+            end
           end
+
+          context 'when custom_roles feature is disabled' do
+            before do
+              stub_licensed_features(custom_roles: false)
+            end
+
+            it { is_expected.to be_disallowed(*permissions) }
+          end
+        end
+      end
+
+      describe ':read_member_role' do
+        using RSpec::Parameterized::TableSyntax
+
+        let(:permissions) { [:read_member_role] }
+
+        where(:role, :allowed) do
+          :guest      | true
+          :reporter   | true
+          :developer  | true
+          :maintainer | true
+          :auditor    | false
+          :owner      | true
+          :admin      | true
         end
 
-        context 'when custom_roles feature is disabled' do
+        with_them do
+          let(:current_user) { public_send(role) }
+
           before do
-            stub_licensed_features(custom_roles: false)
+            enable_admin_mode!(current_user) if role == :admin
+          end
+
+          context 'when custom_roles feature is enabled' do
+            before do
+              stub_licensed_features(custom_roles: true)
+            end
+
+            it { is_expected.to(allowed ? be_allowed(*permissions) : be_disallowed(*permissions)) }
+
+            context 'when memberships are locked to LDAP' do
+              before do
+                allow(group).to receive(:ldap_synced?).and_return(true)
+                stub_application_setting(allow_group_owners_to_manage_ldap: true)
+                stub_application_setting(lock_memberships_to_ldap: true)
+              end
+
+              it { is_expected.to(allowed ? be_allowed(*permissions) : be_disallowed(*permissions)) }
+            end
           end
 
-          it { is_expected.to be_disallowed(*permissions) }
+          context 'when custom_roles feature is disabled' do
+            before do
+              stub_licensed_features(custom_roles: false)
+            end
+
+            it { is_expected.to be_disallowed(*permissions) }
+          end
         end
       end
     end
@@ -3467,7 +3519,7 @@ def create_member_role(member, abilities = member_role_abilities)
 
     context 'for a member role with admin_group_member true' do
       let(:member_role_abilities) { { admin_group_member: true } }
-      let(:allowed_abilities) { [:admin_group_member, :admin_member_role, :read_member_role] }
+      let(:allowed_abilities) { [:admin_group_member, :admin_member_role] }
 
       it_behaves_like 'custom roles abilities'
 

ee/spec/policies/members/member_role_policy_spec.rb

@@ -3,124 +3,144 @@
 require 'spec_helper'
 
 RSpec.describe Members::MemberRolePolicy, feature_category: :system_access do
+  include AdminModeHelper
+
   let_it_be_with_reload(:user) { create(:user) }
   let_it_be(:group) { create(:group) }
+  let_it_be(:instance_member_role) { create(:member_role, :instance) }
+  let_it_be(:group_member_role) { create(:member_role, namespace: group) }
 
-  let(:permissions) { [:admin_member_role, :read_member_role] }
+  let(:member_role) { group_member_role }
 
   subject(:policy) { described_class.new(user, member_role) }
 
-  describe 'rules' do
-    context 'for group member roles' do
-      let_it_be(:member_role) { create(:member_role, namespace: group) }
+  shared_examples 'correct member role permissions' do
+    let(:special_roles) { [:admin, :non_member, :auditor] }
 
-      context 'without the custom roles feature' do
-        before do
-          stub_licensed_features(custom_roles: false)
-        end
-
-        before_all do
-          group.add_owner(user)
-        end
+    before do
+      group.public_send(:"add_#{role}", user) unless special_roles.include?(role)
 
-        it { is_expected.to be_disallowed(*permissions) }
+      if role == :admin
+        user.update!(admin: true)
+        enable_admin_mode!(user)
+      elsif role == :auditor
+        user.update!(auditor: true)
       end
+    end
 
-      context 'with the custom roles feature' do
-        before do
-          stub_licensed_features(custom_roles: true)
-        end
+    context 'when custom_roles feature is enabled' do
+      before do
+        stub_licensed_features(custom_roles: true)
+      end
 
-        context 'when non member' do
-          it { is_expected.to be_disallowed(*permissions) }
-        end
+      context 'for instance-level custom role' do
+        let(:member_role) { instance_member_role }
 
-        context 'when maintainer' do
-          before_all do
-            group.add_maintainer(user)
+        context 'for logged-in users' do
+          it 'returns correct value' do
+            is_expected.to(instance_role_allowed ? be_allowed(permission) : be_disallowed(permission))
           end
-
-          it { is_expected.to be_disallowed(*permissions) }
         end
 
-        context 'when owner' do
-          before_all do
-            group.add_owner(user)
+        context 'when memberships are locked to LDAP' do
+          before do
+            allow(group).to receive(:ldap_synced?).and_return(true)
+            stub_application_setting(allow_group_owners_to_manage_ldap: true)
+            stub_application_setting(lock_memberships_to_ldap: true)
           end
 
-          it { is_expected.to be_allowed(*permissions) }
+          it 'returns correct value' do
+            is_expected.to(instance_role_allowed ? be_allowed(permission) : be_disallowed(permission))
+          end
         end
+      end
 
-        context 'when admin' do
-          before_all do
-            user.update!(admin: true)
-          end
+      context 'for group-level custom role' do
+        let(:member_role) { group_member_role }
 
-          context 'when admin', :enable_admin_mode do
-            before_all do
-              user.update!(admin: true)
-            end
+        it { is_expected.to(group_role_allowed ? be_allowed(permission) : be_disallowed(permission)) }
 
-            it { is_expected.to be_allowed(*permissions) }
+        context 'when memberships are locked to LDAP' do
+          before do
+            allow(group).to receive(:ldap_synced?).and_return(true)
+            stub_application_setting(allow_group_owners_to_manage_ldap: true)
+            stub_application_setting(lock_memberships_to_ldap: true)
           end
+
+          it { is_expected.to(group_role_allowed ? be_allowed(permission) : be_disallowed(permission)) }
         end
       end
     end
 
-    context 'for instance level member roles' do
-      let_it_be(:member_role) { create(:member_role, :instance) }
+    context 'when custom_roles feature is disabled' do
+      before do
+        stub_licensed_features(custom_roles: false)
+      end
 
-      context 'without the custom roles feature', :enable_admin_mode do
-        before do
-          stub_licensed_features(custom_roles: false)
-        end
+      it { is_expected.to be_disallowed(permission) }
+    end
+  end
 
-        before_all do
-          user.update!(admin: true)
-        end
+  describe 'rules' do
+    describe ':read_member_role' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:permission) { :read_member_role }
+
+      where(:role, :instance_role_allowed, :group_role_allowed) do
+        :non_member | true  | false
+        :guest      | true  | true
+        :reporter   | true  | true
+        :developer  | true  | true
+        :maintainer | true  | true
+        :auditor    | true  | false
+        :owner      | true  | true
+        :admin      | true  | true
+      end
 
-        it { is_expected.to be_disallowed(*permissions) }
+      with_them do
+        it_behaves_like 'correct member role permissions'
       end
 
-      context 'with the custom roles feature' do
-        before do
-          stub_licensed_features(custom_roles: true)
-        end
+      context 'for anonymous user' do
+        subject(:policy) { described_class.new(nil, member_role) }
 
-        context 'when non member' do
-          it { is_expected.to be_disallowed(*permissions) }
-        end
+        context 'for group roles' do
+          let(:member_role) { group_member_role }
 
-        context 'when group maintainer' do
-          before_all do
-            group.add_maintainer(user)
+          it 'returns false' do
+            is_expected.to be_disallowed(:read_member_role)
           end
-
-          it { is_expected.to be_disallowed(*permissions) }
         end
 
-        context 'when group owner' do
-          before_all do
-            group.add_owner(user)
-          end
+        context 'for instnace roles' do
+          let(:member_role) { instance_member_role }
 
-          it { is_expected.to be_allowed(:read_member_role) }
-          it { is_expected.to be_disallowed(:admin_member_role) }
+          it 'returns false' do
+            is_expected.to be_disallowed(:read_member_role)
+          end
         end
+      end
+    end
 
-        context 'when instance admin' do
-          before_all do
-            user.update!(admin: true)
-          end
+    describe ':admin_member_role' do
+      using RSpec::Parameterized::TableSyntax
 
-          context 'when admin mode is enabled', :enable_admin_mode do
-            it { is_expected.to be_allowed(*permissions) }
-          end
+      let(:permission) { :admin_member_role }
 
-          context 'when admin mode is disabled' do
-            it { is_expected.to be_disallowed(*permissions) }
-          end
-        end
+      where(:role, :instance_role_allowed, :group_role_allowed) do
+        :non_member | false | false
+        :guest      | false | false
+        :reporter   | false | false
+        :developer  | false | false
+        :maintainer | false | false
+        :auditor    | false | false
+        :owner      | false | true
+        :admin      | true | true
+      end
+
+      with_them do
+        it_behaves_like 'correct member role permissions'
       end
     end
   end

ee/spec/policies/project_policy_spec.rb

@@ -2622,9 +2622,9 @@ def expect_private_project_permissions_as_if_non_member
     let(:permission) { :read_member_role }
 
     where(:role, :allowed) do
-      :guest      | false
-      :reporter   | false
-      :developer  | false
+      :guest      | true
+      :reporter   | true
+      :developer  | true
       :maintainer | true
       :auditor    | false
       :owner      | true

ee/spec/presenters/member_presenter_spec.rb

@@ -99,9 +99,9 @@
       stub_licensed_features(custom_roles: true)
     end
 
-    shared_examples 'returning all custom roles for subgroup' do
-      it 'returns only roles with higher base_access_level than user highest membership in the hierarchy' do
-        expect(described_class.new(member_subgroup, current_user: user).valid_member_roles).to match_array(
+    shared_examples 'returning all custom roles' do
+      it 'returns all roles for the root group' do
+        expect(presenter.valid_member_roles).to match_array(
           [
             {
               base_access_level: Gitlab::Access::REPORTER,
@@ -110,19 +110,21 @@
               description: 'My custom role',
               occupies_seat: true,
               permissions: permissions
+            },
+            {
+              base_access_level: Gitlab::Access::GUEST,
+              member_role_id: member_role_guest.id,
+              name: 'guest plus',
+              description: nil,
+              occupies_seat: false,
+              permissions: permissions
             }
           ]
         )
       end
-    end
 
-    context 'when the user has permissions to manage group roles for root group' do
-      before_all do
-        root_group.add_owner(user)
-      end
-
-      it 'returns all roles for the root group' do
-        expect(presenter.valid_member_roles).to match_array(
+      it 'returns only roles with higher base_access_level than user highest membership in the hierarchy' do
+        expect(described_class.new(member_subgroup, current_user: user).valid_member_roles).to match_array(
           [
             {
               base_access_level: Gitlab::Access::REPORTER,
@@ -131,62 +133,66 @@
               description: 'My custom role',
               occupies_seat: true,
               permissions: permissions
-            },
-            {
-              base_access_level: Gitlab::Access::GUEST,
-              member_role_id: member_role_guest.id,
-              name: 'guest plus',
-              description: nil,
-              occupies_seat: false,
-              permissions: permissions
             }
           ]
         )
       end
-
-      it_behaves_like 'returning all custom roles for subgroup'
     end
 
-    context 'when the user has permissions to manage group roles for subgroup group' do
+    context 'when the user is a member of root group' do
       before_all do
-        subgroup.add_owner(user)
+        root_group.add_guest(user)
       end
 
-      it 'does not return any roles for root group' do
-        expect(presenter.valid_member_roles).to be_empty
+      it_behaves_like 'returning all custom roles'
+    end
+
+    context 'when the user is a member of subgroup group' do
+      before_all do
+        subgroup.add_guest(user)
       end
 
-      it_behaves_like 'returning all custom roles for subgroup'
+      it_behaves_like 'returning all custom roles'
     end
 
-    context 'when the user has admin permissions', :enable_admin_mode do
-      let(:current_user) { admin }
-
+    context 'when on self-managed', :enable_admin_mode do
       before do
         stub_saas_features(gitlab_com_subscriptions: false)
       end
 
-      it 'returns instance-level roles' do
-        expect(presenter.valid_member_roles).to match_array(
-          [
-            {
-              base_access_level: Gitlab::Access::GUEST,
-              member_role_id: member_role_guest_instance.id,
-              name: 'guest plus (instance-level)',
-              description: nil,
-              occupies_seat: false,
-              permissions: permissions
-            },
-            {
-              base_access_level: Gitlab::Access::REPORTER,
-              member_role_id: member_role_reporter_instance.id,
-              name: 'reporter plus (instance-level)',
-              description: nil,
-              occupies_seat: true,
-              permissions: permissions
-            }
-          ]
-        )
+      context 'when getting the roles for a registered user' do
+        let(:current_user) { admin }
+
+        it 'returns instance-level roles' do
+          expect(presenter.valid_member_roles).to match_array(
+            [
+              {
+                base_access_level: Gitlab::Access::GUEST,
+                member_role_id: member_role_guest_instance.id,
+                name: 'guest plus (instance-level)',
+                description: nil,
+                occupies_seat: false,
+                permissions: permissions
+              },
+              {
+                base_access_level: Gitlab::Access::REPORTER,
+                member_role_id: member_role_reporter_instance.id,
+                name: 'reporter plus (instance-level)',
+                description: nil,
+                occupies_seat: true,
+                permissions: permissions
+              }
+            ]
+          )
+        end
+      end
+
+      context 'when getting the roles for a anonymous user' do
+        let(:current_user) { nil }
+
+        it 'does not return any custom roles' do
+          expect(presenter.valid_member_roles).to be_empty
+        end
       end
     end
   end