CE feature gropu-level variable

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

Copyedit docs for group-level secret variables

[ci skip]

Wrap additional routes by dash(-). And remove those routes from path_regex.rb.

Add dash for GROUP_ROUTES

Fix variables_controller.rb and format

ayufan nice catches

Use ancestors for avoiding N queries

Add CASE When Clause for saving order when using where IN

secret_variables_for: rails readability versino

Adopt project_variable_path instead of namespace_project_variable_path. (Resolve confilct from master)

gb nice catches

Use new project_variables_path in this MR

Fix to Variable was successfully created

Use variable_params && variable_params_attributes in project variables_controller.rb

Name as variable_params like project variable controller

app/assets/javascripts/dispatcher.js

@@ -418,6 +418,7 @@ import AuditLogs from './audit_logs';
           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

+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

+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(variable_params)
+        redirect_to group_variables_path(group),
+                    notice: 'Variable was successfully updated.'
+      else
+        render "show"
+      end
+    end
+
+    def create
+      @variable = group.variables.create(variable_params)
+        .present(current_user: current_user)
+
+      if @variable.persisted?
+        redirect_to group_settings_ci_cd_path(group),
+                    notice: 'Variable was successfully created.'
+      else
+        render "show"
+      end
+    end
+
+    def destroy
+      if variable.destroy
+        redirect_to group_settings_ci_cd_path(group),
+                    status: 302,
+                    notice: 'Variable was successfully removed.'
+      else
+        redirect_to group_settings_ci_cd_path(group),
+                    status: 302,
+                    notice: 'Failed to remove the variable.'
+      end
+    end
+
+    private
+
+    def variable_params
+      params.require(:variable).permit(*variable_params_attributes)
+    end
+
+    def variable_params_attributes
+      %i[key value protected]
+    end
+
+    def variable
+      @variable ||= group.variables.find(params[:id]).present(current_user: current_user)
+    end
+
+    def authorize_admin_build!
+      return render_404 unless can?(current_user, :admin_build, group)
+    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

 class Projects::VariablesController < Projects::ApplicationController
   prepend ::EE::Projects::VariablesController
 
+  before_action :variable, only: [:show, :update, :destroy]
   before_action :authorize_admin_build!
 
   layout 'project_settings'
@@ -10,37 +11,39 @@ class Projects::VariablesController < Projects::ApplicationController
   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(variable_params)
+      redirect_to project_variables_path(project),
+                  notice: 'Variable was successfully updated.'
     else
-      render action: "show"
+      render "show"
     end
   end
 
   def create
-    @variable = @project.variables.new(variable_params)
+    @variable = project.variables.create(variable_params)
+      .present(current_user: current_user)
 
-    if @variable.save
-      flash[:notice] = 'Variables were successfully updated.'
-      redirect_to project_settings_ci_cd_path(project)
+    if @variable.persisted?
+      redirect_to project_settings_ci_cd_path(project),
+                  notice: 'Variable was successfully created.'
     else
       render "show"
     end
   end
 
   def destroy
-    @key = @project.variables.find(params[:id])
-    @key.destroy
-
+    if variable.destroy
       redirect_to project_settings_ci_cd_path(project),
                   status: 302,
                   notice: 'Variable was successfully removed.'
+    else
+      redirect_to project_settings_ci_cd_path(project),
+                  status: 302,
+                  notice: 'Failed to remove the variable.'
+    end
   end
 
   private
@@ -52,4 +55,8 @@ class Projects::VariablesController < Projects::ApplicationController
   def variable_params_attributes
     %i[id key value protected _destroy]
   end
+
+  def variable
+    @variable ||= project.variables.find(params[:id]).present(current_user: current_user)
+  end
 end

app/models/ci/build.rb

@@ -204,6 +204,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

+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

@@ -3,6 +3,7 @@ module Ci
     extend Ci::Model
     include HasVariable
     prepend EE::Ci::Variable
+    include Presentable
 
     belongs_to :project
 

app/models/group.rb

@@ -26,6 +26,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'
 
   has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent
@@ -294,6 +295,14 @@ class Group < Namespace
     }
   end
 
+  def secret_variables_for(ref, project)
+    list_of_ids = [self] + ancestors
+    variables = Ci::GroupVariable.where(group: list_of_ids)
+    variables = variables.unprotected unless project.protected_for?(ref)
+    variables = variables.group_by(&:group_id)
+    list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
+  end
+
   protected
 
   def update_two_factor_requirement

app/policies/group_policy.rb

@@ -33,6 +33,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

+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

+module Ci
+  class VariablePresenter < Gitlab::View::Presenter::Delegated
+    presents :variable
+
+    def placeholder
+      'PROJECT_VARIABLE'
+    end
+
+    def form_path
+      if variable.persisted?
+        project_variable_path(project, variable)
+      else
+        project_variables_path(project)
+      end
+    end
+
+    def edit_path
+      project_variable_path(project, variable)
+    end
+
+    def delete_path
+      project_variable_path(project, variable)
+    end
+  end
+end

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

-= 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
   = render 'projects/variables/ee/environment_scope', f: f
   .form-group
     .checkbox

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

 .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

+- 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/projects/variables/_table.html.haml → app/views/ci/variables/_table.html.haml

@@ -13,7 +13,7 @@
         %th Environment scope
       %th
     %tbody
-      - @project.variables.order_key_asc.each do |variable|
+      - @variables.each do |variable|
         - if variable.id?
           %tr
             %td.variable-key= variable.key
@@ -22,11 +22,11 @@
             - if @project.feature_available?(:variable_environment_scope)
               %td.variable-environment-scope= variable.environment_scope
             %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

@@ -13,4 +13,9 @@
             %span
               Projects
 
+        = nav_link(controller: :ci_cd) do
+          = link_to group_settings_ci_cd_path(@group), title: 'Pipelines' do
+            %span
+              Pipelines
+
         = render 'groups/ee/settings_nav'

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

+- page_title "Pipelines"
+= render "groups/settings_head"
+
+= render 'ci/variables/index'

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

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

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

+---
+title: Add Group secret variables
+merge_request: 12582
+author:

config/routes/group.rb

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

db/migrate/20170525130346_create_group_variables_table.rb

+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

+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

@@ -312,6 +312,20 @@ ActiveRecord::Schema.define(version: 20170706151212) 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"
@@ -1871,6 +1885,7 @@ ActiveRecord::Schema.define(version: 20170706151212) 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](#secret-variables) or [protected secret variables](#protected-secret-variables)
+1. Group-level [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)
@@ -143,23 +144,28 @@ script:
 
 >**Notes:**
 - This feature requires GitLab Runner 0.4.0 or higher.
+- Group-level secret variables added in GitLab 9.4.
 - Be aware that secret variables are not masked, and their values can be shown
   in the job logs if explicitly asked to do so. If your project is public or
   internal, you can set the pipelines private from your project's Pipelines
   settings. Follow the discussion in issue [#13784][ce-13784] for masking the
   secret variables.
 
-GitLab CI allows you to define per-project **secret variables** that are set in
-the build environment. The secret variables are stored out of the repository
-(`.gitlab-ci.yml`) and are securely passed to GitLab Runner making them
-available in the build environment. It's the recommended method to use for
-storing things like passwords, secret keys and credentials.
+GitLab CI allows you to define per-project or per-group **secret variables**
+that are set in the build environment. The secret variables are stored out of
+the repository (`.gitlab-ci.yml`) and are securely passed to GitLab Runner
+making them available in the build environment. It's the recommended method to
+use for 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**.
+Project-level 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.
+Likewise, group-level secret variables can be added by going to your group's
+**Settings ➔ Pipelines**, then finding the section called **Secret variables**.
+Any variables of [subgroups] will be inherited recursively.
+
+Once you set them, they will be available for all subsequent pipelines. You can also
+[protect your variables](#protected-secret-variables).
 
 ### Protected secret variables
 
@@ -454,3 +460,4 @@ export CI_REGISTRY_PASSWORD="longalfanumstring"
 [shellexecutors]: https://docs.gitlab.com/runner/executors/
 [triggered]: ../triggers/README.md
 [triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger
+[subgroups]: ../../user/group/subgroups/index.md

lib/gitlab/path_regex.rb

@@ -112,6 +112,7 @@ module Gitlab
     # this group would not be accessible through `/groups/parent/activity` since
     # this would map to the activity-page of its parent.
     GROUP_ROUTES = %w[
+      -
       activity
       analytics
       audit_events

spec/controllers/groups/settings/ci_cd_controller_spec.rb

+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

+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 'Variable was successfully created.'
+        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

@@ -15,13 +15,13 @@ describe Projects::VariablesController do
         post :create, namespace_id: project.namespace.to_param, project_id: project,
                       variable: { key: "one", value: "two" }
 
-        expect(flash[:notice]).to include 'Variables were successfully updated.'
+        expect(flash[:notice]).to include 'Variable was successfully created.'
         expect(response).to redirect_to(project_settings_ci_cd_path(project))
       end
     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

+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

+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

@@ -24,7 +24,7 @@ describe 'Project variables', js: true do
     fill_in('variable_value', with: 'key value')
     click_button('Add new variable')
 
-    expect(page).to have_content('Variables were successfully updated.')
+    expect(page).to have_content('Variable was successfully created.')
     page.within('.variables-table') do
       expect(page).to have_content('key')
       expect(page).to have_content('No')
@@ -36,7 +36,7 @@ describe 'Project variables', js: true do
     fill_in('variable_value', with: '')
     click_button('Add new variable')
 
-    expect(page).to have_content('Variables were successfully updated.')
+    expect(page).to have_content('Variable was successfully created.')
     page.within('.variables-table') do
       expect(page).to have_content('new_key')
     end
@@ -48,7 +48,7 @@ describe 'Project variables', js: true do
     check('Protected')
     click_button('Add new variable')
 
-    expect(page).to have_content('Variables were successfully updated.')
+    expect(page).to have_content('Variable was successfully created.')
     page.within('.variables-table') do
       expect(page).to have_content('key')
       expect(page).to have_content('Yes')
@@ -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

@@ -1375,6 +1375,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

+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

@@ -10,6 +10,7 @@ describe Ci::Variable, models: true do
     end
 
     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) }
 
@@ -456,4 +457,69 @@ 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 children' do
+      let!(:group_child) { create(:group, parent: group) }
+      let!(:variable_child) { create(:ci_group_variable, group: group_child) }
+      let!(:group_child_3) { create(:group, parent: group_child_2) }
+      let!(:variable_child_3) { create(:ci_group_variable, group: group_child_3) }
+      let!(:group_child_2) { create(:group, parent: group_child) }
+      let!(:variable_child_2) { create(:ci_group_variable, group: group_child_2) }
+
+      it 'returns all variables belong to the group and parent groups' do
+        expected_array1 = [protected_variable, secret_variable]
+        expected_array2 = [variable_child, variable_child_2, variable_child_3]
+        got_array = group_child_3.secret_variables_for('ref', project).to_a
+
+        expect(got_array.shift(2)).to contain_exactly(*expected_array1)
+        expect(got_array).to eq(expected_array2)
+      end
+    end
+  end
 end

spec/presenters/ci/group_variable_presenter_spec.rb

+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

+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(project_variable_path(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(project_variables_path(project)) }
+    end
+  end
+
+  describe '#edit_path' do
+    subject { described_class.new(variable).edit_path }
+
+    it { is_expected.to eq(project_variable_path(project, variable)) }
+  end
+
+  describe '#delete_path' do
+    subject { described_class.new(variable).delete_path }
+
+    it { is_expected.to eq(project_variable_path(project, variable)) }
+  end
+end