allow user accounts to be re-enabled, and for associated identities to also get re-enabled.

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