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

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

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
 

app/helpers/ci_variables_helper.rb

+# 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

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
 

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"

app/views/ci/variables/_content.html.haml

-= _('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.')

app/views/ci/variables/_header.html.haml

+- 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"

app/views/ci/variables/_index.html.haml

 - 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

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')

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
 

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)
 

changelogs/unreleased/feature-option-to-make-variables-protected.yml

+---
+title: Add option to make ci variables protected by default
+merge_request: 22744
+author: Alexis Reigel
+type: added

db/migrate/20181031145139_add_protected_ci_variables_to_application_settings.rb

+# 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

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

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 ""
 

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',

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)