From c6b2ff8a21470cb59e3512b2f27f1c926e17ae6f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 9 Feb 2018 13:09:33 -0600 Subject: [PATCH] Hide CI secret variable values on save See https://gitlab.com/gitlab-org/gitlab-ce/issues/42928 --- .../ci_variable_list/ajax_variable_list.js | 1 + .../ci_variable_list/ci_variable_list.js | 4 ++ .../42929-hide-new-variable-values.yml | 5 ++ .../ajax_variable_list_spec.js | 46 ++++++++++++++----- .../ci_variable_list/ci_variable_list_spec.js | 41 +++++++++++++++-- 5 files changed, 82 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/42929-hide-new-variable-values.yml diff --git a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js index 76f93e5c6bd..b33adff609f 100644 --- a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js +++ b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js @@ -75,6 +75,7 @@ export default class AjaxVariableList { if (res.status === statusCodes.OK && res.data) { this.updateRowsWithPersistedVariables(res.data.variables); + this.variableList.hideValues(); } else if (res.status === statusCodes.BAD_REQUEST) { // Validation failed this.errorBox.innerHTML = generateErrorBoxContent(res.data); 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 d91789c2192..3f2ff4002c2 100644 --- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js +++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js @@ -178,6 +178,10 @@ export default class VariableList { this.$container.find('.js-row-remove-button').attr('disabled', !isEnabled); } + hideValues() { + this.secretValues.updateDom(false); + } + getAllData() { // Ignore the last empty row because we don't want to try persist // a blank variable and run into validation problems. diff --git a/changelogs/unreleased/42929-hide-new-variable-values.yml b/changelogs/unreleased/42929-hide-new-variable-values.yml new file mode 100644 index 00000000000..68decd25b5a --- /dev/null +++ b/changelogs/unreleased/42929-hide-new-variable-values.yml @@ -0,0 +1,5 @@ +--- +title: Hide CI secret variable values after saving +merge_request: 17044 +author: +type: changed diff --git a/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js index 5b9cdceee71..ee457a9c48c 100644 --- a/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js +++ b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js @@ -1,8 +1,10 @@ +import $ from 'jquery'; import MockAdapter from 'axios-mock-adapter'; import axios from '~/lib/utils/axios_utils'; import AjaxFormVariableList from '~/ci_variable_list/ajax_variable_list'; const VARIABLE_PATCH_ENDPOINT = 'http://test.host/frontend-fixtures/builds-project/variables'; +const HIDE_CLASS = 'hide'; describe('AjaxFormVariableList', () => { preloadFixtures('projects/ci_cd_settings.html.raw'); @@ -45,16 +47,16 @@ describe('AjaxFormVariableList', () => { const loadingIcon = saveButton.querySelector('.js-secret-variables-save-loading-icon'); mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(() => { - expect(loadingIcon.classList.contains('hide')).toEqual(false); + expect(loadingIcon.classList.contains(HIDE_CLASS)).toEqual(false); return [200, {}]; }); - expect(loadingIcon.classList.contains('hide')).toEqual(true); + expect(loadingIcon.classList.contains(HIDE_CLASS)).toEqual(true); ajaxVariableList.onSaveClicked() .then(() => { - expect(loadingIcon.classList.contains('hide')).toEqual(true); + expect(loadingIcon.classList.contains(HIDE_CLASS)).toEqual(true); }) .then(done) .catch(done.fail); @@ -78,11 +80,11 @@ describe('AjaxFormVariableList', () => { it('hides any previous error box', (done) => { mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(200); - expect(errorBox.classList.contains('hide')).toEqual(true); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(true); ajaxVariableList.onSaveClicked() .then(() => { - expect(errorBox.classList.contains('hide')).toEqual(true); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(true); }) .then(done) .catch(done.fail); @@ -103,17 +105,39 @@ describe('AjaxFormVariableList', () => { .catch(done.fail); }); + it('hides secret values', (done) => { + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(200, {}); + + const row = container.querySelector('.js-row:first-child'); + const valueInput = row.querySelector('.js-ci-variable-input-value'); + const valuePlaceholder = row.querySelector('.js-secret-value-placeholder'); + + valueInput.value = 'bar'; + $(valueInput).trigger('input'); + + expect(valuePlaceholder.classList.contains(HIDE_CLASS)).toBe(true); + expect(valueInput.classList.contains(HIDE_CLASS)).toBe(false); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(valuePlaceholder.classList.contains(HIDE_CLASS)).toBe(false); + expect(valueInput.classList.contains(HIDE_CLASS)).toBe(true); + }) + .then(done) + .catch(done.fail); + }); + it('shows error box with validation errors', (done) => { const validationError = 'some validation error'; mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(400, [ validationError, ]); - expect(errorBox.classList.contains('hide')).toEqual(true); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(true); ajaxVariableList.onSaveClicked() .then(() => { - expect(errorBox.classList.contains('hide')).toEqual(false); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(false); expect(errorBox.textContent.trim().replace(/\n+\s+/m, ' ')).toEqual(`Validation failed ${validationError}`); }) .then(done) @@ -123,11 +147,11 @@ describe('AjaxFormVariableList', () => { it('shows flash message when request fails', (done) => { mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(500); - expect(errorBox.classList.contains('hide')).toEqual(true); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(true); ajaxVariableList.onSaveClicked() .then(() => { - expect(errorBox.classList.contains('hide')).toEqual(true); + expect(errorBox.classList.contains(HIDE_CLASS)).toEqual(true); }) .then(done) .catch(done.fail); @@ -170,9 +194,9 @@ describe('AjaxFormVariableList', () => { const valueInput = row.querySelector('.js-ci-variable-input-value'); keyInput.value = 'foo'; - keyInput.dispatchEvent(new Event('input')); + $(keyInput).trigger('input'); valueInput.value = 'bar'; - valueInput.dispatchEvent(new Event('input')); + $(valueInput).trigger('input'); expect(idInput.value).toEqual(''); 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 6ab7b50e035..5d41f7ca44f 100644 --- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js +++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js @@ -1,6 +1,8 @@ import VariableList from '~/ci_variable_list/ci_variable_list'; import getSetTimeoutPromise from '../helpers/set_timeout_promise_helper'; +const HIDE_CLASS = 'hide'; + describe('VariableList', () => { preloadFixtures('pipeline_schedules/edit.html.raw'); preloadFixtures('pipeline_schedules/edit_with_variables.html.raw'); @@ -92,14 +94,14 @@ describe('VariableList', () => { const $inputValue = $row.find('.js-ci-variable-input-value'); const $placeholder = $row.find('.js-secret-value-placeholder'); - expect($placeholder.hasClass('hide')).toBe(false); - expect($inputValue.hasClass('hide')).toBe(true); + expect($placeholder.hasClass(HIDE_CLASS)).toBe(false); + expect($inputValue.hasClass(HIDE_CLASS)).toBe(true); // Reveal values $wrapper.find('.js-secret-value-reveal-button').click(); - expect($placeholder.hasClass('hide')).toBe(true); - expect($inputValue.hasClass('hide')).toBe(false); + expect($placeholder.hasClass(HIDE_CLASS)).toBe(true); + expect($inputValue.hasClass(HIDE_CLASS)).toBe(false); }); }); }); @@ -179,4 +181,35 @@ describe('VariableList', () => { expect($wrapper.find('.js-ci-variable-input-key:not([disabled])').length).toBe(3); }); }); + + describe('hideValues', () => { + beforeEach(() => { + loadFixtures('projects/ci_cd_settings.html.raw'); + $wrapper = $('.js-ci-variable-list-section'); + + variableList = new VariableList({ + container: $wrapper, + formField: 'variables', + }); + variableList.init(); + }); + + it('should hide value input and show placeholder stars', () => { + const $row = $wrapper.find('.js-row'); + const $inputValue = $row.find('.js-ci-variable-input-value'); + const $placeholder = $row.find('.js-secret-value-placeholder'); + + $row.find('.js-ci-variable-input-value') + .val('foo') + .trigger('input'); + + expect($placeholder.hasClass(HIDE_CLASS)).toBe(true); + expect($inputValue.hasClass(HIDE_CLASS)).toBe(false); + + variableList.hideValues(); + + expect($placeholder.hasClass(HIDE_CLASS)).toBe(false); + expect($inputValue.hasClass(HIDE_CLASS)).toBe(true); + }); + }); });