Skip to content

Commit

Permalink
Require user names to be unique after unicode normalisation
Browse files Browse the repository at this point in the history
As with the previous checks on case sensitivity this only affects
new users, and changes to names of existing users.
  • Loading branch information
tomhughes committed Dec 13, 2023
1 parent 75042a0 commit c12f895
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 12 deletions.
17 changes: 9 additions & 8 deletions app/models/user.rb
Expand Up @@ -34,12 +34,13 @@
#
# Indexes
#
# users_auth_idx (auth_provider,auth_uid) UNIQUE
# users_display_name_idx (display_name) UNIQUE
# users_display_name_lower_idx (lower((display_name)::text))
# users_email_idx (email) UNIQUE
# users_email_lower_idx (lower((email)::text))
# users_home_idx (home_tile)
# users_auth_idx (auth_provider,auth_uid) UNIQUE
# users_display_name_canonical_idx (lower(NORMALIZE(display_name, NFKC)))
# users_display_name_idx (display_name) UNIQUE
# users_display_name_lower_idx (lower((display_name)::text))
# users_email_idx (email) UNIQUE
# users_email_lower_idx (lower((email)::text))
# users_home_idx (home_tile)
#

class User < ApplicationRecord
Expand Down Expand Up @@ -91,7 +92,7 @@ class User < ApplicationRecord
validates :display_name, :presence => true, :length => 3..255,
:exclusion => %w[new terms save confirm confirm-email go_public reset-password forgot-password suspended]
validates :display_name, :if => proc { |u| u.display_name_changed? },
:uniqueness => { :case_sensitive => false }
:normalized_uniqueness => { :case_sensitive => false }
validates :display_name, :if => proc { |u| u.display_name_changed? },
:characters => { :url_safe => true },
:whitespace => { :leading => false, :trailing => false }
Expand Down Expand Up @@ -129,7 +130,7 @@ def self.authenticate(options)
user = find_by("email = ? OR display_name = ?", options[:username].strip, options[:username])

if user.nil?
users = where("LOWER(email) = LOWER(?) OR LOWER(display_name) = LOWER(?)", options[:username].strip, options[:username])
users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", options[:username].strip, options[:username])

user = users.first if users.count == 1
end
Expand Down
18 changes: 18 additions & 0 deletions app/validators/normalized_uniqueness_validator.rb
@@ -0,0 +1,18 @@
class NormalizedUniquenessValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
relation = if options.fetch(:case_sensitive, true)
record.class.where("NORMALIZE(#{attribute}, NFKC) = NORMALIZE(?, NFKC)", value)
else
record.class.where("LOWER(NORMALIZE(#{attribute}, NFKC)) = LOWER(NORMALIZE(?, NFKC))", value)
end

relation = relation.where.not(record.class.primary_key => [record.id_in_database]) if record.persisted?

if relation.exists?
error_options = options.except(:case_sensitive)
error_options[:value] = value

record.errors.add(attribute, :taken, **error_options)
end
end
end
7 changes: 7 additions & 0 deletions db/migrate/20231213182102_add_canonical_user_index.rb
@@ -0,0 +1,7 @@
class AddCanonicalUserIndex < ActiveRecord::Migration[7.1]
disable_ddl_transaction!

def change
add_index :users, "LOWER(NORMALIZE(display_name, NFKC))", :name => "users_display_name_canonical_idx", :algorithm => :concurrently
end
end
8 changes: 8 additions & 0 deletions db/structure.sql
Expand Up @@ -2867,6 +2867,13 @@ CREATE INDEX user_tokens_user_id_idx ON public.user_tokens USING btree (user_id)
CREATE UNIQUE INDEX users_auth_idx ON public.users USING btree (auth_provider, auth_uid);


--
-- Name: users_display_name_canonical_idx; Type: INDEX; Schema: public; Owner: -
--

CREATE INDEX users_display_name_canonical_idx ON public.users USING btree (lower(NORMALIZE(display_name, NFKC)));


--
-- Name: users_display_name_idx; Type: INDEX; Schema: public; Owner: -
--
Expand Down Expand Up @@ -3510,6 +3517,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('23'),
('22'),
('21'),
('20231213182102'),
('20231206141457'),
('20231117170422'),
('20231101222146'),
Expand Down
11 changes: 7 additions & 4 deletions test/models/user_test.rb
Expand Up @@ -27,10 +27,13 @@ def test_unique_email
end

def test_unique_display_name
existing_user = create(:user)
new_user = build(:user, :display_name => existing_user.display_name)
assert_not new_user.save
assert_includes new_user.errors[:display_name], "has already been taken"
create(:user, :display_name => "H\u{e9}nryIV")

%W[H\u{e9}nryIV he\u{301}nryiv H\u{c9}nry\u2163 he\u{301}nry\u2173].each do |name|
new_user = build(:user, :display_name => name)
assert_not new_user.save
assert_includes new_user.errors[:display_name], "has already been taken"
end
end

def test_email_valid
Expand Down

0 comments on commit c12f895

Please sign in to comment.