Merge branch 'bvl-403-for-external-auth-service' into 'master'

Render a 403 when showing an access denied message

Closes #6259

See merge request gitlab-org/gitlab-ee!5964

app/controllers/application_controller.rb

@@ -138,12 +138,17 @@ class ApplicationController < ActionController::Base
   end
 
   def access_denied!(message = nil)
+    # If we display a custom access denied message to the user, we don't want to
+    # hide existence of the resource, rather tell them they cannot access it using
+    # the provided message
+    status = message.present? ? :forbidden : :not_found
+
     respond_to do |format|
-      format.any { head :not_found }
+      format.any { head status }
       format.html do
         render "errors/access_denied",
                layout: "errors",
-               status: 404,
+               status: status,
                locals: { message: message }
       end
     end

ee/changelogs/unreleased/bvl-403-for-external-auth-service.yml

+---
+title: Render a 403 when showing an access denied message
+merge_request: 5964
+author:
+type: fixed

ee/spec/controllers/boards/issues_controller_spec.rb

@@ -96,10 +96,10 @@ describe Boards::IssuesController do
         enable_external_authorization_service_check
       end
 
-      it 'returns a 404 for group boards' do
+      it 'returns a 403 for group boards' do
         get :index, board_id: board
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'is successful for project boards' do

ee/spec/controllers/ee/projects/application_controller_spec.rb

@@ -31,13 +31,21 @@ describe EE::Projects::ApplicationController do
       expect(response).to have_gitlab_http_status(200)
     end
 
-    it 'renders a 404 when the service denies access to the project' do
+    it 'renders a 403 when the service denies access to the project' do
       external_service_deny_access(user, project)
 
       get :show, namespace_id: project.namespace.to_param, id: project.to_param
 
-      expect(response).to have_gitlab_http_status(404)
+      expect(response).to have_gitlab_http_status(403)
       expect(response.body).to match("External authorization denied access to this project")
     end
+
+    it 'renders a 404 when the user cannot see the project at all' do
+      other_project = create(:project, :private)
+
+      get :show, namespace_id: other_project.namespace.to_param, id: other_project.to_param
+
+      expect(response).to have_gitlab_http_status(404)
+    end
   end
 end

ee/spec/controllers/groups/groups_controller_spec.rb

@@ -26,7 +26,7 @@ describe GroupsController do
       it 'does not allow other formats' do
         get :show, id: group.to_param, format: :atom
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
     end
 

ee/spec/controllers/search_controller_spec.rb

@@ -17,10 +17,10 @@ describe SearchController do
     end
 
     describe 'GET #show' do
-      it 'renders a 404 when no project is given' do
+      it 'renders a 403 when no project is given' do
         get :show, scope: 'notes', search: note.note
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'renders a 200 when a project was set' do
@@ -31,10 +31,10 @@ describe SearchController do
     end
 
     describe 'GET #autocomplete' do
-      it 'renders a 404 when no project is given' do
+      it 'renders a 403 when no project is given' do
         get :autocomplete, term: 'hello'
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'renders a 200 when a project was set' do

ee/spec/support/shared_examples/controllers/external_authorization_service_shared_examples.rb

@@ -14,7 +14,7 @@ shared_examples 'disabled when using an external authorization service' do
 
     subject
 
-    expect(response).to have_gitlab_http_status(404)
+    expect(response).to have_gitlab_http_status(403)
   end
 end
 
@@ -35,6 +35,6 @@ shared_examples 'unauthorized when external service denies access' do
 
     subject
 
-    expect(response).to have_gitlab_http_status(404)
+    expect(response).to have_gitlab_http_status(403)
   end
 end

spec/controllers/application_controller_spec.rb

@@ -477,4 +477,28 @@ describe ApplicationController do
       end
     end
   end
+
+  describe '#access_denied' do
+    controller(described_class) do
+      def index
+        access_denied!(params[:message])
+      end
+    end
+
+    before do
+      sign_in user
+    end
+
+    it 'renders a 404 without a message' do
+      get :index
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    it 'renders a 403 when a message is passed to access denied' do
+      get :index, message: 'None shall pass'
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+  end
 end

spec/controllers/concerns/controller_with_cross_project_access_check_spec.rb

@@ -43,13 +43,13 @@ describe ControllerWithCrossProjectAccessCheck do
         end
       end
 
-      it 'renders a 404 with trying to access a cross project page' do
+      it 'renders a 403 with trying to access a cross project page' do
         message = "This page is unavailable because you are not allowed to read "\
                   "information across multiple projects."
 
         get :index
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
         expect(response.body).to match(/#{message}/)
       end
 
@@ -119,7 +119,7 @@ describe ControllerWithCrossProjectAccessCheck do
 
         get :index
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'is executed when the `unless` condition returns true' do
@@ -127,19 +127,19 @@ describe ControllerWithCrossProjectAccessCheck do
 
         get :index
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'does not skip the check on an action that is not skipped' do
         get :show, id: 'hello'
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it 'does not skip the check on an action that was not defined to skip' do
         get :edit, id: 'hello'
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(403)
       end
     end
   end

spec/controllers/search_controller_spec.rb

@@ -32,7 +32,7 @@ describe SearchController do
     it 'still blocks searches without a project_id' do
       get :show, search: 'hello'
 
-      expect(response).to have_gitlab_http_status(404)
+      expect(response).to have_gitlab_http_status(403)
     end
 
     it 'allows searches with a project_id' do