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