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]
@@ -133,6 +133,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/issuable_finder.rb

@@ -40,6 +40,7 @@ class IssuableFinder
     items = by_author(items)
     items = by_label(items)
     items = by_weight(items)
+    items = by_confidentiality(items)
     sort(items)
   end
 
@@ -308,6 +309,26 @@ class IssuableFinder
     params[:weight] == Issue::WEIGHT_ANY
   end
 
+  def by_confidentiality(items)
+    return items unless klass == Issue
+
+    if current_user
+      if current_user.admin? || project.team.member?(current_user.id)
+        items
+      else
+        issuable_table = items.arel_table
+
+        items.where(
+          issuable_table[:confidential].eq(false).or(
+            issuable_table[:confidential].eq(true).and(issuable_table[:author_id].eq(current_user.id))
+          )
+        )
+      end
+    else
+      items.not_confidential
+    end
+  end
+
   def label_names
     params[:label_name].split(',')
   end

app/models/ability.rb

@@ -43,6 +43,8 @@ class Ability
         anonymous_personal_snippet_abilities(subject)
       when subject.is_a?(CommitStatus)
         anonymous_commit_status_abilities(subject)
+      when subject.is_a?(Issue)
+        anonymous_issue_abilities(subject)
       when subject.is_a?(Project) || subject.respond_to?(:project)
         anonymous_project_abilities(subject)
       when subject.is_a?(Group) || subject.respond_to?(:group)
@@ -52,6 +54,12 @@ class Ability
       end
     end
 
+    def anonymous_issue_abilities(subject)
+      rules = anonymous_project_abilities(subject)
+      rules -= confidential_issue_rules if subject.confidential?
+      rules
+    end
+
     def anonymous_project_abilities(subject)
       project = if subject.is_a?(Project)
                   subject
@@ -343,6 +351,13 @@ class Ability
         end
 
         rules += project_abilities(user, subject.project)
+
+        if subject.respond_to?(:confidential) && subject.confidential?
+          unless user.admin? || subject.author == user || subject.project.team.member?(user.id)
+            rules -= confidential_issue_rules
+          end
+        end
+
         rules
       end
     end
@@ -461,5 +476,12 @@ class Ability
         :"admin_#{name}"
       ]
     end
+
+    def confidential_issue_rules
+      [
+        :read_issue,
+        :update_issue
+      ]
+    end
   end
 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) }
+  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
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
 
-  describe "GET #index" do
     it "returns index" do
       get :index, namespace_id: project.namespace.path, project_id: project.path
 
@@ -38,6 +38,136 @@ 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(: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) }
+
+    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 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 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