From a160a3acce3d2bd4e6ee09dfd7c72916cf94f910 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 5 Mar 2022 00:46:49 -0600 Subject: [PATCH] users: add stricter username rules. Add stricter username rules: * Only allow usernames to contain basic letters, numbers, CJK characters, underscores, dashes and periods. * Don't allow names to start or end with punctuation. * Don't allow names to have multiple underscores in a row. * Don't allow active users to have names that look like deleted users (e.g. "user_1234"). * Don't allow emoji or any other Unicode characters except for Chinese, Japanese, and Korean characters. CJK characters are currently grandfathered in but will be disallowed in the future. Users with an invalid name will be shown a permanent sitewide banner until they change their name. --- app/logical/user_name_validator.rb | 35 +++++++--- app/models/user.rb | 16 +++++ app/views/layouts/default.html.erb | 7 ++ .../user_name_change_requests/new.html.erb | 31 +++++++-- .../functional/application_controller_test.rb | 11 +++ ...er_name_change_requests_controller_test.rb | 7 ++ test/unit/user_test.rb | 67 +++++++++++++------ 7 files changed, 140 insertions(+), 34 deletions(-) diff --git a/app/logical/user_name_validator.rb b/app/logical/user_name_validator.rb index 8d44cb064..99ecce9dc 100644 --- a/app/logical/user_name_validator.rb +++ b/app/logical/user_name_validator.rb @@ -7,14 +7,33 @@ # # @see https://guides.rubyonrails.org/active_record_validations.html#custom-validators class UserNameValidator < ActiveModel::EachValidator - def validate_each(rec, attr, value) - name = value + ALLOWED_PUNCTUATION = "_.-" # All other punctuation characters are forbidden - rec.errors.add(attr, "already exists") if User.find_by_name(name).present? - rec.errors.add(attr, "must be more than 1 character long") if name.length <= 1 - rec.errors.add(attr, "must be less than 25 characters long") if name.length >= 25 - rec.errors.add(attr, "cannot have whitespace or colons") if name =~ /[[:space:]]|:/ - rec.errors.add(attr, "cannot begin or end with an underscore") if name =~ /\A_|_\z/ - rec.errors.add(attr, "is not allowed") if name =~ Regexp.union(Danbooru.config.user_name_blacklist) + def validate_each(rec, attr, name) + forbidden_characters = name.delete(ALLOWED_PUNCTUATION).chars.grep(/[[:punct:]]/).uniq + + if rec.new_record? && User.find_by_name(name).present? + rec.errors.add(attr, "already exists") + elsif name.length <= 1 + rec.errors.add(attr, "must be more than 1 character long") + elsif name.length >= 25 + rec.errors.add(attr, "must be less than 25 characters long") + elsif name =~ /[[:space:]]/ + rec.errors.add(attr, "can't contain whitespace") + elsif name =~ /\A[[:punct:]]/ + rec.errors.add(attr, "can't start with '#{name.first}'") + elsif name =~ /[[:punct:]]\z/ + rec.errors.add(attr, "can't end with '#{name.last}'") + elsif name =~ /__/ + rec.errors.add(attr, "can't contain multiple underscores in a row") + elsif forbidden_characters.present? + rec.errors.add(attr, "can't contain #{forbidden_characters.map { |c| "'#{c}'" }.to_sentence}") + elsif name !~ /\A([a-zA-Z0-9]|\p{Han}|\p{Hangul}|\p{Hiragana}|\p{Katakana}|[#{ALLOWED_PUNCTUATION}])+\z/ + rec.errors.add(attr, "must contain only basic letters or numbers") + elsif name =~ /\Auser_\d+\z/i + rec.errors.add(attr, "can't be the same as a deleted user") + elsif name =~ Regexp.union(Danbooru.config.user_name_blacklist) + rec.errors.add(attr, "is not allowed") + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 7a55daa9d..323fe94d2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class User < ApplicationRecord + extend Memoist + class PrivilegeError < StandardError; end module Levels @@ -204,6 +206,18 @@ class User < ApplicationRecord errors.add(:base, "Can't enable privacy mode without a Gold account") end end + + def name_errors + User.validators_on(:name).each do |validator| + validator.validate_each(self, :name, name) + end + + errors + end + + def name_invalid? + name_errors.present? + end end concerning :AuthenticationMethods do @@ -695,4 +709,6 @@ class User < ApplicationRecord def self.available_includes [:inviter] end + + memoize :name_errors end diff --git a/app/views/layouts/default.html.erb b/app/views/layouts/default.html.erb index fe035f1fb..5c45e848c 100644 --- a/app/views/layouts/default.html.erb +++ b/app/views/layouts/default.html.erb @@ -104,6 +104,13 @@ <%= render "users/dmail_notice" %> <% end %> + <% if !CurrentUser.user.is_anonymous? && CurrentUser.user.name_invalid? %> +
+

Action required

+
You must <%= link_to "change your username", new_user_name_change_request_path %> to continue using <%= Danbooru.config.canonical_app_name %>.
+
+ <% end %> +
"> <%= format_text(flash[:notice], inline: true) %>. close diff --git a/app/views/user_name_change_requests/new.html.erb b/app/views/user_name_change_requests/new.html.erb index b1be8501d..0c7698c55 100644 --- a/app/views/user_name_change_requests/new.html.erb +++ b/app/views/user_name_change_requests/new.html.erb @@ -1,13 +1,32 @@
-
-

Name Change Request

+
+

Change Name

-

You can request a name change once per week. Your previous names will still - be visible on your profile to other Danbooru members, but they won't be visible - to search engines.

+ <% if CurrentUser.user.name_invalid? %> +

Your current username is invalid. You must change your username to continue + using <%= Danbooru.config.canonical_app_name %>.

+ +

+ Current name: <%= CurrentUser.user.name %>.
+ Error: <%= CurrentUser.user.name_errors.full_messages.join(". ").html_safe %>. +

+ <% end %> + +
+
Rules
+
    +
  • Names can contain only letters, numbers, underscore ('_'), period ('.'), and dash ('-') characters.
    • +
  • Names must start and end with a letter or number.
    • +
  • Names must be less than 25 characters long.
    • +
  • Names can't insult or impersonate other users.
    • +
  • Names are case-insensitive.
    • +
  • Your previous names will be visible on your profile to other Danbooru members, but they won't be visible to search engines.
    • +
  • You can't change your name more than once per week.
    • +
+
<%= edit_form_for(@change_request) do |f| %> - <%= f.input :desired_name, label: "Name" %> + <%= f.input :desired_name, label: "New name" %> <%= f.input :desired_name_confirmation, label: "Confirm name" %> <%= f.submit "Submit", "data-confirm": "Are you sure you want to change your name?" %> <% end %> diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index 53ff6a86e..8fa07e359 100644 --- a/test/functional/application_controller_test.rb +++ b/test/functional/application_controller_test.rb @@ -75,6 +75,17 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest end end + context "when a user has an invalid username" do + should "show a warning banner" do + @user = create(:user) + @user.update_columns(name: "foo__bar") + + get_auth posts_path, @user + assert_response :success + assert_select "#invalid-name-notice" + end + end + context "on api authentication" do setup do @user = create(:user, password: "password") diff --git a/test/functional/user_name_change_requests_controller_test.rb b/test/functional/user_name_change_requests_controller_test.rb index b9276d24b..d8e6abf40 100644 --- a/test/functional/user_name_change_requests_controller_test.rb +++ b/test/functional/user_name_change_requests_controller_test.rb @@ -12,6 +12,13 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest get_auth new_user_name_change_request_path, @user assert_response :success end + + should "render when the current user's name is invalid" do + @user.update_columns(name: "foo__bar") + get_auth new_user_name_change_request_path, @user + + assert_response :success + end end context "create action" do diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 1006bc6cb..3bcbf157c 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -148,27 +148,51 @@ class UserTest < ActiveSupport::TestCase context "name" do should "not contain whitespace" do # U+2007: https://en.wikipedia.org/wiki/Figure_space - user = FactoryBot.build(:user, :name => "foo\u2007bar") + user = build(:user, name: "foo\u2007bar") user.save - assert_equal(["Name cannot have whitespace or colons"], user.errors.full_messages) + assert_equal(["Name can't contain whitespace"], user.errors.full_messages) + end + + should "be less than 25 characters long" do + user = build(:user, name: "a"*25) + user.save + assert_equal(["Name must be less than 25 characters long"], user.errors.full_messages) end should "not contain a colon" do - user = FactoryBot.build(:user, :name => "a:b") + user = build(:user, name: "a:b") user.save - assert_equal(["Name cannot have whitespace or colons"], user.errors.full_messages) + assert_equal(["Name can't contain ':'"], user.errors.full_messages) end should "not begin with an underscore" do - user = FactoryBot.build(:user, :name => "_x") + user = build(:user, name: "_x") user.save - assert_equal(["Name cannot begin or end with an underscore"], user.errors.full_messages) + assert_equal(["Name can't start with '_'"], user.errors.full_messages) end should "not end with an underscore" do - user = FactoryBot.build(:user, :name => "x_") + user = build(:user, name: "x_") user.save - assert_equal(["Name cannot begin or end with an underscore"], user.errors.full_messages) + assert_equal(["Name can't end with '_'"], user.errors.full_messages) + end + + should "not contain consecutive underscores" do + user = build(:user, name: "x__y") + user.save + assert_equal(["Name can't contain multiple underscores in a row"], user.errors.full_messages) + end + + should "not allow non-ASCII characters" do + user = build(:user, name: "Pokémon") + user.save + assert_equal(["Name must contain only basic letters or numbers"], user.errors.full_messages) + end + + should "not be in the same format as a deleted user" do + user = build(:user, name: "user_1234") + user.save + assert_equal(["Name can't be the same as a deleted user"], user.errors.full_messages) end should "not allow blacklisted names" do @@ -177,11 +201,6 @@ class UserTest < ActiveSupport::TestCase user.save assert_equal(["Name is not allowed"], user.errors.full_messages) end - - should "be updated" do - @user = FactoryBot.create(:user) - @user.update_attribute(:name, "danzig") - end end context "searching for users by name" do @@ -206,10 +225,14 @@ class UserTest < ActiveSupport::TestCase assert_nil(User.name_to_id("does_not_exist")) end - should "work for names containing asterisks or backlashes" do - @user1 = create(:user, name: "user*1") - @user2 = create(:user, name: "user*2") - @user3 = create(:user, name: "user\*3") + should "work for names containing asterisks or backslashes" do + @user1 = build(:user, name: "user*1") + @user2 = build(:user, name: "user*2") + @user3 = build(:user, name: "user\*3") + + @user1.save(validate: false) + @user2.save(validate: false) + @user3.save(validate: false) assert_equal(@user1.id, User.find_by_name("user*1").id) assert_equal(@user2.id, User.find_by_name("user*2").id) @@ -259,9 +282,13 @@ class UserTest < ActiveSupport::TestCase context "when searched by name" do should "match wildcards" do - user1 = FactoryBot.create(:user, :name => "foo") - user2 = FactoryBot.create(:user, :name => "foo*bar") - user3 = FactoryBot.create(:user, :name => "bar\*baz") + user1 = build(:user, name: "foo") + user2 = build(:user, name: "foo*bar") + user3 = build(:user, name: "bar\*baz") + + user1.save(validate: false) + user2.save(validate: false) + user3.save(validate: false) assert_equal([user2.id, user1.id], User.search(name: "foo*").map(&:id)) assert_equal([user2.id], User.search(name: "foo\*bar").map(&:id))