Merge branch '260419-add-group-webhooks-for-subgroups' into 'master'

Resolve "Add support for subgroup events in Group webhooks"

See merge request gitlab-org/gitlab!52658

app/models/concerns/triggerable_hooks.rb

@@ -16,7 +16,8 @@ module TriggerableHooks
     deployment_hooks:         :deployment_events,
     feature_flag_hooks:       :feature_flag_events,
     release_hooks:            :releases_events,
-    member_hooks:             :member_events
+    member_hooks:             :member_events,
+    subgroup_hooks:           :subgroup_events
   }.freeze
 
   extend ActiveSupport::Concern

app/views/shared/web_hooks/_form.html.haml

@@ -50,6 +50,7 @@
         = s_('Webhooks|This URL will be triggered when a confidential issue is created/updated/merged')
     - if @group
       = render_if_exists 'groups/hooks/member_events', form: form
+      = render_if_exists 'groups/hooks/subgroup_events', form: form
     %li
       = form.check_box :merge_requests_events, class: 'form-check-input'
       = form.label :merge_requests_events, class: 'list-label form-check-label gl-ml-1' do

doc/api/groups.md

@@ -1010,7 +1010,7 @@ GET /groups?search=foobar
 ]
 ```
 
-## Hooks
+## Hooks **(PREMIUM)**
 
 Also called Group Hooks and Webhooks.
 These are different from [System Hooks](system_hooks.md) that are system wide and [Project Hooks](projects.md#hooks) that are limited to one project.
@@ -1057,6 +1057,7 @@ GET /groups/:id/hooks/:hook_id
   "wiki_page_events": true,
   "deployment_events": true,
   "releases_events": true,
+  "subgroup_events": true,
   "enable_ssl_verification": true,
   "created_at": "2012-10-12T17:04:47Z"
 }
@@ -1086,6 +1087,7 @@ POST /groups/:id/hooks
 | `wiki_page_events`           | boolean        | no       | Trigger hook on wiki events |
 | `deployment_events`          | boolean        | no       | Trigger hook on deployment events |
 | `releases_events`            | boolean        | no       | Trigger hook on release events |
+| `subgroup_events`            | boolean        | no       | Trigger hook on subgroup events |
 | `enable_ssl_verification`    | boolean        | no       | Do SSL verification when triggering the hook |
 | `token`                      | string         | no       | Secret token to validate received payloads; not returned in the response |
 
@@ -1114,6 +1116,7 @@ PUT /groups/:id/hooks/:hook_id
 | `wiki_events`                | boolean        | no       | Trigger hook on wiki events |
 | `deployment_events`          | boolean        | no       | Trigger hook on deployment events |
 | `releases_events`            | boolean        | no       | Trigger hook on release events |
+| `subgroup_events`            | boolean        | no       | Trigger hook on subgroup events |
 | `enable_ssl_verification`    | boolean        | no       | Do SSL verification when triggering the hook |
 | `token`                      | string         | no       | Secret token to validate received payloads; not returned in the response |
 

doc/user/project/integrations/webhooks.md

@@ -1485,6 +1485,74 @@ X-Gitlab-Event: Member Hook
 }
 ```
 
+### Subgroup events **(PREMIUM)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/260419) in GitLab 13.9.
+
+Subgroup events are triggered when:
+
+- A [subgroup is created in a group](#subgroup-created-in-a-group)
+- A [subgroup is removed from a group](#subgroup-removed-from-a-group)
+
+#### Subgroup created in a group
+
+**Request Header**:
+
+```plaintext
+X-Gitlab-Event: Subgroup Hook
+```
+
+**Request Body**:
+
+```json
+{
+
+  "created_at": "2021-01-20T09:40:12Z",
+  "updated_at": "2021-01-20T09:40:12Z",
+  "event_name": "subgroup_create",
+  "name": "subgroup1",
+  "path": "subgroup1",
+  "full_path": "group1/subgroup1",
+  "group_id": 10,
+  "parent_group_id": 7,
+  "parent_name": "group1",
+  "parent_path": "group1",
+  "parent_full_path": "group1"
+
+}
+```
+
+#### Subgroup removed from a group
+
+**Request Header**:
+
+```plaintext
+X-Gitlab-Event: Subgroup Hook
+```
+
+**Request Body**:
+
+```json
+{
+
+  "created_at": "2021-01-20T09:40:12Z",
+  "updated_at": "2021-01-20T09:40:12Z",
+  "event_name": "subgroup_destroy",
+  "name": "subgroup1",
+  "path": "subgroup1",
+  "full_path": "group1/subgroup1",
+  "group_id": 10,
+  "parent_group_id": 7,
+  "parent_name": "group1",
+  "parent_path": "group1",
+  "parent_full_path": "group1"
+
+}
+```
+
+NOTE:
+Webhooks for when a [subgroup is removed from a group](#subgroup-removed-from-a-group) are not triggered when a [subgroup is transferred to a new parent group](../../group/index.md#transferring-groups)
+
 ### Feature Flag events
 
 Triggered when a feature flag is turned on or off.

ee/app/models/ee/group.rb

@@ -468,6 +468,35 @@ module EE
 
     private
 
+    override :post_create_hook
+    def post_create_hook
+      super
+
+      execute_subgroup_hooks(:create)
+    end
+
+    override :post_destroy_hook
+    def post_destroy_hook
+      super
+
+      execute_subgroup_hooks(:destroy)
+    end
+
+    def execute_subgroup_hooks(event)
+      return unless subgroup?
+      return unless parent.group? # TODO: Remove this after fixing https://gitlab.com/gitlab-org/gitlab/-/issues/301013
+      return unless feature_available?(:group_webhooks)
+
+      run_after_commit do
+        data = ::Gitlab::HookData::SubgroupBuilder.new(self).build(event)
+        # Imagine a case where a subgroup has a webhook with `subgroup_events` enabled.
+        # When this subgroup is removed, there is no point in this subgroup's webhook itself being notified
+        # that `self` was removed. Rather, we should only care about notifying its ancestors
+        # and hence we need to trigger the hooks starting only from its `parent` group.
+        parent.execute_hooks(data, :subgroup_hooks)
+      end
+    end
+
     def custom_project_templates_group_allowed
       return if custom_project_templates_group_id.blank?
       return if children.exists?(id: custom_project_templates_group_id)

ee/app/models/hooks/group_hook.rb

@@ -21,7 +21,8 @@ class GroupHook < WebHook
     :wiki_page_hooks,
     :deployment_hooks,
     :release_hooks,
-    :member_hooks
+    :member_hooks,
+    :subgroup_hooks
   ]
 
   belongs_to :group

ee/app/views/groups/hooks/_subgroup_events.html.haml

+%li
+  = form.check_box :subgroup_events, class: 'form-check-input'
+  = form.label :subgroup_events, class: 'list-label form-check-label gl-ml-1' do
+    %strong= s_('Webhooks|Subgroup events')
+  %p.text-muted.gl-ml-1
+    = s_('Webhooks|This URL will be triggered when a subgroup is created/removed')

ee/changelogs/unreleased/260419-add-group-webhooks-for-subgroups.yml

+---
+title: Add support for subgroup events in Group webhooks
+merge_request: 52658
+author:
+type: added

ee/lib/api/group_hooks.rb

@@ -24,6 +24,7 @@ module API
         optional :wiki_page_events, type: Boolean, desc: "Trigger hook on wiki events"
         optional :deployment_events, type: Boolean, desc: "Trigger hook on deployment events"
         optional :releases_events, type: Boolean, desc: "Trigger hook on release events"
+        optional :subgroup_events, type: Boolean, desc: "Trigger hook on subgroup events"
         optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook"
         optional :token, type: String, desc: "Secret token to validate received payloads; this will not be returned in the response"
       end

ee/lib/ee/api/entities/group_hook.rb

@@ -6,7 +6,7 @@ module EE
       class GroupHook < ::API::Entities::Hook
         expose :group_id, :issues_events, :confidential_issues_events,
                :note_events, :confidential_note_events, :pipeline_events, :wiki_page_events,
-               :job_events, :deployment_events, :releases_events
+               :job_events, :deployment_events, :releases_events, :subgroup_events
       end
     end
   end

ee/spec/controllers/groups/hooks_controller_spec.rb

@@ -40,7 +40,8 @@ RSpec.describe Groups::HooksController do
           url: 'http://example.com',
           wiki_page_events: true,
           deployment_events: true,
-          member_events: true
+          member_events: true,
+          subgroup_events: true
         }
 
         post :create, params: { group_id: group.to_param, hook: hook_params }
@@ -83,7 +84,8 @@ RSpec.describe Groups::HooksController do
             wiki_page_events: true,
             deployment_events: true,
             releases_events: true,
-            member_events: true
+            member_events: true,
+            subgroup_events: true
           }
         end
 

ee/spec/factories/group_hooks.rb

@@ -17,6 +17,7 @@ FactoryBot.define do
       pipeline_events { true }
       wiki_page_events { true }
       releases_events { true }
+      subgroup_events { true }
     end
   end
 end

ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json

@@ -18,7 +18,8 @@
     "wiki_page_events",
     "job_events",
     "deployment_events",
-    "releases_events"
+    "releases_events",
+    "subgroup_events"
   ],
   "properties": {
     "id": { "type": "integer" },
@@ -38,7 +39,8 @@
     "wiki_page_events": { "type": "boolean" },
     "job_events": { "type": "boolean" },
     "deployment_events": { "type": "boolean" },
-    "releases_events": { "type": "boolean" }
+    "releases_events": { "type": "boolean" },
+    "subgroup_events": { "type": "boolean" }
   },
   "additionalProperties": false
 }

ee/spec/models/group_spec.rb

@@ -902,8 +902,10 @@ RSpec.describe Group do
 
   describe "#execute_hooks" do
     context "group_webhooks", :request_store do
-      let_it_be(:group) { create(:group) }
+      let_it_be(:parent_group) { create(:group) }
+      let_it_be(:group) { create(:group, parent: parent_group) }
       let_it_be(:group_hook) { create(:group_hook, group: group, member_events: true) }
+      let_it_be(:parent_group_hook) { create(:group_hook, group: parent_group, member_events: true) }
       let(:data) { { some: 'info' } }
 
       before do
@@ -915,14 +917,17 @@ RSpec.describe Group do
           stub_licensed_features(group_webhooks: true)
         end
 
-        it 'executes the hook' do
-          expect_next_instance_of(WebHookService) do |service|
-            expect(service).to receive(:async_execute).once
-          end
+        context 'execution' do
+          it 'executes the hook for self and ancestor groups by default' do
+            expect(WebHookService).to receive(:new)
+                                        .with(group_hook, data, 'member_hooks').and_call_original
+            expect(WebHookService).to receive(:new)
+                                        .with(parent_group_hook, data, 'member_hooks').and_call_original
 
             group.execute_hooks(data, :member_hooks)
           end
         end
+      end
 
       context 'when group_webhooks feature is disabled' do
         before do
@@ -938,6 +943,126 @@ RSpec.describe Group do
     end
   end
 
+  context 'subgroup hooks', :sidekiq_inline do
+    let_it_be(:grandparent_group) { create(:group) }
+    let_it_be(:parent_group) { create(:group, parent: grandparent_group) }
+    let_it_be(:subgroup) { create(:group, parent: parent_group) }
+    let_it_be(:parent_group_hook) { create(:group_hook, group: parent_group, subgroup_events: true) }
+
+    def webhook_body(subgroup:, parent_group:, event_name:)
+      {
+        created_at: subgroup.created_at.xmlschema,
+        updated_at: subgroup.updated_at.xmlschema,
+        name: subgroup.name,
+        path: subgroup.path,
+        full_path: subgroup.full_path,
+        group_id: subgroup.id,
+        parent_name: parent_group.name,
+        parent_path: parent_group.path,
+        parent_full_path: parent_group.full_path,
+        parent_group_id: parent_group.id,
+        event_name: event_name
+      }
+    end
+
+    def webhook_headers
+      {
+        'Content-Type' => 'application/json',
+        'User-Agent' => "GitLab/#{Gitlab::VERSION}",
+        'X-Gitlab-Event' => 'Subgroup Hook'
+      }
+    end
+
+    before do
+      WebMock.stub_request(:post, parent_group_hook.url)
+    end
+
+    context 'when a subgroup is added to the parent group' do
+      it 'executes the webhook' do
+        subgroup = create(:group, parent: parent_group)
+
+        expect(WebMock).to have_requested(:post, parent_group_hook.url).with(
+          headers: webhook_headers,
+          body: webhook_body(subgroup: subgroup, parent_group: parent_group, event_name: 'subgroup_create')
+        )
+      end
+    end
+
+    context 'when a subgroup is removed from the parent group' do
+      it 'executes the webhook' do
+        subgroup.destroy!
+
+        expect(WebMock).to have_requested(:post, parent_group_hook.url).with(
+          headers: webhook_headers,
+          body: webhook_body(subgroup: subgroup, parent_group: parent_group, event_name: 'subgroup_destroy')
+        )
+      end
+    end
+
+    context 'when the subgroup has subgroup webhooks enabled' do
+      let_it_be(:subgroup_hook) { create(:group_hook, group: subgroup, subgroup_events: true) }
+
+      it 'does not execute the webhook on itself' do
+        subgroup.destroy!
+
+        expect(WebMock).not_to have_requested(:post, subgroup_hook.url)
+      end
+    end
+
+    context 'ancestor groups' do
+      let_it_be(:grand_parent_group_hook) { create(:group_hook, group: grandparent_group, subgroup_events: true) }
+
+      before do
+        WebMock.stub_request(:post, grand_parent_group_hook.url)
+      end
+
+      it 'fires webhook twice when both parent & grandparent group has subgroup_events enabled' do
+        subgroup.destroy!
+
+        expect(WebMock).to have_requested(:post, grand_parent_group_hook.url)
+        expect(WebMock).to have_requested(:post, parent_group_hook.url)
+      end
+
+      context 'when parent group does not have subgroup_events enabled' do
+        before do
+          parent_group_hook.update!(subgroup_events: false)
+        end
+
+        it 'fires webhook once for the grandparent group when it has subgroup_events enabled' do
+          subgroup.destroy!
+
+          expect(WebMock).to have_requested(:post, grand_parent_group_hook.url)
+          expect(WebMock).not_to have_requested(:post, parent_group_hook.url)
+        end
+      end
+    end
+
+    context 'when the group is not a subgroup' do
+      let_it_be(:grand_parent_group_hook) { create(:group_hook, group: grandparent_group, subgroup_events: true) }
+
+      it 'does not proceed to firing any webhooks' do
+        allow(grandparent_group).to receive(:execute_hooks)
+
+        grandparent_group.destroy!
+
+        expect(grandparent_group).not_to have_received(:execute_hooks)
+      end
+    end
+
+    context 'when group webhooks are unlicensed' do
+      before do
+        subgroup.clear_memoization(:feature_available)
+        stub_licensed_features(group_webhooks: false)
+      end
+
+      it 'does not execute the webhook' do
+        subgroup.destroy!
+
+        expect(WebMock).not_to have_requested(:post, parent_group_hook.url)
+      end
+    end
+  end
+
   describe '#self_or_ancestor_marked_for_deletion' do
     context 'delayed deletion feature is not available' do
       before do

ee/spec/requests/api/group_hooks_spec.rb

@@ -120,7 +120,8 @@ RSpec.describe API::GroupHooks do
         pipeline_events: true,
         wiki_page_events: true,
         deployment_events: true,
-        releases_events: true
+        releases_events: true,
+        subgroup_events: true
       }
     end
 
@@ -146,6 +147,7 @@ RSpec.describe API::GroupHooks do
         expect(json_response['wiki_page_events']).to eq(true)
         expect(json_response['deployment_events']).to eq(true)
         expect(json_response['releases_events']).to eq(true)
+        expect(json_response['subgroup_events']).to eq(true)
         expect(json_response['enable_ssl_verification']).to eq(true)
       end
 

lib/gitlab/hook_data/subgroup_builder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HookData
+    class SubgroupBuilder < GroupBuilder
+      # Sample data
+      # {
+      #  :created_at=>"2021-01-20T09:40:12Z",
+      #  :updated_at=>"2021-01-20T09:40:12Z",
+      #  :event_name=>"subgroup_create",
+      #  :name=>"subgroup1",
+      #  :path=>"subgroup1",
+      #  :full_path=>"group1/subgroup1",
+      #  :group_id=>10,
+      #  :parent_group_id=>7,
+      #  :parent_name=>group1,
+      #  :parent_path=>group1,
+      #  :parent_full_path=>group1
+      # }
+
+      private
+
+      def event_data(event)
+        event_name =  case event
+                      when :create
+                        'subgroup_create'
+                      when :destroy
+                        'subgroup_destroy'
+                      end
+
+        { event_name: event_name }
+      end
+
+      def group_data
+        parent = group.parent
+
+        super.merge(
+          parent_group_id: parent.id,
+          parent_name: parent.name,
+          parent_path: parent.path,
+          parent_full_path: parent.full_path
+        )
+      end
+
+      def event_specific_group_data(event)
+        {}
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -32536,6 +32536,9 @@ msgstr ""
 msgid "Webhooks|Secret Token"
 msgstr ""
 
+msgid "Webhooks|Subgroup events"
+msgstr ""
+
 msgid "Webhooks|Tag push events"
 msgstr ""
 
@@ -32563,6 +32566,9 @@ msgstr ""
 msgid "Webhooks|This URL will be triggered when a new tag is pushed to the repository"
 msgstr ""
 
+msgid "Webhooks|This URL will be triggered when a subgroup is created/removed"
+msgstr ""
+
 msgid "Webhooks|This URL will be triggered when a wiki page is created/updated"
 msgstr ""
 

spec/lib/gitlab/hook_data/subgroup_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::SubgroupBuilder do
+  let_it_be(:parent_group) { create(:group) }
+  let_it_be(:subgroup) { create(:group, parent: parent_group) }
+
+  describe '#build' do
+    let(:data) { described_class.new(subgroup).build(event) }
+    let(:event_name) { data[:event_name] }
+    let(:attributes) do
+      [
+        :event_name, :created_at, :updated_at, :name, :path, :full_path, :group_id,
+        :parent_group_id, :parent_name, :parent_path, :parent_full_path
+      ]
+    end
+
+    context 'data' do
+      shared_examples_for 'includes the required attributes' do
+        it 'includes the required attributes' do
+          expect(data).to include(*attributes)
+
+          expect(data[:name]).to eq(subgroup.name)
+          expect(data[:path]).to eq(subgroup.path)
+          expect(data[:full_path]).to eq(subgroup.full_path)
+          expect(data[:group_id]).to eq(subgroup.id)
+          expect(data[:created_at]).to eq(subgroup.created_at.xmlschema)
+          expect(data[:updated_at]).to eq(subgroup.updated_at.xmlschema)
+          expect(data[:parent_name]).to eq(parent_group.name)
+          expect(data[:parent_path]).to eq(parent_group.path)
+          expect(data[:parent_full_path]).to eq(parent_group.full_path)
+          expect(data[:parent_group_id]).to eq(parent_group.id)
+        end
+      end
+
+      context 'on create' do
+        let(:event) { :create }
+
+        it { expect(event_name).to eq('subgroup_create') }
+        it_behaves_like 'includes the required attributes'
+      end
+
+      context 'on destroy' do
+        let(:event) { :destroy }
+
+        it { expect(event_name).to eq('subgroup_destroy') }
+        it_behaves_like 'includes the required attributes'
+      end
+    end
+  end
+end