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

Support issuer ( iss )claim #34

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Support issuer ( iss )claim #34

merged 2 commits into from
Feb 9, 2024

Conversation

abublihi
Copy link
Contributor

in response of #32

Copy link

what-the-diff bot commented Jan 29, 2024

PR Summary

  • Addition of issuer configuration options
    Two new configuration options, issuer_enabled and issuer, have been added in the passport-claims.php file. These options provide more control over the authorization process.

  • Modification of AccessToken to include Issuer Claim
    The AccessToken.php file was updated to include an 'issuer' claim in the access token when issuer_enabled is set to true and an issuer is defined. This adjustment enhances the security measures of our system, if chosen to be used.

  • New Test Methods for Issuer Claim
    Two test methods have been added to the AccessTokenClaimTest.php file. These additional tests ensure the proper functionality of the newly added 'issuer' claim in our access tokens, validating its inclusion or exclusion as configured.

@corbosman
Copy link
Owner

Sorry I've been traveling. Would it be an idea to only use 1 parameter? Just call it 'issuer', and let the config say:

'issuer' => env('JWT_ISSUER', null)

And the code checks for not null?

@abublihi
Copy link
Contributor Author

abublihi commented Feb 2, 2024

I want to make sure the configuration are more readable, what i mean setting the iss may not mean the iss should be included but also you have to enable it, because it's additional not required, some programmer may confuse that they should set it.

@corbosman
Copy link
Owner

I don't think people will get confused. Laravel is full of settings where it's either null or set. Having 2 settings means more code to check if it's enabled but not set properly. I would really prefer just env('JWT_ISSUER', null). You could add a comment to the config file saying it should be null, and you could even comment out the whole setting by default. This package has existed for quite a few years and no one ever asked for iss before. It's a very niche thing to need so I don't think many people will get confused. If you look at the #33 , you might even just do what they ended up doing. It's pretty easy nowadays to add your own AccessToken. Then you don't even need my package.

@abublihi
Copy link
Contributor Author

abublihi commented Feb 3, 2024

I know that i can implement my own AccessToken, but for the sake of simplicity i am adding the iss to this package,

now only one setting for the issuer,

@corbosman corbosman merged commit 702e0cd into corbosman:master Feb 9, 2024
10 checks passed
@corbosman
Copy link
Owner

Thanks for the PR, i pushed a 5.3.0 version including your code.

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