From 34ee75379cf8e6459b8926fbf956a8316f87eea7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 17 Mar 2016 16:38:51 -0300 Subject: [PATCH] Restrict access to confidential issues --- app/controllers/projects/issues_controller.rb | 6 +- app/finders/issues_finder.rb | 6 + app/models/ability.rb | 17 +- app/models/issue.rb | 7 + .../projects/issues_controller_spec.rb | 164 +++++++++++++++++- spec/factories/issues.rb | 4 + 6 files changed, 193 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0907733fe42..6603f28a082 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 20a2b0ce8f0..c2befa5a5b3 100644 --- a/app/finders/issues_finder.rb +++ b/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 diff --git a/app/models/ability.rb b/app/models/ability.rb index ccac08b7d3f..e22da4806e6 100644 --- a/app/models/ability.rb +++ b/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 diff --git a/app/models/issue.rb b/app/models/issue.rb index 2447f860c5a..053387cffd7 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 76d56bc989d..2cd81231144 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1,16 +1,16 @@ 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 + 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 diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb index 722095de590..e72aa9479b7 100644 --- a/spec/factories/issues.rb +++ b/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