Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement access control for AccountStore members #3204

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d9c713d
using single storage unbounded instead of pallet::without_storage_info
silva-fj Dec 12, 2024
b7d0f00
adding integrity_test hook
silva-fj Dec 12, 2024
2a7e934
adding new types to the pallet config
silva-fj Dec 12, 2024
caf080f
adding storage item for account member permissions
silva-fj Dec 12, 2024
24e1fdf
adding new errors
silva-fj Dec 12, 2024
82a36d6
storing default permission on create_account_store
silva-fj Dec 12, 2024
1516f2e
adding comments
silva-fj Dec 12, 2024
eada06f
adding default permissions
silva-fj Dec 12, 2024
dc9a0f1
small refactoring
silva-fj Dec 12, 2024
7d6241b
implementing ensure_permission
silva-fj Dec 12, 2024
eb1eaaa
updating mock
silva-fj Dec 12, 2024
824e982
adding optional permissions to add_account, set defaults if None
silva-fj Dec 12, 2024
2ef4b14
setting permissions on update_account_store_by_one
silva-fj Dec 13, 2024
16bd79e
improving ensure_permission
silva-fj Dec 13, 2024
0a16d34
updating mock
silva-fj Dec 13, 2024
efce874
refactoring add_account_call util
silva-fj Dec 13, 2024
d673c04
adding tests for permissions
silva-fj Dec 13, 2024
ae2444b
updating mock
silva-fj Dec 13, 2024
3454462
adding Permission type to runtimes
silva-fj Dec 13, 2024
5b36919
updating permissions mock and tests
silva-fj Dec 13, 2024
7d0663c
adding set_permissions call
silva-fj Dec 13, 2024
44bc0b1
adjusting ensure_permission
silva-fj Dec 13, 2024
0984565
updating mock
silva-fj Dec 13, 2024
e04a8c0
updating permission filters in the runtimes
silva-fj Dec 13, 2024
63784d0
adding tests for set_permissions call
silva-fj Dec 13, 2024
a893cd6
adding temporary fix for add_account
silva-fj Dec 13, 2024
8cabe70
update storage on remove accounts
silva-fj Dec 14, 2024
ade319b
setting default permission when empty, only if the sender has default
silva-fj Dec 16, 2024
0a29a43
Adjusting worker to allow managing AccountStore member permissions (#…
silva-fj Dec 17, 2024
77b71a5
Merge branch 'dev' into p-1240-implement-access-control-for-accountst…
silva-fj Dec 17, 2024
35786aa
Merge branch 'dev' into p-1240-implement-access-control-for-accountst…
BillyWooo Dec 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/primitives/core/src/omni_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,13 @@ impl OmniAccountConverter for DefaultOmniAccountConverter {
identity.to_omni_account()
}
}

// This type must be kept in sync with the `Permission` type used in the runtime.
#[derive(Encode, Decode, Clone, Debug, PartialEq, Eq)]
pub enum OmniAccountPermission {
All,
AccountManagement,
RequestNativeIntent,
RequestEthereumIntent,
RequestSolanaIntent,
}
143 changes: 139 additions & 4 deletions parachain/pallets/omni-account/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ pub use pallet::*;
use frame_support::pallet_prelude::*;
use frame_support::{
dispatch::{GetDispatchInfo, PostDispatchInfo},
traits::{IsSubType, UnfilteredDispatchable},
traits::{InstanceFilter, IsSubType, UnfilteredDispatchable},
};
use frame_system::pallet_prelude::*;
use sp_core::H256;
use sp_runtime::traits::Dispatchable;
use sp_std::boxed::Box;
use sp_std::vec::Vec;
use sp_std::{vec, vec::Vec};

pub type MemberCount = u32;

Expand Down Expand Up @@ -66,7 +66,6 @@ pub mod pallet {

#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
#[pallet::without_storage_info]
pub struct Pallet<T>(_);

#[pallet::config]
Expand Down Expand Up @@ -104,6 +103,36 @@ pub mod pallet {

/// Convert an `Identity` to OmniAccount type
type OmniAccountConverter: OmniAccountConverter<OmniAccount = Self::AccountId>;

/// The permissions that a member account can have
/// The instance filter determines whether a given call may can be dispatched under this type.
///
/// IMPORTANT: `Default` must be provided and MUST BE the the *most permissive* value.
type Permission: Parameter
+ Member
+ Ord
+ PartialOrd
+ Default
+ InstanceFilter<<Self as Config>::RuntimeCall>
+ MaxEncodedLen;

/// The maximum number of permissions that a member account can have
#[pallet::constant]
type MaxPermissions: Get<u32>;
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn integrity_test() {
assert!(
<T as Config>::MaxAccountStoreLength::get() > 0,
"MaxAccountStoreLength must be greater than 0"
);
assert!(
<T as Config>::MaxPermissions::get() > 0,
"MaxPermissions must be greater than 0"
);
}
}

pub type MemberAccounts<T> = BoundedVec<MemberAccount, <T as Config>::MaxAccountStoreLength>;
Expand All @@ -113,6 +142,7 @@ pub mod pallet {

/// A map between OmniAccount and its MemberAccounts (a bounded vector of MemberAccount)
#[pallet::storage]
#[pallet::unbounded]
#[pallet::getter(fn account_store)]
pub type AccountStore<T: Config> =
StorageMap<Hasher = Blake2_128Concat, Key = T::AccountId, Value = MemberAccounts<T>>;
Expand All @@ -122,6 +152,21 @@ pub mod pallet {
pub type MemberAccountHash<T: Config> =
StorageMap<Hasher = Blake2_128Concat, Key = H256, Value = T::AccountId>;

#[pallet::type_value]
pub fn DefaultPermissions<T: Config>() -> BoundedVec<T::Permission, T::MaxPermissions> {
BoundedVec::try_from(vec![T::Permission::default()]).expect("default permission")
}

/// A map between hash of MemberAccount and its permissions
#[pallet::storage]
pub type MemberAccountPermissions<T: Config> = StorageMap<
Hasher = Blake2_128Concat,
Key = H256,
Value = BoundedVec<T::Permission, T::MaxPermissions>,
QueryKind = ValueQuery,
OnEmpty = DefaultPermissions<T>,
>;

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config> {
Expand All @@ -143,6 +188,8 @@ pub mod pallet {
IntentRequested { who: T::AccountId, intent: Intent },
/// Intent is executed
IntentExecuted { who: T::AccountId, intent: Intent, result: IntentExecutionResult },
/// Member permission set
AccountPermissionsSet { who: T::AccountId, member_account_hash: H256 },
}

#[pallet::error]
Expand All @@ -153,6 +200,8 @@ pub mod pallet {
InvalidAccount,
UnknownAccountStore,
EmptyAccount,
NoPermission,
PermissionsLenLimitReached,
}

#[pallet::call]
Expand All @@ -168,6 +217,7 @@ pub mod pallet {
let _ = T::TEECallOrigin::ensure_origin(origin)?;
let omni_account = MemberAccountHash::<T>::get(member_account_hash)
.ok_or(Error::<T>::AccountNotFound)?;
Self::ensure_permission(call.as_ref(), member_account_hash)?;
let result = call.dispatch(RawOrigin::OmniAccount(omni_account.clone()).into());
system::Pallet::<T>::inc_account_nonce(&omni_account);
Self::deposit_event(Event::DispatchedAsOmniAccount {
Expand All @@ -189,6 +239,7 @@ pub mod pallet {
let _ = T::TEECallOrigin::ensure_origin(origin)?;
let omni_account = MemberAccountHash::<T>::get(member_account_hash)
.ok_or(Error::<T>::AccountNotFound)?;
Self::ensure_permission(call.as_ref(), member_account_hash)?;
let result: Result<
PostDispatchInfo,
sp_runtime::DispatchErrorWithPostInfo<PostDispatchInfo>,
Expand Down Expand Up @@ -217,7 +268,8 @@ pub mod pallet {
#[pallet::weight((195_000_000, DispatchClass::Normal))]
pub fn add_account(
origin: OriginFor<T>,
member_account: MemberAccount, // account to be added
member_account: MemberAccount, // account to be added
permissions: Option<Vec<T::Permission>>, // permissions for the account
) -> DispatchResult {
// mutation of AccountStore requires `OmniAccountOrigin`, same as "remove" and "publicize"
let who = T::OmniAccountOrigin::ensure_origin(origin)?;
Expand All @@ -233,8 +285,16 @@ pub mod pallet {
member_accounts
.try_push(member_account)
.map_err(|_| Error::<T>::AccountStoreLenLimitReached)?;
let member_permissions: BoundedVec<T::Permission, T::MaxPermissions> = permissions
.map_or_else(
|| vec![T::Permission::default()],
|p| if p.is_empty() { vec![T::Permission::default()] } else { p },
)
.try_into()
.map_err(|_| Error::<T>::PermissionsLenLimitReached)?;

MemberAccountHash::<T>::insert(hash, who.clone());
MemberAccountPermissions::<T>::insert(hash, member_permissions);
AccountStore::<T>::insert(who.clone(), member_accounts.clone());

Self::deposit_event(Event::AccountAdded {
Expand All @@ -261,6 +321,7 @@ pub mod pallet {
member_accounts.retain(|member| {
if member_account_hashes.contains(&member.hash()) {
MemberAccountHash::<T>::remove(member.hash());
MemberAccountPermissions::<T>::remove(member.hash());
false
} else {
true
Expand Down Expand Up @@ -336,8 +397,13 @@ pub mod pallet {
.try_push(member_account.clone())
.map_err(|_| Error::<T>::AccountStoreLenLimitReached)?;
}
let mut permissions = BoundedVec::<T::Permission, T::MaxPermissions>::new();
permissions
.try_push(T::Permission::default())
.map_err(|_| Error::<T>::PermissionsLenLimitReached)?;

MemberAccountHash::<T>::insert(member_account.hash(), who_account.clone());
MemberAccountPermissions::<T>::insert(member_account.hash(), permissions);
AccountStore::<T>::insert(who_account.clone(), member_accounts.clone());
Self::deposit_event(Event::AccountStoreUpdated {
who: who_account,
Expand All @@ -359,6 +425,21 @@ pub mod pallet {
Self::deposit_event(Event::IntentExecuted { who, intent, result });
Ok(())
}

#[pallet::call_index(9)]
#[pallet::weight((195_000_000, DispatchClass::Normal))]
pub fn set_permissions(
origin: OriginFor<T>,
member_account_hash: H256,
permissions: Vec<T::Permission>,
) -> DispatchResult {
let who = T::OmniAccountOrigin::ensure_origin(origin)?;
let member_permissions: BoundedVec<T::Permission, T::MaxPermissions> =
{ permissions.try_into().map_err(|_| Error::<T>::PermissionsLenLimitReached)? };
MemberAccountPermissions::<T>::insert(member_account_hash, member_permissions);
Self::deposit_event(Event::AccountPermissionsSet { who, member_account_hash });
Ok(())
}
}

impl<T: Config> Pallet<T> {
Expand All @@ -384,8 +465,13 @@ pub mod pallet {
member_accounts
.try_push(identity.into())
.map_err(|_| Error::<T>::AccountStoreLenLimitReached)?;
let mut permissions = BoundedVec::<T::Permission, T::MaxPermissions>::new();
permissions
.try_push(T::Permission::default())
.map_err(|_| Error::<T>::PermissionsLenLimitReached)?;

MemberAccountHash::<T>::insert(hash, omni_account.clone());
MemberAccountPermissions::<T>::insert(hash, permissions);
AccountStore::<T>::insert(omni_account.clone(), member_accounts.clone());

Self::deposit_event(Event::AccountStoreCreated { who: omni_account.clone() });
Expand All @@ -396,6 +482,55 @@ pub mod pallet {

Ok(member_accounts)
}

fn ensure_permission(
call: &<T as Config>::RuntimeCall,
member_account_hash: H256,
) -> Result<(), Error<T>> {
let member_permissions = MemberAccountPermissions::<T>::get(member_account_hash);

ensure!(
member_permissions.iter().any(|permission| permission.filter(call)),
Error::<T>::NoPermission
);

match call.is_sub_type() {
Some(Call::add_account { permissions: ref new_account_permissions, .. }) => {
// If member has default permission, they can add accounts with any permission
if member_permissions.contains(&T::Permission::default()) {
return Ok(());
}
match new_account_permissions {
Some(new_permissions) => {
// an account can only add another account with the same or less permissions
if new_permissions.is_empty()
|| !new_permissions.iter().all(|p| member_permissions.contains(p))
{
return Err(Error::<T>::NoPermission);
}
},
None => {
// None is equivalent to default permission. It should not be allowed
// if the member_permissions have no default permission
return Err(Error::<T>::NoPermission);
},
}
},
Some(Call::set_permissions { permissions: ref new_permissions, .. }) => {
// If member has default permission, they can set permissions to any value
if member_permissions.contains(&T::Permission::default()) {
return Ok(());
}
// an account can only set permissions to the same or less permissions
if !new_permissions.iter().all(|p| member_permissions.contains(p)) {
return Err(Error::<T>::NoPermission);
}
},
_ => return Ok(()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: is it better to return error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning Err means the member account does not have permissions, but that's not the case if it reaches this point.

The permissions for the call are checked at the beginning of the method, the extra checks for add_account and set_permissions are to make sure the caller cannot set permissions or add an account with more permissions than it (the caller) has. For all the other calls the permissions have already been checked.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I ask is: ensure_permission is only called by add_account and set_permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_permissions is called on dispatch_as_signed and dispatch_as_omni_account (in this case the call to dispatch could be add_account or set_permissions for example)

}

Ok(())
}
}
}

Expand Down
Loading
Loading