Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

have verify_account_authority check passed-in keys #1384

Merged
merged 10 commits into from
Oct 25, 2018
+138 −14
Merged

have verify_account_authority check passed-in keys #1384

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7d44c3a
initial fix for verify_account_authority
jmjatlanta Oct 16, 2018
824ed4f
improved way to verify account and keys
jmjatlanta Oct 19, 2018
48fa400
Removed unneeded method definition
jmjatlanta Oct 19, 2018
c2b63f5
Added multiple signatures to test
jmjatlanta Oct 23, 2018
5db31f5
shorten line length
jmjatlanta Oct 23, 2018
7e4e033
add test for multisig
jmjatlanta Oct 23, 2018
e03d329
improved multisig test
jmjatlanta Oct 23, 2018
673ac2b
fix short line
jmjatlanta Oct 23, 2018
f523ad6
moved tests, fixed comments
jmjatlanta Oct 23, 2018
b078310
clarify comment
jmjatlanta Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions libraries/app/database_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl>
map< pair<asset_id_type,asset_id_type>, std::function<void(const variant&)> > _market_subscriptions;
graphene::chain::database& _db;
const application_options* _app_options = nullptr;

};

//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -2068,17 +2067,27 @@ bool database_api::verify_account_authority( const string& account_name_or_id, c
return my->verify_account_authority( account_name_or_id, signers );
}

bool database_api_impl::verify_account_authority( const string& account_name_or_id, const flat_set<public_key_type>& keys )const
bool database_api_impl::verify_account_authority( const string& account_name_or_id,
const flat_set<public_key_type>& keys )const
{
const account_object* account = get_account_from_string(account_name_or_id);

/// reuse trx.verify_authority by creating a dummy transfer
signed_transaction trx;
// create a dummy transfer
transfer_operation op;
op.from = account->id;
trx.operations.emplace_back(op);
op.from = get_account_from_string(account_name_or_id)->id;
std::vector<operation> ops;
ops.emplace_back(op);

return verify_authority( trx );
try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just to create signed_transaction and call it's verify_authority method ?
So the full implementation of verify_account_authority might be like next:

bool database_api_impl::verify_account_authority( const string& account_name_or_id, 
      const flat_set<public_key_type>& keys )const
{
   // create a dummy transfer
   transfer_operation op;
   op.from = get_account_from_string(account_name_or_id)->id;
   std::vector<operation> ops;
   ops.emplace_back(op);
   signed_transaction trx;
   trx.signees = keys;
   trx.operations.push_back(op);

   return verify_authority(trx);
}

It's less code and more simple IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, but IMO the signees field should be used as an internal cache only, not part of the tx struct's interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

graphene::chain::verify_authority will use signees returned by signed_transaction::get_signature_keys method, so it should work

{
graphene::chain::verify_authority(ops, keys,
[this]( account_id_type id ){ return &id(_db).active; },
[this]( account_id_type id ){ return &id(_db).owner; } );
}
catch (fc::exception& ex)
{
return false;
}

return true;
}

processed_transaction database_api::validate_transaction( const signed_transaction& trx )const
Expand Down
7 changes: 5 additions & 2 deletions libraries/app/include/graphene/app/database_api.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,12 @@ class database_api
bool verify_authority( const signed_transaction& trx )const;

/**
* @return true if the signers have enough authority to authorize an account
* @brief Verify that the public keys have enough authority to authorize a transaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? There is no transaction in the API.

* @param account_name_or_id the account to check
* @param signers the public keys
* @return true if the passed in keys have enough authority to authorize a transaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this? There is no transaction in the API.

*/
bool verify_account_authority( const string& account_name_or_id, const flat_set<public_key_type>& signers )const;
bool verify_account_authority( const string& account_name_or_id, const flat_set<public_key_type>& signers )const;

/**
* Validates a transaction against the current state without broadcasting it on the network.
Expand Down
116 changes: 114 additions & 2 deletions tests/tests/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <graphene/app/database_api.hpp>
#include <graphene/wallet/wallet.hpp>
#include <fc/crypto/digest.hpp>

#include <iostream>

Expand All @@ -45,7 +46,7 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, database_fixture)
/***
* Act
*/
int nbr_keys_desired = 3;
unsigned int nbr_keys_desired = 3;
vector<brain_key_info> derived_keys = graphene::wallet::utility::derive_owner_keys_from_brain_key("SOME WORDS GO HERE", nbr_keys_desired);


Expand All @@ -70,10 +71,121 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, database_fixture)
string expected_prefix = GRAPHENE_ADDRESS_PREFIX;
for (auto info : derived_keys) {
string description = (string) info.pub_key;
BOOST_CHECK_EQUAL(0, description.find(expected_prefix));
BOOST_CHECK_EQUAL(0u, description.find(expected_prefix));
}

} FC_LOG_AND_RETHROW()
}

BOOST_AUTO_TEST_CASE(verify_account_authority) {
try {

ACTORS( (nathan) );
graphene::app::database_api db_api(db);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to move these test cases to database_api_tests.cpp.


// good keys
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_public_key);
BOOST_CHECK(db_api.verify_account_authority( "nathan", public_keys));

// bad keys
flat_set<public_key_type> bad_public_keys;
bad_public_keys.emplace(public_key_type("BTS6MkMxwBjFWmcDjXRoJ4mW9Hd4LCSPwtv9tKG1qYW5Kgu4AhoZy"));
BOOST_CHECK(!db_api.verify_account_authority( "nathan", bad_public_keys));

} FC_LOG_AND_RETHROW()
}

BOOST_AUTO_TEST_CASE( any_two_of_three )
{
try {
fc::ecc::private_key nathan_key1 = fc::ecc::private_key::regenerate(fc::digest("key1"));
fc::ecc::private_key nathan_key2 = fc::ecc::private_key::regenerate(fc::digest("key2"));
fc::ecc::private_key nathan_key3 = fc::ecc::private_key::regenerate(fc::digest("key3"));
const account_object& nathan = create_account("nathan", nathan_key1.get_public_key() );
fund(nathan);
graphene::app::database_api db_api(db);

try {
account_update_operation op;
op.account = nathan.id;
op.active = authority(2, public_key_type(nathan_key1.get_public_key()), 1,
public_key_type(nathan_key2.get_public_key()), 1, public_key_type(nathan_key3.get_public_key()), 1);
op.owner = *op.active;
trx.operations.push_back(op);
sign(trx, nathan_key1);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
trx.clear();
} FC_CAPTURE_AND_RETHROW ((nathan.active))

// two keys should work
{
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_key1.get_public_key());
public_keys.emplace(nathan_key2.get_public_key());
BOOST_CHECK(db_api.verify_account_authority("nathan", public_keys));
}

// the other two keys should work
{
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_key2.get_public_key());
public_keys.emplace(nathan_key3.get_public_key());
BOOST_CHECK(db_api.verify_account_authority("nathan", public_keys));
}

// just one key should not work
{
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_key1.get_public_key());
BOOST_CHECK(!db_api.verify_account_authority("nathan", public_keys));
}
} catch (fc::exception& e) {
edump((e.to_detail_string()));
throw;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test where nathan's account has a multisig from one key and two other accounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added what I think you are asking. It is not much different than the last, so I may be misunderstanding your request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should check different combinations as in any_two_of_three test, perhaps the same combinations, but with 3 accounts, 1 key per 1 account, just my thoughts ...


BOOST_AUTO_TEST_CASE( verify_authority_multiple_accounts )
{
try {
ACTORS( (nathan) (alice) (bob) );

graphene::app::database_api db_api(db);

try {
account_update_operation op;
op.account = nathan.id;
op.active = authority(3, nathan_public_key, 1,
alice.id, 1, bob.id, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we don't need to split this line, it isn't long now

op.owner = *op.active;
trx.operations.push_back(op);
sign(trx, nathan_private_key);
PUSH_TX( db, trx, database::skip_transaction_dupe_check );
trx.clear();
} FC_CAPTURE_AND_RETHROW ((nathan.active))

// requires 3 signatures
{
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_public_key);
public_keys.emplace(alice_public_key);
public_keys.emplace(bob_public_key);
BOOST_CHECK(db_api.verify_account_authority("nathan", public_keys));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need a negative test, e. g. alice_key missing.

// only 2 signatures given
{
flat_set<public_key_type> public_keys;
public_keys.emplace(nathan_public_key);
public_keys.emplace(bob_public_key);
BOOST_CHECK(!db_api.verify_account_authority("nathan", public_keys));
}
} catch (fc::exception& e) {
edump((e.to_detail_string()));
throw;
}
}

BOOST_AUTO_TEST_SUITE_END()