diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2017-07-04 12:19:48 +0000 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2017-07-04 12:19:48 +0000 |
commit | d1488268b2e31b8f3549c6e1e46955619535cd98 (patch) | |
tree | 649bce69f61984ae85205e340b54f1d6bc121f17 | |
parent | 96e986327c4dad9248f9013f191119ffafe4a6d8 (diff) | |
download | gitlab-ce-d1488268b2e31b8f3549c6e1e46955619535cd98.tar.gz |
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
-rw-r--r-- | lib/api/helpers.rb | 6 | ||||
-rw-r--r-- | lib/api/users.rb | 9 | ||||
-rw-r--r-- | spec/requests/api/users_spec.rb | 20 |
3 files changed, 25 insertions, 10 deletions
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a3aec8889d7..2c73a6fdc4e 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -407,11 +407,5 @@ module API exception.status == 500 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 diff --git a/lib/api/users.rb b/lib/api/users.rb index bad4d76b428..5b9d9a71be4 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -4,10 +4,13 @@ module API before do allow_access_with_scope :read_user if request.get? - authenticate! unless request_matches_route?('GET', '/api/v4/users') end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do + before do + authenticate_non_get! + end + helpers do def find_user(params) id = params[:user_id] || params[:id] @@ -405,6 +408,10 @@ module API end resource :user do + before do + authenticate! + end + desc 'Get the currently authenticated user' do success Entities::UserPublic end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index aa95ae8c7cc..8640c16203e 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -169,6 +169,7 @@ describe API::Users do describe "GET /users/:id" do it "returns a user by id" do get api("/users/#{user.id}", user) + expect(response).to have_http_status(200) expect(json_response['username']).to eq(user.username) end @@ -179,9 +180,22 @@ describe API::Users do expect(json_response['is_admin']).to be_nil end - it "returns a 401 if unauthenticated" do - get api("/users/9998") - expect(response).to have_http_status(401) + context 'for an anonymous user' do + it "returns a user by id" do + 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 it "returns a 404 error if user id not found" do |