Merge branch 'bvl-ext-auth-configurable-timeout' into 'master'

Configurable timeout for external authorization

Closes #4832

See merge request gitlab-org/gitlab-ee!4971

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: 20180309160427) do
+ActiveRecord::Schema.define(version: 20180314100728) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -184,6 +184,7 @@ ActiveRecord::Schema.define(version: 20180309160427) do
     t.string "external_authorization_service_url"
     t.string "external_authorization_service_default_label"
     t.boolean "pages_domain_verification_enabled", default: true, null: false
+    t.float "external_authorization_service_timeout", default: 0.5, null: false
   end
 
   create_table "approvals", force: :cascade do |t|

doc/api/settings.md

@@ -173,6 +173,7 @@ PUT /application/settings
 | `external_authorization_service_enabled` | boolean | no | Enable using an external authorization service for accessing projects |
 | `external_authorization_service_url` | string | no | URL to which authorization requests will be directed |
 | `external_authorization_service_default_label` | string | no | The default classification label to use when requesting authorization and no classification label has been specified on the project |
+| `external_authorization_service_timeout` | float | no | The timeout to enforce when performing requests to the external authorization service |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/application/settings?signup_enabled=false&default_project_visibility=internal
@@ -213,6 +214,7 @@ Example response:
   "polling_interval_multiplier": 1.0,
   "external_authorization_service_enabled": true,
   "external_authorization_service_url": "https://authorize.me",
-  "external_authorization_service_default_label": "default"
+  "external_authorization_service_default_label": "default",
+  "external_authorization_service_timeout": 0.5
 }
 ```

doc/user/admin_area/settings/external_authorization.md

@@ -38,6 +38,9 @@ admin area under the settings page:
 The available required properties are:
 
 - **Service URL**: The URL to make authorization requests to
+- **External authorization request timeout**: The timeout after which an
+  authorization request is aborted. When a request times out, access is denied
+  to the user.
 - **Default classification label**: The classification label to use when
   requesting authorization if no specific label is defined on the project
 

ee/app/helpers/ee/application_settings_helper.rb

@@ -2,6 +2,17 @@ module EE
   module ApplicationSettingsHelper
     extend ::Gitlab::Utils::Override
 
+    def external_authorization_description
+      _("If enabled, access to projects will be validated on an external service"\
+        " using their classification label.")
+    end
+
+    def external_authorization_timeout_help_text
+      _("Time in seconds GitLab will wait for a response from the external "\
+        "service. When the service does not respond in time, access will be "\
+        "denied.")
+    end
+
     override :visible_attributes
     def visible_attributes
       super + [
@@ -39,7 +50,8 @@ module EE
       [
         :external_authorization_service_enabled,
         :external_authorization_service_url,
-        :external_authorization_service_default_label
+        :external_authorization_service_default_label,
+        :external_authorization_service_timeout
       ]
     end
 

ee/app/models/ee/application_setting.rb

@@ -42,12 +42,17 @@ module EE
 
       validates :external_authorization_service_url,
                 :external_authorization_service_default_label,
+                :external_authorization_service_timeout,
                 presence: true,
                 if: :external_authorization_service_enabled?
 
       validates :external_authorization_service_url,
                 url: true,
                 if: :external_authorization_service_enabled?
+
+      validates :external_authorization_service_timeout,
+                numericality: { greater_than: 0, less_than_or_equal_to: 10 },
+                if: :external_authorization_service_enabled?
     end
 
     module ClassMethods

ee/app/views/admin/application_settings/_external_authorization_service_form.html.haml

@@ -7,15 +7,20 @@
         .checkbox
           = f.label :external_authorization_service_enabled do
             = f.check_box :external_authorization_service_enabled
-            Enable classification control using an external service
+            = _('Enable classification control using an external service')
         %span.help-block
-          If enabled, access to projects will be validated on an external service
-          using their classification label.
+          = external_authorization_description
           = link_to icon('question-circle'), help_page_path('user/admin_area/settings/external_authorization')
     .form-group
       = f.label :external_authorization_service_url, _('Service URL'), class: 'control-label col-sm-2'
       .col-sm-10
         = f.text_field :external_authorization_service_url, class: 'form-control'
+    .form-group
+      = f.label :external_authorization_service_timeout, _('External authorization request timeout'), class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :external_authorization_service_timeout, class: 'form-control', min: 0.001, max: 10, step: 0.001
+        %span.help-block
+          = external_authorization_timeout_help_text
     .form-group
       = f.label :external_authorization_service_default_label, _('Default classification label'), class: 'control-label col-sm-2'
       .col-sm-10

ee/changelogs/unreleased/bvl-ext-auth-configurable-timeout.yml

+---
+title: Timeout for external authorization is now configurable
+merge_request: 4971
+author:
+type: added

ee/db/migrate/20180314100728_add_external_authorization_service_timeout_to_application_settings.rb

+class AddExternalAuthorizationServiceTimeoutToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    # We can use the regular `add_column` with a default since `application_settings`
+    # is a small table.
+    add_column :application_settings,
+               :external_authorization_service_timeout,
+               :float,
+               default: 0.5
+  end
+
+  def down
+    remove_column :application_settings, :external_authorization_service_timeout
+  end
+end

ee/lib/ee/gitlab/external_authorization.rb

@@ -38,6 +38,12 @@ module EE
           .current_application_settings
           .external_authorization_service_url
       end
+
+      def self.timeout
+        ::Gitlab::CurrentSettings
+          .current_application_settings
+          .external_authorization_service_timeout
+      end
     end
   end
 end

ee/lib/ee/gitlab/external_authorization/client.rb

@@ -6,18 +6,18 @@ module EE
           'Content-Type' => 'application/json',
           'Accept' => 'application/json'
         }.freeze
-        TIMEOUT = 0.5
 
         def self.build(user, label)
           new(
             ::EE::Gitlab::ExternalAuthorization.service_url,
+            ::EE::Gitlab::ExternalAuthorization.timeout,
             user,
             label
           )
         end
 
-        def initialize(url, user, label)
-          @url, @user, @label = url, user, label
+        def initialize(url, timeout, user, label)
+          @url, @timeout, @user, @label = url, timeout, user, label
         end
 
         def request_access
@@ -25,9 +25,9 @@ module EE
             @url,
             headers: REQUEST_HEADERS,
             body: body.to_json,
-            connect_timeout: TIMEOUT,
-            read_timeout: TIMEOUT,
-            write_timeout: TIMEOUT
+            connect_timeout: @timeout,
+            read_timeout: @timeout,
+            write_timeout: @timeout
           )
           EE::Gitlab::ExternalAuthorization::Response.new(response)
         rescue Excon::Error => e

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -85,7 +85,8 @@ describe Admin::ApplicationSettingsController do
         {
           external_authorization_service_enabled: true,
           external_authorization_service_url: 'https://custom.service/',
-          external_authorization_service_default_label: 'default'
+          external_authorization_service_default_label: 'default',
+          external_authorization_service_timeout: 3
         }
       end
       let(:feature) { :external_authorization_service }

ee/spec/lib/ee/gitlab/external_authorization/client_spec.rb

@@ -27,6 +27,19 @@ describe EE::Gitlab::ExternalAuthorization::Client do
       client.request_access
     end
 
+    it 'respects the the timeout' do
+      allow(EE::Gitlab::ExternalAuthorization).to receive(:timeout).and_return(3)
+
+      expect(Excon).to receive(:post).with(dummy_url,
+                                           hash_including(
+                                             connect_timeout: 3,
+                                             read_timeout: 3,
+                                             write_timeout: 3
+                                           ))
+
+      client.request_access
+    end
+
     it 'returns an expected response' do
       expect(Excon).to receive(:post)
 

ee/spec/models/application_setting_spec.rb

@@ -34,6 +34,8 @@ describe ApplicationSetting do
       it { is_expected.not_to allow_value('not a URL').for(:external_authorization_service_url) }
       it { is_expected.to allow_value('https://example.com').for(:external_authorization_service_url) }
       it { is_expected.not_to allow_value(nil).for(:external_authorization_service_default_label) }
+      it { is_expected.not_to allow_value(11).for(:external_authorization_service_timeout) }
+      it { is_expected.not_to allow_value(0).for(:external_authorization_service_timeout) }
     end
   end
 

ee/spec/requests/api/settings_spec.rb

@@ -83,7 +83,8 @@ describe API::Settings, 'EE Settings' do
       {
         external_authorization_service_enabled: true,
         external_authorization_service_url: 'https://custom.service/',
-        external_authorization_service_default_label: 'default'
+        external_authorization_service_default_label: 'default',
+        external_authorization_service_timeout: 9.99
       }
     end
     let(:feature) { :external_authorization_service }

locale/gitlab.pot

@@ -8,8 +8,8 @@ msgid ""
 msgstr ""
 "Project-Id-Version: gitlab 1.0.0\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2018-03-13 20:43+0100\n"
-"PO-Revision-Date: 2018-03-13 20:43+0100\n"
+"POT-Creation-Date: 2018-03-14 16:41+0100\n"
+"PO-Revision-Date: 2018-03-14 16:41+0100\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
 "Language: \n"
@@ -1602,6 +1602,9 @@ msgstr ""
 msgid "Enable Auto DevOps"
 msgstr ""
 
+msgid "Enable classification control using an external service"
+msgstr ""
+
 msgid "Environments|An error occurred while fetching the environments."
 msgstr ""
 
@@ -1740,6 +1743,9 @@ msgstr ""
 msgid "External authorization denied access to this project"
 msgstr ""
 
+msgid "External authorization request timeout"
+msgstr ""
+
 msgid "ExternalAuthorizationService|Classification Label"
 msgstr ""
 
@@ -2146,6 +2152,9 @@ msgstr ""
 msgid "Housekeeping successfully started"
 msgstr ""
 
+msgid "If enabled, access to projects will be validated on an external service using their classification label."
+msgstr ""
+
 msgid "If using GitHub, you’ll see pipeline statuses on GitHub for your commits and pull requests. %{more_info_link}"
 msgstr ""
 
@@ -3979,6 +3988,9 @@ msgstr ""
 msgid "Time between merge request creation and merge/close"
 msgstr ""
 
+msgid "Time in seconds GitLab will wait for a response from the external service. When the service does not respond in time, access will be denied."
+msgstr ""
+
 msgid "Time tracking"
 msgstr ""