Commit d5ec0786 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 86dda614
......@@ -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
......@@ -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
......@@ -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
......
---
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
issue.labels << project.labels.find_by(title: 'bug')
issue.labels << project.labels.find_by(title: 'feature')
end
current_user.refresh_authorized_projects
end
end
......@@ -33,6 +33,6 @@ module SharedAuthentication
end
def current_user
@user || User.first
@user || User.reorder(nil).first
end
end
......@@ -10,8 +10,13 @@ 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) }
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]
......@@ -23,11 +28,6 @@ describe IssuesFinder do
issue3
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
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
......@@ -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}"
......
......@@ -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
......
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