From 4652489f40a4ff2b749f9ad495986a7a17448243 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 17 Jun 2016 14:06:55 +0200
Subject: New Members::DestroyService
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is to ensure we don't send unwanted notifications when deleting a
project. In other words, stop abusing AR callbacks and use services.

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/controllers/concerns/membership_actions.rb |  6 ++--
 app/models/member.rb                           |  7 +----
 app/models/members/group_member.rb             | 12 -------
 app/models/members/project_member.rb           | 12 -------
 app/services/members/destroy_service.rb        | 33 ++++++++++++++++++++
 app/services/notification_service.rb           | 21 +++++--------
 spec/models/member_spec.rb                     | 12 -------
 spec/models/members/group_member_spec.rb       | 10 ------
 spec/models/members/project_member_spec.rb     | 10 ------
 spec/services/members/destroy_service_spec.rb  | 43 ++++++++++++++++++++++++++
 10 files changed, 88 insertions(+), 78 deletions(-)
 create mode 100644 app/services/members/destroy_service.rb
 create mode 100644 spec/services/members/destroy_service_spec.rb

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index a24273fad0b..b1343576b3c 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -23,22 +23,24 @@ module MembershipActions
     @member = membershipable.members.find_by(user_id: current_user)
     return render_403 unless @member
 
+    @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)
+    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
-      @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
       else
         render_403
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..e32eb47b846
--- /dev/null
+++ b/app/services/members/destroy_service.rb
@@ -0,0 +1,33 @@
+module Members
+  class DestroyService < BaseService
+    attr_accessor :member, :current_user
+
+    def initialize(member, user)
+      @member, @current_user = member, user
+    end
+
+    def execute
+      if can?(current_user, "destroy_#{member.type.underscore}".to_sym, member)
+        member.destroy
+
+        notification_service.decline_access_request(member) if member.request?
+      end
+
+      member
+    end
+
+    private
+
+    def abilities
+      Ability.abilities
+    end
+
+    def can?(object, action, subject)
+      abilities.allowed?(object, action, subject)
+    end
+
+    def notification_service
+      NotificationService.new
+    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/spec/models/member_spec.rb b/spec/models/member_spec.rb
index 3ed3202ac6c..e9134a3d283 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -134,18 +134,6 @@ describe Member, models: true do
     it { is_expected.to respond_to(:user_email) }
   end
 
-  describe 'Callbacks' do
-    describe 'after_destroy :post_decline_request, if: :request?' do
-      let(:member) { create(:project_member, requested_at: Time.now.utc) }
-
-      it 'calls #post_decline_request' do
-        expect(member).to receive(:post_decline_request)
-
-        member.destroy
-      end
-    end
-  end
-
   describe ".add_user" do
     let!(:user)    { create(:user) }
     let(:project) { create(:project) }
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index eeb74a462ac..18439cac2a4 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -61,16 +61,6 @@ describe GroupMember, models: true do
       end
     end
 
-    describe '#post_decline_request' do
-      it 'calls NotificationService.decline_group_access_request' do
-        member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now)
-
-        expect_any_instance_of(NotificationService).to receive(:decline_group_access_request)
-
-        member.__send__(:post_decline_request)
-      end
-    end
-
     describe '#real_source_type' do
       subject { create(:group_member).real_source_type }
 
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 1e466f9c620..bbf65edb27c 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -152,15 +152,5 @@ describe ProjectMember, models: true do
         member.__send__(:after_accept_request)
       end
     end
-
-    describe '#post_decline_request' do
-      it 'calls NotificationService.decline_project_access_request' do
-        member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now)
-
-        expect_any_instance_of(NotificationService).to receive(:decline_project_access_request)
-
-        member.__send__(:post_decline_request)
-      end
-    end
   end
 end
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
new file mode 100644
index 00000000000..aa002b4bd22
--- /dev/null
+++ b/spec/services/members/destroy_service_spec.rb
@@ -0,0 +1,43 @@
+require 'spec_helper'
+
+describe Members::DestroyService, services: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+  let!(:member) { create(:project_member, source: project) }
+
+  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
+    end
+  end
+
+  context 'when current user can destroy the given member' do
+    before do
+      project.team << [user, :master]
+    end
+
+    it 'destroys the member' do
+      expect(destroy_member(member, user)).to be_destroyed
+    end
+
+    context 'when the given member is a requester' do
+      before do
+        member.update_column(:requested_at, Time.now)
+      end
+
+      it 'calls Member#after_decline_request' do
+        expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member)
+
+        destroy_member(member, user)
+      end
+    end
+  end
+
+  def destroy_member(member, user)
+    Members::DestroyService.new(member, user).execute
+  end
+end
-- 
cgit v1.2.1


From 6c5b2377f769185f562578791139a13939867b73 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 17 Jun 2016 14:08:02 +0200
Subject: Use the new Members::DestroyService in group/project member
 controllers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/controllers/groups/group_members_controller.rb     | 2 +-
 app/controllers/projects/project_members_controller.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index d0f2e2949f0..c3929ded6dd 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -38,7 +38,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
     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.' }
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 35d067cd029..ff0ac115f55 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -52,7 +52,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
 
     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
-- 
cgit v1.2.1


From 724f986fb2cf8fee3606bffd01da9842634400ef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 17 Jun 2016 16:33:10 +0200
Subject: Redirect to the member's source on request withdrawal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/controllers/concerns/membership_actions.rb               | 3 ++-
 spec/controllers/groups/group_members_controller_spec.rb     | 2 +-
 spec/controllers/projects/project_members_controller_spec.rb | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index b1343576b3c..7e6b83bcc37 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -34,8 +34,9 @@ module MembershipActions
         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 [:dashboard, @member.real_source_type.tableize], notice: notice
+      redirect_to redirect_path, notice: notice
     else
       if cannot_leave?
         alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}."
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb
index 89c2c26a367..8798d709f30 100644
--- a/spec/controllers/groups/group_members_controller_spec.rb
+++ b/spec/controllers/groups/group_members_controller_spec.rb
@@ -134,7 +134,7 @@ describe Groups::GroupMembersController do
           delete :leave, group_id: group
 
           expect(response).to set_flash.to 'Your access request to the group has been withdrawn.'
-          expect(response).to redirect_to(dashboard_groups_path)
+          expect(response).to redirect_to(group_path(group))
           expect(group.members.request).to be_empty
           expect(group.users).not_to include user
         end
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index fc5f458e795..0a8fe5e04c3 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -190,7 +190,7 @@ describe Projects::ProjectMembersController do
                          project_id: project
 
           expect(response).to set_flash.to 'Your access request to the project has been withdrawn.'
-          expect(response).to redirect_to(dashboard_projects_path)
+          expect(response).to redirect_to(namespace_project_path(project.namespace, project))
           expect(project.members.request).to be_empty
           expect(project.users).not_to include user
         end
-- 
cgit v1.2.1


From a08a26ac814d7fd9f7523e22847fab0cc25ceb78 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 17 Jun 2016 16:33:37 +0200
Subject: Don't send the "access declined" email on access request withdrawal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/services/members/destroy_service.rb       | 4 +++-
 spec/services/members/destroy_service_spec.rb | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index e32eb47b846..59a55e42e38 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -10,7 +10,9 @@ module Members
       if can?(current_user, "destroy_#{member.type.underscore}".to_sym, member)
         member.destroy
 
-        notification_service.decline_access_request(member) if member.request?
+        if member.request? && member.user != current_user
+          notification_service.decline_access_request(member)
+        end
       end
 
       member
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index aa002b4bd22..04c2782c125 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/spec/services/members/destroy_service_spec.rb
@@ -34,6 +34,14 @@ describe Members::DestroyService, services: true do
 
         destroy_member(member, user)
       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
+      end
     end
   end
 
-- 
cgit v1.2.1


From 654565c9dc734a597c525a75c8f72dd63235604b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 17 Jun 2016 18:59:33 +0200
Subject: Raise a new Gitlab::Access::AccessDeniedError when permission is not
 enough to destroy a member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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          |  4 +++
 app/controllers/concerns/membership_actions.rb     | 34 +++++-----------------
 app/controllers/groups/group_members_controller.rb |  6 ----
 .../projects/project_members_controller.rb         |  6 ----
 app/services/members/destroy_service.rb            | 26 ++++-------------
 lib/gitlab/access.rb                               |  2 ++
 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
 
-- 
cgit v1.2.1


From bceee987134ad578b3d6f89c9bc61bef8e0c6066 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Sat, 18 Jun 2016 05:44:15 +0200
Subject: Show 'Leave project' only if member can actually leave the project
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/views/layouts/nav/_project.html.haml              | 11 ++++++-----
 app/views/layouts/nav/_project_settings.html.haml     |  2 +-
 .../projects/members/member_leaves_project_spec.rb    | 19 +++++++++++++++++++
 .../members/owner_cannot_leave_project_spec.rb        | 16 ++++++++++++++++
 .../projects/members/user_requests_access_spec.rb     |  1 +
 5 files changed, 43 insertions(+), 6 deletions(-)
 create mode 100644 spec/features/projects/members/member_leaves_project_spec.rb
 create mode 100644 spec/features/projects/members/owner_cannot_leave_project_spec.rb

diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 39ea4920ccc..a9289a8552a 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
diff --git a/spec/features/projects/members/member_leaves_project_spec.rb b/spec/features/projects/members/member_leaves_project_spec.rb
new file mode 100644
index 00000000000..79dec442818
--- /dev/null
+++ b/spec/features/projects/members/member_leaves_project_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+feature 'Projects > Members > Member leaves project', feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  background do
+    project.team << [user, :developer]
+    login_as(user)
+    visit namespace_project_path(project.namespace, project)
+  end
+
+  scenario 'user leaves project' do
+    click_link 'Leave Project'
+
+    expect(current_path).to eq(dashboard_projects_path)
+    expect(project.users.exists?(user.id)).to be_falsey
+  end
+end
diff --git a/spec/features/projects/members/owner_cannot_leave_project_spec.rb b/spec/features/projects/members/owner_cannot_leave_project_spec.rb
new file mode 100644
index 00000000000..67811b1048e
--- /dev/null
+++ b/spec/features/projects/members/owner_cannot_leave_project_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+feature 'Projects > Members > Owner cannot leave project', feature: true do
+  let(:owner) { create(:user) }
+  let(:project) { create(:project) }
+
+  background do
+    project.team << [owner, :owner]
+    login_as(owner)
+    visit namespace_project_path(project.namespace, project)
+  end
+
+  scenario 'user does not see a "Leave Project" link' do
+    expect(page).not_to have_content 'Leave Project'
+  end
+end
diff --git a/spec/features/projects/members/user_requests_access_spec.rb b/spec/features/projects/members/user_requests_access_spec.rb
index fd92a3a2f0c..af420c170ef 100644
--- a/spec/features/projects/members/user_requests_access_spec.rb
+++ b/spec/features/projects/members/user_requests_access_spec.rb
@@ -21,6 +21,7 @@ feature 'Projects > Members > User requests access', feature: true do
     expect(page).to have_content 'Your request for access has been queued for review.'
 
     expect(page).to have_content 'Withdraw Access Request'
+    expect(page).not_to have_content 'Leave Project'
   end
 
   scenario 'user is not listed in the project members page' do
-- 
cgit v1.2.1


From bf05ca88eefb38bb52fb6e31622f9c1a795965e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Sat, 18 Jun 2016 05:45:52 +0200
Subject: Add 'Leave Group' link
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The link was removed in !3798, probably by mistake.

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/views/layouts/nav/_group_settings.html.haml    | 36 +++++++++++++---------
 .../members/last_owner_cannot_leave_group_spec.rb  | 16 ++++++++++
 .../groups/members/member_leaves_group_spec.rb     | 22 +++++++++++++
 .../groups/members/user_requests_access_spec.rb    |  1 +
 4 files changed, 60 insertions(+), 15 deletions(-)
 create mode 100644 spec/features/groups/members/last_owner_cannot_leave_group_spec.rb
 create mode 100644 spec/features/groups/members/member_leaves_group_spec.rb

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/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb b/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb
new file mode 100644
index 00000000000..33bf6d3752f
--- /dev/null
+++ b/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+feature 'Groups > Members > Last owner cannot leave group', feature: true do
+  let(:owner) { create(:user) }
+  let(:group) { create(:group) }
+
+  background do
+    group.add_owner(owner)
+    login_as(owner)
+    visit group_path(group)
+  end
+
+  scenario 'user does not see a "Leave Group" link' do
+    expect(page).not_to have_content 'Leave Group'
+  end
+end
diff --git a/spec/features/groups/members/member_leaves_group_spec.rb b/spec/features/groups/members/member_leaves_group_spec.rb
new file mode 100644
index 00000000000..04dfb22db49
--- /dev/null
+++ b/spec/features/groups/members/member_leaves_group_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+feature 'Groups > Members > Member leaves group', feature: true do
+  let(:user) { create(:user) }
+  let(:owner) { create(:user) }
+  let(:group) { create(:group, :public) }
+
+  background do
+    group.add_owner(owner)
+    group.add_developer(user)
+    login_as(user)
+    visit group_path(group)
+  end
+
+  scenario 'user leaves group' do
+    # find('#group-settings-button').click
+    click_link 'Leave Group'
+
+    expect(current_path).to eq(dashboard_groups_path)
+    expect(group.users.exists?(user.id)).to be_falsey
+  end
+end
diff --git a/spec/features/groups/members/user_requests_access_spec.rb b/spec/features/groups/members/user_requests_access_spec.rb
index a878a96b6ee..1ea607cbca0 100644
--- a/spec/features/groups/members/user_requests_access_spec.rb
+++ b/spec/features/groups/members/user_requests_access_spec.rb
@@ -21,6 +21,7 @@ feature 'Groups > Members > User requests access', feature: true do
     expect(page).to have_content 'Your request for access has been queued for review.'
 
     expect(page).to have_content 'Withdraw Access Request'
+    expect(page).not_to have_content 'Leave Group'
   end
 
   scenario 'user is not listed in the group members page' do
-- 
cgit v1.2.1


From 909a0ff3ace1eb82a4296764777a552779c39839 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Mon, 20 Jun 2016 12:36:59 +0200
Subject: Fix and remove duplicate specs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 features/dashboard/group.feature                   | 44 ----------------------
 features/steps/dashboard/group.rb                  | 42 ---------------------
 .../groups/group_members_controller_spec.rb        |  4 +-
 .../projects/project_members_controller_spec.rb    |  6 +--
 .../groups/members/member_leaves_group_spec.rb     |  1 -
 spec/features/projects_spec.rb                     | 16 --------
 6 files changed, 2 insertions(+), 111 deletions(-)

diff --git a/features/dashboard/group.feature b/features/dashboard/group.feature
index e3c01db2ebb..3ae2c679dc1 100644
--- a/features/dashboard/group.feature
+++ b/features/dashboard/group.feature
@@ -5,53 +5,9 @@ Feature: Dashboard Group
     And "John Doe" is owner of group "Owned"
     And "John Doe" is guest of group "Guest"
 
-  # Leave groups
-
-  @javascript
-  Scenario: Owner should be able to leave from group if he is not the last owner
-    Given "Mary Jane" is owner of group "Owned"
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should see group "Guest" in group list
-    When I click on the "Leave" button for group "Owned"
-    And I visit dashboard groups page
-    Then I should not see group "Owned" in group list
-    Then I should see group "Guest" in group list
-
-  @javascript
-  Scenario: Owner should not be able to leave from group if he is the last owner
-    Given "Mary Jane" is guest of group "Owned"
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should see group "Guest" in group list
-    When I click on the "Leave" button for group "Owned"
-    Then I should see the "Can not leave message"
-
-  @javascript
-  Scenario: Guest should be able to leave from group
-    Given "Mary Jane" is guest of group "Guest"
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should see group "Guest" in group list
-    When I click on the "Leave" button for group "Guest"
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should not see group "Guest" in group list
-
-  @javascript
-  Scenario: Guest should be able to leave from group even if he is the only user in the group
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should see group "Guest" in group list
-    When I click on the "Leave" button for group "Guest"
-    When I visit dashboard groups page
-    Then I should see group "Owned" in group list
-    Then I should not see group "Guest" in group list
-
   Scenario: Create a group from dasboard
     And I visit dashboard groups page
     And I click new group link
     And submit form with new group "Samurai" info
     Then I should be redirected to group "Samurai" page
     And I should see newly created group "Samurai"
-
diff --git a/features/steps/dashboard/group.rb b/features/steps/dashboard/group.rb
index 9b79a3be49b..cf679fea530 100644
--- a/features/steps/dashboard/group.rb
+++ b/features/steps/dashboard/group.rb
@@ -4,44 +4,6 @@ class Spinach::Features::DashboardGroup < Spinach::FeatureSteps
   include SharedPaths
   include SharedUser
 
-  # Leave
-
-  step 'I click on the "Leave" button for group "Owned"' do
-    find(:css, 'li', text: "Owner").find(:css, 'i.fa.fa-sign-out').click
-    # poltergeist always confirms popups.
-  end
-
-  step 'I click on the "Leave" button for group "Guest"' do
-    find(:css, 'li', text: "Guest").find(:css, 'i.fa.fa-sign-out').click
-    # poltergeist always confirms popups.
-  end
-
-  step 'I should not see the "Leave" button for group "Owned"' do
-    expect(find(:css, 'li', text: "Owner")).not_to have_selector(:css, 'i.fa.fa-sign-out')
-    # poltergeist always confirms popups.
-  end
-
-  step 'I should not see the "Leave" button for groupr "Guest"' do
-    expect(find(:css, 'li', text: "Guest")).not_to have_selector(:css,  'i.fa.fa-sign-out')
-    # poltergeist always confirms popups.
-  end
-
-  step 'I should see group "Owned" in group list' do
-    expect(page).to have_content("Owned")
-  end
-
-  step 'I should not see group "Owned" in group list' do
-    expect(page).not_to have_content("Owned")
-  end
-
-  step 'I should see group "Guest" in group list' do
-    expect(page).to have_content("Guest")
-  end
-
-  step 'I should not see group "Guest" in group list' do
-    expect(page).not_to have_content("Guest")
-  end
-
   step 'I click new group link' do
     click_link "New Group"
   end
@@ -60,8 +22,4 @@ class Spinach::Features::DashboardGroup < Spinach::FeatureSteps
     expect(page).to have_content "Samurai"
     expect(page).to have_content "Tokugawa Shogunate"
   end
-
-  step 'I should see the "Can not leave message"' do
-    expect(page).to have_content "You can not leave the \"Owned\" group."
-  end
 end
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb
index 8798d709f30..c8601341d54 100644
--- a/spec/controllers/groups/group_members_controller_spec.rb
+++ b/spec/controllers/groups/group_members_controller_spec.rb
@@ -118,9 +118,7 @@ describe Groups::GroupMembersController do
         it 'cannot removes himself from the group' do
           delete :leave, group_id: group
 
-          expect(response).to redirect_to(group_path(group))
-          expect(response).to set_flash[:alert].to "You can not leave the \"#{group.name}\" group. Transfer or delete the group."
-          expect(group.users).to include user
+          expect(response.status).to eq(403)
         end
       end
 
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index 0a8fe5e04c3..e5e750c855f 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -171,11 +171,7 @@ describe Projects::ProjectMembersController do
           delete :leave, namespace_id: project.namespace,
                          project_id: project
 
-          expect(response).to redirect_to(
-            namespace_project_path(project.namespace, project)
-          )
-          expect(response).to set_flash[:alert].to "You can not leave the \"#{project.human_name}\" project. Transfer or delete the project."
-          expect(project.users).to include user
+          expect(response.status).to eq(403)
         end
       end
 
diff --git a/spec/features/groups/members/member_leaves_group_spec.rb b/spec/features/groups/members/member_leaves_group_spec.rb
index 04dfb22db49..3185ff924b9 100644
--- a/spec/features/groups/members/member_leaves_group_spec.rb
+++ b/spec/features/groups/members/member_leaves_group_spec.rb
@@ -13,7 +13,6 @@ feature 'Groups > Members > Member leaves group', feature: true do
   end
 
   scenario 'user leaves group' do
-    # find('#group-settings-button').click
     click_link 'Leave Group'
 
     expect(current_path).to eq(dashboard_groups_path)
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index 9dd0378d165..6fa8298d489 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -70,22 +70,6 @@ feature 'Project', feature: true do
     end
   end
 
-  describe 'leave project link' do
-    let(:user)    { create(:user) }
-    let(:project) { create(:project, namespace: user.namespace) }
-
-    before do
-      login_with(user)
-      project.team.add_user(user, Gitlab::Access::MASTER)
-      visit namespace_project_path(project.namespace, project)
-    end
-
-    it 'click project-settings and find leave project' do
-      find('#project-settings-button').click
-      expect(page).to have_link('Leave Project')
-    end
-  end
-
   describe 'project title' do
     include WaitForAjax
 
-- 
cgit v1.2.1