From 1c88d92b3fe174a56080575a14d6b473f17f7d8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 16 Feb 2018 15:10:22 +0100 Subject: Improve Member services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/admin/groups_controller.rb | 2 +- app/controllers/concerns/membership_actions.rb | 53 +++++++++++++------ app/controllers/groups/group_members_controller.rb | 20 -------- .../projects/project_members_controller.rb | 20 -------- app/models/concerns/access_requestable.rb | 2 +- app/models/member.rb | 9 ++-- .../members/approve_access_request_service.rb | 15 ++---- app/services/members/authorized_destroy_service.rb | 59 ---------------------- app/services/members/base_service.rb | 8 +-- app/services/members/create_service.rb | 4 +- app/services/members/destroy_service.rb | 52 +++++++++++++++++-- app/services/members/request_access_service.rb | 4 +- app/services/members/update_service.rb | 3 +- app/views/shared/members/update.js.haml | 8 +-- app/workers/remove_expired_members_worker.rb | 2 +- 15 files changed, 112 insertions(+), 149 deletions(-) delete mode 100644 app/services/members/authorized_destroy_service.rb (limited to 'app') 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 -- cgit v1.2.3