Basic BE change

Fix static-snalysis

Move the precedence of group secure variable before project secure variable. Allow project_id to be null.

Separate Ci::VariableProject and Ci::VariableGroup

Add the forgotton files

Add migration file to update type of ci_variables

Fix form_for fpr VariableProject

Fix test

Change the table structure according to the yorik advice

Add necessary migration files. Remove unnecessary migration spec.

Revert safe_model_attributes.yml

Fix models

Fix spec

Avoid self.variable. Use becomes for correct routing.

Use unique index on group_id and key

Add null: false for t.timestamps

Fix schema version

Rename VariableProject and VariableGroup to ProjectVariable and GroupVariable

Rename the rest of them

Add the rest of files

Basic BE change

Fix static-snalysis

Move the precedence of group secure variable before project secure variable. Allow project_id to be null.

Separate Ci::VariableProject and Ci::VariableGroup

Add the forgotton files

Add migration file to update type of ci_variables

Fix form_for fpr VariableProject

Fix test

Change the table structure according to the yorik advice

Add necessary migration files. Remove unnecessary migration spec.

Revert safe_model_attributes.yml

Fix models

Fix spec

Avoid self.variable. Use becomes for correct routing.

Use unique index on group_id and key

Add null: false for t.timestamps

Fix schema version

Rename VariableProject and VariableGroup to ProjectVariable and GroupVariable

Rename the rest of them

Add the rest of files

Implement CURD

Rename codes related to VariableGroup and VariableProject FE part

Remove unneccesary changes

Make Fe code up-to-date

Add protected flag to migration file

Protected group variables essential package

Update schema

Improve doc

Fix logic and spec for models

Fix logic and spec for controllers

 Fix logic and spec for views(pre feature)

Add feature spec

Fixed bugs. placeholder. reveal button. doc.

Add changelog

Remove unnecessary comment

godfat nice catches

Improve secret_variables_for arctecture

Fix spec

Fix StaticAnlysys & path_regex spec

Revert "Improve secret_variables_for arctecture"

This reverts commit c3216ca212322ecf6ca534cb12ce75811a4e77f1.

Use ayufan suggestion for secret_variables_for

Use find instead of find_by

Fix spec message for variable is invalid

Fix spec remove variable.group_id = group.id

godffat spec nitpicks

Use include Gitlab::Routing.url_helpers for presenter spec

app/assets/javascripts/dispatcher.js

@@ -396,6 +396,7 @@ import PerformanceBar from './performance_bar';
           initSettingsPanels();
           break;
         case 'projects:settings:ci_cd:show':
+        case 'groups:settings:ci_cd:show':
           new gl.ProjectVariables();
           break;
         case 'ci:lints:create':

app/controllers/groups/settings/ci_cd_controller.rb

@@ -0,0 +1,24 @@
+module Groups
+  module Settings
+    class CiCdController < Groups::ApplicationController
+      before_action :authorize_admin_pipeline!
+
+      def show
+        define_secret_variables
+      end
+
+      private
+
+      def define_secret_variables
+        @variable = Ci::GroupVariable.new(group: group)
+          .present(current_user: current_user)
+        @variables = group.variables.order_key_asc
+          .map { |variable| variable.present(current_user: current_user) }
+      end
+
+      def authorize_admin_pipeline!
+        return render_404 unless can?(current_user, :admin_pipeline, group)
+      end
+    end
+  end
+end

app/controllers/groups/variables_controller.rb

@@ -0,0 +1,57 @@
+module Groups
+  class VariablesController < Groups::ApplicationController
+    before_action :variable, only: [:show, :update, :destroy]
+    before_action :authorize_admin_build!
+
+    def index
+      redirect_to group_settings_ci_cd_path(group)
+    end
+
+    def show
+    end
+
+    def update
+      if variable.update(group_params)
+        redirect_to group_variables_path(group),
+                    notice: 'Variable was successfully updated.'
+      else
+        render "show"
+      end
+    end
+
+    def create
+      new_variable = Ci::GroupVariable.new(group_params)
+
+      if new_variable.valid? && group.variables << new_variable
+        redirect_to group_settings_ci_cd_path(group),
+                    notice: 'Variables were successfully updated.'
+      else
+        @variable = new_variable.present(current_user: current_user)
+        render "show"
+      end
+    end
+
+    def destroy
+      variable.destroy
+
+      redirect_to group_settings_ci_cd_path(group),
+                  status: 302,
+                  notice: 'Variable was successfully removed.'
+    end
+
+    private
+
+    def authorize_admin_build!
+      return render_404 unless can?(current_user, :admin_build, group)
+    end
+
+    def group_params
+      params.require(:variable)
+        .permit([:key, :value, :protected])
+    end
+
+    def variable
+      @variable ||= group.variables.find(params[:id]).present(current_user: current_user)
+    end
+  end
+end

app/controllers/projects/settings/ci_cd_controller.rb

@@ -21,7 +21,10 @@ module Projects
       end
 
       def define_secret_variables
-        @variable = Ci::Variable.new
+        @variable = Ci::Variable.new(project: project)
+          .present(current_user: current_user)
+        @variables = project.variables.order_key_asc
+          .map { |variable| variable.present(current_user: current_user) }
       end
 
       def define_triggers_variables

app/controllers/projects/variables_controller.rb

@@ -1,53 +1,50 @@
 class Projects::VariablesController < Projects::ApplicationController
+  before_action :variable, only: [:show, :update, :destroy]
   before_action :authorize_admin_build!
 
   layout 'project_settings'
 
   def index
-    redirect_to project_settings_ci_cd_path(@project)
+    redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project)
   end
 
   def show
-    @variable = @project.variables.find(params[:id])
   end
 
   def update
-    @variable = @project.variables.find(params[:id])
-
-    if @variable.update_attributes(variable_params)
-      redirect_to project_variables_path(project), notice: 'Variable was successfully updated.'
+    if @variable.update(project_params)
+      redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variable was successfully updated.'
     else
-      render action: "show"
+      render "show"
     end
   end
 
   def create
-    @variable = @project.variables.new(variable_params)
+    @variable = Ci::Variable.new(project_params)
 
-    if @variable.save
-      flash[:notice] = 'Variables were successfully updated.'
-      redirect_to project_settings_ci_cd_path(project)
+    if @variable.valid? && @project.variables << @variable
+      redirect_to namespace_project_settings_ci_cd_path(project.namespace, project), notice: 'Variables were successfully updated.'
     else
       render "show"
     end
   end
 
   def destroy
-    @key = @project.variables.find(params[:id])
-    @key.destroy
+    variable.destroy
 
-    redirect_to project_settings_ci_cd_path(project),
+    redirect_to namespace_project_settings_ci_cd_path(project.namespace, project),
                 status: 302,
                 notice: 'Variable was successfully removed.'
   end
 
   private
 
-  def variable_params
-    params.require(:variable).permit(*variable_params_attributes)
+  def project_params
+    params.require(:variable)
+      .permit([:id, :key, :value, :protected, :_destroy])
   end
 
-  def variable_params_attributes
-    %i[id key value protected _destroy]
+  def variable
+    @variable ||= project.variables.find(params[:id]).present(current_user: current_user)
   end
 end

app/models/ci/build.rb

@@ -200,6 +200,7 @@ module Ci
       variables += project.deployment_variables if has_environment?
       variables += yaml_variables
       variables += user_variables
+      variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group
       variables += secret_variables(environment: environment)
       variables += trigger_request.user_variables if trigger_request
       variables += persisted_environment_variables if environment

app/models/ci/group_variable.rb

@@ -0,0 +1,13 @@
+module Ci
+  class GroupVariable < ActiveRecord::Base
+    extend Ci::Model
+    include HasVariable
+    include Presentable
+
+    belongs_to :group
+
+    validates :key, uniqueness: { scope: :group_id }
+
+    scope :unprotected, -> { where(protected: false) }
+  end
+end

app/models/ci/variable.rb

@@ -2,6 +2,7 @@ module Ci
   class Variable < ActiveRecord::Base
     extend Ci::Model
     include HasVariable
+    include Presentable
 
     belongs_to :project
 

app/models/group.rb

@@ -22,6 +22,7 @@ class Group < Namespace
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   has_many :labels, class_name: 'GroupLabel'
+  has_many :variables, class_name: 'Ci::GroupVariable'
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects
@@ -248,6 +249,13 @@ class Group < Namespace
     }
   end
 
+  def secret_variables_for(ref, project)
+    variables = []
+    variables += parent.secret_variables_for(ref, project) if has_parent?
+    variables += project.protected_for?(ref) ? self.variables : self.variables.unprotected
+    variables
+  end
+
   protected
 
   def update_two_factor_requirement

app/policies/group_policy.rb

@@ -31,6 +31,8 @@ class GroupPolicy < BasePolicy
   rule { master }.policy do
     enable :create_projects
     enable :admin_milestones
+    enable :admin_pipeline
+    enable :admin_build
   end
 
   rule { owner }.policy do

app/presenters/ci/group_variable_presenter.rb

@@ -0,0 +1,25 @@
+module Ci
+  class GroupVariablePresenter < Gitlab::View::Presenter::Delegated
+    presents :variable
+
+    def placeholder
+      'GROUP_VARIABLE'
+    end
+
+    def form_path
+      if variable.persisted?
+        group_variable_path(group, variable)
+      else
+        group_variables_path(group)
+      end
+    end
+
+    def edit_path
+      group_variable_path(group, variable)
+    end
+
+    def delete_path
+      group_variable_path(group, variable)
+    end
+  end
+end

app/presenters/ci/variable_presenter.rb

@@ -0,0 +1,25 @@
+module Ci
+  class VariablePresenter < Gitlab::View::Presenter::Delegated
+    presents :variable
+
+    def placeholder
+      'PROJECT_VARIABLE'
+    end
+
+    def form_path
+      if variable.persisted?
+        namespace_project_variable_path(project.namespace, project, variable)
+      else
+        namespace_project_variables_path(project.namespace, project)
+      end
+    end
+
+    def edit_path
+      namespace_project_variable_path(project.namespace, project, variable)
+    end
+
+    def delete_path
+      namespace_project_variable_path(project.namespace, project, variable)
+    end
+  end
+end

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

@@ -1,12 +1,12 @@
-= form_for [@project.namespace.becomes(Namespace), @project, @variable] do |f|
+= form_for @variable, as: :variable, url: @variable.form_path do |f|
   = form_errors(@variable)
 
   .form-group
     = f.label :key, "Key", class: "label-light"
-    = f.text_field :key, class: "form-control", placeholder: "PROJECT_VARIABLE", required: true
+    = f.text_field :key, class: "form-control", placeholder: @variable.placeholder, required: true
   .form-group
     = f.label :value, "Value", class: "label-light"
-    = f.text_area :value, class: "form-control", placeholder: "PROJECT_VARIABLE"
+    = f.text_area :value, class: "form-control", placeholder: @variable.placeholder
   .form-group
     .checkbox
       = f.label :protected do

app/views/ci/variables/_index.html.haml

@@ -1,16 +1,16 @@
 .row.prepend-top-default.append-bottom-default
   .col-lg-4
-    = render "projects/variables/content"
+    = render "ci/variables/content"
   .col-lg-8
     %h5.prepend-top-0
       Add a variable
-    = render "projects/variables/form", btn_text: "Add new variable"
+    = render "ci/variables/form", btn_text: "Add new variable"
     %hr
     %h5.prepend-top-0
-      Your variables (#{@project.variables.size})
-    - if @project.variables.empty?
+      Your variables (#{@variables.size})
+    - if @variables.empty?
       %p.settings-message.text-center.append-bottom-0
         No variables found, add one with the form above.
     - else
-      = render "projects/variables/table"
+      = render "ci/variables/table"
       %button.btn.btn-info.js-btn-toggle-reveal-values{ "data-status" => 'hidden' } Reveal Values

app/views/ci/variables/_show.html.haml

@@ -0,0 +1,9 @@
+- page_title "Variables"
+
+.row.prepend-top-default.append-bottom-default
+  .col-lg-3
+    = render "ci/variables/content"
+  .col-lg-9
+    %h5.prepend-top-0
+      Update variable
+    = render "ci/variables/form", btn_text: "Save variable"

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

@@ -11,18 +11,18 @@
       %th Protected
       %th
     %tbody
-      - @project.variables.order_key_asc.each do |variable|
+      - @variables.each do |variable|
         - if variable.id?
           %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 project_variable_path(@project, variable), class: "btn btn-transparent btn-variable-edit" do
+              = link_to variable.edit_path, class: "btn btn-transparent btn-variable-edit" do
                 %span.sr-only
                   Update
                 = icon("pencil")
-              = link_to project_variable_path(@project, variable), class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do
+              = link_to variable.delete_path, class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do
                 %span.sr-only
                   Remove
                 = icon("trash")

app/views/groups/_settings_head.html.haml

@@ -12,3 +12,8 @@
           = link_to projects_group_path(@group), title: 'Projects' do
             %span
               Projects
+
+        = nav_link(controller: :ci_cd) do
+          = link_to group_settings_ci_cd_path(@group), title: 'Pipelines' do
+            %span
+              Pipelines

app/views/groups/settings/ci_cd/show.html.haml

@@ -0,0 +1,4 @@
+- page_title "Pipelines"
+= render "groups/settings_head"
+
+= render 'ci/variables/index'

app/views/groups/variables/show.html.haml

@@ -0,0 +1 @@
+= render 'ci/variables/show'

app/views/projects/settings/ci_cd/show.html.haml

@@ -3,6 +3,6 @@
 = render "projects/settings/head"
 
 = render 'projects/runners/index'
-= render 'projects/variables/index'
+= render 'ci/variables/index'
 = render 'projects/triggers/index'
 = render 'projects/pipelines_settings/show'

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

@@ -1,9 +1 @@
-- page_title "Variables"
-
-.row.prepend-top-default.append-bottom-default
-  .col-lg-3
-    = render "content"
-  .col-lg-9
-    %h5.prepend-top-0
-      Update variable
-    = render "form", btn_text: "Save variable"
+= render 'ci/variables/show'

changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml

@@ -0,0 +1,4 @@
+---
+title: Add Group secret variables
+merge_request: 12582
+author:

config/routes/group.rb

@@ -23,6 +23,12 @@ scope(path: 'groups/*group_id',
   resources :labels, except: [:show] do
     post :toggle_subscription, on: :member
   end
+
+  namespace :settings do
+    resource :ci_cd, only: [:show], controller: 'ci_cd'
+  end
+
+  resources :variables, only: [:index, :show, :update, :create, :destroy]
 end
 
 scope(path: 'groups/*id',

db/migrate/20170525130346_create_group_variables_table.rb

@@ -0,0 +1,23 @@
+class CreateGroupVariablesTable < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    create_table :ci_group_variables do |t|
+      t.string :key, null: false
+      t.text :value
+      t.text :encrypted_value
+      t.string :encrypted_value_salt
+      t.string :encrypted_value_iv
+      t.integer :group_id, null: false
+      t.boolean :protected, default: false, null: false
+
+      t.timestamps_with_timezone null: false
+    end
+
+    add_index :ci_group_variables, [:group_id, :key], unique: true
+  end
+
+  def down
+    drop_table :ci_group_variables
+  end
+end

db/migrate/20170525130758_add_foreign_key_to_group_variables.rb

@@ -0,0 +1,15 @@
+class AddForeignKeyToGroupVariables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :ci_group_variables, :namespaces, column: :group_id
+  end
+
+  def down
+    remove_foreign_key :ci_group_variables, column: :group_id
+  end
+end

db/schema.rb

@@ -253,6 +253,20 @@ ActiveRecord::Schema.define(version: 20170703102400) do
   add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree
   add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree
 
+  create_table "ci_group_variables", force: :cascade do |t|
+    t.string "key", null: false
+    t.text "value"
+    t.text "encrypted_value"
+    t.string "encrypted_value_salt"
+    t.string "encrypted_value_iv"
+    t.integer "group_id", null: false
+    t.boolean "protected", default: false, null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
+  add_index "ci_group_variables", ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree
+
   create_table "ci_pipeline_schedules", force: :cascade do |t|
     t.string "description"
     t.string "ref"
@@ -1550,6 +1564,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do
   add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify
   add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade
   add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade
+  add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify
   add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify

doc/ci/variables/README.md

@@ -10,7 +10,8 @@ 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) or [protected secret variables](#protected-secret-variables)
+1. [Project-level secret variables](#project-level-secret-variables)
+1. [Group-level secret variables](#group-level-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)
@@ -138,7 +139,7 @@ script:
   - 'eval $LS_CMD'  # will execute 'ls -al $TMP_DIR'
 ```
 
-## Secret variables
+## Project-level secret variables
 
 >**Notes:**
 - This feature requires GitLab Runner 0.4.0 or higher.
@@ -158,7 +159,8 @@ Secret variables can be added by going to your project's
 **Settings ➔ Pipelines**, then finding the section called
 **Secret variables**.
 
-Once you set them, they will be available for all subsequent pipelines.
+Once you set them, they will be available for all subsequent pipelines. You can also
+[protect your variables](#protected-secret-variables).
 
 ### Protected secret variables
 
@@ -176,6 +178,19 @@ Protected variables can be added by going to your project's
 
 Once you set them, they will be available for all subsequent pipelines.
 
+## Group-level secret variables
+
+>**Notes:**
+This feature requires GitLab 9.4 or higher.
+
+You can also define variables per Group. The essential functionality is exactly the
+same with [project-level secret variables](#project-level-secret-variables). You
+can also [protect your variables](#protected-secret-variables).
+
+Secret variables can be added by going to your group's
+**Settings ➔ Pipelines**, then finding the section called
+**Secret variables**.
+
 ## Deployment variables
 
 >**Note:**

lib/gitlab/path_regex.rb

@@ -129,6 +129,8 @@ module Gitlab
       pipeline_quota
       projects
       subgroups
+      settings
+      variables
     ].freeze
 
     ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES

spec/controllers/groups/settings/ci_cd_controller_spec.rb

@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+describe Groups::Settings::CiCdController do
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+
+  before do
+    group.add_master(user)
+    sign_in(user)
+  end
+
+  describe 'GET #show' do
+    it 'renders show with 200 status code' do
+      get :show, group_id: group
+
+      expect(response).to have_http_status(200)
+      expect(response).to render_template(:show)
+    end
+  end
+end

spec/controllers/groups/variables_controller_spec.rb

@@ -0,0 +1,56 @@
+require 'spec_helper'
+
+describe Groups::VariablesController do
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+    group.add_master(user)
+  end
+
+  describe 'POST #create' do
+    context 'variable is valid' do
+      it 'shows a success flash message' do
+        post :create, group_id: group, variable: { key: "one", value: "two" }
+
+        expect(flash[:notice]).to include 'Variables were successfully updated.'
+        expect(response).to redirect_to(group_settings_ci_cd_path(group))
+      end
+    end
+
+    context 'variable is invalid' do
+      it 'renders show' do
+        post :create, group_id: group, variable: { key: "..one", value: "two" }
+
+        expect(response).to render_template("groups/variables/show")
+      end
+    end
+  end
+
+  describe 'POST #update' do
+    let(:variable) { create(:ci_group_variable) }
+
+    context 'updating a variable with valid characters' do
+      before do
+        group.variables << variable
+      end
+
+      it 'shows a success flash message' do
+        post :update, group_id: group,
+                      id: variable.id, variable: { key: variable.key, value: 'two' }
+
+        expect(flash[:notice]).to include 'Variable was successfully updated.'
+        expect(response).to redirect_to(group_variables_path(group))
+      end
+
+      it 'renders the action #show if the variable key is invalid' do
+        post :update, group_id: group,
+                      id: variable.id, variable: { key: '?', value: variable.value }
+
+        expect(response).to have_http_status(200)
+        expect(response).to render_template :show
+      end
+    end
+  end
+end

spec/controllers/projects/variables_controller_spec.rb

@@ -21,7 +21,7 @@ describe Projects::VariablesController do
     end
 
     context 'variable is invalid' do
-      it 'shows an alert flash message' do
+      it 'renders show' do
         post :create, namespace_id: project.namespace.to_param, project_id: project,
                       variable: { key: "..one", value: "two" }
 
@@ -35,7 +35,6 @@ describe Projects::VariablesController do
 
     context 'updating a variable with valid characters' do
       before do
-        variable.project_id = project.id
         project.variables << variable
       end
 

spec/factories/ci/group_variables.rb

@@ -0,0 +1,12 @@
+FactoryGirl.define do
+  factory :ci_group_variable, class: Ci::GroupVariable do
+    sequence(:key) { |n| "VARIABLE_#{n}" }
+    value 'VARIABLE_VALUE'
+
+    trait(:protected) do
+      protected true
+    end
+
+    group factory: :group
+  end
+end

spec/features/group_variables_spec.rb

@@ -0,0 +1,78 @@
+require 'spec_helper'
+
+feature 'Group variables', js: true do
+  let(:user) { create(:user) }
+  let(:group) { create(:group) }
+
+  background do
+    group.add_master(user)
+    gitlab_sign_in(user)
+  end
+
+  context 'when user creates a new variable' do
+    background do
+      visit group_settings_ci_cd_path(group)
+      fill_in 'variable_key', with: 'AAA'
+      fill_in 'variable_value', with: 'AAA123'
+      find(:css, "#variable_protected").set(true)
+      click_on 'Add new variable'
+    end
+
+    scenario 'user sees the created variable' do
+      page.within('.variables-table') do
+        expect(find(".variable-key")).to have_content('AAA')
+        expect(find(".variable-value")).to have_content('******')
+        expect(find(".variable-protected")).to have_content('Yes')
+      end
+      click_on 'Reveal Values'
+      page.within('.variables-table') do
+        expect(find(".variable-value")).to have_content('AAA123')
+      end
+    end
+  end
+
+  context 'when user edits a variable' do
+    background do
+      create(:ci_group_variable, key: 'AAA', value: 'AAA123', protected: true,
+                                 group: group)
+
+      visit group_settings_ci_cd_path(group)
+
+      page.within('.variable-menu') do
+        click_on 'Update'
+      end
+
+      fill_in 'variable_key', with: 'BBB'
+      fill_in 'variable_value', with: 'BBB123'
+      find(:css, "#variable_protected").set(false)
+      click_on 'Save variable'
+    end
+
+    scenario 'user sees the updated variable' do
+      page.within('.variables-table') do
+        expect(find(".variable-key")).to have_content('BBB')
+        expect(find(".variable-value")).to have_content('******')
+        expect(find(".variable-protected")).to have_content('No')
+      end
+    end
+  end
+
+  context 'when user deletes a variable' do
+    background do
+      create(:ci_group_variable, key: 'BBB', value: 'BBB123', protected: false,
+                                 group: group)
+
+      visit group_settings_ci_cd_path(group)
+
+      page.within('.variable-menu') do
+        page.accept_alert 'Are you sure?' do
+          click_on 'Remove'
+        end
+      end
+    end
+
+    scenario 'user does not see the deleted variable' do
+      expect(page).to have_no_css('.variables-table')
+    end
+  end
+end

spec/features/variables_spec.rb

@@ -82,7 +82,7 @@ describe 'Project variables', js: true do
 
   it 'deletes variable' do
     page.within('.variables-table') do
-      find('.btn-variable-delete').click
+      click_on 'Remove'
     end
 
     expect(page).not_to have_selector('variables-table')
@@ -90,7 +90,7 @@ describe 'Project variables', js: true do
 
   it 'edits variable' do
     page.within('.variables-table') do
-      find('.btn-variable-edit').click
+      click_on 'Update'
     end
 
     expect(page).to have_content('Update variable')
@@ -104,7 +104,7 @@ describe 'Project variables', js: true do
 
   it 'edits variable with empty value' do
     page.within('.variables-table') do
-      find('.btn-variable-edit').click
+      click_on 'Update'
     end
 
     expect(page).to have_content('Update variable')
@@ -117,7 +117,7 @@ describe 'Project variables', js: true do
 
   it 'edits variable to be protected' do
     page.within('.variables-table') do
-      find('.btn-variable-edit').click
+      click_on 'Update'
     end
 
     expect(page).to have_content('Update variable')
@@ -132,7 +132,7 @@ describe 'Project variables', js: true do
     project.variables.first.update(protected: true)
 
     page.within('.variables-table') do
-      find('.btn-variable-edit').click
+      click_on 'Update'
     end
 
     expect(page).to have_content('Update variable')

spec/models/ci/build_spec.rb

@@ -1356,6 +1356,59 @@ describe Ci::Build, :models do
       end
     end
 
+    context 'when group secret variable is defined' do
+      let(:secret_variable) do
+        { key: 'SECRET_KEY', value: 'secret_value', public: false }
+      end
+
+      let(:group) { create(:group, :access_requestable) }
+
+      before do
+        build.project.update(group: group)
+
+        create(:ci_group_variable,
+               secret_variable.slice(:key, :value).merge(group: group))
+      end
+
+      it { is_expected.to include(secret_variable) }
+    end
+
+    context 'when group protected variable is defined' do
+      let(:protected_variable) do
+        { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
+      end
+
+      let(:group) { create(:group, :access_requestable) }
+
+      before do
+        build.project.update(group: group)
+
+        create(:ci_group_variable,
+               :protected,
+               protected_variable.slice(:key, :value).merge(group: group))
+      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
       let(:trigger) { create(:ci_trigger, project: project) }
       let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) }

spec/models/ci/group_variable_spec.rb

@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Ci::GroupVariable, models: true do
+  subject { build(:ci_group_variable) }
+
+  it { is_expected.to include_module(HasVariable) }
+  it { is_expected.to include_module(Presentable) }
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id) }
+
+  describe '.unprotected' do
+    subject { described_class.unprotected }
+
+    context 'when variable is protected' do
+      before do
+        create(:ci_group_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_group_variable, protected: false) }
+
+      it 'returns the variable' do
+        is_expected.to contain_exactly(variable)
+      end
+    end
+  end
+end

spec/models/ci/variable_spec.rb

@@ -3,10 +3,9 @@ require 'spec_helper'
 describe Ci::Variable, models: true do
   subject { build(:ci_variable) }
 
-  let(:secret_value) { 'secret' }
-
   describe 'validations' do
     it { is_expected.to include_module(HasVariable) }
+    it { is_expected.to include_module(Presentable) }
     it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) }
   end
 

spec/models/group_spec.rb

@@ -13,6 +13,7 @@ describe Group, models: true do
     it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:labels).class_name('GroupLabel') }
+    it { is_expected.to have_many(:variables).class_name('Ci::GroupVariable') }
     it { is_expected.to have_many(:uploads).dependent(:destroy) }
     it { is_expected.to have_one(:chat_team) }
 
@@ -418,4 +419,62 @@ describe Group, models: true do
       expect(calls).to eq 2
     end
   end
+
+  describe '#secret_variables_for' do
+    let(:project) { create(:empty_project, group: group) }
+
+    let!(:secret_variable) do
+      create(:ci_group_variable, value: 'secret', group: group)
+    end
+
+    let!(:protected_variable) do
+      create(:ci_group_variable, :protected, value: 'protected', group: group)
+    end
+
+    subject { group.secret_variables_for('ref', project) }
+
+    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
+
+    context 'when group has a child' do
+      let!(:group_child) { create(:group, :access_requestable, parent: group) }
+      let!(:variable_child) { create(:ci_group_variable, group: group_child) }
+
+      subject { group_child.secret_variables_for('ref', project) }
+
+      it 'returns all variables belong to the group and parent groups' do
+        is_expected.to contain_exactly(secret_variable, protected_variable, variable_child)
+      end
+    end
+  end
 end

spec/presenters/ci/group_variable_presenter_spec.rb

@@ -0,0 +1,63 @@
+require 'spec_helper'
+
+describe Ci::GroupVariablePresenter do
+  include Gitlab::Routing.url_helpers
+
+  let(:group) { create(:group) }
+  let(:variable) { create(:ci_group_variable, group: group) }
+
+  subject(:presenter) do
+    described_class.new(variable)
+  end
+
+  it 'inherits from Gitlab::View::Presenter::Delegated' do
+    expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated)
+  end
+
+  describe '#initialize' do
+    it 'takes a variable and optional params' do
+      expect { presenter }.not_to raise_error
+    end
+
+    it 'exposes variable' do
+      expect(presenter.variable).to eq(variable)
+    end
+
+    it 'forwards missing methods to variable' do
+      expect(presenter.key).to eq(variable.key)
+    end
+  end
+
+  describe '#placeholder' do
+    subject { described_class.new(variable).placeholder }
+
+    it { is_expected.to eq('GROUP_VARIABLE') }
+  end
+
+  describe '#form_path' do
+    context 'when variable is persisted' do
+      subject { described_class.new(variable).form_path }
+
+      it { is_expected.to eq(group_variable_path(group, variable)) }
+    end
+
+    context 'when variable is not persisted' do
+      let(:variable) { build(:ci_group_variable, group: group) }
+      subject { described_class.new(variable).form_path }
+
+      it { is_expected.to eq(group_variables_path(group)) }
+    end
+  end
+
+  describe '#edit_path' do
+    subject { described_class.new(variable).edit_path }
+
+    it { is_expected.to eq(group_variable_path(group, variable)) }
+  end
+
+  describe '#delete_path' do
+    subject { described_class.new(variable).delete_path }
+
+    it { is_expected.to eq(group_variable_path(group, variable)) }
+  end
+end

spec/presenters/ci/variable_presenter_spec.rb

@@ -0,0 +1,63 @@
+require 'spec_helper'
+
+describe Ci::VariablePresenter do
+  include Gitlab::Routing.url_helpers
+
+  let(:project) { create(:empty_project) }
+  let(:variable) { create(:ci_variable, project: project) }
+
+  subject(:presenter) do
+    described_class.new(variable)
+  end
+
+  it 'inherits from Gitlab::View::Presenter::Delegated' do
+    expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated)
+  end
+
+  describe '#initialize' do
+    it 'takes a variable and optional params' do
+      expect { presenter }.not_to raise_error
+    end
+
+    it 'exposes variable' do
+      expect(presenter.variable).to eq(variable)
+    end
+
+    it 'forwards missing methods to variable' do
+      expect(presenter.key).to eq(variable.key)
+    end
+  end
+
+  describe '#placeholder' do
+    subject { described_class.new(variable).placeholder }
+
+    it { is_expected.to eq('PROJECT_VARIABLE') }
+  end
+
+  describe '#form_path' do
+    context 'when variable is persisted' do
+      subject { described_class.new(variable).form_path }
+
+      it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) }
+    end
+
+    context 'when variable is not persisted' do
+      let(:variable) { build(:ci_variable, project: project) }
+      subject { described_class.new(variable).form_path }
+
+      it { is_expected.to eq(namespace_project_variables_path(project.namespace, project)) }
+    end
+  end
+
+  describe '#edit_path' do
+    subject { described_class.new(variable).edit_path }
+
+    it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) }
+  end
+
+  describe '#delete_path' do
+    subject { described_class.new(variable).delete_path }
+
+    it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) }
+  end
+end