Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Root origin always bypass all filter, other origin cannot bypass BaseCallFilter even when constructed from Root origin #9948

Merged
7 commits merged into from
Oct 7, 2021

Conversation

thiolliere
Copy link
Contributor

Currently when an origin caller is constructed from root then it has no filter, this allowed to have root origin bypassing filters.
But:

  • when the caller was then set to another non root origin, then the origin still had no filter.
  • when the filter were reset with reset_filter then the origin had the BaseCallFilter regardless of being root or not
  • when an origin was constructed from a non-root origin and then had its caller switch to root, then the filter were still effective and the origin wasn't able to bypass filter even if it was root.

Now those behavior are "fixed", an origin with a root caller always bypass filter, and if the origin caller is changed this property still hold.
So any origin will have BaseCallFilter (and the added one if some were added) and if the origin caller is root then all filters are bypassed.

Idea from @basti,
cc @athei

@thiolliere thiolliere added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 6, 2021
@thiolliere thiolliere added this to In progress in Runtime via automation Oct 6, 2021
@thiolliere thiolliere moved this from In progress to Please Review in Runtime Oct 6, 2021
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

LGTM

// when caller is system Root. One can use `OriginTrait::reset_filter` to do so.
/// The runtime origin type represanting the origin of a call.
///
/// Origin is always created with the base filter configured in `frame_system::Config::BaseCallFilter`.
Copy link
Member

Choose a reason for hiding this comment

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

Using proper intra doc links here would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do in a follow up when I get more time #9962

@@ -140,7 +141,9 @@ pub fn expand_outer_origin(
}

fn filter_call(&self, call: &Self::Call) -> bool {
(self.filter)(call)
// Root bypasses all filters
matches!(self.caller, OriginCaller::system(#system_path::Origin::<#runtime>::Root))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matches!(self.caller, OriginCaller::system(#system_path::Origin::<#runtime>::Root))
match self.caller { OriginCaller::system(#system_path::Origin::<#runtime>::Root) => true, _ => false }

Or use an absolute path to matches! 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.

indeed, done in 84a9ac0

Runtime automation moved this from Please Review to Needs Audit Oct 6, 2021
@thiolliere
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 7, 2021

Waiting for commit status.

@ghost ghost merged commit 7dbcd4f into master Oct 7, 2021
@ghost ghost deleted the gui-improve-root-filterbypass branch October 7, 2021 15:46
Runtime automation moved this from Needs Audit to Done Oct 7, 2021
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Nov 11, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants