Add API management for scoped group variables

Extends the existing group variables API to support
EE-only params, and allows setting the environment_scope
if the :group_scoped_ci_variables licensed feature is available.

If the feature is not available (pricing tier is not Premium or
higher), the param is silently removed and the update continues
without presenting an error to the user. This behaviour is the
same as when project level scoped variables were a premium feature.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/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