From 0efbc0b240f656a2288f566e61b6fff6f5f6fe4b Mon Sep 17 00:00:00 2001
From: Imre Farkas <ifarkas@gitlab.com>
Date: Fri, 3 Jun 2022 14:58:28 +0000
Subject: [PATCH] Require authentication when enumerating users via GraqhQL
Changelog: changed
---
app/graphql/resolvers/user_resolver.rb | 10 ++++
app/graphql/resolvers/users_resolver.rb | 8 ++-
...require_auth_for_graphql_user_resolver.yml | 8 +++
.../visibility_and_access_controls.md | 4 +-
spec/graphql/resolvers/user_resolver_spec.rb | 49 +++++++++++++++----
spec/graphql/resolvers/users_resolver_spec.rb | 30 ++++++++----
spec/graphql/types/user_type_spec.rb | 8 +--
.../user/starred_projects_query_spec.rb | 21 ++++----
8 files changed, 101 insertions(+), 37 deletions(-)
create mode 100644 config/feature_flags/development/require_auth_for_graphql_user_resolver.yml
diff --git a/app/graphql/resolvers/user_resolver.rb b/app/graphql/resolvers/user_resolver.rb
index 99fd0d4927dd..d2e8aa514693 100644
--- a/app/graphql/resolvers/user_resolver.rb
+++ b/app/graphql/resolvers/user_resolver.rb
@@ -2,6 +2,8 @@
module Resolvers
class UserResolver < BaseResolver
+ include Gitlab::Graphql::Authorize::AuthorizeResource
+
description 'Retrieve a single user'
type Types::UserType, null: true
@@ -23,6 +25,8 @@ def ready?(id: nil, username: nil)
end
def resolve(id: nil, username: nil)
+ authorize!
+
if id
GitlabSchema.object_from_id(id, expected_type: User)
else
@@ -39,5 +43,11 @@ def batch_load(username)
end
end
end
+
+ def authorize!
+ return unless Feature.enabled?(:require_auth_for_graphql_user_resolver)
+
+ raise_resource_not_available_error! unless context[:current_user].present?
+ end
end
end
diff --git a/app/graphql/resolvers/users_resolver.rb b/app/graphql/resolvers/users_resolver.rb
index 1424c14083df..12555f4e5656 100644
--- a/app/graphql/resolvers/users_resolver.rb
+++ b/app/graphql/resolvers/users_resolver.rb
@@ -47,8 +47,12 @@ def ready?(**args)
end
def authorize!(usernames)
- authorized = Ability.allowed?(context[:current_user], :read_users_list)
- authorized &&= usernames.present? if context[:current_user].blank?
+ if Feature.enabled?(:require_auth_for_graphql_user_resolver)
+ authorized = context[:current_user].present?
+ else
+ authorized = Ability.allowed?(context[:current_user], :read_users_list)
+ authorized &&= usernames.present? if context[:current_user].blank?
+ end
raise_resource_not_available_error! unless authorized
end
diff --git a/config/feature_flags/development/require_auth_for_graphql_user_resolver.yml b/config/feature_flags/development/require_auth_for_graphql_user_resolver.yml
new file mode 100644
index 000000000000..7dd6c7ecd957
--- /dev/null
+++ b/config/feature_flags/development/require_auth_for_graphql_user_resolver.yml
@@ -0,0 +1,8 @@
+---
+name: require_auth_for_graphql_user_resolver
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/88020
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/362953
+milestone: '15.1'
+type: development
+group: group::authentication and authorization
+default_enabled: false
diff --git a/doc/user/admin_area/settings/visibility_and_access_controls.md b/doc/user/admin_area/settings/visibility_and_access_controls.md
index 33b392676adb..103eae07517e 100644
--- a/doc/user/admin_area/settings/visibility_and_access_controls.md
+++ b/doc/user/admin_area/settings/visibility_and_access_controls.md
@@ -142,7 +142,9 @@ To restrict visibility levels for projects, snippets, and selected pages:
1. In the **Restricted visibility levels** section, select the desired visibility levels to restrict.
If you restrict the **Public** level:
- User profiles are only visible to logged in users via the Web interface.
- - User attributes are only visible to authenticated users via the GraphQL API.
+ - User attributes via the GraphQL API are:
+ - Not visible in [GitLab 15.1 and later](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/88020).
+ - Only visible to authenticated users between [GitLab 13.1](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33195) and GitLab 15.0.
1. Select **Save changes**.
For more details on project visibility, see
diff --git a/spec/graphql/resolvers/user_resolver_spec.rb b/spec/graphql/resolvers/user_resolver_spec.rb
index 446d765d3eef..32a9b1776293 100644
--- a/spec/graphql/resolvers/user_resolver_spec.rb
+++ b/spec/graphql/resolvers/user_resolver_spec.rb
@@ -6,8 +6,41 @@
include GraphqlHelpers
describe '#resolve' do
+ let_it_be(:current_user) { nil }
let_it_be(:user) { create(:user) }
+ shared_examples 'queries user' do
+ context 'authenticated access' do
+ let_it_be(:current_user) { create(:user) }
+
+ it 'returns the correct user' do
+ expect(
+ resolve_user(args)
+ ).to eq(user)
+ end
+ end
+
+ context 'unauthenticated access' do
+ it 'forbids search' do
+ expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do
+ resolve_user(args)
+ end
+ end
+
+ context 'require_auth_for_graphql_user_resolver feature flag is disabled' do
+ before do
+ stub_feature_flags(require_auth_for_graphql_user_resolver: false)
+ end
+
+ it 'returns the correct user' do
+ expect(
+ resolve_user(args)
+ ).to eq(user)
+ end
+ end
+ end
+ end
+
context 'when neither an ID or a username is provided' do
it 'generates an ArgumentError' do
expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError) do
@@ -23,25 +56,21 @@
end
context 'by username' do
- it 'returns the correct user' do
- expect(
- resolve_user(username: user.username)
- ).to eq(user)
+ include_examples "queries user" do
+ let(:args) { { username: user.username } }
end
end
context 'by ID' do
- it 'returns the correct user' do
- expect(
- resolve_user(id: user.to_global_id)
- ).to eq(user)
+ include_examples "queries user" do
+ let(:args) { { id: user.to_global_id } }
end
end
end
private
- def resolve_user(args = {})
- sync(resolve(described_class, args: args))
+ def resolve_user(args = {}, context = { current_user: current_user })
+ sync(resolve(described_class, args: args, ctx: context))
end
end
diff --git a/spec/graphql/resolvers/users_resolver_spec.rb b/spec/graphql/resolvers/users_resolver_spec.rb
index 1ba296912a3a..5f7a096a14b8 100644
--- a/spec/graphql/resolvers/users_resolver_spec.rb
+++ b/spec/graphql/resolvers/users_resolver_spec.rb
@@ -14,14 +14,6 @@
end
describe '#resolve' do
- it 'generates an error when read_users_list is not authorized' do
- expect(Ability).to receive(:allowed?).with(current_user, :read_users_list).and_return(false)
-
- expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do
- resolve_users
- end
- end
-
context 'when no arguments are passed' do
it 'returns all users' do
expect(resolve_users).to contain_exactly(user1, user2, current_user)
@@ -79,8 +71,26 @@
end
end
- it 'allows to search by username' do
- expect(resolve_users(args: { usernames: [user1.username] })).to contain_exactly(user1)
+ it 'prohibits search by username' do
+ expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do
+ resolve_users(args: { usernames: [user1.username] })
+ end
+ end
+
+ context 'require_auth_for_graphql_user_resolver feature flag is disabled' do
+ before do
+ stub_feature_flags(require_auth_for_graphql_user_resolver: false)
+ end
+
+ it 'prohibits search without usernames passed' do
+ expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do
+ resolve_users
+ end
+ end
+
+ it 'allows to search by username' do
+ expect(resolve_users(args: { usernames: [user1.username] })).to contain_exactly(user1)
+ end
end
end
end
diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb
index c913a4c3662f..fec6a771640d 100644
--- a/spec/graphql/types/user_type_spec.rb
+++ b/spec/graphql/types/user_type_spec.rb
@@ -91,8 +91,8 @@
context 'when requester is nil' do
let(:current_user) { nil }
- it 'returns `****`' do
- expect(user_name).to eq('****')
+ it 'returns nothing' do
+ expect(user_name).to be_nil
end
end
@@ -134,8 +134,8 @@
context 'when requester is nil' do
let(:current_user) { nil }
- it 'returns `****`' do
- expect(user_name).to eq('****')
+ it 'returns nothing' do
+ expect(user_name).to be_nil
end
end
diff --git a/spec/requests/api/graphql/user/starred_projects_query_spec.rb b/spec/requests/api/graphql/user/starred_projects_query_spec.rb
index 37a85b98e5f7..75a17ed34c40 100644
--- a/spec/requests/api/graphql/user/starred_projects_query_spec.rb
+++ b/spec/requests/api/graphql/user/starred_projects_query_spec.rb
@@ -17,7 +17,6 @@
let_it_be(:user, reload: true) { create(:user) }
let(:user_fields) { 'starredProjects { nodes { id } }' }
- let(:current_user) { nil }
let(:starred_projects) do
post_graphql(query, current_user: current_user)
@@ -34,21 +33,23 @@
user.toggle_star(project_c)
end
- it_behaves_like 'a working graphql query' do
- before do
- post_graphql(query)
- end
- end
+ context 'anonymous access' do
+ let(:current_user) { nil }
- it 'found only public project' do
- expect(starred_projects).to contain_exactly(
- a_graphql_entity_for(project_a)
- )
+ it 'returns nothing' do
+ expect(starred_projects).to be_nil
+ end
end
context 'the current user is the user' do
let(:current_user) { user }
+ it_behaves_like 'a working graphql query' do
+ before do
+ post_graphql(query, current_user: current_user)
+ end
+ end
+
it 'found all projects' do
expect(starred_projects).to contain_exactly(
a_graphql_entity_for(project_a),
--
GitLab