Commit 68e499ed authored by Yorick Peterse's avatar Yorick Peterse Committed by Rémy Coutable

Merge branch '18586-user-authorized_projects-is-slow' into 'master'

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

See merge request !5102
parent 7c0d0039
...@@ -40,6 +40,7 @@ v 8.10.0 (unreleased) ...@@ -40,6 +40,7 @@ v 8.10.0 (unreleased)
- Display tooltip for mentioned users and groups !5261 (winniehell) - Display tooltip for mentioned users and groups !5261 (winniehell)
- Allow build email service to be tested - Allow build email service to be tested
- Added day name to contribution calendar tooltips - 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 - Make images fit to the size of the viewport !4810
- Fix check for New Branch button on Issue page !4630 (winniehell) - Fix check for New Branch button on Issue page !4630 (winniehell)
- Fix GFM autocomplete not working on wiki pages - Fix GFM autocomplete not working on wiki pages
......
...@@ -52,10 +52,50 @@ class Issue < ActiveRecord::Base ...@@ -52,10 +52,50 @@ 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) def self.visible_to_user(user)
return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank? return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank?
return all if user.admin? 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(' where('
issues.confidential IS NULL issues.confidential IS NULL
OR issues.confidential IS FALSE OR issues.confidential IS FALSE
......
...@@ -1221,4 +1221,44 @@ class Project < ActiveRecord::Base ...@@ -1221,4 +1221,44 @@ class Project < ActiveRecord::Base
{ key: variable.key, value: variable.value, public: false } { key: variable.key, value: variable.value, public: false }
end end
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 end
...@@ -412,6 +412,8 @@ class User < ActiveRecord::Base ...@@ -412,6 +412,8 @@ class User < ActiveRecord::Base
end end
# Returns projects user is authorized to access. # 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) def authorized_projects(min_access_level = nil)
Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})") Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})")
end end
......
...@@ -22,6 +22,26 @@ describe Issue, models: true do ...@@ -22,6 +22,26 @@ 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
it 'returns a String reference to the object' do it 'returns a String reference to the object' do
expect(subject.to_reference).to eq "##{subject.iid}" expect(subject.to_reference).to eq "##{subject.iid}"
......
...@@ -1276,4 +1276,53 @@ describe Project, models: true do ...@@ -1276,4 +1276,53 @@ describe Project, models: true do
end end
end 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 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