diff options
| -rw-r--r-- | app/assets/javascripts/jobs/components/trigger_block.vue | 62 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/builds.scss | 17 | ||||
| -rw-r--r-- | app/serializers/trigger_variable_entity.rb | 3 | ||||
| -rw-r--r-- | changelogs/unreleased/20422-hide-ui-variables-by-default.yml | 6 | ||||
| -rw-r--r-- | doc/ci/triggers/README.md | 2 | ||||
| -rw-r--r-- | doc/ci/triggers/img/trigger_variables.png | bin | 3637 -> 30193 bytes | |||
| -rw-r--r-- | locale/gitlab.pot | 12 | ||||
| -rw-r--r-- | spec/controllers/projects/jobs_controller_spec.rb | 56 | ||||
| -rw-r--r-- | spec/features/projects/jobs_spec.rb | 87 | ||||
| -rw-r--r-- | spec/fixtures/api/schemas/job/trigger.json | 3 | ||||
| -rw-r--r-- | spec/javascripts/jobs/components/trigger_block_spec.js | 28 | ||||
| -rw-r--r-- | spec/serializers/trigger_variable_entity_spec.rb | 49 |
12 files changed, 259 insertions, 66 deletions
diff --git a/app/assets/javascripts/jobs/components/trigger_block.vue b/app/assets/javascripts/jobs/components/trigger_block.vue index 4a9b2903eec..3cd3b743108 100644 --- a/app/assets/javascripts/jobs/components/trigger_block.vue +++ b/app/assets/javascripts/jobs/components/trigger_block.vue @@ -1,6 +1,9 @@ <script> +import { __ } from '~/locale'; import { GlButton } from '@gitlab/ui'; +const HIDDEN_VALUE = '••••••'; + export default { components: { GlButton, @@ -13,17 +16,26 @@ export default { }, data() { return { - areVariablesVisible: false, + showVariableValues: false, }; }, computed: { hasVariables() { return this.trigger.variables && this.trigger.variables.length > 0; }, + getToggleButtonText() { + return this.showVariableValues ? __('Hide values') : __('Reveal values'); + }, + hasValues() { + return this.trigger.variables.some(v => v.value); + }, }, methods: { - revealVariables() { - this.areVariablesVisible = true; + toggleValues() { + this.showVariableValues = !this.showVariableValues; + }, + getDisplayValue(value) { + return this.showVariableValues ? value : HIDDEN_VALUE; }, }, }; @@ -33,31 +45,33 @@ export default { <div class="build-widget block"> <h4 class="title">{{ __('Trigger') }}</h4> - <p v-if="trigger.short_token" class="js-short-token"> + <p + v-if="trigger.short_token" + class="js-short-token" + :class="{ 'append-bottom-0': !hasVariables }" + > <span class="build-light-text"> {{ __('Token') }} </span> {{ trigger.short_token }} </p> - <p v-if="hasVariables"> - <gl-button - v-if="!areVariablesVisible" - type="button" - class="btn btn-default group js-reveal-variables" - @click="revealVariables" - > - {{ __('Reveal Variables') }} - </gl-button> - </p> + <template v-if="hasVariables"> + <p class="trigger-variables-btn-container"> + <span class="build-light-text"> {{ __('Variables:') }} </span> - <dl v-if="areVariablesVisible" class="js-build-variables trigger-build-variables"> - <template v-for="variable in trigger.variables"> - <dt :key="`${variable.key}-variable`" class="js-build-variable trigger-build-variable"> - {{ variable.key }} - </dt> + <gl-button v-if="hasValues" class="group js-reveal-variables" @click="toggleValues"> + {{ getToggleButtonText }} + </gl-button> + </p> - <dd :key="`${variable.key}-value`" class="js-build-value trigger-build-value"> - {{ variable.value }} - </dd> - </template> - </dl> + <table class="js-build-variables trigger-build-variables"> + <tr v-for="(variable, index) in trigger.variables" :key="`${variable.key}-${index}`"> + <td class="js-build-variable trigger-build-variable trigger-variables-table-cell"> + {{ variable.key }} + </td> + <td class="js-build-value trigger-build-value trigger-variables-table-cell"> + {{ getDisplayValue(variable.value) }} + </td> + </tr> + </table> + </template> </div> </template> diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 81cb519883b..57918eafd6f 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -228,9 +228,16 @@ padding: 16px 0; } + .trigger-variables-btn-container { + @extend .d-flex; + justify-content: space-between; + align-items: center; + } + .trigger-build-variables { margin: 0; overflow-x: auto; + width: 100%; -ms-overflow-style: scrollbar; -webkit-overflow-scrolling: touch; } @@ -243,7 +250,15 @@ .trigger-build-value { padding: 2px 4px; color: $black; - background-color: $white-light; + } + + .trigger-variables-table-cell { + font-size: $gl-font-size-small; + line-height: $gl-line-height; + border: 1px solid $theme-gray-200; + padding: $gl-padding-4 6px; + width: 50%; + vertical-align: top; } .badge.badge-pill { diff --git a/app/serializers/trigger_variable_entity.rb b/app/serializers/trigger_variable_entity.rb index 56203113631..4b28db42e76 100644 --- a/app/serializers/trigger_variable_entity.rb +++ b/app/serializers/trigger_variable_entity.rb @@ -3,5 +3,6 @@ class TriggerVariableEntity < Grape::Entity include RequestAwareEntity - expose :key, :value, :public + expose :key, :public + expose :value, if: ->(_, _) { can?(request.current_user, :admin_build, request.project) } end diff --git a/changelogs/unreleased/20422-hide-ui-variables-by-default.yml b/changelogs/unreleased/20422-hide-ui-variables-by-default.yml new file mode 100644 index 00000000000..60285d49718 --- /dev/null +++ b/changelogs/unreleased/20422-hide-ui-variables-by-default.yml @@ -0,0 +1,6 @@ +--- +title: Pipeline trigger variable values are hidden in the UI by default. Maintainers + have the option to reveal them. +merge_request: 23518 +author: jhampton +type: added diff --git a/doc/ci/triggers/README.md b/doc/ci/triggers/README.md index bffb0121603..8ed04e04e53 100644 --- a/doc/ci/triggers/README.md +++ b/doc/ci/triggers/README.md @@ -148,7 +148,7 @@ file. The parameter is of the form: variables[key]=value ``` -This information is also exposed in the UI. +This information is also exposed in the UI. Please note that _values_ are only viewable by Owners and Maintainers.  diff --git a/doc/ci/triggers/img/trigger_variables.png b/doc/ci/triggers/img/trigger_variables.png Binary files differindex 0c2a761cfa9..f862155b47f 100644 --- a/doc/ci/triggers/img/trigger_variables.png +++ b/doc/ci/triggers/img/trigger_variables.png diff --git a/locale/gitlab.pot b/locale/gitlab.pot index acc1a02d1d5..2aeb015ed09 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3411,6 +3411,9 @@ msgid_plural "Hide values" msgstr[0] "" msgstr[1] "" +msgid "Hide values" +msgstr "" + msgid "Hide whitespace changes" msgstr "" @@ -5617,14 +5620,14 @@ msgstr "" msgid "Retry verification" msgstr "" -msgid "Reveal Variables" -msgstr "" - msgid "Reveal value" msgid_plural "Reveal values" msgstr[0] "" msgstr[1] "" +msgid "Reveal values" +msgstr "" + msgid "Revert this commit" msgstr "" @@ -7254,6 +7257,9 @@ msgstr "" msgid "Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use variables for passwords, secret keys, or whatever you want." msgstr "" +msgid "Variables:" +msgstr "" + msgid "Various container registry settings." msgstr "" diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 51a7cc63cef..fca313dafb1 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -401,18 +401,56 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do context 'with variables' do before do create(:ci_pipeline_variable, pipeline: pipeline, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1') + end - get_show(id: job.id, format: :json) + context 'user is a maintainer' do + before do + project.add_maintainer(user) + + get_show(id: job.id, format: :json) + end + + it 'returns a job_detail' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('job/job_details') + end + + it 'exposes trigger information and variables' do + expect(json_response['trigger']['short_token']).to eq 'toke' + expect(json_response['trigger']['variables'].length).to eq 1 + end + + it 'exposes correct variable properties' do + first_variable = json_response['trigger']['variables'].first + + expect(first_variable['key']).to eq "TRIGGER_KEY_1" + expect(first_variable['value']).to eq "TRIGGER_VALUE_1" + expect(first_variable['public']).to eq false + end end - it 'exposes trigger information and variables' do - expect(response).to have_gitlab_http_status(:ok) - expect(response).to match_response_schema('job/job_details') - expect(json_response['trigger']['short_token']).to eq 'toke' - expect(json_response['trigger']['variables'].length).to eq 1 - expect(json_response['trigger']['variables'].first['key']).to eq "TRIGGER_KEY_1" - expect(json_response['trigger']['variables'].first['value']).to eq "TRIGGER_VALUE_1" - expect(json_response['trigger']['variables'].first['public']).to eq false + context 'user is not a mantainer' do + before do + get_show(id: job.id, format: :json) + end + + it 'returns a job_detail' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('job/job_details') + end + + it 'exposes trigger information and variables' do + expect(json_response['trigger']['short_token']).to eq 'toke' + expect(json_response['trigger']['variables'].length).to eq 1 + end + + it 'exposes correct variable properties' do + first_variable = json_response['trigger']['variables'].first + + expect(first_variable['key']).to eq "TRIGGER_KEY_1" + expect(first_variable['value']).to be_nil + expect(first_variable['public']).to eq false + end end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index d7c4abffddd..651c02c7ecc 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -346,44 +346,85 @@ describe 'Jobs', :clean_gitlab_redis_shared_state do describe 'Variables' do let(:trigger_request) { create(:ci_trigger_request) } + let(:job) { create(:ci_build, pipeline: pipeline, trigger_request: trigger_request) } - let(:job) do - create :ci_build, pipeline: pipeline, trigger_request: trigger_request - end + context 'when user is a maintainer' do + shared_examples 'no reveal button variables behavior' do + it 'renders a hidden value with no reveal values button', :js do + expect(page).to have_content('Token') + expect(page).to have_content('Variables') + + expect(page).not_to have_css('.js-reveal-variables') + + expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') + expect(page).to have_selector('.js-build-value', text: '••••••') + end + end + + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + + visit project_job_path(project, job) + end + + it_behaves_like 'no reveal button variables behavior' + end - shared_examples 'expected variables behavior' do - it 'shows variable key and value after click', :js do - expect(page).to have_content('Token') - expect(page).to have_css('.js-reveal-variables') - expect(page).not_to have_css('.js-build-variable') - expect(page).not_to have_css('.js-build-value') + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') - click_button 'Reveal Variables' + visit project_job_path(project, job) + end - expect(page).not_to have_css('.js-reveal-variables') - expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') - expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + it_behaves_like 'no reveal button variables behavior' end end - context 'when variables are stored in trigger_request' do + context 'when user is a maintainer' do before do - trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + project.add_maintainer(user) + end - visit project_job_path(project, job) + shared_examples 'reveal button variables behavior' do + it 'renders a hidden value with a reveal values button', :js do + expect(page).to have_content('Token') + expect(page).to have_content('Variables') + + expect(page).to have_css('.js-reveal-variables') + + expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') + expect(page).to have_selector('.js-build-value', text: '••••••') + end + + it 'reveals values on button click', :js do + click_button 'Reveal values' + + expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') + expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + end end - it_behaves_like 'expected variables behavior' - end + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) - context 'when variables are stored in pipeline_variables' do - before do - create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + visit project_job_path(project, job) + end - visit project_job_path(project, job) + it_behaves_like 'reveal button variables behavior' end - it_behaves_like 'expected variables behavior' + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + + visit project_job_path(project, job) + end + + it_behaves_like 'reveal button variables behavior' + end end end diff --git a/spec/fixtures/api/schemas/job/trigger.json b/spec/fixtures/api/schemas/job/trigger.json index 1c7e9cc7693..807178c662c 100644 --- a/spec/fixtures/api/schemas/job/trigger.json +++ b/spec/fixtures/api/schemas/job/trigger.json @@ -12,12 +12,11 @@ "type": "object", "required": [ "key", - "value", "public" ], "properties": { "key": { "type": "string" }, - "value": { "type": "string" }, + "value": { "type": "string", "optional": true }, "public": { "type": "boolean" } }, "additionalProperties": false diff --git a/spec/javascripts/jobs/components/trigger_block_spec.js b/spec/javascripts/jobs/components/trigger_block_spec.js index 7254851a9e7..448197b82c0 100644 --- a/spec/javascripts/jobs/components/trigger_block_spec.js +++ b/spec/javascripts/jobs/components/trigger_block_spec.js @@ -31,8 +31,8 @@ describe('Trigger block', () => { }); describe('with variables', () => { - describe('reveal variables', () => { - it('reveals variables on click', done => { + describe('hide/reveal variables', () => { + it('should toggle variables on click', done => { vm = mountComponent(Component, { trigger: { short_token: 'bd7e', @@ -48,6 +48,10 @@ describe('Trigger block', () => { vm.$nextTick() .then(() => { expect(vm.$el.querySelector('.js-build-variables')).not.toBeNull(); + expect(vm.$el.querySelector('.js-reveal-variables').textContent.trim()).toEqual( + 'Hide values', + ); + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain( 'UPLOAD_TO_GCS', ); @@ -58,6 +62,26 @@ describe('Trigger block', () => { ); expect(vm.$el.querySelector('.js-build-variables').textContent).toContain('true'); + + vm.$el.querySelector('.js-reveal-variables').click(); + }) + .then(vm.$nextTick) + .then(() => { + expect(vm.$el.querySelector('.js-reveal-variables').textContent.trim()).toEqual( + 'Reveal values', + ); + + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain( + 'UPLOAD_TO_GCS', + ); + + expect(vm.$el.querySelector('.js-build-value').textContent).toContain('••••••'); + + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain( + 'UPLOAD_TO_S3', + ); + + expect(vm.$el.querySelector('.js-build-value').textContent).toContain('••••••'); }) .then(done) .catch(done.fail); diff --git a/spec/serializers/trigger_variable_entity_spec.rb b/spec/serializers/trigger_variable_entity_spec.rb new file mode 100644 index 00000000000..66567c05f52 --- /dev/null +++ b/spec/serializers/trigger_variable_entity_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe TriggerVariableEntity do + let(:project) { create(:project) } + let(:request) { double('request') } + let(:user) { create(:user) } + let(:variable) { { key: 'TEST_KEY', value: 'TEST_VALUE' } } + + subject { described_class.new(variable, request: request).as_json } + + before do + allow(request).to receive(:current_user).and_return(user) + allow(request).to receive(:project).and_return(project) + end + + it 'exposes the variable key' do + expect(subject).to include(:key) + end + + context 'when user has access to the value' do + context 'when user is maintainer' do + before do + project.team.add_maintainer(user) + end + + it 'exposes the variable value' do + expect(subject).to include(:value) + end + end + + context 'when user is owner' do + let(:user) { project.owner } + + it 'exposes the variable value' do + expect(subject).to include(:value) + end + end + end + + context 'when user does not have access to the value' do + before do + project.team.add_developer(user) + end + + it 'does not expose the variable value' do + expect(subject).not_to include(:value) + end + end +end |
