From 20431ef3600d13d8674328184253878ade41162c Mon Sep 17 00:00:00 2001 From: Dmitry Murzin Date: Fri, 21 Feb 2025 23:16:08 +0400 Subject: [PATCH] Review fixes: Fix NFT permissions Signed-off-by: Dmitry Murzin --- crates/iroha/tests/nft.rs | 63 ++++++++++++++++++++- crates/iroha_executor/src/default/mod.rs | 12 ++-- crates/iroha_executor/src/permission.rs | 71 +++++++++++++++++++----- 3 files changed, 126 insertions(+), 20 deletions(-) diff --git a/crates/iroha/tests/nft.rs b/crates/iroha/tests/nft.rs index 655005ed45..6f7782e1b2 100644 --- a/crates/iroha/tests/nft.rs +++ b/crates/iroha/tests/nft.rs @@ -1,6 +1,7 @@ +use eyre::Result; use iroha::data_model::prelude::*; use iroha_test_network::NetworkBuilder; -use iroha_test_samples::{ALICE_ID, BOB_ID}; +use iroha_test_samples::{gen_account_in, ALICE_ID, BOB_ID}; #[test] fn transfer_nft() { @@ -87,3 +88,63 @@ fn unregister_nft_should_remove_nft_from_account() -> eyre::Result<()> { Ok(()) } + +#[test] +fn nft_owner_cant_modify_nft() -> Result<()> { + let (network, _rt) = NetworkBuilder::new().start_blocking()?; + let client = network.client(); + + let (account_id, account_keypair) = gen_account_in("wonderland"); + let nft_id: NftId = "nft$wonderland".parse()?; + + let create_account = Register::account(Account::new(account_id.clone())); + client.submit_blocking(create_account)?; + + let register_nft = Register::nft(Nft::new(nft_id.clone(), Metadata::default())); + client.submit_blocking(register_nft)?; + + let transfer_nft = Transfer::nft(ALICE_ID.clone(), nft_id.clone(), account_id.clone()); + client.submit_blocking(transfer_nft)?; + + let modify_nft = SetKeyValue::nft(nft_id, "foo".parse()?, "value"); + client + .submit_blocking(modify_nft.clone()) + .expect("Owner of `nft.domain` can modify NFT"); + + let modify_nft_tx = TransactionBuilder::new(network.chain_id(), account_id.clone()) + .with_instructions([modify_nft]) + .sign(account_keypair.private_key()); + let _ = client + .submit_transaction_blocking(&modify_nft_tx) + .expect_err("Owner of NFT can't modify NFT"); + + Ok(()) +} + +#[test] +fn nft_owner_can_transfer_nft() -> Result<()> { + let (network, _rt) = NetworkBuilder::new().start_blocking()?; + let client = network.client(); + + let (account_id, account_keypair) = gen_account_in("wonderland"); + let nft_id: NftId = "nft$wonderland".parse()?; + + let create_account = Register::account(Account::new(account_id.clone())); + client.submit_blocking(create_account)?; + + let register_nft = Register::nft(Nft::new(nft_id.clone(), Metadata::default())); + client.submit_blocking(register_nft)?; + + let transfer_nft1 = Transfer::nft(ALICE_ID.clone(), nft_id.clone(), account_id.clone()); + client.submit_blocking(transfer_nft1)?; + + let transfer_nft2 = Transfer::nft(account_id.clone(), nft_id.clone(), ALICE_ID.clone()); + let transfer_nft2_tx = TransactionBuilder::new(network.chain_id(), account_id.clone()) + .with_instructions([transfer_nft2]) + .sign(account_keypair.private_key()); + client + .submit_transaction_blocking(&transfer_nft2_tx) + .expect("Owner of NFT can transfer NFT"); + + Ok(()) +} diff --git a/crates/iroha_executor/src/default/mod.rs b/crates/iroha_executor/src/default/mod.rs index 6fd629e88f..8b5b171b46 100644 --- a/crates/iroha_executor/src/default/mod.rs +++ b/crates/iroha_executor/src/default/mod.rs @@ -927,7 +927,11 @@ pub mod nft { use super::*; use crate::{ data_model::isi::BuiltInInstruction, - permission::{account::is_account_owner, nft::is_nft_owner, revoke_permissions}, + permission::{ + account::is_account_owner, + nft::{is_nft_full_owner, is_nft_weak_owner}, + revoke_permissions, + }, }; pub fn visit_register_nft(executor: &mut V, isi: &Register) { @@ -965,7 +969,7 @@ pub mod nft { let nft_id = isi.object(); if executor.context().curr_block.is_genesis() - || match is_nft_owner(nft_id, &executor.context().authority, executor.host()) { + || match is_nft_full_owner(nft_id, &executor.context().authority, executor.host()) { Err(err) => deny!(executor, err), Ok(is_owner) => is_owner, } @@ -1019,7 +1023,7 @@ pub mod nft { Ok(true) => execute!(executor, isi), Ok(false) => {} } - match is_nft_owner(nft_id, &executor.context().authority, executor.host()) { + match is_nft_weak_owner(nft_id, &executor.context().authority, executor.host()) { Err(err) => deny!(executor, err), Ok(true) => execute!(executor, isi), Ok(false) => {} @@ -1057,7 +1061,7 @@ pub mod nft { if executor.context().curr_block.is_genesis() { execute!(executor, isi); } - match is_nft_owner(nft_id, &executor.context().authority, executor.host()) { + match is_nft_full_owner(nft_id, &executor.context().authority, executor.host()) { Err(err) => deny!(executor, err), Ok(true) => execute!(executor, isi), Ok(false) => {} diff --git a/crates/iroha_executor/src/permission.rs b/crates/iroha_executor/src/permission.rs index 5119c2e87d..b7b6422142 100644 --- a/crates/iroha_executor/src/permission.rs +++ b/crates/iroha_executor/src/permission.rs @@ -593,9 +593,15 @@ pub mod asset_definition { ); } +/// Module with pass conditions for NFT related tokens +/// +/// - Owner of `nft.domain` can unregister, modify and transfer NFT +/// - Owner of NFT can only transfer NFT +/// +/// So: +/// - *full* owner - can unregister, modify and transfer NFT +/// - *weak* owner - can transfer NFT pub mod nft { - //! Module with pass conditions for NFT related tokens - use iroha_executor_data_model::permission::nft::{ CanModifyNftMetadata, CanRegisterNft, CanTransferNft, CanUnregisterNft, }; @@ -609,16 +615,18 @@ pub mod nft { Iroha, }; - /// Check if `authority` is the owner of NFT + /// Check if `authority` is *week* owner of NFT. /// - /// `authority` is owner of NFT if: + /// `authority` is *week* owner of NFT if: /// - `nft.owned_by` is `authority` /// - `nft.domain_id` domain is owned by `authority` /// + /// Also see [nft] module documentation. + /// /// # Errors /// - if `FindNfts` fails /// - if `is_domain_owner` fails - pub fn is_nft_owner(nft_id: &NftId, authority: &AccountId, host: &Iroha) -> Result { + pub fn is_nft_weak_owner(nft_id: &NftId, authority: &AccountId, host: &Iroha) -> Result { let nft = host .query(FindNfts) .filter_with(|nft| nft.id.eq(nft_id.clone())) @@ -636,20 +644,33 @@ pub mod nft { if nft.owned_by() == authority { Ok(true) } else { - domain::is_domain_owner(nft_id.domain(), authority, host) + is_nft_full_owner(nft_id, authority, host) } } - /// Pass condition that checks if `authority` is the owner of NFT. + /// Check if `authority` is *full* owner of NFT. + /// + /// `authority` is *full* owner of NFT if: + /// - `nft.domain_id` domain is owned by `authority` + /// + /// Also see [nft] module documentation. + /// + /// # Errors + /// - if `is_domain_owner` fails + pub fn is_nft_full_owner(nft_id: &NftId, authority: &AccountId, host: &Iroha) -> Result { + domain::is_domain_owner(nft_id.domain(), authority, host) + } + + /// Pass condition that checks if `authority` is the *weak* owner of NFT. #[derive(Debug, Clone)] - pub struct Owner<'nft> { + pub struct WeakOwner<'nft> { /// NFT id to check against pub nft: &'nft NftId, } - impl PassCondition for Owner<'_> { + impl PassCondition for WeakOwner<'_> { fn validate(&self, authority: &AccountId, host: &Iroha, _context: &Context) -> Result { - if is_nft_owner(self.nft, authority, host)? { + if is_nft_weak_owner(self.nft, authority, host)? { return Ok(()); } @@ -659,6 +680,25 @@ pub mod nft { } } + /// Pass condition that checks if `authority` is the *full* owner of NFT. + #[derive(Debug, Clone)] + pub struct FullOwner<'nft> { + /// NFT id to check against + pub nft: &'nft NftId, + } + + impl PassCondition for FullOwner<'_> { + fn validate(&self, authority: &AccountId, host: &Iroha, _context: &Context) -> Result { + if is_nft_full_owner(self.nft, authority, host)? { + return Ok(()); + } + + Err(ValidationFail::NotPermitted( + "Can't access NFT from domain owned by another account".to_owned(), + )) + } + } + impl ValidateGrantRevoke for CanRegisterNft { fn validate_grant(&self, authority: &AccountId, context: &Context, host: &Iroha) -> Result { super::domain::Owner::from(self).validate(authority, host, context) @@ -674,8 +714,8 @@ pub mod nft { } macro_rules! impl_froms_and_validate_grant_revoke { - ($($name:ty),+ $(,)?) => {$( - impl<'t> From<&'t $name> for Owner<'t> { + ($owner:ident : $($name:ty),+ $(,)?) => {$( + impl<'t> From<&'t $name> for $owner<'t> { fn from(value: &'t $name) -> Self { Self { nft: &value.nft } } @@ -683,7 +723,7 @@ pub mod nft { impl ValidateGrantRevoke for $name { fn validate_grant(&self, authority: &AccountId, context: &Context, host: &Iroha) -> Result { - Owner::from(self).validate(authority, host, context) + $owner::from(self).validate(authority, host, context) } fn validate_revoke( &self, @@ -691,13 +731,14 @@ pub mod nft { context: &Context, host: &Iroha, ) -> Result { - Owner::from(self).validate(authority, host, context) + $owner::from(self).validate(authority, host, context) } } )+}; } - impl_froms_and_validate_grant_revoke!(CanUnregisterNft, CanTransferNft, CanModifyNftMetadata); + impl_froms_and_validate_grant_revoke!(WeakOwner: CanTransferNft); + impl_froms_and_validate_grant_revoke!(FullOwner: CanUnregisterNft, CanModifyNftMetadata); } pub mod account {