-
-
Notifications
You must be signed in to change notification settings - Fork 361
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: add bearer_token authenticator #613
Conversation
Adds a new authenticator to work with Kratos' new API token. Works the same as the cookie_session authenticator but checks for a bearer token in the Authorization header (unless overwritten by token_from)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you! Could you please add some documentation as well? A good place would be around here, probably: https://github.com/ory/oathkeeper/blob/master/docs/docs/pipeline/authn.md#cookie_session
@aeneasr yes, I'll get right on that. Is the naming correct btw? Simple bearer seemed most logical but perhaps you know of a better name? |
Hm yeah maybe just call it |
Ok, i'll rename it and add it to the docs |
So it matches the cookie_session authenticator better as functionality is the same: a session store.
@aeneasr the authenticator has been renamed and docs have been added. Please let me know if anything else is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Just one more little change :)
} | ||
|
||
if len(c.SubjectFrom) == 0 { | ||
c.SubjectFrom = "subject" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAuth2 uses the shorthand sub
- I think we should use sub
per default also:
c.SubjectFrom = "subject" | |
c.SubjectFrom = "sub" |
docs/docs/pipeline/authn.md
Outdated
`{ "subject": "...", "session": { "foo": {"bar": "whatever"} } }`, and so on. | ||
- `subject_from` (string, optional - defaults to `subject`) - A | ||
[GJSON Path](https://github.com/tidwall/gjson/blob/master/SYNTAX.md) pointing | ||
to the `subject` field. This defaults to `subject`. Example: `identity.id` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the `subject` field. This defaults to `subject`. Example: `identity.id` for | |
to the `subject` field. This defaults to `sub`. Example: `identity.id` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just sent in the change. This does make it diverge from the cookie_session configuration, but perhaps that will get the change sometime in the future as well?
Related issue
https://community.ory.sh/t/oathkeeper-example-with-kratos-api-session-token/2373
Proposed changes
Adds a new authenticator to work with Kratos' new API token.
Works the same as the cookie_session authenticator but checks for a bearer token in the Authorization header (unless overwritten by token_from)
Checklist
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.
works.
Further comments
If this is the way to go I can add this to the docs as well, just let me know.