Merge branch '24196-protected-variables' into 'master'

Implementation for protected variables

Closes #24196

See merge request !11688

app/controllers/projects/variables_controller.rb

@@ -42,6 +42,7 @@ class Projects::VariablesController < Projects::ApplicationController
   private
 
   def project_params
-    params.require(:variable).permit([:id, :key, :value, :_destroy])
+    params.require(:variable)
+      .permit([:id, :key, :value, :protected, :_destroy])
   end
 end

app/models/ci/build.rb

@@ -191,7 +191,7 @@ module Ci
       variables += project.deployment_variables if has_environment?
       variables += yaml_variables
       variables += user_variables
-      variables += project.secret_variables
+      variables += project.secret_variables_for(ref).map(&:to_runner_variable)
       variables += trigger_request.user_variables if trigger_request
       variables
     end

app/models/ci/variable.rb

@@ -12,11 +12,16 @@ module Ci
                 message: "can contain only letters, digits and '_'." }
 
     scope :order_key_asc, -> { reorder(key: :asc) }
+    scope :unprotected, -> { where(protected: false) }
 
     attr_encrypted :value,
        mode: :per_attribute_iv_and_salt,
        insecure_mode: true,
        key: Gitlab::Application.secrets.db_key_base,
        algorithm: 'aes-256-cbc'
+
+    def to_runner_variable
+      { key: key, value: value, public: false }
+    end
   end
 end

app/models/project.rb

@@ -1245,12 +1245,19 @@ class Project < ActiveRecord::Base
     variables
   end
 
-  def secret_variables
-    variables.map do |variable|
-      { key: variable.key, value: variable.value, public: false }
+  def secret_variables_for(ref)
+    if protected_for?(ref)
+      variables
+    else
+      variables.unprotected
     end
   end
 
+  def protected_for?(ref)
+    ProtectedBranch.protected?(self, ref) ||
+      ProtectedTag.protected?(self, ref)
+  end
+
   def deployment_variables
     return [] unless deployment_service
 

app/views/projects/pipelines_settings/_show.html.haml

@@ -42,7 +42,7 @@
           = f.label :build_timeout_in_minutes, 'Timeout', class: 'label-light'
           = f.number_field :build_timeout_in_minutes, class: 'form-control', min: '0'
           %p.help-block
-            Per job in minutes. If a job passes this threshold, it will be marked as failed.
+            Per job in minutes. If a job passes this threshold, it will be marked as failed
             = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'timeout'), target: '_blank'
 
         %hr

app/views/projects/variables/_content.html.haml

@@ -1,7 +1,8 @@
 %h4.prepend-top-0
-  Secret Variables
+  Secret variables
+  = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank'
 %p
-  These variables will be set to environment by the runner.
+  These variables will be set to environment by the runner, and could be protected by exposing only to protected branches or tags.
 %p
   So you can use them for passwords, secret keys or whatever you want.
 %p

app/views/projects/variables/_form.html.haml

@@ -7,4 +7,13 @@
   .form-group
     = f.label :value, "Value", class: "label-light"
     = f.text_area :value, class: "form-control", placeholder: "PROJECT_VARIABLE"
+  .form-group
+    .checkbox
+      = f.label :protected do
+        = f.check_box :protected
+        %strong Protected
+      .help-block
+        This variable will be passed only to pipelines running on protected branches and tags
+        = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'protected-secret-variables'), target: '_blank'
+
   = f.submit btn_text, class: "btn btn-save"

app/views/projects/variables/_table.html.haml

@@ -1,12 +1,14 @@
 .table-responsive.variables-table
   %table.table
     %colgroup
+      %col
       %col
       %col
       %col{ width: 100 }
     %thead
       %th Key
       %th Value
+      %th Protected
       %th
     %tbody
       - @project.variables.order_key_asc.each do |variable|
@@ -14,6 +16,7 @@
           %tr
             %td.variable-key= variable.key
             %td.variable-value{ "data-value" => variable.value }******
+            %td.variable-protected= Gitlab::Utils.boolean_to_yes_no(variable.protected)
             %td.variable-menu
               = link_to namespace_project_variable_path(@project.namespace, @project, variable), class: "btn btn-transparent btn-variable-edit" do
                 %span.sr-only

changelogs/unreleased/24196-protected-variables.yml

@@ -0,0 +1,5 @@
+---
+title: Add protected variables which would only be passed to protected branches or
+  protected tags
+merge_request: 11688
+author:

db/migrate/20170524161101_add_protected_to_ci_variables.rb

@@ -0,0 +1,15 @@
+class AddProtectedToCiVariables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:ci_variables, :protected, :boolean, default: false)
+  end
+
+  def down
+    remove_column(:ci_variables, :protected)
+  end
+end

db/schema.rb

@@ -356,6 +356,7 @@ ActiveRecord::Schema.define(version: 20170525174156) do
     t.string "encrypted_value_salt"
     t.string "encrypted_value_iv"
     t.integer "project_id", null: false
+    t.boolean "protected", default: false, null: false
   end
 
   add_index "ci_variables", ["project_id"], name: "index_ci_variables_on_project_id", using: :btree
@@ -1492,4 +1493,4 @@ ActiveRecord::Schema.define(version: 20170525174156) do
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
   add_foreign_key "web_hook_logs", "web_hooks", on_delete: :cascade
-end
+end

doc/api/build_variables.md

@@ -61,11 +61,12 @@ Create a new build variable.
 POST /projects/:id/variables
 ```
 
-| Attribute | Type    | required | Description           |
-|-----------|---------|----------|-----------------------|
-| `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user   |
-| `key`     | string  | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
-| `value`   | string  | yes      | The `value` of a variable |
+| Attribute   | Type    | required | Description           |
+|-------------|---------|----------|-----------------------|
+| `id`        | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user   |
+| `key`       | string  | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
+| `value`     | string  | yes      | The `value` of a variable |
+| `protected` | boolean | no       | Whether the variable is protected |
 
 ```
 curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/variables" --form "key=NEW_VARIABLE" --form "value=new value"
@@ -74,7 +75,8 @@ curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitl
 ```json
 {
     "key": "NEW_VARIABLE",
-    "value": "new value"
+    "value": "new value",
+    "protected": false
 }
 ```
 
@@ -86,11 +88,12 @@ Update a project's build variable.
 PUT /projects/:id/variables/:key
 ```
 
-| Attribute | Type    | required | Description             |
-|-----------|---------|----------|-------------------------|
-| `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user     |
-| `key`     | string  | yes      | The `key` of a variable   |
-| `value`   | string  | yes      | The `value` of a variable |
+| Attribute   | Type    | required | Description             |
+|-------------|---------|----------|-------------------------|
+| `id`        | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user     |
+| `key`       | string  | yes      | The `key` of a variable   |
+| `value`     | string  | yes      | The `value` of a variable |
+| `protected` | boolean | no       | Whether the variable is protected |
 
 ```
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/variables/NEW_VARIABLE" --form "value=updated value"
@@ -99,7 +102,8 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitla
 ```json
 {
     "key": "NEW_VARIABLE",
-    "value": "updated value"
+    "value": "updated value",
+    "protected": true
 }
 ```
 

doc/ci/variables/README.md

@@ -10,7 +10,7 @@ The variables can be overwritten and they take precedence over each other in
 this order:
 
 1. [Trigger variables][triggers] (take precedence over all)
-1. [Secret variables](#secret-variables)
+1. [Secret variables](#secret-variables) or [protected secret variables](#protected-secret-variables)
 1. YAML-defined [job-level variables](../yaml/README.md#job-variables)
 1. YAML-defined [global variables](../yaml/README.md#variables)
 1. [Deployment variables](#deployment-variables)
@@ -153,9 +153,25 @@ storing things like passwords, secret keys and credentials.
 
 Secret variables can be added by going to your project's
 **Settings ➔ Pipelines**, then finding the section called
-**Secret Variables**.
+**Secret variables**.
 
-Once you set them, they will be available for all subsequent jobs.
+Once you set them, they will be available for all subsequent pipelines.
+
+## Protected secret variables
+
+>**Notes:**
+This feature requires GitLab 9.3 or higher.
+
+Secret variables could be protected. Whenever a secret variable is
+protected, it would only be securely passed to pipelines running on the
+[protected branches] or [protected tags]. The other pipelines would not get any
+protected variables.
+
+Protected variables can be added by going to your project's
+**Settings ➔ Pipelines**, then finding the section called
+**Secret variables**, and check *Protected*.
+
+Once you set them, they will be available for all subsequent pipelines.
 
 ## Deployment variables
 
@@ -385,3 +401,5 @@ export CI_REGISTRY_PASSWORD="longalfanumstring"
 [runner]: https://docs.gitlab.com/runner/
 [triggered]: ../triggers/README.md
 [triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger
+[protected branches]: ../../user/project/protected_branches.md
+[protected tags]: ../../user/project/protected_tags.md

lib/api/entities.rb

@@ -675,6 +675,7 @@ module API
 
     class Variable < Grape::Entity
       expose :key, :value
+      expose :protected?, as: :protected
     end
 
     class Pipeline < PipelineBasic

lib/api/variables.rb

@@ -42,6 +42,7 @@ module API
       params do
         requires :key, type: String, desc: 'The key of the variable'
         requires :value, type: String, desc: 'The value of the variable'
+        optional :protected, type: String, desc: 'Whether the variable is protected'
       end
       post ':id/variables' do
         variable = user_project.variables.create(declared(params, include_parent_namespaces: false).to_h)
@@ -59,13 +60,14 @@ module API
       params do
         optional :key, type: String, desc: 'The key of the variable'
         optional :value, type: String, desc: 'The value of the variable'
+        optional :protected, type: String, desc: 'Whether the variable is protected'
       end
       put ':id/variables/:key' do
         variable = user_project.variables.find_by(key: params[:key])
 
         return not_found!('Variable') unless variable
 
-        if variable.update(value: params[:value])
+        if variable.update(declared_params(include_missing: false).except(:key))
           present variable, with: Entities::Variable
         else
           render_validation_error!(variable)

lib/gitlab/utils.rb

@@ -21,5 +21,13 @@ module Gitlab
 
       nil
     end
+
+    def boolean_to_yes_no(bool)
+      if bool
+        'Yes'
+      else
+        'No'
+      end
+    end
   end
 end

spec/factories/ci/variables.rb

@@ -3,6 +3,10 @@ FactoryGirl.define do
     sequence(:key) { |n| "VARIABLE_#{n}" }
     value 'VARIABLE_VALUE'
 
+    trait(:protected) do
+      protected true
+    end
+
     project factory: :empty_project
   end
 end

spec/features/variables_spec.rb

@@ -19,7 +19,7 @@ describe 'Project variables', js: true do
     end
   end
 
-  it 'adds new variable' do
+  it 'adds new secret variable' do
     fill_in('variable_key', with: 'key')
     fill_in('variable_value', with: 'key value')
     click_button('Add new variable')
@@ -27,6 +27,7 @@ describe 'Project variables', js: true do
     expect(page).to have_content('Variables were successfully updated.')
     page.within('.variables-table') do
       expect(page).to have_content('key')
+      expect(page).to have_content('No')
     end
   end
 
@@ -41,6 +42,19 @@ describe 'Project variables', js: true do
     end
   end
 
+  it 'adds new protected variable' do
+    fill_in('variable_key', with: 'key')
+    fill_in('variable_value', with: 'value')
+    check('Protected')
+    click_button('Add new variable')
+
+    expect(page).to have_content('Variables were successfully updated.')
+    page.within('.variables-table') do
+      expect(page).to have_content('key')
+      expect(page).to have_content('Yes')
+    end
+  end
+
   it 'reveals and hides new variable' do
     fill_in('variable_key', with: 'key')
     fill_in('variable_value', with: 'key value')
@@ -85,7 +99,7 @@ describe 'Project variables', js: true do
     click_button('Save variable')
 
     expect(page).to have_content('Variable was successfully updated.')
-    expect(project.variables.first.value).to eq('key value')
+    expect(project.variables(true).first.value).to eq('key value')
   end
 
   it 'edits variable with empty value' do
@@ -98,6 +112,34 @@ describe 'Project variables', js: true do
     click_button('Save variable')
 
     expect(page).to have_content('Variable was successfully updated.')
-    expect(project.variables.first.value).to eq('')
+    expect(project.variables(true).first.value).to eq('')
+  end
+
+  it 'edits variable to be protected' do
+    page.within('.variables-table') do
+      find('.btn-variable-edit').click
+    end
+
+    expect(page).to have_content('Update variable')
+    check('Protected')
+    click_button('Save variable')
+
+    expect(page).to have_content('Variable was successfully updated.')
+    expect(project.variables(true).first).to be_protected
+  end
+
+  it 'edits variable to be unprotected' do
+    project.variables.first.update(protected: true)
+
+    page.within('.variables-table') do
+      find('.btn-variable-edit').click
+    end
+
+    expect(page).to have_content('Update variable')
+    uncheck('Protected')
+    click_button('Save variable')
+
+    expect(page).to have_content('Variable was successfully updated.')
+    expect(project.variables(true).first).not_to be_protected
   end
 end

spec/lib/gitlab/utils_spec.rb

@@ -1,5 +1,7 @@
+require 'spec_helper'
+
 describe Gitlab::Utils, lib: true do
-  delegate :to_boolean, to: :described_class
+  delegate :to_boolean, :boolean_to_yes_no, to: :described_class
 
   describe '.to_boolean' do
     it 'accepts booleans' do
@@ -30,4 +32,11 @@ describe Gitlab::Utils, lib: true do
       expect(to_boolean(nil)).to be_nil
     end
   end
+
+  describe '.boolean_to_yes_no' do
+    it 'converts booleans to Yes or No' do
+      expect(boolean_to_yes_no(true)).to eq('Yes')
+      expect(boolean_to_yes_no(false)).to eq('No')
+    end
+  end
 end

spec/models/ci/build_spec.rb

@@ -1215,16 +1215,49 @@ describe Ci::Build, :models do
       it { is_expected.to include(tag_variable) }
     end
 
-    context 'when secure variable is defined' do
-      let(:secure_variable) do
+    context 'when secret variable is defined' do
+      let(:secret_variable) do
         { key: 'SECRET_KEY', value: 'secret_value', public: false }
       end
 
       before do
-        build.project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value')
+        create(:ci_variable,
+               secret_variable.slice(:key, :value).merge(project: project))
       end
 
-      it { is_expected.to include(secure_variable) }
+      it { is_expected.to include(secret_variable) }
+    end
+
+    context 'when protected variable is defined' do
+      let(:protected_variable) do
+        { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
+      end
+
+      before do
+        create(:ci_variable,
+               :protected,
+               protected_variable.slice(:key, :value).merge(project: project))
+      end
+
+      context 'when the branch is protected' do
+        before do
+          create(:protected_branch, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
+      end
+
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
+      end
+
+      context 'when the ref is not protected' do
+        it { is_expected.not_to include(protected_variable) }
+      end
     end
 
     context 'when build is for triggers' do
@@ -1346,15 +1379,30 @@ describe Ci::Build, :models do
     end
 
     context 'returns variables in valid order' do
+      let(:build_pre_var) { { key: 'build', value: 'value' } }
+      let(:project_pre_var) { { key: 'project', value: 'value' } }
+      let(:pipeline_pre_var) { { key: 'pipeline', value: 'value' } }
+      let(:build_yaml_var) { { key: 'yaml', value: 'value' } }
+
       before do
-        allow(build).to receive(:predefined_variables) { ['predefined'] }
-        allow(project).to receive(:predefined_variables) { ['project'] }
-        allow(pipeline).to receive(:predefined_variables) { ['pipeline'] }
-        allow(build).to receive(:yaml_variables) { ['yaml'] }
-        allow(project).to receive(:secret_variables) { ['secret'] }
+        allow(build).to receive(:predefined_variables) { [build_pre_var] }
+        allow(project).to receive(:predefined_variables) { [project_pre_var] }
+        allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] }
+        allow(build).to receive(:yaml_variables) { [build_yaml_var] }
+
+        allow(project).to receive(:secret_variables_for).with(build.ref) do
+          [create(:ci_variable, key: 'secret', value: 'value')]
+        end
       end
 
-      it { is_expected.to eq(%w[predefined project pipeline yaml secret]) }
+      it do
+        is_expected.to eq(
+          [build_pre_var,
+           project_pre_var,
+           pipeline_pre_var,
+           build_yaml_var,
+           { key: 'secret', value: 'value', public: false }])
+      end
     end
   end
 

spec/models/ci/variable_spec.rb

@@ -12,11 +12,33 @@ describe Ci::Variable, models: true do
   it { is_expected.not_to allow_value('foo bar').for(:key) }
   it { is_expected.not_to allow_value('foo/bar').for(:key) }
 
-  before :each do
-    subject.value = secret_value
+  describe '.unprotected' do
+    subject { described_class.unprotected }
+
+    context 'when variable is protected' do
+      before do
+        create(:ci_variable, :protected)
+      end
+
+      it 'returns nothing' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when variable is not protected' do
+      let(:variable) { create(:ci_variable, protected: false) }
+
+      it 'returns the variable' do
+        is_expected.to contain_exactly(variable)
+      end
+    end
   end
 
   describe '#value' do
+    before do
+      subject.value = secret_value
+    end
+
     it 'stores the encrypted value' do
       expect(subject.encrypted_value).not_to be_nil
     end
@@ -36,4 +58,11 @@ describe Ci::Variable, models: true do
         to raise_error(OpenSSL::Cipher::CipherError, 'bad decrypt')
     end
   end
+
+  describe '#to_runner_variable' do
+    it 'returns a hash for the runner' do
+      expect(subject.to_runner_variable)
+        .to eq(key: subject.key, value: subject.value, public: false)
+    end
+  end
 end

spec/models/project_spec.rb

@@ -1749,6 +1749,90 @@ describe Project, models: true do
     end
   end
 
+  describe '#secret_variables_for' do
+    let(:project) { create(:empty_project) }
+
+    let!(:secret_variable) do
+      create(:ci_variable, value: 'secret', project: project)
+    end
+
+    let!(:protected_variable) do
+      create(:ci_variable, :protected, value: 'protected', project: project)
+    end
+
+    subject { project.secret_variables_for('ref') }
+
+    shared_examples 'ref is protected' do
+      it 'contains all the variables' do
+        is_expected.to contain_exactly(secret_variable, protected_variable)
+      end
+    end
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'contains only the secret variables' do
+        is_expected.to contain_exactly(secret_variable)
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+  end
+
+  describe '#protected_for?' do
+    let(:project) { create(:empty_project) }
+
+    subject { project.protected_for?('ref') }
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+  end
+
   describe '#update_project_statistics' do
     let(:project) { create(:empty_project) }
 

spec/requests/api/variables_spec.rb

@@ -42,6 +42,7 @@ describe API::Variables do
 
         expect(response).to have_http_status(200)
         expect(json_response['value']).to eq(variable.value)
+        expect(json_response['protected']).to eq(variable.protected?)
       end
 
       it 'responds with 404 Not Found if requesting non-existing variable' do
@@ -72,12 +73,13 @@ describe API::Variables do
     context 'authorized user with proper permissions' do
       it 'creates variable' do
         expect do
-          post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2'
+          post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true
         end.to change{project.variables.count}.by(1)
 
         expect(response).to have_http_status(201)
         expect(json_response['key']).to eq('TEST_VARIABLE_2')
         expect(json_response['value']).to eq('VALUE_2')
+        expect(json_response['protected']).to be_truthy
       end
 
       it 'does not allow to duplicate variable key' do
@@ -112,13 +114,14 @@ describe API::Variables do
         initial_variable = project.variables.first
         value_before = initial_variable.value
 
-        put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP'
+        put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP', protected: true
 
         updated_variable = project.variables.first
 
         expect(response).to have_http_status(200)
         expect(value_before).to eq(variable.value)
         expect(updated_variable.value).to eq('VALUE_1_UP')
+        expect(updated_variable).to be_protected
       end
 
       it 'responds with 404 Not Found if requesting non-existing variable' do