-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
refactor(permissions): define default permission set #5075
base: main
Are you sure you want to change the base?
Conversation
8bc8495
to
208c2f0
Compare
208c2f0
to
371e332
Compare
371e332
to
7e9d7a2
Compare
906bb5b
to
502a580
Compare
d88ccc4
to
cd94357
Compare
e1dc255
to
feafd4d
Compare
/// First owner | ||
pub grant_to: AccountId, |
There was a problem hiding this comment.
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
feafd4d
to
c9365d5
Compare
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()) |
There was a problem hiding this comment.
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.
c9365d5
to
f2a7e5c
Compare
There was a problem hiding this comment.
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>
817ec0c
to
45edafa
Compare
45edafa
to
fd4dd86
Compare
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())), | ||
) { |
There was a problem hiding this comment.
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.
784c009
to
0809e4c
Compare
Signed-off-by: Marin Veršić <[email protected]>
0809e4c
to
a4d893a
Compare
Context
CanManagePeers
,CanRegisterDomain
andCanManageRoles
permissionsCanSetKeyValueXXX
+CanRemoveKeyValueXXX
=CanModifyXXXMetadata
CanMintAsset
+CanBurnAsset
+CanTransferAsset
=CanModifyAsset
FindPermissions
so it returns only inherent permissions, not from rolescloses #4206
Solution
Review notes (optional)
Checklist
CONTRIBUTING.md
.