Merge branch 'bugfix/8807-cleanup-tmp-invites' into 'master'

Bugfix/8807 cleanup tmp invites

Closes #8807

See merge request leap/webapp!49

2 files changed, 56 insertions, 37 deletions

diff --git a/app/models/account.rb b/app/models/account.rb
index 4442a68..d77c61f 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -25,61 +25,57 @@ class Account
   #                      configuration by same name.
   #
   def self.create(attrs, options={})
+    User.new(attrs).tap do |user|
+      self.new(user).create(options)
+    end
+  end
+
+  def create(options={})
     identity = nil
-    user = nil
-    user = User.new(attrs)
     if options[:invite_required] == false
       user.ignore_invites!
     end
     user.save
 
     # this is not very atomic, but we do the best we can:
-    if !user.is_tmp? && user.persisted?
+    return unless user.persisted?
+    if !user.is_tmp?
       identity = user.identity
       identity.user_id = user.id
       identity.save
       identity.errors.each do |attr, msg|
         user.errors.add(attr, msg)
       end
-      if user.invite_required?
-        user_invite_code = InviteCode.find_by_invite_code user.invite_code
-        if user.is_test? && user_invite_code.max_uses == 1
-          user_invite_code.destroy
-        else
-          user_invite_code.invite_count += 1
-          user_invite_code.save
-        end
-      end
     end
+    consume_invite_code if user.invite_required?
   rescue VALIDATION_FAILED => ex
-    user.errors.add(:base, ex.to_s) if user
+    user.errors.add(:base, ex.to_s)
   ensure
-    if creation_problem?(user, identity)
+    if creation_problem?(identity)
       user.destroy     if user     && user.persisted?
       identity.destroy if identity && identity.persisted?
     end
-    return user
   end
 
   def update(attrs)
     if attrs[:password_verifier].present?
       update_login(attrs[:login])
-      @user.update_attributes attrs.slice(:password_verifier, :password_salt)
+      user.update_attributes attrs.slice(:password_verifier, :password_salt)
     end
     if attrs[:recovery_code_verifier].present?
-      @user.update_attributes attrs.slice(:recovery_code_verifier, :recovery_code_salt)
+      user.update_attributes attrs.slice(:recovery_code_verifier, :recovery_code_salt)
     end
     # TODO: move into identity controller
     key = update_pgp_key(attrs[:public_key])
-    @user.errors.set :public_key, key.errors.full_messages
-    @user.save && save_identities
-    @user.refresh_identity
+    user.errors.set :public_key, key.errors.full_messages
+    user.save && save_identities
+    user.refresh_identity
   end
 
   def destroy(release_handles=false)
-    return unless @user
-    if !@user.is_tmp?
-      @user.identities.each do |id|
+    return unless user
+    if !user.is_tmp?
+      user.identities.each do |id|
         if release_handles == false
           id.orphan!
         else
@@ -87,44 +83,56 @@ class Account
         end
       end
     end
-    @user.destroy
+    user.destroy
   end
 
   # when a user is disable, all their data and associations remain
   # in place, but the user should not be able to send email or
   # create new authentication certificates.
   def disable
-    if @user && !@user.is_tmp?
-      @user.enabled = false
-      @user.save
-      @user.identities.each do |id|
+    if user && !user.is_tmp?
+      user.enabled = false
+      user.save
+      user.identities.each do |id|
         id.disable!
       end
     end
   end
 
   def enable
-    @user.enabled = true
-    @user.save
-    @user.identities.each do |id|
+    user.enabled = true
+    user.save
+    user.identities.each do |id|
       id.enable!
     end
   end
 
   protected
 
+  attr_reader :user
+
+  def consume_invite_code
+    invite_code = InviteCode.find_by_invite_code user.invite_code
+    if user.is_test? && invite_code.max_uses == 1
+      invite_code.destroy
+    else
+      invite_code.invite_count += 1
+      invite_code.save
+    end
+  end
+
   def update_login(login)
     return unless login.present?
-    @old_identity = Identity.for(@user)
-    @user.login = login
-    @new_identity = Identity.for(@user) # based on the new login
-    @old_identity.destination = @user.email_address # alias old -> new
+    @old_identity = Identity.for(user)
+    user.login = login
+    @new_identity = Identity.for(user) # based on the new login
+    @old_identity.destination = user.email_address # alias old -> new
   end
 
   def update_pgp_key(key)
     PgpKey.new(key).tap do |key|
       if key.present? && key.valid?
-        @new_identity ||= Identity.for(@user)
+        @new_identity ||= Identity.for(user)
         @new_identity.set_key(:pgp, key)
       end
     end
@@ -134,7 +142,7 @@ class Account
     @new_identity.try(:save) && @old_identity.try(:save)
   end
 
-  def self.creation_problem?(user, identity)
+  def creation_problem?(identity)
     return true if user.nil? || !user.persisted? || user.errors.any?
     if !user.is_tmp?
       return true if identity.nil? || !identity.persisted? || identity.errors.any?
diff --git a/test/unit/account_test.rb b/test/unit/account_test.rb
index f81764d..61b40b5 100644
--- a/test/unit/account_test.rb
+++ b/test/unit/account_test.rb
@@ -153,6 +153,17 @@ class AccountTest < ActiveSupport::TestCase
     end
   end
 
+  test "Single use invite code is destroyed when used by tmp user" do
+    with_config invite_required: true do
+      attrs = user_attributes invite_code: @testcode.invite_code
+      attrs[:login] = 'tmp_user_' + attrs[:login]
+      user = Account.create(attrs)
+      user.save
+      assert user.persisted?, user.errors.inspect
+      assert_nil InviteCode.find_by_invite_code user.invite_code
+    end
+  end
+
   test "Invite code stays zero when invite code is not used" do
     #user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
     invalid_user = FactoryGirl.build(:user, :invite_code => @testcode.invite_code)