From 154d32bbc7cfe21d83141ff2c9a3d805165231b8 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 10:45:14 +0200
Subject: use Identity for testing login availability

We create an identity alongside each user. Make sure the identity
is valid when creating the user. This also ensures that the login
picked is available because otherwise the identities address would
not be available anymore.
---
 app/models/identity.rb                   | 30 ++++++++++++------------------
 app/models/user.rb                       | 13 ++++++-------
 test/integration/browser/account_test.rb |  7 +++++++
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/app/models/identity.rb b/app/models/identity.rb
index a4225e7..2be396c 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -10,8 +10,9 @@ class Identity < CouchRest::Model::Base
   property :keys, HashWithIndifferentAccess
   property :cert_fingerprints, Hash
 
-  validate :unique_forward
   validate :alias_available
+  validates :destination, presence: true,
+   uniqueness: {scope: :address}
   validate :address_local_email
   validate :destination_email
 
@@ -44,13 +45,12 @@ class Identity < CouchRest::Model::Base
 
   end
 
-  def self.for(user, attributes = {})
-    find_for(user, attributes) || build_for(user, attributes)
+  def self.for(user)
+    find_for(user) || build_for(user)
   end
 
-  def self.find_for(user, attributes = {})
-    attributes.reverse_merge! attributes_from_user(user)
-    find_by_address_and_destination [attributes[:address], attributes[:destination]]
+  def self.find_for(user)
+    find_by_user_id(user.id) if user && user.persisted?
   end
 
   def self.build_for(user, attributes = {})
@@ -125,23 +125,17 @@ class Identity < CouchRest::Model::Base
 
   protected
 
-  def unique_forward
-    same = Identity.find_by_address_and_destination([address, destination])
-    if same && same != self
-      errors.add :base, "This alias already exists"
-    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"
+    same_address = Identity.by_address.key(address)
+    if same_address.detect { |other| other.user !=self.user }
+      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
+    return if address.valid?
+    # we only hand on the first error for now.
+    self.errors.add(:address, address.errors.messages.values.first)
   end
 
   def destination_email
diff --git a/app/models/user.rb b/app/models/user.rb
index 6678de6..6b4d1a9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -24,7 +24,7 @@ class User < CouchRest::Model::Base
     :uniqueness => true,
     :if => :serverside?
 
-  validate :login_is_unique_alias
+  validate :identity_is_valid
 
   validates :password_salt, :password_verifier,
     :format => { :with => /\A[\dA-Fa-f]+\z/, :message => "Only hex numbers allowed" }
@@ -161,12 +161,11 @@ class User < CouchRest::Model::Base
   #  Validation Functions
   ##
 
-  def login_is_unique_alias
-    alias_identity = Identity.find_by_address(self.email_address)
-    return if alias_identity.blank?
-    if alias_identity.user != self
-      errors.add(:login, "has already been taken")
-    end
+  def identity_is_valid
+    refresh_identity
+    return if identity.valid?
+    # hand on the first error only for now
+    self.errors.add(:login, identity.errors.messages.values.first)
   end
 
   def password
diff --git a/test/integration/browser/account_test.rb b/test/integration/browser/account_test.rb
index 491a9e1..82bb043 100644
--- a/test/integration/browser/account_test.rb
+++ b/test/integration/browser/account_test.rb
@@ -22,6 +22,12 @@ class AccountTest < BrowserIntegrationTest
     assert page.has_content?("Welcome #{username}")
   end
 
+  test "signup with reserved username" do
+    username = 'certmaster'
+    submit_signup username
+    assert page.has_content?("is reserved.")
+  end
+
   test "successful login" do
     username, password = submit_signup
     click_on 'Logout'
@@ -44,6 +50,7 @@ class AccountTest < BrowserIntegrationTest
     click_on I18n.t('account_settings')
     click_on I18n.t('destroy_my_account')
     assert page.has_content?(I18n.t('account_destroyed'))
+    assert_equal 1, Identity.by_address.key("#{username}@test.me").count
     attempt_login(username, password)
     assert_invalid_login(page)
   end
-- 
cgit v1.2.3


From 5c8ab9298cc4705de508a3f3f9d9d6370a01ff5e Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 11:43:50 +0200
Subject: minor: beautify handle lookup in etc/passwd some

---
 app/models/local_email.rb | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/app/models/local_email.rb b/app/models/local_email.rb
index 2b4c65e..ded7baf 100644
--- a/app/models/local_email.rb
+++ b/app/models/local_email.rb
@@ -58,11 +58,9 @@ class LocalEmail < Email
   end
 
   def handle_in_passwd?
-    begin
-      !!Etc.getpwnam(handle)
-    rescue ArgumentError
-      # handle was not found
-      return false
-    end
+    Etc.getpwnam(handle).present?
+  rescue ArgumentError
+    # handle was not found
+    return false
   end
 end
-- 
cgit v1.2.3


From 682b4060cb86c52ffda638f4f9a837f107540610 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 11:44:12 +0200
Subject: ensure identity is cleared on user.reload - fixes test

---
 app/models/pgp_key.rb                    | 3 ++-
 app/models/user.rb                       | 5 +++++
 test/integration/browser/account_test.rb | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/models/pgp_key.rb b/app/models/pgp_key.rb
index 66f8660..3384f4c 100644
--- a/app/models/pgp_key.rb
+++ b/app/models/pgp_key.rb
@@ -25,9 +25,10 @@ class PgpKey
 
   # allow comparison with plain keyblock strings.
   def ==(other)
+    return false if (self.present? != other.present?)
     self.equal?(other) or
     # relax the comparison on line ends.
-    self.to_s.tr_s("\n\r", '') == other.tr_s("\r\n", '')
+    self.to_s.tr_s("\n\r", '') == other.tr_s("\n\r", '')
   end
 
   protected
diff --git a/app/models/user.rb b/app/models/user.rb
index 6b4d1a9..33508b5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -42,6 +42,11 @@ class User < CouchRest::Model::Base
     view :by_created_at
   end # end of design
 
+  def reload
+    super
+    @identity = nil
+  end
+
   def to_json(options={})
     {
       :login => login,
diff --git a/test/integration/browser/account_test.rb b/test/integration/browser/account_test.rb
index 82bb043..8e6d433 100644
--- a/test/integration/browser/account_test.rb
+++ b/test/integration/browser/account_test.rb
@@ -109,7 +109,8 @@ class AccountTest < BrowserIntegrationTest
       # at some point we're done:
       page.assert_no_selector 'input[value="Saving..."]'
       assert page.has_field? 'Public key', with: pgp_key.to_s
-      assert_equal pgp_key, @user.reload.public_key
+      @user.reload
+      assert_equal pgp_key, @user.public_key
     end
   end
 
-- 
cgit v1.2.3


From 6fea83763f07add7d3bd07e3843b75aaf61e19b4 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 12:20:49 +0200
Subject: bring back the alias functionality in Identities

---
 app/models/account.rb  |  1 +
 app/models/identity.rb | 23 +++++++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/app/models/account.rb b/app/models/account.rb
index cf998e4..bffa288 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -18,6 +18,7 @@ class Account
   def self.create(attrs)
     @user = User.create(attrs).tap do |user|
       Identity.create_for user
+      user.refresh_identity
     end
   end
 
diff --git a/app/models/identity.rb b/app/models/identity.rb
index 2be396c..0d25bae 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -11,8 +11,7 @@ class Identity < CouchRest::Model::Base
   property :cert_fingerprints, Hash
 
   validate :alias_available
-  validates :destination, presence: true,
-   uniqueness: {scope: :address}
+  validates :destination, uniqueness: {scope: :address}
   validate :address_local_email
   validate :destination_email
 
@@ -45,12 +44,14 @@ class Identity < CouchRest::Model::Base
 
   end
 
-  def self.for(user)
-    find_for(user) || build_for(user)
+  def self.for(user, attributes = {})
+    find_for(user, attributes) || build_for(user, attributes)
   end
 
-  def self.find_for(user)
-    find_by_user_id(user.id) if user && user.persisted?
+  def self.find_for(user, attributes = {})
+    attributes.reverse_merge! attributes_from_user(user)
+    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 +68,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
 
@@ -127,15 +130,15 @@ class Identity < CouchRest::Model::Base
 
   def alias_available
     same_address = Identity.by_address.key(address)
-    if same_address.detect { |other| other.user !=self.user }
+    if same_address.detect { |other| other.user != self.user }
       errors.add :address, :taken
     end
   end
 
   def address_local_email
-    return if address.valid?
+    return if address.valid? #this ensures it is a valid local email address
     # we only hand on the first error for now.
-    self.errors.add(:address, address.errors.messages.values.first)
+    self.errors.add(:address, address.errors.messages[:email].first)
   end
 
   def destination_email
-- 
cgit v1.2.3


From 09dfa583eca69a3925c384c67c3d98cd8c69b360 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 12:28:07 +0200
Subject: allow changing the user_id on an identity

we set it to nil when we disable it
---
 app/models/identity.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/identity.rb b/app/models/identity.rb
index 0d25bae..a8eaba6 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -130,7 +130,7 @@ class Identity < CouchRest::Model::Base
 
   def alias_available
     same_address = Identity.by_address.key(address)
-    if same_address.detect { |other| other.user != self.user }
+    if same_address.detect { |other| other != self && other.user != self.user }
       errors.add :address, :taken
     end
   end
-- 
cgit v1.2.3


From 5b601707c8af8454dacf2edd846bc3386e148253 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Wed, 28 May 2014 12:29:50 +0200
Subject: adopt tests to new error messages for identities

---
 test/unit/identity_test.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index eca104f..9c938f8 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -39,7 +39,7 @@ class IdentityTest < ActiveSupport::TestCase
     id = Identity.create_for @user, address: alias_name, destination: forward_address
     dup = Identity.build_for @user, address: alias_name, destination: forward_address
     assert !dup.valid?
-    assert_equal ["This alias already exists"], dup.errors[:base]
+    assert_equal ["has already been taken"], dup.errors[:destination]
     id.destroy
   end
 
@@ -48,7 +48,7 @@ class IdentityTest < ActiveSupport::TestCase
     id = Identity.create_for @user, address: alias_name, destination: forward_address
     taken = Identity.build_for other_user, address: alias_name
     assert !taken.valid?
-    assert_equal ["This email has already been taken"], taken.errors[:base]
+    assert_equal ["has already been taken"], taken.errors[:address]
     id.destroy
   end
 
-- 
cgit v1.2.3


From 016e61ce9ab44cf58355e843b0c0d0085d373fc7 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 29 May 2014 09:38:53 +0200
Subject: catch corner cases of account creation

Users now always check if their identity is valid. We need to make
sure this works if the user is a new record and once it has been
persisted.

While the user is a new record the identity will have no user_id.
Old identities that are left to block the login of a user who
canceled their account also have a blank user_id. They still should
render the new identity invalid so the user can't be saved with a
login that has been reserved.

Once the user has been persisted we set the user_id on the identity
and save it too when creating an Account. This allows us to create
a plain user and save it and it will still have an in memory identity
only. But the default is to create the user by means of creating an
account so an identity will be created as well.
---
 app/models/account.rb  |  9 ++++++---
 app/models/identity.rb | 13 +++++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/app/models/account.rb b/app/models/account.rb
index bffa288..32ed445 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -16,10 +16,13 @@ class Account
 
   # Returns the user record so it can be used in views.
   def self.create(attrs)
-    @user = User.create(attrs).tap do |user|
-      Identity.create_for user
-      user.refresh_identity
+    @user = User.create(attrs)
+    if @user.persisted?
+      identity = @user.identity
+      identity.user_id = @user.id
+      identity.save
     end
+    return @user
   end
 
   def update(attrs)
diff --git a/app/models/identity.rb b/app/models/identity.rb
index a8eaba6..25be971 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -129,8 +129,12 @@ class Identity < CouchRest::Model::Base
   protected
 
   def alias_available
-    same_address = Identity.by_address.key(address)
-    if same_address.detect { |other| other != self && other.user != self.user }
+    blocking_identities = Identity.by_address.key(address).all
+    blocking_identities.delete self
+    if self.user
+      blocking_identities.reject! { |other| other.user == self.user }
+    end
+    if blocking_identities.any?
       errors.add :address, :taken
     end
   end
@@ -138,13 +142,14 @@ class Identity < CouchRest::Model::Base
   def address_local_email
     return if address.valid? #this ensures it is a valid local email address
     # we only hand on the first error for now.
-    self.errors.add(:address, address.errors.messages[:email].first)
+    self.errors.add(:address, address.errors.messages.values.first)
   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
+    # we only hand on the first error for now.
+    self.errors.add(:destination, destination.errors.messages.values.first)
   end
 
 end
-- 
cgit v1.2.3


From e0d31118d6e4110d2c280afa9415cfe9def29deb Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 29 May 2014 10:04:07 +0200
Subject: hand on errors from Email to Identity to User

errors.each iterates through all errors for all attrbibutes nicely.
---
 app/models/identity.rb     | 10 ++++++----
 app/models/user.rb         |  6 +++---
 test/unit/identity_test.rb |  1 +
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/app/models/identity.rb b/app/models/identity.rb
index 25be971..f2727c8 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -141,15 +141,17 @@ class Identity < CouchRest::Model::Base
 
   def address_local_email
     return if address.valid? #this ensures it is a valid local email address
-    # we only hand on the first error for now.
-    self.errors.add(:address, address.errors.messages.values.first)
+    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
-    # we only hand on the first error for now.
-    self.errors.add(:destination, destination.errors.messages.values.first)
+    destination.errors.each do |attribute, error|
+      self.errors.add(:destination, error)
+    end
   end
 
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 33508b5..84a795e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -167,10 +167,10 @@ class User < CouchRest::Model::Base
   ##
 
   def identity_is_valid
-    refresh_identity
     return if identity.valid?
-    # hand on the first error only for now
-    self.errors.add(:login, identity.errors.messages.values.first)
+    identity.errors.each do |attribute, error|
+      self.errors.add(:login, error)
+    end
   end
 
   def password
diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index 9c938f8..54c0657 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -107,6 +107,7 @@ class IdentityTest < ActiveSupport::TestCase
     other_user = find_record :user
     taken = Identity.build_for other_user, address: id.address
     assert !taken.valid?
+    assert_equal ["has already been taken"], taken.errors[:address]
     Identity.destroy_all_disabled
   end
 
-- 
cgit v1.2.3


From 85e066920568c19b788b8789c4659092224bb517 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 29 May 2014 10:37:31 +0200
Subject: ensure User#reload returns self

---
 app/models/user.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 84a795e..f8b9ddc 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -43,8 +43,8 @@ class User < CouchRest::Model::Base
   end # end of design
 
   def reload
-    super
     @identity = nil
+    super
   end
 
   def to_json(options={})
-- 
cgit v1.2.3


From bbe7b3b7deb2b44d34f7c39dda2c3db284e2bf10 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 29 May 2014 11:19:21 +0200
Subject: clearify identity validations

Identity.new.valid? should not crash. So validate presence where needed and
skip the other validations if the value is absent.
---
 app/models/identity.rb                 | 23 ++++++++++++++++-------
 test/integration/api/smtp_cert_test.rb |  2 +-
 test/unit/identity_test.rb             | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/app/models/identity.rb b/app/models/identity.rb
index f2727c8..2f6241c 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -10,7 +10,9 @@ class Identity < CouchRest::Model::Base
   property :keys, HashWithIndifferentAccess
   property :cert_fingerprints, Hash
 
-  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
@@ -94,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
@@ -123,12 +129,12 @@ class Identity < CouchRest::Model::Base
 
   # for LoginFormatValidation
   def login
-    self.address.handle
+    address.handle if address.present?
   end
 
   protected
 
-  def alias_available
+  def address_available
     blocking_identities = Identity.by_address.key(address).all
     blocking_identities.delete self
     if self.user
@@ -140,15 +146,18 @@ class Identity < CouchRest::Model::Base
   end
 
   def address_local_email
-    return if address.valid? #this ensures it is a valid local email address
+    # 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
+    # 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
diff --git a/test/integration/api/smtp_cert_test.rb b/test/integration/api/smtp_cert_test.rb
index 04e6f31..f72362d 100644
--- a/test/integration/api/smtp_cert_test.rb
+++ b/test/integration/api/smtp_cert_test.rb
@@ -34,7 +34,7 @@ class SmtpCertTest < ApiIntegrationTest
     cert = OpenSSL::X509::Certificate.new(get_response.body)
     fingerprint = OpenSSL::Digest::SHA1.hexdigest(cert.to_der).scan(/../).join(':')
     today = DateTime.now.to_date.to_s
-    assert_equal({fingerprint => today}, @user.identity.cert_fingerprints)
+    assert_equal({fingerprint => today}, @user.reload.identity.cert_fingerprints)
   end
 
   test "fetching smtp certs requires email account" do
diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index 54c0657..49b2075 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -7,6 +7,22 @@ class IdentityTest < ActiveSupport::TestCase
     @user = find_record :user
   end
 
+  test "blank identity does not crash on valid?" do
+    id = Identity.new
+    assert !id.valid?
+  end
+
+  test "enabled identity requires destination" do
+    id = Identity.new user: @user, address: @user.email_address
+    assert !id.valid?
+    assert_equal ["can't be blank"], id.errors[:destination]
+  end
+
+  test "disabled identity requires no destination" do
+    id = Identity.new address: @user.email_address
+    assert id.valid?
+  end
+
   test "initial identity for a user" do
     id = Identity.for(@user)
     assert_equal @user.email_address, id.address
-- 
cgit v1.2.3