diff options
author | mfluharty <mfluharty@gitlab.com> | 2019-03-29 18:46:31 +0300 |
---|---|---|
committer | mfluharty <mfluharty@gitlab.com> | 2019-03-29 21:49:59 +0300 |
commit | 0481d699075ea48c67037088713689b8d66f1983 (patch) | |
tree | 200b5c4c0a4fac3f416ad4b8dbb402caa0a2eae7 | |
parent | ee8f3d5cd806c5558a2153b1a545b371c47c600f (diff) |
Add control for variable value masking
Show masked switch for each variable
When toggled on, the variable value will be masked in runner logs
Show warning message if the switch is on but the value is not maskable
-rw-r--r-- | app/assets/javascripts/ci_variable_list/ci_variable_list.js | 34 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/ci_variable_list.scss | 1 | ||||
-rw-r--r-- | app/controllers/groups/variables_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/variables_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/ci_variables_helper.rb | 8 | ||||
-rw-r--r-- | app/serializers/group_variable_entity.rb | 1 | ||||
-rw-r--r-- | app/serializers/variable_entity.rb | 1 | ||||
-rw-r--r-- | app/views/ci/variables/_content.html.haml | 2 | ||||
-rw-r--r-- | app/views/ci/variables/_header.html.haml | 2 | ||||
-rw-r--r-- | app/views/ci/variables/_variable_row.html.haml | 22 | ||||
-rw-r--r-- | changelogs/unreleased/13784-validate-variables-for-masking.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 17 | ||||
-rw-r--r-- | spec/features/group_variables_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/project_variables_spec.rb | 2 | ||||
-rw-r--r-- | spec/fixtures/api/schemas/variable.json | 2 | ||||
-rw-r--r-- | spec/javascripts/ci_variable_list/ci_variable_list_spec.js | 19 | ||||
-rw-r--r-- | spec/support/features/variable_list_shared_examples.rb | 123 |
17 files changed, 195 insertions, 50 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 5b20fa141cd..da3100b9386 100644 --- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js +++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js @@ -40,6 +40,12 @@ export default class VariableList { // converted. we need the value as a string. default: $('.js-ci-variable-input-protected').attr('data-default'), }, + masked: { + selector: '.js-ci-variable-input-masked', + // 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-masked').attr('data-default'), + }, environment_scope: { // We can't use a `.js-` class here because // gl_dropdown replaces the <input> and doesn't copy over the class @@ -88,13 +94,16 @@ export default class VariableList { } }); - // Always make sure there is an empty last row - this.$container.on('input trigger-change', inputSelector, () => { + this.$container.on('input trigger-change', inputSelector, e => { + // Always make sure there is an empty last row const $lastRow = this.$container.find('.js-row').last(); if (this.checkIfRowTouched($lastRow)) { this.insertRow($lastRow); } + + // If masked, validate value against regex + this.validateMaskability($(e.currentTarget).closest('.js-row')); }); } @@ -171,12 +180,33 @@ export default class VariableList { checkIfRowTouched($row) { return Object.keys(this.inputMap).some(name => { + // Row should not qualify as touched if only switches have been touched + if (['protected', 'masked'].includes(name)) return false; + const entry = this.inputMap[name]; const $el = $row.find(entry.selector); return $el.length && $el.val() !== entry.default; }); } + validateMaskability($row) { + const invalidInputClass = 'gl-field-error-outline'; + + const maskableRegex = /^\w{8,}$/; // Eight or more alphanumeric characters plus underscores + const variableValue = $row.find(this.inputMap.secret_value.selector).val(); + const isValueMaskable = maskableRegex.test(variableValue) || variableValue === ''; + const isMaskedChecked = $row.find(this.inputMap.masked.selector).val() === 'true'; + + // Show a validation error if the user wants to mask an unmaskable variable value + $row + .find(this.inputMap.secret_value.selector) + .toggleClass(invalidInputClass, isMaskedChecked && !isValueMaskable); + $row + .find('.js-secret-value-placeholder') + .toggleClass(invalidInputClass, isMaskedChecked && !isValueMaskable); + $row.find('.masking-validation-error').toggle(isMaskedChecked && !isValueMaskable); + } + toggleEnableRow(isEnabled = true) { this.$container.find(this.inputMap.key.selector).attr('disabled', !isEnabled); this.$container.find('.js-row-remove-button').attr('disabled', !isEnabled); diff --git a/app/assets/stylesheets/framework/ci_variable_list.scss b/app/assets/stylesheets/framework/ci_variable_list.scss index 7207e5119ce..d9b0e4558ad 100644 --- a/app/assets/stylesheets/framework/ci_variable_list.scss +++ b/app/assets/stylesheets/framework/ci_variable_list.scss @@ -66,6 +66,7 @@ } } +.ci-variable-masked-item, .ci-variable-protected-item { flex: 0 1 auto; display: flex; diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb index 4f641de0357..b44e3b0fff4 100644 --- a/app/controllers/groups/variables_controller.rb +++ b/app/controllers/groups/variables_controller.rb @@ -41,7 +41,7 @@ module Groups end def variable_params_attributes - %i[id key secret_value protected _destroy] + %i[id key secret_value protected masked _destroy] end def authorize_admin_build! diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index bb658bfcc19..05a79d59ffd 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -38,6 +38,6 @@ class Projects::VariablesController < Projects::ApplicationController end def variable_params_attributes - %i[id key secret_value protected _destroy] + %i[id key secret_value protected masked _destroy] end end diff --git a/app/helpers/ci_variables_helper.rb b/app/helpers/ci_variables_helper.rb index e3728804c2a..88ce311a1d4 100644 --- a/app/helpers/ci_variables_helper.rb +++ b/app/helpers/ci_variables_helper.rb @@ -12,4 +12,12 @@ module CiVariablesHelper ci_variable_protected_by_default? end end + + def ci_variable_masked?(variable, only_key_value) + if variable && !only_key_value + variable.masked + else + true + end + end end diff --git a/app/serializers/group_variable_entity.rb b/app/serializers/group_variable_entity.rb index 0edab4a3092..19c5fa26f34 100644 --- a/app/serializers/group_variable_entity.rb +++ b/app/serializers/group_variable_entity.rb @@ -6,4 +6,5 @@ class GroupVariableEntity < Grape::Entity expose :value expose :protected?, as: :protected + expose :masked?, as: :masked end diff --git a/app/serializers/variable_entity.rb b/app/serializers/variable_entity.rb index 85cf367fe51..4d48e13cfca 100644 --- a/app/serializers/variable_entity.rb +++ b/app/serializers/variable_entity.rb @@ -6,4 +6,5 @@ class VariableEntity < Grape::Entity expose :value expose :protected?, as: :protected + expose :masked?, as: :masked end diff --git a/app/views/ci/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml index 90c59bec975..d07cbe4589c 100644 --- a/app/views/ci/variables/_content.html.haml +++ b/app/views/ci/variables/_content.html.haml @@ -1,3 +1,3 @@ -= _('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.') += _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want.') = _('You may also add variables that are made available to the running application by prepending the variable key with <code>K8S_SECRET_</code>.').html_safe = link_to _('More information'), help_page_path('ci/variables/README', anchor: 'variables') diff --git a/app/views/ci/variables/_header.html.haml b/app/views/ci/variables/_header.html.haml index cb7779e2175..dbfa0a9e5a1 100644 --- a/app/views/ci/variables/_header.html.haml +++ b/app/views/ci/variables/_header.html.haml @@ -1,7 +1,7 @@ - expanded = local_assigns.fetch(:expanded) %h4 - = _('Environment variables') + = _('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' } diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml index 16a7527c8ce..d4387e68d49 100644 --- a/app/views/ci/variables/_variable_row.html.haml +++ b/app/views/ci/variables/_variable_row.html.haml @@ -7,12 +7,15 @@ - value = variable&.value - is_protected_default = ci_variable_protected_by_default? - is_protected = ci_variable_protected?(variable, only_key_value) +- is_masked_default = true +- is_masked = ci_variable_masked?(variable, only_key_value) - id_input_name = "#{form_field}[variables_attributes][][id]" - destroy_input_name = "#{form_field}[variables_attributes][][_destroy]" - key_input_name = "#{form_field}[variables_attributes][][key]" - value_input_name = "#{form_field}[variables_attributes][][secret_value]" - protected_input_name = "#{form_field}[variables_attributes][][protected]" +- masked_input_name = "#{form_field}[variables_attributes][][masked]" %li.js-row.ci-variable-row{ data: { is_persisted: "#{!id.nil?}" } } .ci-variable-row-body @@ -22,7 +25,7 @@ name: key_input_name, value: key, placeholder: s_('CiVariables|Input variable key') } - .ci-variable-body-item + .ci-variable-body-item.gl-show-field-errors .form-control.js-secret-value-placeholder.qa-ci-variable-input-value{ class: ('hide' unless id) } = '*' * 20 %textarea.js-ci-variable-input-value.js-secret-value.qa-ci-variable-input-value.form-control{ class: ('hide' if id), @@ -30,6 +33,7 @@ name: value_input_name, placeholder: s_('CiVariables|Input variable value') } = value + %p.masking-validation-error.gl-field-error.hide= s_("CiVariables|This variable will not be masked") - unless only_key_value .ci-variable-body-item.ci-variable-protected-item .append-right-default @@ -46,5 +50,21 @@ = 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') = render_if_exists 'ci/variables/environment_scope', form_field: form_field, variable: variable + - unless only_key_value + .ci-variable-body-item.ci-variable-masked-item + .append-right-default + = s_("CiVariable|Masked") + %button{ type: 'button', + class: "js-project-feature-toggle project-feature-toggle #{'is-checked' if is_masked}", + "aria-label": s_("CiVariable|Toggle masked") } + %input{ type: "hidden", + class: 'js-ci-variable-input-masked js-project-feature-toggle-input', + name: masked_input_name, + value: is_masked, + data: { default: is_masked_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') + = render_if_exists 'ci/variables/environment_scope', form_field: form_field, variable: variable %button.js-row-remove-button.ci-variable-row-remove-button{ type: 'button', 'aria-label': s_('CiVariables|Remove variable row') } = icon('minus-circle') diff --git a/changelogs/unreleased/13784-validate-variables-for-masking.yml b/changelogs/unreleased/13784-validate-variables-for-masking.yml new file mode 100644 index 00000000000..e8e97fac3d2 --- /dev/null +++ b/changelogs/unreleased/13784-validate-variables-for-masking.yml @@ -0,0 +1,5 @@ +--- +title: Add control for masking variable values in runner logs +merge_request: 26751 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1e6f04c7815..4a0a30c9def 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1620,6 +1620,9 @@ msgstr "" msgid "CiVariables|Remove variable row" msgstr "" +msgid "CiVariables|This variable will not be masked" +msgstr "" + msgid "CiVariable|* (All environments)" msgstr "" @@ -1629,9 +1632,15 @@ msgstr "" msgid "CiVariable|Error occurred while saving variables" msgstr "" +msgid "CiVariable|Masked" +msgstr "" + msgid "CiVariable|Protected" msgstr "" +msgid "CiVariable|Toggle masked" +msgstr "" + msgid "CiVariable|Toggle protected" msgstr "" @@ -3181,10 +3190,7 @@ 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." +msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. 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" @@ -8951,6 +8957,9 @@ msgstr "" msgid "Value" msgstr "" +msgid "Variables" +msgstr "" + msgid "Various container registry settings." msgstr "" diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 1a53e7c9512..fc5777e8c7c 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Group variables', :js do let(:user) { create(:user) } let(:group) { create(:group) } - let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', group: group) } + let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', masked: true, group: group) } let(:page_path) { group_settings_ci_cd_path(group) } before do diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb index 6bdf5df1036..76abc640077 100644 --- a/spec/features/project_variables_spec.rb +++ b/spec/features/project_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Project variables', :js do let(:user) { create(:user) } let(:project) { create(:project) } - let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value') } + let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value', masked: true) } let(:page_path) { project_settings_ci_cd_path(project) } before do diff --git a/spec/fixtures/api/schemas/variable.json b/spec/fixtures/api/schemas/variable.json index 6f6b044115b..305071a6b3f 100644 --- a/spec/fixtures/api/schemas/variable.json +++ b/spec/fixtures/api/schemas/variable.json @@ -4,12 +4,14 @@ "id", "key", "value", + "masked", "protected" ], "properties": { "id": { "type": "integer" }, "key": { "type": "string" }, "value": { "type": "string" }, + "masked": { "type": "boolean" }, "protected": { "type": "boolean" }, "environment_scope": { "type": "string", "optional": true } }, 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 70f49469300..394e60fc22c 100644 --- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js +++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js @@ -127,20 +127,25 @@ describe('VariableList', () => { variableList.init(); }); - it('should add another row when editing the last rows protected checkbox', done => { + it('should not add another row when editing the last rows protected checkbox', done => { const $row = $wrapper.find('.js-row:last-child'); $row.find('.ci-variable-protected-item .js-project-feature-toggle').click(); getSetTimeoutPromise() .then(() => { - expect($wrapper.find('.js-row').length).toBe(2); + expect($wrapper.find('.js-row').length).toBe(1); + }) + .then(done) + .catch(done.fail); + }); - // Check for the correct default in the new row - const $protectedInput = $wrapper - .find('.js-row:last-child') - .find('.js-ci-variable-input-protected'); + it('should not add another row when editing the last rows masked checkbox', done => { + const $row = $wrapper.find('.js-row:last-child'); + $row.find('.ci-variable-masked-item .js-project-feature-toggle').click(); - expect($protectedInput.val()).toBe('false'); + getSetTimeoutPromise() + .then(() => { + expect($wrapper.find('.js-row').length).toBe(1); }) .then(done) .catch(done.fail); diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 73156d18c1b..693b796fbdc 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -23,10 +23,13 @@ shared_examples 'variable list' do end end - it 'adds empty variable' do + it 'adds a new protected variable' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') - find('.js-ci-variable-input-value').set('') + find('.js-ci-variable-input-value').set('key_value') + find('.ci-variable-protected-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end click_button('Save variables') @@ -37,17 +40,17 @@ shared_examples 'variable list' do # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do expect(find('.js-ci-variable-input-key').value).to eq('key') - expect(find('.js-ci-variable-input-value', visible: false).value).to eq('') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end end - it 'adds new protected variable' do + it 'defaults to masked' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') find('.js-ci-variable-input-value').set('key_value') - find('.ci-variable-protected-item .js-project-feature-toggle').click - expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') end click_button('Save variables') @@ -59,7 +62,7 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do expect(find('.js-ci-variable-input-key').value).to eq('key') expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') - expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') end end @@ -163,27 +166,6 @@ shared_examples 'variable list' do end end - it 'edits variable with empty value' do - page.within('.js-ci-variable-list-section') do - click_button('Reveal value') - - page.within('.js-row:nth-child(1)') do - find('.js-ci-variable-input-key').set('new_key') - find('.js-ci-variable-input-value').set('') - end - - click_button('Save variables') - wait_for_requests - - visit page_path - - page.within('.js-row:nth-child(1)') do - expect(find('.js-ci-variable-input-key').value).to eq('new_key') - expect(find('.js-ci-variable-input-value', visible: false).value).to eq('') - end - end - end - it 'edits variable to be protected' do # Create the unprotected variable page.within('.js-ci-variable-list-section .js-row:last-child') do @@ -251,6 +233,57 @@ shared_examples 'variable list' do end end + it 'edits variable to be unmasked' do + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + + find('.ci-variable-masked-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + end + end + + it 'edits variable to be masked' do + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + + find('.ci-variable-masked-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + + find('.ci-variable-masked-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + end + end + it 'handles multiple edits and deletion in the middle' do page.within('.js-ci-variable-list-section') do # Create 2 variables @@ -297,11 +330,11 @@ shared_examples 'variable list' do it 'shows validation error box about duplicate keys' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('samekey') - find('.js-ci-variable-input-value').set('value1') + find('.js-ci-variable-input-value').set('value123') end page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('samekey') - find('.js-ci-variable-input-value').set('value2') + find('.js-ci-variable-input-value').set('value456') end click_button('Save variables') @@ -314,4 +347,34 @@ shared_examples 'variable list' do expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/) end end + + it 'shows validation error box about empty values' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('empty_value') + find('.js-ci-variable-input-value').set('') + end + + click_button('Save variables') + wait_for_requests + + page.within('.js-ci-variable-list-section') do + expect(all('.js-ci-variable-error-box ul li').count).to eq(1) + expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables value is invalid/) + end + end + + it 'shows validation error box about unmaskable values' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('unmaskable_value') + find('.js-ci-variable-input-value').set('???') + end + + click_button('Save variables') + wait_for_requests + + page.within('.js-ci-variable-list-section') do + expect(all('.js-ci-variable-error-box ul li').count).to eq(1) + expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables value is invalid/) + end + end end |