Allow push events on branches in ScmWebhookEventValidator

Co-authored-by: Eduardo Navarro <enavarro@suse.com>
openSUSE · Sep 15, 2021 · 8ef1032 · 8ef1032
1 parent a9aa51e
commit 8ef1032
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 43 deletions.
diff --git a/src/api/app/validators/scm_webhook_event_validator.rb b/src/api/app/validators/scm_webhook_event_validator.rb
@@ -1,6 +1,6 @@
 class ScmWebhookEventValidator < ActiveModel::Validator
-  ALLOWED_GITHUB_EVENTS = ['pull_request'].freeze
-  ALLOWED_GITLAB_EVENTS = ['Merge Request Hook'].freeze
+  ALLOWED_GITHUB_EVENTS = ['pull_request', 'push'].freeze
+  ALLOWED_GITLAB_EVENTS = ['Merge Request Hook', 'Push Hook'].freeze
 
   ALLOWED_PULL_REQUEST_ACTIONS = ['closed', 'opened', 'reopened', 'synchronize'].freeze
   ALLOWED_MERGE_REQUEST_ACTIONS = ['close', 'merge', 'open', 'reopen', 'update'].freeze
@@ -25,6 +25,8 @@ def valid_github_event?
       return true if ALLOWED_PULL_REQUEST_ACTIONS.include?(@record.payload[:action])
 
       @record.errors.add(:base, 'Pull request action not supported.')
+    when 'push'
+      valid_push_event?
     else
       true
     end
@@ -39,8 +41,16 @@ def valid_gitlab_event?
       return true if ALLOWED_MERGE_REQUEST_ACTIONS.include?(@record.payload[:action])
 
       @record.errors.add(:base, 'Merge request action not supported.')
+    when 'Push Hook'
+      valid_push_event?
     else
       true
     end
   end
+
+  def valid_push_event?
+    return true if @record.payload.fetch(:ref, '').start_with?('refs/heads/')
+
+    @record.errors.add(:base, 'Push event supported only for branches.')
+  end
 end
diff --git a/src/api/spec/validators/scm_webhook_event_validator_spec.rb b/src/api/spec/validators/scm_webhook_event_validator_spec.rb
@@ -9,21 +9,11 @@
     end
   end
 
-  let(:payload) do
-    {
-      scm: scm,
-      event: event,
-      action: action
-    }
-  end
-
   describe '#validate' do
     subject { fake_model.new(payload) }
 
     context 'when the SCM is unsupported' do
-      let(:scm) { 'GitHoob' }
-      let(:event) { 'pull_request' }
-      let(:action) { 'updated' }
+      let(:payload) { { scm: 'GitHoob', event: 'pull_request', action: 'updated' } }
 
       it 'is not valid and has an error message' do
         subject.valid?
@@ -32,11 +22,8 @@
     end
 
     context 'when the SCM is GitHub' do
-      let(:scm) { 'github' }
-
       context 'for an unsupported event' do
-        let(:event) { 'something' }
-        let(:action) { 'opened' }
+        let(:payload) { { scm: 'github', event: 'something', action: 'opened' } }
 
         it 'is not valid and has an error message' do
           subject.valid?
@@ -45,8 +32,7 @@
       end
 
       context 'for a pull request with an unsupported action' do
-        let(:event) { 'pull_request' }
-        let(:action) { 'something' }
+        let(:payload) { { scm: 'github', event: 'pull_request', action: 'something' } }
 
         it 'is not valid and has an error message' do
           subject.valid?
@@ -55,40 +41,48 @@
       end
 
       context 'for a new pull request' do
-        let(:event) { 'pull_request' }
-        let(:action) { 'opened' }
+        let(:payload) { { scm: 'github', event: 'pull_request', action: 'opened' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a pull request which was updated' do
-        let(:event) { 'pull_request' }
-        let(:action) { 'synchronize' }
+        let(:payload) { { scm: 'github', event: 'pull_request', action: 'synchronize' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a pull request which was closed/merged' do
-        let(:event) { 'pull_request' }
-        let(:action) { 'closed' }
+        let(:payload) { { scm: 'github', event: 'pull_request', action: 'closed' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a pull request which was reopened' do
-        let(:event) { 'pull_request' }
-        let(:action) { 'reopened' }
+        let(:payload) { { scm: 'github', event: 'pull_request', action: 'reopened' } }
+
+        it { is_expected.to be_valid }
+      end
+
+      context "for a push event which isn't for a branch" do
+        let(:payload) { { scm: 'github', event: 'push', ref: 'something' } }
+
+        it 'is not valid and has an error message' do
+          subject.valid?
+          expect(subject.errors.full_messages.to_sentence).to eq('Push event supported only for branches.')
+        end
+      end
+
+      context 'for a push event which is for a branch' do
+        let(:payload) { { scm: 'github', event: 'push', ref: 'refs/heads/master' } }
 
         it { is_expected.to be_valid }
       end
     end
 
     context 'when the SCM is GitLab' do
-      let(:scm) { 'gitlab' }
-
       context 'for an unsupported event' do
-        let(:event) { 'something' }
-        let(:action) { 'open' }
+        let(:payload) { { scm: 'gitlab', event: 'something', action: 'open' } }
 
         it 'is not valid and has an error message' do
           subject.valid?
@@ -97,8 +91,7 @@
       end
 
       context 'for a merge request with an unsupported action' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'something' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'something' } }
 
         it 'is not valid and has an error message' do
           subject.valid?
@@ -107,36 +100,46 @@
       end
 
       context 'for a new merge request' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'open' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'open' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a merge request which was updated' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'update' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'update' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a merge request which was closed' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'close' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'close' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a merge request which was merged' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'merge' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'merge' } }
 
         it { is_expected.to be_valid }
       end
 
       context 'for a merge request which was reopened' do
-        let(:event) { 'Merge Request Hook' }
-        let(:action) { 'reopen' }
+        let(:payload) { { scm: 'gitlab', event: 'Merge Request Hook', action: 'reopen' } }
+
+        it { is_expected.to be_valid }
+      end
+
+      context "for a push event which isn't for a branch" do
+        let(:payload) { { scm: 'gitlab', event: 'Push Hook', ref: 'something' } }
+
+        it 'is not valid and has an error message' do
+          subject.valid?
+          expect(subject.errors.full_messages.to_sentence).to eq('Push event supported only for branches.')
+        end
+      end
+
+      context 'for a push event which is for a branch' do
+        let(:payload) { { scm: 'gitlab', event: 'Push Hook', ref: 'refs/heads/master' } }
 
         it { is_expected.to be_valid }
       end