Merge branch 'mk-fix-permanent-redirect-validation' into 'master'

Fix Route validation when conflicting permanent redirects exist

Closes gitlab-com/support-forum#2883 and #41786

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

4 files changed, 81 insertions, 1 deletions

diff --git a/app/models/route.rb b/app/models/route.rb
index 7ba3ec06041..412f5fb45a5 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,7 +8,7 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  validate :ensure_permanent_paths
+  validate :ensure_permanent_paths, if: :path_changed?
 
   after_create :delete_conflicting_redirects
   after_update :delete_conflicting_redirects, if: :path_changed?
diff --git a/changelogs/unreleased/mk-fix-permanent-redirect-validation.yml b/changelogs/unreleased/mk-fix-permanent-redirect-validation.yml
new file mode 100644
index 00000000000..153b2ccc25c
--- /dev/null
+++ b/changelogs/unreleased/mk-fix-permanent-redirect-validation.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent invalid Route path if path is unchanged
+merge_request: 16397
+author:
+type: fixed
diff --git a/spec/factories/redirect_routes.rb b/spec/factories/redirect_routes.rb
new file mode 100644
index 00000000000..c29c81c5df9
--- /dev/null
+++ b/spec/factories/redirect_routes.rb
@@ -0,0 +1,15 @@
+FactoryBot.define do
+  factory :redirect_route do
+    sequence(:path) { |n| "redirect#{n}" }
+    source factory: :group
+    permanent false
+
+    trait :permanent do
+      permanent true
+    end
+
+    trait :temporary do
+      permanent false
+    end
+  end
+end
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index ddad6862a63..8a3b1034f3c 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -16,6 +16,66 @@ describe Route do
     it { is_expected.to validate_presence_of(:source) }
     it { is_expected.to validate_presence_of(:path) }
     it { is_expected.to validate_uniqueness_of(:path).case_insensitive }
+
+    describe '#ensure_permanent_paths' do
+      context 'when the route is not yet persisted' do
+        let(:new_route) { described_class.new(path: 'foo', source: build(:group)) }
+
+        context 'when permanent conflicting redirects exist' do
+          it 'is invalid' do
+            redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
+            redirect.save!(validate: false)
+
+            expect(new_route.valid?).to be_falsey
+            expect(new_route.errors.first[1]).to eq('foo has been taken before. Please use another one')
+          end
+        end
+
+        context 'when no permanent conflicting redirects exist' do
+          it 'is valid' do
+            expect(new_route.valid?).to be_truthy
+          end
+        end
+      end
+
+      context 'when path has changed' do
+        before do
+          route.path = 'foo'
+        end
+
+        context 'when permanent conflicting redirects exist' do
+          it 'is invalid' do
+            redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
+            redirect.save!(validate: false)
+
+            expect(route.valid?).to be_falsey
+            expect(route.errors.first[1]).to eq('foo has been taken before. Please use another one')
+          end
+        end
+
+        context 'when no permanent conflicting redirects exist' do
+          it 'is valid' do
+            expect(route.valid?).to be_truthy
+          end
+        end
+      end
+
+      context 'when path has not changed' do
+        context 'when permanent conflicting redirects exist' do
+          it 'is valid' do
+            redirect = build(:redirect_route, :permanent, path: 'git_lab/foo/bar')
+            redirect.save!(validate: false)
+
+            expect(route.valid?).to be_truthy
+          end
+        end
+        context 'when no permanent conflicting redirects exist' do
+          it 'is valid' do
+            expect(route.valid?).to be_truthy
+          end
+        end
+      end
+    end
   end
 
   describe 'callbacks' do