kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch '13784-validate-variables-for-masking' into 'master' 

Simple masking frontend - CE

See merge request gitlab-org/gitlab-ce!26751
This commit is contained in:
Clement Ho 2019-04-03 17:55:56 +00:00
parent ee72dc1b34 02c99ec8ce
commit 3de4399406
17 changed files with 205 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
app/assets/javascripts/ci_variable_list/ci_variable_list.js
View File

@ -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);

1
app/assets/stylesheets/framework/ci_variable_list.scss
View File

@ -66,6 +66,7 @@
}
}
.ci-variable-masked-item,
.ci-variable-protected-item {
flex: 0 1 auto;
display: flex;

2
app/controllers/groups/variables_controller.rb
View File

@ -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!

2
app/controllers/projects/variables_controller.rb
View File

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

8
app/helpers/ci_variables_helper.rb
View File

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

1
app/serializers/group_variable_entity.rb
View File

@ -6,4 +6,5 @@ class GroupVariableEntity < Grape::Entity
expose :value
expose :protected?, as: :protected
expose :masked?, as: :masked
end

1
app/serializers/variable_entity.rb
View File

@ -6,4 +6,5 @@ class VariableEntity < Grape::Entity
expose :value
expose :protected?, as: :protected
expose :masked?, as: :masked
end

2
app/views/ci/variables/_content.html.haml
View File

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

2
app/views/ci/variables/_header.html.haml
View File

@ -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' }

20
app/views/ci/variables/_variable_row.html.haml
View File

@ -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
@ -45,6 +49,20 @@
%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')
.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')

5
changelogs/unreleased/13784-validate-variables-for-masking.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Add control for masking variable values in runner logs
merge_request: 26751
author:
type: added

17
locale/gitlab.pot
View File

@ -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"
@ -8948,6 +8954,9 @@ msgstr ""
msgid "Value"
msgstr ""
msgid "Variables"
msgstr ""
msgid "Various container registry settings."
msgstr ""

2
spec/features/group_variables_spec.rb
View File

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

2
spec/features/project_variables_spec.rb
View File

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

2
spec/fixtures/api/schemas/variable.json vendored
View File

@ -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 }
},

19
spec/javascripts/ci_variable_list/ci_variable_list_spec.js
View File

@ -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);

147
spec/support/features/variable_list_shared_examples.rb
View File

@ -23,25 +23,7 @@ shared_examples 'variable list' do
end
end
it 'adds empty 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('')
end
click_button('Save variables')
wait_for_requests
visit page_path
# 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('')
end
end
it 'adds new protected 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('key_value')
@ -63,6 +45,27 @@ shared_examples 'variable list' do
end
end
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')
expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
end
click_button('Save variables')
wait_for_requests
visit page_path
# 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('key_value')
expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
end
end
context 'defaults to the application setting' do
context 'application setting is true' do
before do
@ -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