corbosman / laravel-passport-claims

Add claims to Laravel Passport JWT Tokens
MIT License
81 stars 12 forks source link

allow for customization of the builder class #33

Closed drmmr763 closed 7 months ago

drmmr763 commented 7 months ago

This isn't tested in a real laravel app yet but I did take a stab at it. I will test further in my app soon.

Note: I couldn't quite figure out what the difference between the ServiceProvider setting the AccessToken class was vs the AccessTokenRepository. It appears the Repository is needed for the unit test but I wasn't convinced based on the way the SP was written that it didn't need to be set in both places. Any ideas on this would be helpful.

what-the-diff[bot] commented 7 months ago

PR Summary

corbosman commented 7 months ago

This isn't tested in a real laravel app yet but I did take a stab at it. I will test further in my app soon.

Note: I couldn't quite figure out what the difference between the ServiceProvider setting the AccessToken class was vs the AccessTokenRepository. It appears the Repository is needed for the unit test but I wasn't convinced based on the way the SP was written that it didn't need to be set in both places. Any ideas on this would be helpful.

At some point Laravel added a way to add a custom AccessToken class. Before that existed, the only path to overriding the claims was to first override AccessTokenRepository, then have that class create your own AccessToken instead of Laravel's AccessToken, which would then allow you to build a custom JWT. This was left in for backwards compatability.

corbosman commented 7 months ago

I'm not really sure why your patch is so elaborate. Maybe I'm not understanding it properly. The ISS is a string, so all we'd need is a way to create a string. I see several ways..

  1. simply allow for a 'issuedBy' => 'foobar' in the config.
  2. use a class like 'issuedBy' => MyCustomIssuedByClass, and have the handle method return a string.
  3. both? Test for string vs class and use either.

Then just add one or more lines essentially doing:

  if (...config exists..) $jwt = $jwt->issuedBy(...);

But maybe im just missing the problem.

I'm not around much for a few days traveling.

drmmr763 commented 7 months ago

I thought of doing a config value for issued by alone, but I was thinking possibly there were other builder functions like the iss one that I wasn't aware of, so I was trying to make a drop in replacement for other devs / use cases.

corbosman commented 7 months ago

Letting you insert your own AccessToken kind of defeats the purpose of this package. Then you might as well just add Passport::useAccessTokenEntity(MyCustomAccessToken::class) to one of Laravel's Providers and you don't even need my package. And that might just be the best way to do this. Then you can write your own custom convertToJWT method with all the Registered Claim methods you want.

My package originated in a time when it was not straightforward to insert your own AccessToken (and thus custom claims). It's much easier now, and this package now really is just a convenience if you want to easily add some claims.

drmmr763 commented 7 months ago

Thanks for tip on using Passport::useAccessTokenEntity. I didn't realize I could do that. I jumped through a lot of unanswered github issues about customizing the token and arrived at this project as the solution - thought it was just missing one little piece I needed. But you're right its definitely easier to use that method instead.