Commit 7cac73ed authored by Doug Stull's avatar Doug Stull Committed by Dmytro Zaporozhets (DZ)

Resolve admin_group_member group policy n+1

parent 9fe3efd6
......@@ -22,6 +22,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
feature_category :authentication_and_authorization
def index
preload_max_access
@sort = params[:sort].presence || sort_value_name
@members = GroupMembersFinder
......@@ -50,6 +51,14 @@ class Groups::GroupMembersController < Groups::ApplicationController
private
def preload_max_access
return unless current_user
# this allows the can? against admin type queries in this action to
# only perform the query once, even if it is cached
current_user.max_access_for_group[@group.id] = @group.max_member_access(current_user)
end
def can_manage_members
can?(current_user, :admin_group_member, @group)
end
......
......@@ -551,8 +551,14 @@ class Group < Namespace
def max_member_access_for_user(user, only_concrete_membership: false)
return GroupMember::NO_ACCESS unless user
return GroupMember::OWNER if user.can_admin_all_resources? && !only_concrete_membership
return user.max_access_for_group[id] if user.max_access_for_group[id]
max_member_access = members_with_parents.where(user_id: user)
max_member_access(user)
end
def max_member_access(user)
max_member_access = members_with_parents
.where(user_id: user)
.reorder(access_level: :desc)
.first
&.access_level
......
......@@ -96,6 +96,12 @@ class User < ApplicationRecord
# Virtual attribute for impersonator
attr_accessor :impersonator
attr_writer :max_access_for_group
def max_access_for_group
@max_access_for_group ||= {}
end
#
# Relations
#
......
---
title: Resolve admin_group_member group policy n+1
merge_request: 58948
author:
type: performance
......@@ -5,9 +5,8 @@ require 'spec_helper'
RSpec.describe Groups::GroupMembersController do
include ExternalAuthorizationServiceHelpers
let(:user) { create(:user) }
let(:group) { create(:group, :public) }
let(:membership) { create(:group_member, group: group) }
let_it_be(:user) { create(:user) }
let_it_be(:group, reload: true) { create(:group, :public) }
before do
group.add_owner(user)
......@@ -29,9 +28,13 @@ RSpec.describe Groups::GroupMembersController do
create_list(:group_member, 5, :invited, :developer, group: group, created_by: user)
create_list(:group_member, 5, :access_request, group: group)
# locally 87 vs 128
unresolved_n_plus_ones = 36
# created_by = 8 # 88 vs 134
# oncall_schedules = 6 # 87 vs 128
unresolved_n_plus_ones = 8 # 1 in GDK, 5 in CI hard to say - multiple should likely be lower now
# using_license 84 vs 115 = ~13
# can_update 82 vs 102 = ~13
# can_remove 80 vs 89 = ~13
# can_resend 80 + 5 = ~4
# solving access level reduced from ~80 to ~50
# still have a few queries created by can_update/can_remove that should be reduced
multiple_members_threshold = 5
expect do
......@@ -85,7 +88,8 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'POST #override' do
let(:group) { create(:group_with_ldap_group_link) }
let_it_be(:group) { create(:group_with_ldap_group_link) }
let_it_be(:membership) { create(:group_member, group: group) }
before do
allow(Ability).to receive(:allowed?).and_call_original
......@@ -105,7 +109,7 @@ RSpec.describe Groups::GroupMembersController do
end
context 'when user has minimal access' do
let(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
let_it_be(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
it 'is not successful' do
post :override,
......@@ -195,8 +199,8 @@ RSpec.describe Groups::GroupMembersController do
end
context 'when group has email domain feature disabled' do
let(:email) { 'unverified@gitlab.com' }
let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
let_it_be(:email) { 'unverified@gitlab.com' }
let_it_be(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
before do
stub_licensed_features(group_allowed_email_domains: false)
......@@ -248,8 +252,8 @@ RSpec.describe Groups::GroupMembersController do
end
context 'when group has email domain feature disabled' do
let(:email) { 'unverified@gitlab.com' }
let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
let_it_be(:email) { 'unverified@gitlab.com' }
let_it_be(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
before do
stub_licensed_features(group_allowed_email_domains: false)
......@@ -272,7 +276,7 @@ RSpec.describe Groups::GroupMembersController do
describe 'POST #resend_invite' do
context 'when user has minimal access' do
let(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
let_it_be(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
it 'is not successful' do
post :resend_invite, params: { group_id: group, id: membership }
......
......@@ -5,9 +5,8 @@ require 'spec_helper'
RSpec.describe Groups::GroupMembersController do
include ExternalAuthorizationServiceHelpers
let(:user) { create(:user) }
let(:group) { create(:group, :public) }
let(:membership) { create(:group_member, group: group) }
let_it_be(:user) { create(:user) }
let_it_be(:group, reload: true) { create(:group, :public) }
before do
travel_to DateTime.new(2019, 4, 1)
......@@ -26,13 +25,21 @@ RSpec.describe Groups::GroupMembersController do
end
context 'user with owner access' do
let!(:invited) { create_list(:group_member, 3, :invited, group: group) }
let_it_be(:invited) { create_list(:group_member, 3, :invited, group: group) }
before do
group.add_owner(user)
sign_in(user)
end
it 'assigns max_access_for_group' do
allow(controller).to receive(:current_user).and_return(user)
get :index, params: { group_id: group }
expect(user.max_access_for_group[group.id]).to eq(Gitlab::Access::OWNER)
end
it 'assigns invited members' do
get :index, params: { group_id: group }
......@@ -65,8 +72,8 @@ RSpec.describe Groups::GroupMembersController do
end
context 'when user has owner access to subgroup' do
let(:nested_group) { create(:group, parent: group) }
let(:nested_group_user) { create(:user) }
let_it_be(:nested_group) { create(:group, parent: group) }
let_it_be(:nested_group_user) { create(:user) }
before do
group.add_owner(user)
......@@ -95,7 +102,7 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'POST create' do
let(:group_user) { create(:user) }
let_it_be(:group_user) { create(:user) }
before do
sign_in(user)
......@@ -189,7 +196,7 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'PUT update' do
let(:requester) { create(:group_member, :access_request, group: group) }
let_it_be(:requester) { create(:group_member, :access_request, group: group) }
before do
group.add_owner(user)
......@@ -292,9 +299,9 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'DELETE destroy' do
let(:sub_group) { create(:group, parent: group) }
let!(:member) { create(:group_member, :developer, group: group) }
let!(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
let_it_be(:sub_group) { create(:group, parent: group) }
let_it_be(:member) { create(:group_member, :developer, group: group) }
let_it_be(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
before do
sign_in(user)
......@@ -403,6 +410,8 @@ RSpec.describe Groups::GroupMembersController do
end
context 'and is a requester' do
let(:group) { create(:group, :public) }
before do
group.request_access(user)
end
......@@ -435,7 +444,7 @@ RSpec.describe Groups::GroupMembersController do
end
describe 'POST approve_access_request' do
let(:member) { create(:group_member, :access_request, group: group) }
let_it_be(:member) { create(:group_member, :access_request, group: group) }
before do
sign_in(user)
......@@ -479,6 +488,8 @@ RSpec.describe Groups::GroupMembersController do
end
context 'with external authorization enabled' do
let_it_be(:membership) { create(:group_member, group: group) }
before do
enable_external_authorization_service_check
group.add_owner(user)
......
......@@ -955,11 +955,64 @@ RSpec.describe Group do
it { expect(subject.parent).to be_kind_of(described_class) }
end
context "with member access" do
let_it_be(:group_user) { create(:user) }
describe '#max_member_access_for_user' do
context 'with user in the group' do
before do
group.add_owner(group_user)
end
it 'returns correct access level' do
expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER)
end
end
context 'when user is nil' do
it 'returns NO_ACCESS' do
expect(group.max_member_access_for_user(nil)).to eq(Gitlab::Access::NO_ACCESS)
end
end
context 'evaluating admin access level' do
let_it_be(:admin) { create(:admin) }
context 'when admin mode is enabled', :enable_admin_mode do
it 'returns OWNER by default' do
expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER)
end
end
context 'when admin mode is disabled' do
it 'returns NO_ACCESS' do
expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS)
end
end
it 'returns NO_ACCESS when only concrete membership should be considered' do
expect(group.max_member_access_for_user(admin, only_concrete_membership: true))
.to eq(Gitlab::Access::NO_ACCESS)
end
end
context 'when max_access_for_group is set' do
let(:max_member_access) { 111 }
before do
group_user.max_access_for_group[group.id] = max_member_access
end
it 'uses the cached value' do
expect(group.max_member_access_for_user(group_user)).to eq(max_member_access)
end
end
end
describe '#max_member_access' do
context 'group shared with another group' do
let(:parent_group_user) { create(:user) }
let(:group_user) { create(:user) }
let(:child_group_user) { create(:user) }
let_it_be(:parent_group_user) { create(:user) }
let_it_be(:child_group_user) { create(:user) }
let_it_be(:group_parent) { create(:group, :private) }
let_it_be(:group) { create(:group, :private, parent: group_parent) }
......@@ -980,12 +1033,10 @@ RSpec.describe Group do
end
context 'with user in the group' do
let(:user) { group_user }
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
expect(shared_group_parent.max_member_access(group_user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER)
expect(shared_group_child.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER)
end
context 'with lower group access level than max access level for share' do
......@@ -994,30 +1045,26 @@ RSpec.describe Group do
it 'returns correct access level' do
group.add_reporter(user)
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::REPORTER)
expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::REPORTER)
end
end
end
context 'with user in the parent group' do
let(:user) { parent_group_user }
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_parent.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
context 'with user in the child group' do
let(:user) { child_group_user }
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_parent.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
......@@ -1028,9 +1075,9 @@ RSpec.describe Group do
let(:user) { unrelated_project.owner }
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
......@@ -1042,9 +1089,9 @@ RSpec.describe Group do
end
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
end
......@@ -1067,28 +1114,8 @@ RSpec.describe Group do
end
it 'returns correct access level' do
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::MAINTAINER)
end
end
context 'evaluating admin access level' do
let_it_be(:admin) { create(:admin) }
context 'when admin mode is enabled', :enable_admin_mode do
it 'returns OWNER by default' do
expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER)
end
expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::MAINTAINER)
end
context 'when admin mode is disabled' do
it 'returns NO_ACCESS' do
expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS)
end
end
it 'returns NO_ACCESS when only concrete membership should be considered' do
expect(group.max_member_access_for_user(admin, only_concrete_membership: true))
.to eq(Gitlab::Access::NO_ACCESS)
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