Merge branch 'dm-user-namespace-route-path-validation' into 'master'

Validate user namespace before saving so that errors persist on model

Closes #42140

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

app/models/namespace.rb

@@ -20,6 +20,9 @@ class Namespace < ActiveRecord::Base
 
   has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :project_statistics
+
+  # This should _not_ be `inverse_of: :namespace`, because that would also set
+  # `user.namespace` when this user creates a group with themselves as `owner`.
   belongs_to :owner, class_name: "User"
 
   belongs_to :parent, class_name: "Namespace"

app/models/user.rb

@@ -77,7 +77,7 @@ class User < ActiveRecord::Base
   #
 
   # Namespace for personal projects
-  has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, autosave: true # rubocop:disable Cop/ActiveRecordDependent
+  has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
 
   # Profile
   has_many :keys, -> do
@@ -171,7 +171,7 @@ class User < ActiveRecord::Base
   before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? }
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
   before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? }
-  after_save :ensure_namespace_correct
+  before_validation :ensure_namespace_correct
   after_update :username_changed_hook, if: :username_changed?
   after_destroy :post_destroy_hook
   after_destroy :remove_key_cache
@@ -884,16 +884,10 @@ class User < ActiveRecord::Base
   end
 
   def ensure_namespace_correct
-    # Ensure user has namespace
-    create_namespace!(path: username, name: username) unless namespace
-
-    if username_changed?
-      unless namespace.update_attributes(path: username, name: username)
-        namespace.errors.each do |attribute, message|
-          self.errors.add(:"namespace_#{attribute}", message)
-        end
-        raise ActiveRecord::RecordInvalid.new(namespace)
-      end
+    if namespace
+      namespace.path = namespace.name = username if username_changed?
+    else
+      build_namespace(path: username, name: username)
     end
   end
 

changelogs/unreleased/dm-user-namespace-route-path-validation.yml

+---
+title: Validate user namespace before saving so that errors persist on model
+merge_request:
+author:
+type: fixed

spec/lib/gitlab/closing_issue_extractor_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::ClosingIssueExtractor do
   let(:project) { create(:project) }
   let(:project2) { create(:project) }
-  let(:forked_project) { Projects::ForkService.new(project, project.creator).execute }
+  let(:forked_project) { Projects::ForkService.new(project, project2.creator).execute }
   let(:issue) { create(:issue, project: project) }
   let(:issue2) { create(:issue, project: project2) }
   let(:reference) { issue.to_reference }
@@ -14,6 +14,7 @@ describe Gitlab::ClosingIssueExtractor do
 
   before do
     project.add_developer(project.creator)
+    project.add_developer(project2.creator)
     project2.add_master(project.creator)
   end
 

spec/models/user_spec.rb

@@ -101,7 +101,7 @@ describe User do
         user = build(:user, username: 'dashboard')
 
         expect(user).not_to be_valid
-        expect(user.errors.values).to eq [['dashboard is a reserved name']]
+        expect(user.errors.messages[:username]).to eq ['dashboard is a reserved name']
       end
 
       it 'allows child names' do
@@ -132,6 +132,23 @@ describe User do
           expect(user.errors.messages[:username].first).to match('cannot be changed if a personal project has container registry tags')
         end
       end
+
+      context 'when the username was used by another user before' do
+        let(:username) { 'foo' }
+        let!(:other_user) { create(:user, username: username) }
+
+        before do
+          other_user.username = 'bar'
+          other_user.save!
+        end
+
+        it 'is invalid' do
+          user = build(:user, username: username)
+
+          expect(user).not_to be_valid
+          expect(user.errors.messages[:"namespace.route.path"].first).to eq('foo has been taken before. Please use another one')
+        end
+      end
     end
 
     it 'has a DB-level NOT NULL constraint on projects_limit' do
@@ -2623,7 +2640,7 @@ describe User do
 
       it 'should raise an ActiveRecord::RecordInvalid exception' do
         user2 = build(:user, username: 'foo')
-        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Route path foo has been taken before. Please use another one, Route is invalid/)
+        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Namespace route path foo has been taken before/)
       end
     end
 

spec/services/projects/gitlab_projects_import_service_spec.rb

 require 'spec_helper'
 
 describe Projects::GitlabProjectsImportService do
-  set(:namespace) { build(:namespace) }
+  set(:namespace) { create(:namespace) }
   let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
   subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) }