Merge branch...

Merge branch 'security-anonymous-user-can-enumerate-all-users-through-api-v4-users-id' into 'master'

Prohibit anonymous access for specific user API endpoint

See merge request gitlab-org/security/gitlab!1717

doc/api/users.md

@@ -259,7 +259,7 @@ GET /users?with_custom_attributes=true
 
 ## Single user
 
-Get a single user. This endpoint can be accessed without authentication.
+Get a single user.
 
 ### For user
 
@@ -806,7 +806,7 @@ Example response:
 
 ### Followers and following
 
-Get the followers of a user. This endpoint can be accessed without authentication.
+Get the followers of a user.
 
 ```plaintext
 GET /users/:id/followers

lib/api/users.rb

@@ -140,7 +140,10 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ":id", feature_category: :users do
+        forbidden!('Not authorized!') unless current_user
+
         user = User.find_by(id: params[:id])
+
         not_found!('User') unless user && can?(current_user, :read_user, user)
 
         opts = { with: current_user&.admin? ? Entities::UserDetailsWithAdmin : Entities::User, current_user: current_user }
@@ -156,6 +159,7 @@ module API
       end
       get ":user_id/status", requirements: API::USER_REQUIREMENTS, feature_category: :users do
         user = find_user(params[:user_id])
+
         not_found!('User') unless user && can?(current_user, :read_user, user)
 
         present user.status || {}, with: Entities::UserStatus
@@ -203,6 +207,8 @@ module API
         use :pagination
       end
       get ':id/following', feature_category: :users do
+        forbidden!('Not authorized!') unless current_user
+
         user = find_user(params[:id])
         not_found!('User') unless user && can?(current_user, :read_user_profile, user)
 
@@ -217,6 +223,8 @@ module API
         use :pagination
       end
       get ':id/followers', feature_category: :users do
+        forbidden!('Not authorized!') unless current_user
+
         user = find_user(params[:id])
         not_found!('User') unless user && can?(current_user, :read_user_profile, user)
 

spec/requests/api/users_spec.rb

@@ -53,37 +53,6 @@ RSpec.describe API::Users do
       end
     end
 
-    describe 'GET /users/:id' do
-      context 'when unauthenticated' do
-        it 'does not contain the note of the user' do
-          get api("/users/#{user.id}")
-
-          expect(json_response).not_to have_key('note')
-        end
-      end
-
-      context 'when authenticated' do
-        context 'as an admin' do
-          it 'contains the note of the user' do
-            get api("/users/#{user.id}", admin)
-
-            expect(json_response).to have_key('note')
-            expect(json_response['note']).to eq(user.note)
-            expect(json_response).to have_key('sign_in_count')
-          end
-        end
-
-        context 'as a regular user' do
-          it 'does not contain the note of the user' do
-            get api("/users/#{user.id}", user)
-
-            expect(json_response).not_to have_key('note')
-            expect(json_response).not_to have_key('sign_in_count')
-          end
-        end
-      end
-    end
-
     describe "PUT /users/:id" do
       context 'when user is an admin' do
         it "updates note of the user" do
@@ -527,6 +496,8 @@ RSpec.describe API::Users do
   end
 
   describe "GET /users/:id" do
+    let_it_be(:user2, reload: true) { create(:user, username: 'another_user') }
+
     it "returns a user by id" do
       get api("/users/#{user.id}", user)
 
@@ -564,6 +535,64 @@ RSpec.describe API::Users do
       expect(json_response.keys).not_to include 'trial'
     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(user, :read_user, user2).and_return(false)
+
+      get api("/users/#{user2.id}", user)
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+
+    it 'returns the `created_at` field for public users' do
+      get api("/users/#{user2.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).to include('created_at')
+    end
+
+    it 'does not return the `created_at` field for private users' do
+      get api("/users/#{private_user.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).not_to include('created_at')
+    end
+
+    it 'returns the `followers` field for public users' do
+      get api("/users/#{user2.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).to include('followers')
+    end
+
+    it 'does not return the `followers` field for private users' do
+      get api("/users/#{private_user.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).not_to include('followers')
+    end
+
+    it 'returns the `following` field for public users' do
+      get api("/users/#{user2.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).to include('following')
+    end
+
+    it 'does not return the `following` field for private users' do
+      get api("/users/#{private_user.id}", user)
+
+      expect(response).to match_response_schema('public_api/v4/user/basic')
+      expect(json_response.keys).not_to include('following')
+    end
+
+    it 'does not contain the note of the user' do
+      get api("/users/#{user.id}", user)
+
+      expect(json_response).not_to have_key('note')
+      expect(json_response).not_to have_key('sign_in_count')
+    end
+
     context 'when job title is present' do
       let(:job_title) { 'Fullstack Engineer' }
 
@@ -580,6 +609,14 @@ RSpec.describe API::Users do
     end
 
     context 'when authenticated as admin' do
+      it 'contains the note of the user' do
+        get api("/users/#{user.id}", admin)
+
+        expect(json_response).to have_key('note')
+        expect(json_response['note']).to eq(user.note)
+        expect(json_response).to have_key('sign_in_count')
+      end
+
       it 'includes the `is_admin` field' do
         get api("/users/#{user.id}", admin)
 
@@ -640,62 +677,10 @@ RSpec.describe API::Users do
     end
 
     context 'for an anonymous user' do
-      it "returns a user by id" do
-        get api("/users/#{user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        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_gitlab_http_status(:not_found)
-      end
-
-      it "returns the `created_at` field for public users" do
-        get api("/users/#{user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).to include 'created_at'
-      end
-
-      it "does not return the `created_at` field for private users" do
-        get api("/users/#{private_user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).not_to include 'created_at'
-      end
-
-      it "returns the `followers` field for public users" do
-        get api("/users/#{user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).to include 'followers'
-      end
-
-      it "does not return the `followers` field for private users" do
-        get api("/users/#{private_user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).not_to include 'followers'
-      end
-
-      it "returns the `following` field for public users" do
+      it 'returns 403' do
         get api("/users/#{user.id}")
 
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).to include 'following'
-      end
-
-      it "does not return the `following` field for private users" do
-        get api("/users/#{private_user.id}")
-
-        expect(response).to match_response_schema('public_api/v4/user/basic')
-        expect(json_response.keys).not_to include 'following'
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
 
@@ -788,6 +773,14 @@ RSpec.describe API::Users do
   describe 'GET /users/:id/followers' do
     let(:follower) { create(:user) }
 
+    context 'for an anonymous user' do
+      it 'returns 403' do
+        get api("/users/#{user.id}")
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
+
     context 'user has followers' do
       it 'lists followers' do
         follower.follow(user)
@@ -823,6 +816,14 @@ RSpec.describe API::Users do
   describe 'GET /users/:id/following' do
     let(:followee) { create(:user) }
 
+    context 'for an anonymous user' do
+      it 'returns 403' do
+        get api("/users/#{user.id}")
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
+
     context 'user has followers' do
       it 'lists following user' do
         user.follow(followee)