Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/pages/dashboard/todos/index/todos.js

 /* eslint-disable class-methods-use-this, no-unneeded-ternary */
 
 import $ from 'jquery';
+import { getGroups } from '~/api/groups_api';
+import { getProjects } from '~/api/projects_api';
 import initDeprecatedJQueryDropdown from '~/deprecated_jquery_dropdown';
 import { deprecatedCreateFlash as flash } from '~/flash';
 import axios from '~/lib/utils/axios_utils';
@@ -41,14 +43,37 @@ export default class Todos {
   }
 
   initFilters() {
-    this.initFilterDropdown($('.js-group-search'), 'group_id', ['text']);
-    this.initFilterDropdown($('.js-project-search'), 'project_id', ['text']);
+    this.initAjaxFilterDropdown(getGroups, $('.js-group-search'), 'group_id');
+    this.initAjaxFilterDropdown(getProjects, $('.js-project-search'), 'project_id');
     this.initFilterDropdown($('.js-type-search'), 'type');
     this.initFilterDropdown($('.js-action-search'), 'action_id');
 
     return new UsersSelect();
   }
 
+  initAjaxFilterDropdown(apiMethod, $dropdown, fieldName) {
+    initDeprecatedJQueryDropdown($dropdown, {
+      fieldName,
+      selectable: true,
+      filterable: true,
+      filterRemote: true,
+      data(search, callback) {
+        return apiMethod(search, {}, (data) => {
+          callback(
+            data.map((d) => ({
+              id: d.id,
+              text: d.full_name || d.name_with_namespace,
+            })),
+          );
+        });
+      },
+      clicked: () => {
+        const $formEl = $dropdown.closest('form.filter-form');
+        $formEl.submit();
+      },
+    });
+  }
+
   initFilterDropdown($dropdown, fieldName, searchFields) {
     initDeprecatedJQueryDropdown($dropdown, {
       fieldName,
@@ -58,12 +83,6 @@ export default class Todos {
       data: $dropdown.data('data'),
       clicked: () => {
         const $formEl = $dropdown.closest('form.filter-form');
-        const mutexDropdowns = {
-          group_id: 'project_id',
-          project_id: 'group_id',
-        };
-
-        $formEl.find(`input[name="${mutexDropdowns[fieldName]}"]`).remove();
         $formEl.submit();
       },
     });

app/assets/stylesheets/pages/tree.scss

@@ -22,7 +22,7 @@
 
         .control {
           float: left;
-          margin-left: 10px;
+          margin-left: 8px;
         }
       }
 

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/helpers/todos_helper.rb

@@ -157,16 +157,6 @@ module TodosHelper
     ]
   end
 
-  def todo_projects_options
-    projects = current_user.authorized_projects.sorted_by_activity.non_archived.with_route
-
-    projects = projects.map do |project|
-      { id: project.id, text: project.full_name }
-    end
-
-    projects.unshift({ id: '', text: 'Any Project' }).to_json
-  end
-
   def todo_types_options
     [
       { id: '', text: 'Any Type' },
@@ -240,14 +230,6 @@ module TodosHelper
       false
     end
   end
-
-  def todo_group_options
-    groups = current_user.authorized_groups.with_route.map do |group|
-      { id: group.id, text: group.full_name }
-    end
-
-    groups.unshift({ id: '', text: 'Any Group' }).to_json
-  end
 end
 
 TodosHelper.prepend_if_ee('EE::TodosHelper')

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')

app/views/dashboard/todos/index.html.haml

@@ -42,12 +42,12 @@
             - if params[:group_id].present?
               = hidden_field_tag(:group_id, params[:group_id])
             = dropdown_tag(group_dropdown_label(params[:group_id], 'Group'), options: { toggle_class: 'js-group-search js-filter-submit', title: 'Filter by group', filter: true, filterInput: 'input#group-search', dropdown_class: 'dropdown-menu-selectable dropdown-menu-group js-filter-submit',
-              placeholder: 'Search groups', data: { data: todo_group_options, default_label: 'Group', display: 'static' } })
+              placeholder: 'Search groups', data: { default_label: 'Group', display: 'static' } })
           .filter-item.inline
             - if params[:project_id].present?
               = hidden_field_tag(:project_id, params[:project_id])
             = dropdown_tag(project_dropdown_label(params[:project_id], 'Project'), options: { toggle_class: 'js-project-search js-filter-submit', title: 'Filter by project', filter: true, filterInput: 'input#project-search', dropdown_class: 'dropdown-menu-selectable dropdown-menu-project js-filter-submit',
-              placeholder: 'Search projects', data: { data: todo_projects_options, default_label: 'Project', display: 'static' } })
+              placeholder: 'Search projects', data: { default_label: 'Project', display: 'static' } })
           .filter-item.inline
             - if params[:author_id].present?
               = hidden_field_tag(:author_id, params[:author_id])

changelogs/unreleased/273300-fy21q4-foundations-kr2-audit-and-update-buttons-on-projects-commit.yml

+---
+title: Decrease spacing between controls on the Commit page header
+merge_request: 56129
+author:
+type: other

changelogs/unreleased/ph-todosProjectGroupDropdownApiCall.yml

+---
+title: Move fetching projects and groups on todos page to API call
+merge_request: 56507
+author:
+type: performance

doc/.vale/gitlab/ReferenceLinks.yml

@@ -10,4 +10,4 @@ link: https://docs.gitlab.com/ee/development/documentation/styleguide/index.html
 level: error
 scope: raw
 raw:
-  - '\n\[.*\]: .*'
+  - '\n\[[^\]]*\]: .*'

doc/user/application_security/vulnerability_report/index.md

@@ -119,12 +119,14 @@ Hover over an **Activity** entry and select a link go to that issue.
 
 ## Change status of vulnerabilities
 
+> The option to select a status other than Dismissed was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/292636) in GitLab 13.10.
+
 To change the status of vulnerabilities in the table:
 
 1. Select the checkbox for each vulnerability you want to update the status of.
 1. In the dropdown that appears select the desired status, then select **Change status**.
 
-![Project Vulnerability Report](img/project_security_dashboard_status_change_v13_9.png)
+![Project Vulnerability Report](img/project_security_dashboard_status_change_v13_10.png)
 
 ## Export vulnerability details
 

ee/app/assets/javascripts/on_demand_scans/components/profile_selector/profile_selector.vue

@@ -116,7 +116,7 @@ export default {
       </gl-dropdown>
 
       <div
-        v-if="value && $scopedSlots.summary"
+        v-if="selectedProfile && $scopedSlots.summary"
         data-testid="selected-profile-summary"
         class="gl-mt-6 gl-pt-6 gl-border-t-solid gl-border-gray-100 gl-border-t-1"
       >

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/changelogs/unreleased/djadmin-fix-selected-profile-section.yml

+---
+title: Fix DAST profiles summary for invalid profile ids
+merge_request: 56753
+author:
+type: fixed

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/frontend/on_demand_scans/components/on_demand_scans_form_spec.js

@@ -82,6 +82,7 @@ describe('OnDemandScansForm', () => {
   const findSubmitButton = () => findByTestId('on-demand-scan-submit-button');
   const findSaveButton = () => findByTestId('on-demand-scan-save-button');
   const findCancelButton = () => findByTestId('on-demand-scan-cancel-button');
+  const findProfileSummary = () => findByTestId('selected-profile-summary');
 
   const setValidFormData = () => {
     findNameInput().vm.$emit('input', 'My daily scan');
@@ -102,6 +103,12 @@ describe('OnDemandScansForm', () => {
     });
     return setValidFormData();
   };
+  const selectProfile = (component) => async (profile) => {
+    subject.find(component).vm.$emit('input', profile.id);
+    await subject.vm.$nextTick();
+  };
+  const selectScannerProfile = selectProfile(ScannerProfileSelector);
+  const selectSiteProfile = selectProfile(SiteProfileSelector);
 
   const submitForm = () => findForm().vm.$emit('submit', { preventDefault: () => {} });
   const saveScan = () => findSaveButton().vm.$emit('click');
@@ -515,14 +522,27 @@ describe('OnDemandScansForm', () => {
     });
   });
 
+  describe('scanner profile summary', () => {
+    beforeEach(() => {
+      mountSubject({
+        provide: {
+          glFeatures: {
+            securityDastSiteProfilesAdditionalFields: true,
+          },
+        },
+      });
+    });
+
+    it('does not render the summary provided an invalid profile ID', async () => {
+      await selectScannerProfile({ id: 'gid://gitlab/DastScannerProfile/123' });
+
+      expect(findProfileSummary().exists()).toBe(false);
+    });
+  });
+
   describe('site profile summary', () => {
     const [authEnabledProfile] = siteProfiles;
 
-    const selectSiteProfile = async (profile) => {
-      subject.find(SiteProfileSelector).vm.$emit('input', profile.id);
-      await subject.vm.$nextTick();
-    };
-
     beforeEach(() => {
       mountSubject({
         provide: {
@@ -548,6 +568,12 @@ describe('OnDemandScansForm', () => {
       expect(summary).toMatch(defaultPassword);
       expect(summary).toMatch(defaultRequestHeaders);
     });
+
+    it('does not render the summary provided an invalid profile ID', async () => {
+      await selectSiteProfile({ id: 'gid://gitlab/DastSiteProfile/123' });
+
+      expect(findProfileSummary().exists()).toBe(false);
+    });
   });
 
   describe('populate profiles from query params', () => {

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/helpers/todos_helper_spec.rb

@@ -40,26 +40,6 @@ RSpec.describe TodosHelper do
     end
   end
 
-  describe '#todo_projects_options' do
-    let(:projects) { create_list(:project, 3) }
-    let(:user)     { create(:user) }
-
-    it 'returns users authorised projects in json format' do
-      projects.first.add_developer(user)
-      projects.second.add_developer(user)
-
-      allow(helper).to receive(:current_user).and_return(user)
-
-      expected_results = [
-        { 'id' => '', 'text' => 'Any Project' },
-        { 'id' => projects.second.id, 'text' => projects.second.full_name },
-        { 'id' => projects.first.id, 'text' => projects.first.full_name }
-      ]
-
-      expect(Gitlab::Json.parse(helper.todo_projects_options)).to match_array(expected_results)
-    end
-  end
-
   describe '#todo_target_link' do
     context 'when given a design' do
       let(:todo) { design_todo }

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