diff options
| -rw-r--r-- | app/controllers/users_controller.rb | 3 | ||||
| -rw-r--r-- | app/models/account.rb | 22 | ||||
| -rw-r--r-- | app/models/identity.rb | 104 | ||||
| -rw-r--r-- | app/models/user.rb | 14 | ||||
| -rw-r--r-- | test/functional/identities_controller_test.rb | 2 | ||||
| -rw-r--r-- | test/functional/users_controller_test.rb | 4 | ||||
| -rw-r--r-- | test/integration/browser/account_test.rb | 2 | ||||
| -rw-r--r-- | test/support/api_integration_test.rb | 2 | ||||
| -rw-r--r-- | test/support/browser_integration_test.rb | 2 | ||||
| -rw-r--r-- | test/unit/account_test.rb | 7 | ||||
| -rw-r--r-- | test/unit/identity_test.rb | 29 |
11 files changed, 116 insertions, 75 deletions
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ec52cff..1404b0e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -53,8 +53,7 @@ class UsersController < ApplicationController end def enable - @user.enabled = true - @user.save + @user.account.enable flash[:notice] = I18n.t("actions.user_enabled_message", username: @user.username) redirect_to :back end diff --git a/app/models/account.rb b/app/models/account.rb index 46e5446..abde89d 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -59,11 +59,15 @@ class Account def destroy(destroy_identity=false) return unless @user - if !user.tmp? + if !@user.tmp? if destroy_identity == false - Identity.disable_all_for(@user) + @user.identities.each do |id| + id.orphan! + end else - Identity.destroy_all_for(@user) + @user.identities.each do |id| + id.destroy + end end end @user.destroy @@ -76,7 +80,17 @@ class Account if @user && !@user.tmp? @user.enabled = false @user.save - Identity.remove_cert_fingerprints_for(@user) + @user.identities.each do |id| + id.disable! + end + end + end + + def enable + @user.enabled = true + @user.save + @user.identities.each do |id| + id.enable! end end diff --git a/app/models/identity.rb b/app/models/identity.rb index e4162c8..f987e4e 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -1,9 +1,13 @@ # -# NOTE: there is some confusing terminology between User and Identity: -# If a user is disabled, the user still exists but has been marked as disabled -# and this condition can be easily reversed. If an identity is disabled, then -# it loses any association with the user and exists only to reserve that username -# and prevent anyone else from registering it. +# Identity states: +# +# DISABLED -- An identity is disabled if and only if its associated user +# is also disabled. In the disabled state, incoming email +# should bounce and outgoing email should not be relayed. +# +# ORPHANED -- An identity is orphaned if it has lost its association +# with a user account. This is in order to keep the name +# reserved to prevent anyone else from using it. # class Identity < CouchRest::Model::Base @@ -17,10 +21,12 @@ class Identity < CouchRest::Model::Base property :destination, Email property :keys, HashWithIndifferentAccess property :cert_fingerprints, Hash + property :disabled_cert_fingerprints, Hash + property :enabled, TrueClass, :default => true validates :address, presence: true validate :address_available - validates :destination, presence: true, if: :enabled? + validates :destination, presence: true, if: :user_id validates :destination, uniqueness: {scope: :address} validate :address_local_email validate :destination_email @@ -58,33 +64,33 @@ class Identity < CouchRest::Model::Base identity end - def self.disable_all_for(user) - Identity.by_user_id.key(user.id).each do |identity| - identity.disable - # if the identity is not unique anymore because the destination - # was reset to nil we destroy it. - identity.save || identity.destroy - end + # currently leap_mx ignores enabled property, so we + # also disable the fingerprints instead of just marking + # identity as disabled. + + def disable! + self.disabled_cert_fingerprints = self.cert_fingerprints + self.cert_fingerprints = {} + self.write_attribute(:enabled, false) + self.save end - # if an identity is disabled, it loses contact - # with its former user. but sometimes we want to keep the association - # and remove the fingerprints that allow the user to send email. - def self.remove_cert_fingerprints_for(user) - Identity.by_user_id.key(user.id).each do |identity| - identity.write_attribute(:cert_fingerprints, {}) - identity.save - end + def enable! + self.cert_fingerprints = self.disabled_cert_fingerprints + self.disabled_cert_fingerprints = nil + self.write_attribute(:enabled, true) + self.save end - def self.destroy_all_for(user) - Identity.by_user_id.key(user.id).each do |identity| - identity.destroy - end + # removes the association between this identity and the user. + def orphan! + self.destination = nil + self.user_id = nil + self.disable! end - def self.destroy_all_disabled - Identity.disabled.each do |identity| + def self.destroy_all_orphaned + Identity.orphaned.each do |identity| identity.destroy end end @@ -97,38 +103,28 @@ class Identity < CouchRest::Model::Base end def status - return :blocked if disabled? - case destination - when address - :main_email - when /@#{APP_CONFIG[:domain]}\Z/i, - :alias + if !enabled? || orphaned? + return :blocked else - :forward + case destination + when address + :main_email + when /@#{APP_CONFIG[:domain]}\Z/i, + :alias + else + :forward + end end end - def enabled? - self.user_id - end - - def disabled? - !enabled? - end - def actions - if enabled? + if !orphaned? [] # [:show, :edit] else [:destroy] end end - def disable - self.destination = nil - self.user_id = nil - end - def keys read_attribute('keys') || HashWithIndifferentAccess.new end @@ -153,6 +149,18 @@ class Identity < CouchRest::Model::Base address.handle if address.present? end + def orphaned? + self.user_id.nil? + end + + def self.orphaned + # the "disabled" view is a misnomer. it returns + # identities that have been orphaned, not identities that + # have been disabled. + # TODO: fix the view name + Identity.disabled + end + protected def address_available diff --git a/app/models/user.rb b/app/models/user.rb index 4bb1e79..61793be 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -136,14 +136,27 @@ class User < CouchRest::Model::Base Account.new(self) end + # deprecated def identity @identity ||= Identity.for(self) end + # deprecated def refresh_identity @identity = Identity.for(self) end + def identities + Identity.by_user_id.key(self.id) + end + + def destroy_identities + identities.each do |id| + id.destroy + end + end + + def desired_service_level code = self.desired_service_level_code || APP_CONFIG[:default_service_level] ServiceLevel.new({id: code}) @@ -154,7 +167,6 @@ class User < CouchRest::Model::Base ServiceLevel.new({id: code}) end - def self.send_one_month_warnings # To determine warnings to send, need to get all users where one_month_warning_sent is not set, and where it was created greater than or equal to 1 month ago. diff --git a/test/functional/identities_controller_test.rb b/test/functional/identities_controller_test.rb index e491c52..5af2e88 100644 --- a/test/functional/identities_controller_test.rb +++ b/test/functional/identities_controller_test.rb @@ -1,4 +1,4 @@ -require 'test_helper' +require_relative '../test_helper' class IdentitiesControllerTest < ActionController::TestCase diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 261f201..7b24098 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -86,7 +86,7 @@ class UsersControllerTest < ActionController::TestCase # we destroy the user record and the associated data... user.expects(:destroy) - Identity.expects(:disable_all_for).with(user) + #user.identity.expects(:orphan!) << factory girl user doesn't have identities Ticket.expects(:destroy_all_from).with(user) login :is_admin? => true @@ -101,7 +101,7 @@ class UsersControllerTest < ActionController::TestCase # we destroy the user record and the associated data... user.expects(:destroy) - Identity.expects(:disable_all_for).with(user) + #user.identity.expects(:orphan!) << factory girl user doesn't have identities Ticket.expects(:destroy_all_from).with(user) login user diff --git a/test/integration/browser/account_test.rb b/test/integration/browser/account_test.rb index cbe7ba9..50adb23 100644 --- a/test/integration/browser/account_test.rb +++ b/test/integration/browser/account_test.rb @@ -3,7 +3,7 @@ require 'test_helper' class AccountTest < BrowserIntegrationTest teardown do - Identity.destroy_all_disabled + Identity.destroy_all_orphaned end test "signup successfully when invited" do diff --git a/test/support/api_integration_test.rb b/test/support/api_integration_test.rb index 4077920..3b3481b 100644 --- a/test/support/api_integration_test.rb +++ b/test/support/api_integration_test.rb @@ -21,7 +21,7 @@ class ApiIntegrationTest < ActionDispatch::IntegrationTest teardown do if @user && @user.persisted? - Identity.destroy_all_for @user + @user.destroy_identities @user.reload.destroy end if @token && @token.persisted? diff --git a/test/support/browser_integration_test.rb b/test/support/browser_integration_test.rb index 950a395..1deb8fa 100644 --- a/test/support/browser_integration_test.rb +++ b/test/support/browser_integration_test.rb @@ -86,7 +86,7 @@ class BrowserIntegrationTest < ActionDispatch::IntegrationTest teardown do if @user && @user.reload - Identity.destroy_all_for @user + @user.destroy_identities @user.destroy end end diff --git a/test/unit/account_test.rb b/test/unit/account_test.rb index 7c26d5c..9680b33 100644 --- a/test/unit/account_test.rb +++ b/test/unit/account_test.rb @@ -8,7 +8,7 @@ class AccountTest < ActiveSupport::TestCase end teardown do - Identity.destroy_all_disabled + Identity.destroy_all_orphaned end test "create a new account when invited" do @@ -86,8 +86,11 @@ class AccountTest < ActiveSupport::TestCase cert = stub(expiry: 1.month.from_now, fingerprint: SecureRandom.hex) user.identity.register_cert cert user.identity.save - assert_equal cert.fingerprint, Identity.for(user).cert_fingerprints.keys.first + assert_equal(cert.fingerprint, Identity.for(user).cert_fingerprints.keys.first) user.account.disable assert_equal({}, Identity.for(user).cert_fingerprints) + assert_equal(cert.fingerprint, Identity.for(user).read_attribute(:disabled_cert_fingerprints).keys.first) + user.account.enable + assert_equal(cert.fingerprint, Identity.for(user).cert_fingerprints.keys.first) end end diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb index 1f93109..9d4bc90 100644 --- a/test/unit/identity_test.rb +++ b/test/unit/identity_test.rb @@ -110,33 +110,38 @@ class IdentityTest < ActiveSupport::TestCase assert @id.errors.messages[:destination].include? "needs to be a valid email address" end - test "disabled identity" do + test "disable identity" do @id = Identity.for(@user) - @id.disable + @id.disable! + assert !@id.enabled? + assert @id.valid? + end + + test "orphan identity" do + @id = Identity.for(@user) + @id.orphan! assert_equal @user.email_address, @id.address assert_equal nil, @id.destination assert_equal nil, @id.user - assert !@id.enabled? + assert @id.orphaned? assert @id.valid? end - test "disabled identity blocks handle" do + test "orphaned identity blocks handle" do @id = Identity.for(@user) - @id.disable - @id.save + @id.orphan! 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] end - test "destroy all disabled identities" do + test "destroy all orphaned identities" do @id = Identity.for(@user) - @id.disable - @id.save - assert Identity.disabled.count > 0 - Identity.destroy_all_disabled - assert_equal 0, Identity.disabled.count + @id.orphan! + assert Identity.orphaned.count > 0 + Identity.destroy_all_orphaned + assert_equal 0, Identity.orphaned.count end test "store cert fingerprint" do |