From 01722975771ce310f92e5b367a36ed7cc1b56e37 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Fri, 3 Mar 2023 20:12:08 +0000
Subject: [PATCH] Use GroupHook policies in controller

The policies got introduced in:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/88245
but they were not used in controllers to check access.

To avoid confusion, this makes use of the policies.
---
 ee/app/controllers/groups/hooks_controller.rb |  7 ++++++-
 ee/app/policies/group_hook_policy.rb          |  3 +--
 .../groups/hooks_controller_spec.rb           | 21 +++++++++++++++++--
 ee/spec/policies/group_hook_policy_spec.rb    |  8 +++----
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/ee/app/controllers/groups/hooks_controller.rb b/ee/app/controllers/groups/hooks_controller.rb
index 0f00e651c186e..12b4b1955c7ef 100644
--- a/ee/app/controllers/groups/hooks_controller.rb
+++ b/ee/app/controllers/groups/hooks_controller.rb
@@ -5,9 +5,10 @@ class Groups::HooksController < Groups::ApplicationController
 
   # Authorize
   before_action :group
-  before_action :authorize_admin_group!
+  before_action :authorize_admin_group!, except: :destroy
   before_action :check_group_webhooks_available!
   before_action :hook, only: [:edit, :update, :test, :destroy]
+  before_action :authorize_destroy_group_hook!, only: :destroy
   before_action :hook_logs, only: :edit
   before_action -> { check_rate_limit!(:group_testing_hook, scope: [@group, current_user]) }, only: :test
 
@@ -53,4 +54,8 @@ def trigger_values
   def check_group_webhooks_available!
     render_404 unless @group.licensed_feature_available?(:group_webhooks) || LicenseHelper.show_promotions?(current_user)
   end
+
+  def authorize_destroy_group_hook!
+    render_404 unless can?(current_user, :destroy_web_hook, @hook)
+  end
 end
diff --git a/ee/app/policies/group_hook_policy.rb b/ee/app/policies/group_hook_policy.rb
index 3e96e3ab65e98..15b51a1790f8a 100644
--- a/ee/app/policies/group_hook_policy.rb
+++ b/ee/app/policies/group_hook_policy.rb
@@ -1,10 +1,9 @@
 # frozen_string_literal: true
 
 class GroupHookPolicy < ::BasePolicy
-  delegate(:group)
+  delegate { @subject.group }
 
   rule { can?(:admin_group) }.policy do
-    enable :read_web_hook
     enable :destroy_web_hook
   end
 end
diff --git a/ee/spec/controllers/groups/hooks_controller_spec.rb b/ee/spec/controllers/groups/hooks_controller_spec.rb
index 73e5a59a99ced..fb5a9a1c32fd3 100644
--- a/ee/spec/controllers/groups/hooks_controller_spec.rb
+++ b/ee/spec/controllers/groups/hooks_controller_spec.rb
@@ -3,11 +3,18 @@
 require 'spec_helper'
 
 RSpec.describe Groups::HooksController, feature_category: :integrations do
-  let_it_be(:user)  { create(:user) }
+  let_it_be(:group_owner) { create(:user) }
+  let_it_be(:group_maintainer) { create(:user) }
   let_it_be(:group) { create(:group) }
 
+  let(:user) { group_owner }
+
+  before_all do
+    group.add_owner(group_owner)
+    group.add_maintainer(group_maintainer)
+  end
+
   before do
-    group.add_owner(user)
     sign_in(user)
   end
 
@@ -274,6 +281,16 @@ def it_renders_correctly
     let(:params) { { group_id: group.to_param, id: hook } }
 
     it_behaves_like 'Web hook destroyer'
+
+    context 'When user is not logged in' do
+      let(:user) { group_maintainer }
+
+      it 'renders a 404' do
+        delete :destroy, params: params
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   context 'with group_webhooks disabled' do
diff --git a/ee/spec/policies/group_hook_policy_spec.rb b/ee/spec/policies/group_hook_policy_spec.rb
index c6912e8b855ab..ffa9d193f673b 100644
--- a/ee/spec/policies/group_hook_policy_spec.rb
+++ b/ee/spec/policies/group_hook_policy_spec.rb
@@ -14,8 +14,8 @@
       hook.group.add_maintainer(user)
     end
 
-    it "cannot read and destroy web-hooks" do
-      expect(policy).to be_disallowed(:read_web_hook, :destroy_web_hook)
+    it "cannot destroy web-hooks" do
+      expect(policy).to be_disallowed(:destroy_web_hook)
     end
   end
 
@@ -24,8 +24,8 @@
       hook.group.add_owner(user)
     end
 
-    it "can read and destroy web-hooks" do
-      expect(policy).to be_allowed(:read_web_hook, :destroy_web_hook)
+    it "can destroy web-hooks" do
+      expect(policy).to be_allowed(:destroy_web_hook)
     end
   end
 end
-- 
GitLab