Commit 3d7f99c6 authored by Imre Farkas's avatar Imre Farkas

Worker updating project authoirzations during project create

Specialized worker can bulk insert only the necessary records, instead
of the current approach which recalculates every project authorizations
for members.
parent 19cc6449
...@@ -335,6 +335,11 @@ class Group < Namespace ...@@ -335,6 +335,11 @@ class Group < Namespace
.where(source_id: source_ids) .where(source_id: source_ids)
end end
def members_from_self_and_ancestors_with_effective_access_level
members_with_parents.select([:user_id, 'MAX(access_level) AS access_level'])
.group(:user_id)
end
def members_with_descendants def members_with_descendants
GroupMember GroupMember
.active_without_invites_and_requests .active_without_invites_and_requests
......
# frozen_string_literal: true # frozen_string_literal: true
class Member < ApplicationRecord class Member < ApplicationRecord
include EachBatch
include AfterCommitQueue include AfterCommitQueue
include Sortable include Sortable
include Importable include Importable
......
# frozen_string_literal: true
module AuthorizedProjectUpdate
class ProjectCreateService < BaseService
BATCH_SIZE = 1000
def initialize(project)
@project = project
end
def execute
group = project.group
unless group
return ServiceResponse.error(message: 'Project does not have a group')
end
group.members_from_self_and_ancestors_with_effective_access_level
.each_batch(of: BATCH_SIZE, column: :user_id) do |members|
attributes = members.map do |member|
{ user_id: member.user_id, project_id: project.id, access_level: member.access_level }
end
ProjectAuthorization.insert_all(attributes)
end
ServiceResponse.success
end
private
attr_reader :project
end
end
...@@ -3,6 +3,13 @@ ...@@ -3,6 +3,13 @@
# #
# Do not edit it manually! # Do not edit it manually!
--- ---
- :name: authorized_project_update:authorized_project_update_project_create
:feature_category: :authentication_and_authorization
:has_external_dependencies:
:urgency: :low
:resource_boundary: :unknown
:weight: 1
:idempotent: true
- :name: auto_devops:auto_devops_disable - :name: auto_devops:auto_devops_disable
:feature_category: :auto_devops :feature_category: :auto_devops
:has_external_dependencies: :has_external_dependencies:
......
# frozen_string_literal: true
module AuthorizedProjectUpdate
class ProjectCreateWorker
include ApplicationWorker
feature_category :authentication_and_authorization
urgency :low
queue_namespace :authorized_project_update
idempotent!
def perform(project_id)
project = Project.find(project_id)
AuthorizedProjectUpdate::ProjectCreateService.new(project).execute
end
end
end
...@@ -32,6 +32,8 @@ ...@@ -32,6 +32,8 @@
- 1 - 1
- - authorized_keys - - authorized_keys
- 2 - 2
- - authorized_project_update
- 1
- - authorized_project_update_user_refresh_with_low_urgency - - authorized_project_update_user_refresh_with_low_urgency
- 1 - 1
- - authorized_projects - - authorized_projects
......
...@@ -659,6 +659,42 @@ describe Group do ...@@ -659,6 +659,42 @@ describe Group do
end end
end end
describe '#members_from_self_and_ancestors_with_effective_access_level' do
let!(:group_parent) { create(:group, :private) }
let!(:group) { create(:group, :private, parent: group_parent) }
let!(:group_child) { create(:group, :private, parent: group) }
let!(:user) { create(:user) }
let(:parent_group_access_level) { Gitlab::Access::REPORTER }
let(:group_access_level) { Gitlab::Access::DEVELOPER }
let(:child_group_access_level) { Gitlab::Access::MAINTAINER }
before do
create(:group_member, user: user, group: group_parent, access_level: parent_group_access_level)
create(:group_member, user: user, group: group, access_level: group_access_level)
create(:group_member, user: user, group: group_child, access_level: child_group_access_level)
end
it 'returns effective access level for user' do
expect(group_parent.members_from_self_and_ancestors_with_effective_access_level.as_json).to(
contain_exactly(
hash_including('user_id' => user.id, 'access_level' => parent_group_access_level)
)
)
expect(group.members_from_self_and_ancestors_with_effective_access_level.as_json).to(
contain_exactly(
hash_including('user_id' => user.id, 'access_level' => group_access_level)
)
)
expect(group_child.members_from_self_and_ancestors_with_effective_access_level.as_json).to(
contain_exactly(
hash_including('user_id' => user.id, 'access_level' => child_group_access_level)
)
)
end
end
describe '#direct_and_indirect_members' do describe '#direct_and_indirect_members' do
let!(:group) { create(:group, :nested) } let!(:group) { create(:group, :nested) }
let!(:sub_group) { create(:group, parent: group) } let!(:sub_group) { create(:group, parent: group) }
......
# frozen_string_literal: true
require 'spec_helper'
describe AuthorizedProjectUpdate::ProjectCreateService do
let_it_be(:group_parent) { create(:group, :private) }
let_it_be(:group) { create(:group, :private, parent: group_parent) }
let_it_be(:group_child) { create(:group, :private, parent: group) }
let_it_be(:group_project) { create(:project, group: group) }
let_it_be(:parent_group_user) { create(:user) }
let_it_be(:group_user) { create(:user) }
let_it_be(:child_group_user) { create(:user) }
let(:access_level) { Gitlab::Access::MAINTAINER }
subject(:service) { described_class.new(group_project) }
describe '#perform' do
context 'direct group members' do
before do
create(:group_member, access_level: access_level, group: group, user: group_user)
ProjectAuthorization.delete_all
end
it 'creates project authorization' do
expect { service.execute }.to(
change { ProjectAuthorization.count }.from(0).to(1))
project_authorization = ProjectAuthorization.where(
project_id: group_project.id,
user_id: group_user.id,
access_level: access_level)
expect(project_authorization).to exist
end
end
context 'inherited group members' do
before do
create(:group_member, access_level: access_level, group: group_parent, user: parent_group_user)
ProjectAuthorization.delete_all
end
it 'creates project authorization' do
expect { service.execute }.to(
change { ProjectAuthorization.count }.from(0).to(1))
project_authorization = ProjectAuthorization.where(
project_id: group_project.id,
user_id: parent_group_user.id,
access_level: access_level)
expect(project_authorization).to exist
end
end
context 'membership overrides' do
before do
create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user)
create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user)
ProjectAuthorization.delete_all
end
it 'creates project authorization' do
expect { service.execute }.to(
change { ProjectAuthorization.count }.from(0).to(1))
project_authorization = ProjectAuthorization.where(
project_id: group_project.id,
user_id: group_user.id,
access_level: Gitlab::Access::DEVELOPER)
expect(project_authorization).to exist
end
end
context 'no group member' do
it 'does not create project authorization' do
expect { service.execute }.not_to(
change { ProjectAuthorization.count }.from(0))
end
end
context 'unapproved access requests' do
before do
create(:group_member, :guest, :access_request, user: group_user, group: group)
end
it 'does not create project authorization' do
expect { service.execute }.not_to(
change { ProjectAuthorization.count }.from(0))
end
end
context 'project has more user than BATCH_SIZE' do
let(:batch_size) { 2 }
let(:users) { create_list(:user, batch_size + 1 ) }
before do
stub_const("#{described_class.name}::BATCH_SIZE", batch_size)
users.each do |user|
create(:group_member, access_level: access_level, group: group_parent, user: user)
end
ProjectAuthorization.delete_all
end
it 'bulk creates project authorizations in batches' do
users.each_slice(batch_size) do |batch|
attributes = batch.map do |user|
{ user_id: user.id, project_id: group_project.id, access_level: access_level }
end
expect(ProjectAuthorization).to(
receive(:insert_all).with(array_including(attributes)).and_call_original)
end
expect { service.execute }.to(
change { ProjectAuthorization.count }.from(0).to(batch_size + 1))
end
end
context 'ignores existing project authorizations' do
before do
# ProjectAuthorizations is also created because of an after_commit
# callback on Member model
create(:group_member, access_level: access_level, group: group, user: group_user)
end
it 'does not create project authorization' do
project_authorization = ProjectAuthorization.where(
project_id: group_project.id,
user_id: group_user.id,
access_level: access_level)
expect { service.execute }.not_to(
change { project_authorization.reload.exists? }.from(true))
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe AuthorizedProjectUpdate::ProjectCreateWorker do
let_it_be(:group) { create(:group, :private) }
let_it_be(:group_project) { create(:project, group: group) }
let_it_be(:group_user) { create(:user) }
let(:access_level) { Gitlab::Access::MAINTAINER }
subject(:worker) { described_class.new }
it 'calls AuthorizedProjectUpdate::ProjectCreateService' do
expect_next_instance_of(AuthorizedProjectUpdate::ProjectCreateService) do |service|
expect(service).to(receive(:execute))
end
worker.perform(group_project.id)
end
it 'returns ServiceResponse.success' do
result = worker.perform(group_project.id)
expect(result.success?).to be_truthy
end
context 'idempotence' do
before do
create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: group, user: group_user)
ProjectAuthorization.delete_all
end
include_examples 'an idempotent worker' do
let(:job_args) { group_project.id }
it 'creates project authorization' do
subject
project_authorization = ProjectAuthorization.where(
project_id: group_project.id,
user_id: group_user.id,
access_level: access_level)
expect(project_authorization).to exist
expect(ProjectAuthorization.count).to eq(1)
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