From 8d69436c901a3eee674e72c67d91de3994d30e0c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 31 Jan 2018 10:51:05 -0600 Subject: Validate user namespace before saving so that errors persist on model --- app/models/namespace.rb | 3 +++ app/models/user.rb | 18 ++++++------------ .../dm-user-namespace-route-path-validation.yml | 5 +++++ spec/lib/gitlab/closing_issue_extractor_spec.rb | 3 ++- spec/models/user_spec.rb | 21 +++++++++++++++++++-- .../projects/gitlab_projects_import_service_spec.rb | 2 +- 6 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/dm-user-namespace-route-path-validation.yml diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 06655298950..44c5bb13a59 100644 --- a/app/models/namespace.rb +++ b/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" diff --git a/app/models/user.rb b/app/models/user.rb index 4996cea718c..1df3772e63f 100644 --- a/app/models/user.rb +++ b/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 diff --git a/changelogs/unreleased/dm-user-namespace-route-path-validation.yml b/changelogs/unreleased/dm-user-namespace-route-path-validation.yml new file mode 100644 index 00000000000..36615e5b976 --- /dev/null +++ b/changelogs/unreleased/dm-user-namespace-route-path-validation.yml @@ -0,0 +1,5 @@ +--- +title: Validate user namespace before saving so that errors persist on model +merge_request: +author: +type: fixed diff --git a/spec/lib/gitlab/closing_issue_extractor_spec.rb b/spec/lib/gitlab/closing_issue_extractor_spec.rb index 8c79ef54c6c..28c679af12a 100644 --- a/spec/lib/gitlab/closing_issue_extractor_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 011416cc176..24d4d8f1741 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index bb0e274c93e..6b8f9619bc4 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -1,7 +1,7 @@ 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 }) } -- cgit v1.2.1