Merge branch '29685-wrong-number-of-arguments-calling-http_url_to_repo-on-cloning-project-wikis' into 'master'

Fix ProjectWiki#http_url_to_repo signature

Closes #29685

See merge request !10079

7 files changed, 87 insertions, 23 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 4a3faff7d5b..da4704554b3 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -881,13 +881,9 @@ class Project < ActiveRecord::Base
   end
 
   def http_url_to_repo(user = nil)
-    url = web_url
+    credentials = Gitlab::UrlSanitizer.http_credentials_for_user(user)
 
-    if user
-      url.sub!(%r{\Ahttps?://}) { |protocol| "#{protocol}#{user.username}@" }
-    end
-
-    "#{url}.git"
+    Gitlab::UrlSanitizer.new("#{web_url}.git", credentials: credentials).full_url
   end
 
   # Check if current branch name is marked as protected in the system
diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb
index 539b31780b3..70eef359cdd 100644
--- a/app/models/project_wiki.rb
+++ b/app/models/project_wiki.rb
@@ -42,8 +42,11 @@ class ProjectWiki
     url_to_repo
   end
 
-  def http_url_to_repo
-    [Gitlab.config.gitlab.url, "/", path_with_namespace, ".git"].join('')
+  def http_url_to_repo(user = nil)
+    url = "#{Gitlab.config.gitlab.url}/#{path_with_namespace}.git"
+    credentials = Gitlab::UrlSanitizer.http_credentials_for_user(user)
+
+    Gitlab::UrlSanitizer.new(url, credentials: credentials).full_url
   end
 
   def wiki_base_path
diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb
index c81dc7e30d0..9ce13feb79a 100644
--- a/lib/gitlab/url_sanitizer.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -18,6 +18,12 @@ module Gitlab
       false
     end
 
+    def self.http_credentials_for_user(user)
+      return {} unless user.respond_to?(:username)
+
+      { user: user.username }
+    end
+
     def initialize(url, credentials: nil)
       @url = Addressable::URI.parse(url.strip)
       @credentials = credentials
diff --git a/spec/features/projects/wiki/user_git_access_wiki_page_spec.rb b/spec/features/projects/wiki/user_git_access_wiki_page_spec.rb
new file mode 100644
index 00000000000..6825b95c8aa
--- /dev/null
+++ b/spec/features/projects/wiki/user_git_access_wiki_page_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe 'Projects > Wiki > User views Git access wiki page', :feature do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public) }
+  let(:wiki_page) do
+    WikiPages::CreateService.new(
+      project,
+      user,
+      title: 'home',
+      content: '[some link](other-page)'
+    ).execute
+  end
+
+  before do
+    login_as(user)
+  end
+
+  scenario 'Visit Wiki Page Current Commit' do
+    visit namespace_project_wiki_path(project.namespace, project, wiki_page)
+
+    click_link 'Clone repository'
+    expect(page).to have_text("Clone repository #{project.wiki.path_with_namespace}")
+    expect(page).to have_text(project.wiki.http_url_to_repo(user))
+  end
+end
diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb
index 3fd361de458..fc144a2556a 100644
--- a/spec/lib/gitlab/url_sanitizer_spec.rb
+++ b/spec/lib/gitlab/url_sanitizer_spec.rb
@@ -5,6 +5,7 @@ describe Gitlab::UrlSanitizer, lib: true do
   let(:url_sanitizer) do
     described_class.new("https://github.com/me/project.git", credentials: credentials)
   end
+  let(:user) { double(:user, username: 'john.doe') }
 
   describe '.sanitize' do
     def sanitize_url(url)
@@ -53,12 +54,33 @@ describe Gitlab::UrlSanitizer, lib: true do
     end
   end
 
+  describe '.valid?' do
+    it 'validates url strings' do
+      expect(described_class.valid?(nil)).to be(false)
+      expect(described_class.valid?('valid@project:url.git')).to be(true)
+      expect(described_class.valid?('123://invalid:url')).to be(false)
+    end
+  end
+
+  describe '.http_credentials_for_user' do
+    it { expect(described_class.http_credentials_for_user(user)).to eq({ user: 'john.doe' }) }
+    it { expect(described_class.http_credentials_for_user('foo')).to eq({}) }
+  end
+
   describe '#sanitized_url' do
     it { expect(url_sanitizer.sanitized_url).to eq("https://github.com/me/project.git") }
   end
 
   describe '#credentials' do
     it { expect(url_sanitizer.credentials).to eq(credentials) }
+
+    context 'when user is given to #initialize' do
+      let(:url_sanitizer) do
+        described_class.new("https://github.com/me/project.git", credentials: described_class.http_credentials_for_user(user))
+      end
+
+      it { expect(url_sanitizer.credentials).to eq({ user: 'john.doe' }) }
+    end
   end
 
   describe '#full_url' do
@@ -69,13 +91,13 @@ describe Gitlab::UrlSanitizer, lib: true do
 
       expect(sanitizer.full_url).to eq('user@server:project.git')
     end
-  end
 
-  describe '.valid?' do
-    it 'validates url strings' do
-      expect(described_class.valid?(nil)).to be(false)
-      expect(described_class.valid?('valid@project:url.git')).to be(true)
-      expect(described_class.valid?('123://invalid:url')).to be(false)
+    context 'when user is given to #initialize' do
+      let(:url_sanitizer) do
+        described_class.new("https://github.com/me/project.git", credentials: described_class.http_credentials_for_user(user))
+      end
+
+      it { expect(url_sanitizer.full_url).to eq("https://john.doe@github.com/me/project.git") }
     end
   end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index f68631ebe06..5e5f690acd4 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1914,10 +1914,8 @@ describe Project, models: true do
 
     context 'when no user is given' do
       it 'returns the url to the repo without a username' do
-        url = project.http_url_to_repo
-
-        expect(url).to eq(project.http_url_to_repo)
-        expect(url).not_to include('@')
+        expect(project.http_url_to_repo).to eq("#{project.web_url}.git")
+        expect(project.http_url_to_repo).not_to include('@')
       end
     end
 
@@ -1925,7 +1923,7 @@ describe Project, models: true do
       it 'returns the url to the repo with the username' do
         user = build_stubbed(:user)
 
-        expect(project.http_url_to_repo(user)).to match(%r{https?:\/\/#{user.username}@})
+        expect(project.http_url_to_repo(user)).to start_with("http://#{user.username}@")
       end
     end
   end
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index 58b57bd4fef..b5b9cd024b0 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -35,10 +35,23 @@ describe ProjectWiki, models: true do
   end
 
   describe "#http_url_to_repo" do
-    it "provides the full http url to the repo" do
-      gitlab_url = Gitlab.config.gitlab.url
-      repo_http_url = "#{gitlab_url}/#{subject.path_with_namespace}.git"
-      expect(subject.http_url_to_repo).to eq(repo_http_url)
+    let(:project) { create :empty_project }
+
+    context 'when no user is given' do
+      it 'returns the url to the repo without a username' do
+        expected_url = "#{Gitlab.config.gitlab.url}/#{subject.path_with_namespace}.git"
+
+        expect(project_wiki.http_url_to_repo).to eq(expected_url)
+        expect(project_wiki.http_url_to_repo).not_to include('@')
+      end
+    end
+
+    context 'when user is given' do
+      it 'returns the url to the repo with the username' do
+        user = build_stubbed(:user)
+
+        expect(project_wiki.http_url_to_repo(user)).to start_with("http://#{user.username}@")
+      end
     end
   end