Commit 3cc2a736 authored by Doug Stull's avatar Doug Stull Committed by Etienne Baqué

Send Members API through create service

parent 5749faf3
......@@ -652,6 +652,10 @@ class Group < Namespace
members.owners.connected_to_user.order_recent_sign_in.limit(Member::ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
end
def membership_locked?
false # to support project and group calling this as 'source'
end
def supports_events?
false
end
......
......@@ -1677,6 +1677,10 @@ class Project < ApplicationRecord
end
end
def membership_locked?
false
end
def bots
users.project_bot
end
......
......@@ -4,9 +4,12 @@ module Members
class CreateService < Members::BaseService
BlankInvitesError = Class.new(StandardError)
TooManyInvitesError = Class.new(StandardError)
MembershipLockedError = Class.new(StandardError)
DEFAULT_INVITE_LIMIT = 100
attr_reader :membership_locked
def initialize(*args)
super
......@@ -17,18 +20,22 @@ module Members
def execute
validate_invite_source!
validate_invites!
validate_invitable!
add_members
enqueue_onboarding_progress_action
result
rescue BlankInvitesError, TooManyInvitesError => e
rescue BlankInvitesError, TooManyInvitesError, MembershipLockedError => e
error(e.message)
end
def single_member
members.last
end
private
attr_reader :source, :errors, :invites, :member_created_namespace_id
attr_reader :source, :errors, :invites, :member_created_namespace_id, :members
def invites_from_params
params[:user_ids]
......@@ -38,7 +45,7 @@ module Members
raise ArgumentError, s_('AddMember|No invite source provided.') unless invite_source.present?
end
def validate_invites!
def validate_invitable!
raise BlankInvitesError, blank_invites_message if invites.blank?
return unless user_limit && invites.size > user_limit
......@@ -52,7 +59,7 @@ module Members
end
def add_members
members = source.add_users(
@members = source.add_users(
invites,
params[:access_level],
expires_at: params[:expires_at],
......
......@@ -18,7 +18,7 @@ module Members
params[:email]
end
def validate_invites!
def validate_invitable!
super
# we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails
......
......@@ -468,6 +468,13 @@ module EE
super && !(group && ::Gitlab::CurrentSettings.lock_memberships_to_ldap?)
end
override :membership_locked?
def membership_locked?
return false unless group
group.membership_lock?
end
# TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
def approvals_before_merge
return 0 unless feature_available?(:merge_request_approvers)
......
......@@ -5,9 +5,10 @@ module EE
module CreateService
private
def validate_invites!
def validate_invitable!
super
check_membership_lock!
check_quota!
end
......@@ -21,6 +22,13 @@ module EE
)
end
def check_membership_lock!
return unless source.membership_locked?
@membership_locked = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
raise ::Members::CreateService::MembershipLockedError
end
def invite_quota_exceeded?
return unless source.actual_limits.daily_invites
......
......@@ -51,17 +51,6 @@ module EE
can?(current_user, :read_group_saml_identity, members_source)
end
override :create_member
def create_member(current_user, user, source, params)
member = super
return false unless member
log_audit_event(member) if member.persisted? && member.valid?
member
end
def find_member(params)
source = find_source(:group, params.delete(:id))
authorize! :override_group_member, source
......@@ -76,14 +65,6 @@ module EE
render_validation_error!(updated_member)
end
end
def log_audit_event(member)
::AuditEventService.new(
current_user,
member.source,
action: :create
).for_member(member).security_event
end
end
end
end
......
......@@ -10,32 +10,6 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
allow(members_helpers).to receive(:current_user).and_return(create(:user))
end
shared_examples 'creates security_event' do |source_type|
context "with :source_type == #{source_type.pluralize}" do
it 'creates security_event' do
security_event = members_helpers.log_audit_event(member)
expect(security_event.entity_id).to eq(source.id)
expect(security_event.entity_type).to eq(source_type.capitalize)
expect(security_event.details.fetch(:target_id)).to eq(member.id)
end
end
end
describe '#log_audit_event' do
subject { members_helpers }
it_behaves_like 'creates security_event', 'group' do
let(:source) { create(:group) }
let(:member) { create(:group_member, :owner, group: source, user: create(:user)) }
end
it_behaves_like 'creates security_event', 'project' do
let(:source) { create(:project) }
let(:member) { create(:project_member, project: source, user: create(:user)) }
end
end
describe '.member_sort_options' do
it 'lists all keys available in group member view' do
sort_options = %w[
......
......@@ -926,6 +926,35 @@ RSpec.describe Project do
end
end
describe '#membership_locked?' do
let(:project) { build_stubbed(:project, group: group) }
let(:group) { nil }
context 'when project has no group' do
let(:project) { Project.new }
it 'is false' do
expect(project).not_to be_membership_locked
end
end
context 'with group_membership_lock enabled' do
let(:group) { build_stubbed(:group, membership_lock: true) }
it 'is true' do
expect(project).to be_membership_locked
end
end
context 'with group_membership_lock disabled' do
let(:group) { build_stubbed(:group, membership_lock: false) }
it 'is false' do
expect(project).not_to be_membership_locked
end
end
end
describe '#feature_available?' do
let(:namespace) { build(:namespace) }
let(:plan_license) { nil }
......
......@@ -50,24 +50,48 @@ module API
GroupMembersFinder.new(group).execute
end
def create_member(current_user, user, source, params)
source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
def present_members(members)
present members, with: Entities::Member, current_user: current_user, show_seat_info: params[:show_seat_info]
end
def track_areas_of_focus(member, areas_of_focus)
return unless areas_of_focus
def present_member_invitations(invitations)
present invitations, with: Entities::Invitation, current_user: current_user
end
def add_single_member_by_user_id(create_service_params)
source = create_service_params[:source]
user_id = create_service_params[:user_ids]
user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
if user
conflict!('Member already exists') if member_already_exists?(source, user_id)
instance = ::Members::CreateService.new(current_user, create_service_params)
instance.execute
not_allowed! if instance.membership_locked # This currently can only be reached in EE if group membership is locked
areas_of_focus.each do |area_of_focus|
Gitlab::Tracking.event(::Members::CreateService.name, 'area_of_focus', label: area_of_focus, property: member.id.to_s)
member = instance.single_member
render_validation_error!(member) if member.invalid?
present_members(member)
else
not_found!('User')
end
end
def present_members(members)
present members, with: Entities::Member, current_user: current_user, show_seat_info: params[:show_seat_info]
def add_multiple_members?(user_id)
user_id.include?(',')
end
def present_member_invitations(invitations)
present invitations, with: Entities::Invitation, current_user: current_user
def add_single_member?(user_id)
user_id.present?
end
private
def member_already_exists?(source, user_id)
source.members.exists?(user_id: user_id) # rubocop: disable CodeReuse/ActiveRecord
end
end
end
......
......@@ -96,42 +96,22 @@ module API
optional :invite_source, type: String, desc: 'Source that triggered the member creation process', default: 'members-api'
optional :areas_of_focus, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Areas the inviter wants the member to focus upon'
end
# rubocop: disable CodeReuse/ActiveRecord
post ":id/members" do
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/333434')
source = find_source(source_type, params[:id])
authorize_admin_source!(source_type, source)
if params[:user_id].to_s.include?(',')
create_service_params = params.except(:user_id).merge({ user_ids: params[:user_id], source: source })
user_id = params[:user_id].to_s
create_service_params = params.except(:user_id).merge({ user_ids: user_id, source: source })
if add_multiple_members?(user_id)
::Members::CreateService.new(current_user, create_service_params).execute
elsif params[:user_id].present?
member = source.members.find_by(user_id: params[:user_id])
conflict!('Member already exists') if member
user = User.find_by_id(params[:user_id])
not_found!('User') unless user
member = create_member(current_user, user, source, params)
if !member
not_allowed! # This currently can only be reached in EE
elsif member.valid? && member.persisted?
present_members(member)
Gitlab::Tracking.event(::Members::CreateService.name,
'create_member',
label: params[:invite_source],
property: 'existing_user',
user: current_user)
track_areas_of_focus(member, params[:areas_of_focus])
else
render_validation_error!(member)
end
elsif add_single_member?(user_id)
add_single_member_by_user_id(create_service_params)
end
end
# rubocop: enable CodeReuse/ActiveRecord
desc 'Updates a member of a group or project.' do
success Entities::Member
......
......@@ -2486,6 +2486,12 @@ RSpec.describe Group do
end
end
describe '#membership_locked?' do
it 'returns false' do
expect(build(:group)).not_to be_membership_locked
end
end
describe '#default_owner' do
let(:group) { build(:group) }
......
......@@ -604,6 +604,12 @@ RSpec.describe Project, factory_default: :keep do
end
end
describe '#membership_locked?' do
it 'returns false' do
expect(build(:project)).not_to be_membership_locked
end
end
describe '#autoclose_referenced_issues' do
context 'when DB entry is nil' do
let(:project) { build(:project, autoclose_referenced_issues: nil) }
......
......@@ -311,36 +311,6 @@ RSpec.describe API::Members do
expect(json_response['status']).to eq('error')
expect(json_response['message']).to eq(error_message)
end
context 'with invite_source considerations', :snowplow do
let(:params) { { user_id: user_ids, access_level: Member::DEVELOPER } }
it 'tracks the invite source as api' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: params
expect_snowplow_event(
category: 'Members::CreateService',
action: 'create_member',
label: 'members-api',
property: 'existing_user',
user: maintainer
)
end
it 'tracks the invite source from params' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: params.merge(invite_source: '_invite_source_')
expect_snowplow_event(
category: 'Members::CreateService',
action: 'create_member',
label: '_invite_source_',
property: 'existing_user',
user: maintainer
)
end
end
end
end
......@@ -410,48 +380,28 @@ RSpec.describe API::Members do
end
context 'with areas_of_focus considerations', :snowplow do
context 'when there is 1 user to add' do
let(:user_id) { stranger.id }
let(:user_id) { stranger.id }
context 'when areas_of_focus is present in params' do
it 'tracks the areas_of_focus' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
expect_snowplow_event(
category: 'Members::CreateService',
action: 'area_of_focus',
label: 'Other',
property: source.members.last.id.to_s
)
end
end
context 'when areas_of_focus is not present in params' do
it 'does not track the areas_of_focus' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: { user_id: user_id, access_level: Member::DEVELOPER }
context 'when areas_of_focus is present in params' do
it 'tracks the areas_of_focus' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
expect_no_snowplow_event(category: 'Members::CreateService', action: 'area_of_focus')
end
expect_snowplow_event(
category: 'Members::CreateService',
action: 'area_of_focus',
label: 'Other',
property: source.members.last.id.to_s
)
end
end
context 'when there are multiple users to add' do
let(:user_id) { [developer.id, stranger.id].join(',') }
context 'when areas_of_focus is not present in params' do
it 'does not track the areas_of_focus' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: { user_id: user_id, access_level: Member::DEVELOPER }
context 'when areas_of_focus is present in params' do
it 'tracks the areas_of_focus' do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
expect_snowplow_event(
category: 'Members::CreateService',
action: 'area_of_focus',
label: 'Other',
property: source.members.last.id.to_s
)
end
expect_no_snowplow_event(category: 'Members::CreateService', action: 'area_of_focus')
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