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 similarity index 100% rename from lib/gitlab/database/gitlab_loose_foreign_keys.yml rename to 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 - post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), - params: { user_id: maintainer.id, access_level: Member::MAINTAINER } + it "updates a current member" do + source.add_guest(stranger) - expect(response).to have_gitlab_http_status(:conflict) + post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), + params: { user_id: stranger.id, access_level: Member::MAINTAINER } + + 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