Merge branch '20422-hide-ui-variables-by-default' into 'master'

Resolve "Hide variables in UI by default"

Closes #20422

See merge request gitlab-org/gitlab-ce!23518

app/assets/javascripts/jobs/components/trigger_block.vue

 <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') }}
+    <template v-if="hasVariables">
+      <p class="trigger-variables-btn-container">
+        <span class="build-light-text"> {{ __('Variables:') }} </span>
+
+        <gl-button v-if="hasValues" class="group js-reveal-variables" @click="toggleValues">
+          {{ getToggleButtonText }}
         </gl-button>
       </p>
 
-    <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">
+      <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 }}
-        </dt>
-
-        <dd :key="`${variable.key}-value`" class="js-build-value trigger-build-value">
-          {{ variable.value }}
-        </dd>
+          </td>
+          <td class="js-build-value trigger-build-value trigger-variables-table-cell">
+            {{ getDisplayValue(variable.value) }}
+          </td>
+        </tr>
+      </table>
     </template>
-    </dl>
   </div>
 </template>

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 {

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

changelogs/unreleased/20422-hide-ui-variables-by-default.yml

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

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.
 
 ![Job variables in UI](img/trigger_variables.png)
 

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

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

spec/features/projects/jobs_spec.rb

@@ -346,21 +346,61 @@ 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
+      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
+
+        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
 
-      shared_examples 'expected variables behavior' do
-        it 'shows variable key and value after click', :js do
+          it_behaves_like 'no reveal button variables behavior'
+        end
+      end
+
+      context 'when user is a maintainer' do
+        before do
+          project.add_maintainer(user)
+        end
+
+        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).not_to have_css('.js-build-variable')
-          expect(page).not_to have_css('.js-build-value')
 
-          click_button '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).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')
           end
@@ -373,7 +413,7 @@ describe 'Jobs', :clean_gitlab_redis_shared_state do
             visit project_job_path(project, job)
           end
 
-        it_behaves_like 'expected variables behavior'
+          it_behaves_like 'reveal button variables behavior'
         end
 
         context 'when variables are stored in pipeline_variables' do
@@ -383,7 +423,8 @@ describe 'Jobs', :clean_gitlab_redis_shared_state do
             visit project_job_path(project, job)
           end
 
-        it_behaves_like 'expected variables behavior'
+          it_behaves_like 'reveal button variables behavior'
+        end
       end
     end
 

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

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

spec/serializers/trigger_variable_entity_spec.rb

+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