Skip to content

G
gitlab-ce
Commit 89fdf64c authored by Kamil Trzciński's avatar Kamil Trzciński
Browse files
Options

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

This reverts merge request !25476
parent 1b26f3c5
Show whitespace changes
Inline Side-by-side
Showing with 50 additions and 195 deletions
app/assets/javascripts/ci_variable_list/ci_variable_list.js
View file @ 89fdf64c
......@@ -40,12 +40,6 @@ 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
......@@ -94,16 +88,13 @@ export default class VariableList {
}
});
this.$container.on('input trigger-change', inputSelector, e => {
// Always make sure there is an empty last row
this.$container.on('input trigger-change', inputSelector, () => {
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'));
});
}
......@@ -180,33 +171,12 @@ 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);
......
app/assets/stylesheets/framework/ci_variable_list.scss
View file @ 89fdf64c
......@@ -66,7 +66,6 @@
}
}
.ci-variable-masked-item,
.ci-variable-protected-item {
flex: 0 1 auto;
display: flex;
......
app/controllers/groups/variables_controller.rb
View file @ 89fdf64c
......@@ -41,7 +41,7 @@ module Groups
end
def variable_params_attributes
%i[id key secret_value protected masked _destroy]
%i[id key secret_value protected _destroy]
end
def authorize_admin_build!
......
app/controllers/projects/variables_controller.rb
View file @ 89fdf64c
......@@ -38,6 +38,6 @@ class Projects::VariablesController < Projects::ApplicationController
end
def variable_params_attributes
%i[id key secret_value protected masked _destroy]
%i[id key secret_value protected _destroy]
end
end
app/helpers/ci_variables_helper.rb
View file @ 89fdf64c
......@@ -12,12 +12,4 @@ 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
app/serializers/group_variable_entity.rb
View file @ 89fdf64c
......@@ -6,5 +6,4 @@ class GroupVariableEntity < Grape::Entity
expose :value
expose :protected?, as: :protected
expose :masked?, as: :masked
end
app/serializers/variable_entity.rb
View file @ 89fdf64c
......@@ -6,5 +6,4 @@ class VariableEntity < Grape::Entity
expose :value
expose :protected?, as: :protected
expose :masked?, as: :masked
end
app/views/ci/variables/_content.html.haml
View file @ 89fdf64c
= _('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.')
= _('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.')
= _('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')
app/views/ci/variables/_header.html.haml
View file @ 89fdf64c
- expanded = local_assigns.fetch(:expanded)
%h4
= _('Variables')
= _('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' }
......
app/views/ci/variables/_variable_row.html.haml
View file @ 89fdf64c
......@@ -7,15 +7,12 @@
- 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
......@@ -25,7 +22,7 @@
name: key_input_name,
value: key,
placeholder: s_('CiVariables|Input variable key') }
.ci-variable-body-item.gl-show-field-errors
.ci-variable-body-item
.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),
......@@ -33,7 +30,6 @@
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
......@@ -50,21 +46,5 @@
= 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')
changelogs/unreleased/13784-validate-variables-for-masking.yml deleted 100644 → 0
View file @ 1b26f3c5
---
title: Add control for masking variable values in runner logs
merge_request: 25476
author:
type: added
locale/gitlab.pot
View file @ 89fdf64c
......@@ -1620,9 +1620,6 @@ msgstr ""
msgid "CiVariables|Remove variable row"
msgstr ""
msgid "CiVariables|This variable will not be masked"
msgstr ""
msgid "CiVariable|* (All environments)"
msgstr ""
......@@ -1632,15 +1629,9 @@ 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 ""
......@@ -3190,7 +3181,10 @@ msgstr ""
msgid "Enter the merge request title"
msgstr ""
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."
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"
......@@ -8954,9 +8948,6 @@ msgstr ""
msgid "Value"
msgstr ""
msgid "Variables"
msgstr ""
msgid "Various container registry settings."
msgstr ""
......
spec/features/group_variables_spec.rb
View file @ 89fdf64c
......@@ -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', masked: true, group: group) }
let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', group: group) }
let(:page_path) { group_settings_ci_cd_path(group) }
before do
......
spec/features/project_variables_spec.rb
View file @ 89fdf64c
......@@ -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', masked: true) }
let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value') }
let(:page_path) { project_settings_ci_cd_path(project) }
before do
......
spec/fixtures/api/schemas/variable.json
View file @ 89fdf64c
......@@ -4,14 +4,12 @@
"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 }
},
......
spec/javascripts/ci_variable_list/ci_variable_list_spec.js
View file @ 89fdf64c
......@@ -127,25 +127,20 @@ describe('VariableList', () => {
variableList.init();
});
it('should not add another row when editing the last rows protected checkbox', done => {
it('should 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(1);
})
.then(done)
.catch(done.fail);
});
expect($wrapper.find('.js-row').length).toBe(2);
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();
// Check for the correct default in the new row
const $protectedInput = $wrapper
.find('.js-row:last-child')
.find('.js-ci-variable-input-protected');
getSetTimeoutPromise()
.then(() => {
expect($wrapper.find('.js-row').length).toBe(1);
expect($protectedInput.val()).toBe('false');
})
.then(done)
.catch(done.fail);
......
spec/support/features/variable_list_shared_examples.rb
View file @ 89fdf64c
......@@ -23,13 +23,10 @@ shared_examples 'variable list' do
end
end
it 'adds a new protected variable' do
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('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')
find('.js-ci-variable-input-value').set('')
end
click_button('Save variables')
......@@ -40,17 +37,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('key_value')
expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
expect(find('.js-ci-variable-input-value', visible: false).value).to eq('')
end
end
it 'defaults to masked' do
it 'adds 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')
find('.ci-variable-protected-item .js-project-feature-toggle').click
expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true')
expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
end
click_button('Save variables')
......@@ -62,7 +59,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-masked', visible: false).value).to eq('true')
expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
end
end
......@@ -166,6 +163,27 @@ 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
......@@ -233,57 +251,6 @@ 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
......@@ -330,11 +297,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('value123')
find('.js-ci-variable-input-value').set('value1')
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('value456')
find('.js-ci-variable-input-value').set('value2')
end
click_button('Save variables')
......@@ -347,34 +314,4 @@ 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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7