Merge branch 'security-296-private_profile_exposure' into 'master'

Ensure group and project memberships are not leaked

See merge request gitlab-org/security/gitlab!1076

app/graphql/types/user_type.rb

@@ -33,8 +33,7 @@ module Types
           resolver: Resolvers::TodoResolver,
           description: 'Todos of the user'
     field :group_memberships, Types::GroupMemberType.connection_type, null: true,
-          description: 'Group memberships of the user',
-          method: :group_members
+          description: 'Group memberships of the user'
     field :group_count, GraphQL::INT_TYPE, null: true,
           resolver: Resolvers::Users::GroupCountResolver,
           description: 'Group count for the user',
@@ -44,8 +43,7 @@ module Types
     field :location, ::GraphQL::STRING_TYPE, null: true,
           description: 'The location of the user.'
     field :project_memberships, Types::ProjectMemberType.connection_type, null: true,
-          description: 'Project memberships of the user',
-          method: :project_members
+          description: 'Project memberships of the user'
     field :starred_projects, Types::ProjectType.connection_type, null: true,
           description: 'Projects starred by the user',
           resolver: Resolvers::UserStarredProjectsResolver

app/presenters/user_presenter.rb

@@ -2,4 +2,18 @@
 
 class UserPresenter < Gitlab::View::Presenter::Delegated
   presents :user
+
+  def group_memberships
+    should_be_private? ? GroupMember.none : user.group_members
+  end
+
+  def project_memberships
+    should_be_private? ? ProjectMember.none : user.project_members
+  end
+
+  private
+
+  def should_be_private?
+    !can?(current_user, :read_user_profile, user)
+  end
 end

changelogs/unreleased/security-296-private_profile_exposure.yml

+---
+title: Ensure group and project memberships are not leaked via API for users with private profiles
+merge_request:
+author:
+type: security

spec/requests/api/graphql/user_query_spec.rb

@@ -251,7 +251,7 @@ RSpec.describe 'getting user information' do
 
     context 'the user is private' do
       before do
-        user.update(private_profile: true)
+        user.update!(private_profile: true)
         post_graphql(query, current_user: current_user)
       end
 
@@ -261,6 +261,50 @@ RSpec.describe 'getting user information' do
         it_behaves_like 'a working graphql query'
       end
 
+      context 'we request the groupMemberships' do
+        let_it_be(:membership_a) { create(:group_member, user: user) }
+        let(:group_memberships) { graphql_data_at(:user, :group_memberships, :nodes) }
+        let(:user_fields) { 'groupMemberships { nodes { id } }' }
+
+        it_behaves_like 'a working graphql query'
+
+        it 'cannot be found' do
+          expect(group_memberships).to be_empty
+        end
+
+        context 'the current user is the user' do
+          let(:current_user) { user }
+
+          it 'can be found' do
+            expect(group_memberships).to include(
+              a_hash_including('id' => global_id_of(membership_a))
+            )
+          end
+        end
+      end
+
+      context 'we request the projectMemberships' do
+        let_it_be(:membership_a) { create(:project_member, user: user) }
+        let(:project_memberships) { graphql_data_at(:user, :project_memberships, :nodes) }
+        let(:user_fields) { 'projectMemberships { nodes { id } }' }
+
+        it_behaves_like 'a working graphql query'
+
+        it 'cannot be found' do
+          expect(project_memberships).to be_empty
+        end
+
+        context 'the current user is the user' do
+          let(:current_user) { user }
+
+          it 'can be found' do
+            expect(project_memberships).to include(
+              a_hash_including('id' => global_id_of(membership_a))
+            )
+          end
+        end
+      end
+
       context 'we request the authoredMergeRequests' do
         let(:user_fields) { 'authoredMergeRequests { nodes { id } }' }