Search account domain in lowercase (#13016)

* Search account domain in lowercase

* fix rubocop error

* fix spec/models/account_spec.rb
This commit is contained in:
abcang 2020-02-01 23:42:24 +09:00 committed by GitHub
parent 37dc12dd53
commit 61a7390b66
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 55 additions and 16 deletions

View file

@ -70,14 +70,13 @@ class Account < ApplicationRecord
enum protocol: [:ostatus, :activitypub] enum protocol: [:ostatus, :activitypub]
validates :username, presence: true validates :username, presence: true
validates_with UniqueUsernameValidator, if: -> { will_save_change_to_username? }
# Remote user validations # Remote user validations
validates :username, uniqueness: { scope: :domain, case_sensitive: true }, if: -> { !local? && will_save_change_to_username? }
validates :username, format: { with: /\A#{USERNAME_RE}\z/i }, if: -> { !local? && will_save_change_to_username? } validates :username, format: { with: /\A#{USERNAME_RE}\z/i }, if: -> { !local? && will_save_change_to_username? }
# Local user validations # Local user validations
validates :username, format: { with: /\A[a-z0-9_]+\z/i }, length: { maximum: 30 }, if: -> { local? && will_save_change_to_username? && actor_type != 'Application' } validates :username, format: { with: /\A[a-z0-9_]+\z/i }, length: { maximum: 30 }, if: -> { local? && will_save_change_to_username? && actor_type != 'Application' }
validates_with UniqueUsernameValidator, if: -> { local? && will_save_change_to_username? }
validates_with UnreservedUsernameValidator, if: -> { local? && will_save_change_to_username? } validates_with UnreservedUsernameValidator, if: -> { local? && will_save_change_to_username? }
validates :display_name, length: { maximum: 30 }, if: -> { local? && will_save_change_to_display_name? } validates :display_name, length: { maximum: 30 }, if: -> { local? && will_save_change_to_display_name? }
validates :note, note_length: { maximum: 500 }, if: -> { local? && will_save_change_to_note? } validates :note, note_length: { maximum: 500 }, if: -> { local? && will_save_change_to_note? }

View file

@ -48,7 +48,7 @@ module AccountFinderConcern
end end
def with_usernames def with_usernames
Account.where.not(username: '') Account.where.not(Account.arel_table[:username].lower.eq '')
end end
def matching_username def matching_username
@ -56,11 +56,7 @@ module AccountFinderConcern
end end
def matching_domain def matching_domain
if domain.nil? Account.where(Account.arel_table[:domain].lower.eq(domain.nil? ? nil : domain.to_s.downcase))
Account.where(domain: nil)
else
Account.where(Account.arel_table[:domain].lower.eq domain.to_s.downcase)
end
end end
end end
end end

View file

@ -7,8 +7,9 @@ class UniqueUsernameValidator < ActiveModel::Validator
return if account.username.nil? return if account.username.nil?
normalized_username = account.username.downcase normalized_username = account.username.downcase
normalized_domain = account.domain&.downcase
scope = Account.where(domain: nil).where('lower(username) = ?', normalized_username) scope = Account.where(Account.arel_table[:username].lower.eq normalized_username).where(Account.arel_table[:domain].lower.eq normalized_domain)
scope = scope.where.not(id: account.id) if account.persisted? scope = scope.where.not(id: account.id) if account.persisted?
account.errors.add(:username, :taken) if scope.exists? account.errors.add(:username, :taken) if scope.exists?

View file

@ -619,18 +619,18 @@ RSpec.describe Account, type: :model do
end end
context 'when is remote' do context 'when is remote' do
it 'is invalid if the username is not unique in case-sensitive comparison among accounts in the same normalized domain' do it 'is invalid if the username is same among accounts in the same normalized domain' do
Fabricate(:account, domain: 'にゃん', username: 'username') Fabricate(:account, domain: 'にゃん', username: 'username')
account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'username') account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'username')
account.valid? account.valid?
expect(account).to model_have_error_on_field(:username) expect(account).to model_have_error_on_field(:username)
end end
it 'is valid even if the username is unique only in case-sensitive comparison among accounts in the same normalized domain' do it 'is invalid if the username is not unique in case-insensitive comparison among accounts in the same normalized domain' do
Fabricate(:account, domain: 'にゃん', username: 'username') Fabricate(:account, domain: 'にゃん', username: 'username')
account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'Username') account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'Username')
account.valid? account.valid?
expect(account).not_to model_have_error_on_field(:username) expect(account).to model_have_error_on_field(:username)
end end
it 'is valid even if the username contains hyphens' do it 'is valid even if the username contains hyphens' do

View file

@ -4,22 +4,65 @@ require 'rails_helper'
describe UniqueUsernameValidator do describe UniqueUsernameValidator do
describe '#validate' do describe '#validate' do
context 'when local account' do
it 'does not add errors if username is nil' do
account = double(username: nil, domain: nil, persisted?: false, errors: double(add: nil))
subject.validate(account)
expect(account.errors).to_not have_received(:add)
end
it 'does not add errors when existing one is subject itself' do
account = Fabricate(:account, username: 'abcdef')
expect(account).to be_valid
end
it 'adds an error when the username is already used with ignoring cases' do
Fabricate(:account, username: 'ABCdef')
account = double(username: 'abcDEF', domain: nil, persisted?: false, errors: double(add: nil))
subject.validate(account)
expect(account.errors).to have_received(:add)
end
it 'does not add errors when same username remote account exists' do
Fabricate(:account, username: 'abcdef', domain: 'example.com')
account = double(username: 'abcdef', domain: nil, persisted?: false, errors: double(add: nil))
subject.validate(account)
expect(account.errors).to_not have_received(:add)
end
end
end
context 'when remote account' do
it 'does not add errors if username is nil' do it 'does not add errors if username is nil' do
account = double(username: nil, persisted?: false, errors: double(add: nil)) account = double(username: nil, domain: 'example.com', persisted?: false, errors: double(add: nil))
subject.validate(account) subject.validate(account)
expect(account.errors).to_not have_received(:add) expect(account.errors).to_not have_received(:add)
end end
it 'does not add errors when existing one is subject itself' do it 'does not add errors when existing one is subject itself' do
account = Fabricate(:account, username: 'abcdef') account = Fabricate(:account, username: 'abcdef', domain: 'example.com')
expect(account).to be_valid expect(account).to be_valid
end end
it 'adds an error when the username is already used with ignoring cases' do it 'adds an error when the username is already used with ignoring cases' do
Fabricate(:account, username: 'ABCdef') Fabricate(:account, username: 'ABCdef', domain: 'example.com')
account = double(username: 'abcDEF', persisted?: false, errors: double(add: nil)) account = double(username: 'abcDEF', domain: 'example.com', persisted?: false, errors: double(add: nil))
subject.validate(account) subject.validate(account)
expect(account.errors).to have_received(:add) expect(account.errors).to have_received(:add)
end end
it 'adds an error when the domain is already used with ignoring cases' do
Fabricate(:account, username: 'ABCdef', domain: 'example.com')
account = double(username: 'ABCdef', domain: 'EXAMPLE.COM', persisted?: false, errors: double(add: nil))
subject.validate(account)
expect(account.errors).to have_received(:add)
end
it 'does not add errors when account with the same username and another domain exists' do
Fabricate(:account, username: 'abcdef', domain: 'example.com')
account = double(username: 'abcdef', domain: 'example2.com', persisted?: false, errors: double(add: nil))
subject.validate(account)
expect(account.errors).to_not have_received(:add)
end
end end
end end