Commit 562fb460 authored by Mayra Cabrera's avatar Mayra Cabrera Committed by Douwe Maan

Allow git pull/push on project redirects

parent 87300546
......@@ -40,6 +40,7 @@ class Namespace < ActiveRecord::Base
namespace_path: true
validate :nesting_level_allowed
validate :allowed_path_by_redirects
delegate :name, to: :owner, allow_nil: true, prefix: true
......@@ -257,4 +258,14 @@ class Namespace < ActiveRecord::Base
Namespace.where(id: descendants.select(:id))
.update_all(share_with_group_lock: true)
end
def allowed_path_by_redirects
return if path.nil?
errors.add(:path, "#{path} has been taken before. Please use another one") if namespace_previously_created_with_same_path?
end
def namespace_previously_created_with_same_path?
RedirectRoute.permanent.exists?(path: path)
end
end
......@@ -17,4 +17,32 @@ class RedirectRoute < ActiveRecord::Base
where(wheres, path, "#{sanitize_sql_like(path)}/%")
end
scope :permanent, -> do
if column_permanent_exists?
where(permanent: true)
else
none
end
end
scope :temporary, -> do
if column_permanent_exists?
where(permanent: [false, nil])
else
all
end
end
default_value_for :permanent, false
def permanent=(value)
if self.class.column_permanent_exists?
super
end
end
def self.column_permanent_exists?
ActiveRecord::Base.connection.column_exists?(:redirect_routes, :permanent)
end
end
......@@ -8,6 +8,8 @@ class Route < ActiveRecord::Base
presence: true,
uniqueness: { case_sensitive: false }
validate :ensure_permanent_paths
after_create :delete_conflicting_redirects
after_update :delete_conflicting_redirects, if: :path_changed?
after_update :create_redirect_for_old_path
......@@ -40,7 +42,7 @@ class Route < ActiveRecord::Base
# We are not calling route.delete_conflicting_redirects here, in hopes
# of avoiding deadlocks. The parent (self, in this method) already
# called it, which deletes conflicts for all descendants.
route.create_redirect(old_path) if attributes[:path]
route.create_redirect(old_path, permanent: permanent_redirect?) if attributes[:path]
end
end
end
......@@ -50,16 +52,30 @@ class Route < ActiveRecord::Base
end
def conflicting_redirects
RedirectRoute.matching_path_and_descendants(path)
RedirectRoute.temporary.matching_path_and_descendants(path)
end
def create_redirect(path)
RedirectRoute.create(source: source, path: path)
def create_redirect(path, permanent: false)
RedirectRoute.create(source: source, path: path, permanent: permanent)
end
private
def create_redirect_for_old_path
create_redirect(path_was) if path_changed?
create_redirect(path_was, permanent: permanent_redirect?) if path_changed?
end
def permanent_redirect?
source_type != "Project"
end
def ensure_permanent_paths
return if path.nil?
errors.add(:path, "#{path} has been taken before. Please use another one") if conflicting_redirect_exists?
end
def conflicting_redirect_exists?
RedirectRoute.permanent.matching_path_and_descendants(path).exists?
end
end
---
title: Allow git pull/push on group/user/project redirects
merge_request: 15670
author:
type: added
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddPermanentToRedirectRoute < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
def up
add_column(:redirect_routes, :permanent, :boolean)
end
def down
remove_column(:redirect_routes, :permanent)
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddPermanentIndexToRedirectRoute < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index(:redirect_routes, :permanent)
end
def down
remove_concurrent_index(:redirect_routes, :permanent) if index_exists?(:redirect_routes, :permanent)
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171205190711) do
ActiveRecord::Schema.define(version: 20171206221519) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -1526,10 +1526,12 @@ ActiveRecord::Schema.define(version: 20171205190711) do
t.string "path", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.boolean "permanent"
end
add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree
add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"}
add_index "redirect_routes", ["permanent"], name: "index_redirect_routes_on_permanent", using: :btree
add_index "redirect_routes", ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree
create_table "releases", force: :cascade do |t|
......
......@@ -4,6 +4,7 @@ module API
before { authenticate_by_gitlab_shell_token! }
helpers ::API::Helpers::InternalHelpers
helpers ::Gitlab::Identifier
namespace 'internal' do
# Check if git command is allowed to project
......@@ -176,17 +177,25 @@ module API
post '/post_receive' do
status 200
PostReceive.perform_async(params[:gl_repository], params[:identifier],
params[:changes])
broadcast_message = BroadcastMessage.current&.last&.message
reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease
{
output = {
merge_request_urls: merge_request_urls,
broadcast_message: broadcast_message,
reference_counter_decreased: reference_counter_decreased
}
project = Gitlab::GlRepository.parse(params[:gl_repository]).first
user = identify(params[:identifier])
redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)
if redirect_message
output[:redirected_message] = redirect_message
end
output
end
end
end
......
module Gitlab
module Checks
class ProjectMoved
REDIRECT_NAMESPACE = "redirect_namespace".freeze
def initialize(project, user, redirected_path, protocol)
@project = project
@user = user
@redirected_path = redirected_path
@protocol = protocol
end
def self.fetch_redirect_message(user_id, project_id)
redirect_key = redirect_message_key(user_id, project_id)
Gitlab::Redis::SharedState.with do |redis|
message = redis.get(redirect_key)
redis.del(redirect_key)
message
end
end
def add_redirect_message
Gitlab::Redis::SharedState.with do |redis|
key = self.class.redirect_message_key(user.id, project.id)
redis.setex(key, 5.minutes, redirect_message)
end
end
def redirect_message(rejected: false)
<<~MESSAGE.strip_heredoc
Project '#{redirected_path}' was moved to '#{project.full_path}'.
Please update your Git remote:
#{remote_url_message(rejected)}
MESSAGE
end
def permanent_redirect?
RedirectRoute.permanent.exists?(path: redirected_path)
end
private
attr_reader :project, :redirected_path, :protocol, :user
def self.redirect_message_key(user_id, project_id)
"#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}"
end
def remote_url_message(rejected)
if rejected
"git remote set-url origin #{url} and try again."
else
"git remote set-url origin #{url}"
end
end
def url
protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo
end
end
end
end
......@@ -102,18 +102,15 @@ module Gitlab
end
def check_project_moved!
return unless redirected_path
return if redirected_path.nil?
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}'.
project_moved = Checks::ProjectMoved.new(project, user, redirected_path, protocol)
Please update your Git remote and try again:
git remote set-url origin #{url}
MESSAGE
raise ProjectMovedError, message
if project_moved.permanent_redirect?
project_moved.add_redirect_message
else
raise ProjectMovedError, project_moved.redirect_message(rejected: true)
end
end
def check_command_disabled!(cmd)
......
......@@ -2,9 +2,8 @@
# key-13 or user-36 or last commit
module Gitlab
module Identifier
def identify(identifier, project, newrev)
def identify(identifier, project = nil, newrev = nil)
if identifier.blank?
# Local push from gitlab
identify_using_commit(project, newrev)
elsif identifier =~ /\Auser-\d+\Z/
# git push over http
......@@ -17,6 +16,8 @@ module Gitlab
# Tries to identify a user based on a commit SHA.
def identify_using_commit(project, ref)
return if project.nil? && ref.nil?
commit = project.commit(ref)
return if !commit || !commit.author_email
......
require 'rails_helper'
describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
let(:user) { create(:user) }
let(:project) { create(:project) }
describe '.fetch_redirct_message' do
context 'with a redirect message queue' do
it 'should return the redirect message' do
project_moved = described_class.new(project, user, 'foo/bar', 'http')
project_moved.add_redirect_message
expect(described_class.fetch_redirect_message(user.id, project.id)).to eq(project_moved.redirect_message)
end
it 'should delete the redirect message from redis' do
project_moved = described_class.new(project, user, 'foo/bar', 'http')
project_moved.add_redirect_message
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).not_to be_nil
described_class.fetch_redirect_message(user.id, project.id)
expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).to be_nil
end
end
context 'with no redirect message queue' do
it 'should return nil' do
expect(described_class.fetch_redirect_message(1, 2)).to be_nil
end
end
end
describe '#add_redirect_message' do
it 'should queue a redirect message' do
project_moved = described_class.new(project, user, 'foo/bar', 'http')
expect(project_moved.add_redirect_message).to eq("OK")
end
end
describe '#redirect_message' do
context 'when the push is rejected' do
it 'should return a redirect message telling the user to try again' do
project_moved = described_class.new(project, user, 'foo/bar', 'http')
message = "Project 'foo/bar' was moved to '#{project.full_path}'." +
"\n\nPlease update your Git remote:" +
"\n\n git remote set-url origin #{project.http_url_to_repo} and try again.\n"
expect(project_moved.redirect_message(rejected: true)).to eq(message)
end
end
context 'when the push is not rejected' do
it 'should return a redirect message' do
project_moved = described_class.new(project, user, 'foo/bar', 'http')
message = "Project 'foo/bar' was moved to '#{project.full_path}'." +
"\n\nPlease update your Git remote:" +
"\n\n git remote set-url origin #{project.http_url_to_repo}\n"
expect(project_moved.redirect_message).to eq(message)
end
end
end
describe '#permanent_redirect?' do
context 'with a permanent RedirectRoute' do
it 'should return true' do
project.route.create_redirect('foo/bar', permanent: true)
project_moved = described_class.new(project, user, 'foo/bar', 'http')
expect(project_moved.permanent_redirect?).to be_truthy
end
end
context 'without a permanent RedirectRoute' do
it 'should return false' do
project.route.create_redirect('foo/bar')
project_moved = described_class.new(project, user, 'foo/bar', 'http')
expect(project_moved.permanent_redirect?).to be_falsy
end
end
end
end
......@@ -193,7 +193,15 @@ describe Gitlab::GitAccess do
let(:actor) { build(:rsa_deploy_key_2048, user: user) }
end
describe '#check_project_moved!' do
shared_examples 'check_project_moved' do
it 'enqueues a redirected message' do
push_access_check
expect(Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)).not_to be_nil
end
end
describe '#check_project_moved!', :clean_gitlab_redis_shared_state do
before do
project.add_master(user)
end
......@@ -207,7 +215,40 @@ describe Gitlab::GitAccess do
end
end
context 'when a redirect was followed to find the project' do
context 'when a permanent redirect and ssh protocol' do
let(:redirected_path) { 'some/other-path' }
before do
allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true)
end
it 'allows push and pull access' do
aggregate_failures do
expect { push_access_check }.not_to raise_error
end
end
it_behaves_like 'check_project_moved'
end
context 'with a permanent redirect and http protocol' do
let(:redirected_path) { 'some/other-path' }
let(:protocol) { 'http' }
before do
allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true)
end
it 'allows_push and pull access' do
aggregate_failures do
expect { push_access_check }.not_to raise_error
end
end
it_behaves_like 'check_project_moved'
end
context 'with a temporal redirect and ssh protocol' do
let(:redirected_path) { 'some/other-path' }
it 'blocks push and pull access' do
......@@ -219,16 +260,15 @@ describe Gitlab::GitAccess do
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/)
end
end
end
context 'http protocol' do
let(:protocol) { 'http' }
context 'with a temporal redirect and http protocol' do
let(:redirected_path) { 'some/other-path' }
let(:protocol) { 'http' }
it 'includes the path to the project using HTTP' do
aggregate_failures do
expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
end
end
it 'does not allow to push and pull access' do
expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
end
end
end
......
......@@ -70,6 +70,10 @@ describe Gitlab::Identifier do
expect(identifier.identify_using_commit(project, '123')).to eq(user)
end
end
it 'returns nil if the project & ref are not present' do
expect(identifier.identify_using_commit(nil, nil)).to be_nil
end
end
describe '#identify_using_user' do
......
......@@ -559,4 +559,34 @@ describe Namespace do
end
end
end
describe "#allowed_path_by_redirects" do
let(:namespace1) { create(:namespace, path: 'foo') }
context "when the path has been taken before" do
before do
namespace1.path = 'bar'
namespace1.save!
end
it 'should be invalid' do
namespace2 = build(:group, path: 'foo')
expect(namespace2).to be_invalid
end
it 'should return an error on path' do
namespace2 = build(:group, path: 'foo')
namespace2.valid?
expect(namespace2.errors.messages[:path].first).to eq('foo has been taken before. Please use another one')
end
end
context "when the path has not been taken before" do
it 'should be valid' do
expect(RedirectRoute.count).to eq(0)
namespace = build(:namespace)
expect(namespace).to be_valid
end
end
end
end
......@@ -87,6 +87,7 @@ describe Route do
end
context 'when conflicting redirects exist' do
let(:route) { create(:project).route }
let!(:conflicting_redirect1) { route.create_redirect('bar/test') }
let!(:conflicting_redirect2) { route.create_redirect('bar/test/foo') }
let!(:conflicting_redirect3) { route.create_redirect('gitlab-org') }
......@@ -141,11 +142,50 @@ describe Route do
expect(redirect_route.source).to eq(route.source)
expect(redirect_route.path).to eq('foo')
end
context 'when the source is a Project' do
it 'creates a temporal RedirectRoute' do
project = create(:project)
route = project.route
redirect_route = route.create_redirect('foo')
expect(redirect_route.permanent?).to be_falsy
end
end
context 'when the source is not a project' do
it 'creates a permanent RedirectRoute' do
redirect_route = route.create_redirect('foo', permanent: true)
expect(redirect_route.permanent?).to be_truthy
end
end
end
describe '#delete_conflicting_redirects' do
context 'with permanent redirect' do
it 'does not delete the redirect' do
route.create_redirect("#{route.path}/foo", permanent: true)
expect do
route.delete_conflicting_redirects
end.not_to change { RedirectRoute.count }
end
end
context 'with temporal redirect' do
let(:route) { create(:project).route }
it 'deletes the redirect' do
route.create_redirect("#{route.path}/foo")
expect do
route.delete_conflicting_redirects
end.to change { RedirectRoute.count }.by(-1)
end
end
context 'when a redirect route with the same path exists' do
context 'when the redirect route has matching case' do
let(:route) { create(:project).route }
let!(:redirect1) { route.create_redirect(route.path) }
it 'deletes the redirect' do
......@@ -169,6 +209,7 @@ describe Route do
end
context 'when the redirect route is differently cased' do
let(:route) { create(:project).route }
let!(:redirect1) { route.create_redirect(route.path.upcase) }
it 'deletes the redirect' do
......@@ -185,7 +226,32 @@ describe Route do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
end
context 'with permanent redirects' do
it 'does not return anything' do
route.create_redirect("#{route.path}/foo", permanent: true)
route.create_redirect("#{route.path}/foo/bar", permanent: true)
route.create_redirect("#{route.path}/baz/quz", permanent: true)
expect(route.conflicting_redirects).to be_empty
end
end
context 'with temporal redirects' do
let(:route) { create(:project).route }
it 'returns the redirect routes' do
route = create(:project).route
redirect1 = route.create_redirect("#{route.path}/foo")
redirect2 = route.create_redirect("#{route.path}/foo/bar")
redirect3 = route.create_redirect("#{route.path}/baz/quz")
expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
end
end
context 'when a redirect route with the same path exists' do
let(:route) { create(:project).route }
context 'when the redirect route has matching case' do
let!(:redirect1) { route.create_redirect(route.path) }
......@@ -214,4 +280,42 @@ describe Route do
end
end
end
describe "#conflicting_redirect_exists?" do
context 'when a conflicting redirect exists' do
let(:group1) { create(:group, path: 'foo') }
let(:group2) { create(:group, path: 'baz') }
it 'should not be saved' do
group1.path = 'bar'
group1.save
group2.path = 'foo'
expect(group2.save).to be_falsy
end
it 'should return an error on path' do
group1.path = 'bar'
group1.save
group2.path = 'foo'
group2.valid?
expect(group2.errors["route.path"].first).to eq('foo has been taken before. Please use another one')
end
end
context 'when a conflicting redirect does not exist' do
let(:project1) { create(:project, path: 'foo') }
let(:project2) { create(:project, path: 'baz') }
it 'should be saved' do
project1.path = 'bar'
project1.save
project2.path = 'foo'
expect(project2.save).to be_truthy
end
end
end
end
......@@ -2592,4 +2592,28 @@ describe User do
include_examples 'max member access for groups'
end
end
describe "#username_previously_taken?" do
let(:user1) { create(:user, username: 'foo') }
context 'when the username has been taken before' do
before do
user1.username = 'bar'
user1.save!
end
it 'should raise an ActiveRecord::RecordInvalid exception' do
user2 = build(:user, username: 'foo')
expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Path foo has been taken before/)
end
end
context 'when the username has not been taken before' do
it 'should be valid' do
expect(RedirectRoute.count).to eq(0)
user2 = build(:user, username: 'baz')
expect(user2).to be_valid
end
end
end
end