From 1ce9a3355ee59181df0359ebb455efa9ef323bb6 Mon Sep 17 00:00:00 2001
From: Azul <azul@riseup.net>
Date: Thu, 16 Nov 2017 13:18:55 +0100
Subject: [PATCH] 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 | 42 ++++++++++++++++++-------
app/models/account.rb | 12 -------
test/integration/api/pgp_key_test.rb | 9 ++++--
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb
index cb7b7bc7..65b80c7d 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 d77c61f5..5a4111d3 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 4c7fb4c7..f2744e11 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
--
GitLab