Commit 4bf61b8b authored by Sean McGivern's avatar Sean McGivern Committed by Alejandro Rodríguez

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
parent 12db4cc0
...@@ -23,10 +23,26 @@ class IssuesFinder < IssuableFinder ...@@ -23,10 +23,26 @@ class IssuesFinder < IssuableFinder
private private
def init_collection def init_collection
Issue.visible_to_user(current_user) IssuesFinder.not_restricted_by_confidentiality(current_user)
end end
def iid_pattern def iid_pattern
@iid_pattern ||= %r{\A#{Regexp.escape(Issue.reference_prefix)}(?<iid>\d+)\z} @iid_pattern ||= %r{\A#{Regexp.escape(Issue.reference_prefix)}(?<iid>\d+)\z}
end 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 end
...@@ -30,7 +30,7 @@ module Milestoneish ...@@ -30,7 +30,7 @@ module Milestoneish
end end
def issues_visible_to_user(user) def issues_visible_to_user(user)
issues.visible_to_user(user) IssuesFinder.new(user).execute.where(id: issues)
end end
def upcoming? def upcoming?
......
...@@ -60,61 +60,6 @@ class Issue < ActiveRecord::Base ...@@ -60,61 +60,6 @@ class Issue < ActiveRecord::Base
attributes attributes
end 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 def self.reference_prefix
'#' '#'
end end
......
---
title: Issue#visible_to_user moved to IssuesFinder to prevent accidental use
merge_request:
author:
...@@ -131,5 +131,7 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps ...@@ -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: 'bug')
issue.labels << project.labels.find_by(title: 'feature') issue.labels << project.labels.find_by(title: 'feature')
end end
current_user.refresh_authorized_projects
end end
end end
...@@ -33,6 +33,6 @@ module SharedAuthentication ...@@ -33,6 +33,6 @@ module SharedAuthentication
end end
def current_user def current_user
@user || User.first @user || User.reorder(nil).first
end end
end end
...@@ -10,8 +10,13 @@ describe IssuesFinder do ...@@ -10,8 +10,13 @@ describe IssuesFinder do
let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone, title: 'gitlab') } 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(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'gitlab') }
let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2) } let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2) }
describe '#execute' do
let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') } let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') }
let!(:label_link) { create(:label_link, label: label, target: issue2) } 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 before do
project1.team << [user, :master] project1.team << [user, :master]
...@@ -23,11 +28,6 @@ describe IssuesFinder do ...@@ -23,11 +28,6 @@ describe IssuesFinder do
issue3 issue3
end end
describe '#execute' do
let(:search_user) { user }
let(:params) { {} }
let(:issues) { IssuesFinder.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute }
context 'scope: all' do context 'scope: all' do
let(:scope) { 'all' } let(:scope) { 'all' }
...@@ -193,6 +193,15 @@ describe IssuesFinder do ...@@ -193,6 +193,15 @@ describe IssuesFinder do
expect(issues).to contain_exactly(issue2, issue3) expect(issues).to contain_exactly(issue2, issue3)
end end
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 end
context 'personal scope' do context 'personal scope' do
...@@ -210,5 +219,43 @@ describe IssuesFinder do ...@@ -210,5 +219,43 @@ describe IssuesFinder do
end end
end 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
end end
...@@ -22,26 +22,6 @@ describe Issue, models: true do ...@@ -22,26 +22,6 @@ describe Issue, models: true do
it { is_expected.to have_db_index(:deleted_at) } it { is_expected.to have_db_index(:deleted_at) }
end 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 describe '#to_reference' do
let(:project) { build(:empty_project, name: 'sample-project') } let(:project) { build(:empty_project, name: 'sample-project') }
let(:issue) { build(:issue, iid: 1, project: project) } let(:issue) { build(:issue, iid: 1, project: project) }
......
...@@ -24,8 +24,9 @@ describe Milestone, models: true do ...@@ -24,8 +24,9 @@ describe Milestone, models: true do
it { is_expected.to have_many(:issues) } it { is_expected.to have_many(:issues) }
end end
let(:milestone) { create(:milestone) } let(:project) { create(:project, :public) }
let(:issue) { create(:issue) } let(:milestone) { create(:milestone, project: project) }
let(:issue) { create(:issue, project: project) }
let(:user) { create(:user) } let(:user) { create(:user) }
describe "#title" do describe "#title" do
...@@ -110,8 +111,8 @@ describe Milestone, models: true do ...@@ -110,8 +111,8 @@ describe Milestone, models: true do
describe :items_count do describe :items_count do
before do before do
milestone.issues << create(:issue) milestone.issues << create(:issue, project: project)
milestone.issues << create(:closed_issue) milestone.issues << create(:closed_issue, project: project)
milestone.merge_requests << create(:merge_request) milestone.merge_requests << create(:merge_request)
end end
...@@ -126,7 +127,7 @@ describe Milestone, models: true do ...@@ -126,7 +127,7 @@ describe Milestone, models: true do
describe '#total_items_count' do describe '#total_items_count' do
before do before do
create :closed_issue, milestone: milestone create :closed_issue, milestone: milestone, project: project
create :merge_request, milestone: milestone create :merge_request, milestone: milestone
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment