Add latest changes from gitlab-org/gitlab@master

Gemfile

@@ -313,7 +313,7 @@ gem 'pg_query', '~> 1.3.0'
 gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
-gem 'gitlab-labkit', '~> 0.16.0'
+gem 'gitlab-labkit', '~> 0.16.1'
 # Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
 # because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
 gem 'thrift', '>= 0.14.0'

Gemfile.lock

@@ -447,7 +447,7 @@ GEM
       fog-xml (~> 0.1.0)
       google-api-client (>= 0.44.2, < 0.51)
       google-cloud-env (~> 1.2)
-    gitlab-labkit (0.16.0)
+    gitlab-labkit (0.16.1)
       actionpack (>= 5.0.0, < 7.0.0)
       activesupport (>= 5.0.0, < 7.0.0)
       grpc (~> 1.19)
@@ -1416,7 +1416,7 @@ DEPENDENCIES
   gitlab-experiment (~> 0.5.0)
   gitlab-fog-azure-rm (~> 1.0.1)
   gitlab-fog-google (~> 1.13)
-  gitlab-labkit (~> 0.16.0)
+  gitlab-labkit (~> 0.16.1)
   gitlab-license (~> 1.3)
   gitlab-mail_room (~> 0.0.8)
   gitlab-markup (~> 1.7.1)

app/views/notify/unknown_sign_in_email.html.haml

@@ -45,8 +45,9 @@
             - password_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: 'https://docs.gitlab.com/ee/user/profile/#changing-your-password' }
             = _('If you recently signed in and recognize the IP address, you may disregard this email.')
             %p
-              = _('If you did not recently sign in, you should immediately %{password_link_start}change your password%{password_link_end}.').html_safe % { password_link_start: password_link_start, password_link_end: '</a>'.html_safe }
-              = _('Passwords should be unique and not used for any other sites or services.')
+              - if password_authentication_enabled_for_web?
+                = _('If you did not recently sign in, you should immediately %{password_link_start}change your password%{password_link_end}.').html_safe % { password_link_start: password_link_start, password_link_end: '</a>'.html_safe }
+                = _('Passwords should be unique and not used for any other sites or services.')
 
             - unless @user.two_factor_enabled?
               %p

app/workers/emails_on_push_worker.rb

@@ -96,6 +96,6 @@ class EmailsOnPushWorker # rubocop:disable Scalability/IdempotentWorker
   def valid_recipients(recipients)
     recipients.split.select do |recipient|
       recipient.include?('@')
-    end
+    end.uniq(&:downcase)
   end
 end

changelogs/unreleased/268207_exclude_duplicates_from_emails_on_push_recipients_when_sending.yml

@@ -0,0 +1,5 @@
+---
+title: Exclude duplicates from emails on push recipients
+merge_request: 55588
+author:
+type: changed

changelogs/unreleased/add-client-id-application-context-metadata.yml

@@ -0,0 +1,5 @@
+---
+title: Add client_id to application context
+merge_request: 55683
+author:
+type: added

changelogs/unreleased/chore-unknown-sign-in-info-text.yml

@@ -0,0 +1,5 @@
+---
+title: Show password hint only if password_authentication_enabled_for_web? on new location logins
+merge_request: 46018
+author: Roger Meier
+type: changed

lib/api/api.rb

@@ -58,6 +58,7 @@ module API
         user: -> { @current_user },
         project: -> { @project },
         namespace: -> { @group },
+        runner: -> { @current_runner || @runner },
         caller_id: route.origin,
         remote_ip: request.ip,
         feature_category: feature_category

lib/api/ci/runner.rb

@@ -44,12 +44,12 @@ module API
               forbidden!
             end
 
-          runner = ::Ci::Runner.create(attributes)
+          @runner = ::Ci::Runner.create(attributes)
 
-          if runner.persisted?
-            present runner, with: Entities::RunnerRegistrationDetails
+          if @runner.persisted?
+            present @runner, with: Entities::RunnerRegistrationDetails
           else
-            render_validation_error!(runner)
+            render_validation_error!(@runner)
           end
         end
 
@@ -62,9 +62,7 @@ module API
         delete '/' do
           authenticate_runner!
 
-          runner = ::Ci::Runner.find_by_token(params[:token])
-
-          destroy_conditionally!(runner)
+          destroy_conditionally!(current_runner)
         end
 
         desc 'Validates authentication credentials' do

lib/api/helpers/runner.rb

@@ -73,23 +73,12 @@ module API
       end
 
       def set_application_context
-        if current_job
-          Gitlab::ApplicationContext.push(
-            user: -> { current_job.user },
-            project: -> { current_job.project }
-          )
-        elsif current_runner&.project_type?
-          Gitlab::ApplicationContext.push(
-            project: -> do
-              projects = current_runner.projects.limit(2) # rubocop: disable CodeReuse/ActiveRecord
-              projects.first if projects.length == 1
-            end
-          )
-        elsif current_runner&.group_type?
-          Gitlab::ApplicationContext.push(
-            namespace: -> { current_runner.groups.first }
-          )
-        end
+        return unless current_job
+
+        Gitlab::ApplicationContext.push(
+          user: -> { current_job.user },
+          project: -> { current_job.project }
+        )
       end
     end
   end

lib/gitlab/application_context.rb

@@ -4,6 +4,7 @@ module Gitlab
   # A GitLab-rails specific accessor for `Labkit::Logging::ApplicationContext`
   class ApplicationContext
     include Gitlab::Utils::LazyAttributes
+    include Gitlab::Utils::StrongMemoize
 
     Attribute = Struct.new(:name, :type)
 
@@ -11,6 +12,7 @@ module Gitlab
       Attribute.new(:project, Project),
       Attribute.new(:namespace, Namespace),
       Attribute.new(:user, User),
+      Attribute.new(:runner, ::Ci::Runner),
       Attribute.new(:caller_id, String),
       Attribute.new(:remote_ip, String),
       Attribute.new(:related_class, String),
@@ -47,8 +49,9 @@ module Gitlab
     def to_lazy_hash
       {}.tap do |hash|
         hash[:user] = -> { username } if set_values.include?(:user)
-        hash[:project] = -> { project_path } if set_values.include?(:project)
+        hash[:project] = -> { project_path } if set_values.include?(:project) || set_values.include?(:runner)
         hash[:root_namespace] = -> { root_namespace_path } if include_namespace?
+        hash[:client_id] = -> { client } if include_client?
         hash[:caller_id] = caller_id if set_values.include?(:caller_id)
         hash[:remote_ip] = remote_ip if set_values.include?(:remote_ip)
         hash[:related_class] = related_class if set_values.include?(:related_class)
@@ -75,7 +78,8 @@ module Gitlab
     end
 
     def project_path
-      project&.full_path
+      associated_routable = project || runner_project
+      associated_routable&.full_path
     end
 
     def username
@@ -83,15 +87,43 @@ module Gitlab
     end
 
     def root_namespace_path
-      if namespace
-        namespace.full_path_components.first
-      else
-        project&.full_path_components&.first
-      end
+      associated_routable = namespace || project || runner_project || runner_group
+      associated_routable&.full_path_components&.first
     end
 
     def include_namespace?
-      set_values.include?(:namespace) || set_values.include?(:project)
+      set_values.include?(:namespace) || set_values.include?(:project) || set_values.include?(:runner)
+    end
+
+    def include_client?
+      set_values.include?(:user) || set_values.include?(:runner) || set_values.include?(:remote_ip)
+    end
+
+    def client
+      if user
+        "user/#{user.id}"
+      elsif runner
+        "runner/#{runner.id}"
+      else
+        "ip/#{remote_ip}"
+      end
+    end
+
+    def runner_project
+      strong_memoize(:runner_project) do
+        next unless runner&.project_type?
+
+        projects = runner.projects.take(2) # rubocop: disable CodeReuse/ActiveRecord
+        projects.first if projects.one?
+      end
+    end
+
+    def runner_group
+      strong_memoize(:runner_group) do
+        next unless runner&.group_type?
+
+        runner.groups.first
+      end
     end
   end
 end

spec/graphql/types/snippet_type_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe GitlabSchema.types['Snippet'] do
+  include GraphqlHelpers
+
   let_it_be(:user) { create(:user) }
 
   it 'has the correct fields' do
@@ -25,6 +27,14 @@ RSpec.describe GitlabSchema.types['Snippet'] do
     end
   end
 
+  describe '#user_permissions' do
+    let_it_be(:snippet) { create(:personal_snippet, :repository, :public, author: user) }
+
+    it 'can resolve the snippet permissions' do
+      expect(resolve_field(:user_permissions, snippet)).to eq(snippet)
+    end
+  end
+
   context 'when restricted visibility level is set to public' do
     let_it_be(:snippet) { create(:personal_snippet, :repository, :public, author: user) }
 

spec/graphql/types/snippets/blob_type_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe GitlabSchema.types['SnippetBlob'] do
+  include GraphqlHelpers
+
   it 'has the correct fields' do
     expected_fields = [:rich_data, :plain_data,
                        :raw_path, :size, :binary, :name, :path,
@@ -12,16 +14,37 @@ RSpec.describe GitlabSchema.types['SnippetBlob'] do
     expect(described_class).to have_graphql_fields(*expected_fields)
   end
 
-  specify { expect(described_class.fields['richData'].type).not_to be_non_null }
-  specify { expect(described_class.fields['plainData'].type).not_to be_non_null }
-  specify { expect(described_class.fields['rawPath'].type).to be_non_null }
-  specify { expect(described_class.fields['size'].type).to be_non_null }
-  specify { expect(described_class.fields['binary'].type).to be_non_null }
-  specify { expect(described_class.fields['name'].type).not_to be_non_null }
-  specify { expect(described_class.fields['path'].type).not_to be_non_null }
-  specify { expect(described_class.fields['simpleViewer'].type).to be_non_null }
-  specify { expect(described_class.fields['richViewer'].type).not_to be_non_null }
-  specify { expect(described_class.fields['mode'].type).not_to be_non_null }
-  specify { expect(described_class.fields['externalStorage'].type).not_to be_non_null }
-  specify { expect(described_class.fields['renderedAsText'].type).to be_non_null }
+  let_it_be(:nullity) do
+    {
+      'richData' => be_nullable,
+      'plainData' => be_nullable,
+      'rawPath' => be_non_null,
+      'size' => be_non_null,
+      'binary' => be_non_null,
+      'name' => be_nullable,
+      'path' => be_nullable,
+      'simpleViewer' => be_non_null,
+      'richViewer' => be_nullable,
+      'mode' => be_nullable,
+      'externalStorage' => be_nullable,
+      'renderedAsText' => be_non_null
+    }
+  end
+
+  let_it_be(:blob) { create(:snippet, :public, :repository).blobs.first }
+
+  shared_examples 'a field from the snippet blob presenter' do |field|
+    it "resolves using the presenter", :request_store do
+      presented = SnippetBlobPresenter.new(blob)
+
+      expect(resolve_field(field, blob)).to eq(presented.try(field.method_sym))
+    end
+  end
+
+  described_class.fields.each_value do |field|
+    describe field.graphql_name do
+      it_behaves_like 'a field from the snippet blob presenter', field
+      specify { expect(field.type).to match(nullity.fetch(field.graphql_name)) }
+    end
+  end
 end

spec/lib/gitlab/application_context_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe Gitlab::ApplicationContext do
   describe '.push' do
     it 'passes the expected context on to labkit' do
       fake_proc = duck_type(:call)
-      expected_context = { user: fake_proc }
+      expected_context = { user: fake_proc, client_id: fake_proc }
 
       expect(Labkit::Context).to receive(:push).with(expected_context)
 
@@ -92,6 +92,34 @@ RSpec.describe Gitlab::ApplicationContext do
       expect(result(context))
         .to include(project: project.full_path, root_namespace: project.full_path_components.first)
     end
+
+    describe 'setting the client' do
+      let_it_be(:remote_ip) { '127.0.0.1' }
+      let_it_be(:runner) { create(:ci_runner) }
+      let_it_be(:options) { { remote_ip: remote_ip, runner: runner, user: user } }
+
+      using RSpec::Parameterized::TableSyntax
+
+      where(:provided_options, :client) do
+        [:remote_ip]                 | :remote_ip
+        [:remote_ip, :runner]        | :runner
+        [:remote_ip, :runner, :user] | :user
+      end
+
+      with_them do
+        it 'sets the client_id to the expected value' do
+          context = described_class.new(**options.slice(*provided_options))
+
+          client_id = case client
+                      when :remote_ip then "ip/#{remote_ip}"
+                      when :runner then "runner/#{runner.id}"
+                      when :user then "user/#{user.id}"
+                      end
+
+          expect(result(context)[:client_id]).to eq(client_id)
+        end
+      end
+    end
   end
 
   describe '#use' do

spec/requests/api/api_spec.rb

@@ -112,6 +112,7 @@ RSpec.describe API::API do
                                        'meta.project' => project.full_path,
                                        'meta.root_namespace' => project.namespace.full_path,
                                        'meta.user' => user.username,
+                                       'meta.client_id' => an_instance_of(String),
                                        'meta.feature_category' => 'issue_tracking')
         end
       end
@@ -125,6 +126,7 @@ RSpec.describe API::API do
           expect(log_context).to match('correlation_id' => an_instance_of(String),
                                        'meta.caller_id' => '/api/:version/users',
                                        'meta.remote_ip' => an_instance_of(String),
+                                       'meta.client_id' => an_instance_of(String),
                                        'meta.feature_category' => 'users')
         end
       end

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -806,7 +806,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             subject { request_job(id: job.id) }
 
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { user: user.username, project: project.full_path } }
+              let(:expected_params) { { user: user.username, project: project.full_path, client_id: "user/#{user.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 3 do
@@ -817,7 +817,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           context 'when the runner is of project type' do
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { project: project.full_path } }
+              let(:expected_params) { { project: project.full_path, client_id: "runner/#{runner.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 2 do
@@ -831,7 +831,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             let(:runner) { create(:ci_runner, :group, groups: [group]) }
 
             it_behaves_like 'storing arguments in the application context' do
-              let(:expected_params) { { root_namespace: group.full_path_components.first } }
+              let(:expected_params) { { root_namespace: group.full_path_components.first, client_id: "runner/#{runner.id}" } }
             end
 
             it_behaves_like 'not executing any extra queries for the application context', 2 do

spec/requests/api/ci/runner/runners_delete_spec.rb

@@ -37,8 +37,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
       context 'when valid token is provided' do
         let(:runner) { create(:ci_runner) }
 
+        subject { delete api('/runners'), params: { token: runner.token } }
+
         it 'deletes Runner' do
-          delete api('/runners'), params: { token: runner.token }
+          subject
 
           expect(response).to have_gitlab_http_status(:no_content)
           expect(::Ci::Runner.count).to eq(0)
@@ -48,6 +50,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           let(:request) { api('/runners') }
           let(:params) { { token: runner.token } }
         end
+
+        it_behaves_like 'storing arguments in the application context' do
+          let(:expected_params) { { client_id: "runner/#{runner.id}" } }
+        end
       end
     end
   end

spec/requests/api/ci/runner/runners_post_spec.rb

@@ -39,18 +39,32 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           post api('/runners'), params: { token: token }
         end
 
-        it 'creates runner with default values' do
-          post api('/runners'), params: { token: registration_token }
+        context 'with a registration token' do
+          let(:token) { registration_token }
 
-          runner = ::Ci::Runner.first
+          it 'creates runner with default values' do
+            request
 
-          expect(response).to have_gitlab_http_status(:created)
-          expect(json_response['id']).to eq(runner.id)
-          expect(json_response['token']).to eq(runner.token)
-          expect(runner.run_untagged).to be true
-          expect(runner.active).to be true
-          expect(runner.token).not_to eq(registration_token)
-          expect(runner).to be_instance_type
+            runner = ::Ci::Runner.first
+
+            expect(response).to have_gitlab_http_status(:created)
+            expect(json_response['id']).to eq(runner.id)
+            expect(json_response['token']).to eq(runner.token)
+            expect(runner.run_untagged).to be true
+            expect(runner.active).to be true
+            expect(runner.token).not_to eq(registration_token)
+            expect(runner).to be_instance_type
+          end
+
+          it_behaves_like 'storing arguments in the application context' do
+            subject { request }
+
+            let(:expected_params) { { client_id: "runner/#{::Ci::Runner.first.id}" } }
+          end
+
+          it_behaves_like 'not executing any extra queries for the application context' do
+            let(:subject_proc) { proc { request } }
+          end
         end
 
         context 'when project token is used' do
@@ -71,7 +85,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           it_behaves_like 'storing arguments in the application context' do
             subject { request }
 
-            let(:expected_params) { { project: project.full_path } }
+            let(:expected_params) { { project: project.full_path, client_id: "runner/#{::Ci::Runner.first.id}" } }
           end
 
           it_behaves_like 'not executing any extra queries for the application context' do
@@ -97,7 +111,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           it_behaves_like 'storing arguments in the application context' do
             subject { request }
 
-            let(:expected_params) { { root_namespace: group.full_path_components.first } }
+            let(:expected_params) { { root_namespace: group.full_path_components.first, client_id: "runner/#{::Ci::Runner.first.id}" } }
           end
 
           it_behaves_like 'not executing any extra queries for the application context' do

spec/requests/api/ci/runner/runners_verify_post_spec.rb

@@ -37,11 +37,17 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
       end
 
       context 'when valid token is provided' do
+        subject { post api('/runners/verify'), params: { token: runner.token } }
+
         it 'verifies Runner credentials' do
-          post api('/runners/verify'), params: { token: runner.token }
+          subject
 
           expect(response).to have_gitlab_http_status(:ok)
         end
+
+        it_behaves_like 'storing arguments in the application context' do
+          let(:expected_params) { { client_id: "runner/#{runner.id}" } }
+        end
       end
     end
   end

spec/requests/api/graphql/snippets_spec.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'snippets' do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:snippets) { create_list(:personal_snippet, 3, :repository, author: current_user) }
+
+  describe 'querying for all fields' do
+    let(:query) do
+      graphql_query_for(:snippets, { ids: [global_id_of(snippets.first)] }, <<~SELECT)
+        nodes { #{all_graphql_fields_for('Snippet')} }
+      SELECT
+    end
+
+    it 'can successfully query for snippets and their blobs' do
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data_at(:snippets, :nodes)).to be_one
+      expect(graphql_data_at(:snippets, :nodes, :blobs, :nodes)).to be_present
+    end
+  end
+end

spec/support/matchers/graphql_matchers.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+RSpec::Matchers.define_negated_matcher :be_nullable, :be_non_null
+
 RSpec::Matchers.define :require_graphql_authorizations do |*expected|
   match do |klass|
     permissions = if klass.respond_to?(:required_permissions)
@@ -90,7 +92,7 @@ RSpec::Matchers.define :have_graphql_arguments do |*expected|
     @names ||= Array.wrap(expected).map { |name| GraphqlHelpers.fieldnamerize(name) }
 
     if field.type.try(:ancestors)&.include?(GraphQL::Types::Relay::BaseConnection)
-      @names | %w(after before first last)
+      @names | %w[after before first last]
     else
       @names
     end
@@ -103,9 +105,10 @@ RSpec::Matchers.define :have_graphql_arguments do |*expected|
   end
 
   failure_message do |field|
-    names = expected_names(field)
+    names = expected_names(field).inspect
+    args = field.arguments.keys.inspect
 
-    "expected that #{field.name} would have the following fields: #{names.inspect}, but it has #{field.arguments.keys.inspect}."
+    "expected that #{field.name} would have the following arguments: #{names}, but it has #{args}."
   end
 end
 

spec/workers/emails_on_push_worker_spec.rb

@@ -97,7 +97,7 @@ RSpec.describe EmailsOnPushWorker, :mailer do
       end
 
       it "gracefully handles an input SMTP error" do
-        expect(ActionMailer::Base.deliveries.count).to eq(0)
+        expect(ActionMailer::Base.deliveries).to be_empty
       end
     end
 
@@ -112,6 +112,16 @@ RSpec.describe EmailsOnPushWorker, :mailer do
         end
       end
 
+      context "with mixed-case recipient" do
+        let(:recipients) { user.email.upcase }
+
+        it "retains the case" do
+          perform
+
+          expect(email_recipients).to contain_exactly(recipients)
+        end
+      end
+
       context "when the recipient addresses are a list of email addresses" do
         let(:recipients) do
           1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n")
@@ -120,7 +130,6 @@ RSpec.describe EmailsOnPushWorker, :mailer do
         it "sends the mail to each of the recipients" do
           perform
 
-          expect(ActionMailer::Base.deliveries.count).to eq(5)
           expect(email_recipients).to contain_exactly(*recipients.split)
         end
 
@@ -132,13 +141,22 @@ RSpec.describe EmailsOnPushWorker, :mailer do
         end
       end
 
+      context "when recipients are invalid" do
+        let(:recipients) { "invalid\n\nrecipients" }
+
+        it "ignores them" do
+          perform
+
+          expect(ActionMailer::Base.deliveries).to be_empty
+        end
+      end
+
       context "when the recipient addresses contains angle brackets and are separated by spaces" do
         let(:recipients) { "John Doe <johndoe@example.com> Jane Doe <janedoe@example.com>" }
 
         it "accepts emails separated by whitespace" do
           perform
 
-          expect(ActionMailer::Base.deliveries.count).to eq(2)
           expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
         end
       end
@@ -149,7 +167,6 @@ RSpec.describe EmailsOnPushWorker, :mailer do
         it "accepts both kind of emails" do
           perform
 
-          expect(ActionMailer::Base.deliveries.count).to eq(2)
           expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
         end
       end
@@ -160,10 +177,19 @@ RSpec.describe EmailsOnPushWorker, :mailer do
         it "accepts emails separated by newlines" do
           perform
 
-          expect(ActionMailer::Base.deliveries.count).to eq(2)
           expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
         end
       end
+
+      context 'when the recipient addresses contains duplicates' do
+        let(:recipients) { 'non@dubplicate.com Duplic@te.com duplic@te.com Duplic@te.com duplic@Te.com' }
+
+        it 'deduplicates recipients while treating the domain part as case-insensitive' do
+          perform
+
+          expect(email_recipients).to contain_exactly('non@dubplicate.com', 'Duplic@te.com')
+        end
+      end
     end
   end
 end