fix: respond with error on invalid pgp key

We used to just ignore the key.
Also separated the code for handling key updates from other
user updates. This should eventually be moved to a different
route. Mixing the two makes the implementation really hard.

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
+      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

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

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