summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2018-02-06 18:40:09 +0000
committerSean McGivern <sean@mcgivern.me.uk>2018-02-06 18:40:09 +0000
commite776096e84d01ab3d1d07a028b65e7430e195114 (patch)
tree81010600bf489a0ee0e44a3ed0dc727fa04b581c
parent8999d8eb5ec7aa51a4e28350d981c241f5993045 (diff)
parent8d69436c901a3eee674e72c67d91de3994d30e0c (diff)
downloadgitlab-ce-e776096e84d01ab3d1d07a028b65e7430e195114.tar.gz
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
-rw-r--r--app/models/namespace.rb3
-rw-r--r--app/models/user.rb18
-rw-r--r--changelogs/unreleased/dm-user-namespace-route-path-validation.yml5
-rw-r--r--spec/lib/gitlab/closing_issue_extractor_spec.rb3
-rw-r--r--spec/models/user_spec.rb21
-rw-r--r--spec/services/projects/gitlab_projects_import_service_spec.rb2
6 files changed, 36 insertions, 16 deletions
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 }) }