diff options
author | Mayra Cabrera <mcabrera@gitlab.com> | 2017-12-08 17:42:43 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-12-08 17:42:43 +0000 |
commit | 562fb460b83502c060cd84b1627dc33098e01c31 (patch) | |
tree | 36d354ef789c26146f29a00400c240b76f5c02ca | |
parent | 8730054683c00f4c79cf452beef0df794c70bf6b (diff) | |
download | gitlab-ce-562fb460b83502c060cd84b1627dc33098e01c31.tar.gz |
Allow git pull/push on project redirects
-rw-r--r-- | GITLAB_SHELL_VERSION | 2 | ||||
-rw-r--r-- | app/models/namespace.rb | 11 | ||||
-rw-r--r-- | app/models/redirect_route.rb | 28 | ||||
-rw-r--r-- | app/models/route.rb | 26 | ||||
-rw-r--r-- | changelogs/unreleased/35385-allow-git-pull-push-on-project-redirects.yml | 5 | ||||
-rw-r--r-- | db/migrate/20171204204233_add_permanent_to_redirect_route.rb | 18 | ||||
-rw-r--r-- | db/migrate/20171206221519_add_permanent_index_to_redirect_route.rb | 19 | ||||
-rw-r--r-- | db/schema.rb | 4 | ||||
-rw-r--r-- | lib/api/internal.rb | 13 | ||||
-rw-r--r-- | lib/gitlab/checks/project_moved.rb | 65 | ||||
-rw-r--r-- | lib/gitlab/git_access.rb | 17 | ||||
-rw-r--r-- | lib/gitlab/identifier.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/checks/project_moved_spec.rb | 81 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 60 | ||||
-rw-r--r-- | spec/lib/gitlab/identifier_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 30 | ||||
-rw-r--r-- | spec/models/route_spec.rb | 104 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 24 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 38 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 8 |
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 |