Allow more actions on group members

Specifically, we want to allow `:read_group`, without which
certain policy checks will fail.

This was implemented in support of
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40088, but can
be extracted in advance, and fixes a distinct bug.

The basic assumption of policies is that if `A` delegates to `B` and
user `U` is allowed to perform action `x` on `B`, then that user is
allowed to perform that action on `A` (i.e. policies are transitive).

The `prevent_all` call in this policy breaks that assumption, which is
relied on by GraphQL authorization to hold. Without this fix, the
following situation is possible:

- there exists a group `G`, which is public
- there exists a membership of a user `U` in `G`
- `Ability.allowed?(nil, :read_group, G) === true`
- `Ability.allowed?(nil, :read_group, U) === false`

This means that anonymous users cannot read the membership of public
groups, but they **can** read the membership of public projects.

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

spec/policies/group_member_policy_spec.rb

@@ -28,22 +28,37 @@ 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(*member_related_permissions)
+      expect_allowed(:read_group)
+    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 +68,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