Commit f4804d5b authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'memoize-note-editable' into 'master'

Optimize maximum user access level lookup in loading of notes

See merge request !5412
parents 260102b2 277e9e4e
...@@ -5,6 +5,7 @@ v 8.11.0 (unreleased) ...@@ -5,6 +5,7 @@ v 8.11.0 (unreleased)
- Fix CI status icon link underline (ClemMakesApps) - Fix CI status icon link underline (ClemMakesApps)
- Fix of 'Commits being passed to custom hooks are already reachable when using the UI' - 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 - 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 - Limit git rev-list output count to one in forced push check
- Clean up unused routes (Josef Strzibny) - Clean up unused routes (Josef Strzibny)
- Add green outline to New Branch button. !5447 (winniehell) - Add green outline to New Branch button. !5447 (winniehell)
......
class Projects::IssuesController < Projects::ApplicationController class Projects::IssuesController < Projects::ApplicationController
include NotesHelper
include ToggleSubscriptionAction include ToggleSubscriptionAction
include IssuableActions include IssuableActions
include ToggleAwardEmoji include ToggleAwardEmoji
...@@ -70,6 +71,8 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -70,6 +71,8 @@ class Projects::IssuesController < Projects::ApplicationController
@note = @project.notes.new(noteable: @issue) @note = @project.notes.new(noteable: @issue)
@noteable = @issue @noteable = @issue
preload_max_access_for_authors(@notes, @project)
respond_to do |format| respond_to do |format|
format.html format.html
format.json do format.json do
......
...@@ -3,6 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -3,6 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
include DiffForPath include DiffForPath
include DiffHelper include DiffHelper
include IssuableActions include IssuableActions
include NotesHelper
include ToggleAwardEmoji include ToggleAwardEmoji
before_action :module_enabled before_action :module_enabled
...@@ -385,6 +386,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -385,6 +386,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@project_wiki, @project_wiki,
@ref @ref
) )
preload_max_access_for_authors(@notes, @project)
end end
def define_widget_vars def define_widget_vars
......
...@@ -7,7 +7,7 @@ module NotesHelper ...@@ -7,7 +7,7 @@ module NotesHelper
end end
def note_editable?(note) def note_editable?(note)
note.editable? && can?(current_user, :admin_note, note) Ability.can_edit_note?(current_user, note)
end end
def noteable_json(noteable) def noteable_json(noteable)
...@@ -87,14 +87,13 @@ module NotesHelper ...@@ -87,14 +87,13 @@ module NotesHelper
end end
end end
def note_max_access_for_user(note) def preload_max_access_for_authors(notes, project)
@max_access_by_user_id ||= Hash.new do |hash, key| user_ids = notes.map(&:author_id)
project = key[:project] project.team.max_member_access_for_user_ids(user_ids)
hash[key] = project.team.human_max_access(key[:user_id])
end end
full_key = { project: note.project, user_id: note.author_id } def note_max_access_for_user(note)
@max_access_by_user_id[full_key] note.project.team.human_max_access(note.author_id)
end end
def discussion_diff_path(discussion) def discussion_diff_path(discussion)
......
...@@ -388,6 +388,18 @@ class Ability ...@@ -388,6 +388,18 @@ class Ability
GroupProjectsFinder.new(group).execute(user).any? GroupProjectsFinder.new(group).execute(user).any?
end 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) def namespace_abilities(user, namespace)
rules = [] rules = []
......
...@@ -53,6 +53,10 @@ class Member < ActiveRecord::Base ...@@ -53,6 +53,10 @@ class Member < ActiveRecord::Base
default_value_for :notification_level, NotificationSetting.levels[:global] default_value_for :notification_level, NotificationSetting.levels[:global]
class << self 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) def find_by_invite_token(invite_token)
invite_token = Devise.token_generator.digest(self, :invite_token, invite_token) invite_token = Devise.token_generator.digest(self, :invite_token, invite_token)
find_by(invite_token: invite_token) find_by(invite_token: invite_token)
......
...@@ -132,39 +132,63 @@ class ProjectTeam ...@@ -132,39 +132,63 @@ class ProjectTeam
Gitlab::Access.options_with_owner.key(max_member_access(user_id)) Gitlab::Access.options_with_owner.key(max_member_access(user_id))
end end
# This method assumes project and group members are eager loaded for optimal # Determine the maximum access level for a group of users in bulk.
# performance. #
def max_member_access(user_id) # Returns a Hash mapping user ID -> maximum access level.
access = [] 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]
# Lookup only the IDs we need
user_ids = user_ids - access.keys
if user_ids.present?
user_ids.each { |id| access[id] = Gitlab::Access::NO_ACCESS }
access += project.members.where(user_id: user_id).has_access.pluck(:access_level) member_access = project.members.access_for_user_ids(user_ids)
merge_max!(access, member_access)
if group if group
access += group.members.where(user_id: user_id).has_access.pluck(:access_level) group_access = group.members.access_for_user_ids(user_ids)
merge_max!(access, group_access)
end 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? if project.invited_groups.any? && project.allowed_to_share_with_group?
access << max_invited_level(user_id) 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
end end
access.compact.max def max_member_access(user_id)
max_member_access_for_user_ids([user_id])[user_id]
end end
private private
def max_invited_level(user_id) # For a given group, return the maximum access level for the user. This is the min of
project.project_group_links.map do |group_link| # 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 invited_group = group_link.group
access = invited_group.group_members.find_by(user_id: user_id).try(:access_field) 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 # If the user is not in the list, assume he/she does not have access
# to max allowed access level missing_users = user_ids - access.keys
if access && access > group_link.group_access missing_users.each { |id| access[id] = Gitlab::Access::NO_ACCESS }
access = group_link.group_access
end
access # Cap the maximum access by the invited level access
end.compact.max access.each { |key, value| access[key] = [value, capped_access_level].min }
end end
def fetch_members(level = nil) def fetch_members(level = nil)
...@@ -215,4 +239,8 @@ class ProjectTeam ...@@ -215,4 +239,8 @@ class ProjectTeam
def group def group
project.group project.group
end end
def merge_max!(first_hash, second_hash)
first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new }
end
end end
...@@ -7,6 +7,7 @@ module Gitlab ...@@ -7,6 +7,7 @@ module Gitlab
module Access module Access
class AccessDeniedError < StandardError; end class AccessDeniedError < StandardError; end
NO_ACCESS = 0
GUEST = 10 GUEST = 10
REPORTER = 20 REPORTER = 20
DEVELOPER = 30 DEVELOPER = 30
......
require "spec_helper" require "spec_helper"
describe NotesHelper do describe NotesHelper do
describe "#notes_max_access_for_users" do
let(:owner) { create(:owner) } let(:owner) { create(:owner) }
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:empty_project, namespace: group) } let(:project) { create(:empty_project, namespace: group) }
...@@ -21,17 +20,11 @@ describe NotesHelper do ...@@ -21,17 +20,11 @@ describe NotesHelper do
project.team << [guest, :guest] project.team << [guest, :guest]
end end
describe "#notes_max_access_for_users" do
it 'return human access levels' do 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
expect(helper.note_max_access_for_user(owner_note)).to eq('Owner') 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(master_note)).to eq('Master')
expect(helper.note_max_access_for_user(reporter_note)).to eq('Reporter') 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 end
it 'handles access in different projects' do it 'handles access in different projects' do
...@@ -43,4 +36,16 @@ describe NotesHelper do ...@@ -43,4 +36,16 @@ describe NotesHelper do
expect(helper.note_max_access_for_user(other_note)).to eq('Reporter') expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
end end
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 end
require 'spec_helper' require 'spec_helper'
describe Ability, lib: true do 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 describe '.users_that_can_read_project' do
context 'using a public project' do context 'using a public project' do
it 'returns all the users' do it 'returns all the users' do
......
...@@ -79,6 +79,18 @@ describe Member, models: true do ...@@ -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 } @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
end 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 describe '.invite' do
it { expect(described_class.invite).not_to include @master } it { expect(described_class.invite).not_to include @master }
it { expect(described_class.invite).to include @invited_member } it { expect(described_class.invite).to include @invited_member }
......
...@@ -151,8 +151,8 @@ describe ProjectTeam, models: true do ...@@ -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(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(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(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(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
it { expect(project.team.max_member_access(requester.id)).to be_nil } it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) }
end end
context 'when project is shared with group' do context 'when project is shared with group' do
...@@ -168,14 +168,14 @@ describe ProjectTeam, models: true 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(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(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(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
it { expect(project.team.max_member_access(requester.id)).to be_nil } it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) }
context 'but share_with_group_lock is true' do context 'but share_with_group_lock is true' do
before { project.namespace.update(share_with_group_lock: true) } 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(master.id)).to eq(Gitlab::Access::NO_ACCESS) }
it { expect(project.team.max_member_access(reporter.id)).to be_nil } it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::NO_ACCESS) }
end end
end end
end end
...@@ -194,8 +194,53 @@ describe ProjectTeam, models: true do ...@@ -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(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(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(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(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
it { expect(project.team.max_member_access(requester.id)).to be_nil } 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 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