Merge branch 'improve-autocomplete-controller-for-unknowns' into 'master'

Better handle unknown projects and groups for autocomplete

While working on !963, I noticed that specifying an unknown project ID or group ID would cause an error in the controller. This MR improves on the handling of unknown projects or groups:

1. If the user is authenticated, return a blank JSON with a 404 error.
2. Otherwise, redirect the user to login so we don't leak information about whether certain projects/groups exist.

See merge request !964

2 files changed, 86 insertions, 29 deletions

diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index 8b12643bb97..52e9c58b47c 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/app/controllers/autocomplete_controller.rb
@@ -2,25 +2,34 @@ class AutocompleteController < ApplicationController
   skip_before_action :authenticate_user!, only: [:users]
 
   def users
-    @users =
-      if params[:project_id].present?
-        project = Project.find(params[:project_id])
+    begin
+      @users =
+        if params[:project_id].present?
+          project = Project.find(params[:project_id])
 
-        if can?(current_user, :read_project, project)
-          project.team.users
-        end
-      elsif params[:group_id]
-        group = Group.find(params[:group_id])
+          if can?(current_user, :read_project, project)
+            project.team.users
+          end
+        elsif params[:group_id]
+          group = Group.find(params[:group_id])
 
-        if can?(current_user, :read_group, group)
-          group.users
+          if can?(current_user, :read_group, group)
+            group.users
+          end
+        elsif current_user
+          User.all
         end
-      elsif current_user
-        User.all
-      else
-        User.none
+    rescue ActiveRecord::RecordNotFound
+      if current_user
+        return render json: {}, status: 404
       end
+    end
+
+    if @users.nil? && current_user.nil?
+      authenticate_user!
+    end
 
+    @users ||= User.none
     @users = @users.search(params[:search]) if params[:search].present?
     @users = @users.active
     @users = @users.page(params[:page]).per(PER_PAGE)
diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb
index 9be8d0333ad..1230017c270 100644
--- a/spec/controllers/autocomplete_controller_spec.rb
+++ b/spec/controllers/autocomplete_controller_spec.rb
@@ -9,15 +9,27 @@ describe AutocompleteController do
     before do
       sign_in(user)
       project.team << [user, :master]
-
-      get(:users, project_id: project.id)
     end
 
     let(:body) { JSON.parse(response.body) }
 
-    it { expect(body).to be_kind_of(Array) }
-    it { expect(body.size).to eq 1 }
-    it { expect(body.first["username"]).to eq user.username }
+    describe 'GET #users with project ID' do
+      before do
+        get(:users, project_id: project.id)
+      end
+
+      it { expect(body).to be_kind_of(Array) }
+      it { expect(body.size).to eq 1 }
+      it { expect(body.first["username"]).to eq user.username }
+    end
+
+    describe 'GET #users with unknown project' do
+      before do
+        get(:users, project_id: 'unknown')
+      end
+
+      it { expect(response.status).to eq(404) }
+    end
   end
 
   context 'group members' do
@@ -26,15 +38,27 @@ describe AutocompleteController do
     before do
       sign_in(user)
       group.add_owner(user)
-
-      get(:users, group_id: group.id)
     end
 
     let(:body) { JSON.parse(response.body) }
 
-    it { expect(body).to be_kind_of(Array) }
-    it { expect(body.size).to eq 1 }
-    it { expect(body.first["username"]).to eq user.username }
+    describe 'GET #users with group ID' do
+      before do
+        get(:users, group_id: group.id)
+      end
+
+      it { expect(body).to be_kind_of(Array) }
+      it { expect(body.size).to eq 1 }
+      it { expect(body.first["username"]).to eq user.username }
+    end
+
+    describe 'GET #users with unknown group ID' do
+      before do
+        get(:users, group_id: 'unknown')
+      end
+
+      it { expect(response.status).to eq(404) }
+    end
   end
 
   context 'all users' do
@@ -50,26 +74,50 @@ describe AutocompleteController do
   end
 
   context 'unauthenticated user' do
-    let(:project) { create(:project, :public) }
+    let(:public_project) { create(:project, :public) }
     let(:body) { JSON.parse(response.body) }
 
     describe 'GET #users with public project' do
       before do
-        project.team << [user, :guest]
-        get(:users, project_id: project.id)
+        public_project.team << [user, :guest]
+        get(:users, project_id: public_project.id)
       end
 
       it { expect(body).to be_kind_of(Array) }
       it { expect(body.size).to eq 1 }
     end
 
+    describe 'GET #users with project' do
+      before do
+        get(:users, project_id: project.id)
+      end
+
+      it { expect(response.status).to eq(302) }
+    end
+
+    describe 'GET #users with unknown project' do
+      before do
+        get(:users, project_id: 'unknown')
+      end
+
+      it { expect(response.status).to eq(302) }
+    end
+
+    describe 'GET #users with inaccessible group' do
+      before do
+        project.team << [user, :guest]
+        get(:users, group_id: user.namespace.id)
+      end
+
+      it { expect(response.status).to eq(302) }
+    end
+
     describe 'GET #users with no project' do
       before do
         get(:users)
       end
 
-      it { expect(body).to be_kind_of(Array) }
-      it { expect(body.size).to eq 0 }
+      it { expect(response.status).to eq(302) }
     end
   end
 end