summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMayra Cabrera <mcabrera@gitlab.com>2017-12-08 17:42:43 +0000
committerDouwe Maan <douwe@gitlab.com>2017-12-08 17:42:43 +0000
commit562fb460b83502c060cd84b1627dc33098e01c31 (patch)
tree36d354ef789c26146f29a00400c240b76f5c02ca
parent8730054683c00f4c79cf452beef0df794c70bf6b (diff)
downloadgitlab-ce-562fb460b83502c060cd84b1627dc33098e01c31.tar.gz
Allow git pull/push on project redirects
-rw-r--r--GITLAB_SHELL_VERSION2
-rw-r--r--app/models/namespace.rb11
-rw-r--r--app/models/redirect_route.rb28
-rw-r--r--app/models/route.rb26
-rw-r--r--changelogs/unreleased/35385-allow-git-pull-push-on-project-redirects.yml5
-rw-r--r--db/migrate/20171204204233_add_permanent_to_redirect_route.rb18
-rw-r--r--db/migrate/20171206221519_add_permanent_index_to_redirect_route.rb19
-rw-r--r--db/schema.rb4
-rw-r--r--lib/api/internal.rb13
-rw-r--r--lib/gitlab/checks/project_moved.rb65
-rw-r--r--lib/gitlab/git_access.rb17
-rw-r--r--lib/gitlab/identifier.rb5
-rw-r--r--spec/lib/gitlab/checks/project_moved_spec.rb81
-rw-r--r--spec/lib/gitlab/git_access_spec.rb60
-rw-r--r--spec/lib/gitlab/identifier_spec.rb4
-rw-r--r--spec/models/namespace_spec.rb30
-rw-r--r--spec/models/route_spec.rb104
-rw-r--r--spec/models/user_spec.rb24
-rw-r--r--spec/requests/api/internal_spec.rb38
-rw-r--r--spec/requests/git_http_spec.rb8
20 files changed, 510 insertions, 52 deletions
diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION
index 4e32c7b1caf..269fb5dfe2c 100644
--- a/GITLAB_SHELL_VERSION
+++ b/GITLAB_SHELL_VERSION
@@ -1 +1 @@
-5.10.1
+5.10.2
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 901dbf2ba69..0ff169d4531 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -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
diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb
index 31de204d824..20532527346 100644
--- a/app/models/redirect_route.rb
+++ b/app/models/redirect_route.rb
@@ -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
diff --git a/app/models/route.rb b/app/models/route.rb
index 97e8a6ad9e9..7ba3ec06041 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -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
diff --git a/changelogs/unreleased/35385-allow-git-pull-push-on-project-redirects.yml b/changelogs/unreleased/35385-allow-git-pull-push-on-project-redirects.yml
new file mode 100644
index 00000000000..31450287caf
--- /dev/null
+++ b/changelogs/unreleased/35385-allow-git-pull-push-on-project-redirects.yml
@@ -0,0 +1,5 @@
+---
+title: Allow git pull/push on group/user/project redirects
+merge_request: 15670
+author:
+type: added
diff --git a/db/migrate/20171204204233_add_permanent_to_redirect_route.rb b/db/migrate/20171204204233_add_permanent_to_redirect_route.rb
new file mode 100644
index 00000000000..f3ae471201e
--- /dev/null
+++ b/db/migrate/20171204204233_add_permanent_to_redirect_route.rb
@@ -0,0 +1,18 @@
+# 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
diff --git a/db/migrate/20171206221519_add_permanent_index_to_redirect_route.rb b/db/migrate/20171206221519_add_permanent_index_to_redirect_route.rb
new file mode 100644
index 00000000000..33ce7e1aa68
--- /dev/null
+++ b/db/migrate/20171206221519_add_permanent_index_to_redirect_route.rb
@@ -0,0 +1,19 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 4c697a4a384..c0a141885ad 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -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|
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 451121a4cea..ccaaeca10d4 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -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
diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb
new file mode 100644
index 00000000000..3a1c0a3455e
--- /dev/null
+++ b/lib/gitlab/checks/project_moved.rb
@@ -0,0 +1,65 @@
+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
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 9d7d921bb9c..56f6febe86d 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -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)
diff --git a/lib/gitlab/identifier.rb b/lib/gitlab/identifier.rb
index 94678b6ec40..3f3f10596c5 100644
--- a/lib/gitlab/identifier.rb
+++ b/lib/gitlab/identifier.rb
@@ -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
diff --git a/spec/lib/gitlab/checks/project_moved_spec.rb b/spec/lib/gitlab/checks/project_moved_spec.rb
new file mode 100644
index 00000000000..fa1575e2177
--- /dev/null
+++ b/spec/lib/gitlab/checks/project_moved_spec.rb
@@ -0,0 +1,81 @@
+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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index c9643c5da47..2db560c2cec 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -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
diff --git a/spec/lib/gitlab/identifier_spec.rb b/spec/lib/gitlab/identifier_spec.rb
index cfaeb1f0d4f..0385dd762c2 100644
--- a/spec/lib/gitlab/identifier_spec.rb
+++ b/spec/lib/gitlab/identifier_spec.rb
@@ -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
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 3817f20bfe7..b7c6286fd83 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -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
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index fece370c03f..ddad6862a63 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 03c96a8f5aa..cdabd35b6ba 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -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
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 67e1539cbc3..3c31980b273 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -537,16 +537,7 @@ describe API::Internal do
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
+ let!(:repository) { project.repository }
before do
project.team << [user, :developer]
@@ -555,19 +546,17 @@ describe API::Internal do
end
it 'rejects the push' do
- push_with_path(key, old_path_to_repo)
+ push(key, project)
expect(response).to have_gitlab_http_status(200)
- expect(json_response['status']).to be_falsey
- expect(json_response['message']).to eq(project_moved_message)
+ expect(json_response['status']).to be_falsy
end
it 'rejects the SSH pull' do
- pull_with_path(key, old_path_to_repo)
+ pull(key, project)
expect(response).to have_gitlab_http_status(200)
- expect(json_response['status']).to be_falsey
- expect(json_response['message']).to eq(project_moved_message)
+ expect(json_response['status']).to be_falsy
end
end
end
@@ -695,7 +684,7 @@ describe API::Internal do
# end
# end
- describe 'POST /internal/post_receive' do
+ describe 'POST /internal/post_receive', :clean_gitlab_redis_shared_state do
let(:identifier) { 'key-123' }
let(:valid_params) do
@@ -713,6 +702,8 @@ describe API::Internal do
before do
project.team << [user, :developer]
+ allow(described_class).to receive(:identify).and_return(user)
+ allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
end
it 'enqueues a PostReceive worker job' do
@@ -780,6 +771,19 @@ describe API::Internal do
expect(json_response['broadcast_message']).to eq(nil)
end
end
+
+ context 'with a redirected data' do
+ it 'returns redirected message on the response' do
+ project_moved = Gitlab::Checks::ProjectMoved.new(project, user, 'foo/baz', 'http')
+ project_moved.add_redirect_message
+
+ post api("/internal/post_receive"), valid_params
+
+ expect(response).to have_gitlab_http_status(200)
+ expect(json_response["redirected_message"]).to be_present
+ expect(json_response["redirected_message"]).to eq(project_moved.redirect_message)
+ end
+ end
end
describe 'POST /internal/pre_receive' do
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index a16f98bec36..fa02fffc82a 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -324,9 +324,9 @@ describe 'Git HTTP requests' do
<<-MSG.strip_heredoc
Project '#{redirect.path}' was moved to '#{project.full_path}'.
- Please update your Git remote and try again:
+ Please update your Git remote:
- git remote set-url origin #{project.http_url_to_repo}
+ git remote set-url origin #{project.http_url_to_repo} and try again.
MSG
end
@@ -533,9 +533,9 @@ describe 'Git HTTP requests' do
<<-MSG.strip_heredoc
Project '#{redirect.path}' was moved to '#{project.full_path}'.
- Please update your Git remote and try again:
+ Please update your Git remote:
- git remote set-url origin #{project.http_url_to_repo}
+ git remote set-url origin #{project.http_url_to_repo} and try again.
MSG
end