Merge branch '35385-allow-git-pull-push-on-project-redirects' into 'master'

Allow git pull/push on project redirects

Closes #35385

See merge request gitlab-org/gitlab-ce!15670

(cherry picked from commit 2a181d68c810e89ff5d2e49999775d8b58adb394)

37c99562 Allow git pull/push on project redirects
64811239 Allow group/username transfers:
4344bba4 Fixes wrong condition on git access
b6bac528 Wrap ProjectMoved behavior into a class
70676e3b Fixes permanent migration
3328950e Moves allowed_path_validation up to Namespace
54900c4a Addreses ME review comments
740ed1e0 Add several improvements
6a0d9261 Add fixes based on MR suggestions
8a1a257b Ensures validations are only executed if RedirectRoute#permanent column exists
448f1c54 Fixes broken specs and suggestions on MR
da789af6 Add fixes based on MR comments
2bd96f5f Bump GITLAB_SHELL_VERSION to 5.10.2
75981dc4 Ensure RedirectRoute column information is flushed

8 files changed, 318 insertions, 31 deletions

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