Commit b4fcf54ed9aee21e7fc610c5d6902f1f36024280

Authored by Daniel
2 parents 47ad2ee2 4734ba1b
Exists in colab and in 2 other branches master, stable

Merge pull request #328 from mezuro/gitlab_hooks_verification

Refactor Webhook support to allow verification of parameters
app/controllers/repositories_controller.rb
  1 +require 'webhooks'
  2 +
1 include OwnershipAuthentication 3 include OwnershipAuthentication
2 4
3 class RepositoriesController < ApplicationController 5 class RepositoriesController < ApplicationController
4 before_action :authenticate_user!, except: [:show, :state, :state_with_date, :index, :notify_push] 6 before_action :authenticate_user!, except: [:show, :state, :state_with_date, :index, :notify_push]
5 before_action :project_owner?, only: [:new, :create], unless: Proc.new { params[:project_id].nil? } 7 before_action :project_owner?, only: [:new, :create], unless: Proc.new { params[:project_id].nil? }
6 before_action :repository_owner?, only: [:edit, :update, :destroy, :process_repository] 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 before_action :set_project_id_repository_types_and_configurations, only: [:new, :edit] 11 before_action :set_project_id_repository_types_and_configurations, only: [:new, :edit]
9 12
10 # Gitlab can't send a CSRF token, don't require one 13 # Gitlab can't send a CSRF token, don't require one
@@ -100,14 +103,21 @@ class RepositoriesController &lt; ApplicationController @@ -100,14 +103,21 @@ class RepositoriesController &lt; ApplicationController
100 end 103 end
101 104
102 def notify_push 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 end 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 end 121 end
112 122
113 private 123 private
features/repository/notify_push.feature
@@ -7,7 +7,7 @@ Feature: Notify push to repository @@ -7,7 +7,7 @@ Feature: Notify push to repository
7 Scenario: Valid repository 7 Scenario: Valid repository
8 Given I am a regular user 8 Given I am a regular user
9 And I have a sample configuration with hotspot metrics 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 And I start to process that repository 11 And I start to process that repository
12 And I wait up for a ready processing 12 And I wait up for a ready processing
13 When I push some commits to the repository 13 When I push some commits to the repository
@@ -25,7 +25,7 @@ Feature: Notify push to repository @@ -25,7 +25,7 @@ Feature: Notify push to repository
25 And I have a sample reading group 25 And I have a sample reading group
26 And I have a sample configuration with the Saikuro native metric 26 And I have a sample configuration with the Saikuro native metric
27 And I have a compound metric configuration with script "rtrnaqdfwqefwqr213r2145211234ed a = b=2" within the given mezuro configuration 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 And I start to process that repository 29 And I start to process that repository
30 And I wait up for an error processing 30 And I wait up for an error processing
31 When I push some commits to the repository 31 When I push some commits to the repository
features/step_definitions/repository_steps.rb
@@ -116,6 +116,10 @@ Given(/^I have a sample repository$/) do @@ -116,6 +116,10 @@ Given(/^I have a sample repository$/) do
116 @independent_repository = FactoryGirl.create(:ruby_repository, kalibro_configuration_id: @kalibro_configuration.id) 116 @independent_repository = FactoryGirl.create(:ruby_repository, kalibro_configuration_id: @kalibro_configuration.id)
117 end 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 Given(/^I am at the All Repositories page$/) do 123 Given(/^I am at the All Repositories page$/) do
120 visit repositories_path 124 visit repositories_path
121 end 125 end
@@ -163,11 +167,15 @@ When(/^I get the Creation Date information as &quot;(.*?)&quot;$/) do |variable| @@ -163,11 +167,15 @@ When(/^I get the Creation Date information as &quot;(.*?)&quot;$/) do |variable|
163 end 167 end
164 168
165 When(/^I push some commits to the repository$/) do 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 + set_headers(request.headers)
  172 + page.driver.post(repository_notify_push_path(id: @repository.id), request.params)
167 end 173 end
168 174
169 When(/^I push some commits to an invalid repository$/) do 175 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'} 176 + request = FactoryGirl.build(:gitlab_webhook_request)
  177 + set_headers(request.headers)
  178 + page.driver.post(repository_notify_push_path(id: 0), request.params)
171 end 179 end
172 180
173 Then(/^I should see the sample metric's name$/) do 181 Then(/^I should see the sample metric's name$/) do
@@ -271,5 +279,5 @@ Then(/^Mezuro should process the repository again$/) do @@ -271,5 +279,5 @@ Then(/^Mezuro should process the repository again$/) do
271 end 279 end
272 280
273 Then(/^I should get a not found error$/) do 281 Then(/^I should get a not found error$/) do
274 - expect(@response.status).to eq(404) 282 + expect(page.driver.status_code).to eq(404)
275 end 283 end
features/support/env.rb
@@ -20,8 +20,11 @@ end @@ -20,8 +20,11 @@ end
20 # instead of editing this one. Cucumber will automatically load all features/**/*.rb 20 # instead of editing this one. Cucumber will automatically load all features/**/*.rb
21 # files. 21 # files.
22 22
  23 +require 'warden'
23 require 'cucumber/rails' 24 require 'cucumber/rails'
24 require 'capybara/poltergeist' 25 require 'capybara/poltergeist'
  26 +require_relative 'header'
  27 +
25 #Capybara.default_driver = :poltergeist 28 #Capybara.default_driver = :poltergeist
26 Capybara.javascript_driver = :poltergeist 29 Capybara.javascript_driver = :poltergeist
27 Capybara.default_max_wait_time = 20 # default is 2 seconds 30 Capybara.default_max_wait_time = 20 # default is 2 seconds
@@ -81,5 +84,6 @@ Cucumber::Rails::Database.javascript_strategy = :truncation @@ -81,5 +84,6 @@ Cucumber::Rails::Database.javascript_strategy = :truncation
81 # KalibroClient hooks 84 # KalibroClient hooks
82 require 'kalibro_client/kalibro_cucumber_helpers/hooks.rb' 85 require 'kalibro_client/kalibro_cucumber_helpers/hooks.rb'
83 86
84 -# Warden test helpers so the user authentication can be as fast as possible  
85 -include Warden::Test::Helpers 87 +Warden.test_mode!
  88 +World(Warden::Test::Helpers, HeaderUtils)
  89 +
features/support/header.rb 0 → 100644
@@ -0,0 +1,15 @@ @@ -0,0 +1,15 @@
  1 +module HeaderUtils
  2 + def set_header(key, value)
  3 + header_method = nil
  4 + if defined?(page) && ! page.driver.nil?
  5 + header_method = [:add_header, :header].find(&page.driver.method(:respond_to?))
  6 + end
  7 +
  8 + raise StandardError.new("No header setting method available in current driver: #{page.driver}") unless header_method
  9 + page.driver.send(header_method, key, value)
  10 + end
  11 +
  12 + def set_headers(headers)
  13 + headers.each(&method(:set_header))
  14 + end
  15 +end
lib/webhooks.rb 0 → 100644
@@ -0,0 +1,5 @@ @@ -0,0 +1,5 @@
  1 +require 'webhooks/base'
  2 +require 'webhooks/gitlab'
  3 +
  4 +module Webhooks
  5 +end
lib/webhooks/base.rb 0 → 100644
@@ -0,0 +1,44 @@ @@ -0,0 +1,44 @@
  1 +module Webhooks
  2 + class Base
  3 + attr_reader :request, :repository
  4 +
  5 + def initialize(request, repository)
  6 + @request = request
  7 + @repository = repository
  8 + end
  9 +
  10 + # Returns true if and only if the remote addresses in the request and repository match.
  11 + def valid_address?
  12 + repository.address == webhook_address
  13 + end
  14 +
  15 + # Returns true if and only if the branch in the request and repository match.
  16 + def valid_branch?
  17 + repository.branch == webhook_branch
  18 + end
  19 +
  20 + # Returns true if the request parameters, as determined by the particular hook service, are valid
  21 + # It will usually check headers, IP ranges, signatures, or any other relevant information.
  22 + def valid_request?; raise NotImplementedError; end
  23 +
  24 + # Extracts the remote address from the webhook request.
  25 + def webhook_address; raise NotImplementedError; end
  26 +
  27 + # Extracts the branch from the webhook request.
  28 + def webhook_branch; raise NotImplementedError; end
  29 +
  30 + protected
  31 +
  32 + # Converts a git ref name to a branch. This is an utility function for webhook formats that only provide the ref
  33 + # updated in their information. Returns nil if the passed parameter is not a valid ref.
  34 + # The expected format is 'refs/heads/#{branch_name}'. Anything else will be rejected.
  35 + def branch_from_ref(ref)
  36 + return nil if !ref
  37 +
  38 + ref = ref.split('/')
  39 + return nil if ref.size != 3 || ref[0..1] != ['refs', 'heads']
  40 +
  41 + ref.last
  42 + end
  43 + end
  44 +end
lib/webhooks/gitlab.rb 0 → 100644
@@ -0,0 +1,19 @@ @@ -0,0 +1,19 @@
  1 +module Webhooks
  2 + class GitLab < Base
  3 + def valid_request?
  4 + request.headers['X-Gitlab-Event'] == 'Push Hook'
  5 + end
  6 +
  7 + def webhook_address
  8 + begin
  9 + request.params.fetch(:project).fetch(:git_http_url)
  10 + rescue KeyError
  11 + return nil
  12 + end
  13 + end
  14 +
  15 + def webhook_branch
  16 + branch_from_ref(request.params[:ref])
  17 + end
  18 + end
  19 +end
spec/controllers/repositories_controller_spec.rb
@@ -471,11 +471,12 @@ describe RepositoriesController, :type =&gt; :controller do @@ -471,11 +471,12 @@ describe RepositoriesController, :type =&gt; :controller do
471 end 471 end
472 472
473 describe 'notify_push' do 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 def post_push 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 + @request.headers.merge!(webhook_request.headers)
  479 + post :notify_push, {id: repository.id, format: :json}.merge(webhook_request.params)
479 end 480 end
480 481
481 context 'with a valid repository' do 482 context 'with a valid repository' do
@@ -483,35 +484,67 @@ describe RepositoriesController, :type =&gt; :controller do @@ -483,35 +484,67 @@ describe RepositoriesController, :type =&gt; :controller do
483 Repository.expects(:find).with(repository.id).returns(repository) 484 Repository.expects(:find).with(repository.id).returns(repository)
484 end 485 end
485 486
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 487 + context 'without a valid address' do
  488 + before :each do
  489 + Webhooks::Base.any_instance.expects(:valid_address?).returns(false)
491 post_push 490 post_push
492 end 491 end
493 492
494 - it { is_expected.to respond_with(:ok) } 493 + it { is_expected.to respond_with(:forbidden) }
495 end 494 end
496 495
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 496 + context 'with a valid address' do
  497 + before :each do
  498 + Webhooks::Base.any_instance.expects(:valid_address?).returns(true)
502 end 499 end
503 500
504 - it { is_expected.to respond_with(:ok) }  
505 - end 501 + context 'without a matching branch' do
  502 + before :each do
  503 + Webhooks::Base.any_instance.expects(:valid_branch?).returns(false)
  504 + repository.expects(:cancel_processing_of_repository).never
  505 + repository.expects(:process).never
  506 + post_push
  507 + end
506 508
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 509 + it { is_expected.to respond_with(:ok) }
512 end 510 end
513 511
514 - it { is_expected.to respond_with(:ok) } 512 + context 'with a matching branch' do
  513 + before :each do
  514 + Webhooks::Base.any_instance.expects(:valid_branch?).returns(true)
  515 + end
  516 +
  517 + context 'when the repository is being processed' do
  518 + before do
  519 + repository.expects(:last_processing_state).returns('INTERPRETING')
  520 + repository.expects(:cancel_processing_of_repository).once
  521 + repository.expects(:process).once
  522 + post_push
  523 + end
  524 +
  525 + it { is_expected.to respond_with(:ok) }
  526 + end
  527 +
  528 + context "when the repository's processing resulted in an error" do
  529 + before do
  530 + repository.expects(:last_processing_state).returns('ERROR')
  531 + repository.expects(:process).once
  532 + post_push
  533 + end
  534 +
  535 + it { is_expected.to respond_with(:ok) }
  536 + end
  537 +
  538 + context 'when the repository is not being processed' do
  539 + before do
  540 + repository.expects(:last_processing_state).returns('READY')
  541 + repository.expects(:process).once
  542 + post_push
  543 + end
  544 +
  545 + it { is_expected.to respond_with(:ok) }
  546 + end
  547 + end
515 end 548 end
516 end 549 end
517 550
@@ -523,13 +556,5 @@ describe RepositoriesController, :type =&gt; :controller do @@ -523,13 +556,5 @@ describe RepositoriesController, :type =&gt; :controller do
523 556
524 it { is_expected.to respond_with(:not_found) } 557 it { is_expected.to respond_with(:not_found) }
525 end 558 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 end 559 end
535 end 560 end
spec/factories/repositories.rb
@@ -36,6 +36,18 @@ FactoryGirl.define do @@ -36,6 +36,18 @@ FactoryGirl.define do
36 kalibro_configuration_id 1 36 kalibro_configuration_id 1
37 end 37 end
38 38
  39 + factory :kalibro_client_gitlab_repository, class: Repository do
  40 + id 3
  41 + name "KalibroClient"
  42 + description "KalibroClient"
  43 + license "GPLv3"
  44 + period 1
  45 + scm_type "GIT"
  46 + address "https://gitlab.com/mezuro/kalibro_client.git"
  47 + branch 'master'
  48 + kalibro_configuration_id 1
  49 + end
  50 +
39 factory :another_repository, parent: :repository do 51 factory :another_repository, parent: :repository do
40 id 2 52 id 2
41 end 53 end
spec/factories/webhooks.rb 0 → 100644
@@ -0,0 +1,40 @@ @@ -0,0 +1,40 @@
  1 +require 'action_controller/test_case'
  2 +
  3 +class RequestInfo
  4 + attr_accessor :headers, :params
  5 +
  6 + def request
  7 + req = ActionController::TestRequest.new
  8 + headers.each { |k, v| req.headers[k] = v }
  9 + params.each { |k, v| req.update_param(k, v) }
  10 + req
  11 + end
  12 +end
  13 +
  14 +FactoryGirl.define do
  15 + factory :request, class: RequestInfo do
  16 + headers {}
  17 + params {}
  18 + end
  19 +
  20 + factory :gitlab_webhook_request, parent: :request do
  21 + headers { { 'X-Gitlab-Event' => 'Push Hook' } }
  22 +
  23 + # Excerpt From http://doc.gitlab.com/ee/web_hooks/web_hooks.html
  24 + params { {
  25 + "object_kind" => "push",
  26 + "ref" => "refs/heads/master",
  27 + "project" => {
  28 + "name" => "Kalibro Client",
  29 + "description" => "",
  30 + "git_ssh_url" => "git@example.com:mezuro/kalibro_client.git",
  31 + "git_http_url" => "https://gitlab.com/mezuro/kalibro_client.git",
  32 + "path_with_namespace" => "mezuro/kalibro_client",
  33 + "default_branch" => "master",
  34 + "url" => "git@example.com:mezuro/kalibro_client.git",
  35 + "ssh_url" => "git@example.com:mezuro/kalibro_client.git",
  36 + "http_url" => "https://gitlab.com/mezuro/kalibro_client.git"
  37 + }
  38 + } }
  39 + end
  40 +end
spec/lib/webhooks/base_spec.rb 0 → 100644
@@ -0,0 +1,74 @@ @@ -0,0 +1,74 @@
  1 +require 'rails_helper'
  2 +require 'webhooks'
  3 +
  4 +RSpec.describe Webhooks::Base do
  5 + let(:request) { ActionController::TestRequest.new }
  6 + let(:repository) { FactoryGirl.build(:repository) }
  7 +
  8 + subject { described_class.new(request, repository) }
  9 +
  10 + describe 'initialize' do
  11 + it 'is expected to initialize request and repository' do
  12 + expect(subject.request).to eq(request)
  13 + expect(subject.repository).to eq(repository)
  14 + end
  15 + end
  16 +
  17 + describe 'valid_request?' do
  18 + it 'is expected to not be implemented' do
  19 + expect { subject.valid_request? }.to raise_error(NotImplementedError)
  20 + end
  21 + end
  22 +
  23 + describe 'webhook_address' do
  24 + it 'is expected to not be implemented' do
  25 + expect { subject.webhook_address }.to raise_error(NotImplementedError)
  26 + end
  27 + end
  28 +
  29 + describe 'webhook_branch' do
  30 + it 'is expected to not be implemented' do
  31 + expect { subject.webhook_branch }.to raise_error(NotImplementedError)
  32 + end
  33 + end
  34 +
  35 + describe 'valid_address?' do
  36 + it 'is expected to return true if the repository and webhook addresses match' do
  37 + subject.expects(:webhook_address).returns(repository.address)
  38 + expect(subject.valid_address?).to eq(true)
  39 + end
  40 +
  41 + it "is expected to return false if the repository and webhook addresses don't match" do
  42 + subject.expects(:webhook_address).returns('test')
  43 + expect(subject.valid_address?).to eq(false)
  44 + end
  45 + end
  46 +
  47 + describe 'valid_branch?' do
  48 + it 'is expected to return true if the repository and webhook branches match' do
  49 + subject.expects(:webhook_branch).returns(repository.branch)
  50 + expect(subject.valid_branch?).to eq(true)
  51 + end
  52 +
  53 + it "is expected to return false if the repository and webhook addresses don't match" do
  54 + subject.expects(:webhook_branch).returns('test')
  55 + expect(subject.valid_branch?).to eq(false)
  56 + end
  57 + end
  58 +
  59 + describe 'branch_from_ref' do
  60 + cases = {
  61 + nil => nil,
  62 + 'refs/heads/master' => 'master',
  63 + 'refs/tags/test' => nil,
  64 + 'test' => nil,
  65 + '' => nil
  66 + }
  67 +
  68 + cases.each do |input, output|
  69 + it "is expected to return #{output.inspect} for #{input.inspect}" do
  70 + expect(subject.send(:branch_from_ref, input)).to eq(output)
  71 + end
  72 + end
  73 + end
  74 +end
spec/lib/webhooks/gitlab_spec.rb 0 → 100644
@@ -0,0 +1,60 @@ @@ -0,0 +1,60 @@
  1 +require 'rails_helper'
  2 +require 'webhooks'
  3 +
  4 +RSpec.describe Webhooks::GitLab do
  5 + let(:request) { FactoryGirl.build(:gitlab_webhook_request).request }
  6 + let(:repository) { FactoryGirl.build(:repository) }
  7 + subject { described_class.new(request, repository) }
  8 +
  9 + describe 'valid_request?' do
  10 + context 'with the correct Gitlab header value' do
  11 + it 'is expected to return true' do
  12 + expect(subject.valid_request?).to be(true)
  13 + end
  14 + end
  15 +
  16 + context 'with an incorrect Gitlab header value' do
  17 + let(:request) { FactoryGirl.build(:gitlab_webhook_request, headers: { 'X-Gitlab-Event' => 'Merge Hook' }).request }
  18 + it 'is expected to return false' do
  19 + expect(subject.valid_request?).to be(false)
  20 + end
  21 + end
  22 +
  23 + context 'without a GitLab header' do
  24 + let(:request) { FactoryGirl.build(:gitlab_webhook_request, headers: {}).request }
  25 + it 'is expected to return false' do
  26 + expect(subject.valid_request?).to be(false)
  27 + end
  28 + end
  29 + end
  30 +
  31 + describe 'webhook_address' do
  32 + context 'the git URL is present' do
  33 + it 'is expected to return the URL' do
  34 + expect(subject.webhook_address).to eq(request.params[:project][:git_http_url])
  35 + end
  36 + end
  37 +
  38 + context'the git URL is not present' do
  39 + it 'is expected to return nil' do
  40 + request.expects(:params).returns({})
  41 + expect(subject.webhook_address).to be_nil
  42 + end
  43 + end
  44 + end
  45 +
  46 + describe 'webhook_branch' do
  47 + context 'the git ref is present' do
  48 + it 'is expected to return the branch from the ref' do
  49 + expect(subject.webhook_branch).to eq('master')
  50 + end
  51 + end
  52 +
  53 + context 'the git ref is not present' do
  54 + it 'is expected to return nil' do
  55 + request.expects(:params).returns({})
  56 + expect(subject.webhook_branch).to be_nil
  57 + end
  58 + end
  59 + end
  60 +end