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

Ability to extend the JWT token with information #885

Open
leroy0211 opened this issue Apr 10, 2018 · 18 comments · May be fixed by #1328
Open

Ability to extend the JWT token with information #885

leroy0211 opened this issue Apr 10, 2018 · 18 comments · May be fixed by #1328

Comments

@leroy0211
Copy link

Right now the JWT token has one custom claim called scopes. But it would be a nice feature to add some more claims to the JWT when required.
Like a username, email, user-group, etc.

@Sephster
Copy link
Member

Agreed. Adding as an improvement

@simonhamp
Copy link

If/when this is available, implementors should be extra cautious about what they store in the JWT - especially if it's not encrypted - but even if it is, maybe things like email address aren't the best things to be putting inside a token. Use OpenID Connect instead.

Also, consider the extra weight that adding new claims will give to your headers - in some cases, larger JWTs could cause unexpected errors.

Personally, I would err on the side of keeping tokens slimmer, choosing to add an extra request to your process for less headaches.

@christiaangoossens
Copy link
Contributor

+1, I implemented this now (for adding a group ID to the token) by extending all the grants, which isn't the nicest for maintainability.

@mtangoo
Copy link

mtangoo commented Apr 23, 2018

@simonhamp
Your concern is genuine, but I think it goes against spirit of this library of allowing flexibility in terms of "engine and tires". A word of warning will suffice for those who want to extends it and if someone extends it without knowing what they are doing, let them shoot their feet!

I suggest some sort of freedom we have in other things get extended there too!

@simonhamp
Copy link

@mtangoo I appreciate your point, but I have to disagree. I don't feel that what you suggest is the correct approach. Letting others 'shoot their feet' is one of the factors involved in why there are leaks of billions of people's personal details... we need to help each other improve security and an opinionated stance on the part of widely-used, standards-compliant libraries such as this can make a huge difference on that front.

Flexibility where flexibility is safe and useful, not just for flexibility's sake.

@mtangoo
Copy link

mtangoo commented Apr 23, 2018

we need to help each other improve security and an opinionated stance on the part of widely-used.....Flexibility where flexibility is safe and useful, not just for flexibility's sake.

Thanks for genuine concerns. My point was, putting ability to extend as completely optional and add even a warning that only people who know what they are doing should even venture there. If someone still feels brave to transgress that, then let them shoot their feets for sure.

Am not against helping people to be compliant. But I really get worried when that ends up crippling flexibility.

Cheers!

@simonhamp
Copy link

Hence my original comment 🙂

@mtangoo
Copy link

mtangoo commented May 18, 2018

:)

@nealoke
Copy link

nealoke commented Sep 11, 2018

@christiaangoossens do you mind sharing how you did the adding of (in your case) the group ID to the JWT token? I am needing this as well because users have access to multiple accounts. You mentioned extending the Grant types but I'm unsure how.

@rakeev
Copy link

rakeev commented Dec 10, 2018

@nealoke all you need to do is reimplement AccessTokenEntity::convertToJWT()
Copy it from AccessTokenTrait and adjust as needed
No grant extension needed

@uphlewis
Copy link
Contributor

It would be nice to be able to do this without having to re-implement/copy+paste the guts of AccessTokenTrait::convertToJWT() completely.

How about if we added the following method to AccessTokenTrait:

protected function setCustomScopes(\Lcobucci\JWT\Builder $builder): \Lcobucci\JWT\Builder
{
    return $builder;
}

That gives any descendants the opportunity to simply override the setCustomScopes() method, and then in AccessTokenTrait::convertToJWT() we can run the builder through that method before returning the token.

@andrewmclagan
Copy link

Agree with @uphlewis, even if convertToJWT() returned an instance of Builder and not Token we could extend the trait and modify the token returned in __toString()

@ol1s
Copy link

ol1s commented Oct 14, 2020

Even once you have managed to add custom claims to the JWT, how do you get them back out of the validated request?

Currently BearerTokenValidator is hard-coded to return only 4 specific claims:

// Return the request with additional attributes
return $request
    ->withAttribute('oauth_access_token_id', $token->getClaim('jti'))
    ->withAttribute('oauth_client_id', $token->getClaim('aud'))
    ->withAttribute('oauth_user_id', $token->getClaim('sub'))
    ->withAttribute('oauth_scopes', $token->getClaim('scopes'))

@Radiergummi
Copy link

Radiergummi commented Dec 7, 2020

Since the 8.x release, you cannot override convertToJWT anymore, since it's marked private instead of public. @Sephster, is there any specific reason to not make it protected instead? I recognize the intention to keep things safe and prevent users from shooting their own foot, but OTOH I know what I'm doing and need a few custom claims in my tokens, which will be read by other applications.
It'd be nice to have a way to extend tokens without having to reimplement the whole library :(

Edit: Sorry, my bad. By overriding __toString, we can call a custom implementation instead. This still feels awkward, though, and doesn't invalidate the problem highlighted by @ol1s.

@zploited
Copy link

What is the status of this?
I also need to add data to the generated JWT. In my case, I need to set a "KID" parameter in the header of the token.

This is an old thread, almost 4 years old.. Did someone find a workaround??

@uphlewis
Copy link
Contributor

@zploited the only way is by extending the class(es) (and binding your implementation(s) in your dependency container if you use one)

@decrypted
Copy link

TLDR; my 2 cents for function convertToJWT being private

use AccessTokenTrait{ convertToJWT as public parentconvertToJWT; }

on the topic of if this is needed - I agree that

  • not exposing the builder
  • having Builder as a final class, hence limiting the ability to work this in initJwtConfiguration (like withHeader)
  • not returning Builder with convertToJWT (or an internal function one could override) yet Plain from convertToJWT, which does all the building

is limiting options for people who wanna type "yes, please do" into the code.

I think it's hard to argue that maintaining an out of library copy of convertToJWTor ending up with an disconnected fork is safer on average for someone who needs this.

That said, I can live with the current solution. Thx for this nice clean repo.

@mansguiche
Copy link

Currently looking to use this via Laravel passport and unfortunately going down rabbit holes trying to implement a custom public claim. Please consider making this more easily extensible since the alternative is that I will have to rewrite some of the inner workings which would be unfortunate just for the sake of an ideology.

I understand why you may think that adding this feature could allow people to make their systems less secure. I would still argue that we should educate and allow for informed programmers to extend rather than prohibit said extension. Prohibition of this fairly requested extension has already led to custom work arounds that are far less future proof and could lead to missing patches and bug fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.