From d5ec0786c2043f9bf54eeaa33fd2fe97b4585fff Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 12 Dec 2016 08:43:56 +0000 Subject: Merge branch 'jej-24637-move-issue-visible_to_user-to-finder' into 'security' Issue#visible_to_user moved to IssuesFinder Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/24637. See merge request !2039 --- app/finders/issues_finder.rb | 18 +++++- app/models/concerns/milestoneish.rb | 2 +- app/models/issue.rb | 55 ----------------- ...-24637-move-issue-visible_to_user-to-finder.yml | 4 ++ features/steps/group/milestones.rb | 2 + features/steps/shared/authentication.rb | 2 +- spec/finders/issues_finder_spec.rb | 71 ++++++++++++++++++---- spec/models/issue_spec.rb | 20 ------ spec/models/milestone_spec.rb | 11 ++-- 9 files changed, 90 insertions(+), 95 deletions(-) create mode 100644 changelogs/unreleased/jej-24637-move-issue-visible_to_user-to-finder.yml diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index be00a219205..707eddd4d29 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -23,10 +23,26 @@ class IssuesFinder < IssuableFinder private def init_collection - Issue.visible_to_user(current_user) + IssuesFinder.not_restricted_by_confidentiality(current_user) end def iid_pattern @iid_pattern ||= %r{\A#{Regexp.escape(Issue.reference_prefix)}(?\d+)\z} end + + def self.not_restricted_by_confidentiality(user) + return Issue.where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank? + + return Issue.all if user.admin? + + Issue.where(' + issues.confidential IS NULL + OR issues.confidential IS 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(Gitlab::Access::REPORTER).select(:id)) + end end diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 6b2cb9da0b6..99f17d668e5 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -24,6 +24,6 @@ module Milestoneish end def issues_visible_to_user(user) - issues.visible_to_user(user) + IssuesFinder.new(user).execute.where(id: issues) end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 544f830cc69..b19d82a322a 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -60,61 +60,6 @@ class Issue < ActiveRecord::Base attributes end - class << self - private - - # Returns the project that the current scope belongs to if any, nil otherwise. - # - # Examples: - # - my_project.issues.without_due_date.owner_project => my_project - # - Issue.all.owner_project => nil - def owner_project - # No owner if we're not being called from an association - return unless all.respond_to?(:proxy_association) - - owner = all.proxy_association.owner - - # Check if the association is or belongs to a project - if owner.is_a?(Project) - owner - else - begin - owner.association(:project).target - rescue ActiveRecord::AssociationNotFoundError - nil - end - end - end - end - - def self.visible_to_user(user) - return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank? - return all if user.admin? - - # Check if we are scoped to a specific project's issues - if owner_project - if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER) - # If the project is authorized for the user, they can see all issues in the project - return all - else - # else only non confidential and authored/assigned to them - return where('issues.confidential IS NULL OR issues.confidential IS FALSE - OR issues.author_id = :user_id OR issues.assignee_id = :user_id', - user_id: user.id) - end - end - - where(' - issues.confidential IS NULL - OR issues.confidential IS 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(Gitlab::Access::REPORTER).select(:id)) - end - def self.reference_prefix '#' end diff --git a/changelogs/unreleased/jej-24637-move-issue-visible_to_user-to-finder.yml b/changelogs/unreleased/jej-24637-move-issue-visible_to_user-to-finder.yml new file mode 100644 index 00000000000..db1389e2024 --- /dev/null +++ b/changelogs/unreleased/jej-24637-move-issue-visible_to_user-to-finder.yml @@ -0,0 +1,4 @@ +--- +title: Issue#visible_to_user moved to IssuesFinder to prevent accidental use +merge_request: +author: diff --git a/features/steps/group/milestones.rb b/features/steps/group/milestones.rb index f5fddab357d..c1d1eca9116 100644 --- a/features/steps/group/milestones.rb +++ b/features/steps/group/milestones.rb @@ -131,5 +131,7 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps issue.labels << project.labels.find_by(title: 'bug') issue.labels << project.labels.find_by(title: 'feature') end + + current_user.refresh_authorized_projects end end diff --git a/features/steps/shared/authentication.rb b/features/steps/shared/authentication.rb index 735e0ef6108..5c3e724746b 100644 --- a/features/steps/shared/authentication.rb +++ b/features/steps/shared/authentication.rb @@ -33,6 +33,6 @@ module SharedAuthentication end def current_user - @user || User.first + @user || User.reorder(nil).first end end diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 7f69e888f32..97737d7ddc7 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -10,24 +10,24 @@ describe IssuesFinder do let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone, title: 'gitlab') } let(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'gitlab') } let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2) } - let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') } - let!(:label_link) { create(:label_link, label: label, target: issue2) } - - before do - project1.team << [user, :master] - project2.team << [user, :developer] - project2.team << [user2, :developer] - - issue1 - issue2 - issue3 - end describe '#execute' do + let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') } + let!(:label_link) { create(:label_link, label: label, target: issue2) } let(:search_user) { user } let(:params) { {} } let(:issues) { IssuesFinder.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute } + before do + project1.team << [user, :master] + project2.team << [user, :developer] + project2.team << [user2, :developer] + + issue1 + issue2 + issue3 + end + context 'scope: all' do let(:scope) { 'all' } @@ -193,6 +193,15 @@ describe IssuesFinder do expect(issues).to contain_exactly(issue2, issue3) end end + + it 'finds issues user can access due to group' do + group = create(:group) + project = create(:empty_project, group: group) + issue = create(:issue, project: project) + group.add_user(user, :owner) + + expect(issues).to include(issue) + end end context 'personal scope' do @@ -210,5 +219,43 @@ describe IssuesFinder do end end end + + context 'when project restricts issues' do + let(:scope) { nil } + + it "doesn't return team-only issues to non team members" do + project = create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE) + issue = create(:issue, project: project) + + expect(issues).not_to include(issue) + end + + it "doesn't return issues if feature disabled" do + [project1, project2].each do |project| + project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED) + end + + expect(issues.count).to eq 0 + end + end + end + + describe '.not_restricted_by_confidentiality' do + let(:authorized_user) { create(:user) } + let(:project) { create(:empty_project, namespace: authorized_user.namespace) } + let!(:public_issue) { create(:issue, project: project) } + let!(:confidential_issue) { create(:issue, project: project, confidential: true) } + + it 'returns non confidential issues for nil user' do + expect(IssuesFinder.send(:not_restricted_by_confidentiality, nil)).to include(public_issue) + end + + it 'returns non confidential issues for user not authorized for the issues projects' do + expect(IssuesFinder.send(:not_restricted_by_confidentiality, user)).to include(public_issue) + end + + it 'returns all issues for user authorized for the issues projects' do + expect(IssuesFinder.send(:not_restricted_by_confidentiality, authorized_user)).to include(public_issue, confidential_issue) + end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 300425767ed..82b140acb06 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -22,26 +22,6 @@ describe Issue, models: true do it { is_expected.to have_db_index(:deleted_at) } end - describe '.visible_to_user' do - let(:user) { create(:user) } - let(:authorized_user) { create(:user) } - let(:project) { create(:project, namespace: authorized_user.namespace) } - let!(:public_issue) { create(:issue, project: project) } - let!(:confidential_issue) { create(:issue, project: project, confidential: true) } - - it 'returns non confidential issues for nil user' do - expect(Issue.visible_to_user(nil).count).to be(1) - end - - it 'returns non confidential issues for user not authorized for the issues projects' do - expect(Issue.visible_to_user(user).count).to be(1) - end - - it 'returns all issues for user authorized for the issues projects' do - expect(Issue.visible_to_user(authorized_user).count).to be(2) - end - end - describe '#to_reference' do it 'returns a String reference to the object' do expect(subject.to_reference).to eq "##{subject.iid}" diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 33fe22dd98c..2ee4dbeb478 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -15,8 +15,9 @@ describe Milestone, models: true do it { is_expected.to validate_presence_of(:project) } end - let(:milestone) { create(:milestone) } - let(:issue) { create(:issue) } + let(:project) { create(:project, :public) } + let(:milestone) { create(:milestone, project: project) } + let(:issue) { create(:issue, project: project) } let(:user) { create(:user) } describe "#title" do @@ -101,8 +102,8 @@ describe Milestone, models: true do describe :items_count do before do - milestone.issues << create(:issue) - milestone.issues << create(:closed_issue) + milestone.issues << create(:issue, project: project) + milestone.issues << create(:closed_issue, project: project) milestone.merge_requests << create(:merge_request) end @@ -117,7 +118,7 @@ describe Milestone, models: true do describe '#total_items_count' do before do - create :closed_issue, milestone: milestone + create :closed_issue, milestone: milestone, project: project create :merge_request, milestone: milestone end -- cgit v1.2.1