diff --git a/app/models/project.rb b/app/models/project.rb index ab4afd4159e..4e5fa8821ea 100644 --- a/app/models/project.rb +++ b/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 diff --git a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb index f98ec925ac4..fd718ef3974 100644 --- a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb +++ b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb @@ -1,3 +1,6 @@ +# 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%')") diff --git a/db/schema.rb b/db/schema.rb index 72a3ec2fb10..02300c028d4 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index d407be5dcf4..0b1ed510229 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/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, diff --git a/lib/gitlab/github_import/wiki_formatter.rb b/lib/gitlab/github_import/wiki_formatter.rb index db2c49a497a..6c592ff469c 100644 --- a/lib/gitlab/github_import/wiki_formatter.rb +++ b/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 diff --git a/lib/gitlab/import_url_sanitizer.rb b/lib/gitlab/import_url.rb similarity index 51% rename from lib/gitlab/import_url_sanitizer.rb rename to lib/gitlab/import_url.rb index dfbc4f8303c..7358edac2ee 100644 --- a/lib/gitlab/import_url_sanitizer.rb +++ b/lib/gitlab/import_url.rb @@ -1,7 +1,8 @@ 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 diff --git a/lib/gitlab/import_url_exposer.rb b/lib/gitlab/import_url_exposer.rb deleted file mode 100644 index bf03f5a6daf..00000000000 --- a/lib/gitlab/import_url_exposer.rb +++ /dev/null @@ -1,13 +0,0 @@ -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 diff --git a/spec/lib/gitlab/github_import/project_creator_spec.rb b/spec/lib/gitlab/github_import/project_creator_spec.rb index 290c855642a..2092c1b9584 100644 --- a/spec/lib/gitlab/github_import/project_creator_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/github_import/wiki_formatter_spec.rb b/spec/lib/gitlab/github_import/wiki_formatter_spec.rb index 46d5c2f3296..91cd370987d 100644 --- a/spec/lib/gitlab/github_import/wiki_formatter_spec.rb +++ b/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) } diff --git a/spec/lib/gitlab/import_url_exposer_spec.rb b/spec/lib/gitlab/import_url_exposer_spec.rb deleted file mode 100644 index 878947caea1..00000000000 --- a/spec/lib/gitlab/import_url_exposer_spec.rb +++ /dev/null @@ -1,13 +0,0 @@ -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 diff --git a/spec/lib/gitlab/import_url_spec.rb b/spec/lib/gitlab/import_url_spec.rb new file mode 100644 index 00000000000..f758cb8693c --- /dev/null +++ b/spec/lib/gitlab/import_url_spec.rb @@ -0,0 +1,21 @@ +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