diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index be00a219205de287be0729417a12374b63cfc682..707eddd4d2991138ee6fb9d2f279e25e8ba48416 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 875e983448714ca1c0ca9993e6c6c5fe8eb0bb22..4359f1d7b06ed6852eed6d1139fe553859e35596 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 7fe92051037649e93dd34afbc32bb90e650ddcd1..738c96e4db3d3205d6c7e1c8e4a68d53feaf92b4 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 0000000000000000000000000000000000000000..db1389e202407c0826583fa7e4280e5833938951
--- /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 f5fddab357df5c64d1d8db5f4a88bd33f59e7368..c1d1eca9116e1076e3e360490587c529fc320ae2 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 735e0ef6108a6945ba466bc04d73378f7955d5e8..5c3e724746b069db1bc79bf401820e1f9d18f665 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 7f69e888f3249242c8ef22c8aa1d944dd7c5c364..97737d7ddc730a81d3263a6251a87dba5f3b8e70 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 24e216329a9da98f11454aa29fbd9c0b71742d19..bb56e44db296c68cc6cf37ad980e538eacbaacc8 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 0cc2efae5f9b86617716fa685746e3a0470b8e63..064f29d2d66a8a6dac340e23a4a1215c0b1b5fa2 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