Fix “Withdraw access request” for projects

15 files changed, 290 insertions, 30 deletions

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index c6b1e443de6..8463496114b 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -31,10 +31,17 @@ module MembershipActions
   def request_access
     membershipable.request_access(current_user)
 
-    redirect_to polymorphic_path(membershipable),
+    redirect_to membershipable,
                 notice: 'Your request for access has been queued for review.'
   end
 
+  def withdraw_access_request
+    membershipable.withdraw_access_request(current_user)
+
+    redirect_to membershipable,
+                notice: "Your access request to the #{source_type} has been withdrawn."
+  end
+
   def approve_access_request
     Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute
 
@@ -45,16 +52,11 @@ module MembershipActions
     member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id)
       .execute(:all)
 
-    notice =
-      if member.request?
-        "Your access request to the #{source_type} has been withdrawn."
-      else
-        "You left the \"#{membershipable.human_name}\" #{source_type}."
-      end
+    notice = "You left the \"#{membershipable.human_name}\" #{source_type}."
 
     respond_to do |format|
       format.html do
-        redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize]
+        redirect_path = [:dashboard, membershipable.class.to_s.tableize]
         redirect_to redirect_path, notice: notice
       end
 
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index ab0a8c33efe..da7cfdc0a6e 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -3,7 +3,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   include SortingHelper
 
   # Authorize
-  before_action :authorize_admin_project_member!, except: [:index, :leave, :request_access]
+  before_action :authorize_admin_project_member!, except: [:index, :leave, :request_access, :withdraw_access_request]
 
   def index
     @sort = params[:sort].presence || sort_value_name
diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb
index d4a91e533c1..973747338d0 100644
--- a/app/helpers/gitlab_routing_helper.rb
+++ b/app/helpers/gitlab_routing_helper.rb
@@ -108,6 +108,10 @@ module GitlabRoutingHelper
     request_access_project_project_members_path(project)
   end
 
+  def withdraw_access_request_project_members_path(project, *args)
+    withdraw_access_request_project_project_members_path(project)
+  end
+
   def leave_project_members_path(project, *args)
     leave_project_project_members_path(project)
   end
@@ -135,6 +139,10 @@ module GitlabRoutingHelper
     request_access_group_group_members_path(group)
   end
 
+  def withdraw_access_request_group_members_path(group, *args)
+    withdraw_access_request_group_group_members_path(group)
+  end
+
   def leave_group_members_path(group, *args)
     leave_group_group_members_path(group)
   end
diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb
index 41d471cc92f..0ca29b97949 100644
--- a/app/helpers/members_helper.rb
+++ b/app/helpers/members_helper.rb
@@ -44,4 +44,22 @@ module MembersHelper
     path << "?#{options.to_param}"
     path
   end
+
+  # Returns a `<action>_<source>_access_request` association, e.g.:
+  # - destroy_project_access_request
+  # - destroy_group_access_request
+  def action_access_request_permission(action, access_request)
+    "#{action}_#{access_request.class.name.underscore}".to_sym
+  end
+
+  def withdraw_access_request_message(access_request)
+    source =
+      if access_request.is_a?(ProjectAccessRequest)
+        "the #{access_request.project.human_name} project"
+      elsif access_request.is_a?(GroupAccessRequest)
+        "the #{access_request.group.human_name} group"
+      end
+
+    "Are you sure you want to withdraw your access request for the #{source}?"
+  end
 end
diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb
index 62bc6b809f4..97c5d07ac06 100644
--- a/app/models/concerns/access_requestable.rb
+++ b/app/models/concerns/access_requestable.rb
@@ -10,4 +10,8 @@ module AccessRequestable
   def request_access(user)
     Members::RequestAccessService.new(self, user).execute
   end
+
+  def withdraw_access_request(user)
+    Members::DestroyAccessRequestService.new(self, user, user).execute
+  end
 end
diff --git a/app/models/group_access_request.rb b/app/models/group_access_request.rb
index f750b01982a..d5ddd33b724 100644
--- a/app/models/group_access_request.rb
+++ b/app/models/group_access_request.rb
@@ -1,4 +1,23 @@
 class GroupAccessRequest < ActiveRecord::Base
   belongs_to :group
   belongs_to :user
+  
+  def notifiable?(type, opts = {})
+    NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts))
+  end
+
+  def notifiable_options
+    {}
+  end
+
+  # Make it look like polymorphic Member during refactor
+  alias_attribute :source, :group
+
+  def source_id
+    group_id
+  end
+
+  def real_source_type
+    'Group'
+  end
 end
diff --git a/app/models/project_access_request.rb b/app/models/project_access_request.rb
index f7fd5415a4b..3d69a061d4e 100644
--- a/app/models/project_access_request.rb
+++ b/app/models/project_access_request.rb
@@ -1,4 +1,23 @@
 class ProjectAccessRequest < ActiveRecord::Base
   belongs_to :project
   belongs_to :user
+
+  def notifiable?(type, opts = {})
+    NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts))
+  end
+
+  def notifiable_options
+    {}
+  end
+
+  # Make it look like polymorphic Member during refactor
+  alias_attribute :source, :project
+
+  def source_id
+    project_id
+  end
+
+  def real_source_type
+    'Project'
+  end
 end
diff --git a/app/policies/group_access_request_policy.rb b/app/policies/group_access_request_policy.rb
new file mode 100644
index 00000000000..4f043c4304d
--- /dev/null
+++ b/app/policies/group_access_request_policy.rb
@@ -0,0 +1,19 @@
+class GroupAccessRequestPolicy < BasePolicy
+  delegate :group
+
+  with_scope :subject
+
+  desc "GroupAccessRequest is users' own"
+  with_score 0
+  condition(:is_target_user) { @user && @subject.user == @user }
+
+  rule { anonymous }.prevent_all
+
+  rule { can?(:admin_group_member) }.policy do
+    enable :destroy_group_access_request
+  end
+
+  rule { is_target_user }.policy do
+    enable :destroy_group_access_request
+  end
+end
diff --git a/app/policies/project_access_request_policy.rb b/app/policies/project_access_request_policy.rb
new file mode 100644
index 00000000000..5d888017bf4
--- /dev/null
+++ b/app/policies/project_access_request_policy.rb
@@ -0,0 +1,19 @@
+class ProjectAccessRequestPolicy < BasePolicy
+  delegate :project
+
+  with_scope :subject
+
+  desc "ProjectAccessRequest is users' own"
+  with_score 0
+  condition(:is_target_user) { @user && @subject.user == @user }
+
+  rule { anonymous }.prevent_all
+
+  rule { can?(:admin_project_member) }.policy do
+    enable :destroy_project_access_request
+  end
+
+  rule { is_target_user }.policy do
+    enable :destroy_project_access_request
+  end
+end
diff --git a/app/services/members/destroy_access_request_service.rb b/app/services/members/destroy_access_request_service.rb
new file mode 100644
index 00000000000..3fe5d2afd8b
--- /dev/null
+++ b/app/services/members/destroy_access_request_service.rb
@@ -0,0 +1,38 @@
+module Members
+  class DestroyAccessRequestService < BaseService
+    include MembersHelper
+
+    attr_accessor :source, :access_requester, :current_user
+
+    def initialize(source, access_requester, current_user)
+      @source = source
+      @access_requester = access_requester
+      @current_user = current_user
+    end
+
+    def execute
+      access_request_scope = source.access_requests.where(user: access_requester)
+
+      access_request = access_request_scope.take!
+
+      raise Gitlab::Access::AccessDeniedError unless can_destroy_access_request?(access_request)
+
+      # Why not a simple find then destroy?
+      #
+      #   ActiveRecord does not like not having a primary key, and I don't have
+      #   enough reason to add a composite keys gem yet (which apparently is
+      #   rewritten for every major Rails version).
+      access_request_scope.delete_all
+
+      if current_user != access_requester
+        notification_service.decline_access_request(access_request)
+      end
+    end
+
+    private
+
+    def can_destroy_access_request?(access_request)
+      access_request && can?(current_user, action_access_request_permission(:destroy, access_request), access_request)
+    end
+  end
+end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 4267879b03d..e8fa1943b18 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -215,10 +215,10 @@ class NotificationService
     mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
   end
 
-  def decline_access_request(member)
-    return true unless member.notifiable?(:subscription)
+  def decline_access_request(access_request)
+    return true unless access_request.notifiable?(:subscription)
 
-    mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later
+    mailer.member_access_denied_email(access_request.real_source_type, access_request.source_id, access_request.user_id).deliver_later
   end
 
   # Project invite
diff --git a/app/views/shared/members/_access_request_buttons.html.haml b/app/views/shared/members/_access_request_buttons.html.haml
index ca7778e648f..df0c6ef1e74 100644
--- a/app/views/shared/members/_access_request_buttons.html.haml
+++ b/app/views/shared/members/_access_request_buttons.html.haml
@@ -9,9 +9,9 @@
               class: 'btn'
 - elsif access_request = source.access_requests.find_by(user_id: current_user.id)
   .project-action-button.inline
-    = link_to _('Withdraw Access Request'), polymorphic_path([:leave, source, :members]),
+    = link_to _('Withdraw Access Request'), polymorphic_path([:withdraw_access_request, source, :members]),
               method: :delete,
-              data: { confirm: remove_member_message(access_request) },
+              data: { confirm: withdraw_access_request_message(access_request) },
               class: 'btn'
 - elsif source.request_access_enabled && can?(current_user, :request_access, source)
   .project-action-button.inline
diff --git a/config/routes.rb b/config/routes.rb
index 4fd6cb5d439..5e5e776b3b0 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -4,6 +4,7 @@ require 'sidekiq/cron/web'
 Rails.application.routes.draw do
   concern :access_requestable do
     post :request_access, on: :collection
+    delete :withdraw_access_request, on: :collection
     post :approve_access_request, on: :member
   end
 
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index e83f25e87e1..dcd90441b0d 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -171,22 +171,6 @@ describe Projects::ProjectMembersController do
           expect(response).to have_http_status(403)
         end
       end
-
-      context 'and has requested access' do
-        before do
-          project.request_access(user)
-        end
-
-        it 'removes user from members' do
-          delete :leave, namespace_id: project.namespace,
-                         project_id: project
-
-          expect(response).to set_flash.to 'Your access request to the project has been withdrawn.'
-          expect(response).to redirect_to(project_path(project))
-          expect(project.access_requests).to be_empty
-          expect(project.users).not_to include user
-        end
-      end
     end
   end
 
@@ -206,6 +190,54 @@ describe Projects::ProjectMembersController do
     end
   end
 
+  describe 'DELETE withdraw_access_request' do
+    context 'when the current_user has requested access to the project' do
+      let!(:access_request) { project.request_access(user) }
+
+      before do
+        sign_in(user)
+      end
+
+      it 'redirects with success message' do
+        delete :withdraw_access_request, namespace_id: project.namespace,
+                                         project_id: project
+
+        expect(response).to set_flash.to /Your access request .* has been withdrawn/
+        expect(response).to redirect_to(project)
+      end
+
+      it 'destroys the access request' do
+        delete :withdraw_access_request, namespace_id: project.namespace,
+                                         project_id: project
+
+        expect(project.access_requests.where(user: user)).not_to exist
+      end
+    end
+
+    context 'when the current_user has not requested access to the project' do
+      let(:other_user) { create(:user) }
+      let!(:other_access_request) { project.request_access(other_user) }
+
+      before do
+        sign_in(user)
+      end
+
+      it 'responds 404 Not Found' do
+        delete :withdraw_access_request, namespace_id: project.namespace,
+                                         project_id: project
+
+        expect(response).to have_http_status(404)
+      end
+
+      it "does not destroy another user's access request" do
+        delete :withdraw_access_request, namespace_id: project.namespace,
+                                         project_id: project
+
+        expect(project.access_requests.where(user: other_user)).to exist
+      end
+    end
+  end
+
   describe 'POST approve' do
     let(:member) { create(:project_member, :access_request, project: project) }
 
diff --git a/spec/services/members/destroy_access_request_service_spec.rb b/spec/services/members/destroy_access_request_service_spec.rb
new file mode 100644
index 00000000000..1f564c0d7ee
--- /dev/null
+++ b/spec/services/members/destroy_access_request_service_spec.rb
@@ -0,0 +1,81 @@
+require 'spec_helper'
+
+describe Members::DestroyAccessRequestService do
+  subject { described_class.new(source, access_requester, current_user).execute }
+
+  let(:access_requester) { create(:user) }
+  let(:other_user) { create(:user) }
+  let(:project) { create(:project, :public, :access_requestable) }
+  let(:group) { create(:group, :public, :access_requestable) }
+
+  shared_examples 'a service raising ActiveRecord::RecordNotFound' do
+    it 'raises ActiveRecord::RecordNotFound' do
+      expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
+    it 'raises Gitlab::Access::AccessDeniedError' do
+      expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+
+  shared_examples 'a service destroying an access request' do
+    context 'when the given user did not request access' do
+      let(:current_user) { access_requester }
+
+      it_behaves_like 'a service raising ActiveRecord::RecordNotFound'
+    end
+
+    context 'when the given user requested access' do
+      let!(:access_request) { source.request_access(access_requester) }
+
+      context 'when current_user is the user requesting access' do
+        let(:current_user) { access_requester }
+
+        it 'destroys the access request' do
+          expect { subject }.to change { source.access_requests.count }.by(-1)
+        end
+
+        it 'does not send a decline_access_request notification' do
+          expect_any_instance_of(NotificationService).not_to receive(:decline_access_request)
+
+          subject
+        end
+      end
+
+      context 'when current_user is not the user requesting access' do
+        let(:current_user) { other_user }
+
+        context 'when current_user can decline the access request' do
+          before do
+            project.team << [current_user, :master]
+            group.add_owner(current_user)
+          end
+
+          it 'destroys the access request' do
+            expect { subject }.to change { source.access_requests.count }.by(-1)
+          end
+
+          it 'sends a decline_access_request notification' do
+            expect_any_instance_of(NotificationService).to receive(:decline_access_request)
+
+            subject
+          end
+        end
+
+        context 'when current_user cannot decline the access request' do
+          it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
+        end
+      end
+    end
+  end
+
+  it_behaves_like 'a service destroying an access request' do
+    let(:source) { project }
+  end
+
+  it_behaves_like 'a service destroying an access request' do
+    let(:source) { group }
+  end
+end