Merge branch 'api-group-leaking' into 'master'

API: Return 404 if user does not have access to group

Closes #15185 

After !3587 is merged, I'll update this one to also fix the return code of the tests in !3587.

See merge request !3683

CHANGELOG

@@ -45,6 +45,7 @@ v 8.7.0 (unreleased)
   - Fix admin/projects when using visibility levels on search (PotHix)
   - Build status notifications
   - API: Expose user location (Robert Schilling)
+  - API: Do not leak group existence via return code (Robert Schilling)
   - ClosingIssueExtractor regex now also works with colons. e.g. "Fixes: #1234" !3591
   - Update number of Todos in the sidebar when it's marked as "Done". !3600
   - API: Expose 'updated_at' for issue, snippet, and merge request notes (Robert Schilling)

lib/api/helpers.rb

@@ -91,8 +91,7 @@ module API
       if can?(current_user, :read_group, group)
         group
       else
-        forbidden!("#{current_user.username} lacks sufficient "\
-        "access to #{group.name}")
+        not_found!('Group')
       end
     end
 

spec/requests/api/group_members_spec.rb

@@ -42,9 +42,10 @@ describe API::API, api: true  do
         end
       end
 
-      it "users not part of the group should get access error" do
+      it 'users not part of the group should get access error' do
         get api("/groups/#{group_with_members.id}/members", stranger)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
   end
@@ -165,12 +166,13 @@ describe API::API, api: true  do
     end
   end
 
-  describe "DELETE /groups/:id/members/:user_id" do
-    context "when not a member of the group" do
+  describe 'DELETE /groups/:id/members/:user_id' do
+    context 'when not a member of the group' do
       it "should not delete guest's membership of group_with_members" do
         random_user = create(:user)
         delete api("/groups/#{group_with_members.id}/members/#{owner.id}", random_user)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
 

spec/requests/api/groups_spec.rb

@@ -61,7 +61,8 @@ describe API::API, api: true  do
 
       it "should not return a group not attached to user1" do
         get api("/groups/#{group2.id}", user1)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
 
@@ -92,7 +93,8 @@ describe API::API, api: true  do
 
       it 'should not return a group not attached to user1' do
         get api("/groups/#{group2.path}", user1)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
   end
@@ -133,10 +135,10 @@ describe API::API, api: true  do
     end
 
     context 'when authenticated as an user that cannot see the group' do
-      it 'returns 403 when trying to update the group' do
+      it 'returns 404 when trying to update the group' do
         put api("/groups/#{group2.id}", user1), name: new_group_name
 
-        expect(response.status).to eq(403)
+        expect(response.status).to eq(404)
       end
     end
   end
@@ -157,7 +159,8 @@ describe API::API, api: true  do
 
       it "should not return a group not attached to user1" do
         get api("/groups/#{group2.id}/projects", user1)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
 
@@ -189,7 +192,8 @@ describe API::API, api: true  do
 
       it 'should not return a group not attached to user1' do
         get api("/groups/#{group2.path}/projects", user1)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end
   end
@@ -247,7 +251,8 @@ describe API::API, api: true  do
 
       it "should not remove a group not attached to user1" do
         delete api("/groups/#{group2.id}", user1)
-        expect(response.status).to eq(403)
+
+        expect(response.status).to eq(404)
       end
     end