diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-06-19 13:56:43 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-06-19 13:56:43 +0000 |
commit | b18c12ec6a9564d55bd2851a82ff2513b8485fe0 (patch) | |
tree | 6c6ddb70860550ca7721aa2d1a90fbdd0e9b1ba1 | |
parent | 9af0e8f911040fd3722c55f8c4b6d8302861c9eb (diff) | |
parent | 32b3d09ae5ed778b8d884cd6722f748b39bf87f3 (diff) | |
download | gitlab-ce-b18c12ec6a9564d55bd2851a82ff2513b8485fe0.tar.gz |
Merge branch 'mk-add-project-moved-errors-for-git' into 'master'
Add "project was moved" error messages for git
See merge request !11259
-rw-r--r-- | app/controllers/projects/git_http_client_controller.rb | 46 | ||||
-rw-r--r-- | app/controllers/projects/git_http_controller.rb | 2 | ||||
-rw-r--r-- | lib/api/helpers/internal_helpers.rb | 7 | ||||
-rw-r--r-- | lib/api/internal.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git_access.rb | 21 | ||||
-rw-r--r-- | lib/gitlab/repo_path.rb | 21 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 43 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_wiki_spec.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/repo_path_spec.rb | 75 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 62 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 49 | ||||
-rw-r--r-- | spec/workers/post_receive_spec.rb | 2 |
12 files changed, 265 insertions, 68 deletions
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index 928f17e6a8e..7d0e2b3e2ef 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/app/controllers/projects/git_http_client_controller.rb @@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController include ActionController::HttpAuthentication::Basic include KerberosSpnegoHelper - attr_reader :authentication_result + attr_reader :authentication_result, :redirected_path delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true @@ -14,7 +14,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController skip_before_action :verify_authenticity_token skip_before_action :repository before_action :authenticate_user - before_action :ensure_project_found! private @@ -68,38 +67,14 @@ class Projects::GitHttpClientController < Projects::ApplicationController headers['Www-Authenticate'] = challenges.join("\n") if challenges.any? end - def ensure_project_found! - render_not_found if project.blank? - end - def project - return @project if defined?(@project) - - project_id, _ = project_id_with_suffix - @project = - if project_id.blank? - nil - else - Project.find_by_full_path("#{params[:namespace_id]}/#{project_id}") - end - end + parse_repo_path unless defined?(@project) - # This method returns two values so that we can parse - # params[:project_id] (untrusted input!) in exactly one place. - def project_id_with_suffix - id = params[:project_id] || '' - - %w[.wiki.git .git].each do |suffix| - if id.end_with?(suffix) - # Be careful to only remove the suffix from the end of 'id'. - # Accidentally removing it from the middle is how security - # vulnerabilities happen! - return [id.slice(0, id.length - suffix.length), suffix] - end - end + @project + end - # Something is wrong with params[:project_id]; do not pass it on. - [nil, nil] + def parse_repo_path + @project, @wiki, @redirected_path = Gitlab::RepoPath.parse("#{params[:namespace_id]}/#{params[:project_id]}") end def render_missing_personal_token @@ -114,14 +89,9 @@ class Projects::GitHttpClientController < Projects::ApplicationController end def wiki? - return @wiki if defined?(@wiki) - - _, suffix = project_id_with_suffix - @wiki = suffix == '.wiki.git' - end + parse_repo_path unless defined?(@wiki) - def render_not_found - render plain: 'Not Found', status: :not_found + @wiki end def handle_basic_authentication(login, password) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index b6b62da7b60..71ae60cb8cd 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -56,7 +56,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController end def access - @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities) + @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path) end def access_actor diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index d3732d67622..5e9cf5e68b1 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -10,6 +10,10 @@ module API set_project unless defined?(@project) @project end + + def redirected_path + @redirected_path + end def ssh_authentication_abilities [ @@ -38,8 +42,9 @@ module API def set_project if params[:gl_repository] @project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository]) + @redirected_path = nil else - @project, @wiki = Gitlab::RepoPath.parse(params[:project]) + @project, @wiki, @redirected_path = Gitlab::RepoPath.parse(params[:project]) end end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ecd6d672cf7..9ec418edea4 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -34,7 +34,7 @@ module API access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess access_checker = access_checker_klass - .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities) + .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path) begin access_checker.check(params[:action], params[:changes]) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 0a19d24eb20..0b62911958d 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -22,12 +22,13 @@ module Gitlab PUSH_COMMANDS = %w{ git-receive-pack }.freeze ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS - attr_reader :actor, :project, :protocol, :authentication_abilities + attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path - def initialize(actor, project, protocol, authentication_abilities:) + def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil) @actor = actor @project = project @protocol = protocol + @redirected_path = redirected_path @authentication_abilities = authentication_abilities end @@ -35,6 +36,7 @@ module Gitlab check_protocol! check_active_user! check_project_accessibility! + check_project_moved! check_command_disabled!(cmd) check_command_existence!(cmd) check_repository_existence! @@ -87,6 +89,21 @@ module Gitlab end end + def check_project_moved! + if redirected_path + url = protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo + message = <<-MESSAGE.strip_heredoc + Project '#{redirected_path}' was moved to '#{project.full_path}'. + + Please update your Git remote and try again: + + git remote set-url origin #{url} + MESSAGE + + raise NotFoundError, message + end + end + def check_command_disabled!(cmd) if upload_pack?(cmd) check_upload_pack_disabled! diff --git a/lib/gitlab/repo_path.rb b/lib/gitlab/repo_path.rb index 878e03f61d7..3591fa9145e 100644 --- a/lib/gitlab/repo_path.rb +++ b/lib/gitlab/repo_path.rb @@ -3,16 +3,18 @@ module Gitlab NotFoundError = Class.new(StandardError) def self.parse(repo_path) + wiki = false project_path = strip_storage_path(repo_path.sub(/\.git\z/, ''), fail_on_not_found: false) - project = Project.find_by_full_path(project_path) - if project_path.end_with?('.wiki') && !project - project = Project.find_by_full_path(project_path.chomp('.wiki')) + project, was_redirected = find_project(project_path) + + if project_path.end_with?('.wiki') && project.nil? + project, was_redirected = find_project(project_path.chomp('.wiki')) wiki = true - else - wiki = false end - [project, wiki] + redirected_path = project_path if was_redirected + + [project, wiki, redirected_path] end def self.strip_storage_path(repo_path, fail_on_not_found: true) @@ -30,5 +32,12 @@ module Gitlab result.sub(/\A\/*/, '') end + + def self.find_project(project_path) + project = Project.find_by_full_path(project_path, follow_redirects: true) + was_redirected = project && project.full_path.casecmp(project_path) != 0 + + [project, was_redirected] + end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 3dcc20c48e8..9a86cfa66e4 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -3,11 +3,12 @@ require 'spec_helper' describe Gitlab::GitAccess, lib: true do let(:pull_access_check) { access.check('git-upload-pack', '_any') } let(:push_access_check) { access.check('git-receive-pack', '_any') } - let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) } + let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) } let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:actor) { user } let(:protocol) { 'ssh' } + let(:redirected_path) { nil } let(:authentication_abilities) do [ :read_project, @@ -162,6 +163,46 @@ describe Gitlab::GitAccess, lib: true do end end + describe '#check_project_moved!' do + before do + project.team << [user, :master] + end + + context 'when a redirect was not followed to find the project' do + context 'pull code' do + it { expect { pull_access_check }.not_to raise_error } + end + + context 'push code' do + it { expect { push_access_check }.not_to raise_error } + end + end + + context 'when a redirect was followed to find the project' do + let(:redirected_path) { 'some/other-path' } + + context 'pull code' do + it { expect { pull_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) } + it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) } + + context 'http protocol' do + let(:protocol) { 'http' } + it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) } + end + end + + context 'push code' do + it { expect { push_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) } + it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) } + + context 'http protocol' do + let(:protocol) { 'http' } + it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) } + end + end + end + end + describe '#check_command_disabled!' do before do project.team << [user, :master] diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index a1eb95750ba..797ec8cb23e 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -1,9 +1,10 @@ require 'spec_helper' describe Gitlab::GitAccessWiki, lib: true do - let(:access) { Gitlab::GitAccessWiki.new(user, project, 'web', authentication_abilities: authentication_abilities) } + let(:access) { Gitlab::GitAccessWiki.new(user, project, 'web', authentication_abilities: authentication_abilities, redirected_path: redirected_path) } let(:project) { create(:project, :repository) } let(:user) { create(:user) } + let(:redirected_path) { nil } let(:authentication_abilities) do [ :read_project, diff --git a/spec/lib/gitlab/repo_path_spec.rb b/spec/lib/gitlab/repo_path_spec.rb index f9025397107..efea4f429bf 100644 --- a/spec/lib/gitlab/repo_path_spec.rb +++ b/spec/lib/gitlab/repo_path_spec.rb @@ -4,24 +4,44 @@ describe ::Gitlab::RepoPath do describe '.parse' do set(:project) { create(:project) } - it 'parses a full repository path' do - expect(described_class.parse(project.repository.path)).to eq([project, false]) - end + context 'a repository storage path' do + it 'parses a full repository path' do + expect(described_class.parse(project.repository.path)).to eq([project, false, nil]) + end - it 'parses a full wiki path' do - expect(described_class.parse(project.wiki.repository.path)).to eq([project, true]) + it 'parses a full wiki path' do + expect(described_class.parse(project.wiki.repository.path)).to eq([project, true, nil]) + end end - it 'parses a relative repository path' do - expect(described_class.parse(project.full_path + '.git')).to eq([project, false]) - end + context 'a relative path' do + it 'parses a relative repository path' do + expect(described_class.parse(project.full_path + '.git')).to eq([project, false, nil]) + end - it 'parses a relative wiki path' do - expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true]) - end + it 'parses a relative wiki path' do + expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true, nil]) + end + + it 'parses a relative path starting with /' do + expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false, nil]) + end + + context 'of a redirected project' do + let(:redirect) { project.route.create_redirect('foo/bar') } + + it 'parses a relative repository path' do + expect(described_class.parse(redirect.path + '.git')).to eq([project, false, 'foo/bar']) + end + + it 'parses a relative wiki path' do + expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, true, 'foo/bar.wiki']) + end - it 'parses a relative path starting with /' do - expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false]) + it 'parses a relative path starting with /' do + expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, false, 'foo/bar']) + end + end end end @@ -43,4 +63,33 @@ describe ::Gitlab::RepoPath do ) end end + + describe '.find_project' do + let(:project) { create(:empty_project) } + let(:redirect) { project.route.create_redirect('foo/bar/baz') } + + context 'when finding a project by its canonical path' do + context 'when the cases match' do + it 'returns the project and false' do + expect(described_class.find_project(project.full_path)).to eq([project, false]) + end + end + + context 'when the cases do not match' do + # This is slightly different than web behavior because on the web it is + # easy and safe to redirect someone to the correctly-cased URL. For git + # requests, we should accept wrongly-cased URLs because it is a pain to + # block people's git operations and force them to update remote URLs. + it 'returns the project and false' do + expect(described_class.find_project(project.full_path.upcase)).to eq([project, false]) + end + end + end + + context 'when finding a project via a redirect' do + it 'returns the project and true' do + expect(described_class.find_project(redirect.path)).to eq([project, true]) + end + end + end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 86e15d896df..6deaea956e0 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -321,8 +321,6 @@ describe API::Internal do end context "archived project" do - let(:personal_project) { create(:empty_project, namespace: user.namespace) } - before do project.team << [user, :developer] project.archive! @@ -445,6 +443,42 @@ describe API::Internal do expect(json_response['status']).to be_truthy end end + + context 'the project path was changed' do + let!(:old_path_to_repo) { project.repository.path_to_repo } + let!(:old_full_path) { project.full_path } + let(:project_moved_message) do + <<-MSG.strip_heredoc + Project '#{old_full_path}' was moved to '#{project.full_path}'. + + Please update your Git remote and try again: + + git remote set-url origin #{project.ssh_url_to_repo} + MSG + end + + before do + project.team << [user, :developer] + project.path = 'new_path' + project.save! + end + + it 'rejects the push' do + push_with_path(key, old_path_to_repo) + + expect(response).to have_http_status(200) + expect(json_response['status']).to be_falsey + expect(json_response['message']).to eq(project_moved_message) + end + + it 'rejects the SSH pull' do + pull_with_path(key, old_path_to_repo) + + expect(response).to have_http_status(200) + expect(json_response['status']).to be_falsey + expect(json_response['message']).to eq(project_moved_message) + end + end end describe 'GET /internal/merge_request_urls' do @@ -587,6 +621,17 @@ describe API::Internal do ) end + def pull_with_path(key, path_to_repo, protocol = 'ssh') + post( + api("/internal/allowed"), + key_id: key.id, + project: path_to_repo, + action: 'git-upload-pack', + secret_token: secret_token, + protocol: protocol + ) + end + def push(key, project, protocol = 'ssh', env: nil) post( api("/internal/allowed"), @@ -600,6 +645,19 @@ describe API::Internal do ) end + def push_with_path(key, path_to_repo, protocol = 'ssh', env: nil) + post( + api("/internal/allowed"), + changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master', + key_id: key.id, + project: path_to_repo, + action: 'git-receive-pack', + secret_token: secret_token, + protocol: protocol, + env: env + ) + end + def archive(key, project) post( api("/internal/allowed"), diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index dce78faefc9..b08148eca3c 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -316,6 +316,26 @@ describe 'Git HTTP requests', lib: true do it_behaves_like 'pushes require Basic HTTP Authentication' end end + + context 'and the user requests a redirected path' do + let!(:redirect) { project.route.create_redirect('foo/bar') } + let(:path) { "#{redirect.path}.git" } + let(:project_moved_message) do + <<-MSG.strip_heredoc + Project '#{redirect.path}' was moved to '#{project.full_path}'. + + Please update your Git remote and try again: + + git remote set-url origin #{project.http_url_to_repo} + MSG + end + + it 'downloads get status 404 with "project was moved" message' do + clone_get(path, {}) + expect(response).to have_http_status(:not_found) + expect(response.body).to match(project_moved_message) + end + end end context "when the project is private" do @@ -505,6 +525,33 @@ describe 'Git HTTP requests', lib: true do Rack::Attack::Allow2Ban.reset(ip, options) end end + + context 'and the user requests a redirected path' do + let!(:redirect) { project.route.create_redirect('foo/bar') } + let(:path) { "#{redirect.path}.git" } + let(:project_moved_message) do + <<-MSG.strip_heredoc + Project '#{redirect.path}' was moved to '#{project.full_path}'. + + Please update your Git remote and try again: + + git remote set-url origin #{project.http_url_to_repo} + MSG + end + + it 'downloads get status 404 with "project was moved" message' do + clone_get(path, env) + expect(response).to have_http_status(:not_found) + expect(response.body).to match(project_moved_message) + end + + it 'uploads get status 404 with "project was moved" message' do + upload(path, env) do |response| + expect(response).to have_http_status(:not_found) + expect(response.body).to match(project_moved_message) + end + end + end end context "when the user doesn't have access to the project" do @@ -680,7 +727,7 @@ describe 'Git HTTP requests', lib: true do end context "POST git-receive-pack" do - it "failes to find a route" do + it "fails to find a route" do expect { push_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError) end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 3c93da63f2e..6c4df7350ea 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -32,7 +32,7 @@ describe PostReceive do context "with an absolute path as the project identifier" do it "searches the project by full path" do - expect(Project).to receive(:find_by_full_path).with(project.full_path).and_call_original + expect(Project).to receive(:find_by_full_path).with(project.full_path, follow_redirects: true).and_call_original described_class.new.perform(pwd(project), key_id, base64_changes) end |