From 89fdf64c8a3764cf8c646cb4b9eb9b11a3871eae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 27 Mar 2019 21:42:23 +0000
Subject: [PATCH] Revert "Merge branch '13784-validate-variables-for-masking'
into 'master'"
This reverts merge request !25476
---
.../ci_variable_list/ci_variable_list.js | 34 +----
.../framework/ci_variable_list.scss | 1 -
.../groups/variables_controller.rb | 2 +-
.../projects/variables_controller.rb | 2 +-
app/helpers/ci_variables_helper.rb | 8 --
app/serializers/group_variable_entity.rb | 1 -
app/serializers/variable_entity.rb | 1 -
app/views/ci/variables/_content.html.haml | 2 +-
app/views/ci/variables/_header.html.haml | 2 +-
.../ci/variables/_variable_row.html.haml | 22 +---
.../13784-validate-variables-for-masking.yml | 5 -
locale/gitlab.pot | 17 +--
spec/features/group_variables_spec.rb | 2 +-
spec/features/project_variables_spec.rb | 2 +-
spec/fixtures/api/schemas/variable.json | 2 -
.../ci_variable_list/ci_variable_list_spec.js | 19 +--
.../features/variable_list_shared_examples.rb | 123 +++++-------------
17 files changed, 50 insertions(+), 195 deletions(-)
delete mode 100644 changelogs/unreleased/13784-validate-variables-for-masking.yml
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 da3100b9386a6..5b20fa141cdf9 100644
--- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
@@ -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
+ // 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);
diff --git a/app/assets/stylesheets/framework/ci_variable_list.scss b/app/assets/stylesheets/framework/ci_variable_list.scss
index d9b0e4558ad1a..7207e5119ce3f 100644
--- a/app/assets/stylesheets/framework/ci_variable_list.scss
+++ b/app/assets/stylesheets/framework/ci_variable_list.scss
@@ -66,7 +66,6 @@
}
}
-.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 b44e3b0fff42a..4f641de03575c 100644
--- a/app/controllers/groups/variables_controller.rb
+++ b/app/controllers/groups/variables_controller.rb
@@ -41,7 +41,7 @@ def group_variables_params
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!
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 05a79d59ffd58..bb658bfcc19a6 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -38,6 +38,6 @@ def variables_params
end
def variable_params_attributes
- %i[id key secret_value protected masked _destroy]
+ %i[id key secret_value protected _destroy]
end
end
diff --git a/app/helpers/ci_variables_helper.rb b/app/helpers/ci_variables_helper.rb
index 88ce311a1d421..e3728804c2af8 100644
--- a/app/helpers/ci_variables_helper.rb
+++ b/app/helpers/ci_variables_helper.rb
@@ -12,12 +12,4 @@ def ci_variable_protected?(variable, only_key_value)
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 19c5fa26f340e..0edab4a309257 100644
--- a/app/serializers/group_variable_entity.rb
+++ b/app/serializers/group_variable_entity.rb
@@ -6,5 +6,4 @@ 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 4d48e13cfca4b..85cf367fe5103 100644
--- a/app/serializers/variable_entity.rb
+++ b/app/serializers/variable_entity.rb
@@ -6,5 +6,4 @@ 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 d07cbe4589c78..90c59bec9756b 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. 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')
diff --git a/app/views/ci/variables/_header.html.haml b/app/views/ci/variables/_header.html.haml
index dbfa0a9e5a1a2..cb7779e217592 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
- = _('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' }
diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml
index d4387e68d490a..16a7527c8ce35 100644
--- a/app/views/ci/variables/_variable_row.html.haml
+++ b/app/views/ci/variables/_variable_row.html.haml
@@ -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')
diff --git a/changelogs/unreleased/13784-validate-variables-for-masking.yml b/changelogs/unreleased/13784-validate-variables-for-masking.yml
deleted file mode 100644
index d178bddeb1d4b..0000000000000
--- a/changelogs/unreleased/13784-validate-variables-for-masking.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Add control for masking variable values in runner logs
-merge_request: 25476
-author:
-type: added
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 0d6a2448c6151..c7755c5c7e2b2 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -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 ""
diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb
index fc5777e8c7c3d..1a53e7c951264 100644
--- a/spec/features/group_variables_spec.rb
+++ b/spec/features/group_variables_spec.rb
@@ -3,7 +3,7 @@
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
diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb
index 76abc640077bd..6bdf5df103656 100644
--- a/spec/features/project_variables_spec.rb
+++ b/spec/features/project_variables_spec.rb
@@ -3,7 +3,7 @@
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
diff --git a/spec/fixtures/api/schemas/variable.json b/spec/fixtures/api/schemas/variable.json
index 305071a6b3f33..6f6b044115bbd 100644
--- a/spec/fixtures/api/schemas/variable.json
+++ b/spec/fixtures/api/schemas/variable.json
@@ -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 }
},
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 394e60fc22cc0..70f494693006e 100644
--- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
+++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js
@@ -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);
diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb
index 693b796fbdc9b..73156d18c1be1 100644
--- a/spec/support/features/variable_list_shared_examples.rb
+++ b/spec/support/features/variable_list_shared_examples.rb
@@ -23,13 +23,10 @@
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 @@
# 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 @@
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 @@
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 @@
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 @@
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 @@
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
--
GitLab