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

Allow configuring the JWT builder when generating a token #1328

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Feb 20, 2023

Currently we have to override the whole convertToJWT method to configure JWT (e.g. to add additional claims). This PR makes it possible to override the new withJwtBuilder method instead:

// \League\OAuth2\Server\Entities\Traits\AccessTokenTrait

use Lcobucci\JWT\Builder;

/**
 * Configure the JWT builder instance.
 */
protected function withJwtBuilder(Builder $builder)
{
    return $builder->withClaim('foo', 'bar');
}

Update: This PR also adds withValidatedRequest method to the bearer token validator that makes it possible to configure the request and append additional attributes:

// \League\OAuth2\Server\AuthorizationValidators\BearerTokenValidator

use Lcobucci\JWT\UnencryptedToken;
use Psr\Http\Message\ServerRequestInterface;

/**
 * Configure the validated authorization request instance.
 */
protected function withValidatedRequest(ServerRequestInterface $request, UnencryptedToken $token): ServerRequestInterface
{
    return $request->withAttribute('oauth_foo', $token->claims()->get('foo'));
}

@hafezdivandari
Copy link
Contributor Author

@Sephster any thought/suggestion on this? it's little but very useful change IMO.

@sylfabre
Copy link

sylfabre commented Apr 5, 2023

Thank you @hafezdivandari for this PR, it solves my issue #1327 🎉

@PaolaRuby
Copy link
Contributor

PaolaRuby commented Apr 10, 2023

hi @hafezdivandari , thanks for this PR
Any ideas for getting custom claims on BearerTokenValidator.php

return $request
->withAttribute('oauth_access_token_id', $claims->get('jti'))
->withAttribute('oauth_client_id', $this->convertSingleRecordAudToString($claims->get('aud')))
->withAttribute('oauth_user_id', $claims->get('sub'))
->withAttribute('oauth_scopes', $claims->get('scopes'));

Like

/**
 * Configure the request instance.
 *
 * @return ServerRequestInterface
 */
protected function configRequest($request, $claims)
{
    return $request->withAttribute('oauth_foo', $claims->get('foo'));
}

@hafezdivandari
Copy link
Contributor Author

@PaolaRuby I prefer another PR for that:

/**
 * Configure the request instance.
 *
 * @param \Psr\Http\Message\ServerRequestInterface $request
 * @param \Lcobucci\JWT\Token $token
 *
 * @return Builder
 */
protected function withRequest(ServerRequestInterface $request, Token $token): ServerRequestInterface
{
    return $request->withAttribute('oauth_foo', $token->claims()->get('foo'));
}

@JoyceBabu
Copy link

Currently I am overriding convertToJWT to access the builder. This would be really useful.

@PaolaRuby
Copy link
Contributor

I see that in this package the PRs are almost never reviewed, and when they do they always close them

@JoyceBabu
Copy link

Considering that this is a security related library, I can understand the hesitation in adding unnecessary cruft. There are two positive reviews for this PR. Hopefully, this will be accepted.

@Sephster
Copy link
Member

As Joyce notes, reviewing PRs for this package isn't simple. There is a security consideration and we also have to ensure we are adhering to the OAuth 2 RFCs, of which there are many. This PR hasn't been closed as it will be reviewed. @PaolaRuby we do not close PRs just for the sake of it... please be careful with your choice of words

@PaolaRuby
Copy link
Contributor

we do not close PRs just for the sake of it

Read again, I didn't say that, I just said that they always end up closed, and an option that helps with private claims without having to override many classes has been needed for a long time

@Sephster
Copy link
Member

Always end up closed? This just isn't true.

@PaolaRuby
Copy link
Contributor

on private claims till now, yes
but let's leave it there, that should not be the topic of discussion, but an option to avoid overwriting more than necessary like this. and this
Due to the complexity of the change, it is understandable that it remains for a major release, but for that to happen I imagine that it will take a long time

@thephpleague thephpleague deleted a comment from parallels999 Jul 20, 2023
@NSURLSession0
Copy link

Is there any way we can help to get this pull request merged? I have read all the comments, but I don't quite understand the status now. Also, I don't see any significant security risks. Did I miss anything?
Right now I'm overriding convertToJWT() but there is no way to get the custom claim from the JWT afterwards.

@hafezdivandari
Copy link
Contributor Author

@GrahamCampbell Can you weigh in here please?

@Sephster
Copy link
Member

Sephster commented Aug 2, 2023

Graham isn't involved in this repo. I appreciate you want this and I will get to it but I am currently working on another PR. My earlier reply still stands

@Sephster
Copy link
Member

Sephster commented Aug 2, 2023

I will review this tonight

@Sephster
Copy link
Member

Sephster commented Aug 2, 2023

@hafezdivandari how are you planning to access the custom claims? The bearer token validator usually converts the JWT to a response so scopes etc can be accessed easily but this PR doesn't address this.

I'm inclined to approve this as there is clearly demand for this but just wanted to check your thinking here as I see it as more of a stop gap. Adding the custom claims to the builder only works, but I think it would be better long term to have these populated from some function perhaps linked on a per-client basis so you can have more fine grained control. Appreciate you can do this here but would be nice to have something a bit more integrated long term.

@parallels999
Copy link

parallels999 commented Aug 2, 2023

how are you planning to access the custom claims?

Hi @Sephster, i made a PR for that, I would like to know if that solution mitigates the problem

would be nice to have something a bit more integrated long term.

It would be great, but there could be breaking changes, look at #1122

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Aug 3, 2023

@Sephster Thank you so much for reviewing this. I wanted to keep the changes as little as possible for easier / quicker review, and address the bearer token validator on a separate PR as I said above.

how are you planning to access the custom claims? The bearer token validator usually converts the JWT to a response so scopes etc can be accessed easily but this PR doesn't address this.

However, after your comment I pushed a commit with the same approach to be able to configure the bearer token validator response.

Adding the custom claims to the builder only works, but I think it would be better long term to have these populated from some function perhaps linked on a per-client basis so you can have more fine grained control.

If I have understood your point correctly, this is already tried on PR #1122 as @parallels999 mentioned above?

@josevv28
Copy link

Hi, I need this, any new?

@hafezdivandari
Copy link
Contributor Author

@Sephster Whenever convenient, could you kindly review the new changes in the pull request? I've incorporated your requested changes. Thank you.

@NSURLSession0
Copy link

@Sephster Could you please provide us with an update on your consideration of this feature? I have a feeling of reluctance on your part, but it appears that you have not expressed it explicitly. People spend time on these PR's.

@hafezdivandari
Copy link
Contributor Author

Laravel Passport will release its next major version in few days, we could add support for custom claims on Laravel Passport if this PR was merged. It's requested so many times: laravel/passport#94, laravel/passport#676, laravel/passport#1614, laravel/passport#1578, laravel/passport#1699, laravel/passport#344

@NSURLSession0
Copy link

In regards to @Sephster's noticeable silence, it might be apt to rope @alexbilbie into this conversation as his name is still on thephpleague.com and contributing.md, and perhaps he still cares about this project.

I believe this project would benefit from being maintained by someone who not only has a strong interest in the project itself but also values the input and efforts of those contributing to it.

@Sephster
Copy link
Member

As an open source maintainer, I've been fortunate enough to never be in a position before where I've had to defend my commitment or effort to a project but unfortunately here we are. I'm saddened that this is now the case, but I appreciate that I've had a hand in getting to this situation.

In the past, I've ignored calls for a firm commitment to when something will be merged in because if priorities change, either with the project or life, I inevitably let people down and I don't want this to be the case. I will try my best to be as transparent as possible here to try and give you the answers you seek.

For the past year, I've been readying version 9 of the OAuth 2 server. This version is modernising the library, adding strict types, reintroducing PHP Stan, upgrading league/event, and adding in the first new grant in a long time, the device auth grant.

This piece of work has been all consuming, because everything needs to be checked and rechecked multiple times. The library has to ensure its current functionality continues to work, while also incorporating a new grant. Because this library adheres to RFCs, this is not an easy task. Alex was the original maintainer of this project and will be well aware of the burdens the OAuth RFCs place upon a maintainer.

Unlike some other libraries, we have to ensure that we comply with all implementation details of these RFCs. Anyone who is familiar with the OAuth 2 RFCs will know that they are lengthy, layered, often contradictory between versions, and sometimes vague when you need precision. In short, it is by no means an easy task to add in a new grant.

This work is in its final stages and I'm hoping to push out a release candidate soon. If you want to see progression of this, you can look at the v9-WIP branch where pretty much every file has been edited and over 2500 additions have been made to the codebase.

This is the primary reason I've not finished my review of this PR yet. Work into v9 has been in the pipeline for a long time and is my sole priority at the moment. It is clear that this PR is dear to many of you and I will review it as soon as v9 is out the door which is why it has remained open. It isn't a no, or yes - it is just a "bear with me". I appreciate that has been the state for nearly a year now but work has been underway in other areas for that entire period.

My apologies that I've not been as forthcoming as I could be around this. I know that in most cases, the only reply that will satisfy you is a quick review and approval of this PR but I think it would be a disservice to the many years I've spent maintaining this package to ensure it complies with the rigorous standards we have set ourselves.

@gerryd
Copy link

gerryd commented Feb 15, 2024

While I do understand everyone in this thread really wants this feature merged, I happen to have somewhat of an idea of the massive amount of work @Sephster has put into getting v9 into shape over the last couple of months, for which I am extremely grateful. And this can not be said enough: thank you, Andrew!

This project is, in my opinion, a classic example of XKCD 2347. The obvious fix would be to have a co-maintainer; but finding someone who wants to be in it for the long run and someone you can trust is easier said than done. Meanwhile, let's wait for Andrew to wrap up v9, I'm sure he will get to this PR soon.

@NSURLSession0
Copy link

Please understand, I genuinely didn't mean to come across as attacking @Sephster. The frustration mainly comes from the lack of response to this pull request and no communication as to why, especially since this is not just a request but also a contribution that someone actually spent time on. The above clarification would have been incredibly helpful.

@Sephster: I fully believe in your commitment, and if I've upset you with my words, please accept my sincere apologies!

@vrusua
Copy link

vrusua commented Mar 27, 2024

@Sephster Just wondering how it's coming along. Is there any chance to include this to 9.x release, or? Thanks.

@Sephster
Copy link
Member

Sephster commented Mar 27, 2024

@vrusua it's unlikely this will be added to v9. I've just tagged v9-RC1 and pending any last minute bugs, it will be released soon.

Once that has been released though, I will be giving this feature request my full attention.

The reason I don't want to add this to v9 is I don't want to jeopardise delaying the release, which has already been over a year in the making

@hafezdivandari
Copy link
Contributor Author

Just merged master into this and resolved conflicts.

@pavelwitassek
Copy link

@Sephster Just wondering how it's coming along, the v9 was released at May. Is there any chance to include this to next release? Thank you.

@hafezdivandari hafezdivandari mentioned this pull request Oct 10, 2024
8 tasks
@vrusua
Copy link

vrusua commented Oct 19, 2024

@Sephster Any thoughts when this PR will be merged, or? Thanks.

@Sephster
Copy link
Member

Current focus on fixing the device grant then this will be next to look at

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.