diff --git a/CHANGELOG b/CHANGELOG index 5ff0cb42ccc3db6af0c74b046cf69ff0e6f9834b..fb38db156308d45fa9596eb975fd1946e21b9001 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 fa663c9bda4f0e24ca5133bdbca33a1cbe7ae395..91ff9407216f0d528da5e01a699143ab94d80046 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) + 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 594a61464b9aecf7c9b186a7c83fb3f9a6c40efc..23252fa59ccb3e9f9b623e6186a785f20a8eb81a 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) end def define_widget_vars diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 0f60dd828abe52e51da89504ac3adf4536a33f7e..0c47abe0fbad45987c509358ee9cca70db463af8 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 f33c8d61d3fedb39750ddfc3548379ada813f87e..e47c5539f60f5226e4c5fadb7d4ad2bf4d5e64b4 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -388,6 +388,18 @@ class Ability GroupProjectsFinder.new(group).execute(user).any? end + def can_edit_note?(user, note) + return false if !note.editable? || !user.present? + return true if note.author == user || 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/member.rb b/app/models/member.rb index 44db3d977faf05c922e088c5073b48939b0202a2..24ab1276ee936f9a81098648802df78c9a8e585a 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -53,6 +53,10 @@ class Member < ActiveRecord::Base default_value_for :notification_level, NotificationSetting.levels[:global] class << self + def access_for_user_ids(user_ids) + where(user_id: user_ids).has_access.pluck(:user_id, :access_level).to_h + end + def find_by_invite_token(invite_token) invite_token = Devise.token_generator.digest(self, :invite_token, invite_token) find_by(invite_token: invite_token) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 9d312a53790528cc613e02f54912017f3726d235..fdfaf0527301079373a9abdf56b14754241dea34 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -132,39 +132,63 @@ 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 -> maximum 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] ||= {} + 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.each { |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.access_for_user_ids(user_ids) + merge_max!(access, member_access) + + if group + group_access = group.members.access_for_user_ids(user_ids) + merge_max!(access, group_access) + end + + # Each group produces a list of maximum access level per user. We take the + # max of the values produced by each group. + if project.invited_groups.any? && project.allowed_to_share_with_group? + project.project_group_links.each do |group_link| + invited_access = max_invited_level_for_users(group_link, user_ids) + merge_max!(access, invited_access) + end + 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 - def max_invited_level(user_id) - 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) + # For a given group, return the maximum access level for the user. This is the min of + # the invited access level of the group and the access level of the user within the group. + # For example, if the group has been given DEVELOPER access but the member has MASTER access, + # the user should receive only DEVELOPER access. + def max_invited_level_for_users(group_link, user_ids) + invited_group = group_link.group + capped_access_level = group_link.group_access + access = invited_group.group_members.access_for_user_ids(user_ids) - # If group member has higher access level we should restrict it - # to max allowed access level - if access && access > group_link.group_access - access = group_link.group_access - end + # If the user is not in the list, assume he/she does not have access + missing_users = user_ids - access.keys + missing_users.each { |id| access[id] = Gitlab::Access::NO_ACCESS } - access - end.compact.max + # Cap the maximum access by the invited level access + access.each { |key, value| access[key] = [value, capped_access_level].min } end def fetch_members(level = nil) @@ -215,4 +239,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 de41ea415a67d17ab461dde421c7a6fc551d7649..a533bac26925ff589a89a4c312df8040769e28bd 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 08a9350325872fb451a6657f9791ba41ba4826c5..af371248ae95ee943a14fa2b377ea368d8dbd1a4 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 1acb5846fcf9028026ddb5104637bfb58fc791e7..cd5f40fe3d29fd826ce5597ceeba1bbd4d257e03 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/member_spec.rb b/spec/models/member_spec.rb index 40181a8b906e46ab0e0e78e1f9d60a1d52998f58..44cd3c08718fd577d4d4ba0bb9589be42f912743 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -79,6 +79,18 @@ describe Member, models: true do @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request } end + describe '.access_for_user_ids' do + it 'returns the right access levels' do + users = [@owner_user.id, @master_user.id] + expected = { + @owner_user.id => Gitlab::Access::OWNER, + @master_user.id => Gitlab::Access::MASTER + } + + expect(described_class.access_for_user_ids(users)).to eq(expected) + end + end + describe '.invite' do it { expect(described_class.invite).not_to include @master } it { expect(described_class.invite).to include @invited_member } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 9262aeb6ed890a074f2971af20509474711e9b01..1f42fbd3385d4ef9b4396fed18a7e874fa180bfa 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,53 @@ 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) + second_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) + group.add_developer(second_developer) + + second_group = create(:group) + project.project_group_links.create( + group: second_group, + group_access: Gitlab::Access::MASTER) + second_group.add_master(second_developer) + + users = [master, reporter, promoted_guest, guest, group_developer, second_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, + second_developer.id => Gitlab::Access::MASTER + } + + expect(project.team.max_member_access_for_user_ids(users)).to eq(expected) end end end