Add latest changes from gitlab-org/gitlab@master

13 files changed, 244 insertions, 33 deletions

diff --git a/app/services/concerns/members/bulk_create_users.rb b/app/services/concerns/members/bulk_create_users.rb
index b98917f1396..9cfef96311e 100644
--- a/app/services/concerns/members/bulk_create_users.rb
+++ b/app/services/concerns/members/bulk_create_users.rb
@@ -48,6 +48,7 @@ module Members
           end
 
           if user_ids.present?
+            # we should handle the idea of existing members where users are passed as users - https://gitlab.com/gitlab-org/gitlab/-/issues/352617
             # the below will automatically discard invalid user_ids
             users.concat(User.id_in(user_ids))
             # helps not have to perform another query per user id to see if the member exists later on when fetching
diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb
index e766a7e9044..fcce32ead94 100644
--- a/app/services/members/creator_service.rb
+++ b/app/services/members/creator_service.rb
@@ -67,6 +67,7 @@ module Members
     def create_member_task
       return unless member.persisted?
       return if member_task_attributes.value?(nil)
+      return if member.member_task.present?
 
       member.create_member_task(member_task_attributes)
     end
diff --git a/lib/gitlab/database/gitlab_loose_foreign_keys.yml b/config/gitlab_loose_foreign_keys.yml
index 154e563ad99..154e563ad99 100644
--- a/lib/gitlab/database/gitlab_loose_foreign_keys.yml
+++ b/config/gitlab_loose_foreign_keys.yml
diff --git a/doc/administration/gitaly/index.md b/doc/administration/gitaly/index.md
index 3bb88c56934..ea24e901943 100644
--- a/doc/administration/gitaly/index.md
+++ b/doc/administration/gitaly/index.md
@@ -529,6 +529,10 @@ To monitor [strong consistency](#strong-consistency), you can use the following
 - `gitaly_hook_transaction_voting_delay_seconds`, the client-side delay introduced by waiting for
   the transaction to be committed.
 
+To monitor the number of repositories that have no healthy, up-to-date replicas:
+
+- `gitaly_praefect_unavailable_repositories`
+
 You can also monitor the [Praefect logs](../logs.md#praefect-logs).
 
 #### Database metrics `/db_metrics` endpoint
diff --git a/doc/api/oauth2.md b/doc/api/oauth2.md
index 7d83607ab28..59a929e30f4 100644
--- a/doc/api/oauth2.md
+++ b/doc/api/oauth2.md
@@ -2,7 +2,7 @@
 type: reference, howto
 stage: Manage
 group: Authentication and Authorization
-info: To determine the technical writer assigned to the Stage/Group associated   with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers
 ---
 
 # OAuth 2.0 identity provider API **(FREE)**
@@ -41,7 +41,9 @@ Both **authorization code** (with or without PKCE) and **implicit grant** flows
 registered first via the `/profile/applications` page in your user's account.
 During registration, by enabling proper scopes, you can limit the range of
 resources which the `application` can access. Upon creation, you obtain the
-`application` credentials: _Application ID_ and _Client Secret_ - **keep them secure**.
+`application` credentials: _Application ID_ and _Client Secret_. The _Client Secret_
+**must be kept secure**. It is also advantageous to keep the _Application ID_
+secret when your application architecture allows.
 
 For a list of scopes in GitLab, see [the provider documentation](../integration/oauth_provider.md#authorized-applications).
 
@@ -74,7 +76,10 @@ detailed flow description, from authorization request through access token.
 The following steps describe our implementation of the flow.
 
 The Authorization code with PKCE flow, PKCE for short, makes it possible to securely perform
-the OAuth exchange of client credentials for access tokens on public clients.
+the OAuth exchange of client credentials for access tokens on public clients without
+requiring access to the _Client Secret_ at all. This makes the PKCE flow advantageous
+for single page JavaScript applications or other client side apps where keeping secrets
+from the user is a technical impossibility. 
 
 Before starting the flow, generate the `STATE`, the `CODE_VERIFIER` and the `CODE_CHALLENGE`.
 
@@ -113,7 +118,7 @@ Before starting the flow, generate the `STATE`, the `CODE_VERIFIER` and the `COD
    any HTTP client. The following example uses Ruby's `rest-client`:
 
    ```ruby
-   parameters = 'client_id=APP_ID&client_secret=APP_SECRET&code=RETURNED_CODE&grant_type=authorization_code&redirect_uri=REDIRECT_URI&code_verifier=CODE_VERIFIER'
+   parameters = 'client_id=APP_ID&code=RETURNED_CODE&grant_type=authorization_code&redirect_uri=REDIRECT_URI&code_verifier=CODE_VERIFIER'
    RestClient.post 'https://gitlab.example.com/oauth/token', parameters
    ```
 
@@ -135,7 +140,7 @@ Before starting the flow, generate the `STATE`, the `CODE_VERIFIER` and the `COD
    - Sends new tokens in the response.
 
    ```ruby
-     parameters = 'client_id=APP_ID&client_secret=APP_SECRET&refresh_token=REFRESH_TOKEN&grant_type=refresh_token&redirect_uri=REDIRECT_URI&code_verifier=CODE_VERIFIER'
+     parameters = 'client_id=APP_ID&refresh_token=REFRESH_TOKEN&grant_type=refresh_token&redirect_uri=REDIRECT_URI&code_verifier=CODE_VERIFIER'
      RestClient.post 'https://gitlab.example.com/oauth/token', parameters
    ```
 
diff --git a/doc/development/database/loose_foreign_keys.md b/doc/development/database/loose_foreign_keys.md
index 97d150b1a18..d08e90683fe 100644
--- a/doc/development/database/loose_foreign_keys.md
+++ b/doc/development/database/loose_foreign_keys.md
@@ -61,7 +61,7 @@ following information:
 - Child table name (`ci_pipelines`)
 - The data cleanup method (`async_delete` or `async_nullify`)
 
-The YAML file is located at `lib/gitlab/database/gitlab_loose_foreign_keys.yml`. The file groups
+The YAML file is located at `config/gitlab_loose_foreign_keys.yml`. The file groups
 foreign key definitions by the name of the child table. The child table can have multiple loose
 foreign key definitions, therefore we store them as an array.
 
diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb
index f26ac1318b1..bb1d1261492 100644
--- a/lib/api/helpers/members_helpers.rb
+++ b/lib/api/helpers/members_helpers.rb
@@ -62,13 +62,10 @@ module API
       end
 
       def add_single_member_by_user_id(create_service_params)
-        source = create_service_params[:source]
         user_id = create_service_params[:user_ids]
         user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
 
         if user
-          conflict!('Member already exists') if member_already_exists?(source, user_id)
-
           instance = ::Members::CreateService.new(current_user, create_service_params)
           instance.execute
 
@@ -90,12 +87,6 @@ module API
       def add_single_member?(user_id)
         user_id.present?
       end
-
-      private
-
-      def member_already_exists?(source, user_id)
-        source.members.exists?(user_id: user_id) # rubocop: disable CodeReuse/ActiveRecord
-      end
     end
   end
 end
diff --git a/lib/gitlab/database/loose_foreign_keys.rb b/lib/gitlab/database/loose_foreign_keys.rb
index 9a6aaf2ef7d..1338b18a099 100644
--- a/lib/gitlab/database/loose_foreign_keys.rb
+++ b/lib/gitlab/database/loose_foreign_keys.rb
@@ -32,7 +32,7 @@ module Gitlab
       end
 
       def self.loose_foreign_keys_yaml_path
-        @loose_foreign_keys_yaml_path ||= Rails.root.join('lib/gitlab/database/gitlab_loose_foreign_keys.yml')
+        @loose_foreign_keys_yaml_path ||= Rails.root.join('config/gitlab_loose_foreign_keys.yml')
       end
 
       private_class_method :build_definition
diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/merge_when_pipeline_succeeds_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/merge_when_pipeline_succeeds_spec.rb
index d9b119c2626..85270791f0f 100644
--- a/qa/qa/specs/features/browser_ui/3_create/merge_request/merge_when_pipeline_succeeds_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/merge_when_pipeline_succeeds_spec.rb
@@ -42,7 +42,7 @@ module QA
                   content: <<~EOF
                     test:
                       tags: ["runner-for-#{project.name}"]
-                      script: sleep 20
+                      script: sleep 30
                       only:
                         - merge_requests
                   EOF
@@ -91,7 +91,7 @@ module QA
             end
 
             Page::MergeRequest::Show.perform do |mr|
-              refresh
+              mr.refresh
 
               # Part of the challenge with this test is that the MR widget has many components that could be displayed
               # and many errors states that those components could encounter. Most of the time few of those
@@ -104,15 +104,14 @@ module QA
               mr.retry_until(reload: true, message: 'Wait until ready to click MWPS') do
                 merge_request.reload!
 
-                # Don't try to click MWPS if the MR is merged or the pipeline is complete
-                break if merge_request.state == 'merged' || mr.wait_until { project.pipelines.last }[:status] == 'success'
+                # Click the MWPS button if we can
+                break mr.merge_when_pipeline_succeeds! if mr.has_element?(:merge_button, text: 'Merge when pipeline succeeds')
 
-                # Try to click MWPS if this is a transient test, or if the MWPS button is visible,
-                # otherwise reload the page and retry
-                next false unless transient_test || mr.has_element?(:merge_button, text: 'Merge when pipeline succeeds')
+                # But fail if the button is missing because the pipeline is complete
+                raise "The pipeline already finished before we could click MWPS" if mr.wait_until { project.pipelines.first }[:status] == 'success'
 
-                # No need to keep retrying if we can click MWPS
-                break mr.merge_when_pipeline_succeeds!
+                # Otherwise, if this is not a transient test reload the page and retry
+                next false unless transient_test
               end
 
               aggregate_failures do
diff --git a/scripts/decomposition/generate-loose-foreign-key b/scripts/decomposition/generate-loose-foreign-key
index 860da926594..35f84c64ce1 100755
--- a/scripts/decomposition/generate-loose-foreign-key
+++ b/scripts/decomposition/generate-loose-foreign-key
@@ -107,7 +107,7 @@ def columns(*args)
 end
 
 def add_definition_to_yaml(definition)
-  content = YAML.load_file(Rails.root.join('lib/gitlab/database/gitlab_loose_foreign_keys.yml'))
+  content = YAML.load_file(Rails.root.join('config/gitlab_loose_foreign_keys.yml'))
   table_definitions = content[definition.from_table]
 
   # insert new entry at random place to avoid conflicts
@@ -148,11 +148,11 @@ def add_definition_to_yaml(definition)
 
   # emulate existing formatting
   File.write(
-    Rails.root.join('lib/gitlab/database/gitlab_loose_foreign_keys.yml'),
+    Rails.root.join('config/gitlab_loose_foreign_keys.yml'),
     content.to_yaml.gsub(/^([- ] )/, '  \1')
   )
 
-  exec_cmd("git", "add", "lib/gitlab/database/gitlab_loose_foreign_keys.yml")
+  exec_cmd("git", "add", "config/gitlab_loose_foreign_keys.yml")
 end
 
 def generate_migration(definition)
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index 02061bb8ab6..731e0a4a078 100644
--- a/spec/requests/api/members_spec.rb
+++ b/spec/requests/api/members_spec.rb
@@ -291,6 +291,25 @@ RSpec.describe API::Members do
               user: maintainer
             )
           end
+
+          context 'with an already existing member' do
+            before do
+              source.add_developer(stranger)
+            end
+
+            it 'tracks the invite source from params' do
+              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                   params: params.merge(invite_source: '_invite_source_')
+
+              expect_snowplow_event(
+                category: 'Members::CreateService',
+                action: 'create_member',
+                label: '_invite_source_',
+                property: 'existing_user',
+                user: maintainer
+              )
+            end
+          end
         end
 
         context 'when executing the Members::CreateService for multiple user_ids' do
@@ -399,6 +418,49 @@ RSpec.describe API::Members do
             expect(member.tasks_to_be_done).to match_array([:code, :ci])
             expect(member.member_task.project_id).to eq(project_id)
           end
+
+          context 'with already existing member' do
+            before do
+              source.add_developer(stranger)
+            end
+
+            it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+              member = source.members.find_by(user_id: stranger.id)
+              create(:member_task, member: member, project_id: project_id, tasks_to_be_done: %w(code ci))
+
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: stranger.id,
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.not_to change(MemberTask, :count)
+
+              member.reset
+              expect(response).to have_gitlab_http_status(:created)
+              expect(member.tasks_to_be_done).to match_array([:code, :ci])
+              expect(member.member_task.project_id).to eq(project_id)
+            end
+
+            it 'adds tasks to be done if they do not exist', :aggregate_failures do
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: stranger.id,
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.to change(MemberTask, :count).by(1)
+
+              member = source.members.find_by(user_id: stranger.id)
+              expect(response).to have_gitlab_http_status(:created)
+              expect(member.tasks_to_be_done).to match_array([:issues])
+              expect(member.member_task.project_id).to eq(project_id)
+            end
+          end
         end
 
         context 'when there are multiple users to add' do
@@ -412,14 +474,68 @@ RSpec.describe API::Members do
               expect(member.member_task.project_id).to eq(project_id)
             end
           end
+
+          context 'with already existing members' do
+            before do
+              source.add_developer(stranger)
+              source.add_developer(developer)
+            end
+
+            it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+              members = source.members.where(user_id: [developer.id, stranger.id])
+              members.each do |member|
+                create(:member_task, member: member, project_id: project_id, tasks_to_be_done: %w(code ci))
+              end
+
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: [developer.id, stranger.id].join(','),
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.not_to change(MemberTask, :count)
+
+              expect(response).to have_gitlab_http_status(:created)
+              members.each do |member|
+                member.reset
+                expect(member.tasks_to_be_done).to match_array([:code, :ci])
+                expect(member.member_task.project_id).to eq(project_id)
+              end
+            end
+
+            it 'adds tasks to be done if they do not exist', :aggregate_failures do
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: [developer.id, stranger.id].join(','),
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.to change(MemberTask, :count).by(2)
+
+              expect(response).to have_gitlab_http_status(:created)
+              members = source.members.where(user_id: [developer.id, stranger.id])
+              members.each do |member|
+                expect(member.tasks_to_be_done).to match_array([:issues])
+                expect(member.member_task.project_id).to eq(project_id)
+              end
+            end
+          end
         end
       end
 
-      it "returns 409 if member already exists" do
+      it "updates a current member" do
+        source.add_guest(stranger)
+
         post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-             params: { user_id: maintainer.id, access_level: Member::MAINTAINER }
+             params: { user_id: stranger.id, access_level: Member::MAINTAINER }
 
-        expect(response).to have_gitlab_http_status(:conflict)
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['id']).to eq(stranger.id)
+        expect(json_response['access_level']).to eq(Member::MAINTAINER)
       end
 
       it 'returns 404 when the user_id is not valid' do
diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb
index b3bfdfbcc4d..4d9e69719b4 100644
--- a/spec/services/members/create_service_spec.rb
+++ b/spec/services/members/create_service_spec.rb
@@ -117,6 +117,24 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
           user: user
         )
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_developer(member)
+        end
+
+        it 'tracks the invite source from params' do
+          execute_service
+
+          expect_snowplow_event(
+            category: described_class.name,
+            action: 'create_member',
+            label: '_invite_source_',
+            property: 'existing_user',
+            user: user
+          )
+        end
+      end
     end
 
     context 'when it is a net_new_user' do
diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb
index 5b4b8c8fcc1..f7e09cfca62 100644
--- a/spec/support/shared_examples/models/member_shared_examples.rb
+++ b/spec/support/shared_examples/models/member_shared_examples.rb
@@ -301,8 +301,9 @@ RSpec.shared_examples_for "member creation" do
     end
 
     context 'when `tasks_to_be_done` and `tasks_project_id` are passed' do
+      let(:task_project) { source.is_a?(Group) ? create(:project, group: source) : source }
+
       it 'creates a member_task with the correct attributes', :aggregate_failures do
-        task_project = source.is_a?(Group) ? create(:project, group: source) : source
         described_class.new(source, user, :developer, tasks_to_be_done: %w(ci code), tasks_project_id: task_project.id).execute
 
         member = source.members.last
@@ -310,6 +311,43 @@ RSpec.shared_examples_for "member creation" do
         expect(member.tasks_to_be_done).to match_array([:ci, :code])
         expect(member.member_task.project).to eq(task_project)
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_user(user, :developer)
+        end
+
+        it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+          member = source.members.find_by(user_id: user.id)
+          create(:member_task, member: member, project: task_project, tasks_to_be_done: %w(code ci))
+
+          expect do
+            described_class.new(source,
+                                user,
+                                :developer,
+                                tasks_to_be_done: %w(issues),
+                                tasks_project_id: task_project.id).execute
+          end.not_to change(MemberTask, :count)
+
+          member.reset
+          expect(member.tasks_to_be_done).to match_array([:code, :ci])
+          expect(member.member_task.project).to eq(task_project)
+        end
+
+        it 'adds tasks to be done if they do not exist', :aggregate_failures do
+          expect do
+            described_class.new(source,
+                                user,
+                                :developer,
+                                tasks_to_be_done: %w(issues),
+                                tasks_project_id: task_project.id).execute
+          end.to change(MemberTask, :count).by(1)
+
+          member = source.members.find_by(user_id: user.id)
+          expect(member.tasks_to_be_done).to match_array([:issues])
+          expect(member.member_task.project).to eq(task_project)
+        end
+      end
     end
   end
 end
@@ -393,14 +431,52 @@ RSpec.shared_examples_for "bulk member creation" do
     end
 
     context 'when `tasks_to_be_done` and `tasks_project_id` are passed' do
+      let(:task_project) { source.is_a?(Group) ? create(:project, group: source) : source }
+
       it 'creates a member_task with the correct attributes', :aggregate_failures do
-        task_project = source.is_a?(Group) ? create(:project, group: source) : source
         members = described_class.add_users(source, [user1], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: task_project.id)
         member = members.last
 
         expect(member.tasks_to_be_done).to match_array([:ci, :code])
         expect(member.member_task.project).to eq(task_project)
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_user(user1, :developer)
+        end
+
+        it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+          member = source.members.find_by(user_id: user1.id)
+          create(:member_task, member: member, project: task_project, tasks_to_be_done: %w(code ci))
+
+          expect do
+            described_class.add_users(source,
+                                      [user1.id],
+                                      :developer,
+                                      tasks_to_be_done: %w(issues),
+                                      tasks_project_id: task_project.id)
+          end.not_to change(MemberTask, :count)
+
+          member.reset
+          expect(member.tasks_to_be_done).to match_array([:code, :ci])
+          expect(member.member_task.project).to eq(task_project)
+        end
+
+        it 'adds tasks to be done if they do not exist', :aggregate_failures do
+          expect do
+            described_class.add_users(source,
+                                      [user1.id],
+                                      :developer,
+                                      tasks_to_be_done: %w(issues),
+                                      tasks_project_id: task_project.id)
+          end.to change(MemberTask, :count).by(1)
+
+          member = source.members.find_by(user_id: user1.id)
+          expect(member.tasks_to_be_done).to match_array([:issues])
+          expect(member.member_task.project).to eq(task_project)
+        end
+      end
     end
   end
 end