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

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 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