diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index dd1bc6f5d52bdbf53ccbd2be32c6c3f20995878e..9cc31620d9f5313d196a9736334bd58e53c3586b 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 7e6b83bcc37453c9d0699de8ebb20edea25c579c..52dc396af6af21f8b050ce361300edd5b3e64c6d 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 c3929ded6dd59e46a56b09de3ce4e6c58d30c2c8..2c49fe3833ecceb4d5970b720053e45fb1c0bf7e 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 ff0ac115f5504eb7f571b8ddbc15fbba4abb871c..6ba32d33403afb8fd11346ee5ba5c1ff98db3ea0 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 59a55e42e3831bbbdbc7c0d3a2361e004c075040..15358f80208ea5f024b79c6b1ba78d2e465e486e 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 6d0e30e916f9fe2993d56f4bd45e18a443147828..831f1e635baca56b041c8c4e1f0c0de61eb3fb47 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 04c2782c1254bace1cdfd14ed8244e54bc33fb66..2395445e7fddcb1372b5010dea71680d712f174b 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