From ae0aa02d038f16bb0919eebee805624bb891f48f Mon Sep 17 00:00:00 2001
From: Azul <azul@riseup.net>
Date: Thu, 16 Nov 2017 13:17:22 +0100
Subject: minor: clean up account test

also ensures that created user is cleaned up even if test fails
---
 test/unit/account_test.rb | 163 +++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 87 deletions(-)

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
 
-- 
cgit v1.2.3


From 1ce9a3355ee59181df0359ebb455efa9ef323bb6 Mon Sep 17 00:00:00 2001
From: Azul <azul@riseup.net>
Date: Thu, 16 Nov 2017 13:18:55 +0100
Subject: 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 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
-- 
cgit v1.2.3