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

Conversation

grimadas
Copy link
Contributor

@grimadas grimadas commented Dec 6, 2018

Description of the Change

This PR adds domain permissions for commands: AddAssetQty and SubtractAssetQty.

Benefits

Following changes allow restricting permissions for issuing asset for specific domains. For example, one can create assets only in their domain if has this permission.

Possible Drawbacks

None

Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
@neewy
Copy link
Contributor

neewy commented Dec 7, 2018

I would change the target into a trunk, where we can update documentation as well @grimadas

@grimadas grimadas changed the base branch from dev to trunk/command-domain-permission December 7, 2018 07:50
@@ -68,6 +69,8 @@ namespace {
return {with_validation.str(), without_validation.str()};
}

auto hasCommandPermission() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What it is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed

Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
 - Change id type

 - Add comments for test

Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
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

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

@@ -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

Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
@neewy neewy removed the request for review from nickaleks December 10, 2018 09:07
Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

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

Please add documentation
(modify docs/source/permissions/matrix.csv, add two python examples for BTF, make permissions in docs/source)


// 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

@@ -230,10 +231,10 @@ 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.

@@ -264,6 +265,30 @@ 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.

Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
@grimadas grimadas force-pushed the feature/domain-asset-permission branch from a0206a2 to 16abcff Compare December 11, 2018 13:13
Signed-off-by: grimadas <bulat.nasrulin@gmail.com>
Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

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

👍

@grimadas grimadas changed the base branch from trunk/command-domain-permission to dev December 11, 2018 13:38
@grimadas grimadas merged commit 8f11223 into dev Dec 11, 2018
@grimadas grimadas deleted the feature/domain-asset-permission branch December 11, 2018 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants