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 for customization of the builder class #33

Closed

Conversation

drmmr763
Copy link

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.

Copy link

what-the-diff bot commented Jan 25, 2024

PR Summary

  • Introduction of a New Library
    The library 'collision' was added, which is used for error handling and debugging for applications.

  • Passport Claims Configuration Updates
    A new key has been added to determine the class responsible for constructing access tokens, offering flexibility to functionality.

  • Access Token Creation Refined
    The code responsible for creating new tokens in AccessTokenRepository has been adjusted. It now uses the 'builder' configuration entry, allowing for custom regarding how access tokens are constructed.

  • Service Provider Adjustments
    The Service Provider, a central piece of Laravel applications, has been modified. It now also uses the 'builder' configuration, again improving the flexibility of token handling.

  • Enhancements to Testing
    Two new files were added dedicated to testing. These will ensure the product remains stable and performs as expected when changes are applied. They specifically test if the new 'builder' function is operating as intended.

@drmmr763 drmmr763 mentioned this pull request Jan 25, 2024
@corbosman
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

corbosman commented Jan 25, 2024

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
Copy link
Author

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.

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.

2 participants