Backports for ee-2112

https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2112

Backports for ee-2112
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2112
d9435d61 · Lin Jen-Shin · 2520edef · d9435d61 · d9435d61 · d9435d61
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -14,7 +14,7 @@ def show
  def update
    @variable = @project.variables.find(params[:id])

-    if @variable.update_attributes(project_params)
+    if @variable.update_attributes(variable_params)
      redirect_to project_variables_path(project), notice: 'Variable was successfully updated.'
    else
      render action: "show"
@@ -22,9 +22,9 @@ def update
  end

  def create
-    @variable = Ci::Variable.new(project_params)
+    @variable = @project.variables.new(variable_params)

-    if @variable.valid? && @project.variables << @variable
+    if @variable.save
      flash[:notice] = 'Variables were successfully updated.'
      redirect_to project_settings_ci_cd_path(project)
    else
@@ -43,8 +43,11 @@ def destroy

  private

-  def project_params
-    params.require(:variable)
-      .permit([:id, :key, :value, :protected, :_destroy])
+  def variable_params
+    params.require(:variable).permit(*variable_params_attributes)
+  end
+
+  def variable_params_attributes
+    %i[id key value protected _destroy]
  end
 end
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -186,6 +186,12 @@ def ref_slug

    # Variables whose value does not depend on environment
    def simple_variables
+      variables(environment: nil)
+    end
+
+    # All variables, including those dependent on environment, which could
+    # contain unexpanded variables.
+    def variables(environment: persisted_environment)
      variables = predefined_variables
      variables += project.predefined_variables
      variables += pipeline.predefined_variables
@@ -194,15 +200,11 @@ def simple_variables
      variables += project.deployment_variables if has_environment?
      variables += yaml_variables
      variables += user_variables
-      variables += project.secret_variables_for(ref).map(&:to_runner_variable)
+      variables += secret_variables(environment: environment)
      variables += trigger_request.user_variables if trigger_request
-      variables
-    end
+      variables += persisted_environment_variables if environment

-    # All variables, including those dependent on environment, which could
-    # contain unexpanded variables.
-    def variables
-      simple_variables.concat(persisted_environment_variables)
+      variables
    end

    def merge_request
@@ -370,6 +372,11 @@ def user_variables
      ]
    end

+    def secret_variables(environment: persisted_environment)
+      project.secret_variables_for(ref: ref, environment: environment)
+        .map(&:to_runner_variable)
+    end
+
    def steps
      [Gitlab::Ci::Build::Step.from_commands(self),
       Gitlab::Ci::Build::Step.from_after_script(self)].compact

--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1326,7 +1326,8 @@ def container_registry_variables
    variables
  end

-  def secret_variables_for(ref)
+  def secret_variables_for(ref:, environment: nil)
+    # EE would use the environment
    if protected_for?(ref)
      variables
    else

--- a/doc/ci/variables/README.md
+++ b/doc/ci/variables/README.md
@@ -160,7 +160,7 @@ Secret variables can be added by going to your project's

 Once you set them, they will be available for all subsequent pipelines.

-## Protected secret variables
+### Protected secret variables

 >**Notes:**
 This feature requires GitLab 9.3 or higher.
@@ -426,10 +426,11 @@ export CI_REGISTRY_PASSWORD="longalfanumstring"
 ```

 [ce-13784]: https://gitlab.com/gitlab-org/gitlab-ce/issues/13784
-[runner]: https://docs.gitlab.com/runner/
-[triggered]: ../triggers/README.md
-[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger
+[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium"
+[envs]: ../environments.md
 [protected branches]: ../../user/project/protected_branches.md
 [protected tags]: ../../user/project/protected_tags.md
+[runner]: https://docs.gitlab.com/runner/
 [shellexecutors]: https://docs.gitlab.com/runner/executors/
-[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium"
+[triggered]: ../triggers/README.md
+[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger
--- a/doc/downgrade_ee_to_ce/README.md
+++ b/doc/downgrade_ee_to_ce/README.md
@@ -46,6 +46,19 @@ $ sudo gitlab-rails runner "Service.where(type: ['JenkinsService', 'JenkinsDepre
 $ bundle exec rails runner "Service.where(type: ['JenkinsService', 'JenkinsDeprecatedService']).delete_all" production
 ```

+### Secret variables environment scopes
+
+If you're using this feature and there are variables sharing the same
+key, but they have different scopes in a project, then you might want to
+revisit the environment scope setting for those variables.
+
+In CE, environment scopes are completely ignored, therefore you could
+accidentally get a variable which you're not expecting for a particular
+environment. Make sure that you have the right variables in this case.
+
+Data is completely preserved, so you could always upgrade back to EE and
+restore the behavior if you leave it alone.
+
 ## Downgrade to CE

 After performing the above mentioned steps, you are now ready to downgrade your

--- a/lib/api/variables.rb
+++ b/lib/api/variables.rb
@@ -45,7 +45,9 @@ class Variables < Grape::API
        optional :protected, type: String, desc: 'Whether the variable is protected'
      end
      post ':id/variables' do
-        variable = user_project.variables.create(declared_params(include_missing: false))
+        variable_params = declared_params(include_missing: false)
+
+        variable = user_project.variables.create(variable_params)

        if variable.valid?
          present variable, with: Entities::Variable
@@ -67,7 +69,9 @@ class Variables < Grape::API

        return not_found!('Variable') unless variable

-        if variable.update(declared_params(include_missing: false).except(:key))
+        variable_params = declared_params(include_missing: false).except(:key)
+
+        if variable.update(variable_params)
          present variable, with: Entities::Variable
        else
          render_validation_error!(variable)

--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -30,8 +30,12 @@ def container_repository_name_regex
      @container_repository_regex ||= %r{\A[a-z0-9]+(?:[-._/][a-z0-9]+)*\Z}
    end

+    def environment_name_regex_chars
+      'a-zA-Z0-9_/\\$\\{\\}\\. -'
+    end
+
    def environment_name_regex
-      @environment_name_regex ||= /\A[a-zA-Z0-9_\\\/\${}. -]+\z/.freeze
+      @environment_name_regex ||= /\A[#{environment_name_regex_chars}]+\z/.freeze
    end

    def environment_name_regex_message

--- a/lib/gitlab/sql/glob.rb
+++ b/lib/gitlab/sql/glob.rb
+module Gitlab
+  module SQL
+    module Glob
+      extend self
+
+      # Convert a simple glob pattern with wildcard (*) to SQL LIKE pattern
+      # with SQL expression
+      def to_like(pattern)
+        <<~SQL
+          REPLACE(REPLACE(REPLACE(#{pattern},
+                                  #{q('%')}, #{q('\\%')}),
+                          #{q('_')}, #{q('\\_')}),
+                  #{q('*')}, #{q('%')})
+        SQL
+      end
+
+      def q(string)
+        ActiveRecord::Base.connection.quote(string)
+      end
+    end
+  end
+end
--- a/spec/lib/gitlab/sql/glob_spec.rb
+++ b/spec/lib/gitlab/sql/glob_spec.rb
+require 'spec_helper'
+
+describe Gitlab::SQL::Glob, lib: true do
+  describe '.to_like' do
+    it 'matches * as %' do
+      expect(glob('apple', '*')).to be(true)
+      expect(glob('apple', 'app*')).to be(true)
+      expect(glob('apple', 'apple*')).to be(true)
+      expect(glob('apple', '*pple')).to be(true)
+      expect(glob('apple', 'ap*le')).to be(true)
+
+      expect(glob('apple', '*a')).to be(false)
+      expect(glob('apple', 'app*a')).to be(false)
+      expect(glob('apple', 'ap*l')).to be(false)
+    end
+
+    it 'matches % literally' do
+      expect(glob('100%', '100%')).to be(true)
+
+      expect(glob('100%', '%')).to be(false)
+    end
+
+    it 'matches _ literally' do
+      expect(glob('^_^', '^_^')).to be(true)
+
+      expect(glob('^A^', '^_^')).to be(false)
+    end
+  end
+
+  def glob(string, pattern)
+    match(string, subject.to_like(quote(pattern)))
+  end
+
+  def match(string, pattern)
+    value = query("SELECT #{quote(string)} LIKE #{pattern}")
+              .rows.flatten.first
+
+    case value
+    when 't', 1
+      true
+    else
+      false
+    end
+  end
+
+  def query(sql)
+    ActiveRecord::Base.connection.select_all(sql)
+  end
+
+  def quote(string)
+    ActiveRecord::Base.connection.quote(string)
+  end
+end
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1496,9 +1496,10 @@ def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now)
        allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] }
        allow(build).to receive(:yaml_variables) { [build_yaml_var] }

-        allow(project).to receive(:secret_variables_for).with(build.ref) do
-          [create(:ci_variable, key: 'secret', value: 'value')]
-        end
+        allow(project).to receive(:secret_variables_for)
+          .with(ref: 'master', environment: nil) do
+            [create(:ci_variable, key: 'secret', value: 'value')]
+          end
      end

      it do

--- a/spec/models/ci/variable_spec.rb
+++ b/spec/models/ci/variable_spec.rb
@@ -8,10 +8,6 @@
  describe 'validations' do
    it { is_expected.to include_module(HasVariable) }
    it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) }
-    it { is_expected.to validate_length_of(:key).is_at_most(255) }
-    it { is_expected.to allow_value('foo').for(:key) }
-    it { is_expected.not_to allow_value('foo bar').for(:key) }
-    it { is_expected.not_to allow_value('foo/bar').for(:key) }
  end

  describe '.unprotected' do

--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1875,7 +1875,12 @@ def create_build(new_pipeline = pipeline, name = 'test')
      create(:ci_variable, :protected, value: 'protected', project: project)
    end

-    subject { project.secret_variables_for('ref') }
+    subject { project.secret_variables_for(ref: 'ref') }
+
+    before do
+      stub_application_setting(
+        default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+    end

    shared_examples 'ref is protected' do
      it 'contains all the variables' do
@@ -1884,11 +1889,6 @@ def create_build(new_pipeline = pipeline, name = 'test')
    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