Refactor Webhook support to allow verification of parameters

Ensure webhook requests are only accepted after proper verification of expected headers, repository address and branch, using the newly implemented library. To that effect, split up the logic to extract that information from requests to it's own class, and use it in the notify_push action. Also, add factories to make better testing of the notify_push action possible, such as a factory similar to a real request from Gitlab, and one for the Kalibro Client repository on Gitlab. Adding other hook providers should be quite a lot easier after this is applied.

Refactor Webhook support to allow verification of parameters
Ensure webhook requests are only accepted after proper verification of expected headers, repository address and branch, using the newly implemented library. To that effect, split up the logic to extract that information from requests to it's own class, and use it in the notify_push action. Also, add factories to make better testing of the notify_push action possible, such as a factory similar to a real request from Gitlab, and one for the Kalibro Client repository on Gitlab. Adding other hook providers should be quite a lot easier after this is applied.
Daniel
1 parent c31adf01
Showing 4 changed files with 92 additions and 42 deletions Show diff stats
app/controllers/repositories_controller.rb
features/repository/notify_push.feature
features/step_definitions/repository_steps.rb
spec/controllers/repositories_controller_spec.rb
+require 'webhooks'
+
 include OwnershipAuthentication
  
 class RepositoriesController < ApplicationController
   before_action :authenticate_user!, except: [:show, :state, :state_with_date, :index, :notify_push]
   before_action :project_owner?, only: [:new, :create], unless: Proc.new { params[:project_id].nil? }
   before_action :repository_owner?, only: [:edit, :update, :destroy, :process_repository]
-  before_action :set_repository, only: [:show, :edit, :update, :destroy, :state, :state_with_date, :process_repository]
+  before_action :set_repository, only: [:show, :edit, :update, :destroy, :state, :state_with_date, :process_repository,
+                                        :notify_push]
   before_action :set_project_id_repository_types_and_configurations, only: [:new, :edit]
  
   # Gitlab can't send a CSRF token, don't require one
@@ -100,14 +103,21 @@ class RepositoriesController &lt; ApplicationController
   end
  
   def notify_push
-    gitlab_event = request.headers['X-Gitlab-Event']
-    if gitlab_event.nil? || !gitlab_event.end_with?('Push Hook')
-      return render nothing: true, status: :unprocessable_entity
+    hook_info = Webhooks::GitLab.new(request, @repository)
+    if hook_info.valid_request?
+      if hook_info.valid_address?
+        if hook_info.valid_branch?
+          @repository.cancel_processing_of_repository unless %w(READY ERROR).include? @repository.last_processing_state
+          @repository.process
+        end
+
+        render nothing: true, status: :ok
+      else
+        render nothing: true, status: :forbidden
+      end
+    else
+      render nothing: true, status: :unprocessable_entity
     end
-    set_repository
-    @repository.cancel_processing_of_repository unless %w(READY ERROR).include? @repository.last_processing_state
-    @repository.process
-    render nothing: true, status: :ok
   end
  
   private
@@ -7,7 +7,7 @@ Feature: Notify push to repository
   Scenario: Valid repository
     Given I am a regular user
     And I have a sample configuration with hotspot metrics
-    And I have a sample repository
+    And I have a Kalibro Client GitLab repository
     And I start to process that repository
     And I wait up for a ready processing
     When I push some commits to the repository
@@ -25,7 +25,7 @@ Feature: Notify push to repository
     And I have a sample reading group
     And I have a sample configuration with the Saikuro native metric
     And I have a compound metric configuration with script "rtrnaqdfwqefwqr213r2145211234ed a = b=2" within the given mezuro configuration
-    And I have a sample repository
+    And I have a Kalibro Client GitLab repository
     And I start to process that repository
     And I wait up for an error processing
     When I push some commits to the repository
@@ -116,6 +116,10 @@ Given(/^I have a sample repository$/) do
   @independent_repository = FactoryGirl.create(:ruby_repository, kalibro_configuration_id: @kalibro_configuration.id)
 end
  
+Given(/^I have a Kalibro Client GitLab repository$/) do
+  @independent_repository = FactoryGirl.create(:kalibro_client_gitlab_repository, kalibro_configuration_id: @kalibro_configuration.id)
+end
+
 Given(/^I am at the All Repositories page$/) do
   visit repositories_path
 end
@@ -163,11 +167,19 @@ When(/^I get the Creation Date information as &quot;(.*?)&quot;$/) do |variable|
 end
  
 When(/^I push some commits to the repository$/) do
-  post repository_notify_push_path(id: @repository.id), {}, {'HTTP_X_GITLAB_EVENT' => 'Push Hook'}
+  request = FactoryGirl.build(:gitlab_webhook_request)
+  request.headers.each do |k, v|
+    header k, v
+  end
+  @response = post repository_notify_push_path(id: @repository.id), request.params
 end
  
 When(/^I push some commits to an invalid repository$/) do
-  @response = post repository_notify_push_path(id: 0), {}, {'HTTP_X_GITLAB_EVENT' => 'Push Hook'}
+  request = FactoryGirl.build(:gitlab_webhook_request)
+  request.headers.each do |k, v|
+    header k, v
+  end
+  @response = post repository_notify_push_path(id: 0), request.params
 end
  
 Then(/^I should see the sample metric's name$/) do
@@ -471,11 +471,15 @@ describe RepositoriesController, :type =&gt; :controller do
   end
  
   describe 'notify_push' do
-    let(:repository) { FactoryGirl.build(:repository) }
+    let(:repository) { FactoryGirl.build(:kalibro_client_gitlab_repository) }
+    let(:webhook_request) { FactoryGirl.build(:gitlab_webhook_request) }
  
     def post_push
-      @request.env['HTTP_X_GITLAB_EVENT'] = ['Push Hook', 'Tag Push Hook'].sample
-      post :notify_push, id: repository.id, format: :json
+      webhook_request.headers.each do |k, v|
+        @request.headers[k] = v
+      end
+
+      post :notify_push, {id: repository.id, format: :json}.merge(webhook_request.params)
     end
  
     context 'with a valid repository' do
@@ -483,35 +487,67 @@ describe RepositoriesController, :type =&gt; :controller do
         Repository.expects(:find).with(repository.id).returns(repository)
       end
  
-      context 'when the repository is being processed' do
-        before do
-          repository.expects(:last_processing_state).returns('INTERPRETING')
-          repository.expects(:cancel_processing_of_repository).once
-          repository.expects(:process).once
+      context 'without a valid address' do
+        before :each do
+          Webhooks::Base.any_instance.expects(:valid_address?).returns(false)
           post_push
         end
  
-        it { is_expected.to respond_with(:ok) }
+        it { is_expected.to respond_with(:forbidden) }
       end
  
-      context "when the repository's processing resulted in an error" do
-        before do
-          repository.expects(:last_processing_state).returns('ERROR')
-          repository.expects(:process).once
-          post_push
+      context 'with a valid address' do
+        before :each do
+          Webhooks::Base.any_instance.expects(:valid_address?).returns(true)
         end
  
-        it { is_expected.to respond_with(:ok) }
-      end
+        context 'without a matching branch' do
+          before :each do
+            Webhooks::Base.any_instance.expects(:valid_branch?).returns(false)
+            repository.expects(:cancel_processing_of_repository).never
+            repository.expects(:process).never
+            post_push
+          end
  
-      context 'when the repository is not being processed' do
-        before do
-          repository.expects(:last_processing_state).returns('READY')
-          repository.expects(:process).once
-          post_push
+          it { is_expected.to respond_with(:ok) }
         end
  
-        it { is_expected.to respond_with(:ok) }
+        context 'with a matching branch' do
+          before :each do
+            Webhooks::Base.any_instance.expects(:valid_branch?).returns(true)
+          end
+
+          context 'when the repository is being processed' do
+            before do
+              repository.expects(:last_processing_state).returns('INTERPRETING')
+              repository.expects(:cancel_processing_of_repository).once
+              repository.expects(:process).once
+              post_push
+            end
+
+            it { is_expected.to respond_with(:ok) }
+          end
+
+          context "when the repository's processing resulted in an error" do
+            before do
+              repository.expects(:last_processing_state).returns('ERROR')
+              repository.expects(:process).once
+              post_push
+            end
+
+            it { is_expected.to respond_with(:ok) }
+          end
+
+          context 'when the repository is not being processed' do
+            before do
+              repository.expects(:last_processing_state).returns('READY')
+              repository.expects(:process).once
+              post_push
+            end
+
+            it { is_expected.to respond_with(:ok) }
+          end
+        end
       end
     end
  
@@ -523,13 +559,5 @@ describe RepositoriesController, :type =&gt; :controller do
  
       it { is_expected.to respond_with(:not_found) }
     end
-
-    context 'with an invalid header' do
-      before :each do
-        post :notify_push, id: repository.id, format: :json
-      end
-
-      it { is_expected.to respond_with(:unprocessable_entity) }
-    end
   end
 end
	1	+require 'webhooks'
	2	+
1	3	include OwnershipAuthentication
2	4
3	5	class RepositoriesController < ApplicationController
4	6	before_action :authenticate_user!, except: [:show, :state, :state_with_date, :index, :notify_push]
5	7	before_action :project_owner?, only: [:new, :create], unless: Proc.new { params[:project_id].nil? }
6	8	before_action :repository_owner?, only: [:edit, :update, :destroy, :process_repository]
7		- before_action :set_repository, only: [:show, :edit, :update, :destroy, :state, :state_with_date, :process_repository]
	9	+ before_action :set_repository, only: [:show, :edit, :update, :destroy, :state, :state_with_date, :process_repository,
	10	+ :notify_push]
8	11	before_action :set_project_id_repository_types_and_configurations, only: [:new, :edit]
9	12
10	13	# Gitlab can't send a CSRF token, don't require one
...	...	@@ -100,14 +103,21 @@ class RepositoriesController < ApplicationController
100	103	end
101	104
102	105	def notify_push
103		- gitlab_event = request.headers['X-Gitlab-Event']
104		- if gitlab_event.nil? \|\| !gitlab_event.end_with?('Push Hook')
105		- return render nothing: true, status: :unprocessable_entity
	106	+ hook_info = Webhooks::GitLab.new(request, @repository)
	107	+ if hook_info.valid_request?
	108	+ if hook_info.valid_address?
	109	+ if hook_info.valid_branch?
	110	+ @repository.cancel_processing_of_repository unless %w(READY ERROR).include? @repository.last_processing_state
	111	+ @repository.process
	112	+ end
	113	+
	114	+ render nothing: true, status: :ok
	115	+ else
	116	+ render nothing: true, status: :forbidden
	117	+ end
	118	+ else
	119	+ render nothing: true, status: :unprocessable_entity
106	120	end
107		- set_repository
108		- @repository.cancel_processing_of_repository unless %w(READY ERROR).include? @repository.last_processing_state
109		- @repository.process
110		- render nothing: true, status: :ok
111	121	end
112	122
113	123	private
...	...
...	...	@@ -7,7 +7,7 @@ Feature: Notify push to repository
7	7	Scenario: Valid repository
8	8	Given I am a regular user
9	9	And I have a sample configuration with hotspot metrics
10		- And I have a sample repository
	10	+ And I have a Kalibro Client GitLab repository
11	11	And I start to process that repository
12	12	And I wait up for a ready processing
13	13	When I push some commits to the repository
...	...	@@ -25,7 +25,7 @@ Feature: Notify push to repository
25	25	And I have a sample reading group
26	26	And I have a sample configuration with the Saikuro native metric
27	27	And I have a compound metric configuration with script "rtrnaqdfwqefwqr213r2145211234ed a = b=2" within the given mezuro configuration
28		- And I have a sample repository
	28	+ And I have a Kalibro Client GitLab repository
29	29	And I start to process that repository
30	30	And I wait up for an error processing
31	31	When I push some commits to the repository
...	...
...	...	@@ -116,6 +116,10 @@ Given(/^I have a sample repository$/) do
116	116	@independent_repository = FactoryGirl.create(:ruby_repository, kalibro_configuration_id: @kalibro_configuration.id)
117	117	end
118	118
	119	+Given(/^I have a Kalibro Client GitLab repository$/) do
	120	+ @independent_repository = FactoryGirl.create(:kalibro_client_gitlab_repository, kalibro_configuration_id: @kalibro_configuration.id)
	121	+end
	122	+
119	123	Given(/^I am at the All Repositories page$/) do
120	124	visit repositories_path
121	125	end
...	...	@@ -163,11 +167,19 @@ When(/^I get the Creation Date information as "(.*?)"$/) do \|variable\|
163	167	end
164	168
165	169	When(/^I push some commits to the repository$/) do
166		- post repository_notify_push_path(id: @repository.id), {}, {'HTTP_X_GITLAB_EVENT' => 'Push Hook'}
	170	+ request = FactoryGirl.build(:gitlab_webhook_request)
	171	+ request.headers.each do \|k, v\|
	172	+ header k, v
	173	+ end
	174	+ @response = post repository_notify_push_path(id: @repository.id), request.params
167	175	end
168	176
169	177	When(/^I push some commits to an invalid repository$/) do
170		- @response = post repository_notify_push_path(id: 0), {}, {'HTTP_X_GITLAB_EVENT' => 'Push Hook'}
	178	+ request = FactoryGirl.build(:gitlab_webhook_request)
	179	+ request.headers.each do \|k, v\|
	180	+ header k, v
	181	+ end
	182	+ @response = post repository_notify_push_path(id: 0), request.params
171	183	end
172	184
173	185	Then(/^I should see the sample metric's name$/) do
...	...
...	...	@@ -471,11 +471,15 @@ describe RepositoriesController, :type => :controller do
471	471	end
472	472
473	473	describe 'notify_push' do
474		- let(:repository) { FactoryGirl.build(:repository) }
	474	+ let(:repository) { FactoryGirl.build(:kalibro_client_gitlab_repository) }
	475	+ let(:webhook_request) { FactoryGirl.build(:gitlab_webhook_request) }
475	476
476	477	def post_push
477		- @request.env['HTTP_X_GITLAB_EVENT'] = ['Push Hook', 'Tag Push Hook'].sample
478		- post :notify_push, id: repository.id, format: :json
	478	+ webhook_request.headers.each do \|k, v\|
	479	+ @request.headers[k] = v
	480	+ end
	481	+
	482	+ post :notify_push, {id: repository.id, format: :json}.merge(webhook_request.params)
479	483	end
480	484
481	485	context 'with a valid repository' do
...	...	@@ -483,35 +487,67 @@ describe RepositoriesController, :type => :controller do
483	487	Repository.expects(:find).with(repository.id).returns(repository)
484	488	end
485	489
486		- context 'when the repository is being processed' do
487		- before do
488		- repository.expects(:last_processing_state).returns('INTERPRETING')
489		- repository.expects(:cancel_processing_of_repository).once
490		- repository.expects(:process).once
	490	+ context 'without a valid address' do
	491	+ before :each do
	492	+ Webhooks::Base.any_instance.expects(:valid_address?).returns(false)
491	493	post_push
492	494	end
493	495
494		- it { is_expected.to respond_with(:ok) }
	496	+ it { is_expected.to respond_with(:forbidden) }
495	497	end
496	498
497		- context "when the repository's processing resulted in an error" do
498		- before do
499		- repository.expects(:last_processing_state).returns('ERROR')
500		- repository.expects(:process).once
501		- post_push
	499	+ context 'with a valid address' do
	500	+ before :each do
	501	+ Webhooks::Base.any_instance.expects(:valid_address?).returns(true)
502	502	end
503	503
504		- it { is_expected.to respond_with(:ok) }
505		- end
	504	+ context 'without a matching branch' do
	505	+ before :each do
	506	+ Webhooks::Base.any_instance.expects(:valid_branch?).returns(false)
	507	+ repository.expects(:cancel_processing_of_repository).never
	508	+ repository.expects(:process).never
	509	+ post_push
	510	+ end
506	511
507		- context 'when the repository is not being processed' do
508		- before do
509		- repository.expects(:last_processing_state).returns('READY')
510		- repository.expects(:process).once
511		- post_push
	512	+ it { is_expected.to respond_with(:ok) }
512	513	end
513	514
514		- it { is_expected.to respond_with(:ok) }
	515	+ context 'with a matching branch' do
	516	+ before :each do
	517	+ Webhooks::Base.any_instance.expects(:valid_branch?).returns(true)
	518	+ end
	519	+
	520	+ context 'when the repository is being processed' do
	521	+ before do
	522	+ repository.expects(:last_processing_state).returns('INTERPRETING')
	523	+ repository.expects(:cancel_processing_of_repository).once
	524	+ repository.expects(:process).once
	525	+ post_push
	526	+ end
	527	+
	528	+ it { is_expected.to respond_with(:ok) }
	529	+ end
	530	+
	531	+ context "when the repository's processing resulted in an error" do
	532	+ before do
	533	+ repository.expects(:last_processing_state).returns('ERROR')
	534	+ repository.expects(:process).once
	535	+ post_push
	536	+ end
	537	+
	538	+ it { is_expected.to respond_with(:ok) }
	539	+ end
	540	+
	541	+ context 'when the repository is not being processed' do
	542	+ before do
	543	+ repository.expects(:last_processing_state).returns('READY')
	544	+ repository.expects(:process).once
	545	+ post_push
	546	+ end
	547	+
	548	+ it { is_expected.to respond_with(:ok) }
	549	+ end
	550	+ end
515	551	end
516	552	end
517	553
...	...	@@ -523,13 +559,5 @@ describe RepositoriesController, :type => :controller do
523	559
524	560	it { is_expected.to respond_with(:not_found) }
525	561	end
526		-
527		- context 'with an invalid header' do
528		- before :each do
529		- post :notify_push, id: repository.id, format: :json
530		- end
531		-
532		- it { is_expected.to respond_with(:unprocessable_entity) }
533		- end
534	562	end
535	563	end
...	...