Root origin use no filter by default. Scheduler and Democracy dispatch without asserting BaseCallFilter (#6408)

* make system root origin build runtime origin with no filter

* additional doc

Author: gui1117

frame/democracy/src/tests.rs

@@ -22,7 +22,8 @@ use std::cell::RefCell;
 use codec::Encode;
 use frame_support::{
 	impl_outer_origin, impl_outer_dispatch, assert_noop, assert_ok, parameter_types,
-	impl_outer_event, ord_parameter_types, traits::{Contains, OnInitialize}, weights::Weight,
+	impl_outer_event, ord_parameter_types, traits::{Contains, OnInitialize, Filter},
+	weights::Weight,
 };
 use sp_core::H256;
 use sp_runtime::{
@@ -74,6 +75,14 @@ impl_outer_event! {
 	}
 }
 
+// Test that a fitlered call can be dispatched.
+pub struct BaseFilter;
+impl Filter<Call> for BaseFilter {
+	fn filter(call: &Call) -> bool {
+		!matches!(call, &Call::Balances(pallet_balances::Call::set_balance(..)))
+	}
+}
+
 // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct Test;
@@ -84,7 +93,7 @@ parameter_types! {
 	pub const AvailableBlockRatio: Perbill = Perbill::one();
 }
 impl frame_system::Trait for Test {
-	type BaseCallFilter = ();
+	type BaseCallFilter = BaseFilter;
 	type Origin = Origin;
 	type Index = u64;
 	type BlockNumber = u64;
@@ -225,6 +234,14 @@ fn set_balance_proposal(value: u64) -> Vec<u8> {
 	Call::Balances(pallet_balances::Call::set_balance(42, value, 0)).encode()
 }
 
+#[test]
+fn set_balance_proposal_is_correctly_filtered_out() {
+	for i in 0..10 {
+		let call = Call::decode(&mut &set_balance_proposal(i)[..]).unwrap();
+		assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
+	}
+}
+
 fn set_balance_proposal_hash(value: u64) -> H256 {
 	BlakeTwo256::hash(&set_balance_proposal(value)[..])
 }

frame/scheduler/src/lib.rs

@@ -399,7 +399,7 @@ mod tests {
 
 	use frame_support::{
 		impl_outer_event, impl_outer_origin, impl_outer_dispatch, parameter_types, assert_ok,
-		traits::{OnInitialize, OnFinalize},
+		traits::{OnInitialize, OnFinalize, Filter},
 		weights::constants::RocksDbWeight,
 	};
 	use sp_core::H256;
@@ -469,6 +469,15 @@ mod tests {
 			scheduler<T>,
 		}
 	}
+
+	// Scheduler must dispatch with root and no filter, this tests base filter is indeed not used.
+	pub struct BaseFilter;
+	impl Filter<Call> for BaseFilter {
+		fn filter(call: &Call) -> bool {
+			!matches!(call, Call::Logger(_))
+		}
+	}
+
 	// For testing the pallet, we construct most of a mock runtime. This means
 	// first constructing a configuration type (`Test`) which `impl`s each of the
 	// configuration traits of pallets we want to use.
@@ -481,7 +490,7 @@ mod tests {
 		pub const AvailableBlockRatio: Perbill = Perbill::one();
 	}
 	impl system::Trait for Test {
-		type BaseCallFilter = ();
+		type BaseCallFilter = BaseFilter;
 		type Origin = Origin;
 		type Call = Call;
 		type Index = u64;
@@ -540,7 +549,9 @@ mod tests {
 	#[test]
 	fn basic_scheduling_works() {
 		new_test_ext().execute_with(|| {
-			Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(42, 1000)));
+			let call = Call::Logger(logger::Call::log(42, 1000));
+			assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
+			Scheduler::do_schedule(4, None, 127, call);
 			run_to_block(3);
 			assert!(logger::log().is_empty());
 			run_to_block(4);

frame/support/src/origin.rs

@@ -163,8 +163,8 @@ macro_rules! impl_outer_origin {
 		Modules { };
 		$( $module:ident $( < $generic:ident > )? $( { $generic_instance:ident } )? ,)*
 	) => {
-		// WARNING: All instance must hold the filter `frame_system::Trait::BaseCallFilter`.
-		// One can use `OriginTrait::reset_filter` to do so.
+		// WARNING: All instance must hold the filter `frame_system::Trait::BaseCallFilter`, except
+		// when caller is system Root. One can use `OriginTrait::reset_filter` to do so.
 		#[derive(Clone)]
 		pub struct $name {
 			caller: $caller_name,
@@ -241,28 +241,40 @@ macro_rules! impl_outer_origin {
 
 		#[allow(dead_code)]
 		impl $name {
+			/// Create with system none origin and `frame-system::Trait::BaseCallFilter`.
 			pub fn none() -> Self {
 				$system::RawOrigin::None.into()
 			}
+			/// Create with system root origin and no filter.
 			pub fn root() -> Self {
 				$system::RawOrigin::Root.into()
 			}
+			/// Create with system signed origin and `frame-system::Trait::BaseCallFilter`.
 			pub fn signed(by: <$runtime as $system::Trait>::AccountId) -> Self {
 				$system::RawOrigin::Signed(by).into()
 			}
 		}
 
 		impl From<$system::Origin<$runtime>> for $name {
+			/// Convert to runtime origin:
+			/// * root origin is built with no filter
+			/// * others use `frame-system::Trait::BaseCallFilter`
 			fn from(x: $system::Origin<$runtime>) -> Self {
 				let mut o = $name {
 					caller: $caller_name::system(x),
 					filter: $crate::sp_std::rc::Rc::new(Box::new(|_| true)),
 				};
-				$crate::traits::OriginTrait::reset_filter(&mut o);
+
+				// Root has no filter
+				if !matches!(o.caller, $caller_name::system($system::Origin::<$runtime>::Root)) {
+					$crate::traits::OriginTrait::reset_filter(&mut o);
+				}
+
 				o
 			}
 		}
 		impl Into<$crate::sp_std::result::Result<$system::Origin<$runtime>, $name>> for $name {
+			/// NOTE: converting to pallet origin loses the origin filter information.
 			fn into(self) -> $crate::sp_std::result::Result<$system::Origin<$runtime>, Self> {
 				if let $caller_name::system(l) = self.caller {
 					Ok(l)
@@ -272,13 +284,16 @@ macro_rules! impl_outer_origin {
 			}
 		}
 		impl From<Option<<$runtime as $system::Trait>::AccountId>> for $name {
+			/// Convert to runtime origin with caller being system signed or none and use filter
+			/// `frame-system::Trait::BaseCallFilter`.
 			fn from(x: Option<<$runtime as $system::Trait>::AccountId>) -> Self {
 				<$system::Origin<$runtime>>::from(x).into()
 			}
 		}
 		$(
 			$crate::paste::item! {
 				impl From<$module::Origin < $( $generic )? $(, $module::$generic_instance )? > > for $name {
+					/// Convert to runtime origin using `frame-system::Trait::BaseCallFilter`.
 					fn from(x: $module::Origin < $( $generic )? $(, $module::$generic_instance )? >) -> Self {
 						let mut o = $name {
 							caller: $caller_name::[< $module $( _ $generic_instance )? >](x),
@@ -294,6 +309,7 @@ macro_rules! impl_outer_origin {
 						$name,
 					>>
 				for $name {
+					/// NOTE: converting to pallet origin loses the origin filter information.
 					fn into(self) -> $crate::sp_std::result::Result<
 						$module::Origin < $( $generic )? $(, $module::$generic_instance )? >,
 						Self,
@@ -402,7 +418,7 @@ mod tests {
 	#[test]
 	fn test_default_filter() {
 		assert_eq!(OriginWithSystem::root().filter_call(&0), true);
-		assert_eq!(OriginWithSystem::root().filter_call(&1), false);
+		assert_eq!(OriginWithSystem::root().filter_call(&1), true);
 		assert_eq!(OriginWithSystem::none().filter_call(&0), true);
 		assert_eq!(OriginWithSystem::none().filter_call(&1), false);
 		assert_eq!(OriginWithSystem::signed(0).filter_call(&0), true);

frame/system/src/lib.rs

@@ -149,7 +149,8 @@ pub fn extrinsics_data_root<H: Hash>(xts: Vec<Vec<u8>>) -> H::Output {
 }
 
 pub trait Trait: 'static + Eq + Clone {
-	/// The basic call filter to use in Origin.
+	/// The basic call filter to use in Origin. All origins are built with this filter as base,
+	/// except Root.
 	type BaseCallFilter: Filter<Self::Call>;
 
 	/// The `Origin` type used by dispatchable calls.