Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/alerts_settings/components/alerts_settings_form.vue

@@ -223,6 +223,10 @@ export default {
     testAlertModal() {
       return this.isFormDirty ? testAlertModalId : null;
     },
+    prometheusUrlInvalidFeedback() {
+      const { blankUrlError, invalidUrlError } = i18n.integrationFormSteps.prometheusFormUrl;
+      return this.integrationForm.apiUrl?.length ? invalidUrlError : blankUrlError;
+    },
   },
   watch: {
     tabIndex(val) {
@@ -288,6 +292,9 @@ export default {
       if (this.isHttp) {
         this.validationState.apiUrl = true;
         this.validateName();
+        if (!this.validationState.name) {
+          this.$refs.integrationName.$el.scrollIntoView({ behavior: 'smooth', block: 'center' });
+        }
       } else if (this.isPrometheus) {
         this.validationState.name = true;
         this.validateApiUrl();
@@ -300,6 +307,11 @@ export default {
       this.$emit('save-and-test-alert-payload', this.dataForSave, this.testAlertPayload);
     },
     submit(testAfterSubmit = false) {
+      this.triggerValidation();
+
+      if (!this.isFormValid) {
+        return;
+      }
       const event = this.currentIntegration ? 'update-integration' : 'create-new-integration';
       this.$emit(event, this.dataForSave, testAfterSubmit);
     },
@@ -412,7 +424,6 @@ export default {
             :disabled="isSelectDisabled"
             class="gl-max-w-full"
             :options="integrationTypesOptions"
-            @change="triggerValidation"
           />
 
           <alert-settings-form-help-block
@@ -439,6 +450,7 @@ export default {
           >
             <gl-form-input
               id="name-integration"
+              ref="integrationName"
               v-model="integrationForm.name"
               type="text"
               :placeholder="$options.i18n.integrationFormSteps.nameIntegration.placeholder"
@@ -473,7 +485,7 @@ export default {
             class="gl-my-4"
             :label="$options.i18n.integrationFormSteps.prometheusFormUrl.label"
             label-for="api-url"
-            :invalid-feedback="$options.i18n.integrationFormSteps.prometheusFormUrl.error"
+            :invalid-feedback="prometheusUrlInvalidFeedback"
             :state="validationState.apiUrl"
           >
             <gl-form-input

app/assets/javascripts/alerts_settings/constants.js

@@ -70,7 +70,8 @@ export const i18n = {
     prometheusFormUrl: {
       label: s__('AlertSettings|Prometheus API base URL'),
       help: s__('AlertSettings|URL cannot be blank and must start with http or https'),
-      error: s__('AlertSettings|URL is invalid.'),
+      blankUrlError: __('URL cannot be blank'),
+      invalidUrlError: __('URL is invalid'),
     },
     restKeyInfo: {
       label: s__(

changelogs/unreleased/21044-controller-profiles-slackscontroller-edit-executes-more-than-100-sq.yml

+---
+title: Reduce number of SQL queries in Profiles::SlacksController#edit
+merge_request: 57674
+author:
+type: performance

changelogs/unreleased/325820-update-validation-trigger.yml

+---
+title: Update validation trigger flow on the alerts integration form
+merge_request: 57697
+author:
+type: changed

doc/user/application_security/sast/analyzers.md

@@ -43,6 +43,18 @@ dedicated containers for each analysis.
 SAST is pre-configured with a set of **default images** that are maintained by
 GitLab, but users can also integrate their own **custom images**.
 
+## SAST analyzer features
+
+For an analyzer to be considered Generally Available, it is expected to minimally 
+support the following features:
+
+- [Customizable configuration](index.md#available-variables)
+- [Customizable rulesets](index.md#customize-rulesets)
+- [Scan projects](index.md#supported-languages-and-frameworks)
+- [Multi-project support](index.md#multi-project-support)
+- [Offline support](index.md#running-sast-in-an-offline-environment)
+- [Emits JSON report format](index.md#reports-json-format)
+
 ## Official default analyzers
 
 Any custom change to the official analyzers can be achieved by using a

doc/user/project/issues/related_issues.md

@@ -6,7 +6,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w
 
 # Related issues **(FREE)**
 
-> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1797) in [GitLab Starter](https://about.gitlab.com/pricing/) 9.4.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1797) in GitLab 9.4.
 > - The simple "relates to" relationship [moved](https://gitlab.com/gitlab-org/gitlab/-/issues/212329) to [GitLab Free](https://about.gitlab.com/pricing/) in 13.4.
 
 Related issues are a bi-directional relationship between any two issues
@@ -16,8 +16,8 @@ and projects.
 You can set any issue as:
 
 - Related to another issue
-- Blocking another issue **(STARTER)**
-- Blocked by another issue **(STARTER)**
+- Blocking another issue **(PREMIUM)**
+- Blocked by another issue **(PREMIUM)**
 
 The relationship only shows up in the UI if the user can see both issues.
 
@@ -28,28 +28,30 @@ To manage related issues through our API, visit the [issue links API documentati
 
 ## Adding a related issue
 
-> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/2035) in [GitLab Starter](https://about.gitlab.com/pricing/) 12.8.
-> - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/34239) to warn when attempting to close an issue that is blocked by others in [GitLab Starter](https://about.gitlab.com/pricing/) 13.0.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/2035) in GitLab 12.8.
+> - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/34239) to warn when attempting to close an issue that is blocked by others in GitLab 13.0.
 >   When you try to close an issue with open blockers, you see a warning that you can dismiss.
 
 1. Relate one issue to another by clicking the related issues "+" button
 in the header of the related issue block.
 
+1. Select the relationship the between the two issues. Either:
+   - **relates to**.
+   - **blocks**. **(PREMIUM)**
+   - **is blocked by**. **(PREMIUM)**
 1. Input the issue reference number or paste in the full URL of the issue.
 
-1. **(STARTER)** Select whether the current issue relates to, blocks, or is blocked by the issues being entered.
+   ![Adding a related issue](img/related_issues_add_v12_8.png)
 
-    ![Adding a related issue](img/related_issues_add_v12_8.png)
+   Issues of the same project can be specified just by the reference number.
+   Issues from a different project require additional information like the
+   group and the project name. For example:
 
-    Issues of the same project can be specified just by the reference number.
-    Issues from a different project require additional information like the
-    group and the project name. For example:
+   - The same project: `#44`
+   - The same group: `project#44`
+   - Different group: `group/project#44`
 
-    - same project: `#44`
-    - same group: `project#44`
-    - different group: `group/project#44`
-
-    Valid references are added to a temporary list that you can review.
+   Valid references are added to a temporary list that you can review.
 
 1. When you have added all the related issues, click **Add** to submit.
 
@@ -60,7 +62,7 @@ them categorized so their relationships can be better understood visually.
 
 ## Removing a related issue
 
-In the related issues block, click the "x" icon on the right-side of each issue
+In the related issues block, click the remove button (**{close}**) on the right-side of each issue
 token that you wish to remove.
 
 Due to the bi-directional relationship, it no longer appears in either issue.

ee/app/controllers/profiles/slacks_controller.rb

@@ -10,7 +10,7 @@ class Profiles::SlacksController < Profiles::ApplicationController
   feature_category :users
 
   def edit
-    @projects = disabled_projects if current_user
+    @projects = disabled_projects.select([:id, :name]) if current_user
   end
 
   def slack_link

ee/app/helpers/ee/services_helper.rb

@@ -36,7 +36,7 @@ module EE
 
     def add_to_slack_data(projects)
       {
-        projects: projects,
+        projects: projects.as_json(only: [:id, :name]),
         sign_in_path: new_session_path(:user, redirect_to_referer: 'yes'),
         is_signed_in: current_user.present?,
         slack_link_profile_slack_path: slack_link_profile_slack_path,

ee/changelogs/unreleased/sfang-deprovision-service-error.yml

+---
+title: Allow SCIM deprovisioning for minimal access users
+merge_request: 57178
+author:
+type: fixed

ee/lib/api/scim.rb

@@ -69,7 +69,7 @@ module API
           parsed_hash = parser.update_params
 
           if parser.deprovision_user?
-            deprovision(identity)
+            patch_deprovision(identity)
           elsif reprovisionable?(identity) && parser.reprovision_user?
             reprovision(identity)
           elsif parsed_hash[:extern_uid]
@@ -103,14 +103,28 @@ module API
           group.scim_identities.with_extern_uid(extern_uid).first
         end
 
-        def deprovision(identity)
-          ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
+        # delete_deprovision handles the response and returns either no_content! or a detailed error message.
+        def delete_deprovision(identity)
+          service = ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
 
-          true
-        rescue => e
-          logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
+          if service.success?
+            no_content!
+          else
+            logger.error(identity: identity, error: service.class.name, message: service.message, source: "#{__FILE__}:#{__LINE__}")
+            scim_error!(message: service.message)
+          end
+        end
 
-          false
+        # The method that calls patch_deprovision, update_scim_user, expects a truthy/falsey value, and then continues to handle the request.
+        def patch_deprovision(identity)
+          service = ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
+
+          if service.success?
+            true
+          else
+            logger.error(identity: identity, error: service.class.name, message: service.message, source: "#{__FILE__}:#{__LINE__}")
+            false
+          end
         end
 
         def reprovision(identity)
@@ -212,11 +226,7 @@ module API
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
-          destroyed = deprovision(identity)
-
-          scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed
-
-          no_content!
+          delete_deprovision(identity)
         end
       end
     end

ee/lib/ee/gitlab/scim/deprovision_service.rb

@@ -13,23 +13,29 @@ module EE
         end
 
         def execute
+          return error(_("Could not remove %{user} from %{group}. User is not a group member.") % { user: user.name, group: group.name }) unless group_membership
+          return error(_("Could not remove %{user} from %{group}. Cannot remove last group owner.") % { user: user.name, group: group.name }) if group.last_owner?(user)
+
           ScimIdentity.transaction do
             identity.update!(active: false)
             remove_group_access
           end
+
+          ServiceResponse.success(message: _("User %{user} was removed from %{group}.") % { user: user.name, group: group.name })
         end
 
         private
 
         def remove_group_access
-          return unless group_membership
-          return if group.last_owner?(user)
-
           ::Members::DestroyService.new(user).execute(group_membership)
         end
 
         def group_membership
-          @group_membership ||= group.group_member(user)
+          @group_membership ||= group.all_group_members.with_user(user).first
+        end
+
+        def error(message)
+          ServiceResponse.error(message: message)
         end
       end
     end

ee/spec/helpers/ee/services_helper_spec.rb

@@ -3,9 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe EE::ServicesHelper do
+  include Devise::Test::ControllerHelpers
+
   let(:controller_class) do
     Class.new(ActionController::Base) do
       include EE::ServicesHelper
+      include ActionView::Helpers::AssetUrlHelper
 
       def slack_auth_project_settings_slack_url(project)
         "http://some-path/project/1"
@@ -111,4 +114,54 @@ RSpec.describe EE::ServicesHelper do
       is_expected.to include(:issues_show_path)
     end
   end
+
+  describe '#add_to_slack_data' do
+    let_it_be(:projects) { create_list(:project, 3) }
+
+    def relation
+      Project.id_in(projects.pluck(:id))
+    end
+
+    let(:request) do
+      double(:Request,
+             optional_port: nil,
+             path_parameters: {},
+             protocol: 'https',
+             routes: nil,
+             env: { 'warden' => warden },
+             engine_script_name: nil,
+             original_script_name: nil,
+             host: 'example.com')
+    end
+
+    before do
+      allow(subject).to receive(:request).and_return(request)
+    end
+
+    it 'includes the required keys' do
+      additions = Gitlab::Json.parse(subject.add_to_slack_data(relation))
+      expect(additions.keys).to match_array %w[
+        projects
+        sign_in_path
+        is_signed_in
+        slack_link_profile_slack_path
+        gitlab_for_slack_gif_path
+        gitlab_logo_path
+        slack_logo_path
+        docs_path
+      ]
+    end
+
+    it 'does not suffer from N+1 performance issues' do
+      baseline = ActiveRecord::QueryRecorder.new { subject.add_to_slack_data(relation.limit(1)) }
+
+      expect do
+        subject.add_to_slack_data(relation)
+      end.not_to exceed_query_limit(baseline)
+    end
+
+    it 'serializes nil projects without error' do
+      expect(subject.add_to_slack_data(nil)).to include('"projects":null')
+    end
+  end
 end

ee/spec/lib/ee/gitlab/scim/deprovision_service_spec.rb

@@ -4,36 +4,89 @@ require 'spec_helper'
 
 RSpec.describe ::EE::Gitlab::Scim::DeprovisionService do
   describe '#execute' do
-    let_it_be(:identity) { create(:scim_identity, active: true) }
-    let_it_be(:group) { identity.group }
-    let_it_be(:user) { identity.user }
+    let(:identity) { create(:scim_identity, active: true) }
+    let(:group) { identity.group }
+    let(:user) { identity.user }
 
     let(:service) { described_class.new(identity) }
 
-    it 'deactivates scim identity' do
-      service.execute
+    context 'when user is successfully removed' do
+      before do
+        create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
+      end
 
-      expect(identity.active).to be false
-    end
+      it 'deactivates scim identity' do
+        expect { service.execute }.to change { identity.active }.from(true).to(false)
+      end
+
+      it 'removes group access' do
+        service.execute
 
-    it 'removes group access' do
-      create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
+        expect(group.all_group_members.pluck(:user_id)).not_to include(user.id)
+      end
 
-      service.execute
+      it 'returns the successful deprovision message' do
+        response = service.execute
 
-      expect(group.members.pluck(:user_id)).not_to include(user.id)
+        expect(response.message).to include("User #{user.name} was removed from #{group.name}.")
+      end
     end
 
-    it 'does not remove the last owner' do
-      create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
+    context 'with minimal access role' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, group: group, user: user, access_level: ::Gitlab::Access::MINIMAL_ACCESS)
+      end
+
+      it 'deactivates scim identity' do
+        expect { service.execute }.to change { identity.active }.from(true).to(false)
+      end
 
-      service.execute
+      it 'removes group access' do
+        service.execute
 
-      expect(identity.group.members.pluck(:user_id)).to include(user.id)
+        expect(group.all_group_members.pluck(:user_id)).not_to include(user.id)
+      end
+
+      it 'returns the successful deprovision message' do
+        response = service.execute
+
+        expect(response.message).to include("User #{user.name} was removed from #{group.name}.")
+      end
     end
 
-    it 'does not change group membership when the user is not a member' do
-      expect { service.execute }.not_to change { group.members.count }
+    context 'when user is not successfully removed' do
+      context 'when user is the last owner' do
+        before do
+          create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
+        end
+
+        it 'does not remove the last owner' do
+          service.execute
+
+          expect(identity.group.members.pluck(:user_id)).to include(user.id)
+        end
+
+        it 'returns the last group owner error' do
+          response = service.execute
+
+          expect(response.error?).to be true
+          expect(response.errors).to include("Could not remove #{user.name} from #{group.name}. Cannot remove last group owner.")
+        end
+      end
+
+      context 'when user is not a group member' do
+        it 'does not change group membership when the user is not a member' do
+          expect { service.execute }.not_to change { group.members.count }
+        end
+
+        it 'returns the group membership error' do
+          response = service.execute
+
+          expect(response.error?).to be true
+          expect(response.errors).to include("Could not remove #{user.name} from #{group.name}. User is not a group member.")
+        end
+      end
     end
   end
 end

ee/spec/lib/ee/gitlab/scim/reprovision_service_spec.rb

@@ -38,5 +38,16 @@ RSpec.describe ::EE::Gitlab::Scim::ReprovisionService do
 
       expect { service.execute }.not_to change { group.members.count }
     end
+
+    context 'with minimal access user' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, group: group, user: user, access_level: ::Gitlab::Access::MINIMAL_ACCESS)
+      end
+
+      it 'does not change group membership when the user is already a member' do
+        expect { service.execute }.not_to change { group.all_group_members.count }
+      end
+    end
   end
 end

ee/spec/requests/api/scim_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe API::Scim do
 
   before do
     stub_licensed_features(group_allowed_email_domains: true, group_saml: true)
-    group.add_owner(user)
+    group.add_developer(user)
     create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER)
   end
 
@@ -337,6 +337,27 @@ RSpec.describe API::Scim do
         expect(identity.reload.active).to be false
       end
 
+      context 'with owner' do
+        params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
+
+        before do
+          group.add_owner(user)
+          call_patch_api(params)
+        end
+
+        it 'responds with 412' do
+          expect(response).to have_gitlab_http_status(:precondition_failed)
+        end
+
+        it 'returns the last group owner error' do
+          expect(response.body).to include("Error updating #{user.name}")
+        end
+
+        it 'does not deactivate the identity' do
+          expect(identity.reload.active).to be true
+        end
+      end
+
       context 'Reprovision user' do
         let_it_be(:params) { { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'true' }] }.to_query }
 
@@ -450,6 +471,25 @@ RSpec.describe API::Scim do
         end
       end
 
+      context 'with owner' do
+        before do
+          group.add_owner(user)
+          delete scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
+        end
+
+        it 'responds with 412' do
+          expect(response).to have_gitlab_http_status(:precondition_failed)
+        end
+
+        it 'returns the last group owner error' do
+          expect(response.body).to include("Could not remove #{user.name} from #{group.name}. Cannot remove last group owner.")
+        end
+
+        it 'does not deactivate the identity' do
+          expect(identity.reload.active).to be true
+        end
+      end
+
       it 'responds with 404 if there is no user' do
         delete scim_api("scim/v2/groups/#{group.full_path}/Users/123")
 

locale/gitlab.pot

@@ -3005,9 +3005,6 @@ msgstr ""
 msgid "AlertSettings|URL cannot be blank and must start with http or https"
 msgstr ""
 
-msgid "AlertSettings|URL is invalid."
-msgstr ""
-
 msgid "AlertSettings|Utilize the URL and authorization key below to authorize Prometheus to send alerts to GitLab. Review the Prometheus documentation to learn where to add these details, and the %{linkStart}GitLab documentation%{linkEnd} to learn more about configuring your endpoint."
 msgstr ""
 
@@ -8726,6 +8723,12 @@ msgstr ""
 msgid "Could not load usage counts. Please refresh the page to try again."
 msgstr ""
 
+msgid "Could not remove %{user} from %{group}. Cannot remove last group owner."
+msgstr ""
+
+msgid "Could not remove %{user} from %{group}. User is not a group member."
+msgstr ""
+
 msgid "Could not remove the trigger."
 msgstr ""
 
@@ -32403,6 +32406,12 @@ msgstr ""
 msgid "URL"
 msgstr ""
 
+msgid "URL cannot be blank"
+msgstr ""
+
+msgid "URL is invalid"
+msgstr ""
+
 msgid "URL is required"
 msgstr ""
 
@@ -33165,6 +33174,9 @@ msgstr ""
 msgid "User %{username} was successfully removed."
 msgstr ""
 
+msgid "User %{user} was removed from %{group}."
+msgstr ""
+
 msgid "User ID"
 msgstr ""
 

spec/frontend/alerts_settings/components/alerts_settings_form_spec.js

@@ -10,6 +10,9 @@ import alertFields from '../mocks/alert_fields.json';
 import parsedMapping from '../mocks/parsed_mapping.json';
 import { defaultAlertSettingsConfig } from './util';
 
+const scrollIntoViewMock = jest.fn();
+HTMLElement.prototype.scrollIntoView = scrollIntoViewMock;
+
 describe('AlertsSettingsForm', () => {
   let wrapper;
   const mockToastShow = jest.fn();
@@ -410,33 +413,35 @@ describe('AlertsSettingsForm', () => {
       createComponent();
     });
 
-    it('should not be able to submit when no integration type is selected', () => {
-      selectOptionAtIndex(0);
+    it('should not be able to submit when no integration type is selected', async () => {
+      await selectOptionAtIndex(0);
 
       expect(findSubmitButton().attributes('disabled')).toBe('disabled');
     });
 
-    it('should not be able to submit when HTTP integration form is invalid', () => {
-      selectOptionAtIndex(1);
-
+    it('should not be able to submit when HTTP integration form is invalid', async () => {
+      await selectOptionAtIndex(1);
+      await findFormFields().at(0).vm.$emit('input', '');
       expect(findSubmitButton().attributes('disabled')).toBe('disabled');
     });
 
     it('should be able to submit when HTTP integration  form is valid', async () => {
       await selectOptionAtIndex(1);
-      await findFormFields().at(0).setValue('Name');
+      await findFormFields().at(0).vm.$emit('input', 'Name');
       expect(findSubmitButton().attributes('disabled')).toBe(undefined);
     });
 
-    it('should not be able to submit when Prometheus integration form is invalid', () => {
-      selectOptionAtIndex(2);
+    it('should not be able to submit when Prometheus integration form is invalid', async () => {
+      await selectOptionAtIndex(2);
+      await findFormFields().at(0).vm.$emit('input', '');
 
       expect(findSubmitButton().attributes('disabled')).toBe('disabled');
     });
 
     it('should be able to submit when Prometheus integration  form is valid', async () => {
       await selectOptionAtIndex(2);
-      await findFormFields().at(0).setValue('http://valid.url');
+      await findFormFields().at(0).vm.$emit('input', 'http://valid.url');
+
       expect(findSubmitButton().attributes('disabled')).toBe(undefined);
     });
 
@@ -445,7 +450,7 @@ describe('AlertsSettingsForm', () => {
         currentIntegration: { type: typeSet.http, name: 'Existing integration' },
       });
       await nextTick();
-      await findFormFields().at(0).setValue('Updated name');
+      await findFormFields().at(0).vm.$emit('input', 'Updated name');
 
       expect(findSubmitButton().attributes('disabled')).toBe(undefined);
     });
@@ -458,5 +463,20 @@ describe('AlertsSettingsForm', () => {
 
       expect(findSubmitButton().attributes('disabled')).toBe('disabled');
     });
+
+    it('should disable submit button after click on validation failure', async () => {
+      await selectOptionAtIndex(1);
+      findSubmitButton().trigger('click');
+      await nextTick();
+
+      expect(findSubmitButton().attributes('disabled')).toBe('disabled');
+    });
+
+    it('should scroll to invalid field on validation failure', async () => {
+      await selectOptionAtIndex(1);
+      findSubmitButton().trigger('click');
+
+      expect(scrollIntoViewMock).toHaveBeenCalledWith({ behavior: 'smooth', block: 'center' });
+    });
   });
 });