-
-
Notifications
You must be signed in to change notification settings - Fork 964
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 ability to convert session to JWT when calling whoami #3472
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3472 +/- ##
==========================================
- Coverage 78.76% 78.17% -0.60%
==========================================
Files 341 341
Lines 23707 22643 -1064
==========================================
- Hits 18673 17701 -972
+ Misses 3667 3615 -52
+ Partials 1367 1327 -40
|
1486983
to
3f00e67
Compare
This patch adds a query parameter `tokenize` to `/session/whoami` which encodes the session to a JWT. It is possible to customize the JWT claims by using a JsonNet template, and furthermore change the expiry of the token. The tokenize feature supports multiple templates, which makes it easy to use the resulting JWT in a variety of use cases.
Co-authored-by: Jonas Hungershausen <[email protected]>
27957fc
to
8745db4
Compare
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.
Very nice 🎉
I only have a few final remarks, but it would be fine to merge IMO right away.
"description": "A list of different templates that govern how a session is converted to a token format.", | ||
"type": "object", | ||
"patternProperties": { | ||
"[a-zA-Z0-9-_.]+": { |
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.
Any reason to not allow just any key here?
"[a-zA-Z0-9-_.]+": { | |
".+": { |
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.
Prevent users from using spaces etc, which then need to be properly encoded in the URL
@@ -221,11 +243,19 @@ func (h *Handler) whoami(w http.ResponseWriter, r *http.Request, _ httprouter.Pa | |||
// s.Devices = nil | |||
s.Identity = s.Identity.CopyWithoutCredentials() |
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 guess it would make this a bit more complex, but did we consider expand
here? It would be a breaking change to not expand everything on the "normal" whoami, but with this new API it would be worth a consideration to require expanding only certain fields. As the dependencies are basically set by the template, we should probably make this part of the template config instead of a request parameter.
Throwing this in here to ensure that we don't have breaking changes later.
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.
It might make more sense to later provide a new /sessions/v2/whoami
API that would introduce expand
across all APIs in a unique way.
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 think /sessions/v2/whoami
is the way to go
session/manager.go
Outdated
@@ -142,6 +142,9 @@ type Manager interface { | |||
// MaybeRedirectAPICodeFlow for API+Code flows redirects the user to the return_to URL and adds the code query parameter. | |||
// `handled` is true if the request a redirect was written, false otherwise. | |||
MaybeRedirectAPICodeFlow(w http.ResponseWriter, r *http.Request, f flow.Flow, sessionID uuid.UUID, uiNode node.UiNodeGroup) (handled bool, err error) | |||
|
|||
// TokenizeSession sets a session's tokenized value. | |||
TokenizeSession(ctx context.Context, template string, session *Session) error |
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.
Is this even used?
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.
nope :) deleted
Co-authored-by: Patrik <[email protected]>
This patch adds a query parameter
tokenize_as
to/session/whoami
which encodes the session to a JWT. It is possible to customize the JWT claims by using a JsonNet template, and furthermore change the expiry of the token.The tokenize feature supports multiple templates, which makes it easy to use the resulting JWT in a variety of use cases.
Closes #2487