Merge branch...

Merge branch '939-repositoryupdatemirrorworker-updatemirrorerror-rugged-invaliderror-cannot-set-empty-url' into 'master'

Fix mirrored projects allowing empty import urls

Because of missing validations in the projects model, you could
create a new project with a nil or empty import_url with mirror=true.
This would cause exceptions when
RepositoryUpdateMirrorWorker::UpdateMirrorError ran and attempted to
update one of these projects. This was made easier by the fact that
you could show the "Mirror repository" checkbox in the UI, check it,
hide it, and still have its value considered.

The missing validations have been added, and minor adjustments have
been made to the UI to improve the previous behaviour. A migration
was also added to update mirror=false in all projects with a nil or
empty import url that exists because of the described bug.

Closes #939

See merge request !700

CHANGELOG-EE

@@ -10,6 +10,9 @@ v 8.11.6
   - Exclude blocked users from potential MR approvers
   - Add 'Sync now' to group members page !704
 
+v 8.11.6
+  - Fix mirrored projects allowing empty import urls
+
 v 8.11.5
   - API: Restore backward-compatibility for POST /projects/:id/members when membership is locked
 

app/models/project.rb

@@ -169,7 +169,6 @@ class Project < ActiveRecord::Base
   validates_uniqueness_of :name, scope: :namespace_id
   validates_uniqueness_of :path, scope: :namespace_id
   validates :import_url, addressable_url: true, if: :external_import?
-  validates :mirror_user, presence: true, if: :mirror?
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :avatar_type,
@@ -183,6 +182,11 @@ class Project < ActiveRecord::Base
     presence: true,
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
 
+  with_options if: :mirror? do |project|
+    project.validates :import_url, presence: true
+    project.validates :mirror_user, presence: true
+  end
+
   add_authentication_token_field :runners_token
   before_save :ensure_runners_token
   before_validation :mark_remote_mirrors_for_removal

app/views/projects/new.html.haml

@@ -139,5 +139,8 @@
 
   $('.import_git').click(function( event ) {
     $projectImportUrl = $('#project_import_url')
+    $projectMirror = $('#project_mirror')
+
     $projectImportUrl.attr('disabled', !$projectImportUrl.attr('disabled'))
+    $projectMirror.attr('disabled', !$projectMirror.attr('disabled'))
   });

app/views/shared/_import_form.html.haml

@@ -19,7 +19,7 @@
   .col-sm-offset-2.col-sm-10
     .checkbox
       = f.label :mirror do
-        = f.check_box :mirror
+        = f.check_box :mirror, disabled: true
         %strong
           Mirror repository
       .help-block

db/migrate/20160831231325_update_mirror_when_empty_import_url_in_projects.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class UpdateMirrorWhenEmptyImportUrlInProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    update_column_in_batches(:projects, :mirror, false) do |table, query|
+      query.where(table[:import_url].eq(nil).or(table[:import_url].eq('')))
+    end
+  end
+end

lib/gitlab/url_sanitizer.rb

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

spec/lib/gitlab/url_sanitizer_spec.rb

@@ -70,4 +70,12 @@ describe Gitlab::UrlSanitizer, lib: true do
       expect(sanitizer.full_url).to eq('user@server:project.git')
     end
   end
+
+  describe '.valid?' do
+    it 'validates url strings' do
+      expect(described_class.valid?(nil)).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
+  end
 end

spec/models/project_spec.rb

@@ -134,6 +134,13 @@ describe Project, models: true do
       end
     end
 
+    context 'mirror' do
+      subject { build(:project, mirror: true) }
+
+      it { is_expected.to validate_presence_of(:import_url) }
+      it { is_expected.to validate_presence_of(:mirror_user) }
+    end
+
     it 'does not allow an invalid URI as import_url' do
       project2 = build(:project, import_url: 'invalid://')