diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs index fb3f77fbb97981f89b97784ffa7d9c1b9cea97e7..7ae48e6f49fe0651e9635ff01d19cb4d0d6ccea7 100644 --- a/pallets/authority-members/src/lib.rs +++ b/pallets/authority-members/src/lib.rs @@ -215,13 +215,10 @@ pub mod pallet { #[pallet::call] impl<T: Config> Pallet<T> { #[pallet::weight(0)] - pub fn go_offline( - origin: OriginFor<T>, - member_id: T::MemberId, - ) -> DispatchResultWithPostInfo { + pub fn go_offline(origin: OriginFor<T>) -> DispatchResultWithPostInfo { // Verification phase // let who = ensure_signed(origin)?; - Self::verify_ownership_and_membership(&who, member_id)?; + let member_id = Self::verify_ownership_and_membership(&who)?; if !Members::<T>::contains_key(member_id) { return Err(Error::<T>::MemberNotFound.into()); @@ -244,13 +241,10 @@ pub mod pallet { Ok(().into()) } #[pallet::weight(0)] - pub fn go_online( - origin: OriginFor<T>, - member_id: T::MemberId, - ) -> DispatchResultWithPostInfo { + pub fn go_online(origin: OriginFor<T>) -> DispatchResultWithPostInfo { // Verification phase // let who = ensure_signed(origin)?; - Self::verify_ownership_and_membership(&who, member_id)?; + let member_id = Self::verify_ownership_and_membership(&who)?; if !Members::<T>::contains_key(member_id) { return Err(Error::<T>::MemberNotFound.into()); @@ -285,11 +279,10 @@ pub mod pallet { #[pallet::weight(0)] pub fn set_session_keys( origin: OriginFor<T>, - member_id: T::MemberId, keys: T::KeysWrapper, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin.clone())?; - Self::verify_ownership_and_membership(&who, member_id)?; + let member_id = Self::verify_ownership_and_membership(&who)?; let _post_info = pallet_session::Call::<T>::set_keys { keys: keys.into(), @@ -332,11 +325,10 @@ pub mod pallet { #[pallet::weight(0)] pub fn purge_old_session_keys( origin: OriginFor<T>, - member_id: T::MemberId, accounts_ids: Vec<T::AccountId>, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - Self::verify_ownership_and_membership(&who, member_id)?; + let member_id = Self::verify_ownership_and_membership(&who)?; for account_id in accounts_ids { if !T::IsMember::is_member(&member_id) { @@ -476,21 +468,15 @@ pub mod pallet { } fn verify_ownership_and_membership( who: &T::AccountId, - member_id: T::MemberId, - ) -> Result<(), DispatchError> { - if let Some(expected_member_id) = T::MemberIdOf::convert(who.clone()) { - if expected_member_id != member_id { - return Err(Error::<T>::NotOwner.into()); - } - } else { - return Err(Error::<T>::MemberIdNotFound.into()); - } + ) -> Result<T::MemberId, DispatchError> { + let member_id = + T::MemberIdOf::convert(who.clone()).ok_or(Error::<T>::MemberIdNotFound)?; if !T::IsMember::is_member(&member_id) { return Err(Error::<T>::NotMember.into()); } - Ok(()) + Ok(member_id) } } } diff --git a/pallets/authority-members/src/tests.rs b/pallets/authority-members/src/tests.rs index f92c27e059a82ad254cd3fcddab47e564bbe212f..94cfd0a191e3b8ca21b3ef7649eaa0a6b22d4daf 100644 --- a/pallets/authority-members/src/tests.rs +++ b/pallets/authority-members/src/tests.rs @@ -79,12 +79,10 @@ fn test_max_keys_life_rule() { // Member 3 and 6 rotate their sessions keys assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(3), - 3, UintAuthorityId(3).into() ),); assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(6), - 6, UintAuthorityId(6).into() ),); @@ -129,7 +127,7 @@ fn test_go_offline() { run_to_block(1); // Member 9 should be able to go offline - assert_ok!(AuthorityMembers::go_offline(Origin::signed(9), 9),); + assert_ok!(AuthorityMembers::go_offline(Origin::signed(9)),); // Verify state assert_eq!(AuthorityMembers::incoming(), EMPTY); @@ -181,7 +179,6 @@ fn test_go_online() { // Member 12 should be able to set his session keys assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(12), - 12, UintAuthorityId(12).into(), )); assert_eq!( @@ -193,7 +190,7 @@ fn test_go_online() { ); // Member 12 should be able to go online - assert_ok!(AuthorityMembers::go_online(Origin::signed(12), 12),); + assert_ok!(AuthorityMembers::go_online(Origin::signed(12)),); // Verify state assert_eq!(AuthorityMembers::incoming(), vec![12]); @@ -232,28 +229,26 @@ fn test_too_many_authorities() { // Member 12 set his session keys then go online assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(12), - 12, UintAuthorityId(12).into(), )); assert_eq!(AuthorityMembers::authorities_counter(), 3); - assert_ok!(AuthorityMembers::go_online(Origin::signed(12), 12),); + assert_ok!(AuthorityMembers::go_online(Origin::signed(12)),); // Member 15 can't go online because there is already 4 authorities "planned" assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(15), - 15, UintAuthorityId(15).into(), )); assert_eq!(AuthorityMembers::authorities_counter(), 4); assert_err!( - AuthorityMembers::go_online(Origin::signed(15), 15), + AuthorityMembers::go_online(Origin::signed(15)), Error::<Test>::TooManyAuthorities, ); // If member 3 go_offline, member 15 can go_online - assert_ok!(AuthorityMembers::go_offline(Origin::signed(3), 3),); + assert_ok!(AuthorityMembers::go_offline(Origin::signed(3)),); assert_eq!(AuthorityMembers::authorities_counter(), 3); - assert_ok!(AuthorityMembers::go_online(Origin::signed(15), 15),); + assert_ok!(AuthorityMembers::go_online(Origin::signed(15)),); assert_eq!(AuthorityMembers::authorities_counter(), 4); }); } @@ -266,16 +261,15 @@ fn test_go_online_then_go_offline_in_same_session() { // Member 12 set his session keys & go online assert_ok!(AuthorityMembers::set_session_keys( Origin::signed(12), - 12, UintAuthorityId(12).into(), )); - assert_ok!(AuthorityMembers::go_online(Origin::signed(12), 12),); + assert_ok!(AuthorityMembers::go_online(Origin::signed(12)),); run_to_block(2); // Member 12 should be able to go offline at the same session to "cancel" his previous // action - assert_ok!(AuthorityMembers::go_offline(Origin::signed(12), 12),); + assert_ok!(AuthorityMembers::go_offline(Origin::signed(12)),); // Verify state assert_eq!(AuthorityMembers::incoming(), EMPTY); @@ -297,12 +291,12 @@ fn test_go_offline_then_go_online_in_same_session() { run_to_block(6); // Member 9 go offline - assert_ok!(AuthorityMembers::go_offline(Origin::signed(9), 9),); + assert_ok!(AuthorityMembers::go_offline(Origin::signed(9)),); run_to_block(7); // Member 9 should be able to go online at the same session to "cancel" his previous action - assert_ok!(AuthorityMembers::go_online(Origin::signed(9), 9),); + assert_ok!(AuthorityMembers::go_online(Origin::signed(9)),); // Verify state assert_eq!(AuthorityMembers::incoming(), EMPTY); diff --git a/pallets/duniter-wot/src/lib.rs b/pallets/duniter-wot/src/lib.rs index b2e70823fcdba1244c992ee813b776a9ddfcc087..2f1d90fb34604bc0e087d7f31232d1517aa0b227 100644 --- a/pallets/duniter-wot/src/lib.rs +++ b/pallets/duniter-wot/src/lib.rs @@ -137,7 +137,7 @@ where && cert_meta.issued_count < T::MaxByIssuer::get() } fn can_confirm_identity(idty_index: IdtyIndex, owner_key: AccountId) -> bool { - pallet_membership::Pallet::<T, I>::request_membership( + pallet_membership::Pallet::<T, I>::force_request_membership( RawOrigin::Root.into(), idty_index, MembershipMetaData(owner_key), diff --git a/pallets/duniter-wot/src/tests.rs b/pallets/duniter-wot/src/tests.rs index 8d420ccee6b9f29053b2bea1c7f2fa2e1c94ea2e..99fb108313835aedc711ef21f6c5cc30738cdffa 100644 --- a/pallets/duniter-wot/src/tests.rs +++ b/pallets/duniter-wot/src/tests.rs @@ -235,7 +235,6 @@ fn test_idty_membership_expire_them_requested() { run_to_block(6); assert_ok!(Membership::request_membership( Origin::signed(3), - 3, crate::MembershipMetaData(3) )); diff --git a/pallets/membership/src/lib.rs b/pallets/membership/src/lib.rs index ee6b19d28d1dbeb42e18918aeb1564e361ccfe14..505fb1fc1a26bec820707e6a7c7d6cccb894c6e9 100644 --- a/pallets/membership/src/lib.rs +++ b/pallets/membership/src/lib.rs @@ -226,53 +226,33 @@ pub mod pallet { #[pallet::call] impl<T: Config<I>, I: 'static> Pallet<T, I> { #[pallet::weight(0)] - pub fn request_membership( + pub fn force_request_membership( origin: OriginFor<T>, idty_id: T::IdtyId, metadata: T::MetaData, ) -> DispatchResultWithPostInfo { - let is_root = match origin.into() { - Ok(RawOrigin::Root) => true, - Ok(RawOrigin::Signed(account_id)) => { - if let Some(expected_idty_id) = T::IdtyIdOf::convert(account_id.clone()) { - if idty_id != expected_idty_id { - return Err(BadOrigin.into()); - } else if !metadata.validate(&account_id) { - return Err(Error::<T, I>::InvalidMetaData.into()); - } - } else { - return Err(Error::<T, I>::IdtyIdNotFound.into()); - } - false - } - _ => return Err(BadOrigin.into()), - }; - if PendingMembership::<T, I>::contains_key(&idty_id) { - return Err(Error::<T, I>::MembershipAlreadyRequested.into()); - } - if Membership::<T, I>::contains_key(&idty_id) { - return Err(Error::<T, I>::MembershipAlreadyAcquired.into()); - } - if RevokedMembership::<T, I>::contains_key(&idty_id) { - return Err(Error::<T, I>::MembershipRevokedRecently.into()); + ensure_root(origin)?; + + Self::do_request_membership(idty_id, metadata) + } + + #[pallet::weight(0)] + pub fn request_membership( + origin: OriginFor<T>, + metadata: T::MetaData, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + + let idty_id = T::IdtyIdOf::convert(who.clone()).ok_or(Error::<T, I>::IdtyIdNotFound)?; + if !metadata.validate(&who) { + return Err(Error::<T, I>::InvalidMetaData.into()); } - if !is_root - && !T::IsIdtyAllowedToRequestMembership::is_idty_allowed_to_request_membership( - &idty_id, - ) + if !T::IsIdtyAllowedToRequestMembership::is_idty_allowed_to_request_membership(&idty_id) { return Err(Error::<T, I>::IdtyNotAllowedToRequestMembership.into()); } - let block_number = frame_system::pallet::Pallet::<T>::block_number(); - let expire_on = block_number + T::PendingMembershipPeriod::get(); - - PendingMembership::<T, I>::insert(idty_id, metadata); - PendingMembershipsExpireOn::<T, I>::append(expire_on, idty_id); - Self::deposit_event(Event::MembershipRequested(idty_id)); - T::OnEvent::on_event(&sp_membership::Event::MembershipRequested(idty_id)); - - Ok(().into()) + Self::do_request_membership(idty_id, metadata) } #[pallet::weight(0)] @@ -363,6 +343,30 @@ pub mod pallet { MembershipsExpireOn::<T, I>::append(expire_on, idty_id); 0 } + fn do_request_membership( + idty_id: T::IdtyId, + metadata: T::MetaData, + ) -> DispatchResultWithPostInfo { + if PendingMembership::<T, I>::contains_key(&idty_id) { + return Err(Error::<T, I>::MembershipAlreadyRequested.into()); + } + if Membership::<T, I>::contains_key(&idty_id) { + return Err(Error::<T, I>::MembershipAlreadyAcquired.into()); + } + if RevokedMembership::<T, I>::contains_key(&idty_id) { + return Err(Error::<T, I>::MembershipRevokedRecently.into()); + } + + let block_number = frame_system::pallet::Pallet::<T>::block_number(); + let expire_on = block_number + T::PendingMembershipPeriod::get(); + + PendingMembership::<T, I>::insert(idty_id, metadata); + PendingMembershipsExpireOn::<T, I>::append(expire_on, idty_id); + Self::deposit_event(Event::MembershipRequested(idty_id)); + T::OnEvent::on_event(&sp_membership::Event::MembershipRequested(idty_id)); + + Ok(().into()) + } pub(super) fn do_revoke_membership(idty_id: T::IdtyId) -> Weight { Self::remove_membership(&idty_id); if T::RevocationPeriod::get() > Zero::zero() { diff --git a/pallets/membership/src/tests.rs b/pallets/membership/src/tests.rs index 7ff7ab9eb18eda6e99497a9a24af3958bdd95099..89b015fc7739e56cdb875d20fe81b4cec85d689b 100644 --- a/pallets/membership/src/tests.rs +++ b/pallets/membership/src/tests.rs @@ -131,17 +131,13 @@ fn test_membership_revocation() { // Membership 0 can't request membership before the end of RevokePeriod (1 + 4 = 5) run_to_block(2); assert_eq!( - DefaultMembership::request_membership(Origin::signed(0), 0, ()), + DefaultMembership::request_membership(Origin::signed(0), ()), Err(Error::<Test, _>::MembershipRevokedRecently.into()) ); // Membership 0 can request membership after the end of RevokePeriod (1 + 4 = 5) run_to_block(5); - assert_ok!(DefaultMembership::request_membership( - Origin::signed(0), - 0, - () - ),); + assert_ok!(DefaultMembership::request_membership(Origin::signed(0), ()),); assert_eq!( System::events()[0].event, RuntimeEvent::DefaultMembership(Event::MembershipRequested(0)) @@ -154,11 +150,7 @@ fn test_pending_membership_expiration() { new_test_ext(Default::default()).execute_with(|| { // Idty 0 request membership run_to_block(1); - assert_ok!(DefaultMembership::request_membership( - Origin::signed(0), - 0, - () - ),); + assert_ok!(DefaultMembership::request_membership(Origin::signed(0), ()),); assert_eq!( System::events()[0].event, RuntimeEvent::DefaultMembership(Event::MembershipRequested(0)) @@ -183,11 +175,7 @@ fn test_membership_workflow() { new_test_ext(Default::default()).execute_with(|| { // Idty 0 request membership run_to_block(1); - assert_ok!(DefaultMembership::request_membership( - Origin::signed(0), - 0, - () - ),); + assert_ok!(DefaultMembership::request_membership(Origin::signed(0), ()),); assert_eq!( System::events()[0].event, RuntimeEvent::DefaultMembership(Event::MembershipRequested(0)) diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs index 59ec8b28e343e127669b21691f0e598089cb1af0..cfffc01cdcaf09f47c9ad28b7fc9884787f7ddb5 100644 --- a/runtime/common/src/handlers.rs +++ b/runtime/common/src/handlers.rs @@ -77,7 +77,7 @@ impl< ) -> Weight { (match membership_event { sp_membership::Event::MembershipAcquired( - idty_index, + _idty_index, SmithsMembershipMetaData { owner_key, session_keys, @@ -85,7 +85,6 @@ impl< }, ) => { let call = pallet_authority_members::Call::<Runtime>::set_session_keys { - member_id: *idty_index, keys: session_keys.clone(), }; if let Err(e) = call.dispatch_bypass_filter(