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

fix append empty role bug #1654

Merged
merged 3 commits into from
Aug 16, 2018
Merged

fix append empty role bug #1654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c77d8ad
fix append empty role bug
vdrobnyi Aug 15, 2018
7979a35
refactor
vdrobnyi Aug 15, 2018
58bfc25
fix issue
vdrobnyi Aug 16, 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
22 changes: 7 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 @@ -396,7 +396,7 @@ namespace iroha {
SELECT CASE
WHEN EXISTS (SELECT * FROM inserted) THEN 0
%s
ELSE 5
ELSE 4
Copy link
Contributor

@igor-egorov igor-egorov Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is absolutely unclear what are the magic numbers used :(
Even more - neither executor itself nor its methods do not have documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Error codes concept is still in development as I understand, so this is internal API.

END AS result)");
if (do_validation_) {
cmd =
Expand All @@ -407,9 +407,7 @@ namespace iroha {
SELECT permission FROM role_has_permissions
WHERE role_id = :role_id
),
role_has_any_permission AS (
SELECT permission <> '0'::bit(%2%) FROM role_permissions
),

account_roles AS (
SELECT role_id FROM account_has_roles WHERE account_id = :creator_id
),
Expand All @@ -426,14 +424,14 @@ namespace iroha {
Role::kAppendRole)
% std::to_string(bits))
.str()
% R"( WHERE (SELECT * FROM role_has_any_permission) AND
% R"( WHERE
EXISTS (SELECT * FROM account_roles) AND
(SELECT * FROM account_has_role_permissions)
AND (SELECT * FROM has_perm))"
% R"( WHEN NOT (SELECT * FROM role_has_any_permission) THEN 1
WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 2
WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 3
WHEN NOT (SELECT * FROM has_perm) THEN 4)");
% R"(
WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 1
WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 2
WHEN NOT (SELECT * FROM has_perm) THEN 3)");
} else {
cmd =
(cmd
Expand All @@ -447,12 +445,6 @@ namespace iroha {
st.exchange(soci::use(creator_account_id_, "creator_id"));
st.exchange(soci::use(creator_account_id_, "role_account_id"));
std::vector<std::function<std::string()>> message_gen = {
[&] {
return (boost::format("is valid command validation failed: no "
"permissions in role %s")
% command.roleName())
.str();
},
[&] {
return (boost::format("is valid command validation failed: no "
"roles in account %s")
Expand Down
62 changes: 37 additions & 25 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 @@ -90,7 +90,8 @@ namespace iroha {
true)));
}

std::string role = "role";
const std::string role = "role";
const std::string another_role = "role2";
shared_model::interface::RolePermissionSet role_permissions;
shared_model::interface::permissions::Grantable grantable_permission;
std::unique_ptr<shared_model::interface::Account> account;
Expand Down Expand Up @@ -393,10 +394,10 @@ namespace iroha {
role_permissions2.set(
shared_model::interface::permissions::Role::kRemoveMySignatory);
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions2)),
another_role, role_permissions2)),
true)));
ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), "role2")))));
account->accountId(), another_role)))));
}

/**
Expand All @@ -409,42 +410,55 @@ namespace iroha {
role_permissions2.set(
shared_model::interface::permissions::Role::kRemoveMySignatory);
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions2)),
another_role, role_permissions2)),
true)));
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), "role2")),
account->accountId(), another_role)),
true)));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), "role2")
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
!= roles->end());
}

TEST_F(AppendRole, InvalidAppendRoleTestWhenNoPerms) {
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions)),
another_role, role_permissions)),
true)));
ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), "role2")))));
account->accountId(), another_role)))));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), "role2")
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
== roles->end());
}

TEST_F(AppendRole, ValidAppendRoleTest) {
addAllPerms();
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions)),
another_role, role_permissions)),
true)));
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), "role2")))));
account->accountId(), another_role)))));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), "role2")
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
!= roles->end());
}

TEST_F(AppendRole, ValidAppendRoleTestWhenEmptyPerms) {
addAllPerms();
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole(
another_role, {})),
true)));
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), another_role)))));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
!= roles->end());
}

class CreateAccount : public CommandExecutorTest {
public:
void SetUp() override {
Expand Down Expand Up @@ -628,7 +642,7 @@ namespace iroha {
addAllPerms();
ASSERT_TRUE(
err(execute(buildCommand(TestTransactionBuilder().createDomain(
domain2->domainId(), "role2")))));
domain2->domainId(), another_role)))));
}

/**
Expand Down Expand Up @@ -685,7 +699,7 @@ namespace iroha {
TEST_F(CreateRole, ValidCreateRoleTest) {
addAllPerms();
ASSERT_TRUE(val(execute(buildCommand(
TestTransactionBuilder().createRole("role2", role_permissions)))));
TestTransactionBuilder().createRole(another_role, role_permissions)))));
auto rl = query->getRolePermissions(role);
ASSERT_TRUE(rl);
ASSERT_EQ(rl.get(), role_permissions);
Expand All @@ -700,8 +714,8 @@ namespace iroha {
role_permissions2.set(
shared_model::interface::permissions::Role::kRemoveMySignatory);
ASSERT_TRUE(err(execute(buildCommand(
TestTransactionBuilder().createRole("role2", role_permissions2)))));
auto rl = query->getRolePermissions("role2");
TestTransactionBuilder().createRole(another_role, role_permissions2)))));
auto rl = query->getRolePermissions(another_role);
ASSERT_TRUE(rl);
ASSERT_TRUE(rl->none());
}
Expand All @@ -716,7 +730,7 @@ namespace iroha {
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions)),
another_role, role_permissions)),
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createDomain(
Expand All @@ -728,7 +742,7 @@ namespace iroha {
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().appendRole(
account->accountId(), "role2")),
account->accountId(), another_role)),
true)));
}
};
Expand All @@ -741,10 +755,10 @@ namespace iroha {
TEST_F(DetachRole, ValidDetachRoleTest) {
addAllPerms();
ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().detachRole(
account->accountId(), "role2")))));
account->accountId(), another_role)))));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), "role2")
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
== roles->end());
}

Expand All @@ -755,10 +769,10 @@ namespace iroha {
*/
TEST_F(DetachRole, InvalidDetachRoleTestWhenNoPerms) {
ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().detachRole(
account->accountId(), "role2")))));
account->accountId(), another_role)))));
auto roles = query->getAccountRoles(account->accountId());
ASSERT_TRUE(roles);
ASSERT_TRUE(std::find(roles->begin(), roles->end(), "role2")
ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role)
!= roles->end());
}

Expand All @@ -772,7 +786,7 @@ namespace iroha {
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createRole(
"role2", role_permissions)),
another_role, role_permissions)),
true)));
ASSERT_TRUE(
val(execute(buildCommand(TestTransactionBuilder().createDomain(
Expand Down Expand Up @@ -1405,8 +1419,6 @@ namespace iroha {
std::unique_ptr<shared_model::interface::Account> account2;
};

void checkTransfer() {}

/**
* @given command
* @when trying to add transfer asset
Expand Down