From 735563329d1f86ee4d72b37cd22eed1168935e8e Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 7 Mar 2016 12:50:35 +0100
Subject: [PATCH] refactored a bunch of stuff based on MR feedback
---
app/models/project_import_data.rb | 2 +-
...8_remove_wrong_import_url_from_projects.rb | 38 +++++++++++--------
lib/gitlab/github_import/importer.rb | 4 +-
lib/gitlab/github_import/project_creator.rb | 3 +-
lib/gitlab/import_url_exposer.rb | 14 +++----
spec/lib/gitlab/import_url_exposer_spec.rb | 13 +++++++
6 files changed, 44 insertions(+), 30 deletions(-)
create mode 100644 spec/lib/gitlab/import_url_exposer_spec.rb
diff --git a/app/models/project_import_data.rb b/app/models/project_import_data.rb
index 23f4e97b8aa..f3b9daa0d1a 100644
--- a/app/models/project_import_data.rb
+++ b/app/models/project_import_data.rb
@@ -12,7 +12,7 @@ require 'file_size_validator'
class ProjectImportData < ActiveRecord::Base
belongs_to :project
- attr_encrypted :credentials, key: Gitlab::Application.secrets.db_key_base, marshal: true
+ attr_encrypted :credentials, key: Gitlab::Application.secrets.db_key_base, marshal: true, encode: true
serialize :data, JSON
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 dfa9f2d4dee..881af783c61 100644
--- a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb
+++ b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb
@@ -2,43 +2,49 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration
class ImportUrlSanitizer
def initialize(url)
- @url = url
+ @url = URI.parse(url)
end
def sanitized_url
- @sanitized_url ||= @url[regex_extractor, 1] + @url[regex_extractor, 3]
+ @sanitized_url ||= safe_url
end
def credentials
- @credentials ||= @url[regex_extractor, 2]
+ @credentials ||= { user: @url.user, password: @url.password }
end
private
- # Regex matches 1 <first part of URL>, 2 <token or to be encrypted stuff>,
- # 3 <last part of URL>
- def regex_extractor
- /(.*\/\/)(.*)(\@.*)/
+ 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
+ attr_encrypted :credentials, key: Gitlab::Application.secrets.db_key_base, marshal: true, encode: true
end
def up
- projects_with_wrong_import_url.each do |project_id|
- project = Project.find(project_id["id"])
- sanitizer = ImportUrlSanitizer.new(project.import_url)
+ projects_with_wrong_import_url.each do |project|
+ sanitizer = ImportUrlSanitizer.new(project["import_url"])
ActiveRecord::Base.transaction do
- project.update_columns(import_url: sanitizer.sanitized_url)
- if project.import_data
- project.import_data.credentials = sanitizer.credentials
- project.save!
- end
+ execute("UPDATE projects SET import_url = '#{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 = '#{fake_import_data.encrypted_credentials}' WHERE project_id = #{project['id']}")
end
end
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 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 LIKE '%//%:%@%' or p.import_url like '#{"_"*40}@github.com%'")
end
end
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 515fd4720d5..d478d3b5398 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -7,8 +7,8 @@ module Gitlab
def initialize(project)
@project = project
- github_session = project.import_data.credentials if import_data
- @client = Client.new(github_session["github_access_token"])
+ credentials = project.import_data.credentials if import_data
+ @client = Client.new(credentials["github_access_token"])
@formatter = Gitlab::ImportFormatter.new
end
diff --git a/lib/gitlab/github_import/project_creator.rb b/lib/gitlab/github_import/project_creator.rb
index b5ed32e5b1e..52aba93a51d 100644
--- a/lib/gitlab/github_import/project_creator.rb
+++ b/lib/gitlab/github_import/project_creator.rb
@@ -32,8 +32,7 @@ module Gitlab
def create_import_data(project)
project.create_import_data(
- credentials: { github_access_token: session_data.delete(:github_access_token) },
- data: { github_session: session_data })
+ credentials: { github_access_token: session_data.delete(:github_access_token) })
end
end
end
diff --git a/lib/gitlab/import_url_exposer.rb b/lib/gitlab/import_url_exposer.rb
index f1919dffa8a..bf03f5a6daf 100644
--- a/lib/gitlab/import_url_exposer.rb
+++ b/lib/gitlab/import_url_exposer.rb
@@ -2,16 +2,12 @@ module Gitlab
# Exposes an import URL that includes the credentials unencrypted.
# Extracted to its own class to prevent unintended use.
module ImportUrlExposer
- extend self
- def expose(import_url:, credentials: )
- import_url.sub("//", "//#{parsed_credentials(credentials)}@")
- end
-
- private
-
- def parsed_credentials(credentials)
- credentials.values.join(":")
+ 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/import_url_exposer_spec.rb b/spec/lib/gitlab/import_url_exposer_spec.rb
new file mode 100644
index 00000000000..878947caea1
--- /dev/null
+++ b/spec/lib/gitlab/import_url_exposer_spec.rb
@@ -0,0 +1,13 @@
+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
--
GitLab