Automatic merge of gitlab-org/gitlab master

changelogs/unreleased/202423-foreign-key-webhooks-groups.yml

+---
+title: Add foreign key from web_hooks to groups
+merge_request: 57735
+author:
+type: other

changelogs/unreleased/sh-log-upload-api-exceed-size.yml

+---
+title: Log message when upload via API exceeds limit
+merge_request: 57774
+author:
+type: added

db/migrate/20210325092215_add_not_valid_foreign_key_to_group_hooks.rb

+# frozen_string_literal: true
+
+class AddNotValidForeignKeyToGroupHooks < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_foreign_key :web_hooks, :namespaces, column: :group_id, on_delete: :cascade, validate: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key_if_exists :web_hooks, column: :group_id
+    end
+  end
+end

db/schema_migrations/20210325092215

+ea819fd401c5566986fd495ed3b8aa0d296d6c9e3fedf2a10f34cb7fbaeedb20
\ No newline at end of file

db/structure.sql

@@ -26639,6 +26639,9 @@ ALTER TABLE ONLY requirements_management_test_reports
 ALTER TABLE ONLY pool_repositories
     ADD CONSTRAINT fk_rails_d2711daad4 FOREIGN KEY (source_project_id) REFERENCES projects(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY web_hooks
+    ADD CONSTRAINT fk_rails_d35697648e FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE NOT VALID;
+
 ALTER TABLE ONLY group_group_links
     ADD CONSTRAINT fk_rails_d3a0488427 FOREIGN KEY (shared_group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 

ee/app/graphql/resolvers/dast_site_profile_resolver.rb

@@ -2,6 +2,8 @@
 
 module Resolvers
   class DastSiteProfileResolver < BaseResolver
+    include LooksAhead
+
     alias_method :project, :object
 
     type Types::DastSiteProfileType.connection_type, null: true
@@ -12,7 +14,20 @@ module Resolvers
                description: "ID of the site profile."
     end
 
-    def resolve(**args)
+    def resolve_with_lookahead(**args)
+      apply_lookahead(find_dast_site_profiles(args))
+    end
+
+    private
+
+    def preloads
+      {
+        request_headers: [:secret_variables],
+        auth: [:secret_variables]
+      }
+    end
+
+    def find_dast_site_profiles(args)
       if args[:id]
         # TODO: remove this coercion when the compatibility layer is removed
         # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883

ee/app/graphql/types/dast/site_profile_auth_type.rb

@@ -3,6 +3,8 @@
 module Types
   module Dast
     class SiteProfileAuthType < BaseObject
+      REDACTED_PASSWORD = '••••••••'
+
       graphql_name 'DastSiteProfileAuth'
       description 'Input type for DastSiteProfile authentication'
 
@@ -39,7 +41,9 @@ module Types
             description: 'Redacted password to authenticate with on the target website.'
 
       def password
-        nil
+        return unless object.secret_variables.any? { |variable| variable.key == ::Dast::SiteProfileSecretVariable::PASSWORD }
+
+        REDACTED_PASSWORD
       end
     end
   end

ee/app/graphql/types/dast_site_profile_type.rb

@@ -2,6 +2,8 @@
 
 module Types
   class DastSiteProfileType < BaseObject
+    REDACTED_REQUEST_HEADERS = '[Redacted]'
+
     graphql_name 'DastSiteProfile'
     description 'Represents a DAST Site Profile'
 
@@ -68,7 +70,10 @@ module Types
     end
 
     def request_headers
-      nil
+      return unless Feature.enabled?(:security_dast_site_profiles_additional_fields, object.project, default_enabled: :yaml)
+      return unless object.secret_variables.any? { |variable| variable.key == ::Dast::SiteProfileSecretVariable::REQUEST_HEADERS }
+
+      REDACTED_REQUEST_HEADERS
     end
 
     def normalized_target_url

ee/app/models/dast/site_profile_secret_variable.rb

@@ -2,6 +2,9 @@
 
 module Dast
   class SiteProfileSecretVariable < ApplicationRecord
+    REQUEST_HEADERS = 'DAST_REQUEST_HEADERS_BASE64'
+    PASSWORD = 'DAST_PASSWORD_BASE64'
+
     include Ci::HasVariable
     include Ci::Maskable
 

ee/app/views/projects/_merge_trains_settings.html.haml

@@ -4,7 +4,7 @@
   .gl-pl-6
     = form.check_box :merge_trains_enabled, class: 'form-check-input js-merge-options-merge-trains gl-pl-6', data: { qa_selector: 'merge_trains_checkbox' }
     = form.label :merge_trains_enabled, class: 'form-check-label' do
-      = s_('ProjectSettings|Enable merge trains.')
+      = s_('ProjectSettings|Enable merge trains')
       .text-secondary.mb-2
         - merge_trains_help_link_url = help_page_path('ci/merge_request_pipelines/pipelines_for_merged_results/merge_trains/index.md')
         - merge_trains_help_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: merge_trains_help_link_url }

ee/spec/features/projects/settings/disable_merge_trains_setting_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe 'Disable Merge Trains Setting', :js do
     include_examples 'loads correct checkbox state'
 
     it "checking merge trains checkbox doesn't affect merge pipelines checkbox" do
-      check('Enable merge trains.')
+      check('Enable merge trains')
 
       expect(find('#project_merge_trains_enabled')).to be_checked
       expect(find('#project_merge_pipelines_enabled')).not_to be_disabled
@@ -69,7 +69,7 @@ RSpec.describe 'Disable Merge Trains Setting', :js do
     end
 
     it 'unchecking merge pipelines checkbox unchecks merge trains checkbox if it was previously checked' do
-      check('Enable merge trains.')
+      check('Enable merge trains')
       uncheck('Enable merged results pipelines')
 
       expect(find('#project_merge_pipelines_enabled')).not_to be_checked
@@ -120,7 +120,7 @@ RSpec.describe 'Disable Merge Trains Setting', :js do
     end
 
     it "unchecking merge trains checkbox doesn't affect merge pipelines checkbox" do
-      uncheck('Enable merge trains.')
+      uncheck('Enable merge trains')
 
       expect(find('#project_merge_trains_enabled')).not_to be_checked
       expect(find('#project_merge_pipelines_enabled')).not_to be_disabled

ee/spec/graphql/types/dast/site_profile_auth_type_spec.rb

@@ -2,9 +2,66 @@
 
 require 'spec_helper'
 
-RSpec.describe Types::Dast::SiteProfileAuthType do
+RSpec.describe GitlabSchema.types['DastSiteProfileAuth'] do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user, developer_projects: [project]) }
+  let_it_be(:object, reload: true) { create(:dast_site_profile, project: project) }
+  let_it_be(:fields) { %i[enabled url usernameField passwordField username password] }
+
+  before do
+    stub_licensed_features(security_on_demand_scans: true)
+  end
+
   specify { expect(described_class.graphql_name).to eq('DastSiteProfileAuth') }
   specify { expect(described_class).to require_graphql_authorizations(:read_on_demand_scans) }
 
-  it { expect(described_class).to have_graphql_fields(%w[enabled url usernameField passwordField username password]) }
+  it { expect(described_class).to have_graphql_fields(fields) }
+
+  describe 'enabled field' do
+    it 'is auth_enabled' do
+      expect(resolve_field(:enabled, object, current_user: user)).to eq(object.auth_enabled)
+    end
+  end
+
+  describe 'url field' do
+    it 'is auth_url' do
+      expect(resolve_field(:url, object, current_user: user)).to eq(object.auth_url)
+    end
+  end
+
+  describe 'usernameField field' do
+    it 'is auth_username_field' do
+      expect(resolve_field(:username_field, object, current_user: user)).to eq(object.auth_username_field)
+    end
+  end
+
+  describe 'passwordField field' do
+    it 'is auth_password_field' do
+      expect(resolve_field(:password_field, object, current_user: user)).to eq(object.auth_password_field)
+    end
+  end
+
+  describe 'username field' do
+    it 'is auth_username' do
+      expect(resolve_field(:username, object, current_user: user)).to eq(object.auth_username)
+    end
+  end
+
+  describe 'password field' do
+    context 'when there is no associated secret variable' do
+      it 'is nil' do
+        expect(resolve_field(:password, object, current_user: user)).to be_nil
+      end
+    end
+
+    context 'when there an associated secret variable' do
+      it 'is redacted' do
+        create(:dast_site_profile_secret_variable, dast_site_profile: object, key: Dast::SiteProfileSecretVariable::PASSWORD)
+
+        expect(resolve_field(:password, object, current_user: user)).to eq('••••••••')
+      end
+    end
+  end
 end

ee/spec/graphql/types/dast_site_profile_type_spec.rb

@@ -5,23 +5,11 @@ require 'spec_helper'
 RSpec.describe GitlabSchema.types['DastSiteProfile'] do
   include GraphqlHelpers
 
-  let_it_be(:dast_site_profile) { create(:dast_site_profile) }
-  let_it_be(:project) { dast_site_profile.project }
-  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user, developer_projects: [project]) }
+  let_it_be(:object, reload: true) { create(:dast_site_profile, project: project) }
   let_it_be(:fields) { %i[id profileName targetUrl editPath excludedUrls requestHeaders validationStatus userPermissions normalizedTargetUrl auth referencedInSecurityPolicies] }
 
-  subject do
-    GitlabSchema.execute(
-      query,
-      context: {
-        current_user: user
-      },
-      variables: {
-        fullPath: project.full_path
-      }
-    ).as_json
-  end
-
   before do
     stub_licensed_features(security_on_demand_scans: true)
   end
@@ -31,51 +19,47 @@ RSpec.describe GitlabSchema.types['DastSiteProfile'] do
   specify { expect(described_class).to expose_permissions_using(Types::PermissionTypes::DastSiteProfile) }
 
   it { expect(described_class).to have_graphql_fields(fields) }
-
-  describe 'dast_site_profiles' do
-    before do
-      project.add_developer(user)
-    end
-
-    let(:query) do
-      %(
-        query project($fullPath: ID!) {
-          project(fullPath: $fullPath) {
-            dastSiteProfiles(first: 1) {
-              nodes { #{all_graphql_fields_for('DastSiteProfile')} }
-            }
-          }
-        }
-      )
-    end
-
-    let(:first_dast_site_profile) do
-      subject.dig('data', 'project', 'dastSiteProfiles', 'nodes', 0)
-    end
+  it { expect(described_class).to have_graphql_field(:referenced_in_security_policies, calls_gitaly?: true, complexity: 10) }
 
   describe 'id field' do
-      it 'is a global id' do
-        expect(first_dast_site_profile['id']).to eq(dast_site_profile.to_global_id.to_s)
+    it 'is the global id' do
+      expect(resolve_field(:id, object, current_user: user)).to eq(object.to_global_id)
     end
   end
 
-    describe 'profile_name field' do
+  describe 'profileName field' do
     it 'is the name' do
-        expect(first_dast_site_profile['profileName']).to eq(dast_site_profile.name)
+      expect(resolve_field(:profile_name, object, current_user: user)).to eq(object.name)
     end
   end
 
-    describe 'target_url field' do
+  describe 'targetUrl field' do
     it 'is the url of the associated dast_site' do
-        expect(first_dast_site_profile['targetUrl']).to eq(dast_site_profile.dast_site.url)
+      expect(resolve_field(:target_url, object, current_user: user)).to eq(object.dast_site.url)
     end
   end
 
-    describe 'edit_path field' do
+  describe 'editPath field' do
     it 'is the relative path to edit the dast_site_profile' do
-        path = "/#{project.full_path}/-/security/configuration/dast_scans/dast_site_profiles/#{dast_site_profile.id}/edit"
+      path = "/#{project.full_path}/-/security/configuration/dast_scans/dast_site_profiles/#{object.id}/edit"
 
-        expect(first_dast_site_profile['editPath']).to eq(path)
+      expect(resolve_field(:edit_path, object, current_user: user)).to eq(path)
+    end
+  end
+
+  describe 'auth field' do
+    context 'when the feature flag is disabled' do
+      it 'is nil' do
+        stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+
+        expect(resolve_field(:auth, object, current_user: user)).to be_nil
+      end
+    end
+
+    context 'when the feature flag is enabled' do
+      it 'is the dast_site_profile' do
+        expect(resolve_field(:auth, object, current_user: user)).to eq(object)
+      end
     end
   end
 
@@ -84,64 +68,60 @@ RSpec.describe GitlabSchema.types['DastSiteProfile'] do
       it 'is nil' do
         stub_feature_flags(security_dast_site_profiles_additional_fields: false)
 
-          expect(first_dast_site_profile['excludedUrls']).to eq(nil)
+        expect(resolve_field(:excluded_urls, object, current_user: user)).to be_nil
       end
     end
 
     context 'when the feature flag is enabled' do
       it 'is the excluded urls' do
-          expect(first_dast_site_profile['excludedUrls']).to eq(dast_site_profile.excluded_urls)
+        expect(resolve_field(:excluded_urls, object, current_user: user)).to eq(object.excluded_urls)
       end
     end
   end
 
-    describe 'auth field' do
+  describe 'requestHeaders field' do
     context 'when the feature flag is disabled' do
       it 'is nil' do
         stub_feature_flags(security_dast_site_profiles_additional_fields: false)
 
-          expect(first_dast_site_profile['auth']).to eq(nil)
+        expect(resolve_field(:request_headers, object, current_user: user)).to be_nil
       end
     end
 
     context 'when the feature flag is enabled' do
-        it 'includes the correct values' do
-          auth = first_dast_site_profile['auth']
+      context 'when there is no associated secret variable' do
+        it 'is nil' do
+          expect(resolve_field(:request_headers, object, current_user: user)).to be_nil
+        end
+      end
 
-          expect(auth).to include(
-            'enabled' => false,
-            'url' => dast_site_profile.auth_url,
-            'usernameField' => dast_site_profile.auth_username_field,
-            'passwordField' => dast_site_profile.auth_password_field,
-            'username' => dast_site_profile.auth_username,
-            'password' => nil
-          )
+      context 'when there an associated secret variable' do
+        it 'is redacted' do
+          create(:dast_site_profile_secret_variable, dast_site_profile: object, key: Dast::SiteProfileSecretVariable::REQUEST_HEADERS)
+
+          expect(resolve_field(:request_headers, object, current_user: user)).to eq('[Redacted]')
+        end
       end
     end
   end
 
   describe 'validation_status field' do
     it 'is the validation status' do
-        expect(first_dast_site_profile['validationStatus']).to eq('NONE')
+      expect(resolve_field(:validation_status, object, current_user: user)).to eq('none')
     end
   end
 
-    describe 'normalized_target_url field' do
+  describe 'normalizedTargetUrl field' do
     it 'is the normalized url of the associated dast_site' do
-        normalized_url = DastSiteValidation.get_normalized_url_base(dast_site_profile.dast_site.url)
+      normalized_url = DastSiteValidation.get_normalized_url_base(object.dast_site.url)
 
-        expect(first_dast_site_profile['normalizedTargetUrl']).to eq(normalized_url)
+      expect(resolve_field(:normalized_target_url, object, current_user: user)).to eq(normalized_url)
     end
   end
 
-    context 'when there are no dast_site_profiles' do
-      let(:project) { create(:project) }
-
-      it 'has no nodes' do
-        nodes = subject.dig('data', 'project', 'dastSiteProfiles', 'nodes')
-
-        expect(nodes).to be_empty
-      end
+  describe 'referencedInSecurityPolicies field' do
+    it 'is the policies' do
+      expect(resolve_field(:referenced_in_security_policies, object, current_user: user)).to eq(object.referenced_in_security_policies)
     end
   end
 end

ee/spec/models/dast_site_profile_spec.rb

@@ -122,6 +122,7 @@ RSpec.describe DastSiteProfile, type: :model do
     end
   end
 
+  describe 'instance methods' do
     describe '#destroy!' do
       context 'when the associated dast_site has no dast_site_profiles' do
         it 'is also destroyed' do
@@ -184,4 +185,5 @@ RSpec.describe DastSiteProfile, type: :model do
         end
       end
     end
+  end
 end

ee/spec/requests/api/graphql/project/dast_site_profiles_spec.rb

@@ -70,7 +70,7 @@ RSpec.describe 'Query.project(fullPath).dastSiteProfiles' do
       expect(first_dast_site_profile_response['id']).to eq(dast_site_profile.to_global_id.to_s)
     end
 
-    it 'eager loads the dast site and dast site validation' do
+    it 'avoids N+1 queries' do
       control = ActiveRecord::QueryRecorder.new do
         post_graphql(
           query,

lib/api/projects.rb

@@ -67,6 +67,16 @@ module API
 
         PROJECT_ATTACHMENT_SIZE_EXEMPT
       end
+
+      # This is to help determine which projects to use in https://gitlab.com/gitlab-org/gitlab/-/issues/325788
+      def log_if_upload_exceed_max_size(user_project, file)
+        return if file.size <= user_project.max_attachment_size
+        return if exempt_from_global_attachment_size?(user_project)
+
+        if file.size > user_project.max_attachment_size
+          Gitlab::AppLogger.info({ message: "File exceeds maximum size", file_bytes: file.size, project_id: user_project.id, project_path: user_project.full_path })
+        end
+      end
     end
 
     helpers do
@@ -576,6 +586,8 @@ module API
         requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded'
       end
       post ":id/uploads", feature_category: :not_owned do
+        log_if_upload_exceed_max_size(user_project, params[:file])
+
         service = UploadService.new(user_project, params[:file])
         service.override_max_attachment_size = project_attachment_size(user_project)
         upload = service.execute

locale/gitlab.pot

@@ -24131,7 +24131,7 @@ msgstr ""
 msgid "ProjectSettings|Enable \"Delete source branch\" option by default"
 msgstr ""
 
-msgid "ProjectSettings|Enable merge trains."
+msgid "ProjectSettings|Enable merge trains"
 msgstr ""
 
 msgid "ProjectSettings|Enable merged results pipelines"

spec/db/schema_spec.rb

@@ -87,7 +87,6 @@ RSpec.describe 'Database schema' do
     users_star_projects: %w[user_id],
     vulnerability_identifiers: %w[external_id],
     vulnerability_scanners: %w[external_id],
-    web_hooks: %w[group_id],
     web_hook_logs_part_0c5294f417: %w[web_hook_id]
   }.with_indifferent_access.freeze
 

spec/requests/api/projects_spec.rb

@@ -1519,6 +1519,8 @@ RSpec.describe API::Projects do
   end
 
   describe "POST /projects/:id/uploads" do
+    let(:file) { fixture_file_upload("spec/fixtures/dk.png", "image/png") }
+
     before do
       project
     end
@@ -1528,7 +1530,7 @@ RSpec.describe API::Projects do
         expect(instance).to receive(:override_max_attachment_size=).with(project.max_attachment_size).and_call_original
       end
 
-      post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") }
+      post api("/projects/#{project.id}/uploads", user), params: { file: file }
 
       expect(response).to have_gitlab_http_status(:created)
       expect(json_response['alt']).to eq("dk")
@@ -1538,13 +1540,21 @@ RSpec.describe API::Projects do
       expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads")
     end
 
+    it "logs a warning if file exceeds attachment size" do
+      allow(Gitlab::CurrentSettings).to receive(:max_attachment_size).and_return(0)
+
+      expect(Gitlab::AppLogger).to receive(:info).with(hash_including(message: 'File exceeds maximum size')).and_call_original
+
+      post api("/projects/#{project.id}/uploads", user), params: { file: file }
+    end
+
     shared_examples 'capped upload attachments' do
       it "limits the upload to 1 GB" do
         expect_next_instance_of(UploadService) do |instance|
           expect(instance).to receive(:override_max_attachment_size=).with(1.gigabyte).and_call_original
         end
 
-        post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") }
+        post api("/projects/#{project.id}/uploads", user), params: { file: file }
 
         expect(response).to have_gitlab_http_status(:created)
       end