Merge branch 'develop'

16 files changed, 209 insertions, 76 deletions

diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb
index f66277d..3cbb6dc 100644
--- a/users/app/controllers/users_controller.rb
+++ b/users/app/controllers/users_controller.rb
@@ -47,8 +47,16 @@ class UsersController < UsersBaseController
   end
 
   def destroy
-    @user.destroy
-    redirect_to admin? ? users_url : root_url
+    @user.account.destroy
+    flash[:notice] = I18n.t(:account_destroyed)
+    # admins can destroy other users
+    if @user != current_user
+      redirect_to users_url
+    else
+      # let's remove the invalid session
+      logout
+      redirect_to root_url
+    end
   end
 
 end
diff --git a/users/app/controllers/v1/users_controller.rb b/users/app/controllers/v1/users_controller.rb
index 03a5a62..0903888 100644
--- a/users/app/controllers/v1/users_controller.rb
+++ b/users/app/controllers/v1/users_controller.rb
@@ -24,15 +24,9 @@ module V1
     end
 
     def update
-      account.update params[:user]
+      @user.account.update params[:user]
       respond_with @user
     end
 
-    protected
-
-    def account
-      @user.account
-    end
-
   end
 end
diff --git a/users/app/models/account.rb b/users/app/models/account.rb
index 5368a1b..5c943bb 100644
--- a/users/app/models/account.rb
+++ b/users/app/models/account.rb
@@ -1,5 +1,10 @@
 #
-# A Composition of a User record and it's identity records.
+# The Account model takes care of the livecycle of a user.
+# It composes a User record and it's identity records.
+# It also allows for other engines to hook into the livecycle by
+# monkeypatching the create, update and destroy methods.
+# There's an ActiveSupport load_hook at the end of this file to
+# make this more easy.
 #
 class Account
 
@@ -29,9 +34,7 @@ class Account
 
   def destroy
     return unless @user
-    Identity.by_user_id.key(@user.id).each do |identity|
-      identity.destroy
-    end
+    Identity.disable_all_for(@user)
     @user.destroy
   end
 
@@ -54,4 +57,7 @@ class Account
     @new_identity.try(:save) && @old_identity.try(:save)
   end
 
+  # You can hook into the account lifecycle from different engines using
+  #   ActiveSupport.on_load(:account) do ...
+  ActiveSupport.run_load_hooks(:account, self)
 end
diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb
index e0a24e9..97966d0 100644
--- a/users/app/models/identity.rb
+++ b/users/app/models/identity.rb
@@ -27,6 +27,17 @@ class Identity < CouchRest::Model::Base
         emit(doc.address, doc.keys["pgp"]);
       }
     EOJS
+    view :disabled,
+      map: <<-EOJS
+      function(doc) {
+        if (doc.type != 'Identity') {
+          return;
+        }
+        if (typeof doc.user_id === "undefined") {
+          emit(doc._id, 1);
+        }
+      }
+    EOJS
 
   end
 
@@ -50,6 +61,19 @@ 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
+      identity.save
+    end
+  end
+
+  def self.destroy_all_disabled
+    Identity.disabled.each do |identity|
+      identity.destroy
+    end
+  end
+
   def self.attributes_from_user(user)
     { user_id: user.id,
       address: user.email_address,
@@ -57,6 +81,15 @@ class Identity < CouchRest::Model::Base
     }
   end
 
+  def enabled?
+    self.destination && self.user_id
+  end
+
+  def disable
+    self.destination = nil
+    self.user_id = nil
+  end
+
   def keys
     read_attribute('keys') || HashWithIndifferentAccess.new
   end
@@ -93,7 +126,8 @@ class Identity < CouchRest::Model::Base
   end
 
   def destination_email
-    return if destination.valid? #this ensures it is 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
   end
 
diff --git a/users/app/models/token.rb b/users/app/models/token.rb
index dd87344..001eb40 100644
--- a/users/app/models/token.rb
+++ b/users/app/models/token.rb
@@ -11,6 +11,25 @@ class Token < CouchRest::Model::Base
 
   validates :user_id, presence: true
 
+  design do
+    view :by_last_seen_at
+  end
+
+  def self.expires_after
+    APP_CONFIG[:auth] && APP_CONFIG[:auth][:token_expires_after]
+  end
+
+  def self.expired
+    return [] unless expires_after
+    by_last_seen_at.endkey(expires_after.minutes.ago)
+  end
+
+  def self.destroy_all_expired
+    self.expired.each do |token|
+      token.destroy
+    end
+  end
+
   def authenticate
     if expired?
       destroy
@@ -27,21 +46,16 @@ class Token < CouchRest::Model::Base
   end
 
   def expired?
-    expires_after and
-    last_seen_at + expires_after.minutes < Time.now
-  end
-
-  def expires_after
-    APP_CONFIG[:auth] && APP_CONFIG[:auth][:token_expires_after]
+    Token.expires_after and
+    last_seen_at < Token.expires_after.minutes.ago
   end
 
   def initialize(*args)
     super
-    self.id = SecureRandom.urlsafe_base64(32).gsub(/^_*/, '')
-    self.last_seen_at = Time.now
-  end
-
-  design do
+    if new_record?
+      self.id = SecureRandom.urlsafe_base64(32).gsub(/^_*/, '')
+      self.last_seen_at = Time.now
+    end
   end
 end
 
diff --git a/users/config/locales/en.yml b/users/config/locales/en.yml
index b69f7f4..1b5dd5e 100644
--- a/users/config/locales/en.yml
+++ b/users/config/locales/en.yml
@@ -17,6 +17,7 @@ en:
   destroy_my_account: "Destroy my account"
   destroy_account_info: "This will permanently destroy your account and all the data associated with it. Proceed with caution!"
   admin_destroy_account: "Destroy the account %{username}"
+  account_destroyed: "The account has been destroyed successfully."
   set_email_address: "Set email address"
   forward_email: "Forward Email"
   email_aliases: "Email Aliases"
diff --git a/users/test/factories.rb b/users/test/factories.rb
index c87e290..f5fb77d 100644
--- a/users/test/factories.rb
+++ b/users/test/factories.rb
@@ -19,6 +19,8 @@ FactoryGirl.define do
     end
   end
 
-  factory :token
+  factory :token do
+    user
+  end
 
 end
diff --git a/users/test/functional/sessions_controller_test.rb b/users/test/functional/sessions_controller_test.rb
index a630e6e..28143da 100644
--- a/users/test/functional/sessions_controller_test.rb
+++ b/users/test/functional/sessions_controller_test.rb
@@ -41,20 +41,12 @@ class SessionsControllerTest < ActionController::TestCase
     assert_json_error :login => I18n.t(:all_strategies_failed)
   end
 
-  test "logout should reset warden user" do
-    expect_warden_logout
+  test "destory should logout" do
+    login
+    expect_logout
     delete :destroy
     assert_response :redirect
     assert_redirected_to root_url
   end
 
-  def expect_warden_logout
-    raw = mock('raw session') do
-      expects(:inspect)
-    end
-    request.env['warden'].expects(:raw_session).returns(raw)
-    request.env['warden'].expects(:logout)
-  end
-
-
 end
diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb
index 052de04..9c5f8d9 100644
--- a/users/test/functional/users_controller_test.rb
+++ b/users/test/functional/users_controller_test.rb
@@ -77,7 +77,11 @@ class UsersControllerTest < ActionController::TestCase
 
   test "admin can destroy user" do
     user = find_record :user
+
+    # we destroy the user record and the associated data...
     user.expects(:destroy)
+    Identity.expects(:disable_all_for).with(user)
+    Ticket.expects(:destroy_all_from).with(user)
 
     login :is_admin? => true
     delete :destroy, :id => user.id
@@ -88,9 +92,14 @@ class UsersControllerTest < ActionController::TestCase
 
   test "user can cancel account" do
     user = find_record :user
+
+    # we destroy the user record and the associated data...
     user.expects(:destroy)
+    Identity.expects(:disable_all_for).with(user)
+    Ticket.expects(:destroy_all_from).with(user)
 
     login user
+    expect_logout
     delete :destroy, :id => @current_user.id
 
     assert_response :redirect
diff --git a/users/test/functional/v1/sessions_controller_test.rb b/users/test/functional/v1/sessions_controller_test.rb
index ff9fca1..4200e8f 100644
--- a/users/test/functional/v1/sessions_controller_test.rb
+++ b/users/test/functional/v1/sessions_controller_test.rb
@@ -52,26 +52,11 @@ class V1::SessionsControllerTest < ActionController::TestCase
     assert_equal @user.id, token.user_id
   end
 
-  test "logout should reset session" do
-    expect_warden_logout
-    delete :destroy
-    assert_response 204
-  end
-
-  test "logout should destroy token" do
+  test "destroy should logout" do
     login
-    expect_warden_logout
-    @token.expects(:destroy)
+    expect_logout
     delete :destroy
     assert_response 204
   end
 
-  def expect_warden_logout
-    raw = mock('raw session') do
-      expects(:inspect)
-    end
-    request.env['warden'].expects(:raw_session).returns(raw)
-    request.env['warden'].expects(:logout)
-  end
-
 end
diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb
index 8e03856..b349489 100644
--- a/users/test/integration/browser/account_test.rb
+++ b/users/test/integration/browser/account_test.rb
@@ -6,6 +6,10 @@ class AccountTest < BrowserIntegrationTest
     Capybara.current_driver = Capybara.javascript_driver
   end
 
+  teardown do
+    Identity.destroy_all_disabled
+  end
+
   test "normal account workflow" do
     username, password = submit_signup
     assert page.has_content?("Welcome #{username}")
@@ -19,23 +23,32 @@ class AccountTest < BrowserIntegrationTest
   test "successful login" do
     username, password = submit_signup
     click_on 'Logout'
-    click_on 'Log In'
-    fill_in 'Username', with: username
-    fill_in 'Password', with: password
-    click_on 'Log In'
+    attempt_login(username, password)
     assert page.has_content?("Welcome #{username}")
     User.find_by_login(username).account.destroy
   end
 
   test "failed login" do
     visit '/'
-    click_on 'Log In'
-    fill_in 'Username', with: "username"
-    fill_in 'Password', with: "wrong password"
-    click_on 'Log In'
-    assert page.has_selector? 'input.btn-primary.disabled'
-    assert page.has_content? I18n.t(:invalid_user_pass)
-    assert page.has_no_selector? 'input.btn-primary.disabled'
+    attempt_login("username", "wrong password")
+    assert_invalid_login(page)
+  end
+
+  test "account destruction" do
+    username, password = submit_signup
+    click_on I18n.t('account_settings')
+    click_on I18n.t('destroy_my_account')
+    assert page.has_content?(I18n.t('account_destroyed'))
+    attempt_login(username, password)
+    assert_invalid_login(page)
+  end
+
+  test "handle blocked after account destruction" do
+    username, password = submit_signup
+    click_on I18n.t('account_settings')
+    click_on I18n.t('destroy_my_account')
+    submit_signup(username)
+    assert page.has_content?('has already been taken')
   end
 
   test "change password" do
@@ -47,10 +60,7 @@ class AccountTest < BrowserIntegrationTest
       click_on 'Save'
     end
     click_on 'Logout'
-    click_on 'Log In'
-    fill_in 'Username', with: username
-    fill_in 'Password', with: "other password"
-    click_on 'Log In'
+    attempt_login(username, "other password")
     assert page.has_content?("Welcome #{username}")
     User.find_by_login(username).account.destroy
   end
@@ -92,6 +102,19 @@ class AccountTest < BrowserIntegrationTest
     assert page.has_content?("server failed")
   end
 
+  def attempt_login(username, password)
+    click_on 'Log In'
+    fill_in 'Username', with: username
+    fill_in 'Password', with: password
+    click_on 'Log In'
+  end
+
+  def assert_invalid_login(page)
+    assert page.has_selector? 'input.btn-primary.disabled'
+    assert page.has_content? I18n.t(:invalid_user_pass)
+    assert page.has_no_selector? 'input.btn-primary.disabled'
+  end
+
   def inject_malicious_js
     page.execute_script <<-EOJS
       var calc = new srp.Calculate();
diff --git a/users/test/support/auth_test_helper.rb b/users/test/support/auth_test_helper.rb
index 609f115..50e9453 100644
--- a/users/test/support/auth_test_helper.rb
+++ b/users/test/support/auth_test_helper.rb
@@ -38,12 +38,26 @@ module AuthTestHelper
     end
   end
 
+  def expect_logout
+    expect_warden_logout
+    @token.expects(:destroy) if @token
+  end
+
   protected
 
   def header_for_token_auth
     @token = find_record(:token, :authenticate => @current_user)
     ActionController::HttpAuthentication::Token.encode_credentials @token.id
   end
+
+  def expect_warden_logout
+    raw = mock('raw session') do
+      expects(:inspect)
+    end
+    request.env['warden'].expects(:raw_session).returns(raw)
+    request.env['warden'].expects(:logout)
+  end
+
 end
 
 class ActionController::TestCase
diff --git a/users/test/support/integration_test_helper.rb b/users/test/support/integration_test_helper.rb
index cfe72cf..51e47c6 100644
--- a/users/test/support/integration_test_helper.rb
+++ b/users/test/support/integration_test_helper.rb
@@ -1,7 +1,7 @@
 module IntegrationTestHelper
-  def submit_signup
-    username = "test_#{SecureRandom.urlsafe_base64}".downcase
-    password = SecureRandom.base64
+  def submit_signup(username = nil, password = nil)
+    username ||= "test_#{SecureRandom.urlsafe_base64}".downcase
+    password ||= SecureRandom.base64
     visit '/users/new'
     fill_in 'Username', with: username
     fill_in 'Password', with: password
diff --git a/users/test/unit/account_test.rb b/users/test/unit/account_test.rb
index 94a9980..4fb3c3d 100644
--- a/users/test/unit/account_test.rb
+++ b/users/test/unit/account_test.rb
@@ -2,6 +2,10 @@ require 'test_helper'
 
 class AccountTest < ActiveSupport::TestCase
 
+  teardown do
+    Identity.destroy_all_disabled
+  end
+
   test "create a new account" do
     user = Account.create(FactoryGirl.attributes_for(:user))
     assert user.valid?
@@ -13,7 +17,8 @@ class AccountTest < ActiveSupport::TestCase
   end
 
   test "create and remove a user account" do
-    assert_no_difference "Identity.count" do
+    # We keep an identity that will block the handle from being reused.
+    assert_difference "Identity.count" do
       assert_no_difference "User.count" do
         user = Account.create(FactoryGirl.attributes_for(:user))
         user.account.destroy
diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb
index 0842a77..eca104f 100644
--- a/users/test/unit/identity_test.rb
+++ b/users/test/unit/identity_test.rb
@@ -90,6 +90,35 @@ class IdentityTest < ActiveSupport::TestCase
     assert id.errors.messages[:destination].include? "needs to be a valid email address"
   end
 
+  test "disabled identity" do
+    id = Identity.for(@user)
+    id.disable
+    assert_equal @user.email_address, id.address
+    assert_equal nil, id.destination
+    assert_equal nil, id.user
+    assert !id.enabled?
+    assert id.valid?
+  end
+
+  test "disabled identity blocks handle" do
+    id = Identity.for(@user)
+    id.disable
+    id.save
+    other_user = find_record :user
+    taken = Identity.build_for other_user, address: id.address
+    assert !taken.valid?
+    Identity.destroy_all_disabled
+  end
+
+  test "destroy all disabled identities" do
+    id = Identity.for(@user)
+    id.disable
+    id.save
+    assert Identity.count > 0
+    Identity.destroy_all_disabled
+    assert_equal 0, Identity.disabled.count
+  end
+
   def alias_name
     @alias_name ||= Faker::Internet.user_name
   end
diff --git a/users/test/unit/token_test.rb b/users/test/unit/token_test.rb
index f56c576..6c9f209 100644
--- a/users/test/unit/token_test.rb
+++ b/users/test/unit/token_test.rb
@@ -7,9 +7,6 @@ class ClientCertificateTest < ActiveSupport::TestCase
     @user = find_record :user
   end
 
-  teardown do
-  end
-
   test "new token for user" do
     sample = Token.new(:user_id => @user.id)
     assert sample.valid?
@@ -61,6 +58,26 @@ class ClientCertificateTest < ActiveSupport::TestCase
     end
   end
 
+  test "Token.destroy_all_expired is noop if no expiry is set" do
+    expired = FactoryGirl.create :token, last_seen_at: 2.hours.ago
+    with_config auth: {} do
+      Token.destroy_all_expired
+    end
+    assert_equal expired, Token.find(expired.id)
+  end
+
+  test "Token.destroy_all_expired cleans up expired tokens only" do
+    expired = FactoryGirl.create :token, last_seen_at: 2.hours.ago
+    fresh = FactoryGirl.create :token
+    with_config auth: {token_expires_after: 60} do
+      Token.destroy_all_expired
+    end
+    assert_nil Token.find(expired.id)
+    assert_equal fresh, Token.find(fresh.id)
+    fresh.destroy
+  end
+
+
 
 
 end