Commit ea63346d authored by Alejandro Rodríguez's avatar Alejandro Rodríguez

Refactor user authorization check for a single project to avoid querying all user projects

Currently, even when searching for all authorized issues of *one* project, we run the
`Users#authorized_projects` query (which can be rather slow). This update checks if
we are handling issues of just one project and does the authorization check locally.
It does have the downside of basically repeating the logic of `Users#authorized_projects`
on `Project#authorized_for_user`.
parent b4717017
Pipeline #3751515 passed with stages
in 691 minutes and 5 seconds
......@@ -38,6 +38,7 @@ v 8.10.0 (unreleased)
- Display tooltip for mentioned users and groups !5261 (winniehell)
- Allow build email service to be tested
- Added day name to contribution calendar tooltips
- Refactor user authorization check for a single project to avoid querying all user projects
- Make images fit to the size of the viewport !4810
- Fix check for New Branch button on Issue page !4630 (winniehell)
- Fix GFM autocomplete not working on wiki pages
......
......@@ -52,10 +52,50 @@ 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
......
......@@ -1210,4 +1210,44 @@ class Project < ActiveRecord::Base
{ key: variable.key, value: variable.value, public: false }
end
end
# Checks if `user` is authorized for this project, with at least the
# `min_access_level` (if given).
#
# If you change the logic of this method, please also update `User#authorized_projects`
def authorized_for_user?(user, min_access_level = nil)
return false unless user
return true if personal? && namespace_id == user.namespace_id
authorized_for_user_by_group?(user, min_access_level) ||
authorized_for_user_by_members?(user, min_access_level) ||
authorized_for_user_by_shared_projects?(user, min_access_level)
end
private
def authorized_for_user_by_group?(user, min_access_level)
member = user.group_members.find_by(source_id: group)
member && (!min_access_level || member.access_level >= min_access_level)
end
def authorized_for_user_by_members?(user, min_access_level)
member = members.find_by(user_id: user)
member && (!min_access_level || member.access_level >= min_access_level)
end
def authorized_for_user_by_shared_projects?(user, min_access_level)
shared_projects = user.group_members.joins(group: :shared_projects).
where(project_group_links: { project_id: self })
if min_access_level
members_scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } }
shared_projects = shared_projects.where(members: members_scope)
end
shared_projects.any?
end
end
......@@ -412,6 +412,8 @@ class User < ActiveRecord::Base
end
# Returns projects user is authorized to access.
#
# If you change the logic of this method, please also update `Project#authorized_for_user`
def authorized_projects(min_access_level = nil)
Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})")
end
......
......@@ -22,6 +22,26 @@ 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}"
......
......@@ -1187,4 +1187,53 @@ describe Project, models: true do
end
end
end
describe 'authorized_for_user' do
let(:group) { create(:group) }
let(:developer) { create(:user) }
let(:master) { create(:user) }
let(:personal_project) { create(:project, namespace: developer.namespace) }
let(:group_project) { create(:project, namespace: group) }
let(:members_project) { create(:project) }
let(:shared_project) { create(:project) }
before do
group.add_master(master)
group.add_developer(developer)
members_project.team << [developer, :developer]
members_project.team << [master, :master]
create(:project_group_link, project: shared_project, group: group)
end
it 'returns false for no user' do
expect(personal_project.authorized_for_user?(nil)).to be(false)
end
it 'returns true for personal projects of the user' do
expect(personal_project.authorized_for_user?(developer)).to be(true)
end
it 'returns true for projects of groups the user is a member of' do
expect(group_project.authorized_for_user?(developer)).to be(true)
end
it 'returns true for projects for which the user is a member of' do
expect(members_project.authorized_for_user?(developer)).to be(true)
end
it 'returns true for projects shared on a group the user is a member of' do
expect(shared_project.authorized_for_user?(developer)).to be(true)
end
it 'checks for the correct minimum level access' do
expect(group_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
expect(group_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
expect(members_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
expect(members_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
expect(shared_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
expect(shared_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
end
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