Merge pull request #168 from azul/bugfix/fix-login-validations

Fix login validations

1 files changed, 34 insertions, 21 deletions

diff --git a/app/models/identity.rb b/app/models/identity.rb
index a4225e7..2f6241c 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -10,8 +10,10 @@ class Identity < CouchRest::Model::Base
   property :keys, HashWithIndifferentAccess
   property :cert_fingerprints, Hash
 
-  validate :unique_forward
-  validate :alias_available
+  validates :address, presence: true
+  validate :address_available
+  validates :destination, presence: true, if: :enabled?
+  validates :destination, uniqueness: {scope: :address}
   validate :address_local_email
   validate :destination_email
 
@@ -50,7 +52,8 @@ class Identity < CouchRest::Model::Base
 
   def self.find_for(user, attributes = {})
     attributes.reverse_merge! attributes_from_user(user)
-    find_by_address_and_destination [attributes[:address], attributes[:destination]]
+    id = find_by_address_and_destination attributes.values_at(:address, :destination)
+    return id if id && id.user == user
   end
 
   def self.build_for(user, attributes = {})
@@ -67,7 +70,9 @@ class Identity < CouchRest::Model::Base
   def self.disable_all_for(user)
     Identity.by_user_id.key(user.id).each do |identity|
       identity.disable
-      identity.save
+      # if the identity is not unique anymore because the destination
+      # was reset to nil we destroy it.
+      identity.save || identity.destroy
     end
   end
 
@@ -91,7 +96,11 @@ class Identity < CouchRest::Model::Base
   end
 
   def enabled?
-    self.destination && self.user_id
+    self.user_id
+  end
+
+  def disabled?
+    !enabled?
   end
 
   def disable
@@ -120,34 +129,38 @@ class Identity < CouchRest::Model::Base
 
   # for LoginFormatValidation
   def login
-    self.address.handle
+    address.handle if address.present?
   end
 
   protected
 
-  def unique_forward
-    same = Identity.find_by_address_and_destination([address, destination])
-    if same && same != self
-      errors.add :base, "This alias already exists"
+  def address_available
+    blocking_identities = Identity.by_address.key(address).all
+    blocking_identities.delete self
+    if self.user
+      blocking_identities.reject! { |other| other.user == self.user }
     end
-  end
-
-  def alias_available
-    same = Identity.find_by_address(address)
-    if same && same.user != self.user
-      errors.add :base, "This email has already been taken"
+    if blocking_identities.any?
+      errors.add :address, :taken
     end
   end
 
   def address_local_email
-    return if address.valid? #this ensures it is LocalEmail
-    self.errors.add(:address, address.errors.messages[:email].first) #assumes only one error
+    # caught by presence validation
+    return if address.blank?
+    return if address.valid?
+    address.errors.each do |attribute, error|
+      self.errors.add(:address, error)
+    end
   end
 
   def destination_email
-    return if destination.nil?   # this identity is disabled
-    return if destination.valid? # this ensures it is Email
-    self.errors.add(:destination, destination.errors.messages[:email].first) #assumes only one error #TODO
+    # caught by presence validation or this identity is disabled
+    return if destination.blank?
+    return if destination.valid?
+    destination.errors.each do |attribute, error|
+      self.errors.add(:destination, error)
+    end
   end
 
 end