diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 4 | ||||
| -rw-r--r-- | app/controllers/concerns/membership_actions.rb | 34 | ||||
| -rw-r--r-- | app/controllers/groups/group_members_controller.rb | 6 | ||||
| -rw-r--r-- | app/controllers/projects/project_members_controller.rb | 6 | ||||
| -rw-r--r-- | app/services/members/destroy_service.rb | 26 | ||||
| -rw-r--r-- | lib/gitlab/access.rb | 2 | ||||
| -rw-r--r-- | spec/services/members/destroy_service_spec.rb | 24 |
7 files changed, 42 insertions, 60 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dd1bc6f5d52..9cc31620d9f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,6 +36,10 @@ class ApplicationController < ActionController::Base render_404 end + rescue_from Gitlab::Access::AccessDeniedError do |exception| + render_403 + end + def redirect_back_or_default(default: root_path, options: {}) redirect_to request.referer.present? ? :back : default, options end diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index 7e6b83bcc37..52dc396af6a 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -21,32 +21,18 @@ module MembershipActions def leave @member = membershipable.members.find_by(user_id: current_user) - return render_403 unless @member - - @member = Members::DestroyService.new(@member, current_user).execute + Members::DestroyService.new(@member, current_user).execute source_type = @member.real_source_type.humanize(capitalize: false) - - if @member.destroyed? - notice = - if @member.request? - "Your access request to the #{source_type} has been withdrawn." - else - "You left the \"#{@member.source.human_name}\" #{source_type}." - end - redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] - - redirect_to redirect_path, notice: notice - else - if cannot_leave? - alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}." - alert << " Transfer or delete the #{source_type}." - - redirect_to polymorphic_url(membershipable), alert: alert + notice = + if @member.request? + "Your access request to the #{source_type} has been withdrawn." else - render_403 + "You left the \"#{@member.source.human_name}\" #{source_type}." end - end + redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] + + redirect_to redirect_path, notice: notice end protected @@ -54,8 +40,4 @@ module MembershipActions def membershipable raise NotImplementedError end - - def cannot_leave? - raise NotImplementedError - end end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index c3929ded6dd..2c49fe3833e 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -36,8 +36,6 @@ class Groups::GroupMembersController < Groups::ApplicationController def destroy @group_member = @group.group_members.find(params[:id]) - return render_403 unless can?(current_user, :destroy_group_member, @group_member) - Members::DestroyService.new(@group_member, current_user).execute respond_to do |format| @@ -68,8 +66,4 @@ class Groups::GroupMembersController < Groups::ApplicationController # MembershipActions concern alias_method :membershipable, :group - - def cannot_leave? - @group.last_owner?(current_user) - end end diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index ff0ac115f55..6ba32d33403 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -50,8 +50,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController def destroy @project_member = @project.project_members.find(params[:id]) - return render_403 unless can?(current_user, :destroy_project_member, @project_member) - Members::DestroyService.new(@project_member, current_user).execute respond_to do |format| @@ -98,8 +96,4 @@ class Projects::ProjectMembersController < Projects::ApplicationController # MembershipActions concern alias_method :membershipable, :project - - def cannot_leave? - current_user == @project.owner - end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 59a55e42e38..15358f80208 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -7,29 +7,15 @@ module Members end def execute - if can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) - member.destroy - - if member.request? && member.user != current_user - notification_service.decline_access_request(member) - end + unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) + raise Gitlab::Access::AccessDeniedError end - member - end - - private - - def abilities - Ability.abilities - end - - def can?(object, action, subject) - abilities.allowed?(object, action, subject) - end + member.destroy - def notification_service - NotificationService.new + if member.request? && member.user != current_user + notification_service.decline_access_request(member) + end end end end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 6d0e30e916f..831f1e635ba 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -5,6 +5,8 @@ # module Gitlab module Access + class AccessDeniedError < StandardError; end + GUEST = 10 REPORTER = 20 DEVELOPER = 30 diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 04c2782c125..2395445e7fd 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -5,13 +5,23 @@ describe Members::DestroyService, services: true do let(:project) { create(:project) } let!(:member) { create(:project_member, source: project) } + context 'when member is nil' do + before do + project.team << [user, :developer] + end + + it 'does not destroy the member' do + expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + context 'when current user cannot destroy the given member' do before do project.team << [user, :developer] end it 'does not destroy the member' do - expect(destroy_member(member, user)).not_to be_destroyed + expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) end end @@ -21,7 +31,9 @@ describe Members::DestroyService, services: true do end it 'destroys the member' do - expect(destroy_member(member, user)).to be_destroyed + destroy_member(member, user) + + expect(member).to be_destroyed end context 'when the given member is a requester' do @@ -42,6 +54,14 @@ describe Members::DestroyService, services: true do destroy_member(member, member.user) end 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) + + destroy_member(member, member.user) + end + end end end |