Automatic merge of gitlab-org/gitlab master

app/services/members/create_service.rb

@@ -62,10 +62,18 @@ def cannot_assign_owner_responsibilities_to_member_in_project?
 
     def invites_from_params
       # String, Nil, Array, Integer
-      return params[:user_id] if params[:user_id].is_a?(Array)
-      return [] unless params[:user_id]
+      users = param_to_array(params[:user_id] || params[:username])
+      if params.key?(:username)
+        User.by_username(users).pluck_primary_key
+      else
+        users.to_a
+      end
+    end
+
+    def param_to_array(param)
+      return param if param.is_a?(Array)
 
-      params[:user_id].to_s.split(',').uniq
+      param.to_s.split(',').uniq
     end
 
     def validate_source_type!

doc/api/members.md

@@ -586,10 +586,11 @@ POST /projects/:id/members
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `id`      | integer/string | yes | The ID or [URL-encoded path of the project or group](rest/index.md#namespaced-path-encoding) owned by the authenticated user |
-| `user_id` | integer/string | yes | The user ID of the new member or multiple IDs separated by commas |
-| `access_level` | integer | yes | [A valid access level](access_requests.md#valid-access-levels) |
-| `expires_at` | string | no | A date string in the format `YEAR-MONTH-DAY` |
+| `id`      | integer/string | yes | The ID or [URL-encoded path of the project or group](rest/index.md#namespaced-path-encoding) owned by the authenticated user. |
+| `user_id` | integer/string | yes, if `username` is not provided | The user ID of the new member or multiple IDs separated by commas. |
+| `username` | string | yes, if `user_id` is not provided | The username of the new member or multiple usernames separated by commas. |
+| `access_level` | integer | yes | [A valid access level](access_requests.md#valid-access-levels). |
+| `expires_at` | string | no | A date string in the format `YEAR-MONTH-DAY`. |
 | `invite_source` | string | no | The source of the invitation that starts the member creation process. GitLab team members can view more information in this confidential issue: `https://gitlab.com/gitlab-org/gitlab/-/issues/327120>`. |
 | `member_role_id` | integer | no | The ID of a member role. Ultimate only. |
 

ee/app/views/shared/_saml_reload_modal.html.haml

 - return unless current_user
 
 - root_group = group_or_project.root_ancestor
-- return unless Feature.enabled?(:saml_reload, root_group)
-
 - saml_provider = root_group.respond_to?(:saml_provider) && root_group.saml_provider
 - return unless saml_provider
 

ee/config/feature_flags/gitlab_com_derisk/saml_reload.yml

----
-name: saml_reload
-feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/419578
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142569
-rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17452
-milestone: '16.11'
-group: group::authentication
-type: gitlab_com_derisk
-default_enabled: false

ee/spec/views/shared/_saml_reload_modal.haml_spec.rb

@@ -13,7 +13,6 @@
 
   before do
     allow(view).to receive(:current_user).and_return(user)
-    stub_feature_flags(saml_reload: true)
   end
 
   context 'with root group' do

lib/api/helpers.rb

@@ -12,8 +12,6 @@ module Helpers
 
     SUDO_HEADER = "HTTP_SUDO"
     GITLAB_SHARED_SECRET_HEADER = "Gitlab-Shared-Secret"
-    GITLAB_SHELL_API_HEADER = "Gitlab-Shell-Api-Request"
-    GITLAB_SHELL_JWT_ISSUER = "gitlab-shell"
     SUDO_PARAM = :sudo
     API_USER_ENV = 'gitlab.api.user'
     API_TOKEN_ENV = 'gitlab.api.token'
@@ -341,15 +339,11 @@ def authenticate_non_get!
     end
 
     def authenticate_by_gitlab_shell_token!
-      payload, _ = JSONWebToken::HMACToken.decode(headers[GITLAB_SHELL_API_HEADER], secret_token)
-      unauthorized! unless payload['iss'] == GITLAB_SHELL_JWT_ISSUER
-    rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::ImmatureSignature => ex
-      Gitlab::ErrorTracking.track_exception(ex)
-      unauthorized!
+      unauthorized! unless Gitlab::Shell.verify_api_request(headers)
     end
 
     def authenticate_by_gitlab_shell_or_workhorse_token!
-      return require_gitlab_workhorse! unless headers[GITLAB_SHELL_API_HEADER].present?
+      return require_gitlab_workhorse! unless Gitlab::Shell.header_set?(headers)
 
       authenticate_by_gitlab_shell_token!
     end

lib/api/helpers/members_helpers.rb

@@ -88,14 +88,8 @@ def present_members_with_invited_private_group_accessibility(members, source)
         present_members members
       end
 
-      def add_single_member_by_user_id(create_service_params)
-        source = create_service_params[:source]
-        user_id = create_service_params[:user_id]
-        user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
-
-        not_found!('User') unless user
-
-        conflict!('Member already exists') if member_already_exists?(source, user_id)
+      def add_single_member(create_service_params)
+        check_existing_membership(create_service_params)
 
         instance = ::Members::CreateService.new(current_user, create_service_params)
         result = instance.execute
@@ -120,12 +114,15 @@ def add_single_member_by_user_id(create_service_params)
         end
       end
 
-      def add_multiple_members?(user_id)
-        user_id.include?(',')
+      def check_existing_membership(create_service_params)
+        user_id = User.get_ids_by_ids_or_usernames(create_service_params[:user_id], create_service_params[:username]).first
+
+        not_found!('User') unless user_id
+        conflict!('Member already exists') if member_already_exists?(create_service_params[:source], user_id)
       end
 
-      def add_single_member?(user_id)
-        user_id.present?
+      def add_multiple_members?(user_id, username)
+        user_id&.include?(',') || username&.include?(',')
       end
 
       def self.member_access_levels

lib/api/members.rb

@@ -111,9 +111,12 @@ class Members < ::API::Base
         end
         params do
           requires :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)'
-          requires :user_id, types: [Integer, String], desc: 'The user ID of the new member or multiple IDs separated by commas.'
+          optional :user_id, types: [Integer, String], desc: 'The user ID of the new member or multiple IDs separated by commas.'
+          optional :username, type: String, desc: 'The username of the new member or multiple usernames separated by commas.'
           optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
           optional :invite_source, type: String, desc: 'Source that triggered the member creation process', default: 'members-api'
+          mutually_exclusive :user_id, :username
+          at_least_one_of :user_id, :username
         end
 
         post ":id/members", feature_category: feature_category do
@@ -121,10 +124,10 @@ class Members < ::API::Base
 
           create_service_params = params.merge(source: source)
 
-          if add_multiple_members?(params[:user_id].to_s)
+          if add_multiple_members?(params[:user_id].to_s, params[:username])
             ::Members::CreateService.new(current_user, create_service_params).execute
-          elsif add_single_member?(params[:user_id].to_s)
-            add_single_member_by_user_id(create_service_params)
+          else
+            add_single_member(create_service_params)
           end
         end
 

lib/gitlab/shell.rb

@@ -14,7 +14,24 @@ module Gitlab
   class Shell
     Error = Class.new(StandardError)
 
+    API_HEADER = 'Gitlab-Shell-Api-Request'
+    JWT_ISSUER = 'gitlab-shell'
+
     class << self
+      def verify_api_request(headers)
+        payload, header = JSONWebToken::HMACToken.decode(headers[API_HEADER], secret_token)
+        return unless payload['iss'] == JWT_ISSUER
+
+        [payload, header]
+      rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::ImmatureSignature => ex
+        Gitlab::ErrorTracking.track_exception(ex)
+        nil
+      end
+
+      def header_set?(headers)
+        headers[API_HEADER].present?
+      end
+
       # Retrieve GitLab Shell secret token
       #
       # @return [String] secret token

spec/lib/gitlab/shell_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require 'stringio'
 
-RSpec.describe Gitlab::Shell do
+RSpec.describe Gitlab::Shell, feature_category: :source_code_management do
   let_it_be(:project) { create(:project, :repository) }
 
   let(:repository) { project.repository }
@@ -13,6 +13,51 @@
     described_class.instance_variable_set(:@secret_token, nil)
   end
 
+  describe '.verify_api_request' do
+    subject { described_class.verify_api_request(headers) }
+
+    let(:encoded_token) { JWT.encode(payload, described_class.secret_token, 'HS256') }
+    let(:payload) { { 'iss' => described_class::JWT_ISSUER } }
+    let(:headers) { { described_class::API_HEADER => encoded_token } }
+
+    it 'returns the decoded JWT' do
+      is_expected.to eq([
+        { 'iss' => described_class::JWT_ISSUER },
+        { 'alg' => 'HS256' }
+      ])
+    end
+
+    context 'when secret is wrong' do
+      let(:encoded_token) { JWT.encode(payload, 'wrong secret', 'HS256') }
+
+      it 'returns nil' do
+        is_expected.to be_nil
+      end
+    end
+
+    context 'when issuer is wrong' do
+      let(:payload) { { 'iss' => 'wrong issuer' } }
+
+      it 'returns nil' do
+        is_expected.to be_nil
+      end
+    end
+  end
+
+  describe '.header_set?' do
+    subject { described_class.header_set?(headers) }
+
+    let(:headers) { { described_class::API_HEADER => 'token' } }
+
+    it { is_expected.to be_truthy }
+
+    context 'when header is missing' do
+      let(:headers) { { 'another_header' => 'token' } }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
   describe '.secret_token' do
     let(:secret_file) { 'tmp/tests/.secret_shell_test' }
     let(:link_file) { 'tmp/tests/shell-secret-test/.gitlab_shell_secret' }

spec/lib/initializer_connections_spec.rb

@@ -44,7 +44,7 @@ def block_with_error
       expect { block_with_database_call }.to raise_error(/Database connection should not be called during initializer/)
     end
 
-    it 'restores original connection handler' do
+    it 'restores original connection handler', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/444963' do
       original_handler = ActiveRecord::Base.connection_handler
 
       expect { block_with_database_call }.to raise_error(/Database connection should not be called during initializer/)

spec/requests/api/members_spec.rb

@@ -637,6 +637,34 @@
             expect(json_response['message']).to eq(error_message)
           end
         end
+
+        context 'when executing the Members::CreateService for multiple usernames' do
+          let(:usernames) { [stranger.username, access_requester.username].join(',') }
+
+          it 'returns success when it successfully create all members' do
+            expect do
+              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                params: { username: usernames, access_level: Member::DEVELOPER }
+
+              expect(response).to have_gitlab_http_status(:created)
+            end.to change { source.members.count }.by(2)
+            expect(json_response['status']).to eq('success')
+          end
+
+          it 'returns the error message if there was an error adding members to group' do
+            error_message = 'Unable to find Username'
+            allow_next_instance_of(::Members::CreateService) do |service|
+              expect(service).to receive(:execute).and_return({ status: :error, message: error_message })
+            end
+
+            expect do
+              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                params: { username: usernames, access_level: Member::DEVELOPER }
+            end.not_to change { source.members.count }
+            expect(json_response['status']).to eq('error')
+            expect(json_response['message']).to eq(error_message)
+          end
+        end
       end
 
       context 'access levels' do
@@ -1047,6 +1075,32 @@ def request
       end
     end
 
+    context 'add member to project' do
+      it 'allows adding by username' do
+        post api("/projects/#{project.id}/members", maintainer),
+          params: { username: access_requester.username, access_level: Member::DEVELOPER }
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['username']).to eq(access_requester.username)
+      end
+
+      it 'returns a 400 if user_id is also provided' do
+        post api("/projects/#{project.id}/members", maintainer),
+          params: { username: access_requester.username, user_id: access_requester.id, access_level: Member::DEVELOPER }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq('user_id, username are mutually exclusive')
+      end
+
+      it 'returns a 400 if user_id and username is missing' do
+        post api("/projects/#{project.id}/members", maintainer),
+          params: { access_level: Member::DEVELOPER }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to match('at least one parameter must be provided')
+      end
+    end
+
     context 'remove bot from project' do
       it 'returns a 403 forbidden' do
         project_bot = create(:user, :project_bot)

spec/support/helpers/gitlab_shell_helpers.rb

@@ -4,11 +4,11 @@ module GitlabShellHelpers
   extend self
 
   def gitlab_shell_internal_api_request_header(
-    issuer: API::Helpers::GITLAB_SHELL_JWT_ISSUER, secret_token: Gitlab::Shell.secret_token)
+    issuer: Gitlab::Shell::JWT_ISSUER, secret_token: Gitlab::Shell.secret_token)
     jwt_token = JSONWebToken::HMACToken.new(secret_token).tap do |token|
       token.issuer = issuer
     end
 
-    { API::Helpers::GITLAB_SHELL_API_HEADER => jwt_token.encoded }
+    { Gitlab::Shell::API_HEADER => jwt_token.encoded }
   end
 end