Merge branch 'enable_ssl_verification_web_hook' into 'master'

Enable SSL verification for Webhooks

https://dev.gitlab.org/gitlab/gitlabhq/issues/2121

See merge request !1135

CHANGELOG

@@ -25,6 +25,7 @@ v 7.14.1
   - Fix "Reload with full diff" URL button in compare branch view (Stan Hu)
   - Only include base URL in OmniAuth full_host parameter (Stan Hu)
   - Fix Error 500 in API when accessing a group that has an avatar (Stan Hu)
+  - Ability to enable SSL verification for Webhooks
 
 v 7.14.0
   - Fix bug where non-project members of the target project could set labels on new merge requests.

app/controllers/admin/hooks_controller.rb

@@ -39,6 +39,6 @@ class Admin::HooksController < Admin::ApplicationController
   end
 
   def hook_params
-    params.require(:hook).permit(:url)
+    params.require(:hook).permit(:url, :enable_ssl_verification)
   end
 end

app/controllers/projects/hooks_controller.rb

@@ -53,6 +53,7 @@ class Projects::HooksController < Projects::ApplicationController
   end
 
   def hook_params
-    params.require(:hook).permit(:url, :push_events, :issues_events, :merge_requests_events, :tag_push_events, :note_events)
+    params.require(:hook).permit(:url, :push_events, :issues_events,
+      :merge_requests_events, :tag_push_events, :note_events, :enable_ssl_verification)
   end
 end

app/controllers/projects/services_controller.rb

@@ -8,7 +8,7 @@ class Projects::ServicesController < Projects::ApplicationController
                     :push_events, :issues_events, :merge_requests_events, :tag_push_events,
                     :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url,
                     :notify, :color,
-                    :server_host, :server_port, :default_irc_uri]
+                    :server_host, :server_port, :default_irc_uri, :enable_ssl_verification]
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]

app/models/hooks/web_hook.rb

@@ -25,6 +25,7 @@ class WebHook < ActiveRecord::Base
   default_value_for :note_events, false
   default_value_for :merge_requests_events, false
   default_value_for :tag_push_events, false
+  default_value_for :enable_ssl_verification, false
 
   # HTTParty timeout
   default_timeout Gitlab.config.gitlab.webhook_timeout
@@ -41,7 +42,7 @@ class WebHook < ActiveRecord::Base
                      "Content-Type" => "application/json",
                      "X-Gitlab-Event" => hook_name.singularize.titleize
                    },
-                   verify: false)
+                   verify: enable_ssl_verification)
     else
       post_url = url.gsub("#{parsed_url.userinfo}@", "")
       auth = {
@@ -54,7 +55,7 @@ class WebHook < ActiveRecord::Base
                      "Content-Type" => "application/json",
                      "X-Gitlab-Event" => hook_name.singularize.titleize
                    },
-                   verify: false,
+                   verify: enable_ssl_verification,
                    basic_auth: auth)
     end
   rescue SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e

app/models/project_services/buildkite_service.rb

@@ -23,7 +23,7 @@ require "addressable/uri"
 class BuildkiteService < CiService
   ENDPOINT = "https://buildkite.com"
 
-  prop_accessor :project_url, :token
+  prop_accessor :project_url, :token, :enable_ssl_verification
 
   validates :project_url, presence: true, if: :activated?
   validates :token, presence: true, if: :activated?
@@ -37,6 +37,7 @@ class BuildkiteService < CiService
   def compose_service_hook
     hook = service_hook || build_service_hook
     hook.url = webhook_url
+    hook.enable_ssl_verification = enable_ssl_verification
     hook.save
   end
 
@@ -96,7 +97,11 @@ class BuildkiteService < CiService
 
       { type: 'text',
         name: 'project_url',
-        placeholder: "#{ENDPOINT}/example/project" }
+        placeholder: "#{ENDPOINT}/example/project" },
+      
+      { type: 'checkbox',
+        name: 'enable_ssl_verification',
+        title: "Enable SSL verification" }
     ]
   end
 

app/models/project_services/gitlab_ci_service.rb

@@ -21,7 +21,7 @@
 class GitlabCiService < CiService
   API_PREFIX = "api/v1"
 
-  prop_accessor :project_url, :token
+  prop_accessor :project_url, :token, :enable_ssl_verification
   validates :project_url,
     presence: true,
     format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" }, if: :activated?
@@ -34,6 +34,7 @@ class GitlabCiService < CiService
   def compose_service_hook
     hook = service_hook || build_service_hook
     hook.url = [project_url, "/build", "?token=#{token}"].join("")
+    hook.enable_ssl_verification = enable_ssl_verification
     hook.save
   end
 
@@ -136,7 +137,8 @@ class GitlabCiService < CiService
   def fields
     [
       { type: 'text', name: 'token', placeholder: 'GitLab CI project specific token' },
-      { type: 'text', name: 'project_url', placeholder: 'http://ci.gitlabhq.com/projects/3' }
+      { type: 'text', name: 'project_url', placeholder: 'http://ci.gitlabhq.com/projects/3' },
+      { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" }
     ]
   end
 

app/views/admin/hooks/index.html.haml

@@ -18,6 +18,13 @@
     = f.label :url, "URL:", class: 'control-label'
     .col-sm-10
       = f.text_field :url, class: "form-control"
+  .form-group
+    = f.label :enable_ssl_verification, "SSL verification", class: 'control-label checkbox'
+    .col-sm-10
+      .checkbox
+        = f.label :enable_ssl_verification do
+          = f.check_box :enable_ssl_verification
+          %strong Enable SSL verification
   .form-actions
     = f.submit "Add System Hook", class: "btn btn-create"
 %hr
@@ -32,6 +39,7 @@
           .list-item-name
             = link_to admin_hook_path(hook) do
               %strong= hook.url
+            %p SSL Verification: #{hook.enable_ssl_verification ? "enabled" : "disabled"}
 
           .pull-right
             = link_to 'Test Hook', admin_hook_test_path(hook), class: "btn btn-sm"

app/views/projects/hooks/index.html.haml

@@ -55,6 +55,13 @@
             %strong Merge Request events
           %p.light
             This url will be triggered when a merge request is created
+  .form-group
+    = f.label :enable_ssl_verification, "SSL verification", class: 'control-label checkbox'
+    .col-sm-10
+      .checkbox
+        = f.label :enable_ssl_verification do
+          = f.check_box :enable_ssl_verification
+          %strong Enable SSL verification
   .form-actions
     = f.submit "Add Web Hook", class: "btn btn-create"
 
@@ -74,3 +81,4 @@
             - %w(push_events tag_push_events issues_events note_events merge_requests_events).each do |trigger|
               - if hook.send(trigger)
                 %span.label.label-gray= trigger.titleize
+            SSL Verification: #{hook.enable_ssl_verification ? "enabled" : "disabled"}

db/migrate/20150824002011_add_enable_ssl_verification.rb

+class AddEnableSslVerification < ActiveRecord::Migration
+  def change
+    add_column :web_hooks, :enable_ssl_verification, :boolean, default: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150818213832) do
+ActiveRecord::Schema.define(version: 20150824002011) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -573,6 +573,7 @@ ActiveRecord::Schema.define(version: 20150818213832) do
     t.boolean  "merge_requests_events",   default: false,         null: false
     t.boolean  "tag_push_events",         default: false
     t.boolean  "note_events",             default: false,         null: false
+    t.boolean  "enable_ssl_verification", default: false
   end
 
   add_index "web_hooks", ["created_at", "id"], name: "index_web_hooks_on_created_at_and_id", using: :btree

features/admin/hooks.feature

+@admin
+Feature: Admin Hooks
+  Background:
+    Given I sign in as an admin
+
+  Scenario: On Admin Hooks
+    Given I visit admin hooks page
+    Then I submit the form with enabled SSL verification
+    And I see new hook with enabled SSL verification
\ No newline at end of file

features/project/hooks.feature

@@ -13,6 +13,11 @@ Feature: Project Hooks
     When I submit new hook
     Then I should see newly created hook
 
+  Scenario: I add new hook with SSL verification enabled
+    Given I visit project hooks page
+    When I submit new hook with SSL verification enabled
+    Then I should see newly created hook with SSL verification enabled
+
   Scenario: I test hook
     Given project has hook
     And I visit project hooks page

features/steps/admin/hooks.rb

+class Spinach::Features::AdminHooks < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedPaths
+  include SharedAdmin
+
+  step "I submit the form with enabled SSL verification" do
+    fill_in 'hook_url', with: 'http://google.com'
+    check "Enable SSL verification"
+    click_on "Add System Hook"
+  end
+
+  step "I see new hook with enabled SSL verification" do
+    expect(page).to have_content "SSL Verification: enabled"
+  end
+end

features/steps/project/hooks.rb

@@ -28,11 +28,24 @@ class Spinach::Features::ProjectHooks < Spinach::FeatureSteps
     expect { click_button "Add Web Hook" }.to change(ProjectHook, :count).by(1)
   end
 
+  step 'I submit new hook with SSL verification enabled' do
+    @url = FFaker::Internet.uri("http")
+    fill_in "hook_url", with: @url
+    check "hook_enable_ssl_verification"
+    expect { click_button "Add Web Hook" }.to change(ProjectHook, :count).by(1)
+  end
+
   step 'I should see newly created hook' do
     expect(current_path).to eq namespace_project_hooks_path(current_project.namespace, current_project)
     expect(page).to have_content(@url)
   end
 
+  step 'I should see newly created hook with SSL verification enabled' do
+    expect(current_path).to eq namespace_project_hooks_path(current_project.namespace, current_project)
+    expect(page).to have_content(@url)
+    expect(page).to have_content("SSL Verification: enabled")
+  end
+
   step 'I click test hook button' do
     stub_request(:post, @hook.url).to_return(status: 200)
     click_link 'Test Hook'