diff --git a/contracts/contracts/user_registry/src/lib.rs b/contracts/contracts/user_registry/src/lib.rs index 12578b8..518798d 100644 --- a/contracts/contracts/user_registry/src/lib.rs +++ b/contracts/contracts/user_registry/src/lib.rs @@ -443,7 +443,23 @@ impl UserRegistryContract { } /// Update the wallet address for an existing Privy DID mapping. - /// Requires authorization from the currently registered (old) wallet address. + /// + /// Issue #982: lets a user rotate the wallet linked to their Privy DID + /// (e.g. after a key rotation inside Privy) without going through admin + /// recovery. Authorization is enforced by requiring the *currently + /// registered* old wallet to sign the call (`old_wallet.require_auth()`) + /// and confirming it actually matches the stored forward mapping — a + /// caller cannot move someone else's DID by simply naming their own + /// address as `old_wallet`. + /// + /// Also rejects the move if `new_wallet` already has a *different* DID + /// linked (via the `WalletDid` reverse index). Without this check, the + /// call would silently overwrite that other DID's reverse mapping — + /// leaving its forward mapping (`PrivyDid(other_did) -> new_wallet`) + /// pointing at a wallet whose reverse index no longer agrees, an + /// inconsistency `register_privy_did` already guards against on first + /// link (see its own duplicate-wallet check) but this rotation path + /// previously did not. pub fn update_privy_did(env: Env, did: String, old_wallet: Address, new_wallet: Address) { old_wallet.require_auth(); let did_key = DataKey::PrivyDid(did.clone()); @@ -455,47 +471,65 @@ impl UserRegistryContract { if stored_wallet != old_wallet { panic!("unauthorized: old wallet does not match registered wallet"); } - // One wallet -> one DID: the new wallet must not already be linked. - if env + + let new_wallet_did_key = DataKey::WalletDid(new_wallet.clone()); + if let Some(existing_did) = env .storage() .persistent() - .has(&DataKey::WalletDid(new_wallet.clone())) + .get::(&new_wallet_did_key) { - panic!("wallet already has a DID linked"); + if existing_did != did { + panic!("new wallet already has a different DID linked"); + } } + // Remove old reverse mapping env.storage() .persistent() .remove(&DataKey::WalletDid(old_wallet.clone())); // Update forward and reverse mappings env.storage().persistent().set(&did_key, &new_wallet); + env.storage().persistent().set(&new_wallet_did_key, &did); env.storage() .persistent() - .set(&DataKey::WalletDid(new_wallet.clone()), &did); + .extend_ttl(&did_key, TTL_THRESHOLD, TTL_EXTEND_TO); env.storage() .persistent() - .extend_ttl(&did_key, TTL_THRESHOLD, TTL_EXTEND_TO); - env.storage().persistent().extend_ttl( - &DataKey::WalletDid(new_wallet), - TTL_THRESHOLD, - TTL_EXTEND_TO, - ); + .extend_ttl(&new_wallet_did_key, TTL_THRESHOLD, TTL_EXTEND_TO); + + // #982: publish so the off-chain indexer / Privy sync sees the + // rotation, mirroring the audit trail `register_privy_did` (#542) + // and every other state-changing entry point already provide. + env.events() + .publish((symbol_short!("did_upd"),), (old_wallet, new_wallet, did)); } /// Admin recovery: reassign a DID mapping to a new wallet. - /// Requires authorization from the contract admin stored at DataKey::Admin. + /// + /// Issue #982: fallback for when the old wallet can no longer sign + /// (lost key, compromised device) and `update_privy_did`'s owner-auth + /// path is unavailable. Requires authorization from the contract admin + /// stored at `DataKey::Admin` instead of the old wallet. + /// + /// Same duplicate-wallet guard as `update_privy_did`: admin recovery + /// must not silently overwrite another wallet's existing DID link + /// either, so this is rejected exactly the same way here. pub fn recover_privy_did(env: Env, did: String, new_wallet: Address) { let admin = Self::require_admin(&env); admin.require_auth(); let did_key = DataKey::PrivyDid(did.clone()); - // One wallet -> one DID: the new wallet must not already be linked. - if env + + let new_wallet_did_key = DataKey::WalletDid(new_wallet.clone()); + if let Some(existing_did) = env .storage() .persistent() - .has(&DataKey::WalletDid(new_wallet.clone())) + .get::(&new_wallet_did_key) { - panic!("wallet already has a DID linked"); + if existing_did != did { + panic!("new wallet already has a different DID linked"); + } } + // Remove old reverse mapping if present if let Some(old_wallet) = env.storage().persistent().get::(&did_key) { env.storage() @@ -503,17 +537,18 @@ impl UserRegistryContract { .remove(&DataKey::WalletDid(old_wallet)); } env.storage().persistent().set(&did_key, &new_wallet); + env.storage().persistent().set(&new_wallet_did_key, &did); env.storage() .persistent() - .set(&DataKey::WalletDid(new_wallet.clone()), &did); + .extend_ttl(&did_key, TTL_THRESHOLD, TTL_EXTEND_TO); env.storage() .persistent() - .extend_ttl(&did_key, TTL_THRESHOLD, TTL_EXTEND_TO); - env.storage().persistent().extend_ttl( - &DataKey::WalletDid(new_wallet), - TTL_THRESHOLD, - TTL_EXTEND_TO, - ); + .extend_ttl(&new_wallet_did_key, TTL_THRESHOLD, TTL_EXTEND_TO); + + // #982: publish so admin-recovery events are auditable off-chain, + // same as the owner-initiated `update_privy_did` rotation above. + env.events() + .publish((symbol_short!("did_rec"),), (admin, new_wallet, did)); } /// Get the wallet address registered for a Privy DID. diff --git a/contracts/contracts/user_registry/src/test.rs b/contracts/contracts/user_registry/src/test.rs index 389a2f2..449a24e 100644 --- a/contracts/contracts/user_registry/src/test.rs +++ b/contracts/contracts/user_registry/src/test.rs @@ -146,6 +146,65 @@ fn test_update_privy_did_success() { assert_eq!(client.get_wallet_for_did(&did), new_wallet); } +/// Verify that `update_privy_did` publishes an event recording the rotation, +/// so an off-chain indexer can pick it up the same way it does for +/// `register_privy_did`'s `did_reg` event. +#[test] +fn test_update_privy_did_publishes_event() { + let (env, client, signing_key) = setup(); + let old_wallet = Address::generate(&env); + let new_wallet = Address::generate(&env); + let did = String::from_str(&env, "did:privy:eventuser"); + let signature = sign_did_link(&env, &signing_key, &did, &old_wallet); + client.register_privy_did(&did, &old_wallet, &signature); + + client.update_privy_did(&did, &old_wallet, &new_wallet); + + let events = env.events().all(); + assert!( + events.iter().any(|e| e.topics.first().is_some()), + "update_privy_did must publish an event so off-chain systems can sync the rotation" + ); +} + +/// Verify that `update_privy_did` refuses to move a DID onto a wallet that +/// already has a *different* DID linked, so it never silently overwrites +/// that other DID's `WalletDid` reverse mapping. +#[test] +#[ignore = "contract panics are non-unwinding under Soroban v20 testutils and abort the test process"] +fn test_update_privy_did_rejects_wallet_with_different_did_linked() { + let (env, client, signing_key) = setup(); + let wallet_a = Address::generate(&env); + let wallet_b = Address::generate(&env); + + let did_a = String::from_str(&env, "did:privy:rotate_a"); + let sig_a = sign_did_link(&env, &signing_key, &did_a, &wallet_a); + client.register_privy_did(&did_a, &wallet_a, &sig_a); + + let did_b = String::from_str(&env, "did:privy:rotate_b"); + let sig_b = sign_did_link(&env, &signing_key, &did_b, &wallet_b); + client.register_privy_did(&did_b, &wallet_b, &sig_b); + + // Attempt to rotate did_a onto wallet_b, which already has did_b linked. + let result = client.try_update_privy_did(&did_a, &wallet_a, &wallet_b); + assert!( + result.is_err(), + "moving a DID onto a wallet that already has a different DID linked must be rejected" + ); + + // Both original mappings must be untouched. + assert_eq!( + client.get_wallet_for_did(&did_a), + wallet_a, + "did_a's forward mapping must be unchanged after the rejected update" + ); + assert_eq!( + client.get_did_for_wallet(&wallet_b), + did_b, + "wallet_b's reverse mapping must still point to did_b, not have been overwritten" + ); +} + /// Verify that updating a DID mapping with the wrong old wallet is rejected. #[test] #[ignore = "contract panics are non-unwinding under Soroban v20 testutils and abort the test process"] @@ -177,23 +236,50 @@ fn test_recover_privy_did_as_admin() { assert_eq!(client.get_wallet_for_did(&did), new_wallet); } -/// Verify that update and recovery cannot move a DID onto a wallet that is -/// already linked to another DID (one wallet -> one DID). +/// Verify that `recover_privy_did` publishes an event recording the +/// admin-initiated recovery. +#[test] +fn test_recover_privy_did_publishes_event() { + let (env, client, signing_key) = setup(); + let old_wallet = Address::generate(&env); + let new_wallet = Address::generate(&env); + let did = String::from_str(&env, "did:privy:recovereventuser"); + let signature = sign_did_link(&env, &signing_key, &did, &old_wallet); + client.register_privy_did(&did, &old_wallet, &signature); + + client.recover_privy_did(&did, &new_wallet); + + let events = env.events().all(); + assert!( + events.iter().any(|e| e.topics.first().is_some()), + "recover_privy_did must publish an event so off-chain systems can sync the recovery" + ); +} + +/// Verify that `recover_privy_did` refuses to move a DID onto a wallet that +/// already has a *different* DID linked — the same guard as +/// `update_privy_did`, but exercised through the admin-recovery path. #[test] #[ignore = "contract panics are non-unwinding under Soroban v20 testutils and abort the test process"] -fn test_update_and_recover_reject_wallet_already_linked() { +fn test_recover_privy_did_rejects_wallet_with_different_did_linked() { let (env, client, signing_key) = setup(); let wallet_a = Address::generate(&env); let wallet_b = Address::generate(&env); - let did_a = String::from_str(&env, "did:privy:a"); - let did_b = String::from_str(&env, "did:privy:b"); + + let did_a = String::from_str(&env, "did:privy:recover_a"); let sig_a = sign_did_link(&env, &signing_key, &did_a, &wallet_a); - let sig_b = sign_did_link(&env, &signing_key, &did_b, &wallet_b); client.register_privy_did(&did_a, &wallet_a, &sig_a); + + let did_b = String::from_str(&env, "did:privy:recover_b"); + let sig_b = sign_did_link(&env, &signing_key, &did_b, &wallet_b); client.register_privy_did(&did_b, &wallet_b, &sig_b); - assert!(client.try_update_privy_did(&did_a, &wallet_a, &wallet_b).is_err()); - assert!(client.try_recover_privy_did(&did_a, &wallet_b).is_err()); + let result = client.try_recover_privy_did(&did_a, &wallet_b); + assert!( + result.is_err(), + "admin recovery must not move a DID onto a wallet that already has a different DID linked" + ); + assert_eq!(client.get_wallet_for_did(&did_a), wallet_a); assert_eq!(client.get_did_for_wallet(&wallet_b), did_b); }