Merge branch 'ajk-group-member-policy-fix' into 'master'

Allow more actions on group members

See merge request gitlab-org/gitlab!50445

app/policies/group_member_policy.rb

@@ -10,7 +10,11 @@ class GroupMemberPolicy < BasePolicy
   with_score 0
   condition(:is_target_user) { @user && @subject.user_id == @user.id }
 
-  rule { anonymous }.prevent_all
+  rule { anonymous }.policy do
+    prevent :update_group_member
+    prevent :destroy_group_member
+  end
+
   rule { last_owner }.policy do
     prevent :update_group_member
     prevent :destroy_group_member

app/policies/group_policy.rb

@@ -66,7 +66,7 @@ class GroupPolicy < BasePolicy
   with_scope :subject
   condition(:has_project_with_service_desk_enabled) { @subject.has_project_with_service_desk_enabled? }
 
-  rule { design_management_enabled }.policy do
+  rule { can?(:read_group) & design_management_enabled }.policy do
     enable :read_design_activity
   end
 

changelogs/unreleased/ajk-group-member-policy-fix.yml

+---
+title: Allow more actions on group members
+merge_request: 50445
+author:
+type: fixed

spec/policies/group_member_policy_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe GroupMemberPolicy do
+  include DesignManagementTestHelpers
+
   let(:guest) { create(:user) }
   let(:owner) { create(:user) }
   let(:group) { create(:group, :private) }
@@ -28,22 +30,64 @@ RSpec.describe GroupMemberPolicy do
     permissions.each { |p| is_expected.not_to be_allowed(p) }
   end
 
-  context 'with guest user' do
-    let(:current_user) { guest }
+  context 'with anonymous user' do
+    let(:group) { create(:group, :public) }
+    let(:current_user) { nil }
+    let(:membership) { guest.members.first }
 
     it do
-      expect_disallowed(:member_related_permissions)
+      expect_disallowed(:read_design_activity, *member_related_permissions)
+      expect_allowed(:read_group)
+    end
+
+    context 'design management is enabled' do
+      before do
+        create(:project, :public, group: group) # Necessary to enable design management
+        enable_design_management
+      end
+
+      specify do
+        expect_allowed(:read_design_activity)
+      end
+    end
+
+    context 'for a private group' do
+      let(:group) { create(:group, :private) }
+
+      specify do
+        expect_disallowed(:read_group, :read_design_activity, *member_related_permissions)
+      end
+    end
+
+    context 'for an internal group' do
+      let(:group) { create(:group, :internal) }
+
+      specify do
+        expect_disallowed(:read_group, :read_design_activity, *member_related_permissions)
+      end
     end
   end
 
+  context 'with guest user, for own membership' do
+    let(:current_user) { guest }
+
+    specify { expect_disallowed(:update_group_member) }
+    specify { expect_allowed(:read_group, :destroy_group_member) }
+  end
+
+  context 'with guest user, for other membership' do
+    let(:current_user) { guest }
+    let(:membership) { owner.members.first }
+
+    specify { expect_disallowed(:destroy_group_member, :update_group_member) }
+    specify { expect_allowed(:read_group) }
+  end
+
   context 'with one owner' do
     let(:current_user) { owner }
 
-    it do
-      expect_disallowed(:destroy_group_member)
-      expect_disallowed(:update_group_member)
-      expect_allowed(:read_group)
-    end
+    specify { expect_disallowed(*member_related_permissions) }
+    specify { expect_allowed(:read_group) }
   end
 
   context 'with more than one owner' do
@@ -53,10 +97,7 @@ RSpec.describe GroupMemberPolicy do
       group.add_owner(create(:user))
     end
 
-    it do
-      expect_allowed(:destroy_group_member)
-      expect_allowed(:update_group_member)
-    end
+    specify { expect_allowed(*member_related_permissions) }
   end
 
   context 'with the group parent' do

spec/requests/api/graphql/group/group_members_spec.rb

@@ -10,15 +10,13 @@ RSpec.describe 'getting group members information' do
   let_it_be(:user_1) { create(:user, username: 'user') }
   let_it_be(:user_2) { create(:user, username: 'test') }
 
-  let(:member_data) { graphql_data['group']['groupMembers']['edges'] }
-
   before_all do
     [user_1, user_2].each { |user| parent_group.add_guest(user) }
   end
 
   context 'when the request is correct' do
     it_behaves_like 'a working graphql query' do
-      before_all do
+      before do
         fetch_members
       end
     end
@@ -80,12 +78,10 @@ RSpec.describe 'getting group members information' do
   end
 
   context 'when unauthenticated' do
-    it 'returns nothing' do
+    it 'returns visible members' do
       fetch_members(current_user: nil)
 
-      expect(graphql_errors).to be_nil
-      expect(response).to have_gitlab_http_status(:success)
-      expect(member_data).to be_empty
+      expect_array_response(user_1, user_2)
     end
   end
 
@@ -112,8 +108,8 @@ RSpec.describe 'getting group members information' do
 
   def expect_array_response(*items)
     expect(response).to have_gitlab_http_status(:success)
-    expect(member_data).to be_an Array
-    expect(member_data.map { |node| node["node"]["user"]["id"] })
-      .to match_array(items.map { |u| global_id_of(u) })
+    member_gids = graphql_data_at(:group, :group_members, :edges, :node, :user, :id)
+
+    expect(member_gids).to match_array(items.map { |u| global_id_of(u) })
   end
 end

spec/requests/api/graphql/project/project_members_spec.rb

@@ -11,15 +11,13 @@ RSpec.describe 'getting project members information' do
   let_it_be(:user_1) { create(:user, username: 'user') }
   let_it_be(:user_2) { create(:user, username: 'test') }
 
-  let(:member_data) { graphql_data['project']['projectMembers']['edges'] }
-
   before_all do
     [user_1, user_2].each { |user| parent_group.add_guest(user) }
   end
 
   context 'when the request is correct' do
     it_behaves_like 'a working graphql query' do
-      before_all do
+      before do
         fetch_members(project: parent_project)
       end
     end
@@ -114,8 +112,7 @@ RSpec.describe 'getting project members information' do
 
   def expect_array_response(*items)
     expect(response).to have_gitlab_http_status(:success)
-    expect(member_data).to be_an Array
-    expect(member_data.map { |node| node['node']['user']['id'] })
-      .to match_array(items.map { |u| global_id_of(u) })
+    member_gids = graphql_data_at(:project, :project_members, :edges, :node, :user, :id)
+    expect(member_gids).to match_array(items.map { |u| global_id_of(u) })
   end
 end