Raise a new Gitlab::Access::AccessDeniedError when permission is not enough to destroy a member

This is a try for a new approach to put the access checks at the service level.

Signed-off-by: Rémy Coutable <remy@rymai.me>

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

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

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

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

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
+      member.destroy
 
-    private
-
-    def abilities
-      Ability.abilities
-    end
-
-    def can?(object, action, subject)
-      abilities.allowed?(object, action, subject)
-    end
-
-    def notification_service
-      NotificationService.new
+      if member.request? && member.user != current_user
+        notification_service.decline_access_request(member)
+      end
     end
   end
 end

lib/gitlab/access.rb

@@ -5,6 +5,8 @@
 #
 module Gitlab
   module Access
+    class AccessDeniedError < StandardError; end
+
     GUEST     = 10
     REPORTER  = 20
     DEVELOPER = 30

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