Review fixes: Fix NFT permissions

Signed-off-by: Dmitry Murzin <diralik@yandex.ru>

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(())
+}

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<V: Execute + Visit + ?Sized>(executor: &mut V, isi: &Register<Nft>) {
@@ -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) => {}

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<bool> {
+    pub fn is_nft_weak_owner(nft_id: &NftId, authority: &AccountId, host: &Iroha) -> Result<bool> {
         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<bool> {
+        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,30 +714,31 @@ 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 }
                 }
             }
 
             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,
                     authority: &AccountId,
                     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 {