diff --git a/app/models/account.rb b/app/models/account.rb index cf998e46565be2b1f3ffb8ebb8e883b90f18c178..32ed445cb2c33f73e53f872b0e8150828a12853c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -16,9 +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 = 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 a4225e749d07393420e83efe0f316e6e1306cc0a..2f6241c099d3a7294efc1f252dae8e840bd551fb 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 diff --git a/app/models/local_email.rb b/app/models/local_email.rb index 2b4c65ee3630d488c8a18b71a0910590a8bc61b4..ded7baf722ab1836ec91611aa38492ec11064834 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 diff --git a/app/models/pgp_key.rb b/app/models/pgp_key.rb index 66f866045ef7f5e7a52d4e6108b33eabc176618a..3384f4cf56129f07fe69cb41ffc0471720f10868 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 6678de6bafc322793fe151eb422cb99c34e1387f..f8b9ddc3a4322d7f7e5ad59693b83a9350c75823 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" } @@ -42,6 +42,11 @@ class User < CouchRest::Model::Base view :by_created_at end # end of design + def reload + @identity = nil + super + end + def to_json(options={}) { :login => login, @@ -161,11 +166,10 @@ 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") + def identity_is_valid + return if identity.valid? + identity.errors.each do |attribute, error| + self.errors.add(:login, error) end end diff --git a/test/integration/api/smtp_cert_test.rb b/test/integration/api/smtp_cert_test.rb index 04e6f316f969304f185482b54f4bbc3a1dda12d1..f72362d3cb7408a704cadbb26a014298f4617eaf 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/integration/browser/account_test.rb b/test/integration/browser/account_test.rb index 491a9e13ae53547c0ec5dc05428b7d7fc92ca9ac..8e6d433502463deb616b15d820d3c3338add4990 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 @@ -102,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 diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb index eca104ffbd14e903d9a674fbcb6152be2d97908f..49b207506b08ff95791b4986f5bacd66526e0151 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 @@ -39,7 +55,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 +64,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 @@ -107,6 +123,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