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.

7 files changed, 57 insertions, 18 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index be944da5a67..5bb2cdc5eff 100644
--- a/app/models/group.rb
+++ b/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
 
diff --git a/app/models/member.rb b/app/models/member.rb
index 7228e82e978..29f9d61e870 100644
--- a/app/models/member.rb
+++ b/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
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 28e10bc6172..47040f95533 100644
--- a/app/models/members/group_member.rb
+++ b/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'
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index b3a91feb091..c0e17f4bfc8 100644
--- a/app/models/members/project_member.rb
+++ b/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
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 87398303c68..fb07298c6c2 100644
--- a/app/policies/group_policy.rb
+++ b/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
diff --git a/changelogs/unreleased/33154-permissions-for-project-labels-and-group-labels.yml b/changelogs/unreleased/33154-permissions-for-project-labels-and-group-labels.yml
new file mode 100644
index 00000000000..3b98525167d
--- /dev/null
+++ b/changelogs/unreleased/33154-permissions-for-project-labels-and-group-labels.yml
@@ -0,0 +1,4 @@
+---
+title: Allow group reporters to manage group labels
+merge_request:
+author:
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 4c37a553227..a8331ceb5ff 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/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