Commit 20f679d6 authored by Timothy Andrew's avatar Timothy Andrew

Allow unauthenticated access to the `/api/v4/users` API.

- The issue filtering frontend code needs access to this API for non-logged-in
  users + public projects. It uses the API to fetch information for a user by
  username.

- We don't authenticate this API anymore, but instead - if the `current_user` is
  not present:

  - Verify that the `username` parameter has been passed. This disallows an
    unauthenticated user from grabbing a list of all users on the instance. The
    `UsersFinder` class performs an exact match on the `username`, so we are
    guaranteed to get 0 or 1 users.
  - Verify that the resulting user (if any) is accessible to be viewed publicly
    by calling `can?(current_user, :read_user, user)`
parent f0886918
...@@ -27,8 +27,11 @@ class UsersFinder ...@@ -27,8 +27,11 @@ class UsersFinder
users = by_search(users) users = by_search(users)
users = by_blocked(users) users = by_blocked(users)
users = by_active(users) users = by_active(users)
if current_user
users = by_external_identity(users) users = by_external_identity(users)
users = by_external(users) users = by_external(users)
end
users users
end end
......
...@@ -407,5 +407,11 @@ module API ...@@ -407,5 +407,11 @@ module API
exception.status == 500 exception.status == 500
end end
# Does the current route match the route identified by
# `description`?
def route_matches_description?(description)
options.dig(:route_options, :description) == description
end
end end
end end
...@@ -4,7 +4,7 @@ module API ...@@ -4,7 +4,7 @@ module API
before do before do
allow_access_with_scope :read_user if request.get? allow_access_with_scope :read_user if request.get?
authenticate! authenticate! unless route_matches_description?("Get the list of users")
end end
resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
...@@ -51,15 +51,26 @@ module API ...@@ -51,15 +51,26 @@ module API
use :pagination use :pagination
end end
get do get do
unless can?(current_user, :read_users_list)
render_api_error!("Not authorized.", 403)
end
authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?) authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
users = UsersFinder.new(current_user, params).execute users = UsersFinder.new(current_user, params).execute
entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic authorized =
if current_user
can?(current_user, :read_users_list)
else
# When `current_user` is not present, require that the `username`
# parameter is passed, to prevent an unauthenticated user from accessing
# a list of all the users on the GitLab instance. `UsersFinder` performs
# an exact match on the `username` parameter, so we are guaranteed to
# get either 0 or 1 `users` here.
params[:username].present? &&
users.all? { |user| can?(current_user, :read_user, user) }
end
render_api_error!("Not authorized.", 403) unless authorized
entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic
present paginate(users), with: entity present paginate(users), with: entity
end end
......
...@@ -13,9 +13,40 @@ describe API::Users do ...@@ -13,9 +13,40 @@ describe API::Users do
describe 'GET /users' do describe 'GET /users' do
context "when unauthenticated" do context "when unauthenticated" do
it "returns authentication error" do it "returns authorization error when the `username` parameter is not passed" do
get api("/users") get api("/users")
expect(response).to have_http_status(401)
expect(response).to have_http_status(403)
end
it "returns the user when a valid `username` parameter is passed" do
user = create(:user)
get api("/users"), username: user.username
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.size).to eq(1)
expect(json_response[0]['id']).to eq(user.id)
expect(json_response[0]['username']).to eq(user.username)
end
it "returns authorization error when the `username` parameter refers to an inaccessible user" do
user = create(:user)
expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
get api("/users"), username: user.username
expect(response).to have_http_status(403)
end
it "returns an empty response when an invalid `username` parameter is passed" do
get api("/users"), username: 'invalid'
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.size).to eq(0)
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment