Merge branch '3288-fix-repository-mirroring-when-url-is-empty-string' into 'master'

Fix a regression breaking projects with an empty import URL

Closes #3288

See merge request !2824

app/validators/addressable_url_validator.rb

@@ -29,9 +29,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   private
 
   def valid_url?(value)
-    return false unless value
-
-    valid_protocol?(value) && valid_uri?(value)
+    valid_uri?(value) && valid_protocol?(value)
   end
 
   def valid_uri?(value)

changelogs/unreleased-ee/3288-fix-repository-mirroring-when-url-is-empty-string.yml

+---
+title: Fix a regression breaking projects with an empty import URL
+merge_request: 2824
+author:
+type: fixed

ee/app/models/ee/project.rb

@@ -384,7 +384,8 @@ module EE
     def username_only_import_url=(value)
       unless ::Gitlab::UrlSanitizer.valid?(value)
         self.import_url = value
-        return
+        self.import_data&.user = nil
+        return value
       end
 
       url = ::Gitlab::UrlSanitizer.new(value)

lib/gitlab/url_sanitizer.rb

@@ -9,7 +9,7 @@ module Gitlab
     end
 
     def self.valid?(url)
-      return false unless url
+      return false unless url.present?
 
       Addressable::URI.parse(url.strip)
 

spec/ee/spec/models/ee/project_spec.rb

@@ -814,28 +814,24 @@ describe Project do
   end
 
   describe '#username_only_import_url' do
-    def build_project(username: 'user', password: 'password')
-      build(:project, import_url: 'http://example.com').tap do |project|
-        project.build_import_data(credentials: { user: username, password: password })
-      end
-    end
+    where(:import_url, :username, :expected_import_url) do
+      '' | 'foo' | ''
+      '' | ''    | ''
+      '' | nil   | ''
 
-    it 'shows the bare url when no username is present' do
-      project = build_project(username: nil)
+      nil | 'foo' | nil
+      nil | ''    | nil
+      nil | nil   | nil
 
-      expect(project.username_only_import_url).to eq('http://example.com')
+      'http://example.com' | 'foo' | 'http://foo@example.com'
+      'http://example.com' | ''    | 'http://example.com'
+      'http://example.com' | nil   | 'http://example.com'
     end
 
-    it 'shows the URL with username when present' do
-      project = build_project(password: nil)
+    with_them do
+      let(:project) { build(:project, import_url: import_url, import_data_attributes: { user: username, password: 'password' }) }
 
-      expect(project.username_only_import_url).to eq('http://user@example.com')
-    end
-
-    it 'excludes the pasword when present' do
-      project = build_project
-
-      expect(project.username_only_import_url).to eq('http://user@example.com')
+      it { expect(project.username_only_import_url).to eq(expected_import_url) }
     end
   end
 
@@ -855,5 +851,15 @@ describe Project do
       expect(project.import_url).to eq('http://user:pass@example.com')
       expect(project.import_data.password).to eq('pass')
     end
+
+    it 'clears the username if passed the empty string' do
+      project = build(:project, import_url: 'http://olduser:pass@old.example.com')
+      project.username_only_import_url = ''
+
+      expect(project.username_only_import_url).to eq('')
+      expect(project.import_url).to eq('')
+      expect(project.import_data.user).to be_nil
+      expect(project.import_data.password).to eq('pass')
+    end
   end
 end

spec/lib/gitlab/url_sanitizer_spec.rb

@@ -57,6 +57,7 @@ describe Gitlab::UrlSanitizer do
   describe '.valid?' do
     it 'validates url strings' do
       expect(described_class.valid?(nil)).to be(false)
+      expect(described_class.valid?('')).to be(false)
       expect(described_class.valid?('valid@project:url.git')).to be(true)
       expect(described_class.valid?('123://invalid:url')).to be(false)
     end