Anonymous user can enumerate all users through GraphQL endpoint

Merge branch 'security-571-anonymous-user-can-enumerate-all-users-through-graphql-endpoint-14-7' into '14-7-stable-ee'

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

Changelog: security

app/graphql/resolvers/users_resolver.rb

@@ -29,7 +29,7 @@ module Resolvers
               description: 'Return only admin users.'
     def resolve(ids: nil, usernames: nil, sort: nil, search: nil, admins: nil)
-      authorize!
+      authorize!(usernames)
       ::UsersFinder.new(context[:current_user], finder_params(ids, usernames, sort, search, admins)).execute
     end
@@ -46,8 +46,11 @@ module Resolvers
       super
     end
-    def authorize!
-      Ability.allowed?(context[:current_user], :read_users_list) || raise_resource_not_available_error!
+    def authorize!(usernames)
+      authorized = Ability.allowed?(context[:current_user], :read_users_list)
+      authorized &&= usernames.present? if context[:current_user].blank?
+
+      raise_resource_not_available_error! unless authorized
     end
     private

lib/api/users.rb

@@ -105,9 +105,6 @@ module API
           params.except!(:created_after, :created_before, :order_by, :sort, :two_factor, :without_projects)
         end
-        users = UsersFinder.new(current_user, params).execute
-        users = reorder_users(users)
-
         authorized = can?(current_user, :read_users_list)
         # When `current_user` is not present, require that the `username`
@@ -119,6 +116,9 @@ module API
         forbidden!("Not authorized to access /api/v4/users") unless authorized
+        users = UsersFinder.new(current_user, params).execute
+        users = reorder_users(users)
+
         entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic
         users = users.preload(:identities, :u2f_registrations) if entity == Entities::UserWithAdmin
         users = users.preload(:identities, :webauthn_registrations) if entity == Entities::UserWithAdmin

spec/graphql/resolvers/users_resolver_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Resolvers::UsersResolver do
   let_it_be(:user1) { create(:user, name: "SomePerson") }
   let_it_be(:user2) { create(:user, username: "someone123784") }
+  let_it_be(:current_user) { create(:user) }
   specify do
     expect(described_class).to have_nullable_graphql_type(Types::UserType.connection_type)
@@ -14,14 +15,14 @@ RSpec.describe Resolvers::UsersResolver do
   describe '#resolve' do
     it 'raises an error when read_users_list is not authorized' do
-      expect(Ability).to receive(:allowed?).with(nil, :read_users_list).and_return(false)
+      expect(Ability).to receive(:allowed?).with(current_user, :read_users_list).and_return(false)
       expect { resolve_users }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
     end
     context 'when no arguments are passed' do
       it 'returns all users' do
-        expect(resolve_users).to contain_exactly(user1, user2)
+        expect(resolve_users).to contain_exactly(user1, user2, current_user)
       end
     end
@@ -65,9 +66,21 @@ RSpec.describe Resolvers::UsersResolver do
         expect(resolve_users( args: { search: "someperson" } )).to contain_exactly(user1)
       end
     end
+
+    context 'with anonymous access' do
+      let_it_be(:current_user) { nil }
+
+      it 'prohibits search without usernames passed' do
+        expect { resolve_users }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+      end
+
+      it 'allows to search by username' do
+        expect(resolve_users(args: { usernames: [user1.username] })).to contain_exactly(user1)
+      end
+    end
   end
   def resolve_users(args: {}, ctx: {})
-    resolve(described_class, args: args, ctx: ctx)
+    resolve(described_class, args: args, ctx: { current_user: current_user }.merge(ctx))
   end
 end

spec/requests/api/graphql/users_spec.rb

@@ -5,11 +5,13 @@ require 'spec_helper'
 RSpec.describe 'Users' do
   include GraphqlHelpers
-  let_it_be(:current_user) { create(:user, created_at: 1.day.ago) }
+  let_it_be(:user0) { create(:user, created_at: 1.day.ago) }
   let_it_be(:user1) { create(:user, created_at: 2.days.ago) }
   let_it_be(:user2) { create(:user, created_at: 3.days.ago) }
   let_it_be(:user3) { create(:user, created_at: 4.days.ago) }
+  let(:current_user) { user0 }
+
   describe '.users' do
     shared_examples 'a working users query' do
       it_behaves_like 'a working graphql query' do
@@ -19,7 +21,7 @@ RSpec.describe 'Users' do
       end
       it 'includes a list of users' do
-        post_graphql(query)
+        post_graphql(query, current_user: current_user)
         expect(graphql_data.dig('users', 'nodes')).not_to be_empty
       end
@@ -47,7 +49,7 @@ RSpec.describe 'Users' do
       let_it_be(:query) { graphql_query_for(:users, { ids: user1.to_global_id.to_s, usernames: user1.username }, 'nodes { id }') }
       it 'displays an error' do
-        post_graphql(query)
+        post_graphql(query, current_user: current_user)
         expect(graphql_errors).to include(
           a_hash_including('message' => a_string_matching(%r{Provide either a list of usernames or ids}))
@@ -66,14 +68,14 @@ RSpec.describe 'Users' do
         it_behaves_like 'a working users query'
-        it 'includes all non-admin users', :aggregate_failures do
-          post_graphql(query)
+        it 'includes all users', :aggregate_failures do
+          post_query
           expect(graphql_data.dig('users', 'nodes')).to include(
+            { "id" => user0.to_global_id.to_s },
             { "id" => user1.to_global_id.to_s },
             { "id" => user2.to_global_id.to_s },
             { "id" => user3.to_global_id.to_s },
-            { "id" => current_user.to_global_id.to_s },
             { "id" => admin.to_global_id.to_s },
             { "id" => another_admin.to_global_id.to_s }
           )
@@ -81,10 +83,12 @@ RSpec.describe 'Users' do
       end
       context 'when current user is an admin' do
+        let(:current_user) { admin }
+
         it_behaves_like 'a working users query'
         it 'includes only admins', :aggregate_failures do
-          post_graphql(query, current_user: admin)
+          post_graphql(query, current_user: current_user)
           expect(graphql_data.dig('users', 'nodes')).to include(
             { "id" => another_admin.to_global_id.to_s },
@@ -92,10 +96,10 @@ RSpec.describe 'Users' do
           )
           expect(graphql_data.dig('users', 'nodes')).not_to include(
+            { "id" => user0.to_global_id.to_s },
             { "id" => user1.to_global_id.to_s },
             { "id" => user2.to_global_id.to_s },
-            { "id" => user3.to_global_id.to_s },
-            { "id" => current_user.to_global_id.to_s }
+            { "id" => user3.to_global_id.to_s }
           )
         end
       end
@@ -110,7 +114,7 @@ RSpec.describe 'Users' do
     end
     context 'when sorting by created_at' do
-      let_it_be(:ascending_users) { [user3, user2, user1, current_user].map { |u| global_id_of(u) } }
+      let_it_be(:ascending_users) { [user3, user2, user1, user0].map { |u| global_id_of(u) } }
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do