Search account domain in lowercase (#13016)

* Search account domain in lowercase

* fix rubocop error

* fix spec/models/account_spec.rb

diff --git a/app/models/account.rb b/app/models/account.rb
index da6f51a9cb7e86324178e944dd9b0f94375bcd4b..0eb719d652a7d69fbd64a10c575e36a818243d87 100644 (file)
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -70,14 +70,13 @@ class Account < ApplicationRecord
   enum protocol: [:ostatus, :activitypub]
 
   validates :username, presence: true
+  validates_with UniqueUsernameValidator, if: -> { will_save_change_to_username? }
 
   # 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? }
 
   # 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_with UniqueUsernameValidator, 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 :note, note_length: { maximum: 500 }, if: -> { local? && will_save_change_to_note? }

diff --git a/app/models/concerns/account_finder_concern.rb b/app/models/concerns/account_finder_concern.rb
index a54c2174d490db945ae8c352cac4b8c3bb371214..04b2c981b17c4aea5317ba47398866bb4ced3170 100644 (file)
--- a/app/models/concerns/account_finder_concern.rb
+++ b/app/models/concerns/account_finder_concern.rb
@@ -48,7 +48,7 @@ module AccountFinderConcern
     end
 
     def with_usernames
-      Account.where.not(username: '')
+      Account.where.not(Account.arel_table[:username].lower.eq '')
     end
 
     def matching_username
@@ -56,11 +56,7 @@ module AccountFinderConcern
     end
 
     def matching_domain
-      if domain.nil?
-        Account.where(domain: nil)
-      else
-        Account.where(Account.arel_table[:domain].lower.eq domain.to_s.downcase)
-      end
+      Account.where(Account.arel_table[:domain].lower.eq(domain.nil? ? nil : domain.to_s.downcase))
     end
   end
 end

diff --git a/app/validators/unique_username_validator.rb b/app/validators/unique_username_validator.rb
index 4e24e3f5fe90f4c9224be1ce9505c457bf5d7b38..f87eb06baf2972d197bd0a0473798df152fa1bcb 100644 (file)
--- a/app/validators/unique_username_validator.rb
+++ b/app/validators/unique_username_validator.rb
@@ -7,8 +7,9 @@ class UniqueUsernameValidator < ActiveModel::Validator
     return if account.username.nil?
 
     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?
 
     account.errors.add(:username, :taken) if scope.exists?

diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index 4266122b24a1058adce27a4a6d55bfa6e6414f1f..98d29e6f3d37defe194fe2d3d8922ca50e9214ad 100644 (file)
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@@ -619,18 +619,18 @@ RSpec.describe Account, type: :model do
     end
 
     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')
         account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'username')
         account.valid?
         expect(account).to model_have_error_on_field(:username)
       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')
         account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'Username')
         account.valid?
-        expect(account).not_to model_have_error_on_field(:username)
+        expect(account).to model_have_error_on_field(:username)
       end
 
       it 'is valid even if the username contains hyphens' do

diff --git a/spec/validators/unique_username_validator_spec.rb b/spec/validators/unique_username_validator_spec.rb
index c2e2eedf41f26c5b53dc623eca69eb3d1b23aafa..6867cbc6ce2b6171d4f7e8bbc897f033f22290ad 100644 (file)
--- a/spec/validators/unique_username_validator_spec.rb
+++ b/spec/validators/unique_username_validator_spec.rb
@@ -4,22 +4,65 @@ require 'rails_helper'
 
 describe UniqueUsernameValidator 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
-      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)
       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')
+      account = Fabricate(:account, username: 'abcdef', domain: 'example.com')
       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', persisted?: false, errors: double(add: nil))
+      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 '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