From 776023cc0becd2b299732049c586b46d61439799 Mon Sep 17 00:00:00 2001 From: Dmitry Murzin Date: Fri, 15 Nov 2024 20:28:50 +0300 Subject: [PATCH 1/2] refactor: Simplify revoking permission in default executor Signed-off-by: Dmitry Murzin --- crates/iroha_executor/src/default/mod.rs | 117 +++++------------------ crates/iroha_executor/src/permission.rs | 36 +++++++ 2 files changed, 60 insertions(+), 93 deletions(-) diff --git a/crates/iroha_executor/src/default/mod.rs b/crates/iroha_executor/src/default/mod.rs index 7a8479a8ee3..b1276cf644e 100644 --- a/crates/iroha_executor/src/default/mod.rs +++ b/crates/iroha_executor/src/default/mod.rs @@ -166,7 +166,7 @@ pub mod domain { use super::*; use crate::permission::{ - account::is_account_owner, accounts_permissions, domain::is_domain_owner, roles_permissions, + account::is_account_owner, domain::is_domain_owner, revoke_permissions, }; pub fn visit_register_domain( @@ -202,30 +202,13 @@ pub mod domain { .is_owned_by(&executor.context().authority, executor.host()) } { - let mut err = None; - for (owner_id, permission) in accounts_permissions(executor.host()) { - if is_permission_domain_associated(&permission, domain_id) { - let isi = &Revoke::account_permission(permission, owner_id.clone()); - - if let Err(error) = executor.host().submit(isi) { - err = Some(error); - break; - } - } - } - if let Some(err) = err { - deny!(executor, err); + revoke_permissions(executor, |permission| { + is_permission_domain_associated(permission, domain_id) + }); + if executor.verdict().is_err() { + return; } - for (role_id, permission) in roles_permissions(executor.host()) { - if is_permission_domain_associated(&permission, domain_id) { - let isi = &Revoke::role_permission(permission, role_id.clone()); - - if let Err(err) = executor.host().submit(isi) { - deny!(executor, err); - } - } - } execute!(executor, isi); } deny!(executor, "Can't unregister domain"); @@ -389,7 +372,7 @@ pub mod account { }; use super::*; - use crate::permission::{account::is_account_owner, accounts_permissions, roles_permissions}; + use crate::permission::{account::is_account_owner, revoke_permissions}; pub fn visit_register_account( executor: &mut V, @@ -441,30 +424,13 @@ pub mod account { .is_owned_by(&executor.context().authority, executor.host()) } { - let mut err = None; - for (owner_id, permission) in accounts_permissions(executor.host()) { - if is_permission_account_associated(&permission, account_id) { - let isi = &Revoke::account_permission(permission, owner_id.clone()); - - if let Err(error) = executor.host().submit(isi) { - err = Some(error); - break; - } - } - } - if let Some(err) = err { - deny!(executor, err); + revoke_permissions(executor, |permission| { + is_permission_account_associated(permission, account_id) + }); + if executor.verdict().is_err() { + return; } - for (role_id, permission) in roles_permissions(executor.host()) { - if is_permission_account_associated(&permission, account_id) { - let isi = &Revoke::role_permission(permission, role_id.clone()); - - if let Err(err) = executor.host().submit(isi) { - deny!(executor, err); - } - } - } execute!(executor, isi); } deny!(executor, "Can't unregister another account"); @@ -580,8 +546,7 @@ pub mod asset_definition { use super::*; use crate::permission::{ - account::is_account_owner, accounts_permissions, - asset_definition::is_asset_definition_owner, roles_permissions, + account::is_account_owner, asset_definition::is_asset_definition_owner, revoke_permissions, }; pub fn visit_register_asset_definition( @@ -638,30 +603,13 @@ pub mod asset_definition { .is_owned_by(&executor.context().authority, executor.host()) } { - let mut err = None; - for (owner_id, permission) in accounts_permissions(executor.host()) { - if is_permission_asset_definition_associated(&permission, asset_definition_id) { - let isi = &Revoke::account_permission(permission, owner_id.clone()); - - if let Err(error) = executor.host().submit(isi) { - err = Some(error); - break; - } - } - } - if let Some(err) = err { - deny!(executor, err); + revoke_permissions(executor, |permission| { + is_permission_asset_definition_associated(permission, asset_definition_id) + }); + if executor.verdict().is_err() { + return; } - for (role_id, permission) in roles_permissions(executor.host()) { - if is_permission_asset_definition_associated(&permission, asset_definition_id) { - let isi = &Revoke::role_permission(permission, role_id.clone()); - - if let Err(err) = executor.host().submit(isi) { - deny!(executor, err); - } - } - } execute!(executor, isi); } deny!( @@ -1367,7 +1315,7 @@ pub mod trigger { use super::*; use crate::permission::{ - accounts_permissions, domain::is_domain_owner, roles_permissions, trigger::is_trigger_owner, + domain::is_domain_owner, revoke_permissions, trigger::is_trigger_owner, }; pub fn visit_register_trigger( @@ -1420,28 +1368,11 @@ pub mod trigger { .is_owned_by(&executor.context().authority, executor.host()) } { - let mut err = None; - for (owner_id, permission) in accounts_permissions(executor.host()) { - if is_permission_trigger_associated(&permission, trigger_id) { - let isi = &Revoke::account_permission(permission, owner_id.clone()); - - if let Err(error) = executor.host().submit(isi) { - err = Some(error); - break; - } - } - } - if let Some(err) = err { - deny!(executor, err); - } - - for (role_id, permission) in roles_permissions(executor.host()) { - if is_permission_trigger_associated(&permission, trigger_id) { - let isi = &Revoke::role_permission(permission, role_id.clone()); - if let Err(err) = executor.host().submit(isi) { - deny!(executor, err); - } - } + revoke_permissions(executor, |permission| { + is_permission_trigger_associated(permission, trigger_id) + }); + if executor.verdict().is_err() { + return; } execute!(executor, isi); diff --git a/crates/iroha_executor/src/permission.rs b/crates/iroha_executor/src/permission.rs index 7460b5e0df4..08ec2482a6d 100644 --- a/crates/iroha_executor/src/permission.rs +++ b/crates/iroha_executor/src/permission.rs @@ -5,11 +5,13 @@ use alloc::{borrow::ToOwned as _, vec::Vec}; use iroha_executor_data_model::permission::Permission; use crate::{ + deny, prelude::Context, smart_contract::{ data_model::{executor::Result, permission::Permission as PermissionObject, prelude::*}, prelude::*, }, + Execute, }; /// Declare permission types of current module. Use it with a full path to the permission. @@ -1084,3 +1086,37 @@ pub(crate) fn roles_permissions(host: &Iroha) -> impl Iterator( + executor: &mut V, + condition: impl Fn(&PermissionObject) -> bool, +) { + let mut err = None; + for (owner_id, permission) in accounts_permissions(executor.host()) { + if condition(&permission) { + let isi = Revoke::account_permission(permission, owner_id.clone()); + + if let Err(error) = executor.host().submit(&isi) { + err = Some(error); + break; + } + } + } + if let Some(err) = err { + deny!(executor, err); + } + + for (role_id, permission) in roles_permissions(executor.host()) { + if condition(&permission) { + let isi = Revoke::role_permission(permission, role_id.clone()); + + if let Err(err) = executor.host().submit(&isi) { + deny!(executor, err); + } + } + } +} From e71898ce58f3131c1e32c66cfbea91a4651d9041 Mon Sep 17 00:00:00 2001 From: Dmitry Murzin Date: Mon, 18 Nov 2024 17:17:08 +0300 Subject: [PATCH 2/2] Review fixes Signed-off-by: Dmitry Murzin --- crates/iroha_executor/src/default/mod.rs | 24 ++++++++++++------------ crates/iroha_executor/src/permission.rs | 21 ++++++--------------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/crates/iroha_executor/src/default/mod.rs b/crates/iroha_executor/src/default/mod.rs index b1276cf644e..26d354a3f5d 100644 --- a/crates/iroha_executor/src/default/mod.rs +++ b/crates/iroha_executor/src/default/mod.rs @@ -202,11 +202,11 @@ pub mod domain { .is_owned_by(&executor.context().authority, executor.host()) } { - revoke_permissions(executor, |permission| { + let err = revoke_permissions(executor, |permission| { is_permission_domain_associated(permission, domain_id) }); - if executor.verdict().is_err() { - return; + if let Err(err) = err { + deny!(executor, err); } execute!(executor, isi); @@ -424,11 +424,11 @@ pub mod account { .is_owned_by(&executor.context().authority, executor.host()) } { - revoke_permissions(executor, |permission| { + let err = revoke_permissions(executor, |permission| { is_permission_account_associated(permission, account_id) }); - if executor.verdict().is_err() { - return; + if let Err(err) = err { + deny!(executor, err); } execute!(executor, isi); @@ -603,11 +603,11 @@ pub mod asset_definition { .is_owned_by(&executor.context().authority, executor.host()) } { - revoke_permissions(executor, |permission| { + let err = revoke_permissions(executor, |permission| { is_permission_asset_definition_associated(permission, asset_definition_id) }); - if executor.verdict().is_err() { - return; + if let Err(err) = err { + deny!(executor, err); } execute!(executor, isi); @@ -1368,11 +1368,11 @@ pub mod trigger { .is_owned_by(&executor.context().authority, executor.host()) } { - revoke_permissions(executor, |permission| { + let err = revoke_permissions(executor, |permission| { is_permission_trigger_associated(permission, trigger_id) }); - if executor.verdict().is_err() { - return; + if let Err(err) = err { + deny!(executor, err); } execute!(executor, isi); diff --git a/crates/iroha_executor/src/permission.rs b/crates/iroha_executor/src/permission.rs index 08ec2482a6d..e5ef2a51b6c 100644 --- a/crates/iroha_executor/src/permission.rs +++ b/crates/iroha_executor/src/permission.rs @@ -5,7 +5,6 @@ use alloc::{borrow::ToOwned as _, vec::Vec}; use iroha_executor_data_model::permission::Permission; use crate::{ - deny, prelude::Context, smart_contract::{ data_model::{executor::Result, permission::Permission as PermissionObject, prelude::*}, @@ -1089,34 +1088,26 @@ pub(crate) fn roles_permissions(host: &Iroha) -> impl Iterator( executor: &mut V, condition: impl Fn(&PermissionObject) -> bool, -) { - let mut err = None; +) -> Result<(), ValidationFail> { for (owner_id, permission) in accounts_permissions(executor.host()) { if condition(&permission) { let isi = Revoke::account_permission(permission, owner_id.clone()); - if let Err(error) = executor.host().submit(&isi) { - err = Some(error); - break; - } + executor.host().submit(&isi)?; } } - if let Some(err) = err { - deny!(executor, err); - } for (role_id, permission) in roles_permissions(executor.host()) { if condition(&permission) { let isi = Revoke::role_permission(permission, role_id.clone()); - if let Err(err) = executor.host().submit(&isi) { - deny!(executor, err); - } + executor.host().submit(&isi)?; } } + + Ok(()) }