summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2018-02-16 15:10:22 +0100
committerRémy Coutable <remy@rymai.me>2018-02-27 16:02:22 +0100
commit1c88d92b3fe174a56080575a14d6b473f17f7d8f (patch)
tree7150c48b368e82d70f6bfc228c3d836c7be7c3ac
parente82f629be4b9c91e2611095cd4296e487ed137ef (diff)
downloadgitlab-ce-1c88d92b3fe174a56080575a14d6b473f17f7d8f.tar.gz
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--app/controllers/admin/groups_controller.rb2
-rw-r--r--app/controllers/concerns/membership_actions.rb53
-rw-r--r--app/controllers/groups/group_members_controller.rb20
-rw-r--r--app/controllers/projects/project_members_controller.rb20
-rw-r--r--app/models/concerns/access_requestable.rb2
-rw-r--r--app/models/member.rb9
-rw-r--r--app/services/members/approve_access_request_service.rb15
-rw-r--r--app/services/members/authorized_destroy_service.rb59
-rw-r--r--app/services/members/base_service.rb8
-rw-r--r--app/services/members/create_service.rb4
-rw-r--r--app/services/members/destroy_service.rb52
-rw-r--r--app/services/members/request_access_service.rb4
-rw-r--r--app/services/members/update_service.rb3
-rw-r--r--app/views/shared/members/update.js.haml8
-rw-r--r--app/workers/remove_expired_members_worker.rb2
-rw-r--r--lib/api/access_requests.rb4
-rw-r--r--lib/api/members.rb10
-rw-r--r--lib/api/v3/members.rb2
-rw-r--r--spec/services/members/approve_access_request_service_spec.rb31
-rw-r--r--spec/services/members/authorized_destroy_service_spec.rb110
-rw-r--r--spec/services/members/create_service_spec.rb6
-rw-r--r--spec/services/members/destroy_service_spec.rb143
-rw-r--r--spec/services/members/request_access_service_spec.rb6
-rw-r--r--spec/services/members/update_service_spec.rb4
24 files changed, 251 insertions, 326 deletions
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index a94726887d9..cc38608eda5 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -48,7 +48,7 @@ class Admin::GroupsController < Admin::ApplicationController
def members_update
member_params = params.permit(:user_ids, :access_level, :expires_at)
- result = Members::CreateService.new(@group, current_user, member_params.merge(limit: -1)).execute
+ result = Members::CreateService.new(current_user, member_params.merge(limit: -1)).execute(@group)
if result[:status] == :success
redirect_to [:admin, @group], notice: 'Users were successfully added.'
diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index 82fdb797d2a..7a6a00b8e13 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -3,33 +3,31 @@ module MembershipActions
def create
create_params = params.permit(:user_ids, :access_level, :expires_at)
- result = Members::CreateService.new(membershipable, current_user, create_params).execute
-
- redirect_url = members_page_url
+ result = Members::CreateService.new(current_user, create_params).execute(membershipable)
if result[:status] == :success
- redirect_to redirect_url, notice: 'Users were successfully added.'
+ redirect_to members_page_url, notice: 'Users were successfully added.'
else
- redirect_to redirect_url, alert: result[:message]
+ redirect_to members_page_url, alert: result[:message]
end
end
def update
+ update_params = params.require(root_params_key).permit(:access_level, :expires_at)
member = membershipable.members_and_requesters.find(params[:id])
- @member = Members::UpdateService
- .new(membershipable, current_user, member_params)
+ member = Members::UpdateService
+ .new(current_user, update_params)
.execute(member)
.present(current_user: current_user)
respond_to do |format|
- format.js { render 'shared/members/update' }
+ format.js { render 'shared/members/update', locals: { member: member } }
end
end
def destroy
member = membershipable.members_and_requesters.find(params[:id])
- Members::DestroyService.new(membershipable, current_user, params)
- .execute(member)
+ Members::DestroyService.new(current_user).execute(member)
respond_to do |format|
format.html do
@@ -51,7 +49,7 @@ module MembershipActions
def approve_access_request
access_requester = membershipable.requesters.find(params[:id])
Members::ApproveAccessRequestService
- .new(membershipable, current_user, params)
+ .new(current_user, params)
.execute(access_requester)
redirect_to members_page_url
@@ -59,8 +57,7 @@ module MembershipActions
def leave
member = membershipable.members_and_requesters.find_by!(user_id: current_user.id)
- Members::DestroyService.new(membershipable, current_user)
- .execute(member)
+ Members::DestroyService.new(current_user).execute(member)
notice =
if member.request?
@@ -79,17 +76,43 @@ module MembershipActions
end
end
+ def resend_invite
+ member = membershipable.members.find(params[:id])
+
+ if member.invite?
+ member.resend_invite
+
+ redirect_to members_page_url, notice: 'The invitation was successfully resent.'
+ else
+ redirect_to members_page_url, alert: 'The invitation has already been accepted.'
+ end
+ end
+
protected
def membershipable
raise NotImplementedError
end
+ def root_params_key
+ case membershipable
+ when Namespace
+ :group_member
+ when Project
+ :project_member
+ else
+ raise "Unknown membershipable type: #{membershipable}!"
+ end
+ end
+
def members_page_url
- if membershipable.is_a?(Project)
+ case membershipable
+ when Namespace
+ polymorphic_url([membershipable, :members])
+ when Project
project_project_members_path(membershipable)
else
- polymorphic_url([membershipable, :members])
+ raise "Unknown membershipable type: #{membershipable}!"
end
end
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 23ade14edfd..f210434b2d7 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -27,26 +27,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
@group_member = @group.group_members.new
end
- def resend_invite
- redirect_path = group_group_members_path(@group)
-
- @group_member = @group.group_members.find(params[:id])
-
- if @group_member.invite?
- @group_member.resend_invite
-
- redirect_to redirect_path, notice: 'The invitation was successfully resent.'
- else
- redirect_to redirect_path, alert: 'The invitation has already been accepted.'
- end
- end
-
- protected
-
- def member_params
- params.require(:group_member).permit(:access_level, :user_id, :expires_at)
- end
-
# MembershipActions concern
alias_method :membershipable, :group
end
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 006d5df767c..e9b4679f94c 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -26,20 +26,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
@project_member = @project.project_members.new
end
- def resend_invite
- redirect_path = project_project_members_path(@project)
-
- @project_member = @project.project_members.find(params[:id])
-
- if @project_member.invite?
- @project_member.resend_invite
-
- redirect_to redirect_path, notice: 'The invitation was successfully resent.'
- else
- redirect_to redirect_path, alert: 'The invitation has already been accepted.'
- end
- end
-
def import
@projects = current_user.authorized_projects.order_id_desc
end
@@ -58,12 +44,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
notice: notice)
end
- protected
-
- def member_params
- params.require(:project_member).permit(:user_id, :access_level, :expires_at)
- end
-
# MembershipActions concern
alias_method :membershipable, :project
end
diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb
index 62bc6b809f4..d502e7e54c6 100644
--- a/app/models/concerns/access_requestable.rb
+++ b/app/models/concerns/access_requestable.rb
@@ -8,6 +8,6 @@ module AccessRequestable
extend ActiveSupport::Concern
def request_access(user)
- Members::RequestAccessService.new(self, user).execute
+ Members::RequestAccessService.new(user).execute(self)
end
end
diff --git a/app/models/member.rb b/app/models/member.rb
index ba040bbeff0..408e8b2d704 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -128,7 +128,7 @@ class Member < ActiveRecord::Base
find_by(invite_token: invite_token)
end
- def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil)
+ def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil, ldap: false)
# `user` can be either a User object, User ID or an email to be invited
member = retrieve_member(source, user, existing_members)
access_level = retrieve_access_level(access_level)
@@ -143,10 +143,13 @@ class Member < ActiveRecord::Base
if member.request?
::Members::ApproveAccessRequestService.new(
- source,
current_user,
access_level: access_level
- ).execute(member)
+ ).execute(
+ member,
+ skip_authorization: ldap,
+ skip_log_audit_event: ldap
+ )
else
member.save
end
diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb
index 19431ac76dc..6be08b590bc 100644
--- a/app/services/members/approve_access_request_service.rb
+++ b/app/services/members/approve_access_request_service.rb
@@ -1,25 +1,20 @@
module Members
class ApproveAccessRequestService < Members::BaseService
- # opts - A hash of options
- # :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])
+ def execute(access_requester, skip_authorization: false, skip_log_audit_event: false)
+ raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_update_access_requester?(access_requester)
access_requester.access_level = params[:access_level] if params[:access_level]
access_requester.accept_request
- after_execute(member: access_requester, **opts)
+ after_execute(member: access_requester, skip_log_audit_event: skip_log_audit_event)
access_requester
end
private
- def can_update_access_requester?(access_requester, ldap)
- access_requester && (
- ldap ||
- can?(current_user, update_member_permission(access_requester), access_requester)
- )
+ def can_update_access_requester?(access_requester)
+ can?(current_user, update_member_permission(access_requester), access_requester)
end
end
end
diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb
deleted file mode 100644
index 793a951d63b..00000000000
--- a/app/services/members/authorized_destroy_service.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-module Members
- class AuthorizedDestroyService < BaseService
- def initialize(current_user = nil)
- @current_user = current_user
- end
-
- def execute(member)
- return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
-
- Member.transaction do
- unassign_issues_and_merge_requests(member) unless member.invite?
- member.notification_setting&.destroy
-
- member.destroy
- end
-
- if member.request? && member.user != current_user
- notification_service.decline_access_request(member)
- end
-
- member
- end
-
- private
-
- def unassign_issues_and_merge_requests(member)
- if member.is_a?(GroupMember)
- issues = Issue.unscoped.select(1)
- .joins(:project)
- .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
-
- # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
- IssueAssignee.unscoped
- .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
- .delete_all
-
- MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
- .execute
- .update_all(assignee_id: nil)
- else
- project = member.source
-
- # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
- issues = Issue.unscoped.select(1)
- .where('issues.id = issue_assignees.issue_id')
- .where(project_id: project.id)
-
- # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
- IssueAssignee.unscoped
- .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
- .delete_all
-
- project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
- end
-
- member.user.invalidate_cache_counts
- end
- end
-end
diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb
index 5c52468aaca..74556fb20cf 100644
--- a/app/services/members/base_service.rb
+++ b/app/services/members/base_service.rb
@@ -1,17 +1,13 @@
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
+ def initialize(current_user = nil, params = {})
@current_user = current_user
@params = params
end
- def after_execute(**args)
+ def after_execute(args)
# overriden in EE::Members modules
end
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 61802ba09ce..bc6a9405aac 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -2,7 +2,7 @@ module Members
class CreateService < Members::BaseService
DEFAULT_LIMIT = 100
- def execute
+ def execute(source)
return error('No users specified.') if params[:user_ids].blank?
user_ids = params[:user_ids].split(',').uniq
@@ -17,7 +17,7 @@ module Members
current_user: current_user
)
- members.compact.each { |member| after_execute(member: member) }
+ members.each { |member| after_execute(member: member) }
success
end
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index 477ab127b2d..b141bfd5fbc 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -1,9 +1,20 @@
module Members
class DestroyService < Members::BaseService
- def execute(member)
- raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
+ def execute(member, skip_authorization: false)
+ raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
- AuthorizedDestroyService.new(current_user).execute(member)
+ return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
+
+ Member.transaction do
+ unassign_issues_and_merge_requests(member) unless member.invite?
+ member.notification_setting&.destroy
+
+ member.destroy
+ end
+
+ if member.request? && member.user != current_user
+ notification_service.decline_access_request(member)
+ end
after_execute(member: member)
@@ -13,7 +24,7 @@ module Members
private
def can_destroy_member?(member)
- member && can?(current_user, destroy_member_permission(member), member)
+ can?(current_user, destroy_member_permission(member), member)
end
def destroy_member_permission(member)
@@ -26,5 +37,38 @@ module Members
raise "Unknown member type: #{member}!"
end
end
+
+ def unassign_issues_and_merge_requests(member)
+ if member.is_a?(GroupMember)
+ issues = Issue.unscoped.select(1)
+ .joins(:project)
+ .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
+
+ # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+ IssueAssignee.unscoped
+ .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+ .delete_all
+
+ MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
+ .execute
+ .update_all(assignee_id: nil)
+ else
+ project = member.source
+
+ # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
+ issues = Issue.unscoped.select(1)
+ .where('issues.id = issue_assignees.issue_id')
+ .where(project_id: project.id)
+
+ # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+ IssueAssignee.unscoped
+ .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+ .delete_all
+
+ project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
+ end
+
+ member.user.invalidate_cache_counts
+ end
end
end
diff --git a/app/services/members/request_access_service.rb b/app/services/members/request_access_service.rb
index e30722efc13..24293b30005 100644
--- a/app/services/members/request_access_service.rb
+++ b/app/services/members/request_access_service.rb
@@ -1,6 +1,6 @@
module Members
class RequestAccessService < Members::BaseService
- def execute
+ def execute(source)
raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
source.members.create(
@@ -12,7 +12,7 @@ module Members
private
def can_request_access?(source)
- source && can?(current_user, :request_access, source)
+ can?(current_user, :request_access, source)
end
end
end
diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb
index ebea9f96069..48b3d59f7bd 100644
--- a/app/services/members/update_service.rb
+++ b/app/services/members/update_service.rb
@@ -2,8 +2,7 @@ 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)
+ raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), member)
old_access_level = member.human_access
diff --git a/app/views/shared/members/update.js.haml b/app/views/shared/members/update.js.haml
index 597d984d0eb..55050bd8a15 100644
--- a/app/views/shared/members/update.js.haml
+++ b/app/views/shared/members/update.js.haml
@@ -1,4 +1,6 @@
+- member = local_assigns.fetch(:member)
+
:plain
- var $listItem = $('#{escape_javascript(render('shared/members/member', member: @member))}');
- $("##{dom_id(@member)} .list-item-name").replaceWith($listItem.find('.list-item-name'));
- gl.utils.localTimeAgo($('.js-timeago'), $("##{dom_id(@member)}"));
+ var $listItem = $('#{escape_javascript(render('shared/members/member', member: member))}');
+ $("##{dom_id(member)} .list-item-name").replaceWith($listItem.find('.list-item-name'));
+ gl.utils.localTimeAgo($('.js-timeago'), $("##{dom_id(member)}"));
diff --git a/app/workers/remove_expired_members_worker.rb b/app/workers/remove_expired_members_worker.rb
index c3974c5ea79..68960f72bf6 100644
--- a/app/workers/remove_expired_members_worker.rb
+++ b/app/workers/remove_expired_members_worker.rb
@@ -5,7 +5,7 @@ class RemoveExpiredMembersWorker
def perform
Member.expired.find_each do |member|
begin
- Members::AuthorizedDestroyService.new.execute(member)
+ Members::DestroyService.new.execute(member, skip_authorization: true)
rescue => ex
logger.error("Expired Member ID=#{member.id} cannot be removed - #{ex}")
end
diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index 23ea05e1105..ae13c248171 100644
--- a/lib/api/access_requests.rb
+++ b/lib/api/access_requests.rb
@@ -55,7 +55,7 @@ module API
access_requester = source.requesters.find_by!(user_id: params[:user_id])
member = ::Members::ApproveAccessRequestService
- .new(source, current_user, declared_params)
+ .new(current_user, declared_params)
.execute(access_requester)
status :created
@@ -73,7 +73,7 @@ module API
member = source.requesters.find_by!(user_id: params[:user_id])
destroy_conditionally!(member) do
- ::Members::DestroyService.new(source, current_user).execute(member)
+ ::Members::DestroyService.new(current_user).execute(member)
end
end
end
diff --git a/lib/api/members.rb b/lib/api/members.rb
index 6367cde04bb..8b12986d09e 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -83,11 +83,9 @@ module API
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)
+ ::Members::UpdateService
+ .new(current_user, declared_params(include_missing: false))
+ .execute(member)
if updated_member.valid?
present updated_member, with: Entities::Member
@@ -105,7 +103,7 @@ module API
member = source.members.find_by!(user_id: params[:user_id])
destroy_conditionally!(member) do
- ::Members::DestroyService.new(source, current_user).execute(member)
+ ::Members::DestroyService.new(current_user).execute(member)
end
end
end
diff --git a/lib/api/v3/members.rb b/lib/api/v3/members.rb
index dddc7d15d42..88dd598f1e9 100644
--- a/lib/api/v3/members.rb
+++ b/lib/api/v3/members.rb
@@ -124,7 +124,7 @@ module API
status(200 )
{ message: "Access revoked", id: params[:user_id].to_i }
else
- ::Members::DestroyService.new(source, current_user).execute(member)
+ ::Members::DestroyService.new(current_user).execute(member)
present member, with: ::API::Entities::Member
end
diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb
index 90c81c413a4..7076571b753 100644
--- a/spec/services/members/approve_access_request_service_spec.rb
+++ b/spec/services/members/approve_access_request_service_spec.rb
@@ -6,28 +6,27 @@ describe Members::ApproveAccessRequestService do
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, current_user, params).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { described_class.new(current_user).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, current_user, params).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
+ expect { described_class.new(current_user).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, current_user, params).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
+ expect { described_class.new(current_user).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
end
it 'returns a <Source>Member' do
- member = described_class.new(source, current_user, params).execute(access_requester, opts)
+ member = described_class.new(current_user).execute(access_requester, opts)
expect(member).to be_a "#{source.class}Member".constantize
expect(member.requested_at).to be_nil
@@ -35,7 +34,7 @@ describe Members::ApproveAccessRequestService do
context 'with a custom access level' do
it 'returns a ProjectMember with the custom access level' do
- member = described_class.new(source, current_user, params.merge(access_level: Gitlab::Access::MASTER)).execute(access_requester, opts)
+ member = described_class.new(current_user, access_level: Gitlab::Access::MASTER).execute(access_requester, opts)
expect(member.access_level).to eq(Gitlab::Access::MASTER)
end
@@ -61,8 +60,8 @@ describe Members::ApproveAccessRequestService do
end
end
- context 'and :ldap option is false' do
- let(:opts) { { ldap: false } }
+ context 'and :skip_authorization option is false' do
+ let(:opts) { { skip_authorization: false } }
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
let(:source) { project }
@@ -73,8 +72,8 @@ describe Members::ApproveAccessRequestService do
end
end
- context 'and :ldap option is true' do
- let(:opts) { { ldap: true } }
+ context 'and :skip_authorization option is true' do
+ let(:opts) { { skip_authorization: true } }
it_behaves_like 'a service approving an access request' do
let(:source) { project }
@@ -84,18 +83,6 @@ describe Members::ApproveAccessRequestService do
let(:source) { group }
end
end
-
- context 'and :ldap param is true' do
- let(:params) { { ldap: true } }
-
- 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
end
context 'when current user cannot approve access request to the project' do
diff --git a/spec/services/members/authorized_destroy_service_spec.rb b/spec/services/members/authorized_destroy_service_spec.rb
deleted file mode 100644
index db3435398bd..00000000000
--- a/spec/services/members/authorized_destroy_service_spec.rb
+++ /dev/null
@@ -1,110 +0,0 @@
-require 'spec_helper'
-
-describe Members::AuthorizedDestroyService do
- let(:member_user) { create(:user) }
- let(:project) { create(:project, :public) }
- let(:group) { create(:group, :public) }
- let(:group_project) { create(:project, :public, group: group) }
-
- def number_of_assigned_issuables(user)
- Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
- end
-
- context 'Invited users' do
- # Regression spec for issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/32504
- it 'destroys invited project member' do
- project.add_developer(member_user)
-
- member = create :project_member, :invited, project: project
-
- expect { described_class.new(member_user).execute(member) }
- .to change { Member.count }.from(3).to(2)
- end
-
- it "doesn't destroy invited project member notification_settings" do
- project.add_developer(member_user)
-
- member = create :project_member, :invited, project: project
-
- expect { described_class.new(member_user).execute(member) }
- .not_to change { NotificationSetting.count }
- end
-
- it 'destroys invited group member' do
- group.add_developer(member_user)
-
- member = create :group_member, :invited, group: group
-
- expect { described_class.new(member_user).execute(member) }
- .to change { Member.count }.from(2).to(1)
- end
-
- it "doesn't destroy invited group member notification_settings" do
- group.add_developer(member_user)
-
- member = create :group_member, :invited, group: group
-
- expect { described_class.new(member_user).execute(member) }
- .not_to change { NotificationSetting.count }
- end
- end
-
- context 'Requested user' 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_user).execute(member) }
- .not_to change { NotificationSetting.count }
- end
- end
-
- context 'Group member' do
- let(:member) { group.members.find_by(user_id: member_user.id) }
-
- before do
- group.add_developer(member_user)
- end
-
- it "unassigns issues and merge requests" do
- issue = create :issue, project: group_project, assignees: [member_user]
- create :issue, assignees: [member_user]
- 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_user).execute(member) }
- .to change { number_of_assigned_issuables(member_user) }.from(4).to(2)
-
- expect(issue.reload.assignee_ids).to be_empty
- expect(merge_request.reload.assignee_id).to be_nil
- end
-
- it 'destroys member notification_settings' do
- group.add_developer(member_user)
- member = group.members.find_by(user_id: member_user.id)
-
- expect { described_class.new(member_user).execute(member) }
- .to change { member_user.notification_settings.count }.by(-1)
- end
- end
-
- context 'Project member' do
- let(:member) { project.members.find_by(user_id: member_user.id) }
-
- before do
- project.add_developer(member_user)
- end
-
- it "unassigns issues and merge requests" 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_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_user).execute(member) }
- .to change { member_user.notification_settings.count }.by(-1)
- end
- end
-end
diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb
index 6bd4718e780..1831c62d788 100644
--- a/spec/services/members/create_service_spec.rb
+++ b/spec/services/members/create_service_spec.rb
@@ -11,7 +11,7 @@ describe Members::CreateService do
it 'adds user to members' do
params = { user_ids: project_user.id.to_s, access_level: Gitlab::Access::GUEST }
- result = described_class.new(project, user, params).execute
+ result = described_class.new(user, params).execute(project)
expect(result[:status]).to eq(:success)
expect(project.users).to include project_user
@@ -19,7 +19,7 @@ describe Members::CreateService do
it 'adds no user to members' do
params = { user_ids: '', access_level: Gitlab::Access::GUEST }
- result = described_class.new(project, user, params).execute
+ result = described_class.new(user, params).execute(project)
expect(result[:status]).to eq(:error)
expect(result[:message]).to be_present
@@ -30,7 +30,7 @@ describe Members::CreateService do
user_ids = 1.upto(101).to_a.join(',')
params = { user_ids: user_ids, access_level: Gitlab::Access::GUEST }
- result = described_class.new(project, user, params).execute
+ result = described_class.new(user, params).execute(project)
expect(result[:status]).to eq(:error)
expect(result[:message]).to be_present
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index 7464d73fd26..10c264a90c5 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/spec/services/members/destroy_service_spec.rb
@@ -3,113 +3,200 @@ require 'spec_helper'
describe Members::DestroyService do
let(:current_user) { create(:user) }
let(:member_user) { create(:user) }
- let(:project) { create(:project, :public) }
let(:group) { create(:group, :public) }
+ let(:group_project) { create(:project, :public, group: group) }
+ let(:opts) { {} }
shared_examples 'a service raising ActiveRecord::RecordNotFound' do
it 'raises ActiveRecord::RecordNotFound' do
- expect { described_class.new(source, current_user).execute(member) }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { described_class.new(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, current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
+ expect { described_class.new(current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
+ def number_of_assigned_issuables(user)
+ Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
+ end
+
shared_examples 'a service destroying a member' do
it 'destroys the member' do
- expect { described_class.new(source, current_user).execute(member) }.to change { source.members.count }.by(-1)
+ expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1)
+ end
+
+ it 'unassigns issues and merge requests' do
+ if member.invite?
+ expect { described_class.new(current_user).execute(member, opts) }
+ .not_to change { number_of_assigned_issuables(member_user) }
+ else
+ create :issue, assignees: [member_user]
+ issue = create :issue, project: group_project, assignees: [member_user]
+ merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
+
+ expect { described_class.new(current_user).execute(member, opts) }
+ .to change { number_of_assigned_issuables(member_user) }.from(3).to(1)
+
+ expect(issue.reload.assignee_ids).to be_empty
+ expect(merge_request.reload.assignee_id).to be_nil
+ end
+ end
+
+ it 'destroys member notification_settings' do
+ if member_user.notification_settings.any?
+ expect { described_class.new(current_user).execute(member, opts) }
+ .to change { member_user.notification_settings.count }.by(-1)
+ else
+ expect { described_class.new(current_user).execute(member, opts) }
+ .not_to change { member_user.notification_settings.count }
+ end
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
+ it_behaves_like 'a service destroying a member'
it 'calls Member#after_decline_request' do
- expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
+ expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member)
- described_class.new(source, current_user).execute(access_requester)
+ described_class.new(current_user).execute(member)
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)
+ expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member)
- described_class.new(source, member_user).execute(access_requester)
+ described_class.new(member_user).execute(member)
end
end
end
context 'with a member' do
before do
- project.add_developer(member_user)
+ group_project.add_developer(member_user)
group.add_developer(member_user)
end
- 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
- let(:source) { project }
+ let(:member) { group_project.members.find_by(user_id: member_user.id) }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { group_project.members.find_by(user_id: member_user.id) }
end
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
- let(:source) { group }
+ let(:member) { group.members.find_by(user_id: member_user.id) }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { group.members.find_by(user_id: member_user.id) }
end
end
context 'when current user can destroy the given member' do
before do
- project.add_master(current_user)
+ group_project.add_master(current_user)
group.add_owner(current_user)
end
it_behaves_like 'a service destroying a member' do
- let(:source) { project }
+ let(:member) { group_project.members.find_by(user_id: member_user.id) }
end
it_behaves_like 'a service destroying a member' do
- let(:source) { group }
+ let(:member) { group.members.find_by(user_id: member_user.id) }
end
end
end
context 'with an access requester' do
before do
- project.update_attributes(request_access_enabled: true)
+ group_project.update_attributes(request_access_enabled: true)
group.update_attributes(request_access_enabled: true)
- project.request_access(member_user)
+ group_project.request_access(member_user)
group.request_access(member_user)
end
- let(:access_requester) { source.requesters.find_by(user_id: member_user.id) }
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 }
+ let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
end
it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
- let(:source) { group }
- let(:member) { access_requester }
+ let(:member) { group.requesters.find_by(user_id: member_user.id) }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { group.requesters.find_by(user_id: member_user.id) }
end
end
context 'when current user can destroy the given access requester' do
before do
- project.add_master(current_user)
+ group_project.add_master(current_user)
group.add_owner(current_user)
end
it_behaves_like 'a service destroying an access requester' do
- let(:source) { project }
+ let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
end
it_behaves_like 'a service destroying an access requester' do
- let(:source) { group }
+ let(:member) { group.requesters.find_by(user_id: member_user.id) }
+ end
+ end
+ end
+
+ context 'with an invited user' do
+ let(:project_invited_member) { create(:project_member, :invited, project: group_project) }
+ let(:group_invited_member) { create(:group_member, :invited, group: group) }
+
+ context 'when current user cannot destroy the given invited user' do
+ it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+ let(:member) { project_invited_member }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { project_invited_member }
+ end
+
+ it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+ let(:member) { group_invited_member }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:opts) { { skip_authorization: true } }
+ let(:member) { group_invited_member }
+ end
+ end
+
+ context 'when current user can destroy the given invited user' do
+ before do
+ group_project.add_master(current_user)
+ group.add_owner(current_user)
+ end
+
+ # Regression spec for issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/32504
+ it_behaves_like 'a service destroying a member' do
+ let(:member) { project_invited_member }
+ end
+
+ it_behaves_like 'a service destroying a member' do
+ let(:member) { group_invited_member }
end
end
end
diff --git a/spec/services/members/request_access_service_spec.rb b/spec/services/members/request_access_service_spec.rb
index 0a704bba521..e93ba5a85c0 100644
--- a/spec/services/members/request_access_service_spec.rb
+++ b/spec/services/members/request_access_service_spec.rb
@@ -5,17 +5,17 @@ describe Members::RequestAccessService do
shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
it 'raises Gitlab::Access::AccessDeniedError' do
- expect { described_class.new(source, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+ expect { described_class.new(user).execute(source) }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
shared_examples 'a service creating a access request' do
it 'succeeds' do
- expect { described_class.new(source, user).execute }.to change { source.requesters.count }.by(1)
+ expect { described_class.new(user).execute(source) }.to change { source.requesters.count }.by(1)
end
it 'returns a <Source>Member' do
- member = described_class.new(source, user).execute
+ member = described_class.new(user).execute(source)
expect(member).to be_a "#{source.class}Member".constantize
expect(member.requested_at).to be_present
diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb
index 17c5f643f23..a451272dd1f 100644
--- a/spec/services/members/update_service_spec.rb
+++ b/spec/services/members/update_service_spec.rb
@@ -13,14 +13,14 @@ describe Members::UpdateService do
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) }
+ expect { described_class.new(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)
+ updated_member = described_class.new(current_user, params).execute(member, permission: permission)
expect(updated_member).to be_valid
expect(updated_member.access_level).to eq(Gitlab::Access::MASTER)