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

41 files changed, 733 insertions, 52 deletions

diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index ffd8d494a40..81267c68cfc 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/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':
diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb
new file mode 100644
index 00000000000..0142ad8278c
--- /dev/null
+++ b/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
diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb
new file mode 100644
index 00000000000..6fba9dab2a7
--- /dev/null
+++ b/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
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb
index 24fe78bc1bd..ea7ceb3eaa5 100644
--- a/app/controllers/projects/settings/ci_cd_controller.rb
+++ b/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
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 326d31ecec2..716e1347604 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/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
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index ba2ecbf82a2..25e75a19f37 100644
--- a/app/models/ci/build.rb
+++ b/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
diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb
new file mode 100644
index 00000000000..f64bc245a67
--- /dev/null
+++ b/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
diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb
index 0b8d0ff881a..cf0fe04ddaf 100644
--- a/app/models/ci/variable.rb
+++ b/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
 
diff --git a/app/models/group.rb b/app/models/group.rb
index b93fce6100d..f625b8c250c 100644
--- a/app/models/group.rb
+++ b/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
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index dcb37416ca3..6defab75fce 100644
--- a/app/policies/group_policy.rb
+++ b/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
diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb
new file mode 100644
index 00000000000..81fea106a5c
--- /dev/null
+++ b/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
diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb
new file mode 100644
index 00000000000..3f14d972c50
--- /dev/null
+++ b/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
diff --git a/app/views/projects/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml
index 98f618ca3b8..98f618ca3b8 100644
--- a/app/views/projects/variables/_content.html.haml
+++ b/app/views/ci/variables/_content.html.haml
diff --git a/app/views/projects/variables/_form.html.haml b/app/views/ci/variables/_form.html.haml
index 0a70a301cb4..eebd0955c80 100644
--- a/app/views/projects/variables/_form.html.haml
+++ b/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
diff --git a/app/views/projects/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml
index 5e6786f6698..007c2344b5a 100644
--- a/app/views/projects/variables/_index.html.haml
+++ b/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
diff --git a/app/views/ci/variables/_show.html.haml b/app/views/ci/variables/_show.html.haml
new file mode 100644
index 00000000000..2bfb290629d
--- /dev/null
+++ b/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"
diff --git a/app/views/projects/variables/_table.html.haml b/app/views/ci/variables/_table.html.haml
index 4ce6a828812..71a0b56c4f4 100644
--- a/app/views/projects/variables/_table.html.haml
+++ b/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")
diff --git a/app/views/groups/_settings_head.html.haml b/app/views/groups/_settings_head.html.haml
index 2454e7355a7..623d233a46a 100644
--- a/app/views/groups/_settings_head.html.haml
+++ b/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
diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml
new file mode 100644
index 00000000000..bf36baf48ab
--- /dev/null
+++ b/app/views/groups/settings/ci_cd/show.html.haml
@@ -0,0 +1,4 @@
+- page_title "Pipelines"
+= render "groups/settings_head"
+
+= render 'ci/variables/index'
diff --git a/app/views/groups/variables/show.html.haml b/app/views/groups/variables/show.html.haml
new file mode 100644
index 00000000000..df533952b76
--- /dev/null
+++ b/app/views/groups/variables/show.html.haml
@@ -0,0 +1 @@
+= render 'ci/variables/show'
diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml
index 00ccc3ec41e..6afb38c5709 100644
--- a/app/views/projects/settings/ci_cd/show.html.haml
+++ b/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'
diff --git a/app/views/projects/variables/show.html.haml b/app/views/projects/variables/show.html.haml
index 297a53ca98c..df533952b76 100644
--- a/app/views/projects/variables/show.html.haml
+++ b/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'
diff --git a/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml b/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml
new file mode 100644
index 00000000000..333895ffba9
--- /dev/null
+++ b/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml
@@ -0,0 +1,4 @@
+---
+title: Add Group secret variables
+merge_request: 12582
+author:
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 11cdff55ed8..bd04ad51076 100644
--- a/config/routes/group.rb
+++ b/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',
diff --git a/db/migrate/20170525130346_create_group_variables_table.rb b/db/migrate/20170525130346_create_group_variables_table.rb
new file mode 100644
index 00000000000..eaa38dbc40d
--- /dev/null
+++ b/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
diff --git a/db/migrate/20170525130758_add_foreign_key_to_group_variables.rb b/db/migrate/20170525130758_add_foreign_key_to_group_variables.rb
new file mode 100644
index 00000000000..0146235c5ba
--- /dev/null
+++ b/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
diff --git a/db/schema.rb b/db/schema.rb
index a47a6ae9a98..f4d83a4dd9c 100644
--- a/db/schema.rb
+++ b/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
diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md
index 3501aae75ec..ee3349d9526 100644
--- a/doc/ci/variables/README.md
+++ b/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:**
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index 10eb99fb461..f0d5e3f7bc0 100644
--- a/lib/gitlab/path_regex.rb
+++ b/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
diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb
new file mode 100644
index 00000000000..2e0efb57c74
--- /dev/null
+++ b/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
diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb
new file mode 100644
index 00000000000..c11fe93ffca
--- /dev/null
+++ b/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
diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb
index a0ecc756653..0304ea6f93c 100644
--- a/spec/controllers/projects/variables_controller_spec.rb
+++ b/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
 
diff --git a/spec/factories/ci/group_variables.rb b/spec/factories/ci/group_variables.rb
new file mode 100644
index 00000000000..565ced9eb1a
--- /dev/null
+++ b/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
diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb
new file mode 100644
index 00000000000..37814ba6238
--- /dev/null
+++ b/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
diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb
index 1a2dedf27eb..a7c24c7d718 100644
--- a/spec/features/variables_spec.rb
+++ b/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')
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 431fcda165d..cf6d356c524 100644
--- a/spec/models/ci/build_spec.rb
+++ b/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) }
diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb
new file mode 100644
index 00000000000..24b914face9
--- /dev/null
+++ b/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
diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb
index 4ffbfa6c130..890ffaae494 100644
--- a/spec/models/ci/variable_spec.rb
+++ b/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
 
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 4de1683b21c..dab33aa4418 100644
--- a/spec/models/group_spec.rb
+++ b/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
diff --git a/spec/presenters/ci/group_variable_presenter_spec.rb b/spec/presenters/ci/group_variable_presenter_spec.rb
new file mode 100644
index 00000000000..d404028405b
--- /dev/null
+++ b/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
diff --git a/spec/presenters/ci/variable_presenter_spec.rb b/spec/presenters/ci/variable_presenter_spec.rb
new file mode 100644
index 00000000000..3b615c4bf36
--- /dev/null
+++ b/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