more refactoring

app/models/project.rb

@@ -405,14 +405,17 @@ class Project < ActiveRecord::Base
   end
 
   def import_url=(value)
-    sanitizer = Gitlab::ImportUrlSanitizer.new(value)
-    self[:import_url] = sanitizer.sanitized_url
-    create_import_data(credentials: sanitizer.credentials)
+    import_url = Gitlab::ImportUrl.new(value)
+    create_import_data(credentials: import_url.credentials)
+    super(import_url.sanitized_url)
   end
 
   def import_url
     if import_data
-      Gitlab::ImportUrlExposer.expose(import_url: self[:import_url], credentials: import_data.credentials)
+      import_url = Gitlab::ImportUrl.new(super, credentials: import_data.credentials)
+      import_url.full_url
+    else
+      super
     end
   end
 
@@ -447,6 +450,7 @@ class Project < ActiveRecord::Base
   def safe_import_url
     result = URI.parse(self.import_url)
     result.password = '*****' unless result.password.nil?
+    result.user = '*****' unless result.user.nil? #tokens or other data may be saved as user
     result.to_s
   rescue
     self.import_url

db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb

+# Loops through old importer projects that kept a token/password in the import URL
+# and encrypts the credentials into a separate field in project#import_data
+# #down method not supported
 class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
 
   class FakeProjectImportData
@@ -7,24 +10,18 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
   end
 
   def up
-    projects_with_wrong_import_url.find_in_batches do |project_batch|
-      project_batch.each do |project|
-        sanitizer = Gitlab::ImportUrlSanitizer.new(project["import_url"])
+    projects_with_wrong_import_url do |project|
+      import_url = Gitlab::ImportUrl.new(project["import_url"])
 
-        ActiveRecord::Base.transaction do
-          execute("UPDATE projects SET import_url = '#{quote(sanitizer.sanitized_url)}' WHERE id = #{project['id']}")
-          fake_import_data = FakeProjectImportData.new
-          fake_import_data.credentials = sanitizer.credentials
-          execute("UPDATE project_import_data SET encrypted_credentials = '#{quote(fake_import_data.encrypted_credentials)}' WHERE project_id = #{project['id']}")
-        end
+      ActiveRecord::Base.transaction do
+        execute("UPDATE projects SET import_url = '#{quote(import_url.sanitized_url)}' WHERE id = #{project['id']}")
+        fake_import_data = FakeProjectImportData.new
+        fake_import_data.credentials = import_url.credentials
+        execute("UPDATE project_import_data SET encrypted_credentials = '#{quote(fake_import_data.encrypted_credentials)}' WHERE project_id = #{project['id']}")
       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 IS NOT NULL AND (p.import_url LIKE '%//%:%@%' OR p.import_url LIKE '#{"_"*40}@github.com%')")

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160316204731) do
+ActiveRecord::Schema.define(version: 20160302151724) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -260,7 +260,6 @@ ActiveRecord::Schema.define(version: 20160316204731) do
   end
 
   add_index "ci_runners", ["description"], name: "index_ci_runners_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
-  add_index "ci_runners", ["token"], name: "index_ci_runners_on_token", using: :btree
   add_index "ci_runners", ["token"], name: "index_ci_runners_on_token_trigram", using: :gin, opclasses: {"token"=>"gin_trgm_ops"}
 
   create_table "ci_services", force: :cascade do |t|
@@ -686,6 +685,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.text    "data"
     t.text    "encrypted_credentials"
     t.text    "encrypted_credentials_iv"
+    t.text    "encrypted_credentials_salt"
   end
 
   create_table "projects", force: :cascade do |t|
@@ -725,7 +725,6 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.boolean  "pending_delete",         default: false
     t.boolean  "public_builds",          default: true,     null: false
     t.string   "main_language"
-    t.integer  "pushes_since_gc",        default: 0
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree
@@ -811,6 +810,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.string   "file_name"
     t.string   "type"
     t.integer  "visibility_level", default: 0, null: false
+    t.datetime "expires_at"
   end
 
   add_index "snippets", ["author_id"], name: "index_snippets_on_author_id", using: :btree
@@ -869,7 +869,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
   create_table "todos", force: :cascade do |t|
     t.integer  "user_id",     null: false
     t.integer  "project_id",  null: false
-    t.integer  "target_id"
+    t.integer  "target_id",   null: false
     t.string   "target_type", null: false
     t.integer  "author_id"
     t.integer  "action",      null: false
@@ -877,11 +877,9 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "note_id"
-    t.string   "commit_id"
   end
 
   add_index "todos", ["author_id"], name: "index_todos_on_author_id", using: :btree
-  add_index "todos", ["commit_id"], name: "index_todos_on_commit_id", using: :btree
   add_index "todos", ["note_id"], name: "index_todos_on_note_id", using: :btree
   add_index "todos", ["project_id"], name: "index_todos_on_project_id", using: :btree
   add_index "todos", ["state"], name: "index_todos_on_state", using: :btree
@@ -946,7 +944,6 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.string   "unlock_token"
     t.datetime "otp_grace_period_started_at"
     t.boolean  "ldap_email",                  default: false, null: false
-    t.boolean  "external",                    default: false
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

lib/gitlab/github_import/importer.rb

@@ -7,9 +7,12 @@ module Gitlab
 
       def initialize(project)
         @project = project
-        credentials = project.import_data.credentials if import_data
-        @client = Client.new(credentials[:user])
-        @formatter = Gitlab::ImportFormatter.new
+        if import_data_credentials
+          @client = Client.new(import_data_credentials[:user])
+          @formatter = Gitlab::ImportFormatter.new
+        else
+          raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}"
+        end
       end
 
       def execute
@@ -18,6 +21,10 @@ module Gitlab
 
       private
 
+      def import_data_credentials
+        @import_data_credentials ||= project.import_data.credentials if project.import_data
+      end
+
       def import_issues
         client.list_issues(project.import_source, state: :all,
                                                   sort: :created,

lib/gitlab/github_import/wiki_formatter.rb

@@ -12,7 +12,7 @@ module Gitlab
       end
 
       def import_url
-        project.import_url.import_url.sub(/\.git\z/, ".wiki.git")
+        project.import_url.sub(/\.git\z/, ".wiki.git")
       end
     end
   end

lib/gitlab/import_url_sanitizer.rb → lib/gitlab/import_url.rb

 module Gitlab
-  class ImportUrlSanitizer
-    def initialize(url)
+  class ImportUrl
+    def initialize(url, credentials: nil)
       @url = URI.parse(url)
+      @credentials = credentials
     end
 
     def sanitized_url
@@ -12,8 +13,19 @@ module Gitlab
       @credentials ||= { user: @url.user, password: @url.password }
     end
 
+    def full_url
+      @full_url ||= generate_full_url.to_s
+    end
+
     private
 
+    def generate_full_url
+      @full_url = @url.dup
+      @full_url.user = @credentials[:user]
+      @full_url.password = @credentials[:password]
+      @full_url
+    end
+
     def safe_url
       safe_url = @url.dup
       safe_url.password = nil

lib/gitlab/import_url_exposer.rb

-module Gitlab
-  # Exposes an import URL that includes the credentials unencrypted.
-  # Extracted to its own class to prevent unintended use.
-  module ImportUrlExposer
-
-    def self.expose(import_url:, credentials: )
-      uri = URI.parse(import_url)
-      uri.user = credentials[:user]
-      uri.password = credentials[:password]
-      uri
-    end
-  end
-end

spec/lib/gitlab/github_import/project_creator_spec.rb

@@ -26,8 +26,9 @@ describe Gitlab::GithubImport::ProjectCreator, lib: true do
     project_creator = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, user, access_params)
     project = project_creator.execute
 
-    expect(project.import_url).to eq("https://gitlab.com/asd/vim.git")
-    expect(project.import_data.credentials).to eq(:github_access_token => "asdffg")
+    expect(project.import_url).to eq("https://asdffg@gitlab.com/asd/vim.git")
+    expect(project.safe_import_url).to eq("https://*****@gitlab.com/asd/vim.git")
+    expect(project.import_data.credentials).to eq(:user => "asdffg", :password => nil)
     expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
   end
 end

spec/lib/gitlab/github_import/wiki_formatter_spec.rb

@@ -3,12 +3,7 @@ require 'spec_helper'
 describe Gitlab::GithubImport::WikiFormatter, lib: true do
   let(:project) do
     create(:project, namespace: create(:namespace, path: 'gitlabhq'),
-           import_url: 'https://github.com/gitlabhq/sample.gitlabhq.git')
-  end
-
-  let!(:project_import_data) do
-    create(:project_import_data, credentials: { github_access_token: 'xxx' },
-           project: project)
+           import_url: 'https://xxx@github.com/gitlabhq/sample.gitlabhq.git')
   end
 
   subject(:wiki) { described_class.new(project) }

spec/lib/gitlab/import_url_exposer_spec.rb

-require 'spec_helper'
-
-describe 'Gitlab::ImportUrlExposer' do
-
-  describe :expose do
-    let(:credentials) do
-      Gitlab::ImportUrlExposer.expose(import_url: "https://github.com/me/project.git", credentials: {user: 'blah', password: 'password'})
-    end
-
-    it { expect(credentials).to be_a(URI) }
-    it { expect(credentials.to_s).to eq("https://blah:password@github.com/me/project.git") }
-  end
-end

spec/lib/gitlab/import_url_spec.rb

+require 'spec_helper'
+
+describe Gitlab::ImportUrl do
+
+  let(:credentials) { { user: 'blah', password: 'password' } }
+  let(:import_url) do
+    Gitlab::ImportUrl.new("https://github.com/me/project.git", credentials: credentials)
+  end
+
+  describe :full_url do
+    it { expect(import_url.full_url).to eq("https://blah:password@github.com/me/project.git") }
+  end
+
+  describe :sanitized_url do
+    it { expect(import_url.sanitized_url).to eq("https://github.com/me/project.git") }
+  end
+
+  describe :credentials do
+    it { expect(import_url.credentials).to eq(credentials) }
+  end
+end