Merge branch '18755-fix-destroy-project-causes-post_decline_request-to-be-executed' into 'master'

Resolve "Destroying a project causes post_decline_request to be executed"

## What does this MR do?

Ensure we don't send "access request declined" to access requesters when a project is deleted.

## Are there points in the code the reviewer needs to double check?

I've created a service to decouple the notification sending from the AR model.

## Why was this MR needed?

Because there was an issue.

## What are the relevant issue numbers?

Fixes #18755, #18750.

## Does this MR meet the acceptance criteria?

- [x] No CHANGELOG needed.
- [x] Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4744

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

12 files changed, 71 insertions, 102 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 a24273fad0b..52dc396af6a 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -21,29 +21,18 @@ module MembershipActions
 
   def leave
     @member = membershipable.members.find_by(user_id: current_user)
-    return render_403 unless @member
+    Members::DestroyService.new(@member, current_user).execute
 
     source_type = @member.real_source_type.humanize(capitalize: false)
-
-    if can?(current_user, action_member_permission(:destroy, @member), @member)
-      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
-      @member.destroy
-
-      redirect_to [:dashboard, @member.real_source_type.tableize], 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
@@ -51,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 d0f2e2949f0..2c49fe3833e 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -36,9 +36,7 @@ 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)
-
-    @group_member.destroy
+    Members::DestroyService.new(@group_member, current_user).execute
 
     respond_to do |format|
       format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
@@ -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 35d067cd029..6ba32d33403 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -50,9 +50,7 @@ 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)
-
-    @project_member.destroy
+    Members::DestroyService.new(@project_member, current_user).execute
 
     respond_to do |format|
       format.html do
@@ -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/models/member.rb b/app/models/member.rb
index 4ee3f1bb5c2..c74a16367db 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -48,7 +48,6 @@ class Member < ActiveRecord::Base
   after_create :post_create_hook, unless: [:pending?, :importing?]
   after_update :post_update_hook, unless: [:pending?, :importing?]
   after_destroy :post_destroy_hook, unless: :pending?
-  after_destroy :post_decline_request, if: :request?
 
   delegate :name, :username, :email, to: :user, prefix: true
 
@@ -188,7 +187,7 @@ class Member < ActiveRecord::Base
   end
 
   def send_request
-    # override in subclass
+    notification_service.new_access_request(self)
   end
 
   def post_create_hook
@@ -215,10 +214,6 @@ class Member < ActiveRecord::Base
     post_create_hook
   end
 
-  def post_decline_request
-    # override in subclass
-  end
-
   def system_hook_service
     SystemHooksService.new
   end
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 363db877968..2f13d339c89 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -33,12 +33,6 @@ class GroupMember < Member
     super
   end
 
-  def send_request
-    notification_service.new_group_access_request(self)
-
-    super
-  end
-
   def post_create_hook
     notification_service.new_group_member(self)
 
@@ -64,10 +58,4 @@ class GroupMember < Member
 
     super
   end
-
-  def post_decline_request
-    notification_service.decline_group_access_request(self)
-
-    super
-  end
 end
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 250ee04fd1d..e9d3a82ba15 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -111,12 +111,6 @@ class ProjectMember < Member
     super
   end
 
-  def send_request
-    notification_service.new_project_access_request(self)
-
-    super
-  end
-
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
@@ -152,12 +146,6 @@ class ProjectMember < Member
     super
   end
 
-  def post_decline_request
-    notification_service.decline_project_access_request(self)
-
-    super
-  end
-
   def event_service
     EventCreateService.new
   end
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
new file mode 100644
index 00000000000..15358f80208
--- /dev/null
+++ b/app/services/members/destroy_service.rb
@@ -0,0 +1,21 @@
+module Members
+  class DestroyService < BaseService
+    attr_accessor :member, :current_user
+
+    def initialize(member, user)
+      @member, @current_user = member, user
+    end
+
+    def execute
+      unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member)
+        raise Gitlab::Access::AccessDeniedError
+      end
+
+      member.destroy
+
+      if member.request? && member.user != current_user
+        notification_service.decline_access_request(member)
+      end
+    end
+  end
+end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 19832a19b2b..590350a11e5 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -181,15 +181,16 @@ class NotificationService
     end
   end
 
-  # Project access request
-  def new_project_access_request(project_member)
-    mailer.member_access_requested_email(project_member.real_source_type, project_member.id).deliver_later
+  # Members
+  def new_access_request(member)
+    mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
   end
 
-  def decline_project_access_request(project_member)
-    mailer.member_access_denied_email(project_member.real_source_type, project_member.project.id, project_member.user.id).deliver_later
+  def decline_access_request(member)
+    mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later
   end
 
+  # Project invite
   def invite_project_member(project_member, token)
     mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later
   end
@@ -216,15 +217,7 @@ class NotificationService
     mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later
   end
 
-  # Group access request
-  def new_group_access_request(group_member)
-    mailer.member_access_requested_email(group_member.real_source_type, group_member.id).deliver_later
-  end
-
-  def decline_group_access_request(group_member)
-    mailer.member_access_denied_email(group_member.real_source_type, group_member.group.id, group_member.user.id).deliver_later
-  end
-
+  # Group invite
   def invite_group_member(group_member, token)
     mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later
   end
diff --git a/app/views/layouts/nav/_group_settings.html.haml b/app/views/layouts/nav/_group_settings.html.haml
index dac46648b9f..3a24b09ab7e 100644
--- a/app/views/layouts/nav/_group_settings.html.haml
+++ b/app/views/layouts/nav/_group_settings.html.haml
@@ -1,16 +1,22 @@
 - if current_user
-  - if access = @group.users.find_by(id: current_user.id)
-    .controls
-      .dropdown.group-settings-dropdown
-        %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'}
-          = icon('cog')
-          = icon('caret-down')
-        %ul.dropdown-menu.dropdown-menu-align-right
-          - if can?(current_user, :admin_group, @group)
-            = nav_link(path: 'groups#projects') do
-              = link_to projects_group_path(@group), title: 'Projects' do
-                Projects
-            %li.divider
-            %li
-              = link_to edit_group_path(@group) do
-                Edit Group
+  - can_edit = can?(current_user, :admin_group, @group)
+  - member = @group.members.non_request.find_by(user_id: current_user.id)
+  - can_leave = member && can?(current_user, :destroy_group_member, member)
+
+  .controls
+    .dropdown.group-settings-dropdown
+      %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'}
+        = icon('cog')
+        = icon('caret-down')
+      %ul.dropdown-menu.dropdown-menu-align-right
+        = nav_link(path: 'groups#projects') do
+          = link_to 'Projects', projects_group_path(@group), title: 'Projects'
+        %li.divider
+        - if can_edit
+          %li
+            = link_to 'Edit Group', edit_group_path(@group)
+        - if can_leave
+          %li
+            = link_to polymorphic_path([:leave, @group, :members]),
+              data: { confirm: leave_confirmation_message(@group) }, method: :delete, title: 'Leave group' do
+              Leave Group
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 7762746f848..27e840df503 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -5,19 +5,20 @@
         = icon('cog')
         = icon('caret-down')
       %ul.dropdown-menu.dropdown-menu-align-right
-        - is_project_member = @project.users.exists?(current_user.id)
-        - access = @project.team.max_member_access(current_user.id)
         - can_edit = can?(current_user, :admin_project, @project)
+        -# We don't use @project.team.find_member because it searches for group members too...
+        - member = @project.members.non_request.find_by(user_id: current_user.id)
+        - can_leave = member && can?(current_user, :destroy_project_member, member)
 
-        = render 'layouts/nav/project_settings', access: access, can_edit: can_edit
+        = render 'layouts/nav/project_settings', can_edit: can_edit
 
-        - if can_edit || is_project_member
+        - if can_edit || can_leave
           %li.divider
           - if can_edit
             %li
               = link_to edit_project_path(@project) do
                 Edit Project
-          - if is_project_member
+          - if can_leave
             %li
               = link_to polymorphic_path([:leave, @project, :members]),
                 data: { confirm: leave_confirmation_message(@project) }, method: :delete, title: 'Leave project' do
diff --git a/app/views/layouts/nav/_project_settings.html.haml b/app/views/layouts/nav/_project_settings.html.haml
index 13d32bd1354..51a54b4f262 100644
--- a/app/views/layouts/nav/_project_settings.html.haml
+++ b/app/views/layouts/nav/_project_settings.html.haml
@@ -3,7 +3,7 @@
     = link_to namespace_project_project_members_path(@project.namespace, @project), title: 'Members', class: 'team-tab tab' do
       %span
         Members
-- if access && can_edit
+- if can_edit
   - if @project.allowed_to_share_with_group?
     = nav_link(controller: :group_links) do
       = link_to namespace_project_group_links_path(@project.namespace, @project), title: "Groups" do