Improve handling of credentials in URL for RemoteMirror.

* Now #url= and #url always handle the full URL, encryption is done transparently
* #safe_url was added for future purposes
* Little refactor for UpdateRemoteMirrorService

app/models/remote_mirror.rb

@@ -95,11 +95,26 @@ class RemoteMirror < ActiveRecord::Base
 
   def url=(value)
     mirror_url = Gitlab::ImportUrl.new(value)
-    self.credentials = mirror_url.credentials if mirror_url.credentials.values.any?
+    self.credentials = mirror_url.credentials
 
     super(mirror_url.sanitized_url)
   end
 
+  def url
+    if super
+      Gitlab::ImportUrl.new(super, credentials: credentials).full_url
+    end
+  end
+
+  def safe_url
+    return if url.nil?
+
+    result = URI.parse(url)
+    result.password = '*****' if result.password
+    result.user = '*****' if result.user && result.user != "git" #tokens or other data may be saved as user
+    result.to_s
+  end
+
   def full_url
     mirror_url = Gitlab::ImportUrl.new(url, credentials: credentials)
     mirror_url.full_url

app/services/projects/update_remote_mirror_service.rb

@@ -96,10 +96,6 @@ module Projects
 
     def remote_tags
       @remote_tags ||= repository.remote_tags(mirror.ref_name).each_with_object({}) do |(name, target), tags|
-        # We're only interested in tag references
-        # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name
-        next if name =~ /\^\{\}\Z/
-
         tags[name] = target
       end
     end

lib/gitlab/backend/shell.rb

@@ -55,6 +55,10 @@ module Gitlab
         break if target.nil?
 
         name = path.split('/', 3).last
+        # We're only interested in tag references
+        # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name
+        next if name =~ /\^\{\}\Z/
+
         [name, target]
       end
 

spec/models/remote_mirror_spec.rb

+require 'rails_helper'
+
+describe RemoteMirror, focus: true do
+  describe 'encrypting credentials' do
+    context 'when setting URL for a first time' do
+      it 'should store the URL without credentials' do
+        mirror = create_mirror_with_url('http://foo:bar@test.com')
+
+        expect(mirror.read_attribute(:url)).to eq('http://test.com')
+      end
+
+      it 'should store the credentials on a separate field' do
+        mirror = create_mirror_with_url('http://foo:bar@test.com')
+
+        expect(mirror.credentials).to eq({ user: 'foo', password: 'bar' })
+      end
+    end
+
+    context 'when updating the URL' do
+      it 'should allow a new URL without credentials' do
+        mirror = create_mirror_with_url('http://foo:bar@test.com')
+
+        mirror.update_attribute(:url, 'http://test.com')
+
+        expect(mirror.url).to eq('http://test.com')
+        expect(mirror.credentials).to eq({ user: nil, password: nil })
+      end
+
+      it 'should allow a new URL with credentials' do
+        mirror = create_mirror_with_url('http://test.com')
+
+        mirror.update_attribute(:url, 'http://foo:bar@test.com')
+
+        expect(mirror.url).to eq('http://foo:bar@test.com')
+        expect(mirror.credentials).to eq({ user: 'foo', password: 'bar' })
+      end
+    end
+  end
+
+  describe '#safe_url' do
+    context 'when URL contains credentials' do
+      it 'should mask the credentials' do
+        mirror = create_mirror_with_url('http://foo:bar@test.com')
+
+        expect(mirror.safe_url).to eq('http://*****:*****@test.com')
+      end
+    end
+
+    context 'when URL does not contain credentials' do
+      it 'should show the full URL' do
+        mirror = create_mirror_with_url('http://test.com')
+
+        expect(mirror.safe_url).to eq('http://test.com')
+      end
+    end
+  end
+
+  def create_mirror_with_url(url)
+    project = FactoryGirl.create(:project)
+    project.remote_mirrors.create!(url: url)
+  end
+end