diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/controllers/projects/issues_controller.rb | 3 | ||||
| -rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 3 | ||||
| -rw-r--r-- | app/helpers/notes_helper.rb | 15 | ||||
| -rw-r--r-- | app/models/ability.rb | 14 | ||||
| -rw-r--r-- | app/models/project_team.rb | 46 | ||||
| -rw-r--r-- | lib/gitlab/access.rb | 1 | ||||
| -rw-r--r-- | spec/helpers/notes_helper_spec.rb | 57 | ||||
| -rw-r--r-- | spec/models/ability_spec.rb | 56 | ||||
| -rw-r--r-- | spec/models/project_team_spec.rb | 51 |
10 files changed, 194 insertions, 53 deletions
diff --git a/CHANGELOG b/CHANGELOG index 5ff0cb42ccc..fb38db15630 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,7 @@ v 8.11.0 (unreleased) - Fix CI status icon link underline (ClemMakesApps) - Fix of 'Commits being passed to custom hooks are already reachable when using the UI' - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable + - Optimize maximum user access level lookup in loading of notes - Limit git rev-list output count to one in forced push check - Add green outline to New Branch button. !5447 (winniehell) - Retrieve rendered HTML from cache in one request diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index fa663c9bda4..16ed7c2b6b4 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -1,4 +1,5 @@ class Projects::IssuesController < Projects::ApplicationController + include NotesHelper include ToggleSubscriptionAction include IssuableActions include ToggleAwardEmoji @@ -70,6 +71,8 @@ class Projects::IssuesController < Projects::ApplicationController @note = @project.notes.new(noteable: @issue) @noteable = @issue + preload_max_access_for_authors(@notes, @project) if @notes + respond_to do |format| format.html format.json do diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 594a61464b9..da1b9c3e48a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -3,6 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController include DiffForPath include DiffHelper include IssuableActions + include NotesHelper include ToggleAwardEmoji before_action :module_enabled @@ -385,6 +386,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController @project_wiki, @ref ) + + preload_max_access_for_authors(@notes, @project) if @notes end def define_widget_vars diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 0f60dd828ab..0c47abe0fba 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -7,7 +7,7 @@ module NotesHelper end def note_editable?(note) - note.editable? && can?(current_user, :admin_note, note) + Ability.can_edit_note?(current_user, note) end def noteable_json(noteable) @@ -87,14 +87,13 @@ module NotesHelper end end - def note_max_access_for_user(note) - @max_access_by_user_id ||= Hash.new do |hash, key| - project = key[:project] - hash[key] = project.team.human_max_access(key[:user_id]) - end + def preload_max_access_for_authors(notes, project) + user_ids = notes.map(&:author_id) + project.team.max_member_access_for_user_ids(user_ids) + end - full_key = { project: note.project, user_id: note.author_id } - @max_access_by_user_id[full_key] + def note_max_access_for_user(note) + note.project.team.human_max_access(note.author_id) end def discussion_diff_path(discussion) diff --git a/app/models/ability.rb b/app/models/ability.rb index f33c8d61d3f..6884d99c5a6 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -388,6 +388,20 @@ class Ability GroupProjectsFinder.new(group).execute(user).any? end + def can_edit_note?(user, note) + return false unless note.editable? + return false unless user.present? + return true if note.author == user + return true if user.admin? + + if note.project + max_access_level = note.project.team.max_member_access(user.id) + max_access_level >= Gitlab::Access::MASTER + else + false + end + end + def namespace_abilities(user, namespace) rules = [] diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 9d312a53790..67faea1f9f3 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -132,22 +132,41 @@ class ProjectTeam Gitlab::Access.options_with_owner.key(max_member_access(user_id)) end - # This method assumes project and group members are eager loaded for optimal - # performance. - def max_member_access(user_id) - access = [] + # Determine the maximum access level for a group of users in bulk. + # + # Returns a Hash mapping user ID -> maxmum access level. + def max_member_access_for_user_ids(user_ids) + user_ids = user_ids.uniq + key = "max_member_access:#{project.id}" + RequestStore.store[key] ||= Hash.new + access = RequestStore.store[key] - access += project.members.where(user_id: user_id).has_access.pluck(:access_level) + # Lookup only the IDs we need + user_ids = user_ids - access.keys - if group - access += group.members.where(user_id: user_id).has_access.pluck(:access_level) - end + if user_ids.present? + user_ids.map { |id| access[id] = Gitlab::Access::NO_ACCESS } - if project.invited_groups.any? && project.allowed_to_share_with_group? - access << max_invited_level(user_id) + member_access = project.members.where(user_id: user_ids).has_access.pluck(:user_id, :access_level).to_h + merge_max!(access, member_access) + + if group + group_access = group.members.where(user_id: user_ids).has_access.pluck(:user_id, :access_level).to_h + merge_max!(access, group_access) + end + + if project.invited_groups.any? && project.allowed_to_share_with_group? + # Not optimized + invited_levels = user_ids.map { |id| [id, max_invited_level(id)] }.to_h + merge_max!(access, invited_levels) + end end - access.compact.max + access + end + + def max_member_access(user_id) + max_member_access_for_user_ids([user_id])[user_id] end private @@ -156,6 +175,7 @@ class ProjectTeam project.project_group_links.map do |group_link| invited_group = group_link.group access = invited_group.group_members.find_by(user_id: user_id).try(:access_field) + access = Gitlab::Access::NO_ACCESS unless access.present? # If group member has higher access level we should restrict it # to max allowed access level @@ -215,4 +235,8 @@ class ProjectTeam def group project.group end + + def merge_max!(first_hash, second_hash) + first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new } + end end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index de41ea415a6..a533bac2692 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -7,6 +7,7 @@ module Gitlab module Access class AccessDeniedError < StandardError; end + NO_ACCESS = 0 GUEST = 10 REPORTER = 20 DEVELOPER = 30 diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 08a93503258..af371248ae9 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/spec/helpers/notes_helper_spec.rb @@ -1,37 +1,30 @@ require "spec_helper" describe NotesHelper do - describe "#notes_max_access_for_users" do - let(:owner) { create(:owner) } - let(:group) { create(:group) } - let(:project) { create(:empty_project, namespace: group) } - let(:master) { create(:user) } - let(:reporter) { create(:user) } - let(:guest) { create(:user) } - - let(:owner_note) { create(:note, author: owner, project: project) } - let(:master_note) { create(:note, author: master, project: project) } - let(:reporter_note) { create(:note, author: reporter, project: project) } - let!(:notes) { [owner_note, master_note, reporter_note] } - - before do - group.add_owner(owner) - project.team << [master, :master] - project.team << [reporter, :reporter] - project.team << [guest, :guest] - end + let(:owner) { create(:owner) } + let(:group) { create(:group) } + let(:project) { create(:empty_project, namespace: group) } + let(:master) { create(:user) } + let(:reporter) { create(:user) } + let(:guest) { create(:user) } - it 'return human access levels' do - original_method = project.team.method(:human_max_access) - expect_any_instance_of(ProjectTeam).to receive(:human_max_access).exactly(3).times do |*args| - original_method.call(args[1]) - end + let(:owner_note) { create(:note, author: owner, project: project) } + let(:master_note) { create(:note, author: master, project: project) } + let(:reporter_note) { create(:note, author: reporter, project: project) } + let!(:notes) { [owner_note, master_note, reporter_note] } + before do + group.add_owner(owner) + project.team << [master, :master] + project.team << [reporter, :reporter] + project.team << [guest, :guest] + end + + describe "#notes_max_access_for_users" do + it 'return human access levels' do expect(helper.note_max_access_for_user(owner_note)).to eq('Owner') expect(helper.note_max_access_for_user(master_note)).to eq('Master') expect(helper.note_max_access_for_user(reporter_note)).to eq('Reporter') - # Call it again to ensure value is cached - expect(helper.note_max_access_for_user(owner_note)).to eq('Owner') end it 'handles access in different projects' do @@ -43,4 +36,16 @@ describe NotesHelper do expect(helper.note_max_access_for_user(other_note)).to eq('Reporter') end end + + describe '#preload_max_access_for_authors' do + it 'loads multiple users' do + expected_access = { + owner.id => Gitlab::Access::OWNER, + master.id => Gitlab::Access::MASTER, + reporter.id => Gitlab::Access::REPORTER + } + + expect(helper.preload_max_access_for_authors(notes, project)).to eq(expected_access) + end + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 1acb5846fcf..cd5f40fe3d2 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -1,6 +1,62 @@ require 'spec_helper' describe Ability, lib: true do + describe '.can_edit_note?' do + let(:project) { create(:empty_project) } + let!(:note) { create(:note_on_issue, project: project) } + + context 'using an anonymous user' do + it 'returns false' do + expect(described_class.can_edit_note?(nil, note)).to be_falsy + end + end + + context 'using a system note' do + it 'returns false' do + system_note = create(:note, system: true) + user = create(:user) + + expect(described_class.can_edit_note?(user, system_note)).to be_falsy + end + end + + context 'using users with different access levels' do + let(:user) { create(:user) } + + it 'returns true for the author' do + expect(described_class.can_edit_note?(note.author, note)).to be_truthy + end + + it 'returns false for a guest user' do + project.team << [user, :guest] + + expect(described_class.can_edit_note?(user, note)).to be_falsy + end + + it 'returns false for a developer' do + project.team << [user, :developer] + + expect(described_class.can_edit_note?(user, note)).to be_falsy + end + + it 'returns true for a master' do + project.team << [user, :master] + + expect(described_class.can_edit_note?(user, note)).to be_truthy + end + + it 'returns true for a group owner' do + group = create(:group) + project.project_group_links.create( + group: group, + group_access: Gitlab::Access::MASTER) + group.add_owner(user) + + expect(described_class.can_edit_note?(user, note)).to be_truthy + end + end + end + describe '.users_that_can_read_project' do context 'using a public project' do it 'returns all the users' do diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 9262aeb6ed8..115fffd82d9 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -151,8 +151,8 @@ describe ProjectTeam, models: true do it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::MASTER) } it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) } it { expect(project.team.max_member_access(guest.id)).to eq(Gitlab::Access::GUEST) } - it { expect(project.team.max_member_access(nonmember.id)).to be_nil } - it { expect(project.team.max_member_access(requester.id)).to be_nil } + it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) } + it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) } end context 'when project is shared with group' do @@ -168,14 +168,14 @@ describe ProjectTeam, models: true do it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::DEVELOPER) } it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) } - it { expect(project.team.max_member_access(nonmember.id)).to be_nil } - it { expect(project.team.max_member_access(requester.id)).to be_nil } + it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) } + it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) } context 'but share_with_group_lock is true' do before { project.namespace.update(share_with_group_lock: true) } - it { expect(project.team.max_member_access(master.id)).to be_nil } - it { expect(project.team.max_member_access(reporter.id)).to be_nil } + it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::NO_ACCESS) } + it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::NO_ACCESS) } end end end @@ -194,8 +194,43 @@ describe ProjectTeam, models: true do it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::MASTER) } it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) } it { expect(project.team.max_member_access(guest.id)).to eq(Gitlab::Access::GUEST) } - it { expect(project.team.max_member_access(nonmember.id)).to be_nil } - it { expect(project.team.max_member_access(requester.id)).to be_nil } + it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) } + it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) } + end + end + + describe "#max_member_access_for_users" do + it 'returns correct roles for different users' do + master = create(:user) + reporter = create(:user) + promoted_guest = create(:user) + guest = create(:user) + project = create(:project) + + project.team << [master, :master] + project.team << [reporter, :reporter] + project.team << [promoted_guest, :guest] + project.team << [guest, :guest] + + group = create(:group) + group_developer = create(:user) + project.project_group_links.create( + group: group, + group_access: Gitlab::Access::DEVELOPER) + + group.add_master(promoted_guest) + group.add_developer(group_developer) + users = [master, reporter, promoted_guest, guest, group_developer].map(&:id) + + expected = { + master.id => Gitlab::Access::MASTER, + reporter.id => Gitlab::Access::REPORTER, + promoted_guest.id => Gitlab::Access::DEVELOPER, + guest.id => Gitlab::Access::GUEST, + group_developer.id => Gitlab::Access::DEVELOPER + } + + expect(project.team.max_member_access_for_user_ids(users)).to eq(expected) end end end |