From d1488268b2e31b8f3549c6e1e46955619535cd98 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 4 Jul 2017 12:19:48 +0000
Subject: [PATCH] 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
---
 lib/api/helpers.rb              |  6 ------
 lib/api/users.rb                |  9 ++++++++-
 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