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

refactor(permissions): define default permission set #5075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(permissions): define default permission set #5075

wants to merge 1 commit into from
+666 −1,048

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Sep 13, 2024 •
edited
Loading

Context

  • added CanManagePeers, CanRegisterDomain and CanManageRoles permissions
  • unified CanSetKeyValueXXX + CanRemoveKeyValueXXX = CanModifyXXXMetadata
  • unified CanMintAsset + CanBurnAsset + CanTransferAsset = CanModifyAsset
  • changed FindPermissions so it returns only inherent permissions, not from roles
  • fixed validation of grant/revoke for roles

closes #4206

Solution

  • Describe the approach taken to achieve the objective / resolve the issue.

Review notes (optional)

  • For complex PRs, try to provide some information on how to approach the review more effectively.
  • For example, is there a natural order in which the affected files should be reviewed?

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@mversic mversic force-pushed the default_permission_set branch 3 times, most recently from 8bc8495 to 208c2f0 Compare September 13, 2024 08:42
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Sep 13, 2024
nxsaken
nxsaken previously approved these changes Sep 13, 2024
View reviewed changes
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Sep 13, 2024
@github-actions GitHub Actions
Copy link

@BAStos525

@mversic mversic force-pushed the default_permission_set branch 7 times, most recently from 906bb5b to 502a580 Compare September 15, 2024 05:43
@mversic mversic changed the title chore(permissions): define default permission set refactor(permissions): define default permission set Sep 15, 2024
@mversic mversic force-pushed the default_permission_set branch 2 times, most recently from d88ccc4 to cd94357 Compare September 15, 2024 06:07
@mversic mversic force-pushed the default_permission_set branch 5 times, most recently from e1dc255 to feafd4d Compare September 15, 2024 11:16
mversic
mversic commented Sep 15, 2024
View reviewed changes
crates/iroha_data_model/src/role.rs
Comment on lines +87 to +88
/// First owner
pub grant_to: AccountId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When role is first registered it has to be given to someone who's the initial owner and can then transitively grant the role to others #5078. An alternative would be to grant role to the account that registers the role, but I felt that complicates things because I would have to track and limit how many new roles has each account created. Instead I introduced CanManageRoles permission

mversic
mversic commented Sep 16, 2024
View reviewed changes
crates/iroha_core/src/state.rs
Comment on lines -582 to -592
let mut tokens = self
.account_inherent_permissions(account_id)
.collect::<BTreeSet<_>>();

for role_id in self.account_roles_iter(account_id) {
if let Some(role) = self.roles().get(role_id) {
tokens.extend(role.permissions.iter());
}
}

Ok(tokens.into_iter())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to remove this and have FindPermissions return only inherent permissions because I was hitting a bug where executor would query all permissions of an account and would then try to revoke them but some of the returned permissions were inside roles and not inherent so the revoke instruction would fail. This was happening with is_permission_domain_associated and the likes

Alternatively, I could have taken an approach where this query returns inherent and role permissions but clearly separated so that the caller can know how to work with each and only revoke the inherent ones.

mversic
mversic commented Sep 16, 2024
View reviewed changes
crates/iroha_core/src/smartcontracts/isi/account.rs Outdated
Copy link
Contributor Author

@mversic mversic Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

a reminder that if I revert this, we should also emit Account::PermissionChanged on every Grant<Permission, Role> or Revoke<Permission, Role>

@mversic mversic force-pushed the default_permission_set branch 4 times, most recently from 817ec0c to 45edafa Compare September 16, 2024 04:58
@mversic mversic assigned nxsaken and unassigned nxsaken and 0x009922 Sep 16, 2024
@mversic @nxsaken
refactor(permissions): define default permission set
fd4dd86 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@Erigara Erigara self-assigned this Sep 19, 2024
Erigara
Erigara reviewed Sep 19, 2024
View reviewed changes
crates/iroha/tests/integration/asset.rs
@@ -12,7 +12,7 @@ use iroha::{
},
};
use iroha_config::parameters::actual::Root as Config;
use iroha_executor_data_model::permission::asset::CanTransferUserAsset;
use iroha_executor_data_model::permission::asset::CanModifyAsset;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for combining this permissions?
We can emulate CanModifyAsset as role with 3 permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but is this granularity necessary? Do you want to give someone the privilege to Burn but not Mint and Transfer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not insisting on granularity, but trying to understand is it possible use case to allow only transfers for example.
It feels like minting asset should be prohibited almost always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand now, I'll restore this

crates/iroha_executor/src/permission.rs
Comment on lines +144 to +147
let role = match crate::data_model::query::builder::QueryBuilderExt::execute_single(
iroha_smart_contract::query(FindRoles)
.filter_with(|role| role.id.eq(role_id.clone())),
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If user has many roles we introduce a lot of repeated FindRoles queries.
We can avoid that by query FindRoles only once and provide complex filter with or of all required roles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Erigara Erigara Erigara left review comments

@nxsaken nxsaken nxsaken approved these changes

@s8sato s8sato Awaiting requested review from s8sato s8sato is a code owner

@SamHSmith SamHSmith Awaiting requested review from SamHSmith SamHSmith is a code owner

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@0x009922 0x009922 Awaiting requested review from 0x009922 0x009922 is a code owner

@DCNick3 DCNick3 Awaiting requested review from DCNick3 DCNick3 is a code owner

@dima74 dima74 Awaiting requested review from dima74 dima74 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@nxsaken nxsaken

@Erigara Erigara

@mversic mversic

Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define a set of default permission tokens
4 participants
@mversic @nxsaken @Erigara @0x009922