diff options
| -rw-r--r-- | app/graphql/resolvers/users_resolver.rb | 9 | ||||
| -rw-r--r-- | lib/api/users.rb | 6 | ||||
| -rw-r--r-- | spec/graphql/resolvers/users_resolver_spec.rb | 19 | ||||
| -rw-r--r-- | spec/requests/api/graphql/users_spec.rb | 24 |
4 files changed, 39 insertions, 19 deletions
diff --git a/app/graphql/resolvers/users_resolver.rb b/app/graphql/resolvers/users_resolver.rb index c6de3dba41a..1424c14083d 100644 --- a/app/graphql/resolvers/users_resolver.rb +++ b/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 diff --git a/lib/api/users.rb b/lib/api/users.rb index d540978931e..6d4f12d80f8 100644 --- a/lib/api/users.rb +++ b/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 diff --git a/spec/graphql/resolvers/users_resolver_spec.rb b/spec/graphql/resolvers/users_resolver_spec.rb index 031d7c99eef..29947c33430 100644 --- a/spec/graphql/resolvers/users_resolver_spec.rb +++ b/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 diff --git a/spec/requests/api/graphql/users_spec.rb b/spec/requests/api/graphql/users_spec.rb index 67cd35ee545..fe824834a2c 100644 --- a/spec/requests/api/graphql/users_spec.rb +++ b/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 |