From 52fef918deb77c068bffbe636243c7973c37fb78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 28 Jul 2016 19:31:17 +0200 Subject: Improve Members::DestroyService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/concerns/membership_actions.rb | 10 +- app/controllers/groups/group_members_controller.rb | 5 +- .../projects/project_members_controller.rb | 5 +- app/services/members/destroy_service.rb | 38 +++++-- lib/api/access_requests.rb | 4 +- lib/api/members.rb | 17 ++-- spec/requests/api/access_requests_spec.rb | 14 ++- spec/services/members/destroy_service_spec.rb | 112 +++++++++++++-------- 8 files changed, 131 insertions(+), 74 deletions(-) diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index 634a10dc357..5617113e10c 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -15,18 +15,16 @@ module MembershipActions end def leave - @member = membershipable.members.find_by(user_id: current_user) || - membershipable.requesters.find_by(user_id: current_user) - Members::DestroyService.new(@member, current_user).execute + Members::DestroyService.new(membershipable, current_user, user_id: current_user.id).execute(:all) - source_type = @member.real_source_type.humanize(capitalize: false) + source_type = membershipable.class.to_s.humanize(capitalize: false) notice = if @member.request? "Your access request to the #{source_type} has been withdrawn." else - "You left the \"#{@member.source.human_name}\" #{source_type}." + "You left the \"#{membershipable.human_name}\" #{source_type}." end - redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] + redirect_path = @member.request? ? @member.source : [:dashboard, membershipable.class.to_s.tableize] redirect_to redirect_path, notice: notice end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 272164cd0cc..0047fcec2ab 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -40,10 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController end def destroy - @group_member = @group.members.find_by(id: params[:id]) || - @group.requesters.find_by(id: params[:id]) - - Members::DestroyService.new(@group_member, current_user).execute + Members::DestroyService.new(@group, current_user, user_id: params[:id]).execute(:all) respond_to do |format| format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 42a7e5a2c30..c9786840f57 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -55,10 +55,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController end def destroy - @project_member = @project.members.find_by(id: params[:id]) || - @project.requesters.find_by(id: params[:id]) - - Members::DestroyService.new(@project_member, current_user).execute + Members::DestroyService.new(@project, current_user, user_id: params[:id]).execute(:all) respond_to do |format| format.html do diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 9a2bf82ef51..b3d79d577bd 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -1,17 +1,41 @@ module Members class DestroyService < BaseService - attr_accessor :member, :current_user + include MembersHelper - def initialize(member, current_user) - @member = member + attr_accessor :source + + ALLOWED_SCOPES = %i[members requesters all] + + def initialize(source, current_user, params = {}) + @source = source @current_user = current_user + @params = params end - def execute - unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) - raise Gitlab::Access::AccessDeniedError - end + def execute(scope = :members) + raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope) + + member = find_member(scope) + + raise Gitlab::Access::AccessDeniedError if cannot_destroy_member?(member) + AuthorizedDestroyService.new(member, current_user).execute end + + private + + def find_member(scope) + case scope + when :all + source.members.find_by(user_id: params[:user_id]) || + source.requesters.find_by!(user_id: params[:user_id]) + else + source.public_send(scope).find_by!(user_id: params[:user_id]) + end + end + + def cannot_destroy_member?(member) + !member || !can?(current_user, action_member_permission(:destroy, member), member) + end end end diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index b5974429c50..60f90959b9a 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -75,9 +75,7 @@ module API required_attributes! [:user_id] source = find_source(source_type, params[:id]) - access_requester = source.requesters.find_by!(user_id: params[:user_id]) - - ::Members::DestroyService.new(access_requester, current_user).execute + ::Members::DestroyService.new(source, current_user, declared(params)).execute(:requesters) end end end diff --git a/lib/api/members.rb b/lib/api/members.rb index 94c16710d9a..4cc80f09a9b 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -59,19 +59,20 @@ module API authorize_admin_source!(source_type, source) required_attributes! [:user_id, :access_level] - access_requester = source.requesters.find_by(user_id: params[:user_id]) - if access_requester - # We pass current_user = access_requester so that the requester doesn't - # receive a "access denied" email - ::Members::DestroyService.new(access_requester, access_requester.user).execute - end - member = source.members.find_by(user_id: params[:user_id]) # This is to ensure back-compatibility but 409 behavior should be used # for both project and group members in 9.0! conflict!('Member already exists') if source_type == 'group' && member + access_requester = source.requesters.find_by(user_id: params[:user_id]) + if access_requester + # We delete a potential access requester before creating the new member. + # We pass current_user = access_requester so that the requester doesn't + # receive a "access denied" email. + ::Members::DestroyService.new(source, access_requester.user, params).execute(:requesters) + end + unless member source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) member = source.members.find_by(user_id: params[:user_id]) @@ -147,7 +148,7 @@ module API if member.nil? { message: "Access revoked", id: params[:user_id].to_i } else - ::Members::DestroyService.new(member, current_user).execute + ::Members::DestroyService.new(source, current_user, params).execute present member.user, with: Entities::Member, member: member end diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb index 905a7311372..b7e5c2af82a 100644 --- a/spec/requests/api/access_requests_spec.rb +++ b/spec/requests/api/access_requests_spec.rb @@ -195,7 +195,7 @@ describe API::AccessRequests, api: true do end context 'when authenticated as the access requester' do - it 'returns 200' do + it 'deletes the access requester' do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester) @@ -205,7 +205,7 @@ describe API::AccessRequests, api: true do end context 'when authenticated as a master/owner' do - it 'returns 200' do + it 'deletes the access requester' do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master) @@ -213,6 +213,16 @@ describe API::AccessRequests, api: true do end.to change { source.requesters.count }.by(-1) end + context 'user_id matches a member' do + it 'returns 404' do + expect do + delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{developer.id}", master) + + expect(response).to have_http_status(404) + end.not_to change { source.requesters.count } + end + end + context 'user_id does not match an existing access requester' do it 'returns 404' do expect do diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 2395445e7fd..06a6b0083c9 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -2,70 +2,102 @@ require 'spec_helper' describe Members::DestroyService, services: true do let(:user) { create(:user) } - let(:project) { create(:project) } - let!(:member) { create(:project_member, source: project) } + let(:member_user) { create(:user) } + let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } - context 'when member is nil' do - before do - project.team << [user, :developer] + 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) end + end - it 'does not destroy the member' do - expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + 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) end end - context 'when current user cannot destroy the given member' do - before do - project.team << [user, :developer] + 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) end - it 'does not destroy the member' do - expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + context 'when the given member is an access requester' do + before do + source.members.find_by(user_id: member_user).destroy + source.request_access(member_user) + end + let(:access_requester) { source.requesters.find_by(user_id: member_user) } + + it_behaves_like 'a service raising ActiveRecord::RecordNotFound' + + %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 + + 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 + end end end - context 'when current user can destroy the given member' do - before do - project.team << [user, :master] + 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 'destroys the member' do - destroy_member(member, user) + it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do + let(:source) { group } + end + end - expect(member).to be_destroyed + context 'when a member is found' do + before do + project.team << [member_user, :developer] + group.add_developer(member_user) end + let(:params) { { user_id: member_user.id } } - context 'when the given member is a requester' do - before do - member.update_column(:requested_at, Time.now) + context 'when current user cannot destroy the given member' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { project } end - it 'calls Member#after_decline_request' do - expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) - - destroy_member(member, user) + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { group } end + 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(member) - - destroy_member(member, member.user) - end + context 'when current user can destroy the given member' do + before do + project.team << [user, :master] + group.add_owner(user) end - context 'when current user is the member and ' do - it 'does not call Member#after_decline_request' do - expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) + it_behaves_like 'a service destroying a member' do + let(:source) { project } + end - destroy_member(member, member.user) - end + it_behaves_like 'a service destroying a member' do + let(:source) { group } end end end - - def destroy_member(member, user) - Members::DestroyService.new(member, user).execute - end end -- cgit v1.2.1