Remove explicit audit event log in MembershipActions
Move it to Members::ApproveAccessRequestService. Also, note that there was a double audit event log for access request destruction. Signed-off-by: Rémy Coutable <remy@rymai.me>
This commit is contained in:
parent
3bf448267b
commit
bf41063679
|
@ -15,8 +15,9 @@ module MembershipActions
|
|||
end
|
||||
|
||||
def destroy
|
||||
member = membershipable.members_and_requesters.find(params[:id])
|
||||
Members::DestroyService.new(membershipable, current_user, params)
|
||||
.execute(:all)
|
||||
.execute(member)
|
||||
|
||||
respond_to do |format|
|
||||
format.html do
|
||||
|
@ -36,14 +37,18 @@ module MembershipActions
|
|||
end
|
||||
|
||||
def approve_access_request
|
||||
Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute
|
||||
access_requester = membershipable.requesters.find(params[:id])
|
||||
Members::ApproveAccessRequestService
|
||||
.new(membershipable, current_user, params)
|
||||
.execute(access_requester)
|
||||
|
||||
redirect_to members_page_url
|
||||
end
|
||||
|
||||
def leave
|
||||
member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id)
|
||||
.execute(:all)
|
||||
member = membershipable.members_and_requesters.find_by!(user_id: current_user.id)
|
||||
Members::DestroyService.new(membershipable, current_user)
|
||||
.execute(member)
|
||||
|
||||
notice =
|
||||
if member.request?
|
||||
|
|
|
@ -28,12 +28,11 @@ class Groups::GroupMembersController < Groups::ApplicationController
|
|||
end
|
||||
|
||||
def update
|
||||
@group_member = @group.members_and_requesters.find(params[:id])
|
||||
member = @group.members_and_requesters.find(params[:id])
|
||||
@group_member = Members::UpdateService
|
||||
.new(@group, current_user, member_params)
|
||||
.execute(member)
|
||||
.present(current_user: current_user)
|
||||
|
||||
return render_403 unless can?(current_user, :update_group_member, @group_member)
|
||||
|
||||
@group_member.update_attributes(member_params)
|
||||
end
|
||||
|
||||
def resend_invite
|
||||
|
|
|
@ -27,12 +27,11 @@ class Projects::ProjectMembersController < Projects::ApplicationController
|
|||
end
|
||||
|
||||
def update
|
||||
@project_member = @project.members_and_requesters.find(params[:id])
|
||||
member = @project.members_and_requesters.find(params[:id])
|
||||
@project_member = Members::UpdateService
|
||||
.new(@project, current_user, member_params)
|
||||
.execute(member)
|
||||
.present(current_user: current_user)
|
||||
|
||||
return render_403 unless can?(current_user, :update_project_member, @project_member)
|
||||
|
||||
@project_member.update_attributes(member_params)
|
||||
end
|
||||
|
||||
def resend_invite
|
||||
|
|
|
@ -145,9 +145,8 @@ class Member < ActiveRecord::Base
|
|||
::Members::ApproveAccessRequestService.new(
|
||||
source,
|
||||
current_user,
|
||||
id: member.id,
|
||||
access_level: access_level
|
||||
).execute
|
||||
).execute(member)
|
||||
else
|
||||
member.save
|
||||
end
|
||||
|
|
|
@ -1,51 +1,25 @@
|
|||
module Members
|
||||
class ApproveAccessRequestService < BaseService
|
||||
include MembersHelper
|
||||
|
||||
attr_accessor :source
|
||||
|
||||
# source - The source object that respond to `#requesters` (i.g. project or group)
|
||||
# current_user - The user that performs the access request approval
|
||||
# params - A hash of parameters
|
||||
# :user_id - User ID used to retrieve the access requester
|
||||
# :id - Member ID used to retrieve the access requester
|
||||
# :access_level - Optional access level set when the request is accepted
|
||||
def initialize(source, current_user, params = {})
|
||||
@source = source
|
||||
@current_user = current_user
|
||||
@params = params.slice(:user_id, :id, :access_level)
|
||||
end
|
||||
|
||||
class ApproveAccessRequestService < Members::BaseService
|
||||
# opts - A hash of options
|
||||
# :force - Bypass permission check: current_user can be nil in that case
|
||||
def execute(opts = {})
|
||||
condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] }
|
||||
access_requester = source.requesters.find_by!(condition)
|
||||
|
||||
raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts)
|
||||
# :ldap - The call is from a LDAP sync: current_user can be nil in that case
|
||||
def execute(access_requester, opts = {})
|
||||
raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts[:ldap])
|
||||
|
||||
access_requester.access_level = params[:access_level] if params[:access_level]
|
||||
access_requester.accept_request
|
||||
|
||||
after_execute(member: access_requester, **opts)
|
||||
|
||||
access_requester
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def can_update_access_requester?(access_requester, opts = {})
|
||||
def can_update_access_requester?(access_requester, ldap)
|
||||
access_requester && (
|
||||
opts[:force] ||
|
||||
ldap ||
|
||||
can?(current_user, update_member_permission(access_requester), access_requester)
|
||||
)
|
||||
end
|
||||
|
||||
def update_member_permission(member)
|
||||
case member
|
||||
when GroupMember
|
||||
:update_group_member
|
||||
when ProjectMember
|
||||
:update_project_member
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,12 +1,10 @@
|
|||
module Members
|
||||
class AuthorizedDestroyService < BaseService
|
||||
attr_accessor :member, :user
|
||||
|
||||
def initialize(member, user = nil)
|
||||
@member, @user = member, user
|
||||
def initialize(current_user = nil)
|
||||
@current_user = current_user
|
||||
end
|
||||
|
||||
def execute
|
||||
def execute(member)
|
||||
return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
|
||||
|
||||
Member.transaction do
|
||||
|
@ -16,7 +14,7 @@ module Members
|
|||
member.destroy
|
||||
end
|
||||
|
||||
if member.request? && member.user != user
|
||||
if member.request? && member.user != current_user
|
||||
notification_service.decline_access_request(member)
|
||||
end
|
||||
|
||||
|
@ -36,7 +34,7 @@ module Members
|
|||
.where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
|
||||
.delete_all
|
||||
|
||||
MergeRequestsFinder.new(user, group_id: member.source_id, assignee_id: member.user_id)
|
||||
MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
|
||||
.execute
|
||||
.update_all(assignee_id: nil)
|
||||
else
|
||||
|
|
|
@ -0,0 +1,53 @@
|
|||
module Members
|
||||
class BaseService < ::BaseService
|
||||
attr_accessor :source
|
||||
|
||||
# source - The source object that respond to `#members` (e.g. project or group)
|
||||
# current_user - The user that performs the action
|
||||
# params - A hash of parameters
|
||||
def initialize(source, current_user, params = {})
|
||||
@source = source
|
||||
@current_user = current_user
|
||||
@params = params
|
||||
end
|
||||
|
||||
def after_execute(**args)
|
||||
# overriden in EE::Members modules
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def update_member_permission(member)
|
||||
case member
|
||||
when GroupMember
|
||||
:update_group_member
|
||||
when ProjectMember
|
||||
:update_project_member
|
||||
else
|
||||
raise "Unknown member type: #{member}!"
|
||||
end
|
||||
end
|
||||
|
||||
def override_member_permission(member)
|
||||
case member
|
||||
when GroupMember
|
||||
:override_group_member
|
||||
when ProjectMember
|
||||
:override_project_member
|
||||
else
|
||||
raise "Unknown member type: #{member}!"
|
||||
end
|
||||
end
|
||||
|
||||
def action_member_permission(action, member)
|
||||
case action
|
||||
when :update
|
||||
update_member_permission(member)
|
||||
when :override
|
||||
override_member_permission(member)
|
||||
else
|
||||
raise "Unknown action '#{action}' on #{member}!"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1,14 +1,7 @@
|
|||
module Members
|
||||
class CreateService < BaseService
|
||||
class CreateService < Members::BaseService
|
||||
DEFAULT_LIMIT = 100
|
||||
|
||||
def initialize(source, current_user, params = {})
|
||||
@source = source
|
||||
@current_user = current_user
|
||||
@params = params
|
||||
@error = nil
|
||||
end
|
||||
|
||||
def execute
|
||||
return error('No users specified.') if params[:user_ids].blank?
|
||||
|
||||
|
@ -17,13 +10,15 @@ module Members
|
|||
return error("Too many users specified (limit is #{user_limit})") if
|
||||
user_limit && user_ids.size > user_limit
|
||||
|
||||
@source.add_users(
|
||||
members = source.add_users(
|
||||
user_ids,
|
||||
params[:access_level],
|
||||
expires_at: params[:expires_at],
|
||||
current_user: current_user
|
||||
)
|
||||
|
||||
members.compact.each { |member| after_execute(member: member) }
|
||||
|
||||
success
|
||||
end
|
||||
|
||||
|
|
|
@ -1,40 +1,17 @@
|
|||
module Members
|
||||
class DestroyService < BaseService
|
||||
include MembersHelper
|
||||
|
||||
attr_accessor :source
|
||||
|
||||
ALLOWED_SCOPES = %i[members requesters all].freeze
|
||||
|
||||
def initialize(source, current_user, params = {})
|
||||
@source = source
|
||||
@current_user = current_user
|
||||
@params = params
|
||||
end
|
||||
|
||||
def execute(scope = :members)
|
||||
raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope)
|
||||
|
||||
member = find_member!(scope)
|
||||
|
||||
class DestroyService < Members::BaseService
|
||||
def execute(member)
|
||||
raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
|
||||
|
||||
AuthorizedDestroyService.new(member, current_user).execute
|
||||
AuthorizedDestroyService.new(current_user).execute(member)
|
||||
|
||||
after_execute(member: member)
|
||||
|
||||
member
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def find_member!(scope)
|
||||
condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] }
|
||||
case scope
|
||||
when :all
|
||||
source.members.find_by(condition) ||
|
||||
source.requesters.find_by!(condition)
|
||||
else
|
||||
source.public_send(scope).find_by!(condition) # rubocop:disable GitlabSecurity/PublicSend
|
||||
end
|
||||
end
|
||||
|
||||
def can_destroy_member?(member)
|
||||
member && can?(current_user, destroy_member_permission(member), member)
|
||||
end
|
||||
|
@ -45,6 +22,8 @@ module Members
|
|||
:destroy_group_member
|
||||
when ProjectMember
|
||||
:destroy_project_member
|
||||
else
|
||||
raise "Unknown member type: #{member}!"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,12 +1,5 @@
|
|||
module Members
|
||||
class RequestAccessService < BaseService
|
||||
attr_accessor :source
|
||||
|
||||
def initialize(source, current_user)
|
||||
@source = source
|
||||
@current_user = current_user
|
||||
end
|
||||
|
||||
class RequestAccessService < Members::BaseService
|
||||
def execute
|
||||
raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
|
||||
|
||||
|
|
|
@ -0,0 +1,17 @@
|
|||
module Members
|
||||
class UpdateService < Members::BaseService
|
||||
# returns the updated member
|
||||
def execute(member, permission: :update)
|
||||
permission_target = permission == :override ? source : member
|
||||
raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), permission_target)
|
||||
|
||||
old_access_level = member.human_access
|
||||
|
||||
if member.update_attributes(params)
|
||||
after_execute(action: permission, old_access_level: old_access_level, member: member)
|
||||
end
|
||||
|
||||
member
|
||||
end
|
||||
end
|
||||
end
|
|
@ -5,7 +5,7 @@ class RemoveExpiredMembersWorker
|
|||
def perform
|
||||
Member.expired.find_each do |member|
|
||||
begin
|
||||
Members::AuthorizedDestroyService.new(member).execute
|
||||
Members::AuthorizedDestroyService.new.execute(member)
|
||||
rescue => ex
|
||||
logger.error("Expired Member ID=#{member.id} cannot be removed - #{ex}")
|
||||
end
|
||||
|
|
|
@ -53,7 +53,10 @@ module API
|
|||
put ':id/access_requests/:user_id/approve' do
|
||||
source = find_source(source_type, params[:id])
|
||||
|
||||
member = ::Members::ApproveAccessRequestService.new(source, current_user, declared_params).execute
|
||||
access_requester = source.requesters.find_by!(user_id: params[:user_id])
|
||||
member = ::Members::ApproveAccessRequestService
|
||||
.new(source, current_user, declared_params)
|
||||
.execute(access_requester)
|
||||
|
||||
status :created
|
||||
present member, with: Entities::Member
|
||||
|
@ -70,8 +73,7 @@ module API
|
|||
member = source.requesters.find_by!(user_id: params[:user_id])
|
||||
|
||||
destroy_conditionally!(member) do
|
||||
::Members::DestroyService.new(source, current_user, params)
|
||||
.execute(:requesters)
|
||||
::Members::DestroyService.new(source, current_user).execute(member)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -81,12 +81,18 @@ module API
|
|||
source = find_source(source_type, params.delete(:id))
|
||||
authorize_admin_source!(source_type, source)
|
||||
|
||||
member = source.members.find_by!(user_id: params.delete(:user_id))
|
||||
member = source.members.find_by!(user_id: params[:user_id])
|
||||
updated_member =
|
||||
::Members::UpdateService.new(
|
||||
source,
|
||||
current_user,
|
||||
declared_params(include_missing: false)
|
||||
).execute(member)
|
||||
|
||||
if member.update_attributes(declared_params(include_missing: false))
|
||||
present member, with: Entities::Member
|
||||
if updated_member.valid?
|
||||
present updated_member, with: Entities::Member
|
||||
else
|
||||
render_validation_error!(member)
|
||||
render_validation_error!(updated_member)
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -99,7 +105,7 @@ module API
|
|||
member = source.members.find_by!(user_id: params[:user_id])
|
||||
|
||||
destroy_conditionally!(member) do
|
||||
::Members::DestroyService.new(source, current_user, declared_params).execute
|
||||
::Members::DestroyService.new(source, current_user).execute(member)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -124,7 +124,7 @@ module API
|
|||
status(200 )
|
||||
{ message: "Access revoked", id: params[:user_id].to_i }
|
||||
else
|
||||
::Members::DestroyService.new(source, current_user, declared_params).execute
|
||||
::Members::DestroyService.new(source, current_user).execute(member)
|
||||
|
||||
present member, with: ::API::Entities::Member
|
||||
end
|
||||
|
|
|
@ -1,70 +1,57 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Members::ApproveAccessRequestService do
|
||||
let(:user) { create(:user) }
|
||||
let(:access_requester) { create(:user) }
|
||||
let(:project) { create(:project, :public, :access_requestable) }
|
||||
let(:group) { create(:group, :public, :access_requestable) }
|
||||
let(:current_user) { create(:user) }
|
||||
let(:access_requester_user) { create(:user) }
|
||||
let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) }
|
||||
let(:params) { {} }
|
||||
let(:opts) { {} }
|
||||
|
||||
shared_examples 'a service raising ActiveRecord::RecordNotFound' do
|
||||
it 'raises ActiveRecord::RecordNotFound' do
|
||||
expect { described_class.new(source, user, params).execute(opts) }.to raise_error(ActiveRecord::RecordNotFound)
|
||||
expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
it 'raises Gitlab::Access::AccessDeniedError' do
|
||||
expect { described_class.new(source, user, params).execute(opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
|
||||
expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service approving an access request' do
|
||||
it 'succeeds' do
|
||||
expect { described_class.new(source, user, params).execute(opts) }.to change { source.requesters.count }.by(-1)
|
||||
expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
|
||||
end
|
||||
|
||||
it 'returns a <Source>Member' do
|
||||
member = described_class.new(source, user, params).execute(opts)
|
||||
member = described_class.new(source, current_user, params).execute(access_requester, opts)
|
||||
|
||||
expect(member).to be_a "#{source.class}Member".constantize
|
||||
expect(member.requested_at).to be_nil
|
||||
end
|
||||
|
||||
context 'with a custom access level' do
|
||||
let(:params2) { params.merge(user_id: access_requester.id, access_level: Gitlab::Access::MASTER) }
|
||||
|
||||
it 'returns a ProjectMember with the custom access level' do
|
||||
member = described_class.new(source, user, params2).execute(opts)
|
||||
member = described_class.new(source, current_user, params.merge(access_level: Gitlab::Access::MASTER)).execute(access_requester, opts)
|
||||
|
||||
expect(member.access_level).to eq Gitlab::Access::MASTER
|
||||
expect(member.access_level).to eq(Gitlab::Access::MASTER)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when no access requester are found' do
|
||||
let(:params) { { user_id: 42 } }
|
||||
|
||||
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
|
||||
context 'when an access requester is found' do
|
||||
before do
|
||||
project.request_access(access_requester)
|
||||
group.request_access(access_requester)
|
||||
project.request_access(access_requester_user)
|
||||
group.request_access(access_requester_user)
|
||||
end
|
||||
let(:params) { { user_id: access_requester.id } }
|
||||
|
||||
context 'when current user is nil' do
|
||||
let(:user) { nil }
|
||||
|
||||
context 'and :force option is not given' do
|
||||
context 'and :ldap option is not given' do
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
@ -74,8 +61,8 @@ describe Members::ApproveAccessRequestService do
|
|||
end
|
||||
end
|
||||
|
||||
context 'and :force option is false' do
|
||||
let(:opts) { { force: false } }
|
||||
context 'and :ldap option is false' do
|
||||
let(:opts) { { ldap: false } }
|
||||
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { project }
|
||||
|
@ -86,8 +73,8 @@ describe Members::ApproveAccessRequestService do
|
|||
end
|
||||
end
|
||||
|
||||
context 'and :force option is true' do
|
||||
let(:opts) { { force: true } }
|
||||
context 'and :ldap option is true' do
|
||||
let(:opts) { { ldap: true } }
|
||||
|
||||
it_behaves_like 'a service approving an access request' do
|
||||
let(:source) { project }
|
||||
|
@ -98,8 +85,8 @@ describe Members::ApproveAccessRequestService do
|
|||
end
|
||||
end
|
||||
|
||||
context 'and :force param is true' do
|
||||
let(:params) { { user_id: access_requester.id, force: true } }
|
||||
context 'and :ldap param is true' do
|
||||
let(:params) { { ldap: true } }
|
||||
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { project }
|
||||
|
@ -123,8 +110,8 @@ describe Members::ApproveAccessRequestService do
|
|||
|
||||
context 'when current user can approve access request to the project' do
|
||||
before do
|
||||
project.add_master(user)
|
||||
group.add_owner(user)
|
||||
project.add_master(current_user)
|
||||
group.add_owner(current_user)
|
||||
end
|
||||
|
||||
it_behaves_like 'a service approving an access request' do
|
||||
|
@ -134,14 +121,6 @@ describe Members::ApproveAccessRequestService do
|
|||
it_behaves_like 'a service approving an access request' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
context 'when given a :id' do
|
||||
let(:params) { { id: project.requesters.find_by!(user_id: access_requester.id).id } }
|
||||
|
||||
it_behaves_like 'a service approving an access request' do
|
||||
let(:source) { project }
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -17,7 +17,7 @@ describe Members::AuthorizedDestroyService do
|
|||
|
||||
member = create :project_member, :invited, project: project
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { Member.count }.from(3).to(2)
|
||||
end
|
||||
|
||||
|
@ -26,7 +26,7 @@ describe Members::AuthorizedDestroyService do
|
|||
|
||||
member = create :project_member, :invited, project: project
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.not_to change { NotificationSetting.count }
|
||||
end
|
||||
|
||||
|
@ -35,7 +35,7 @@ describe Members::AuthorizedDestroyService do
|
|||
|
||||
member = create :group_member, :invited, group: group
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { Member.count }.from(2).to(1)
|
||||
end
|
||||
|
||||
|
@ -44,7 +44,7 @@ describe Members::AuthorizedDestroyService do
|
|||
|
||||
member = create :group_member, :invited, group: group
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.not_to change { NotificationSetting.count }
|
||||
end
|
||||
end
|
||||
|
@ -53,7 +53,7 @@ describe Members::AuthorizedDestroyService do
|
|||
it "doesn't destroy member notification_settings" do
|
||||
member = create(:project_member, user: member_user, requested_at: Time.now)
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.not_to change { NotificationSetting.count }
|
||||
end
|
||||
end
|
||||
|
@ -71,7 +71,7 @@ describe Members::AuthorizedDestroyService do
|
|||
merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
|
||||
create :merge_request, target_project: project, source_project: project, assignee: member_user
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { number_of_assigned_issuables(member_user) }.from(4).to(2)
|
||||
|
||||
expect(issue.reload.assignee_ids).to be_empty
|
||||
|
@ -82,7 +82,7 @@ describe Members::AuthorizedDestroyService do
|
|||
group.add_developer(member_user)
|
||||
member = group.members.find_by(user_id: member_user.id)
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { member_user.notification_settings.count }.by(-1)
|
||||
end
|
||||
end
|
||||
|
@ -98,12 +98,12 @@ describe Members::AuthorizedDestroyService do
|
|||
create :issue, project: project, assignees: [member_user]
|
||||
create :merge_request, target_project: project, source_project: project, assignee: member_user
|
||||
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { number_of_assigned_issuables(member_user) }.from(2).to(0)
|
||||
end
|
||||
|
||||
it 'destroys member notification_settings' do
|
||||
expect { described_class.new(member, member_user).execute }
|
||||
expect { described_class.new(member_user).execute(member) }
|
||||
.to change { member_user.notification_settings.count }.by(-1)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,80 +1,55 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Members::DestroyService do
|
||||
let(:user) { create(:user) }
|
||||
let(:current_user) { create(:user) }
|
||||
let(:member_user) { create(:user) }
|
||||
let(:project) { create(:project, :public) }
|
||||
let(:group) { create(:group, :public) }
|
||||
|
||||
shared_examples 'a service raising ActiveRecord::RecordNotFound' do
|
||||
it 'raises ActiveRecord::RecordNotFound' do
|
||||
expect { described_class.new(source, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
|
||||
expect { described_class.new(source, current_user).execute(member) }.to raise_error(ActiveRecord::RecordNotFound)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
it 'raises Gitlab::Access::AccessDeniedError' do
|
||||
expect { described_class.new(source, user, params).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
|
||||
expect { described_class.new(source, current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service destroying a member' do
|
||||
it 'destroys the member' do
|
||||
expect { described_class.new(source, user, params).execute }.to change { source.members.count }.by(-1)
|
||||
expect { described_class.new(source, current_user).execute(member) }.to change { source.members.count }.by(-1)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service destroying an access requester' do
|
||||
it 'destroys the access requester' do
|
||||
expect { described_class.new(source, current_user).execute(access_requester) }.to change { source.requesters.count }.by(-1)
|
||||
end
|
||||
|
||||
context 'when the given member is an access requester' do
|
||||
before do
|
||||
source.members.find_by(user_id: member_user).destroy
|
||||
source.update_attributes(request_access_enabled: true)
|
||||
source.request_access(member_user)
|
||||
end
|
||||
let(:access_requester) { source.requesters.find_by(user_id: member_user) }
|
||||
it 'calls Member#after_decline_request' do
|
||||
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
|
||||
|
||||
it_behaves_like 'a service raising ActiveRecord::RecordNotFound'
|
||||
described_class.new(source, current_user).execute(access_requester)
|
||||
end
|
||||
|
||||
%i[requesters all].each do |scope|
|
||||
context "and #{scope} scope is passed" do
|
||||
it 'destroys the access requester' do
|
||||
expect { described_class.new(source, user, params).execute(scope) }.to change { source.requesters.count }.by(-1)
|
||||
end
|
||||
context 'when current user is the member' do
|
||||
it 'does not call Member#after_decline_request' do
|
||||
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
|
||||
|
||||
it 'calls Member#after_decline_request' do
|
||||
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
|
||||
|
||||
described_class.new(source, user, params).execute(scope)
|
||||
end
|
||||
|
||||
context 'when current user is the member' do
|
||||
it 'does not call Member#after_decline_request' do
|
||||
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
|
||||
|
||||
described_class.new(source, member_user, params).execute(scope)
|
||||
end
|
||||
end
|
||||
end
|
||||
described_class.new(source, member_user).execute(access_requester)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when no member are found' do
|
||||
let(:params) { { user_id: 42 } }
|
||||
|
||||
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
|
||||
context 'when a member is found' do
|
||||
context 'with a member' do
|
||||
before do
|
||||
project.add_developer(member_user)
|
||||
group.add_developer(member_user)
|
||||
end
|
||||
let(:params) { { user_id: member_user.id } }
|
||||
let(:member) { source.members.find_by(user_id: member_user.id) }
|
||||
|
||||
context 'when current user cannot destroy the given member' do
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
|
@ -88,8 +63,8 @@ describe Members::DestroyService do
|
|||
|
||||
context 'when current user can destroy the given member' do
|
||||
before do
|
||||
project.add_master(user)
|
||||
group.add_owner(user)
|
||||
project.add_master(current_user)
|
||||
group.add_owner(current_user)
|
||||
end
|
||||
|
||||
it_behaves_like 'a service destroying a member' do
|
||||
|
@ -99,14 +74,42 @@ describe Members::DestroyService do
|
|||
it_behaves_like 'a service destroying a member' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when given a :id' do
|
||||
let(:params) { { id: project.members.find_by!(user_id: user.id).id } }
|
||||
context 'with an access requester' do
|
||||
before do
|
||||
project.update_attributes(request_access_enabled: true)
|
||||
group.update_attributes(request_access_enabled: true)
|
||||
project.request_access(member_user)
|
||||
group.request_access(member_user)
|
||||
end
|
||||
let(:access_requester) { source.requesters.find_by(user_id: member_user.id) }
|
||||
|
||||
it 'destroys the member' do
|
||||
expect { described_class.new(project, user, params).execute }
|
||||
.to change { project.members.count }.by(-1)
|
||||
end
|
||||
context 'when current user cannot destroy the given access requester' do
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { project }
|
||||
let(:member) { access_requester }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { group }
|
||||
let(:member) { access_requester }
|
||||
end
|
||||
end
|
||||
|
||||
context 'when current user can destroy the given access requester' do
|
||||
before do
|
||||
project.add_master(current_user)
|
||||
group.add_owner(current_user)
|
||||
end
|
||||
|
||||
it_behaves_like 'a service destroying an access requester' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service destroying an access requester' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,59 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Members::UpdateService do
|
||||
let(:project) { create(:project, :public) }
|
||||
let(:group) { create(:group, :public) }
|
||||
let(:current_user) { create(:user) }
|
||||
let(:member_user) { create(:user) }
|
||||
let(:permission) { :update }
|
||||
let(:member) { source.members_and_requesters.find_by!(user_id: member_user.id) }
|
||||
let(:params) do
|
||||
{ access_level: Gitlab::Access::MASTER }
|
||||
end
|
||||
|
||||
shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
it 'raises Gitlab::Access::AccessDeniedError' do
|
||||
expect { described_class.new(source, current_user, params).execute(member, permission: permission) }
|
||||
.to raise_error(Gitlab::Access::AccessDeniedError)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'a service updating a member' do
|
||||
it 'updates the member' do
|
||||
updated_member = described_class.new(source, current_user, params).execute(member, permission: permission)
|
||||
|
||||
expect(updated_member).to be_valid
|
||||
expect(updated_member.access_level).to eq(Gitlab::Access::MASTER)
|
||||
end
|
||||
end
|
||||
|
||||
before do
|
||||
project.add_developer(member_user)
|
||||
group.add_developer(member_user)
|
||||
end
|
||||
|
||||
context 'when current user cannot update the given member' do
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
|
||||
context 'when current user can update the given member' do
|
||||
before do
|
||||
project.add_master(current_user)
|
||||
group.add_owner(current_user)
|
||||
end
|
||||
|
||||
it_behaves_like 'a service updating a member' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'a service updating a member' do
|
||||
let(:source) { group }
|
||||
end
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue