diff --git a/CHANGELOG b/CHANGELOG
index c15077afb08f1651ca22f92be7e6f195d63cedde..2a9cfa84ad4d5135df3c789591a9bcfeb2b09d25 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 60abd47409e5bc51dbab480bbeb4192f44723814..60af8c15340af52567cb7f0d40407a3659ccce43 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -52,10 +52,50 @@ def hook_attrs
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
diff --git a/app/models/project.rb b/app/models/project.rb
index c96de0f6c72c4f47620923de26aa7c6c2abbae81..d6191e80e62b122bdba10e64bef6e31e30eaf1c5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1210,4 +1210,44 @@ def secret_variables
{ 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
diff --git a/app/models/user.rb b/app/models/user.rb
index 975e935fa2049e3f11a822f595f7027ab2a9f6b5..8dc10becd696febd545fb23d7fb5a20b342e9905 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -412,6 +412,8 @@ def authorized_groups
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
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index b87d68283e6cb279c14ce737def53791ea39acf9..6a897c9669049ef43b567f15eccd4c9442bb4c48 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -22,6 +22,26 @@
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}"
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index e3e7319beb2bec45d4c68f33b89544f512cb24d7..a2b089c51e2534b203eacca370fa13b02f592368 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1187,4 +1187,53 @@
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