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

Feature/global auth session #358

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Conversation

Sbou
Copy link
Contributor

@Sbou Sbou commented Feb 12, 2020

Related issue

#292

Discussed with @aeneasr

Proposed changes

The AuthenticationSession is now global to all the life of the request handler.
The signature changes to:

type AuthenticationSession struct {
	Subject      string                 `json:"subject"`
	Extra        map[string]interface{} `json:"extra"`
	Header       http.Header            `json:"header"`
	MatchContext MatchContext           `json:"matchContext"`
}

type MatchContext struct {
	RegexpCaptureGroups []string `json:"regexpCaptureGroups"`
	URL                 *url.URL `json:"url"`
}

So it's now possible to use it in all authorizers or mutators.
For example with mutator_id_token

{"claims": "{\"custom-claim\": \"{{ print .Extra.abc }}\", \"custom-claim2\": \"{{ index .MatchContext.RegexpCaptureGroups 0 }}\", \"aud\": [\"foo\", \"bar\"]}"}

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

Should we keep compatibility with the ReplaceAllString syntax in the config of the keto_engine_acp_ory authorizer?

New syntax

{
  "handler": "keto_engine_acp_ory",
  "config": {
    "required_action": "my:action:{{ index .MatchContext.RegexpCaptureGroups 0 }}",
    "required_resource": "my:resource:{{ index .MatchContext.RegexpCaptureGroups 1 }}:foo:{{ index .MatchContext.RegexpCaptureGroups 0 }}"
  }
}

@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2020

Hey, I haven't missed this - just a lot to do at the moment. Will review this in the next days!

@aeneasr aeneasr self-requested a review February 13, 2020 17:17
@aeneasr aeneasr self-assigned this Feb 13, 2020
@aeneasr aeneasr added the feat New feature or request. label Feb 13, 2020
@aeneasr aeneasr added this to the v0.36.0 milestone Feb 13, 2020
@aeneasr aeneasr linked an issue Feb 13, 2020 that may be closed by this pull request
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Great job! This looks really solid! Before merging we need to update the docs also and (if I'm correct) and there's a breaking change update the rule migrator as well.

The docs should probably receive updates:

pipeline/authn/authenticator.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator.go Outdated Show resolved Hide resolved
pipeline/mutate/template.go Outdated Show resolved Hide resolved
pipeline/mutate/template.go Outdated Show resolved Hide resolved
@@ -135,6 +143,7 @@ func TestMutatorHydrator(t *testing.T) {
"foo": "hello",
"bar": 3.14,
}
sampleCaptureGroups := []string{"resource", "context"}
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe these are actually used in this test?

Copy link
Member

Choose a reason for hiding this comment

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

ping @Sbou :)

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 only use it in the "No Changes" test to validate that we don't lose this part in the serialization/deserialization step.

"No Changes": {
	Setup:   defaultRouterSetup(),
	Session: newAuthenticationSession(setExtra(sampleKey, sampleValue), setMatchContext(sampleCaptureGroups)),
	Rule:    &rule.Rule{ID: "test-rule"},
	Config:  defaultConfigForMutator(),
	Request: &http.Request{},
	Match:   newAuthenticationSession(setExtra(sampleKey, sampleValue), setMatchContext(sampleCaptureGroups)),
	Err:     nil,
},

pipeline/authz/keto_engine_acp_ory_test.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Feb 14, 2020

The version this will be released (after merge) as is v0.37.0+oryOS.18

@Sbou Sbou force-pushed the feature/global-auth-session branch 2 times, most recently from 67215a1 to 230d9fe Compare February 20, 2020 17:09
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you so much for your hard work (and sorry for my late review) - this looks perfect!

One thing is left before this can be merged, we need to update the documentation here:

It would also be cool to have the new capabilities documented somewhere, maybe as a new section below here ( https://github.com/ory/docs/blob/master/docs/oathkeeper/api-access-rules.md#scoped-credentials )?

@aeneasr
Copy link
Member

aeneasr commented Mar 7, 2020

I’m sorry, I merged another PR which now gives a conflict here. As I don’t habe access to a PC right now, could you rebase/merge with master? Thank you for your hard work!

@Sbou Sbou force-pushed the feature/global-auth-session branch from 230d9fe to fd16ceb Compare March 9, 2020 08:33
@Sbou
Copy link
Contributor Author

Sbou commented Mar 9, 2020

@aeneasr I rebased the Pull Request.

@aeneasr aeneasr merged commit a421293 into ory:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize the use of values extracted from the rule
2 participants