Merge branch 'feature/option-to-make-variables-protected' into 'master'

Option to make variables protected by default

Closes #51822

See merge request gitlab-org/gitlab-ce!22744

17 files changed, 138 insertions, 26 deletions

diff --git a/app/assets/javascripts/ci_variable_list/ci_variable_list.js b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
index ee0f7cda189..5b20fa141cd 100644
--- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
@@ -36,7 +36,9 @@ export default class VariableList {
       },
       protected: {
         selector: '.js-ci-variable-input-protected',
-        default: 'false',
+        // use `attr` instead of `data` as we don't want the value to be
+        // converted. we need the value as a string.
+        default: $('.js-ci-variable-input-protected').attr('data-default'),
       },
       environment_scope: {
         // We can't use a `.js-` class here because
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 086bb38ce9a..72731d969a2 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -218,7 +218,8 @@ module ApplicationSettingsHelper
       :version_check_enabled,
       :web_ide_clientside_preview_enabled,
       :diff_max_patch_bytes,
-      :commit_email_hostname
+      :commit_email_hostname,
+      :protected_ci_variables
     ]
   end
 
diff --git a/app/helpers/ci_variables_helper.rb b/app/helpers/ci_variables_helper.rb
new file mode 100644
index 00000000000..e3728804c2a
--- /dev/null
+++ b/app/helpers/ci_variables_helper.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module CiVariablesHelper
+  def ci_variable_protected_by_default?
+    Gitlab::CurrentSettings.current_application_settings.protected_ci_variables
+  end
+
+  def ci_variable_protected?(variable, only_key_value)
+    if variable && !only_key_value
+      variable.protected
+    else
+      ci_variable_protected_by_default?
+    end
+  end
+end
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 4319db42019..73be94eade6 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -302,7 +302,8 @@ class ApplicationSetting < ActiveRecord::Base
       user_show_add_ssh_key_message: true,
       usage_stats_set_by_user_id: nil,
       diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
-      commit_email_hostname: default_commit_email_hostname
+      commit_email_hostname: default_commit_email_hostname,
+      protected_ci_variables: false
     }
   end
 
diff --git a/app/views/admin/application_settings/_ci_cd.html.haml b/app/views/admin/application_settings/_ci_cd.html.haml
index 0d42094fc89..fdaad1cf181 100644
--- a/app/views/admin/application_settings/_ci_cd.html.haml
+++ b/app/views/admin/application_settings/_ci_cd.html.haml
@@ -49,5 +49,12 @@
         Once that time passes, the jobs will be archived and no longer able to be
         retried. Make it empty to never expire jobs. It has to be no less than 1 day,
         for example: <code>15 days</code>, <code>1 month</code>, <code>2 years</code>.
+    .form-group
+      .form-check
+        = f.check_box :protected_ci_variables, class: 'form-check-input'
+        = f.label :protected_ci_variables, class: 'form-check-label' do
+          = s_('AdminSettings|Environment variables are protected by default')
+      .form-text.text-muted
+        = s_('AdminSettings|When creating a new environment variable it will be protected by default.')
 
   = f.submit 'Save changes', class: "btn btn-success"
diff --git a/app/views/ci/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml
index d355e7799df..fa82611d9c1 100644
--- a/app/views/ci/variables/_content.html.haml
+++ b/app/views/ci/variables/_content.html.haml
@@ -1 +1 @@
-= _('Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use variables for passwords, secret keys, or whatever you want.')
+= _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use environment variables for passwords, secret keys, or whatever you want.')
diff --git a/app/views/ci/variables/_header.html.haml b/app/views/ci/variables/_header.html.haml
new file mode 100644
index 00000000000..cb7779e2175
--- /dev/null
+++ b/app/views/ci/variables/_header.html.haml
@@ -0,0 +1,11 @@
+- expanded = local_assigns.fetch(:expanded)
+
+%h4
+  = _('Environment variables')
+  = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'variables'), target: '_blank', rel: 'noopener noreferrer'
+
+%button.btn.btn-default.js-settings-toggle{ type: 'button' }
+  = expanded ? _('Collapse') : _('Expand')
+
+%p.append-bottom-0
+  = render "ci/variables/content"
diff --git a/app/views/ci/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml
index f34305e94fa..dc9ccb6cc39 100644
--- a/app/views/ci/variables/_index.html.haml
+++ b/app/views/ci/variables/_index.html.haml
@@ -1,5 +1,10 @@
 - save_endpoint = local_assigns.fetch(:save_endpoint, nil)
 
+- if ci_variable_protected_by_default?
+  %p.settings-message.text-center
+    - link_start = '<a href="%{url}">'.html_safe % { url: help_page_path('ci/variables/README', anchor: 'protected-variables') }
+    = s_('Environment variables are configured by your administrator to be %{link_start}protected%{link_end} by default').html_safe % { link_start: link_start, link_end: '</a>'.html_safe }
+
 .row
   .col-lg-12.js-ci-variable-list-section{ data: { save_endpoint: save_endpoint } }
     .hide.alert.alert-danger.js-ci-variable-error-box
diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml
index 6ee55836dd2..16a7527c8ce 100644
--- a/app/views/ci/variables/_variable_row.html.haml
+++ b/app/views/ci/variables/_variable_row.html.haml
@@ -5,7 +5,8 @@
 - id = variable&.id
 - key = variable&.key
 - value = variable&.value
-- is_protected = variable && !only_key_value ? variable.protected : false
+- is_protected_default = ci_variable_protected_by_default?
+- is_protected = ci_variable_protected?(variable, only_key_value)
 
 - id_input_name = "#{form_field}[variables_attributes][][id]"
 - destroy_input_name = "#{form_field}[variables_attributes][][_destroy]"
@@ -39,7 +40,8 @@
           %input{ type: "hidden",
             class: 'js-ci-variable-input-protected js-project-feature-toggle-input',
             name: protected_input_name,
-            value: is_protected }
+            value: is_protected,
+            data: { default: is_protected_default.to_s } }
           %span.toggle-icon
             = sprite_icon('status_success_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-checked')
             = sprite_icon('status_failed_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-unchecked')
diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml
index a5e6abdba52..d9332e36ef5 100644
--- a/app/views/groups/settings/ci_cd/show.html.haml
+++ b/app/views/groups/settings/ci_cd/show.html.haml
@@ -5,13 +5,7 @@
 
 %section.settings#ci-variables.no-animate{ class: ('expanded' if expanded) }
   .settings-header
-    %h4
-      = _('Variables')
-      = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'variables'), target: '_blank', rel: 'noopener noreferrer'
-    %button.btn.btn-default.js-settings-toggle{ type: "button" }
-      = expanded ? _('Collapse') : _('Expand')
-    %p.append-bottom-0
-      = render "ci/variables/content"
+    = render 'ci/variables/header', expanded: expanded
   .settings-content
     = render 'ci/variables/index', save_endpoint: group_variables_path
 
diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml
index 98e2829ba43..6966bf96724 100644
--- a/app/views/projects/settings/ci_cd/show.html.haml
+++ b/app/views/projects/settings/ci_cd/show.html.haml
@@ -43,13 +43,7 @@
 
 %section.qa-variables-settings.settings.no-animate{ class: ('expanded' if expanded) }
   .settings-header
-    %h4
-      = _('Variables')
-      = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'variables'), target: '_blank', rel: 'noopener noreferrer'
-    %button.btn.js-settings-toggle{ type: 'button' }
-      = expanded ? _('Collapse') : _('Expand')
-    %p.append-bottom-0
-      = render "ci/variables/content"
+    = render 'ci/variables/header', expanded: expanded
   .settings-content
     = render 'ci/variables/index', save_endpoint: project_variables_path(@project)
 
diff --git a/changelogs/unreleased/feature-option-to-make-variables-protected.yml b/changelogs/unreleased/feature-option-to-make-variables-protected.yml
new file mode 100644
index 00000000000..c99c0481c35
--- /dev/null
+++ b/changelogs/unreleased/feature-option-to-make-variables-protected.yml
@@ -0,0 +1,5 @@
+---
+title: Add option to make ci variables protected by default
+merge_request: 22744
+author: Alexis Reigel
+type: added
diff --git a/db/migrate/20181031145139_add_protected_ci_variables_to_application_settings.rb b/db/migrate/20181031145139_add_protected_ci_variables_to_application_settings.rb
new file mode 100644
index 00000000000..85ee34afe1e
--- /dev/null
+++ b/db/migrate/20181031145139_add_protected_ci_variables_to_application_settings.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddProtectedCiVariablesToApplicationSettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :protected_ci_variables, :boolean, default: false, allow_null: false)
+  end
+
+  def down
+    remove_column(:application_settings, :protected_ci_variables)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 604ed1cd6b0..7aa1e8e055b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -166,6 +166,7 @@ ActiveRecord::Schema.define(version: 20181218192239) do
     t.integer "diff_max_patch_bytes", default: 102400, null: false
     t.integer "archive_builds_in_seconds"
     t.string "commit_email_hostname"
+    t.boolean "protected_ci_variables", default: false, null: false
     t.string "runners_registration_token_encrypted"
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
   end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 7b93a875850..59c377c9ea3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -435,9 +435,15 @@ msgstr ""
 msgid "AdminProjects|Delete project"
 msgstr ""
 
+msgid "AdminSettings|Environment variables are protected by default"
+msgstr ""
+
 msgid "AdminSettings|Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages."
 msgstr ""
 
+msgid "AdminSettings|When creating a new environment variable it will be protected by default."
+msgstr ""
+
 msgid "AdminUsers|Block user"
 msgstr ""
 
@@ -2743,6 +2749,15 @@ msgstr ""
 msgid "Enter the merge request title"
 msgstr ""
 
+msgid "Environment variables"
+msgstr ""
+
+msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use environment variables for passwords, secret keys, or whatever you want."
+msgstr ""
+
+msgid "Environment variables are configured by your administrator to be %{link_start}protected%{link_end} by default"
+msgstr ""
+
 msgid "Environments"
 msgstr ""
 
@@ -7353,12 +7368,6 @@ msgstr ""
 msgid "Users requesting access to"
 msgstr ""
 
-msgid "Variables"
-msgstr ""
-
-msgid "Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use variables for passwords, secret keys, or whatever you want."
-msgstr ""
-
 msgid "Various container registry settings."
 msgstr ""
 
diff --git a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
index 30b15011def..bef59b86d0c 100644
--- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
+++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
@@ -118,6 +118,8 @@ describe('VariableList', () => {
       loadFixtures('projects/ci_cd_settings.html.raw');
       $wrapper = $('.js-ci-variable-list-section');
 
+      $wrapper.find('.js-ci-variable-input-protected').attr('data-default', 'false');
+
       variableList = new VariableList({
         container: $wrapper,
         formField: 'variables',
diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb
index bce1fb01355..0a464d77cb7 100644
--- a/spec/support/features/variable_list_shared_examples.rb
+++ b/spec/support/features/variable_list_shared_examples.rb
@@ -63,6 +63,52 @@ shared_examples 'variable list' do
     end
   end
 
+  context 'defaults to the application setting' do
+    context 'application setting is true' do
+      before do
+        stub_application_setting(protected_ci_variables: true)
+
+        visit page_path
+      end
+
+      it 'defaults to protected' do
+        page.within('.js-ci-variable-list-section .js-row:last-child') do
+          find('.js-ci-variable-input-key').set('key')
+        end
+
+        values = all('.js-ci-variable-input-protected', visible: false).map(&:value)
+
+        expect(values).to eq %w(false true true)
+      end
+
+      it 'shows a message regarding the changed default' do
+        expect(page).to have_content 'Environment variables are configured by your administrator to be protected by default'
+      end
+    end
+
+    context 'application setting is false' do
+      before do
+        stub_application_setting(protected_ci_variables: false)
+
+        visit page_path
+      end
+
+      it 'defaults to unprotected' do
+        page.within('.js-ci-variable-list-section .js-row:last-child') do
+          find('.js-ci-variable-input-key').set('key')
+        end
+
+        values = all('.js-ci-variable-input-protected', visible: false).map(&:value)
+
+        expect(values).to eq %w(false false false)
+      end
+
+      it 'does not show a message regarding the default' do
+        expect(page).not_to have_content 'Environment variables are configured by your administrator to be protected by default'
+      end
+    end
+  end
+
   it 'reveals and hides variables' do
     page.within('.js-ci-variable-list-section') do
       expect(first('.js-ci-variable-input-key').value).to eq(variable.key)