Merge branch 'fix/8798-key-errors' into 'master'

Fix/8798 key errors

Closes #8798

See merge request leap/webapp!54

4 files changed, 113 insertions, 113 deletions

diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb
index cb7b7bc..65b80c7 100644
--- a/app/controllers/api/users_controller.rb
+++ b/app/controllers/api/users_controller.rb
@@ -34,14 +34,6 @@ module Api
       end
     end
 
-    def user_response
-      @user.to_hash.tap do |user_hash|
-        if @user == current_user
-          user_hash['is_admin'] = @user.is_admin?
-        end
-      end
-    end
-
     def create
       if current_user.is_monitor?
         create_test_account
@@ -53,8 +45,14 @@ module Api
     end
 
     def update
-      @user.account.update user_update_params
-      respond_with @user
+      if user_update_params.present?
+        @user.account.update user_update_params
+        respond_with @user
+      else
+        # TODO: move into identity controller
+        key = update_pgp_key(user_key_param[:public_key])
+        respond_with key
+      end
     end
 
     def destroy
@@ -67,13 +65,24 @@ module Api
 
     private
 
+    def user_response
+      @user.to_hash.tap do |user_hash|
+        if @user == current_user
+          user_hash['is_admin'] = @user.is_admin?
+        end
+      end
+    end
+
     def user_update_params
       params.require(:user).permit :login,
         :password_verifier,
         :password_salt,
         :recovery_code_verifier,
-        :recovery_code_salt,
-        :public_key
+        :recovery_code_salt
+    end
+
+    def user_key_param
+      params.require(:user).permit :public_key
     end
 
     def release_handles
@@ -99,5 +108,14 @@ module Api
       end
     end
 
+    def update_pgp_key(key)
+      PgpKey.new(key).tap do |key|
+        if key.valid?
+          identity = Identity.for(@user)
+          identity.set_key(:pgp, key)
+          identity.save
+        end
+      end
+    end
   end
 end
diff --git a/app/models/account.rb b/app/models/account.rb
index d77c61f..5a4111d 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -65,9 +65,6 @@ class Account
     if attrs[:recovery_code_verifier].present?
       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
   end
@@ -129,15 +126,6 @@ class Account
     @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.set_key(:pgp, key)
-      end
-    end
-  end
-
   def save_identities
     @new_identity.try(:save) && @old_identity.try(:save)
   end
diff --git a/test/integration/api/pgp_key_test.rb b/test/integration/api/pgp_key_test.rb
index 4c7fb4c..f2744e1 100644
--- a/test/integration/api/pgp_key_test.rb
+++ b/test/integration/api/pgp_key_test.rb
@@ -14,16 +14,16 @@ class PgpKeyTest < SrpTest
     assert_equal key, Identity.for(@user).keys[:pgp]
   end
 
-  # eventually probably want to remove most of this into a non-integration
-  # functional test
   test "prevent uploading invalid key" do
     update_user public_key: "invalid key"
+    assert_invalid_key_response
     assert_nil Identity.for(@user).keys[:pgp]
   end
 
   test "prevent emptying public key" do
     update_user public_key: key
     update_user public_key: ""
+    assert_invalid_key_response
     assert_equal key, Identity.for(@user).keys[:pgp]
   end
 
@@ -32,4 +32,9 @@ class PgpKeyTest < SrpTest
   def key
     @key ||= FactoryGirl.build :pgp_key
   end
+
+  def assert_invalid_key_response
+    assert_response :unprocessable_entity
+    assert_json_error "public_key_block"=>["does not look like an armored pgp public key block"]
+  end
 end
diff --git a/test/unit/account_test.rb b/test/unit/account_test.rb
index 61b40b5..ebfff6b 100644
--- a/test/unit/account_test.rb
+++ b/test/unit/account_test.rb
@@ -8,164 +8,153 @@ class AccountTest < ActiveSupport::TestCase
   end
 
   teardown do
+    @user.account.destroy if @user && @user.persisted?
     Identity.destroy_all_orphaned
   end
 
   test "create a new account when invited" do
-    user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
-    assert user.valid?, "unexpected errors: #{user.errors.inspect}"
-    assert user.persisted?
-    assert id = user.identity
-    assert_equal user.email_address, id.address
-    assert_equal user.email_address, id.destination
-    user.account.destroy
+    @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+    assert @user.valid?, "unexpected errors: #{@user.errors.inspect}"
+    assert @user.persisted?
+    assert id = @user.identity
+    assert_equal @user.email_address, id.address
+    assert_equal @user.email_address, id.destination
   end
 
   test "fail to create account without invite" do
     with_config invite_required: true do
-      user = Account.create(user_attributes)
-      assert !user.valid?, "user should not be valid"
-      assert !user.persisted?, "user should not have been saved"
-      assert_has_errors user, invite_code: "This is not a valid code"
+      @user = Account.create(user_attributes)
+      assert !@user.valid?, "user should not be valid"
+      assert !@user.persisted?, "user should not have been saved"
+      assert_has_errors @user, invite_code: "This is not a valid code"
     end
   end
 
   test "allow invite_required override" do
     with_config invite_required: true do
-      user = Account.create(user_attributes, :invite_required => false)
-      assert user.valid?, "unexpected errors: #{user.errors.inspect}"
-      assert user.persisted?, "user should have been saved"
-      user.account.destroy
+      @user = Account.create(user_attributes, :invite_required => false)
+      assert @user.valid?, "unexpected errors: #{@user.errors.inspect}"
+      assert @user.persisted?, "user should have been saved"
     end
   end
 
   test "create a new account" do
     with_config invite_required: false do
-      user = Account.create(user_attributes)
-      assert user.valid?, "unexpected errors: #{user.errors.inspect}"
-      assert user.persisted?
-      user.account.destroy
+      @user = Account.create(user_attributes)
+      assert @user.valid?, "unexpected errors: #{@user.errors.inspect}"
+      assert @user.persisted?
     end
   end
 
   test "error on reused username" do
     with_config invite_required: false do
       attributes = user_attributes
-      user = Account.create attributes
+      @user = Account.create attributes
       dup = Account.create attributes
       assert !dup.valid?
       assert_has_errors dup, login: "has already been taken"
-      user.account.destroy
     end
   end
 
   test "error on invalid username" do
     with_config invite_required: false do
       attributes = FactoryGirl.attributes_for :user, login: "a"
-      user = Account.create attributes
-      assert !user.valid?
-      assert_has_errors user, login: "Must have at least two characters"
+      @user = Account.create attributes
+      assert !@user.valid?
+      assert_has_errors @user, login: "Must have at least two characters"
     end
   end
 
-  test "create and remove a user account" do
+  test "create and remove a @user account" 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(user_attributes( :invite_code => @testcode.invite_code))
-        user.account.destroy
+        @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+        @user.account.destroy
       end
     end
   end
 
   test "change username and create alias" do
-    user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
-    old_id = user.identity
-    old_email = user.email_address
-    user.account.update(user_attributes)
-    user.reload
+    @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+    old_id = @user.identity
+    old_email = @user.email_address
+    @user.account.update(user_attributes)
+    @user.reload
     old_id.reload
-    assert user.valid?
-    assert user.persisted?
-    assert id = user.identity
+    assert @user.valid?
+    assert @user.persisted?
+    assert id = @user.identity
     assert id.persisted?
-    assert_equal user.email_address, id.address
-    assert_equal user.email_address, id.destination
-    assert_equal user.email_address, old_id.destination
+    assert_equal @user.email_address, id.address
+    assert_equal @user.email_address, id.destination
+    assert_equal @user.email_address, old_id.destination
     assert_equal old_email, old_id.address
-    user.account.destroy
   end
 
   test "create recovery code if it does not exist" do
-    user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
-    user.account.update(:recovery_code_verifier => "abc", :recovery_code_salt => "123")
-    user.reload
+    @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+    @user.account.update(:recovery_code_verifier => "abc", :recovery_code_salt => "123")
+    @user.reload
 
-    assert_equal "abc", user.recovery_code_verifier
-    assert_equal "123", user.recovery_code_salt
-
-    user.account.destroy
+    assert_equal "abc", @user.recovery_code_verifier
+    assert_equal "123", @user.recovery_code_salt
   end
 
   test "update recovery code that already exists" do
-    user = Account.create(user_attributes(
+    @user = Account.create(user_attributes(
       :invite_code => @testcode.invite_code,
       :recovery_code_verifier => "000",
       :recovery_code_salt => "111"))
 
-    user.account.update(:recovery_code_verifier => "abc", :recovery_code_salt => "123")
-    user.reload
-
-    assert_equal "abc", user.recovery_code_verifier
-    assert_equal "123", user.recovery_code_salt
+    @user.account.update(:recovery_code_verifier => "abc", :recovery_code_salt => "123")
+    @user.reload
 
-    user.account.destroy
+    assert_equal "abc", @user.recovery_code_verifier
+    assert_equal "123", @user.recovery_code_salt
   end
 
   test "update password" do
-    user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
-    user.account.update(:password_verifier => "551A8B", :password_salt => "551A8B")
-
-    assert_equal "551A8B", user.password_verifier
-    assert_equal "551A8B", user.password_salt
+    @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+    @user.account.update(:password_verifier => "551A8B", :password_salt => "551A8B")
 
-    user.account.destroy
+    assert_equal "551A8B", @user.password_verifier
+    assert_equal "551A8B", @user.password_salt
   end
 
   test "Invite code count goes up by 1 when the invite code is entered" do
     with_config invite_required: true do
-      user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
-      user_code = InviteCode.find_by_invite_code user.invite_code
-      user.save
-      assert user.persisted?
+      @user = Account.create(user_attributes( :invite_code => @testcode.invite_code))
+      user_code = InviteCode.find_by_invite_code @user.invite_code
+      @user.save
+      assert @user.persisted?
       assert_equal 1, user_code.invite_count
     end
   end
 
-  test "Single use invite code is destroyed when used by test user" do
+  test "Single use invite code is destroyed when used by test @user" do
     with_config invite_required: true do
       attrs = user_attributes invite_code: @testcode.invite_code
       attrs[:login] = 'test_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
+      @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 "Single use invite code is destroyed when used by tmp user" do
+  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
+      @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)
     invalid_user.save
     user_code = InviteCode.find_by_invite_code invalid_user.invite_code
@@ -175,16 +164,16 @@ class AccountTest < ActiveSupport::TestCase
   end
 
   test "disabled accounts have no cert fingerprints" do
-    user = Account.create(user_attributes)
+    @user = Account.create(user_attributes)
     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)
-    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)
+    @user.identity.register_cert cert
+    @user.identity.save
+    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
 
   # Pixelated relies on the ability to test invite codes without sending a
@@ -192,15 +181,15 @@ class AccountTest < ActiveSupport::TestCase
   # So we better make sure we return the appropriate errors
   test "errors trying to create account with invite only" do
     with_config invite_required: true do
-      user = Account.create invite_code: @testcode.invite_code
-      assert user.errors[:invite_code].blank?
+      @user = Account.create invite_code: @testcode.invite_code
+      assert @user.errors[:invite_code].blank?
     end
   end
 
   test "errors trying to create account with invalid invite only" do
     with_config invite_required: true do
-      user = Account.create invite_code: "wrong_invite_code"
-      assert_has_errors user, invite_code: "This is not a valid code"
+      @user = Account.create invite_code: "wrong_invite_code"
+      assert_has_errors @user, invite_code: "This is not a valid code"
     end
   end