Restrict access to confidential issues

app/controllers/projects/issues_controller.rb

@@ -5,7 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :issue, only: [:edit, :update, :show]
   # Allow read any issue
-  before_action :authorize_read_issue!
+  before_action :authorize_read_issue!, only: [:show]
   # Allow write(create) issue
   before_action :authorize_create_issue!, only: [:new, :create]
@@ -128,6 +128,10 @@ class Projects::IssuesController < Projects::ApplicationController
   end
   alias_method :subscribable_resource, :issue
+  def authorize_read_issue!
+    return render_404 unless can?(current_user, :read_issue, @issue)
+  end
+
   def authorize_update_issue!
     return render_404 unless can?(current_user, :update_issue, @issue)
   end

app/finders/issues_finder.rb

@@ -19,4 +19,10 @@ class IssuesFinder < IssuableFinder
   def klass
     Issue
   end
+
+  private
+
+  def init_collection
+    Issue.visible_to_user(current_user)
+  end
 end

app/models/ability.rb

@@ -49,7 +49,6 @@ class Ability
         rules = [
           :read_project,
           :read_wiki,
-          :read_issue,
           :read_label,
           :read_milestone,
           :read_project_snippet,
@@ -63,6 +62,9 @@ class Ability
         # Allow to read builds by anonymous user if guests are allowed
         rules << :read_build if project.public_builds?
+        # Allow to read issues by anonymous user if issue is not confidential
+        rules << :read_issue unless subject.is_a?(Issue) && subject.confidential?
+
         rules - project_disabled_features_rules(project)
       else
         []
@@ -321,6 +323,7 @@ class Ability
         end
         rules += project_abilities(user, subject.project)
+        rules = filter_confidential_issues_abilities(user, subject, rules) if subject.is_a?(Issue)
         rules
       end
     end
@@ -439,5 +442,17 @@ class Ability
         :"admin_#{name}"
       ]
     end
+
+    def filter_confidential_issues_abilities(user, issue, rules)
+      return rules if user.admin? || !issue.confidential?
+
+      unless issue.author == user || issue.assignee == user || issue.project.team.member?(user.id)
+        rules.delete(:admin_issue)
+        rules.delete(:read_issue)
+        rules.delete(:update_issue)
+      end
+
+      rules
+    end
   end
 end

app/models/issue.rb

@@ -58,6 +58,13 @@ class Issue < ActiveRecord::Base
     attributes
   end
+  def self.visible_to_user(user)
+    return where(confidential: false) if user.blank?
+    return all if user.admin?
+
+    where('issues.confidential = false OR (issues.confidential = true AND (issues.author_id = :user_id OR issues.assignee_id = :user_id OR issues.project_id IN(:project_ids)))', user_id: user.id, project_ids: user.authorized_projects.select(:id))
+  end
+
   def self.reference_prefix
     '#'
   end

spec/controllers/projects/issues_controller_spec.rb

 require('spec_helper')
 describe Projects::IssuesController do
-  let(:project) { create(:project) }
-  let(:user)    { create(:user) }
-  let(:issue) { create(:issue, project: project) }
-  before do
-    sign_in(user)
-    project.team << [user, :developer]
-  end
-  describe "GET #index" do
+  describe "GET #index" do
+    let(:project) { create(:project) }
+    let(:user) { create(:user) }
+    let(:issue) { create(:issue, project: project) }
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
     it "returns index" do
       get :index, namespace_id: project.namespace.path, project_id: project.path
@@ -38,6 +38,152 @@ describe Projects::IssuesController do
       get :index, namespace_id: project.namespace.path, project_id: project.path
       expect(response.status).to eq(404)
     end
+  end
+
+  describe 'Confidential Issues' do
+    let(:project) { create(:empty_project, :public) }
+    let(:assignee) { create(:assignee) }
+    let(:author) { create(:user) }
+    let(:non_member) { create(:user) }
+    let(:member) { create(:user) }
+    let(:admin) { create(:admin) }
+    let!(:issue) { create(:issue, project: project) }
+    let!(:unescaped_parameter_value) { create(:issue, :confidential, project: project, author: author) }
+    let!(:request_forgery_timing_attack) { create(:issue, :confidential, project: project, assignee: assignee) }
+
+    describe 'GET #index' do
+      it 'should not list confidential issues for guests' do
+        sign_out(:user)
+        get_issues
+
+        expect(assigns(:issues)).to eq [issue]
+      end
+
+      it 'should not list confidential issues for non project members' do
+        sign_in(non_member)
+        get_issues
+
+        expect(assigns(:issues)).to eq [issue]
+      end
+
+      it 'should list confidential issues for author' do
+        sign_in(author)
+        get_issues
+
+        expect(assigns(:issues)).to include unescaped_parameter_value
+        expect(assigns(:issues)).not_to include request_forgery_timing_attack
+      end
+
+      it 'should list confidential issues for assignee' do
+        sign_in(assignee)
+        get_issues
+
+        expect(assigns(:issues)).not_to include unescaped_parameter_value
+        expect(assigns(:issues)).to include request_forgery_timing_attack
+      end
+
+      it 'should list confidential issues for project members' do
+        sign_in(member)
+        project.team << [member, :developer]
+
+        get_issues
+
+        expect(assigns(:issues)).to include unescaped_parameter_value
+        expect(assigns(:issues)).to include request_forgery_timing_attack
+      end
+
+      it 'should list confidential issues for admin' do
+        sign_in(admin)
+        get_issues
+
+        expect(assigns(:issues)).to include unescaped_parameter_value
+        expect(assigns(:issues)).to include request_forgery_timing_attack
+      end
+
+      def get_issues
+        get :index,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param
+      end
+    end
+    shared_examples_for 'restricted action' do |http_status|
+      it 'returns 404 for guests' do
+        sign_out :user
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status :not_found
+      end
+
+      it 'returns 404 for non project members' do
+        sign_in(non_member)
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status :not_found
+      end
+
+      it "returns #{http_status[:success]} for author" do
+        sign_in(author)
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status http_status[:success]
+      end
+
+      it "returns #{http_status[:success]} for assignee" do
+        sign_in(assignee)
+        go(id: request_forgery_timing_attack.to_param)
+
+        expect(response).to have_http_status http_status[:success]
+      end
+
+      it "returns #{http_status[:success]} for project members" do
+        sign_in(member)
+        project.team << [member, :developer]
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status http_status[:success]
+      end
+
+      it "returns #{http_status[:success]} for admin" do
+        sign_in(admin)
+        go(id: unescaped_parameter_value.to_param)
+
+        expect(response).to have_http_status http_status[:success]
+      end
+    end
+
+    describe 'GET #show' do
+      it_behaves_like 'restricted action', success: 200
+
+      def go(id:)
+        get :show,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: id
+      end
+    end
+
+    describe 'GET #edit' do
+      it_behaves_like 'restricted action', success: 200
+
+      def go(id:)
+        get :edit,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: id
+      end
+    end
+
+    describe 'PUT #update' do
+      it_behaves_like 'restricted action', success: 302
+
+      def go(id:)
+        put :update,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: id,
+          issue: { title: 'New title' }
+      end
+    end
   end
 end

spec/factories/issues.rb

@@ -4,6 +4,10 @@ FactoryGirl.define do
     author
     project
+    trait :confidential do
+      confidential true
+    end
+
     trait :closed do
       state :closed
     end