Allow group reporters to manage group labels

Previously, only group masters could do this. However, project reporters can
manage project labels, so there doesn't seem to be any need to restrict group
labels further.

Also, save a query or two by getting a single GroupMember object to find out if
the user is a master or not.

app/models/group.rb

@@ -222,6 +222,16 @@ class Group < Namespace
     User.where(id: members_with_parents.select(:user_id))
   end
+  def max_member_access_for_user(user)
+    return GroupMember::OWNER if user.admin?
+
+    members_with_parents.
+      where(user_id: user).
+      reorder(access_level: :desc).
+      first&.
+      access_level || GroupMember::NO_ACCESS
+  end
+
   def mattermost_team_params
     max_length = 59

app/models/member.rb

@@ -200,6 +200,10 @@ class Member < ActiveRecord::Base
     source_type
   end
+  def access_field
+    access_level
+  end
+
   def invite?
     self.invite_token.present?
   end

app/models/members/group_member.rb

@@ -25,10 +25,6 @@ class GroupMember < Member
     source
   end
-  def access_field
-    access_level
-  end
-
   # Because source_type is `Namespace`...
   def real_source_type
     'Group'

app/models/members/project_member.rb

@@ -79,10 +79,6 @@ class ProjectMember < Member
     end
   end
-  def access_field
-    access_level
-  end
-
   def project
     source
   end

app/policies/group_policy.rb

@@ -4,22 +4,25 @@ class GroupPolicy < BasePolicy
     return unless @user
     globally_viewable = @subject.public? || (@subject.internal? && !@user.external?)
-    member = @subject.users_with_parents.include?(@user)
-    owner = @user.admin? || @subject.has_owner?(@user)
-    master = owner || @subject.has_master?(@user)
+    access_level = @subject.max_member_access_for_user(@user)
+    owner = access_level >= GroupMember::OWNER
+    master = access_level >= GroupMember::MASTER
+    reporter = access_level >= GroupMember::REPORTER
     can_read = false
     can_read ||= globally_viewable
-    can_read ||= member
-    can_read ||= @user.admin?
+    can_read ||= access_level >= GroupMember::GUEST
     can_read ||= GroupProjectsFinder.new(group: @subject, current_user: @user).execute.any?
     can! :read_group if can_read
+    if reporter
+      can! :admin_label
+    end
+
     # Only group masters and group owners can create new projects
     if master
       can! :create_projects
       can! :admin_milestones
-      can! :admin_label
     end
     # Only group owner and administrators can admin group
@@ -31,7 +34,7 @@ class GroupPolicy < BasePolicy
       can! :create_subgroup if @user.can_create_group
     end
-    if globally_viewable && @subject.request_access_enabled && !member
+    if globally_viewable && @subject.request_access_enabled && access_level == GroupMember::NO_ACCESS
       can! :request_access
     end
   end

changelogs/unreleased/33154-permissions-for-project-labels-and-group-labels.yml

+---
+title: Allow group reporters to manage group labels
+merge_request:
+author:

spec/policies/group_policy_spec.rb

@@ -9,11 +9,12 @@ describe GroupPolicy, models: true do
   let(:admin) { create(:admin) }
   let(:group) { create(:group) }
+  let(:reporter_permissions) { [:admin_label] }
+
   let(:master_permissions) do
     [
       :create_projects,
-      :admin_milestones,
-      :admin_label
+      :admin_milestones
     ]
   end
@@ -42,6 +43,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.not_to include(*reporter_permissions)
       is_expected.not_to include(*master_permissions)
       is_expected.not_to include(*owner_permissions)
     end
@@ -52,6 +54,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.not_to include(*reporter_permissions)
       is_expected.not_to include(*master_permissions)
       is_expected.not_to include(*owner_permissions)
     end
@@ -62,6 +65,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.to include(*reporter_permissions)
       is_expected.not_to include(*master_permissions)
       is_expected.not_to include(*owner_permissions)
     end
@@ -72,6 +76,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.to include(*reporter_permissions)
       is_expected.not_to include(*master_permissions)
       is_expected.not_to include(*owner_permissions)
     end
@@ -82,6 +87,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.to include(*reporter_permissions)
       is_expected.to include(*master_permissions)
       is_expected.not_to include(*owner_permissions)
     end
@@ -92,6 +98,7 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.to include(*reporter_permissions)
       is_expected.to include(*master_permissions)
       is_expected.to include(*owner_permissions)
     end
@@ -102,14 +109,27 @@ describe GroupPolicy, models: true do
     it do
       is_expected.to include(:read_group)
+      is_expected.to include(*reporter_permissions)
       is_expected.to include(*master_permissions)
       is_expected.to include(*owner_permissions)
     end
   end
-  describe 'private nested group inherit permissions', :nested_groups do
+  describe 'private nested group use the highest access level from the group and inherited permissions', :nested_groups do
     let(:nested_group) { create(:group, :private, parent: group) }
+    before do
+      nested_group.add_guest(guest)
+      nested_group.add_guest(reporter)
+      nested_group.add_guest(developer)
+      nested_group.add_guest(master)
+
+      group.owners.destroy_all
+
+      group.add_guest(owner)
+      nested_group.add_owner(owner)
+    end
+
     subject { described_class.abilities(current_user, nested_group).to_set }
     context 'with no user' do
@@ -117,6 +137,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.not_to include(:read_group)
+        is_expected.not_to include(*reporter_permissions)
         is_expected.not_to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end
@@ -127,6 +148,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.to include(:read_group)
+        is_expected.not_to include(*reporter_permissions)
         is_expected.not_to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end
@@ -137,6 +159,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.to include(:read_group)
+        is_expected.to include(*reporter_permissions)
         is_expected.not_to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end
@@ -147,6 +170,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.to include(:read_group)
+        is_expected.to include(*reporter_permissions)
         is_expected.not_to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end
@@ -157,6 +181,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.to include(:read_group)
+        is_expected.to include(*reporter_permissions)
         is_expected.to include(*master_permissions)
         is_expected.not_to include(*owner_permissions)
       end
@@ -167,6 +192,7 @@ describe GroupPolicy, models: true do
       it do
         is_expected.to include(:read_group)
+        is_expected.to include(*reporter_permissions)
         is_expected.to include(*master_permissions)
         is_expected.to include(*owner_permissions)
       end