Commit d1488268 authored by Timothy Andrew's avatar Timothy Andrew

Simplify authentication logic in the v4 users API for !12445.

- Rather than using an explicit check to turn off authentication for the
  `/users` endpoint, simply call `authenticate_non_get!`.

- All `GET` endpoints we wish to restrict already call
  `authenticated_as_admin!`, and so remain inacessible to anonymous users.

- This _does_ open up the `/users/:id` endpoint to anonymous access. It contains
  the same access check that `/users` users, and so is safe for use here.

- More context: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12445#note_34031323
parent 96e98632
...@@ -407,11 +407,5 @@ module API ...@@ -407,11 +407,5 @@ module API
exception.status == 500 exception.status == 500
end end
# Does the current route match the route identified by
# `description`?
def request_matches_route?(method, route)
request.request_method == method && request.path == route
end
end end
end end
...@@ -4,10 +4,13 @@ module API ...@@ -4,10 +4,13 @@ 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! unless request_matches_route?('GET', '/api/v4/users')
end end
resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
before do
authenticate_non_get!
end
helpers do helpers do
def find_user(params) def find_user(params)
id = params[:user_id] || params[:id] id = params[:user_id] || params[:id]
...@@ -405,6 +408,10 @@ module API ...@@ -405,6 +408,10 @@ module API
end end
resource :user do resource :user do
before do
authenticate!
end
desc 'Get the currently authenticated user' do desc 'Get the currently authenticated user' do
success Entities::UserPublic success Entities::UserPublic
end end
......
...@@ -169,6 +169,7 @@ describe API::Users do ...@@ -169,6 +169,7 @@ describe API::Users do
describe "GET /users/:id" do describe "GET /users/:id" do
it "returns a user by id" do it "returns a user by id" do
get api("/users/#{user.id}", user) get api("/users/#{user.id}", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username) expect(json_response['username']).to eq(user.username)
end end
...@@ -179,9 +180,22 @@ describe API::Users do ...@@ -179,9 +180,22 @@ describe API::Users do
expect(json_response['is_admin']).to be_nil expect(json_response['is_admin']).to be_nil
end end
it "returns a 401 if unauthenticated" do context 'for an anonymous user' do
get api("/users/9998") it "returns a user by id" do
expect(response).to have_http_status(401) get api("/users/#{user.id}")
expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username)
end
it "returns a 404 if the target user is present but inaccessible" do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
get api("/users/#{user.id}")
expect(response).to have_http_status(404)
end
end end
it "returns a 404 error if user id not found" do it "returns a 404 error if user id not found" do
......
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