From 31665e2789fddd5725269ccd0c771404172ec8b0 Mon Sep 17 00:00:00 2001 From: Nuno Alexandre Date: Tue, 26 May 2020 14:49:50 +0200 Subject: [PATCH] runtime: Unregister user if not an org member Also make the returned errors more granular, reflected in the test coverage. --- CHANGELOG.md | 1 + runtime-tests/tests/user_registration.rs | 94 +++++++++++++++++++----- runtime/src/registry.rs | 32 ++++---- 3 files changed, 93 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 551f31ba..b790cbe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Upcoming ### Breaking changes +* Only unregistered a user if not a member of any org * Tx author needs to have an associated registered user to operate on Orgs * `Org::members` is now `Vec` * client: `Client::new_emulator()` now returns a pair of a `Client` and diff --git a/runtime-tests/tests/user_registration.rs b/runtime-tests/tests/user_registration.rs index face8588..8601beb8 100644 --- a/runtime-tests/tests/user_registration.rs +++ b/runtime-tests/tests/user_registration.rs @@ -129,35 +129,93 @@ async fn unregister_user() { } #[async_std::test] -async fn unregister_user_with_invalid_sender() { +async fn unregister_user_member_of_an_org() { let (client, _) = Client::new_emulator(); - let alice = key_pair_from_string("Alice"); - let register_user_message = random_register_user_message(); + let (author, user_id) = key_pair_with_associated_user(&client).await; - // Reistration. - let tx_included = submit_ok(&client, &alice, register_user_message.clone()).await; - assert!(tx_included - .events - .contains(&RegistryEvent::UserRegistered(register_user_message.user_id.clone()).into())); - assert!(tx_included.result.is_ok()); + // Have user registering an org, which sets the associated user as its single member. + let register_org = random_register_org_message(); + submit_ok(&client, &author, register_org.clone()).await; + let org = client.get_org(register_org.org_id).await.unwrap().unwrap(); + assert_eq!(org.members, vec![user_id.clone()]); + + // Unregistration. + let initial_balance = client.free_balance(&author.public()).await.unwrap(); + + let unregister_user_message = message::UnregisterUser { + user_id: user_id.clone(), + }; + let random_fee = random_balance(); + let tx_unregister_applied = submit_ok_with_fee( + &client, + &author, + unregister_user_message.clone(), + random_fee, + ) + .await; + assert_eq!( + tx_unregister_applied.result, + Err(RegistryError::UnregisterableUser.into()) + ); assert!( - user_exists(&client, register_user_message.user_id.clone()).await, - "User not found in users list", + user_exists(&client, unregister_user_message.user_id.clone()).await, + "The user was expected to still exist" + ); + assert_eq!( + client.free_balance(&author.public()).await.unwrap(), + initial_balance - random_fee, + "The tx fee was not charged properly." ); +} - // Invalid unregistration. - let bad_actor = key_pair_from_string("BadActor"); - // The bad actor needs funds to submit transactions. - transfer(&client, &alice, bad_actor.public(), 1000).await; +#[async_std::test] +async fn unregister_user_with_invalid_sender() { + let (client, _) = Client::new_emulator(); + let (_, user_id) = key_pair_with_associated_user(&client).await; + // Invalid unregistration. + let (bad_actor, _) = key_pair_with_associated_user(&client).await; let unregister_user_message = message::UnregisterUser { - user_id: register_user_message.user_id.clone(), + user_id: user_id.clone(), }; let tx_unregister_applied = submit_ok(&client, &bad_actor, unregister_user_message.clone()).await; - assert!(tx_unregister_applied.result.is_err()); + + assert_eq!( + tx_unregister_applied.result, + Err(RegistryError::InsufficientSenderPermissions.into()) + ); assert!( - user_exists(&client, register_user_message.user_id.clone()).await, + user_exists(&client, user_id.clone()).await, "The user was expected to exist" ); } + +#[async_std::test] +async fn unregister_user_with_no_associated_user() { + let (client, _) = Client::new_emulator(); + let alice = key_pair_from_string("Alice"); + let initial_balance = client.free_balance(&alice.public()).await.unwrap(); + let unregister_user_message = message::UnregisterUser { + user_id: random_id(), + }; + + assert!( + !user_exists(&client, unregister_user_message.user_id.clone()).await, + "User should not exist", + ); + + let random_fee = random_balance(); + let tx_unregister_applied = + submit_ok_with_fee(&client, &alice, unregister_user_message.clone(), random_fee).await; + assert_eq!( + tx_unregister_applied.result, + Err(RegistryError::InexistentUser.into()) + ); + + assert_eq!( + client.free_balance(&alice.public()).await.unwrap(), + initial_balance - random_fee, + "The tx fee was not charged properly." + ); +} diff --git a/runtime/src/registry.rs b/runtime/src/registry.rs index 25203e8e..a5aa67a1 100644 --- a/runtime/src/registry.rs +++ b/runtime/src/registry.rs @@ -274,25 +274,19 @@ decl_module! { #[weight = SimpleDispatchInfo::InsecureFreeNormal] pub fn unregister_user(origin, message: message::UnregisterUser) -> DispatchResult { - fn can_be_unregistered(user: state::User, sender: AccountId) -> bool { - user.account_id == sender && user.projects.is_empty() - } - let sender = ensure_signed(origin)?; + let sender_user = get_user_with_account(sender).ok_or(RegistryError::InexistentUser)?; - match store::Users::get(message.user_id.clone()) { - None => Err(RegistryError::InexistentUser.into()), - Some(user) => { - if can_be_unregistered(user, sender) { - store::Users::remove(message.user_id.clone()); - Self::deposit_event(Event::UserUnregistered(message.user_id)); - Ok(()) - } - else { - Err(RegistryError::UnregisterableUser.into()) - } - } + if sender_user.id != message.user_id { + return Err(RegistryError::InsufficientSenderPermissions.into()); + } + if find_org(|org| org.members.contains(&sender_user.id)).is_some() { + return Err(RegistryError::UnregisterableUser.into()); } + + store::Users::remove(message.user_id.clone()); + Self::deposit_event(Event::UserUnregistered(message.user_id)); + Ok(()) } #[weight = SimpleDispatchInfo::InsecureFreeNormal] @@ -424,6 +418,12 @@ pub fn get_user_with_account(account_id: AccountId) -> Option { .map(|(id, user)| User::new(id, user)) } +pub fn find_org(predicate: impl Fn(&state::Org) -> bool) -> Option { + store::Orgs::iter() + .find(|(_, org)| predicate(org)) + .map(|(_, org)| org) +} + /// Check whether the user associated with the given account_id is a member of the given org. /// Return false if the account doesn't have an associated user or if said user is not a member /// of the org.