Merge branch '33154-permissions-for-project-labels-and-group-labels' into 'master'

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