diff options
-rw-r--r-- | app/finders/issues_finder.rb | 18 | ||||
-rw-r--r-- | app/models/concerns/milestoneish.rb | 2 | ||||
-rw-r--r-- | app/models/issue.rb | 55 | ||||
-rw-r--r-- | changelogs/unreleased/jej-24637-move-issue-visible_to_user-to-finder.yml | 4 | ||||
-rw-r--r-- | features/steps/group/milestones.rb | 2 | ||||
-rw-r--r-- | features/steps/shared/authentication.rb | 2 | ||||
-rw-r--r-- | spec/finders/issues_finder_spec.rb | 71 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 20 | ||||
-rw-r--r-- | spec/models/milestone_spec.rb | 11 |
9 files changed, 90 insertions, 95 deletions
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)}(?<iid>\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 875e9834487..4359f1d7b06 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -30,7 +30,7 @@ module Milestoneish end def issues_visible_to_user(user) - issues.visible_to_user(user) + IssuesFinder.new(user).execute.where(id: issues) end def upcoming? diff --git a/app/models/issue.rb b/app/models/issue.rb index 7fe92051037..738c96e4db3 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.team.member?(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 24e216329a9..bb56e44db29 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 let(:project) { build(:empty_project, name: 'sample-project') } let(:issue) { build(:issue, iid: 1, project: project) } diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 0cc2efae5f9..064f29d2d66 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -24,8 +24,9 @@ describe Milestone, models: true do it { is_expected.to have_many(:issues) } 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 @@ -110,8 +111,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 @@ -126,7 +127,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 |