[fix] #4331: Revoke removed PermissionTokens on Upgrade<Executor>

client/tests/integration/smartcontracts/Cargo.toml

@@ -13,6 +13,7 @@ members = [
     "mint_rose_trigger",
     "executor_with_admin",
     "executor_with_custom_token",
+    "executor_remove_token",
     "executor_with_migration_fail",
     "query_assets_and_save_cursor",
 ]

client/tests/integration/smartcontracts/executor_remove_token/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "executor_remove_token"
+
+edition.workspace = true
+version.workspace = true
+authors.workspace = true
+
+license.workspace = true
+
+[lib]
+crate-type = ['cdylib']
+
+[dependencies]
+iroha_executor.workspace = true
+iroha_schema.workspace = true
+
+parity-scale-codec.workspace = true
+anyhow.workspace = true
+serde_json.workspace = true
+serde.workspace = true
+
+panic-halt.workspace = true
+lol_alloc.workspace = true
+getrandom.workspace = true

client/tests/integration/smartcontracts/executor_remove_token/src/lib.rs

@@ -0,0 +1,37 @@
+//! Runtime Executor which removes [`token::CanControlDomainLives`] permission token.
+//! Needed for tests.
+
+#![no_std]
+
+extern crate alloc;
+#[cfg(not(test))]
+extern crate panic_halt;
+
+use iroha_executor::{default::default_permission_token_schema, prelude::*};
+use lol_alloc::{FreeListAllocator, LockedAllocator};
+
+#[global_allocator]
+static ALLOC: LockedAllocator<FreeListAllocator> = LockedAllocator::new(FreeListAllocator::new());
+
+getrandom::register_custom_getrandom!(iroha_executor::stub_getrandom);
+
+#[derive(Constructor, ValidateEntrypoints, Validate, Visit)]
+struct Executor {
+    verdict: Result,
+    block_height: u64,
+}
+
+#[entrypoint]
+pub fn migrate(_block_height: u64) -> MigrationResult {
+    // Note that actually migration will reset token schema to default (minus `CanUnregisterDomain`)
+    // So any added custom permission tokens will be also removed
+    let mut schema = default_permission_token_schema();
+    schema.remove::<iroha_executor::default::tokens::domain::CanUnregisterDomain>();
+
+    let (token_ids, schema_str) = schema.serialize();
+    iroha_executor::set_permission_token_schema(
+        &iroha_executor::data_model::permission::PermissionTokenSchema::new(token_ids, schema_str),
+    );
+
+    Ok(())
+}

client/tests/integration/upgrade.rs

@@ -113,6 +113,76 @@ fn executor_upgrade_should_run_migration() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn executor_upgrade_should_revoke_removed_permissions() -> Result<()> {
+    let (_rt, _peer, client) = <PeerBuilder>::new().with_port(11_030).start_with_runtime();
+    wait_for_genesis_committed(&vec![client.clone()], 0);
+
+    // Permission which will be removed by executor
+    let can_unregister_domain_token = PermissionToken::new(
+        "CanUnregisterDomain".parse()?,
+        &json!({ "domain_id": DomainId::from_str("wonderland")? }),
+    );
+
+    // Register `TEST_ROLE` with permission
+    let test_role_id: RoleId = "TEST_ROLE".parse()?;
+    let test_role =
+        Role::new(test_role_id.clone()).add_permission(can_unregister_domain_token.clone());
+    client.submit_blocking(Register::role(test_role))?;
+
+    // Check that permission exists
+    assert!(client
+        .request(FindPermissionTokenSchema)?
+        .token_ids()
+        .contains(&can_unregister_domain_token.definition_id));
+
+    // Check that `TEST_ROLE` has permission
+    assert!(client
+        .request(FindAllRoles::new())?
+        .collect::<QueryResult<Vec<_>>>()?
+        .into_iter()
+        .find(|role| role.id == test_role_id)
+        .expect("Failed to find Role")
+        .permissions
+        .contains(&can_unregister_domain_token));
+
+    // Check that Alice has permission
+    let alice_id: AccountId = "alice@wonderland".parse()?;
+    assert!(client
+        .request(FindPermissionTokensByAccountId::new(alice_id.clone()))?
+        .collect::<QueryResult<Vec<_>>>()?
+        .contains(&can_unregister_domain_token));
+
+    upgrade_executor(
+        &client,
+        "tests/integration/smartcontracts/executor_remove_token",
+    )?;
+
+    // Check that permission doesn't exist
+    assert!(!client
+        .request(FindPermissionTokenSchema)?
+        .token_ids()
+        .contains(&can_unregister_domain_token.definition_id));
+
+    // Check that `TEST_ROLE` doesn't have permission
+    assert!(!client
+        .request(FindAllRoles::new())?
+        .collect::<QueryResult<Vec<_>>>()?
+        .into_iter()
+        .find(|role| role.id == test_role_id)
+        .expect("Failed to find Role")
+        .permissions
+        .contains(&can_unregister_domain_token));
+
+    // Check that Alice doesn't have permission
+    assert!(!client
+        .request(FindPermissionTokensByAccountId::new(alice_id.clone()))?
+        .collect::<QueryResult<Vec<_>>>()?
+        .contains(&can_unregister_domain_token));
+
+    Ok(())
+}
+
 #[test]
 fn migration_fail_should_not_cause_any_effects() {
     let (_rt, _peer, client) = <PeerBuilder>::new().with_port(10_995).start_with_runtime();

core/src/smartcontracts/isi/world.rs

@@ -18,6 +18,7 @@ impl Registrable for NewRole {
 /// Iroha Special Instructions that have `World` as their target.
 pub mod isi {
     use eyre::Result;
+    use indexmap::IndexSet;
     use iroha_data_model::{
         isi::error::{InstructionExecutionError, InvalidParameterError, RepetitionError},
         prelude::*,
@@ -345,6 +346,12 @@ pub mod isi {
         ) -> Result<(), Error> {
             let raw_executor = self.executor;
 
+            let permissions_before = state_transaction
+                .world
+                .permission_token_schema
+                .token_ids
+                .clone();
+
             // Cloning executor to avoid multiple mutable borrows of `state_transaction`.
             // Also it's a cheap operation.
             let mut upgraded_executor = state_transaction.world.executor.clone();
@@ -359,6 +366,8 @@ pub mod isi {
 
             *state_transaction.world.executor.get_mut() = upgraded_executor;
 
+            revoke_removed_permissions(authority, state_transaction, permissions_before)?;
+
             state_transaction
                 .world
                 .emit_events(std::iter::once(ExecutorEvent::Upgraded));
@@ -367,6 +376,71 @@ pub mod isi {
         }
     }
 
+    fn revoke_removed_permissions(
+        authority: &AccountId,
+        state_transaction: &mut StateTransaction,
+        permissions_before: Vec<PermissionTokenId>,
+    ) -> Result<(), Error> {
+        let world = state_transaction.world();
+        let permissions_after = world
+            .permission_token_schema()
+            .token_ids
+            .iter()
+            .collect::<IndexSet<_>>();
+        let permissions_removed = permissions_before
+            .into_iter()
+            .filter(|permission| !permissions_after.contains(permission))
+            .collect::<IndexSet<_>>();
+        if permissions_removed.is_empty() {
+            return Ok(());
+        }
+
+        let to_revoke_from_accounts = find_related_accounts(world, &permissions_removed);
+        let to_revoke_from_roles = find_related_roles(world, &permissions_removed);
+
+        for (account_id, permission) in to_revoke_from_accounts {
+            let revoke = Revoke::permission(permission, account_id);
+            revoke.execute(authority, state_transaction)?
+        }
+        for (role_id, permission) in to_revoke_from_roles {
+            let revoke = Revoke::role_permission(permission, role_id);
+            revoke.execute(authority, state_transaction)?
+        }
+        Ok(())
+    }
+
+    fn find_related_accounts(
+        world: &impl WorldReadOnly,
+        permissions: &IndexSet<PermissionTokenId>,
+    ) -> Vec<(AccountId, PermissionToken)> {
+        world
+            .account_permission_tokens()
+            .iter()
+            .flat_map(|(account_id, account_permissions)| {
+                account_permissions
+                    .iter()
+                    .filter(|permission| permissions.contains(&permission.definition_id))
+                    .map(|permission| (account_id.clone(), permission.clone()))
+            })
+            .collect()
+    }
+
+    fn find_related_roles(
+        world: &impl WorldReadOnly,
+        permissions: &IndexSet<PermissionTokenId>,
+    ) -> Vec<(RoleId, PermissionToken)> {
+        world
+            .roles()
+            .iter()
+            .flat_map(|(role_id, role)| {
+                role.permissions
+                    .iter()
+                    .filter(|permission| permissions.contains(&permission.definition_id))
+                    .map(|permission| (role_id.clone(), permission.clone()))
+            })
+            .collect()
+    }
+
     impl Execute for Log {
         fn execute(
             self,

data_model/src/isi.rs

@@ -1276,7 +1276,7 @@ isi_box! {
         PermissionToken(Revoke<PermissionToken, Account>),
         /// Revoke [`Role`] from [`Account`].
         Role(Revoke<RoleId, Account>),
-        /// Revoke [`PermissionToken`] from [`Account`].
+        /// Revoke [`PermissionToken`] from [`Role`].
         RolePermissionToken(Revoke<PermissionToken, Role>),
     }
 }