From 4bf61b8bd4b04eace6d0f205573f15fc9d981682 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 12 Dec 2016 08:43:56 +0000
Subject: [PATCH] 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 --------------
...7-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)}(?<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
--
GitLab