diff options
| -rw-r--r-- | app/finders/users_finder.rb | 7 | ||||
| -rw-r--r-- | lib/api/helpers.rb | 6 | ||||
| -rw-r--r-- | lib/api/users.rb | 23 | ||||
| -rw-r--r-- | spec/requests/api/users_spec.rb | 35 |
4 files changed, 61 insertions, 10 deletions
diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb index dbd50d1db7c..0534317df8f 100644 --- a/app/finders/users_finder.rb +++ b/app/finders/users_finder.rb @@ -27,8 +27,11 @@ class UsersFinder users = by_search(users) users = by_blocked(users) users = by_active(users) - users = by_external_identity(users) - users = by_external(users) + + if current_user + users = by_external_identity(users) + users = by_external(users) + end users end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2c73a6fdc4e..1322afaa64f 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -407,5 +407,11 @@ module API exception.status == 500 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 diff --git a/lib/api/users.rb b/lib/api/users.rb index c10e3364382..34619c90d8b 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -4,7 +4,7 @@ module API before do allow_access_with_scope :read_user if request.get? - authenticate! + authenticate! unless route_matches_description?("Get the list of users") end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do @@ -51,15 +51,26 @@ module API use :pagination end 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?) 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 end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 18000d91795..01541901330 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -13,9 +13,40 @@ describe API::Users do describe 'GET /users' 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") - 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 |