Merge branch 'scoped-group-variables-api' into 'master'

Add environment scope to group CI variables API [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55573

app/finders/ci/variables_finder.rb

@@ -2,16 +2,14 @@
 
 module Ci
   class VariablesFinder
-    attr_reader :project, :params
-
-    def initialize(project, params)
-      @project, @params = project, params
+    def initialize(resource, params)
+      @resource, @params = resource, params
 
       raise ArgumentError, 'Please provide params[:key]' if params[:key].blank?
     end
 
     def execute
-      variables = project.variables
+      variables = resource.variables
       variables = by_key(variables)
       variables = by_environment_scope(variables)
       variables
@@ -19,6 +17,8 @@ module Ci
 
     private
 
+    attr_reader :resource, :params
+
     def by_key(variables)
       variables.by_key(params[:key])
     end

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

@@ -7,6 +7,9 @@
       %tr
         %td.gl-text-truncate
           = variable.key
+        - if Feature.enabled?(:scoped_group_variables, default_enabled: :yaml)
+          %td.gl-text-truncate
+            = variable.environment_scope
         %td.gl-text-truncate
           %a.group-origin-link{ href: group_settings_ci_cd_path(variable.group) }
             = variable.group.name

app/views/ci/group_variables/_variable_header.html.haml

 %tr
   %th
     = s_('Key')
+  - if Feature.enabled?(:scoped_group_variables, default_enabled: :yaml)
+    %th
+      = s_('Environments')
   %th
     = s_('Group')

ee/app/models/ee/group.rb

@@ -244,6 +244,11 @@ module EE
       feature_available?(:group_project_templates)
     end
 
+    def scoped_variables_available?
+      ::Feature.enabled?(:scoped_group_variables, self, default_enabled: :yaml) &&
+        feature_available?(:group_scoped_ci_variables)
+    end
+
     def actual_size_limit
       return ::Gitlab::CurrentSettings.repository_size_limit if repository_size_limit.nil?
 

ee/app/models/license.rb

@@ -98,6 +98,7 @@ class License < ApplicationRecord
     group_repository_analytics
     group_saml
     group_saml_group_sync
+    group_scoped_ci_variables
     group_wikis
     incident_sla
     incident_metric_upload

ee/lib/ee/api/helpers/variables_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Helpers
+      module VariablesHelpers
+        extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        prepended do
+          params :optional_group_variable_params_ee do
+            optional :environment_scope, type: String, desc: 'The environment scope of the variable'
+          end
+        end
+
+        override :filter_variable_parameters
+        def filter_variable_parameters(owner, params)
+          if owner.is_a?(::Group) && !owner.scoped_variables_available?
+            params.delete(:environment_scope)
+          end
+
+          params
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/api/helpers/variables_helpers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::API::Helpers::VariablesHelpers do
+  let(:klass) { Class.new.include(described_class).new }
+
+  describe '#filter_variable_parameters' do
+    let(:params) { { key: 'KEY', environment_scope: 'production' } }
+
+    subject { klass.filter_variable_parameters(owner, params) }
+
+    context 'owner is a project' do
+      let(:owner) { create(:project) }
+
+      it { is_expected.to eq(params) }
+    end
+
+    context 'owner is a group' do
+      let(:owner) { create(:group) }
+
+      before do
+        allow(owner).to receive(:scoped_variables_available?).and_return(scoped_variables_available)
+      end
+
+      context 'scoped variables are available' do
+        let(:scoped_variables_available) { true }
+
+        it { is_expected.to eq(params) }
+      end
+
+      context 'scoped variables are not available' do
+        let(:scoped_variables_available) { false }
+
+        it { is_expected.to eq(params.except(:environment_scope)) }
+      end
+    end
+  end
+end

ee/spec/models/group_spec.rb

@@ -676,6 +676,30 @@ RSpec.describe Group do
     end
   end
 
+  describe '#scoped_variables_available?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:group) { create(:group) }
+
+    subject { group.scoped_variables_available? }
+
+    where(:feature_enabled, :feature_available, :scoped_variables_available) do
+      true  | true  | true
+      false | true  | false
+      true  | false | false
+      false | false | false
+    end
+
+    with_them do
+      before do
+        stub_feature_flags(scoped_group_variables: feature_enabled)
+        stub_licensed_features(group_scoped_ci_variables: feature_available)
+      end
+
+      it { is_expected.to eq(scoped_variables_available) }
+    end
+  end
+
   describe '#minimal_access_role_allowed?' do
     subject { group.minimal_access_role_allowed? }
 

ee/spec/requests/api/group_variables_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::GroupVariables do
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+
+  describe 'GET /groups/:id/variables/:key' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+
+    before do
+      group.add_owner(user)
+    end
+
+    context 'when there are two variables with the same key on different environments' do
+      let!(:var1) { create(:ci_group_variable, group: group, key: 'key1', environment_scope: 'staging') }
+      let!(:var2) { create(:ci_group_variable, group: group, key: 'key1', environment_scope: 'production') }
+
+      context 'when filter[environment_scope] is not passed' do
+        it 'returns 409' do
+          get api("/groups/#{group.id}/variables/key1", user)
+
+          expect(response).to have_gitlab_http_status(:conflict)
+        end
+      end
+
+      context 'when filter[environment_scope] is passed' do
+        it 'returns the variable' do
+          get api("/groups/#{group.id}/variables/key1", user), params: { 'filter[environment_scope]': 'production' }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['value']).to eq(var2.value)
+        end
+      end
+
+      context 'when wrong filter[environment_scope] is passed' do
+        it 'returns not_found' do
+          get api("/groups/#{group.id}/variables/key1", user), params: { 'filter[environment_scope]': 'invalid' }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+
+        context 'when there is only one variable with provided key' do
+          it 'returns not_found' do
+            get api("/groups/#{group.id}/variables/#{variable.key}", user), params: { 'filter[environment_scope]': 'invalid' }
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
+    end
+  end
+
+  describe 'POST /groups/:id/variables' do
+    context 'scoped variables' do
+      let(:params) do
+        {
+          key: 'KEY',
+          value: 'VALUE',
+          environment_scope: 'production'
+        }
+      end
+
+      subject { post api("/groups/#{group.id}/variables", user), params: params }
+
+      before do
+        group.add_owner(user)
+        stub_licensed_features(group_scoped_ci_variables: scoped_variables_available)
+      end
+
+      context ':group_scoped_ci_variables licensed feature is available' do
+        let(:scoped_variables_available) { true }
+
+        it 'creates a variable with the provided environment scope' do
+          expect { subject }.to change { group.variables.count }.by(1)
+
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['environment_scope']).to eq('production')
+        end
+
+        context 'a variable with the same key and scope exists already' do
+          let!(:variable) { create(:ci_group_variable, group: group, key: 'KEY', environment_scope: 'production')}
+
+          it 'does not create a variable' do
+            expect { subject }.not_to change { group.variables.count }
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+          end
+        end
+      end
+
+      context ':group_scoped_ci_variables licensed feature is not available' do
+        let(:scoped_variables_available) { false }
+
+        it 'creates a variable, but does not use the provided environment scope' do
+          expect { subject }.to change { group.variables.count }.by(1)
+
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['environment_scope']).to eq('*')
+        end
+
+        context 'a variable with the same key and scope exists already' do
+          let!(:variable) { create(:ci_group_variable, group: group, key: 'KEY', environment_scope: '*')}
+
+          it 'does not create a variable' do
+            expect { subject }.not_to change { group.variables.count }
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+          end
+        end
+      end
+    end
+  end
+
+  describe 'PUT /groups/:id/variables/:key' do
+    let!(:variable) { create(:ci_group_variable, group: group, environment_scope: '*') }
+
+    subject { put api("/groups/#{group.id}/variables/#{variable.key}", user), params: { environment_scope: 'production' } }
+
+    context 'scoped variables' do
+      before do
+        group.add_owner(user)
+        stub_licensed_features(group_scoped_ci_variables: scoped_variables_available)
+      end
+
+      context ':group_scoped_ci_variables licensed feature is available' do
+        let(:scoped_variables_available) { true }
+
+        it 'updates the variable' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(variable.reload.environment_scope).to eq('production')
+          expect(json_response['environment_scope']).to eq('production')
+        end
+
+        context 'a variable with the same key and scope exists already' do
+          let!(:conflicting_variable) { create(:ci_group_variable, group: group, key: variable.key, environment_scope: 'production')}
+
+          it 'does not update the variable' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(variable.reload.environment_scope).to eq('*')
+          end
+        end
+      end
+
+      context ':group_scoped_ci_variables licensed feature is not available' do
+        let(:scoped_variables_available) { false }
+
+        it 'does not update the variable' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(variable.reload.environment_scope).to eq('*')
+          expect(json_response['environment_scope']).to eq('*')
+        end
+      end
+    end
+  end
+end

lib/api/group_variables.rb

@@ -8,6 +8,8 @@ module API
     before { authorize! :admin_group, user_group }
     feature_category :continuous_integration
 
+    helpers Helpers::VariablesHelpers
+
     params do
       requires :id, type: String, desc: 'The ID of a group'
     end
@@ -30,16 +32,13 @@ module API
       params do
         requires :key, type: String, desc: 'The key of the variable'
       end
-      # rubocop: disable CodeReuse/ActiveRecord
       get ':id/variables/:key' do
-        key = params[:key]
-        variable = user_group.variables.find_by(key: key)
+        variable = find_variable(user_group, params)
 
         break not_found!('GroupVariable') unless variable
 
         present variable, with: Entities::Ci::Variable
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Create a new variable in a group' do
         success Entities::Ci::Variable
@@ -50,12 +49,19 @@ module API
         optional :protected, type: String, desc: 'Whether the variable is protected'
         optional :masked, type: String, desc: 'Whether the variable is masked'
         optional :variable_type, type: String, values: ::Ci::GroupVariable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file. Defaults to env_var'
+
+        use :optional_group_variable_params_ee
       end
       post ':id/variables' do
+        filtered_params = filter_variable_parameters(
+          user_group,
+          declared_params(include_missing: false)
+        )
+
         variable = ::Ci::ChangeVariableService.new(
           container: user_group,
           current_user: current_user,
-          params: { action: :create, variable_params: declared_params(include_missing: false) }
+          params: { action: :create, variable_params: filtered_params }
         ).execute
 
         if variable.valid?
@@ -74,13 +80,19 @@ module API
         optional :protected, type: String, desc: 'Whether the variable is protected'
         optional :masked, type: String, desc: 'Whether the variable is masked'
         optional :variable_type, type: String, values: ::Ci::GroupVariable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file'
+
+        use :optional_group_variable_params_ee
       end
-      # rubocop: disable CodeReuse/ActiveRecord
       put ':id/variables/:key' do
+        filtered_params = filter_variable_parameters(
+          user_group,
+          declared_params(include_missing: false)
+        )
+
         variable = ::Ci::ChangeVariableService.new(
           container: user_group,
           current_user: current_user,
-          params: { action: :update, variable_params: declared_params(include_missing: false) }
+          params: { action: :update, variable_params: filtered_params }
         ).execute
 
         if variable.valid?
@@ -91,7 +103,6 @@ module API
       rescue ::ActiveRecord::RecordNotFound
         not_found!('GroupVariable')
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Delete an existing variable from a group' do
         success Entities::Ci::Variable
@@ -99,21 +110,18 @@ module API
       params do
         requires :key, type: String, desc: 'The key of the variable'
       end
-      # rubocop: disable CodeReuse/ActiveRecord
       delete ':id/variables/:key' do
-        variable = user_group.variables.find_by!(key: params[:key])
+        variable = find_variable(user_group, params)
+        break not_found!('GroupVariable') unless variable
 
         destroy_conditionally!(variable) do |target_variable|
           ::Ci::ChangeVariableService.new(
             container: user_group,
             current_user: current_user,
-            params: { action: :destroy, variable_params: declared_params(include_missing: false) }
+            params: { action: :destroy, variable: variable }
           ).execute
         end
-      rescue ::ActiveRecord::RecordNotFound
-        not_found!('GroupVariable')
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

lib/api/helpers/variables_helpers.rb

+# frozen_string_literal: true
+
+module API
+  module Helpers
+    module VariablesHelpers
+      extend ActiveSupport::Concern
+      extend Grape::API::Helpers
+
+      params :optional_group_variable_params_ee do
+      end
+
+      def filter_variable_parameters(_, params)
+        params # Overridden in EE
+      end
+
+      def find_variable(owner, params)
+        variables = ::Ci::VariablesFinder.new(owner, params).execute.to_a
+
+        return variables.first unless variables.many? # rubocop: disable CodeReuse/ActiveRecord
+
+        conflict!("There are multiple variables with provided parameters. Please use 'filter[environment_scope]'")
+      end
+    end
+  end
+end
+
+API::Helpers::VariablesHelpers.prepend_if_ee('EE::API::Helpers::VariablesHelpers')

lib/api/variables.rb

@@ -9,21 +9,7 @@ module API
 
     feature_category :continuous_integration
 
-    helpers do
-      def filter_variable_parameters(params)
-        # This method exists so that EE can more easily filter out certain
-        # parameters, without having to modify the source code directly.
-        params
-      end
-
-      def find_variable(params)
-        variables = ::Ci::VariablesFinder.new(user_project, params).execute.to_a
-
-        return variables.first unless variables.many? # rubocop: disable CodeReuse/ActiveRecord
-
-        conflict!("There are multiple variables with provided parameters. Please use 'filter[environment_scope]'")
-      end
-    end
+    helpers Helpers::VariablesHelpers
 
     params do
       requires :id, type: String, desc: 'The ID of a project'
@@ -49,7 +35,7 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ':id/variables/:key' do
-        variable = find_variable(params)
+        variable = find_variable(user_project, params)
         not_found!('Variable') unless variable
 
         present variable, with: Entities::Ci::Variable
@@ -71,7 +57,7 @@ module API
         variable = ::Ci::ChangeVariableService.new(
           container: user_project,
           current_user: current_user,
-          params: { action: :create, variable_params: filter_variable_parameters(declared_params(include_missing: false)) }
+          params: { action: :create, variable_params: declared_params(include_missing: false) }
         ).execute
 
         if variable.valid?
@@ -95,17 +81,13 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       put ':id/variables/:key' do
-        variable = find_variable(params)
+        variable = find_variable(user_project, params)
         not_found!('Variable') unless variable
 
-        variable_params = filter_variable_parameters(
-          declared_params(include_missing: false)
-            .except(:key, :filter)
-        )
         variable = ::Ci::ChangeVariableService.new(
           container: user_project,
           current_user: current_user,
-          params: { action: :update, variable: variable, variable_params: variable_params }
+          params: { action: :update, variable: variable, variable_params: declared_params(include_missing: false).except(:key, :filter) }
         ).execute
 
         if variable.valid?
@@ -125,7 +107,7 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       delete ':id/variables/:key' do
-        variable = find_variable(params)
+        variable = find_variable(user_project, params)
         not_found!('Variable') unless variable
 
         ::Ci::ChangeVariableService.new(

spec/finders/ci/variables_finder_spec.rb

@@ -3,42 +3,57 @@
 require 'spec_helper'
 
 RSpec.describe Ci::VariablesFinder do
-  let!(:project) { create(:project) }
-  let!(:params) { {} }
+  shared_examples 'scoped variables' do
+    describe '#initialize' do
+      subject { described_class.new(owner, params) }
 
-  let!(:var1) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'staging') }
-  let!(:var2) { create(:ci_variable, project: project, key: 'key2', environment_scope: 'staging') }
-  let!(:var3) { create(:ci_variable, project: project, key: 'key2', environment_scope: 'production') }
+      context 'without key filter' do
+        let!(:params) { {} }
 
-  describe '#initialize' do
-    subject { described_class.new(project, params) }
-
-    context 'without key filter' do
-      let!(:params) { {} }
-
-      it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError, 'Please provide params[:key]')
+        it 'raises an error' do
+          expect { subject }.to raise_error(ArgumentError, 'Please provide params[:key]')
+        end
       end
     end
-  end
 
-  describe '#execute' do
-    subject { described_class.new(project.reload, params).execute }
+    describe '#execute' do
+      subject { described_class.new(owner.reload, params).execute }
 
-    context 'with key filter' do
-      let!(:params) { { key: 'key1' } }
+      context 'with key filter' do
+        let!(:params) { { key: 'key1' } }
 
-      it 'returns var1' do
-        expect(subject).to contain_exactly(var1)
+        it 'returns var1' do
+          expect(subject).to contain_exactly(var1)
+        end
       end
-    end
 
-    context 'with key and environment_scope filter' do
-      let!(:params) { { key: 'key2', filter: { environment_scope: 'staging' } } }
+      context 'with key and environment_scope filter' do
+        let!(:params) { { key: 'key2', filter: { environment_scope: 'staging' } } }
 
-      it 'returns var2' do
-        expect(subject).to contain_exactly(var2)
+        it 'returns var2' do
+          expect(subject).to contain_exactly(var2)
+        end
       end
     end
   end
+
+  context 'for a project' do
+    let(:owner) { create(:project) }
+
+    let!(:var1) { create(:ci_variable, project: owner, key: 'key1', environment_scope: 'staging') }
+    let!(:var2) { create(:ci_variable, project: owner, key: 'key2', environment_scope: 'staging') }
+    let!(:var3) { create(:ci_variable, project: owner, key: 'key2', environment_scope: 'production') }
+
+    include_examples 'scoped variables'
+  end
+
+  context 'for a group' do
+    let(:owner) { create(:group) }
+
+    let!(:var1) { create(:ci_group_variable, group: owner, key: 'key1', environment_scope: 'staging') }
+    let!(:var2) { create(:ci_group_variable, group: owner, key: 'key2', environment_scope: 'staging') }
+    let!(:var3) { create(:ci_group_variable, group: owner, key: 'key2', environment_scope: 'production') }
+
+    include_examples 'scoped variables'
+  end
 end

spec/lib/api/helpers/variables_helpers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Helpers::VariablesHelpers do
+  let(:helper) { Class.new.include(described_class).new }
+
+  describe '#filter_variable_parameters' do
+    let(:project) { double }
+    let(:params) { double }
+
+    subject { helper.filter_variable_parameters(project, params) }
+
+    it 'returns unmodified params (overridden in EE)' do
+      expect(subject).to eq(params)
+    end
+  end
+
+  describe '#find_variable' do
+    let(:owner) { double }
+    let(:params) { double }
+    let(:variables) { [double] }
+
+    subject { helper.find_variable(owner, params) }
+
+    before do
+      expect(Ci::VariablesFinder).to receive(:new).with(owner, params)
+        .and_return(double(execute: variables))
+    end
+
+    it { is_expected.to eq(variables.first) }
+
+    context 'there are multiple variables with the supplied key' do
+      let(:variables) { [double, double] }
+
+      it 'raises a conflict!' do
+        expect(helper).to receive(:conflict!).with(/There are multiple variables with provided parameters/)
+
+        subject
+      end
+    end
+  end
+end

spec/requests/api/group_variables_spec.rb

@@ -55,6 +55,7 @@ RSpec.describe API::GroupVariables do
         expect(json_response['value']).to eq(variable.value)
         expect(json_response['protected']).to eq(variable.protected?)
         expect(json_response['variable_type']).to eq(variable.variable_type)
+        expect(json_response['environment_scope']).to eq(variable.environment_scope)
       end
 
       it 'responds with 404 Not Found if requesting non-existing variable' do
@@ -98,6 +99,7 @@ RSpec.describe API::GroupVariables do
         expect(json_response['protected']).to be_truthy
         expect(json_response['masked']).to be_truthy
         expect(json_response['variable_type']).to eq('env_var')
+        expect(json_response['environment_scope']).to eq('*')
       end
 
       it 'creates variable with optional attributes' do
@@ -111,6 +113,7 @@ RSpec.describe API::GroupVariables do
         expect(json_response['protected']).to be_falsey
         expect(json_response['masked']).to be_falsey
         expect(json_response['variable_type']).to eq('file')
+        expect(json_response['environment_scope']).to eq('*')
       end
 
       it 'does not allow to duplicate variable key' do