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

Jump to bottom

have verify_account_authority check passed-in keys #1384

Merged
merged 10 commits into from
Oct 25, 2018
Merged

have verify_account_authority check passed-in keys #1384

Show file tree
Hide file tree
Changes from 2 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
32 changes: 22 additions & 10 deletions libraries/app/database_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl>
vector<withdraw_permission_object> get_withdraw_permissions_by_giver(const std::string account_id_or_name, withdraw_permission_id_type start, uint32_t limit)const;
vector<withdraw_permission_object> get_withdraw_permissions_by_recipient(const std::string account_id_or_name, withdraw_permission_id_type start, uint32_t limit)const;

//private:
// private:
Copy link
Member

Choose a reason for hiding this comment

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

we maybe just remove this comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried (hence the space I accidentally added). It did not compile due to _db being used outside, plus some others. I was thinking of fixing all of it, but resisted the urge because I was unsure why it was originally commented out.

static string price_to_string( const price& _price, const asset_object& _base, const asset_object& _quote );

template<typename T>
Expand Down Expand Up @@ -272,7 +272,9 @@ 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;

private:
bool public_key_found(const flat_set<public_key_type>& to_be_found,
const vector<public_key_type>& collection_to_search)const;
};

//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -2068,17 +2070,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
19 changes: 19 additions & 0 deletions tests/tests/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,23 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, database_fixture)
} 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_SUITE_END()