From 386e4d846c7e5c836a29295dcb793f73c2790a07 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=89lo=C3=AFs?= <c@elo.tf>
Date: Sun, 11 Sep 2022 18:10:51 +0200
Subject: [PATCH] fix(identity): should be able to revoke idty with old key
before ChangeOwnerKeyPeriod (!110)
* test(identity): test revocation with old key
* fix(identity) should be able to revoke idty with old key
* fix(identity): old revoc key should expire after ChangeOwnerKeyPeriod
old revoc key should expire after ChangeOwnerKeyPeriod blocks
---
pallets/identity/src/lib.rs | 9 ++-
pallets/identity/src/tests.rs | 102 +++++++++++++++++++++++++++++++---
2 files changed, 101 insertions(+), 10 deletions(-)
diff --git a/pallets/identity/src/lib.rs b/pallets/identity/src/lib.rs
index fea93ed3c..07c81a031 100644
--- a/pallets/identity/src/lib.rs
+++ b/pallets/identity/src/lib.rs
@@ -495,8 +495,11 @@ pub mod pallet {
let idty_value = Identities::<T>::get(idty_index).ok_or(Error::<T>::IdtyNotFound)?;
ensure!(
- if let Some((ref old_owner_key, _)) = idty_value.old_owner_key {
- revocation_key == idty_value.owner_key || &revocation_key == old_owner_key
+ if let Some((ref old_owner_key, last_change)) = idty_value.old_owner_key {
+ revocation_key == idty_value.owner_key
+ || (&revocation_key == old_owner_key
+ && frame_system::Pallet::<T>::block_number()
+ < last_change + T::ChangeOwnerKeyPeriod::get())
} else {
revocation_key == idty_value.owner_key
},
@@ -516,7 +519,7 @@ pub mod pallet {
ensure!(
(REVOCATION_PAYLOAD_PREFIX, revocation_payload)
- .using_encoded(|bytes| revocation_sig.verify(bytes, &idty_value.owner_key)),
+ .using_encoded(|bytes| revocation_sig.verify(bytes, &revocation_key)),
Error::<T>::InvalidRevocationSig
);
diff --git a/pallets/identity/src/tests.rs b/pallets/identity/src/tests.rs
index ef3f2dd19..5e3aa5a08 100644
--- a/pallets/identity/src/tests.rs
+++ b/pallets/identity/src/tests.rs
@@ -20,8 +20,7 @@ use crate::{
NEW_OWNER_KEY_PAYLOAD_PREFIX, REVOCATION_PAYLOAD_PREFIX,
};
use codec::Encode;
-//use frame_support::assert_noop;
-use frame_support::{assert_err, assert_ok};
+use frame_support::{assert_noop, assert_ok};
use frame_system::{EventRecord, Phase};
use sp_runtime::testing::TestSignature;
@@ -212,7 +211,7 @@ fn test_change_owner_key() {
assert_eq!(System::sufficients(&10), 0);
// Caller should have an associated identity
- assert_err!(
+ assert_noop!(
Identity::change_owner_key(
Origin::signed(42),
10,
@@ -222,7 +221,7 @@ fn test_change_owner_key() {
);
// Payload must be signed by the new key
- assert_err!(
+ assert_noop!(
Identity::change_owner_key(
Origin::signed(1),
10,
@@ -232,7 +231,7 @@ fn test_change_owner_key() {
);
// Payload must be prefixed
- assert_err!(
+ assert_noop!(
Identity::change_owner_key(
Origin::signed(1),
10,
@@ -242,7 +241,7 @@ fn test_change_owner_key() {
);
// New owner key should not be used by another identity
- assert_err!(
+ assert_noop!(
Identity::change_owner_key(
Origin::signed(1),
2,
@@ -277,7 +276,7 @@ fn test_change_owner_key() {
// Alice can't re-change her owner key too early
new_key_payload.old_owner_key = &10;
- assert_err!(
+ assert_noop!(
Identity::change_owner_key(
Origin::signed(10),
100,
@@ -330,6 +329,95 @@ fn test_change_owner_key() {
});
}
+#[test]
+fn test_idty_revocation_with_old_key() {
+ new_test_ext(IdentityConfig {
+ identities: vec![alice()],
+ })
+ .execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let new_key_payload = NewOwnerKeyPayload {
+ genesis_hash: &genesis_hash,
+ idty_index: 1u64,
+ old_owner_key: &1u64,
+ };
+ let revocation_payload = RevocationPayload {
+ idty_index: 1u64,
+ genesis_hash,
+ };
+
+ // We need to initialize at least one block before any call
+ run_to_block(1);
+
+ // Change alice owner key
+ assert_ok!(Identity::change_owner_key(
+ Origin::signed(1),
+ 10,
+ TestSignature(10, (NEW_OWNER_KEY_PAYLOAD_PREFIX, new_key_payload).encode())
+ ));
+ assert!(Identity::identity(&1).is_some());
+ let idty_val = Identity::identity(&1).unwrap();
+ assert_eq!(idty_val.owner_key, 10);
+ assert_eq!(idty_val.old_owner_key, Some((1, 1)));
+
+ // We should be able to revoke Alice identity with old key
+ run_to_block(2);
+ assert_ok!(Identity::revoke_identity(
+ Origin::signed(42),
+ 1,
+ 1,
+ TestSignature(1, (REVOCATION_PAYLOAD_PREFIX, revocation_payload).encode())
+ ));
+
+ //run_to_block(2 + <Test as crate::Config>::ChangeOwnerKeyPeriod::get());
+ });
+}
+
+#[test]
+fn test_idty_revocation_with_old_key_after_old_key_expiration() {
+ new_test_ext(IdentityConfig {
+ identities: vec![alice()],
+ })
+ .execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let new_key_payload = NewOwnerKeyPayload {
+ genesis_hash: &genesis_hash,
+ idty_index: 1u64,
+ old_owner_key: &1u64,
+ };
+ let revocation_payload = RevocationPayload {
+ idty_index: 1u64,
+ genesis_hash,
+ };
+
+ // We need to initialize at least one block before any call
+ run_to_block(1);
+
+ // Change alice owner key
+ assert_ok!(Identity::change_owner_key(
+ Origin::signed(1),
+ 10,
+ TestSignature(10, (NEW_OWNER_KEY_PAYLOAD_PREFIX, new_key_payload).encode())
+ ));
+ assert!(Identity::identity(&1).is_some());
+ let idty_val = Identity::identity(&1).unwrap();
+ assert_eq!(idty_val.owner_key, 10);
+ assert_eq!(idty_val.old_owner_key, Some((1, 1)));
+
+ // We should not be able to revoke Alice identity with old key after ChangeOwnerKeyPeriod
+ run_to_block(2 + <Test as crate::Config>::ChangeOwnerKeyPeriod::get());
+ assert_noop!(
+ Identity::revoke_identity(
+ Origin::signed(42),
+ 1,
+ 1,
+ TestSignature(1, (REVOCATION_PAYLOAD_PREFIX, revocation_payload).encode())
+ ),
+ Error::<Test>::InvalidRevocationKey
+ );
+ });
+}
+
#[test]
fn test_idty_revocation() {
new_test_ext(IdentityConfig {
--
GitLab