some refactoring based on feedback

app/models/project.rb

@@ -404,6 +404,18 @@ class Project < ActiveRecord::Base
     self.import_data.destroy if self.import_data
   end
 
+  def import_url=(value)
+    sanitizer = Gitlab::ImportUrlSanitizer.new(value)
+    self[:import_url] = sanitizer.sanitized_url
+    create_import_data(credentials: sanitizer.credentials)
+  end
+
+  def import_url
+    if import_data
+      Gitlab::ImportUrlExposer.expose(import_url: self[:import_url], credentials: import_data.credentials)
+    end
+  end
+
   def import?
     external_import? || forked?
   end

db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb

@@ -1,29 +1,5 @@
 class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
 
-  class ImportUrlSanitizer
-    def initialize(url)
-      @url = URI.parse(url)
-    end
-
-    def sanitized_url
-      @sanitized_url ||= safe_url
-    end
-
-    def credentials
-      @credentials ||= { user: @url.user, password: @url.password }
-    end
-
-    private
-
-    def safe_url
-      safe_url = @url.dup
-      safe_url.password = nil
-      safe_url.user = nil
-      safe_url
-    end
-
-  end
-
   class FakeProjectImportData
     extend AttrEncrypted
     attr_accessor :credentials
@@ -31,20 +7,30 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
   end
 
   def up
-    projects_with_wrong_import_url.each do |project|
+    projects_with_wrong_import_url.find_in_batches do |project_batch|
-      sanitizer = ImportUrlSanitizer.new(project["import_url"])
+      project_batch.each do |project|
+        sanitizer = Gitlab::ImportUrlSanitizer.new(project["import_url"])
 
-      ActiveRecord::Base.transaction do
+        ActiveRecord::Base.transaction do
-        execute("UPDATE projects SET import_url = '#{sanitizer.sanitized_url}' WHERE id = #{project['id']}")
+          execute("UPDATE projects SET import_url = '#{quote(sanitizer.sanitized_url)}' WHERE id = #{project['id']}")
-        fake_import_data = FakeProjectImportData.new
+          fake_import_data = FakeProjectImportData.new
-        fake_import_data.credentials = sanitizer.credentials
+          fake_import_data.credentials = sanitizer.credentials
-        execute("UPDATE project_import_data SET encrypted_credentials = '#{fake_import_data.encrypted_credentials}' WHERE project_id = #{project['id']}")
+          execute("UPDATE project_import_data SET encrypted_credentials = '#{quote(fake_import_data.encrypted_credentials)}' WHERE project_id = #{project['id']}")
+        end
       end
     end
   end
 
+  def down
+
+  end
+
   def projects_with_wrong_import_url
     # TODO Check live with #operations for possible false positives. Also, consider regex? But may have issues MySQL/PSQL
-    select_all("SELECT p.id, p.import_url from projects p WHERE p.import_url LIKE '%//%:%@%' or p.import_url like '#{"_"*40}@github.com%'")
+    select_all("SELECT p.id, p.import_url FROM projects p WHERE p.import_url IS NOT NULL AND (p.import_url LIKE '%//%:%@%' OR p.import_url LIKE '#{"_"*40}@github.com%')")
+  end
+
+  def quote(value)
+    ActiveRecord::Base.connection.quote(value)
   end
 end

db/schema.rb

@@ -416,7 +416,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.string   "state"
     t.integer  "iid"
     t.integer  "updated_by_id"
-    t.boolean  "confidential",              default: false
+    t.boolean  "confidential",  default: false
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -684,6 +684,8 @@ ActiveRecord::Schema.define(version: 20160316204731) do
   create_table "project_import_data", force: :cascade do |t|
     t.integer "project_id"
     t.text    "data"
+    t.text    "encrypted_credentials"
+    t.text    "encrypted_credentials_iv"
   end
 
   create_table "projects", force: :cascade do |t|

lib/gitlab/github_import/importer.rb

@@ -8,7 +8,7 @@ module Gitlab
       def initialize(project)
         @project = project
         credentials = project.import_data.credentials if import_data
-        @client = Client.new(credentials["github_access_token"])
+        @client = Client.new(credentials[:user])
         @formatter = Gitlab::ImportFormatter.new
       end
 

lib/gitlab/github_import/project_creator.rb

@@ -11,7 +11,7 @@ module Gitlab
       end
 
       def execute
-        project = ::Projects::CreateService.new(
+        ::Projects::CreateService.new(
           current_user,
           name: repo.name,
           path: repo.name,
@@ -20,19 +20,9 @@ module Gitlab
           visibility_level: repo.private ? Gitlab::VisibilityLevel::PRIVATE : Gitlab::VisibilityLevel::PUBLIC,
           import_type: "github",
           import_source: repo.full_name,
-          import_url: repo.clone_url,
+          import_url: repo.clone_url.sub("https://", "https://#{@session_data[:github_access_token]}@"),
           wiki_enabled: !repo.has_wiki? # If repo has wiki we'll import it later
         ).execute
-
-        create_import_data(project)
-        project
-      end
-
-      private
-
-      def create_import_data(project)
-        project.create_import_data(
-          credentials: { github_access_token: session_data.delete(:github_access_token) })
       end
     end
   end

lib/gitlab/github_import/wiki_formatter.rb

@@ -12,9 +12,7 @@ module Gitlab
       end
 
       def import_url
-        import_url = Gitlab::ImportUrlExposer.expose(import_url: project.import_url,
+        project.import_url.import_url.sub(/\.git\z/, ".wiki.git")
-                                                     credentials: project.import_data.credentials)
-        import_url.sub(/\.git\z/, ".wiki.git")
       end
     end
   end

lib/gitlab/import_url_sanitizer.rb

@@ -0,0 +1,24 @@
+module Gitlab
+  class ImportUrlSanitizer
+    def initialize(url)
+      @url = URI.parse(url)
+    end
+
+    def sanitized_url
+      @sanitized_url ||= safe_url.to_s
+    end
+
+    def credentials
+      @credentials ||= { user: @url.user, password: @url.password }
+    end
+
+    private
+
+    def safe_url
+      safe_url = @url.dup
+      safe_url.password = nil
+      safe_url.user = nil
+      safe_url
+    end
+  end
+end