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

feat(policy): filter statements subset by selector #27

Closed
wants to merge 4 commits into from

Conversation

fabiobozzo
Copy link
Collaborator

Requirement: splitting the policy evaluation into different chunks.

capability.Policy has now a Match method (refactored from standalone function) and a Filter method which allows to filter policy statements by Selector.

goos: darwin
goarch: arm64
pkg: github.com/ucan-wg/go-ucan/capability/policy
BenchmarkPolicyFilter
BenchmarkPolicyFilter/Filter_by_sel1
BenchmarkPolicyFilter/Filter_by_sel1-12         	35227159	        33.80 ns/op
BenchmarkPolicyFilter/Filter_by_sel2
BenchmarkPolicyFilter/Filter_by_sel2-12         	35496963	        33.83 ns/op
BenchmarkPolicyFilter/Filter_by_sel3
BenchmarkPolicyFilter/Filter_by_sel3-12         	100000000	        10.74 ns/op
PASS

Comment on lines 92 to 95
// assuming the first statement's selector is representative
if len(c.statements) > 0 {
return c.statements[0].Selector()
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MichaelMure I'm not sure about this 💭

Comment on lines 59 to 60
// NewFieldSegment creates a new segment for a field.
func NewFieldSegment(field string) segment {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MichaelMure we don't use all those constructors atm. But I needed to deal w/ segment from the parent pkg (tests) and didn't want to make that type public. wdyt

func (c connective) Selector() selector.Selector {
// assuming the first statement's selector is representative
if len(c.statements) > 0 {
return c.statements[0].Selector()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right to me...

In general I don't really have enough context in the PR description to understand why this is necessary, plus I'm not sure I understand why policy statements need to have a selector now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is arguably off-spec (and very exploratory for now), but the idea is that when you pass a UCAN as a HTTP bearer (the off-spec part), you likely want to do some or all of the following:

  • include "external args" (http.method, http.path, ...) into your normal invocation args, and have the policies operate on those elements
  • evaluate the policies in multiple stages in your server because different aspects are handled in different part of the pipeline (some args might come from the HTTP header, some from the body ...)

To be clear, all this HTTP bearer features are not going to be in go-ucan, but this specific facility in this PR might come handy even outside of this scenario, for example when you want to sub-delegate on a subset of the original policies. TBD.

That HTTP bearer thing might become a UCAN extension in the future.

require.NoError(t, err)

p = Policy{stmt1, stmt2}
filtered = p.FilterWithMatcher(selector.Selector{selector.NewFieldSegment(".http")}, selector.SegmentStartsWith)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MichaelMure here .http "startsWith" selector matches .http.host statement.

MichaelMure added a commit that referenced this pull request Oct 16, 2024
MichaelMure added a commit that referenced this pull request Oct 16, 2024
MichaelMure added a commit that referenced this pull request Oct 16, 2024
MichaelMure added a commit that referenced this pull request Oct 16, 2024
@MichaelMure
Copy link
Collaborator

closing in favor of #44

MichaelMure added a commit that referenced this pull request Oct 16, 2024
MichaelMure added a commit that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants