Merge branch 'atevans-improve-external-username-sanitizer' into 'master'

Improve extra_slug_path_sanitization account creation

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147971



Merged-by: Aboobacker MK <akarakath@gitlab.com>
Approved-by: Rohit Shambhuni <rshambhuni@gitlab.com>
Approved-by: Aboobacker MK <akarakath@gitlab.com>
Co-authored-by: agius <andrew@atevans.com>

ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb

@@ -161,6 +161,17 @@ def user
           expect(user.username).to eq('ricky.the.raccoon')
         end
 
+        context 'and there is an existing user with the sanitized username' do
+          before do
+            create(:user, :with_namespace, username: 'ricky.the.raccoon')
+          end
+
+          it 'creates new user with non-conflicting username' do
+            expect { result }.to change { User.count }.by(1)
+            expect(user.username).to eq('ricky.the.raccoon1')
+          end
+        end
+
         context 'and extra_slug_sanitization FF is disabled' do
           before do
             stub_feature_flags(extra_slug_path_sanitization: false)

ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb

@@ -110,6 +110,17 @@ def user
           expect(user.username).to eq('ricky.the.raccoon')
         end
 
+        context 'and there is an existing user with the sanitized username' do
+          before do
+            create(:user, :with_namespace, username: 'ricky.the.raccoon')
+          end
+
+          it 'creates new user with non-conflicting username' do
+            expect { result }.to change { User.count }.by(1)
+            expect(user.username).to eq('ricky.the.raccoon1')
+          end
+        end
+
         context 'and extra_slug_sanitization FF is disabled' do
           before do
             stub_feature_flags(extra_slug_path_sanitization: false)

lib/gitlab/auth/external_username_sanitizer.rb

@@ -11,14 +11,33 @@ def initialize(external_username)
 
       def sanitize
         # remove most characters illegal in usernames / slugs
-        valid_username = ::Namespace.clean_path(external_username)
+        slug = Gitlab::Slug::Path.new(external_username).generate
+
         # remove leading - , _ , or . characters not removed by Namespace.clean_path
-        valid_username = valid_username.sub(/\A[_.-]+/, '')
+        slug = slug.sub(/\A[_.-]+/, '')
+
         # remove trailing - , _ or . characters not removed by Namespace.clean_path
-        valid_username = valid_username.sub(/[_.-]+\z/, '')
+        # hard to write a regex to match end-of-string without ReDoS, so just use plain Ruby
+        slug = slug[0...-1] while slug.end_with?('.', '-', '_')
+
         # remove consecutive - , _ , or . characters
-        valid_username = valid_username.gsub(/([_.-])[_.-]+/, '\1')
-        Gitlab::Utils::Uniquify.new.string(valid_username) do |s|
+        slug = slug.gsub(/([_.-])[_.-]+/, '\1')
+
+        slug = unique_by_namespace(slug)
+
+        validated_path(slug)
+      end
+
+      # decomposed from Namespace.clean_path
+      def unique_by_namespace(slug)
+        path = Namespaces::RandomizedSuffixPath.new(slug).to_s
+        Gitlab::Utils::Uniquify.new.string(path) do |s|
+          Namespace.all.find_by_path_or_name(s)
+        end
+      end
+
+      def validated_path(path)
+        Gitlab::Utils::Uniquify.new.string(path) do |s|
           !NamespacePathValidator.valid_path?(s)
         end
       end

spec/lib/gitlab/auth/external_username_sanitizer_spec.rb

@@ -26,4 +26,17 @@
       it { is_expected.to eq(output) }
     end
   end
+
+  context 'when external username is a ReDoS pattern' do
+    let(:external_username) { "#{'-' * 54773}\x00-z" }
+
+    it { is_expected.to eq('z') }
+  end
+
+  context 'with existing users' do
+    let!(:user_capybara) { create(:user, :with_namespace, username: 'carly_the_capybara') }
+    let(:external_username) { '___carly_the_capybara' }
+
+    it { is_expected.to eq('carly_the_capybara1') }
+  end
 end

spec/lib/gitlab/auth/o_auth/user_spec.rb

@@ -908,6 +908,13 @@ def result_identities(dn, uid)
 
         expect(oauth_user2.gl_user.username).to eq('johngitlab-ETC1')
       end
+
+      it 'generates the username with a counter for special characters' do
+        oauth_user.save # rubocop:disable Rails/SaveBang -- not an ActiveRecord model, no save! method
+        oauth_user2 = described_class.new(OmniAuth::AuthHash.new(uid: 'my-uid2', provider: provider, info: { nickname: 'johngitlab---ETC@othermail.com', email: 'john@othermail.com' }))
+
+        expect(oauth_user2.gl_user.username).to eq('johngitlab-ETC1')
+      end
     end
 
     context 'when username is a reserved word' do