Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Feature/domain asset permission #1924

Merged
merged 8 commits into from
Dec 11, 2018
Merged

Feature/domain asset permission #1924

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a2103e1
Add asset qty domain permissions
grimadas Dec 6, 2018
9c61779
Fix postgres tests
grimadas Dec 6, 2018
452fd8c
Remove unused command
grimadas Dec 7, 2018
ea5e92b
Fix comments:
grimadas Dec 10, 2018
46ac649
Add rp to permission
grimadas Dec 10, 2018
16abcff
Add example files for new permissions
grimadas Dec 11, 2018
d674f3a
Update rst file
grimadas Dec 11, 2018
873d0ca
Merge remote-tracking branch 'origin/dev' into feature/domain-asset-p…
grimadas Dec 11, 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
62 changes: 47 additions & 15 deletions irohad/ametsuchi/impl/postgres_command_executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "ametsuchi/impl/postgres_command_executor.hpp"

#include <soci/postgresql/soci-postgresql.h>
#include <boost/algorithm/string.hpp>
#include <boost/format.hpp>
#include "ametsuchi/impl/soci_utils.hpp"
#include "cryptography/public_key.hpp"
Expand Down Expand Up @@ -230,13 +231,13 @@ namespace {
}

std::string checkAccountRolePermission(
shared_model::interface::permissions::Role permission,
shared_model::interface::permissions::Role role,
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a role, it is a permission. We have two kinds of permissions: role and grantable. This one is role permission.

Should be changed back. Also it can be named as role_permission.

const shared_model::interface::types::AccountIdType &account_id) {
const auto perm_str =
shared_model::interface::RolePermissionSet({permission}).toBitstring();
shared_model::interface::RolePermissionSet({role}).toBitstring();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the comment to the line 234.

const auto bits = shared_model::interface::RolePermissionSet::size();
std::string query = (boost::format(R"(
SELECT COALESCE(bit_or(rp.permission), '0'::bit(%1%))
SELECT COALESCE(bit_or(permission), '0'::bit(%1%))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove rp here? It actually makes SQL easier to read

& '%2%' = '%2%' FROM role_has_permissions AS rp
JOIN account_has_roles AS ar on ar.role_id = rp.role_id
WHERE ar.account_id = %3%)")
Expand Down Expand Up @@ -264,6 +265,31 @@ namespace {
return query;
}

std::string checkAccountDomainRoleOrGlobalRolePermission(
shared_model::interface::permissions::Role global_role,
shared_model::interface::permissions::Role domain_role,
Copy link
Contributor

Choose a reason for hiding this comment

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

The two above are permissions too. See comment to postgres_command_executor.cpp:234 for details.
Please fix the name of function arguments.

const shared_model::interface::types::AccountIdType &creator_id,
const shared_model::interface::types::AccountIdType
&id_with_target_domain) {
std::string query = (boost::format(R"(WITH
has_global_role_perm AS (%1%),
has_domain_role_perm AS (%2%)
SELECT CASE
WHEN (SELECT * FROM has_global_role_perm) THEN true
WHEN ((split_part(%3%, '@', 2) = split_part(%4%, '@', 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think not this WHEN never used. You can change the type of id_with_target_domain to AssetIdType

OR (split_part(%3%, '@', 2) = split_part(%4%, '#', 2))) THEN
CASE
WHEN (SELECT * FROM has_domain_role_perm) THEN true
ELSE false
END
ELSE false END
)") % checkAccountRolePermission(global_role, creator_id)
% checkAccountRolePermission(domain_role, creator_id)
% creator_id % id_with_target_domain)
.str();
return query;
}

std::string checkAccountHasRoleOrGrantablePerm(
shared_model::interface::permissions::Role role,
shared_model::interface::permissions::Grantable grantable,
Expand Down Expand Up @@ -1183,9 +1209,12 @@ namespace iroha {
{"addAssetQuantity",
addAssetQuantityBase,
{(boost::format(R"(has_perm AS (%s),)")
% checkAccountRolePermission(
% checkAccountDomainRoleOrGlobalRolePermission(
shared_model::interface::permissions::Role::kAddAssetQty,
"$1"))
shared_model::interface::permissions::Role::
kAddDomainAssetQty,
"$1",
"$2"))
.str(),
"AND (SELECT * from has_perm)",
"WHEN NOT (SELECT * from has_perm) THEN 2"}});
Expand Down Expand Up @@ -1444,17 +1473,20 @@ namespace iroha {
WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 5
)"}});

statements.push_back(
{"subtractAssetQuantity",
subtractAssetQuantityBase,
{(boost::format(R"(
statements.push_back({"subtractAssetQuantity",
subtractAssetQuantityBase,
{(boost::format(R"(
has_perm AS (%s),)")
% checkAccountRolePermission(shared_model::interface::permissions::
Role::kSubtractAssetQty,
"$1"))
.str(),
R"( AND (SELECT * FROM has_perm))",
R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}});
% checkAccountDomainRoleOrGlobalRolePermission(
shared_model::interface::permissions::Role::
kSubtractAssetQty,
shared_model::interface::permissions::Role::
kSubtractDomainAssetQty,
"$1",
"$2"))
.str(),
R"( AND (SELECT * FROM has_perm))",
R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}});

statements.push_back(
{"transferAsset",
Expand Down
3 changes: 2 additions & 1 deletion shared_model/interfaces/permissions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ namespace shared_model {
kTransferMyAssets,
kSetMyAccountDetail,
kGetBlocks,

kAddDomainAssetQty,
kSubtractDomainAssetQty,
COUNT
};

Expand Down
4 changes: 4 additions & 0 deletions shared_model/schema/primitive.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ enum RolePermission {
can_grant_can_remove_my_signatory = 39;
can_grant_can_transfer_my_assets = 40;
can_grant_can_set_my_account_detail = 41;

// Domain commands permissions
can_add_domain_asset_qty = 43;
can_subtract_domain_asset_qty = 44;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move those two lines to the line 36 (after can_subtract_asset_qty = 4).

No need to introduce an additional section for commands permissions.
It is fine to have unordered elements inside proto enum (e.g. can_get_blocks permission)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or move them to the end of the block with command permissions

}

enum GrantablePermission {
Expand Down
191 changes: 180 additions & 11 deletions test/module/irohad/ametsuchi/postgres_executor_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,23 @@ namespace iroha {
true)));
}

void addOnePerm(
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a comment please

const shared_model::interface::permissions::Role perm,
const shared_model::interface::types::AccountIdType account_id =
"id@domain",
const shared_model::interface::types::RoleIdType role_id = "all") {
shared_model::interface::RolePermissionSet permissions;
permissions.set(perm);
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createRole(
role_id, permissions)),
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().appendRole(
account_id, role_id)),
true)));
}

/**
* Check that command result contains specific error code and error
* message
Expand Down Expand Up @@ -167,16 +184,16 @@ namespace iroha {
/**
* Add default asset and check that it is done
*/
void addAsset() {
void addAsset(const shared_model::interface::types::DomainIdType &domain_id = "domain") {
auto asset = clone(TestAccountAssetBuilder()
.domainId(domain->domainId())
.domainId(domain_id)
.assetId(asset_id)
.precision(1)
.build());

ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createAsset(
"coin", domain->domainId(), 1)),
"coin", domain_id, 1)),
true)));
}

Expand All @@ -185,8 +202,8 @@ namespace iroha {
};

/**
* @given command
* @when trying to add account asset
* @given addAccountAsset command
* @when trying to add asset to account
* @then account asset is successfully added
*/
TEST_F(AddAccountAssetTest, Valid) {
Expand All @@ -211,10 +228,79 @@ namespace iroha {
}

/**
* @given command
* @when trying to add account asset without permission
* @then account asset not added
* @given addAccountAsset command
* @when trying to add asset to account with a domain permission
* @then account asset is successfully added
*/
TEST_F(AddAccountAssetTest, DomainPermValid) {
addAsset();
addOnePerm(shared_model::interface::permissions::Role::kAddDomainAssetQty);
ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())))));
auto account_asset =
query->getAccountAsset(account->accountId(), asset_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());
ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())))));
account_asset = query->getAccountAsset(account->accountId(), asset_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ("2.0", account_asset.get()->balance().toStringRepr());
}

/**
* @given addAccountAsset command and invalid domain permission
* @when trying to add asset
* @then account asset is not added
*/
TEST_F(AddAccountAssetTest, DomainPermInvalid) {
std::unique_ptr<shared_model::interface::Domain> domain2;
domain2 = clone(
TestDomainBuilder().domainId("domain2").defaultRole(role).build());
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createDomain(
domain2->domainId(), role)),
true)));
addAsset(domain2->domainId());
addOnePerm(shared_model::interface::permissions::Role::kAddDomainAssetQty);

auto asset2_id = "coin#"+domain2->domainId();


ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset2_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())),
true)));


auto account_asset =
query->getAccountAsset(account->accountId(), asset2_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());

auto cmd_result = execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset2_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())));

std::vector<std::string> query_args{
account->accountId(), asset_amount_one_zero, asset2_id, "1"};
CHECK_ERROR_CODE_AND_MESSAGE(cmd_result, 2, query_args);
}


/**
* @given command
* @when trying to add account asset without permission
* @then account asset not added
*/
TEST_F(AddAccountAssetTest, NoPerms) {
addAsset();
ASSERT_TRUE(val(execute(
Expand Down Expand Up @@ -1653,16 +1739,16 @@ namespace iroha {
/**
* Add default asset and check that it is done
*/
void addAsset() {
void addAsset(const shared_model::interface::types::DomainIdType &domain_id="domain") {
auto asset = clone(TestAccountAssetBuilder()
.domainId(domain->domainId())
.domainId(domain_id)
.assetId(asset_id)
.precision(1)
.build());

ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createAsset(
"coin", domain->domainId(), 1)),
"coin", domain_id, 1)),
true)));
}

Expand Down Expand Up @@ -1739,6 +1825,89 @@ namespace iroha {
account_asset.get()->balance().toStringRepr());
}


/**
* @given command and domain permission
* @when trying to subtract account asset
* @then account asset is successfully subtracted
*/
TEST_F(SubtractAccountAssetTest, DomainPermValid) {
addAsset();
addOnePerm(shared_model::interface::permissions::Role::kSubtractDomainAssetQty);

ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())),
true)));
auto account_asset =
query->getAccountAsset(account->accountId(), asset_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());
ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())),
true)));
account_asset = query->getAccountAsset(account->accountId(), asset_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ("2.0", account_asset.get()->balance().toStringRepr());
ASSERT_TRUE(val(execute(buildCommand(
TestTransactionBuilder()
.subtractAssetQuantity(asset_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())))));
account_asset = query->getAccountAsset(account->accountId(), asset_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());
}

/**
* @given command and invalid domain permission/ permission in other domain
* @when trying to subtract asset
* @then no account asset is subtracted
*/
TEST_F(SubtractAccountAssetTest, DomainPermInvalid) {
std::unique_ptr<shared_model::interface::Domain> domain2;
domain2 = clone(
TestDomainBuilder().domainId("domain2").defaultRole(role).build());
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createDomain(
domain2->domainId(), role)),
true)));
addAsset(domain2->domainId());
addOnePerm(shared_model::interface::permissions::Role::kSubtractDomainAssetQty);

auto asset2_id = "coin#"+domain2->domainId();
ASSERT_TRUE(val(execute(
buildCommand(TestTransactionBuilder()
.addAssetQuantity(asset2_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())),
true)));
auto account_asset =
query->getAccountAsset(account->accountId(), asset2_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());

auto cmd_result = execute(buildCommand(
TestTransactionBuilder()
.subtractAssetQuantity(asset2_id, asset_amount_one_zero)
.creatorAccountId(account->accountId())));

std::vector<std::string> query_args{
account->accountId(), asset2_id, asset_amount_one_zero, "1"};
CHECK_ERROR_CODE_AND_MESSAGE(cmd_result, 2, query_args);

account_asset = query->getAccountAsset(account->accountId(), asset2_id);
ASSERT_TRUE(account_asset);
ASSERT_EQ(asset_amount_one_zero,
account_asset.get()->balance().toStringRepr());
}



/**
* @given command
* @when trying to subtract account asset with non-existing asset
Expand Down