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

23 files changed, 160 insertions, 14 deletions

diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb
index 24fe78bc1bd..602a8115f7d 100644
--- a/app/controllers/projects/settings/ci_cd_controller.rb
+++ b/app/controllers/projects/settings/ci_cd_controller.rb
@@ -21,7 +21,7 @@ module Projects
       end
 
       def define_secret_variables
-        @variable = Ci::Variable.new
+        @variable = Ci::VariableProject.new
       end
 
       def define_triggers_variables
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 573d1c05622..0f244790ec9 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -22,7 +22,7 @@ class Projects::VariablesController < Projects::ApplicationController
   end
 
   def create
-    @variable = Ci::Variable.new(project_params)
+    @variable = Ci::VariableProject.new(project_params)
 
     if @variable.valid? && @project.variables << @variable
       flash[:notice] = 'Variables were successfully updated.'
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 2e7a80d308b..d93bba07d8b 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -194,6 +194,7 @@ module Ci
       variables += project.deployment_variables if has_environment?
       variables += yaml_variables
       variables += user_variables
+      variables += project.group.secret_variables if project&.group.present?
       variables += project.secret_variables_for(ref).map(&:to_runner_variable)
       variables += trigger_request.user_variables if trigger_request
       variables
diff --git a/app/models/ci/variable_group.rb b/app/models/ci/variable_group.rb
new file mode 100644
index 00000000000..3abdb8d9cd5
--- /dev/null
+++ b/app/models/ci/variable_group.rb
@@ -0,0 +1,9 @@
+module Ci
+  class VariableGroup < Ci::Variable
+    self.table_name = 'ci_group_variables'
+
+    belongs_to :group
+
+    validates :key, uniqueness: { scope: :group_id }
+  end
+end
diff --git a/app/models/ci/variable_project.rb b/app/models/ci/variable_project.rb
new file mode 100644
index 00000000000..6d68aab8c7d
--- /dev/null
+++ b/app/models/ci/variable_project.rb
@@ -0,0 +1,9 @@
+module Ci
+  class VariableProject < Ci::Variable
+    self.table_name = 'ci_variables'
+
+    belongs_to :project
+
+    validates :key, uniqueness: { scope: :project_id }
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index a6fdb30f84c..2514a05cf2d 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
   has_many :labels, class_name: 'GroupLabel'
+  has_many :variables, dependent: :destroy, class_name: 'Ci::VariableGroup'
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects
@@ -248,6 +249,18 @@ class Group < Namespace
     }
   end
 
+  def secret_variables
+    var = variables.map do |variable|
+      { key: variable.key, value: variable.value, public: false }
+    end
+
+    if has_parent?
+      parent.secret_variables + var
+    else
+      var
+    end
+  end
+
   protected
 
   def update_two_factor_requirement
diff --git a/app/models/project.rb b/app/models/project.rb
index 315e1a6cb17..242609841b6 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -165,7 +165,7 @@ class Project < ActiveRecord::Base
   has_many :builds, class_name: 'Ci::Build' # the builds are created from the commit_statuses
   has_many :runner_projects, dependent: :destroy, class_name: 'Ci::RunnerProject'
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
-  has_many :variables, class_name: 'Ci::Variable'
+  has_many :variables, class_name: 'Ci::VariableProject'
   has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger'
   has_many :environments, dependent: :destroy
   has_many :deployments, dependent: :destroy
diff --git a/app/views/projects/variables/_form.html.haml b/app/views/projects/variables/_form.html.haml
index 0a70a301cb4..3264d0c81d7 100644
--- a/app/views/projects/variables/_form.html.haml
+++ b/app/views/projects/variables/_form.html.haml
@@ -1,4 +1,4 @@
-= form_for [@project.namespace.becomes(Namespace), @project, @variable] do |f|
+= form_for [@project.namespace.becomes(Namespace), @project, @variable.becomes(Ci::Variable)] do |f|
   = form_errors(@variable)
 
   .form-group
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..7e09ead69e3
--- /dev/null
+++ b/db/migrate/20170525130346_create_group_variables_table.rb
@@ -0,0 +1,22 @@
+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.timestamps 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 d52dab5417e..6d7c99eb1eb 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -253,6 +253,19 @@ 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.datetime "created_at"
+    t.datetime "updated_at"
+  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"
@@ -1531,6 +1544,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do
   add_foreign_key "chat_teams", "namespaces", on_delete: :cascade
   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_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/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb
index a0ecc756653..b7e0af5feb2 100644
--- a/spec/controllers/projects/variables_controller_spec.rb
+++ b/spec/controllers/projects/variables_controller_spec.rb
@@ -31,7 +31,7 @@ describe Projects::VariablesController do
   end
 
   describe 'POST #update' do
-    let(:variable) { create(:ci_variable) }
+    let(:variable) { create(:ci_variable_project) }
 
     context 'updating a variable with valid characters' do
       before do
diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb
index f83366136fd..09131061967 100644
--- a/spec/factories/ci/variables.rb
+++ b/spec/factories/ci/variables.rb
@@ -7,6 +7,12 @@ FactoryGirl.define do
       protected true
     end
 
-    project factory: :empty_project
+    factory :ci_variable_project, class: Ci::VariableProject do
+      project factory: :empty_project
+    end
+
+    factory :ci_variable_group, class: Ci::VariableGroup do
+      group factory: :group
+    end
   end
 end
diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb
index 1a2dedf27eb..dca73381fb1 100644
--- a/spec/features/variables_spec.rb
+++ b/spec/features/variables_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe 'Project variables', js: true do
   let(:user)     { create(:user) }
   let(:project)  { create(:empty_project) }
-  let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') }
+  let(:variable) { create(:ci_variable_project, key: 'test_key', value: 'test value') }
 
   before do
     gitlab_sign_in(user)
diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb
index 5417c7534ea..fc226400dbb 100644
--- a/spec/lib/gitlab/import_export/relation_factory_spec.rb
+++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb
@@ -182,7 +182,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do
   context 'encrypted attributes' do
     let(:relation_sym) { 'Ci::Variable' }
     let(:relation_hash) do
-      create(:ci_variable).as_json
+      create(:ci_variable, project_id: project.id).as_json
     end
 
     it 'has no value for the encrypted attribute' do
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 7de5e2e3920..175c61ad3c7 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1310,8 +1310,8 @@ describe Ci::Build, :models do
       it { is_expected.to include(tag_variable) }
     end
 
-    context 'when secret variable is defined' do
-      let(:secret_variable) do
+    context 'when project secure variable is defined' do
+      let(:secure_variable) do
         { key: 'SECRET_KEY', value: 'secret_value', public: false }
       end
 
@@ -1320,6 +1320,21 @@ describe Ci::Build, :models do
                secret_variable.slice(:key, :value).merge(project: project))
       end
 
+      it { is_expected.to include(secure_variable) }
+    end
+
+    context 'when group secure variable is defined' do
+      let(:secure_variable) do
+        { key: 'SECRET_KEY', value: 'secret_value', public: false }
+      end
+
+      let(:group) { create(:group, :access_requestable) }
+
+      before do
+        allow(build.project).to receive(:group).and_return(group)
+        build.project.group.variables << Ci::VariableGroup.new(key: 'SECRET_KEY', value: 'secret_value')
+      end
+
       it { is_expected.to include(secret_variable) }
     end
 
diff --git a/spec/models/ci/variable_group_spec.rb b/spec/models/ci/variable_group_spec.rb
new file mode 100644
index 00000000000..a1e4bd4a7f2
--- /dev/null
+++ b/spec/models/ci/variable_group_spec.rb
@@ -0,0 +1,7 @@
+require 'spec_helper'
+
+describe Ci::VariableGroup, models: true do
+  subject { build(:ci_variable_group) }
+
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id) }
+end
diff --git a/spec/models/ci/variable_project_spec.rb b/spec/models/ci/variable_project_spec.rb
new file mode 100644
index 00000000000..9325f584388
--- /dev/null
+++ b/spec/models/ci/variable_project_spec.rb
@@ -0,0 +1,7 @@
+require 'spec_helper'
+
+describe Ci::VariableProject, models: true do
+  subject { build(:ci_variable_project) }
+
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id) }
+end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 4de1683b21c..48bdc5959a7 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::VariableGroup') }
     it { is_expected.to have_many(:uploads).dependent(:destroy) }
     it { is_expected.to have_one(:chat_team) }
 
@@ -379,6 +380,33 @@ describe Group, models: true do
     end
   end
 
+  describe '#secret_variables' do
+    let!(:variable) { create(:ci_variable_group, group: group) }
+
+    subject { group.secret_variables }
+
+    it 'returns all variables belong to the group' do
+      is_expected.to eq(format_variables([variable]))
+    end
+
+    context 'when group has a child' do
+      let!(:group_child) { create(:group, :access_requestable, parent: group) }
+      let!(:variable_child) { create(:ci_variable_group, group: group_child) }
+
+      subject { group_child.secret_variables }
+
+      it 'returns all variables belong to the group and parent groups' do
+        is_expected.to eq(format_variables([variable, variable_child]))
+      end
+    end
+
+    def format_variables(variables)
+      variables.map do |variable|
+        { key: variable.key, value: variable.value, public: false }
+      end
+    end
+  end
+
   describe '#update_two_factor_requirement' do
     let(:user) { create(:user) }
 
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 339a57a1f20..ce8ba9fb338 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -554,7 +554,7 @@ describe API::Runner do
             before do
               trigger = create(:ci_trigger, project: project)
               create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger)
-              project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value')
+              project.variables << Ci::VariableProject.new(key: 'SECRET_KEY', value: 'secret_value')
             end
 
             it 'returns variables for triggers' do
diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb
index e0975024b80..b08848e58a3 100644
--- a/spec/requests/api/variables_spec.rb
+++ b/spec/requests/api/variables_spec.rb
@@ -6,7 +6,7 @@ describe API::Variables do
   let!(:project) { create(:empty_project, creator_id: user.id) }
   let!(:master) { create(:project_member, :master, user: user, project: project) }
   let!(:developer) { create(:project_member, :developer, user: user2, project: project) }
-  let!(:variable) { create(:ci_variable, project: project) }
+  let!(:variable) { create(:ci_variable_project, project: project) }
 
   describe 'GET /projects/:id/variables' do
     context 'authorized user with proper permissions' do
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index c969d08d0dd..fba3f334ac6 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -184,7 +184,7 @@ describe Ci::API::Builds do
         before do
           trigger = create(:ci_trigger, project: project)
           create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [build], trigger: trigger)
-          project.variables << Ci::Variable.new(key: "SECRET_KEY", value: "secret_value")
+          project.variables << Ci::VariableProject.new(key: "SECRET_KEY", value: "secret_value")
         end
 
         it "returns variables for triggers" do
diff --git a/spec/support/import_export/export_file_helper.rb b/spec/support/import_export/export_file_helper.rb
index 57b6abe12b7..6d4c9a94489 100644
--- a/spec/support/import_export/export_file_helper.rb
+++ b/spec/support/import_export/export_file_helper.rb
@@ -37,7 +37,7 @@ module ExportFileHelper
 
     create(:event, :created, target: milestone, project: project, author: user)
     create(:project_member, :master, user: user, project: project)
-    create(:ci_variable, project: project)
+    create(:ci_variable_project, project: project)
     create(:ci_trigger, project: project)
     key = create(:deploy_key)
     key.projects << project