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

PHP8 & lcobucci/jwt 4 compatibility #2117

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

BenceSzalai
Copy link

@BenceSzalai BenceSzalai commented Apr 29, 2021

This PR addresses #2088, #2082, #2103 and probably others. It probably supersedes #2073 which does not include all required changes to update lcobucci/jwt to v4.x. Also relates to topics mentioned under #2059.

Existing tests are updated and pass, and I've also tried some basic use-cases. You can see 3 fixes in the gitlog. The "funny" thing is that those serious issues were not indicated by any automated tests. Probably it would be best to make some tests that check if the tokens are generated with the right content and validated properly, instead of only checking if mocked methods are being called. That being said, I cannot spend more time on this, so I'll leave it up to others.

Please note I'm not a security expert, so review before using this for anything serious!

BREAKING CHANGE: The signature of `Tymon\JWTAuth\Providers\JWT\Lcobucci::__construct()` has been changed, because the previously injected dependencies are deprecated now. But it looks like it has only really been set up the way it was to facilitate testing, and the change should not affect users using it through the ServiceProvider.
All tokens were generated the same from the library, because the builder instance was not persistent between adding the claims.
Fixes "Error : Call to undefined method DateTimeImmutable::getValue()"
@CodeNinja1337 CodeNinja1337 mentioned this pull request May 6, 2021
@sergiy-petrov
Copy link

@tymondesigns any chance to review this?

@piratadelfuturo
Copy link

Can anybody merge this pull request please? :)

@chandreshinf
Copy link

@tymondesigns Can you please merge this pull request?

@OzanKurt
Copy link

Please merge @tymondesigns

@dees040
Copy link

dees040 commented Jun 10, 2021

+1. This package is starting to conflict with other packages using version v4.x of lcobucci/jwt

@tymondesigns
Copy link
Owner

Github is not allowing me to run the workflow/tests for this right now due to a 500 error. I will check back later to see if it's resolved. I may end up just re-creating the PR if I need to

@Afnisse
Copy link

Afnisse commented Jun 15, 2021

@tymondesigns any update about this, I'm using PHP8 and I'm using another library that require version 4 of lcobucci/jwt so there is no chance to install both packages right now, if this pull merged it will save me a lot of time, Thank you for your effort

@dees040
Copy link

dees040 commented Jun 17, 2021

I've made a temporary fork until this PR is merged and submitted it to Packagist. Might be useful for other people here.

composer require dees040/jwt-auth

@petyots
Copy link

petyots commented Jun 24, 2021

Come on, guys... @tymondesigns

@mohamed-foly
Copy link

@tymondesigns

@emielmolenaar
Copy link

@tymondesigns A merge would be great 😄

@piratadelfuturo
Copy link

What if he’s dead? :(

@emielmolenaar
Copy link

Well I really hope he's OK and healthy.

@piratadelfuturo
Copy link

Well I really hope he's OK and healthy.

Just saying because I tried to contact through LinkedIn and there was no answer, also his Twitter has been dead for a while…

@givisok
Copy link

givisok commented Jul 12, 2021

Well I really hope he's OK and healthy.

Just saying because I tried to contact through LinkedIn and there was no answer, also his Twitter has been dead for a while…

He is alive! Please look at the github profile activity

@dv336699
Copy link

dv336699 commented Jul 12, 2021

For those that can, I'd suggest migrating to https://laravel.com/docs/master/sanctum or https://laravel.com/docs/master/passport

@tymondesigns has put a lot of work into this library to solve a problem that existed in 2016 (5-6 years ago) and like everybody else we thank him for his time and effort. Even though this project is sponsored by auth0 I don't think they are paying him enough to dedicate more time to it. It doesn't make much sense to provide updates/support when there are official libraries that will give you the same functionality.

Giving the project to another maintainer on 99% of the cases is not an option as the person taking over would have to be trusted.

@Messhias
Copy link

For those that can, I'd suggest migrating to https://laravel.com/docs/master/sanctum or https://laravel.com/docs/master/passport

@tymondesigns has put a lot of work into this library to solve a problem that existed in 2016 (5-6 years ago) and like everybody else we thank him for his time and effort. Even though this project is sponsored by auth0 I don't think they are paying him enough to dedicate more time to it. It doesn't make much sense to provide updates/support when there are official libraries that will give you the same functionality.

Giving the project to another maintainer on 99% of the cases is not an option as the person taking over would have to be trusted.

Ok, but in this case that's a good solution, it's good for @tymondesigns to leave another maintainer to help him merge the PR's opened.

Copy link

@dir dir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through the code and looks well handled, also tested successfully on local.

@Messhias
Copy link

Messhias commented Jul 14, 2021 via email

@dir
Copy link

dir commented Jul 23, 2021

It's been 6 months, I hate to do this, but... fork inbound? This library has to do with security, and we can't go 6 months without a PR.

I don't have time to manage this, but would be willing to help out. cc @Messhias @BenceSzalai

@BenceSzalai
Copy link
Author

BenceSzalai commented Jul 23, 2021

I am happy to help when and where I can, and this applies to this repo as well as to forks, however I assume forking such a popular library would need wide community consensus and I'm certainly not the guy to build that out.
On the other hand as each PR belongs to a fork, I see no problem if someone starts to collect reasonable changes and updates and others use those even maybe directly from github instead of packagist as a temporary solution. But encouraging people to switch completely may be a delicate topic, for many reasons but especially when we are talking about security related libraries.
Correct me if i'm wrong, i'm just thinking out loud...

Edit:
Also @tymondesigns reacted to this but he got 500 errors and would probably come back later, so while the 6 months assessment may be true, it's not like the original maintainer would have disappeared completely!

Hopefully he will appoint some other maintainers if he's schedule stays too tight for the foreseeable future.

@Messhias
Copy link

I am happy to help when and where I can, and this applies to this repo as well as to forks, however I assume forking such a popular library would need wide community consensus and I'm certainly not the guy to build that out.
On the other hand as each PR belongs to a fork, I see no problem if someone starts to collect reasonable changes and updates and others use those even maybe directly from github instead of packagist as a temporary solution. But encouraging people to switch completely may be a delicate topic, for many reasons but especially when we are talking about security related libraries.
Correct me if i'm wrong, i'm just thinking out loud...

Edit:
Also @tymondesigns reacted to this but he got 500 errors and would probably come back later, so while the 6 months assessment may be true, it's not like the original maintainer would have disappeared completely!

Hopefully he will appoint some other maintainers if he's schedule stays too tight for the foreseeable future.

I'm helping in the best way I can do too since I'm already in the Unreal Engine open source too trying to help, but the whole point of the topic and those PR's start being old is because the library is still a solo maintainer.

And about changing, you're totally right in the case of this library because if you see there's already a wide developer using it, if you type "laravel JWT package" on google this library it's the ones show first, there's a plenty tutorial using it. So the best way to keep doing some work with consistency is @tymondesigns to take at least 4 more maintainers or at least 2, keep it going (even in a smoothy and slow way), and start-stop the gap of months between an acceptance of a PR to another.

@mashoodpv
Copy link

Any chance to get the new release with PR?

@bryce13950
Copy link

Any chance to get the new release with PR?

@Messhias
Copy link

Messhias commented Aug 7, 2021

Any chance to get the new release with PR?

HAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHAHAHAHHAHAHAHHAHAHAHAHAHHAHA

@ye7iaserag
Copy link

@tymondesigns please, we can't move forward without this

@lvandyk
Copy link

lvandyk commented Aug 23, 2021

+1

@rcerljenko
Copy link

Hi guys,

I was facing the same problems with lcobucci/4 incompatibility + Socialite Apple Provider requirements etc... After some time i figured that this library has pretty much no activity at all. I don't mean any disrespect to @tymondesigns. He's done a GREAT job and many thanks to him for everything but I think that now it's time to move on (at least we decided that in our company where we have multiple ongoing Laravel projects that use API auth).

So, I've decided to build a package of my own that handles JWT auth for Laravel apps => https://github.com/rcerljenko/laravel-jwt

Package highlights:

  • Latest Google Firebase powered JWT backend library
  • Minimum dependeny footprint (only latest Laravel and Firebase)
  • Simple JWT configuration and use (config file + trait)
  • No middlewares, facades, etc... just plain and simple config file, auth guard and trait.

Who should use this package?

  • People that need to move on for techincal reasons from this package (like we had to in my company)
  • People who expect that library follows latest Laravel and other dependency updates
  • People who are willing to help in future development by discussing and sending PRs

As I said, we already use this in our company on production projects and it looks stable, safe and it doesn't stops us from installing some packages that we couldn't before.

Feel free to at least take a look and give some smart insight!

Cheers!

@Messhias
Copy link

Hi guys,

I was facing the same problems with lcobucci/4 incompatibility + Socialite Apple Provider requirements etc... After some time i figured that this library has pretty much no activity at all. I don't mean any disrespect to @tymondesigns. He's done a GREAT job and many thanks to him for everything but I think that now it's time to move on (at least we decided that in our company where we have multiple ongoing Laravel projects that use API auth).

So, I've decided to build a package of my own that handles JWT auth for Laravel apps => https://github.com/rcerljenko/laravel-jwt

Package highlights:

  • Latest Google Firebase powered JWT backend library
  • Minimum dependeny footprint (only latest Laravel and Firebase)
  • Simple JWT configuration and use (config file + trait)
  • No middlewares, facades, etc... just plain and simple config file, auth guard and trait.

Who should use this package?

  • People that need to move on for techincal reasons from this package (like we had to in my company)
  • People who expect that library follows latest Laravel and other dependency updates
  • People who are willing to help in future development by discussing and sending PRs

As I said, we already use this in our company on production projects and it looks stable, safe and it doesn't stops us from installing some packages that we couldn't before.

Feel free to at least take a look and give some smart insight!

Cheers!

I almost changed to your package, but I don't see the reason for firebase be mandatory.

@rcerljenko
Copy link

@Messhias well you have to have some sort of JWT generator library like Lcobucci, Namshi, Firebase, etc... Alternative is to create your own provider but I don't see the point in that since these exist.

@Messhias
Copy link

@Messhias well you have to have some sort of JWT generator library like Lcobucci, Namshi, Firebase, etc... Alternative is to create your own provider but I don't see the point in that since these exist.

Might be you can modulate your package too, like to accept working with or without it. I guess it'll help you out attract more maintainers.

@rcerljenko
Copy link

@Messhias I see... you mean like to add support for different JWT libraries so that user can choose via config file which one to use? That's a great idea but it adds an extra dependencies besides the default Firebase.

@Messhias
Copy link

@Messhias I see... you mean like to add support for different JWT libraries so that user can choose via config file which one to use? That's a great idea but it adds an extra dependencies besides the default Firebase.

Yes, it add, but you already added one, what's the problem with more dependencies? More robust libraries work in that way if this is your goal with your library.

@rcerljenko
Copy link

@Messhias my goal is really a simple, easy to use and easy to maintain library that serves it's purpose and that's a stable and secure JWT auth guard for Laravel apps... nothing more and nothing less

@Barmunksu
Copy link

Barmunksu commented Sep 15, 2021

Minimum dependeny footprint (only latest Laravel and Firebase)

@rcerljenko After release of a new version of Laravel, will you immediately drop support for the old versions? I am worried about the lts version (Laravel 9)

@rcerljenko
Copy link

@Barmunksu no. I will continue to support Laravel 8 as well as 9. No reason to drop support for now...

@niladam
Copy link

niladam commented Oct 8, 2021

@tymondesigns i absolutely understand you're probably on a new project or a new road. I can also understand that maybe you do not have the time to help with this anymore.

What i can't understand is how you can be active on github and simply ignoring this. See, most of open source developers keep saying that: "it's open source, you can contribute too!" --- Well, on this specific issue people came together and helped.

This is an issue keeping people from actually upgrading to PHP 8, which has been released almost one year ago.

If you do not have the time to keep maintaining this, please ask for help. I'm sure people will come together and help (as they already did), but continuing to keep this specific issue (others as well) in limbo appears to me as bad faith.

As always, i like to thank for the hard work to the developers who's packages i use and i think this is the case too. Even though i've only inherited a project stumbling on this issue, i think i can safely assume that this project is abandoned.

I will probably move to other alternatives.

Thank you for your time spent on building and maintaining this.

@Messhias
Copy link

Messhias commented Oct 8, 2021

@tymondesigns i absolutely understand you're probably on a new project or a new road. I can also understand that maybe you do not have the time to help with this anymore.

What i can't understand is how you can be active on github and simply ignoring this. See, most of open source developers keep saying that: "it's open source, you can contribute too!" --- Well, on this specific issue people came together and helped.

This is an issue keeping people from actually upgrading to PHP 8, which has been released almost one year ago.

If you do not have the time to keep maintaining this, please ask for help. I'm sure people will come together and help (as they already did), but continuing to keep this specific issue (others as well) in limbo appears to me as bad faith.

As always, i like to thank for the hard work to the developers who's packages i use and i think this is the case too. Even though i've only inherited a project stumbling on this issue, i think i can safely assume that this project is abandoned.

I will probably move to other alternatives.

Thank you for your time spent on building and maintaining this.

Hi,
due to the inactivity of the project, we have forked the repository (https://github.com/PHP-Open-Source-Saver/jwt-auth). Currently, we are going through the PR and identify which are relevant for us. It would be great if you would recreate your PR in our repository.

@jonkerw85
Copy link

PLease, love see this one merged!

@mfn
Copy link
Contributor

mfn commented Jan 4, 2022

@wjonkerhulst the fork mentioned in the comment above yours has switched to "lcobucci/jwt": "^4.0"

This happened in the https://github.com/PHP-Open-Source-Saver/jwt-auth/releases/tag/1.1.0 release.

Please see the migration guide at https://github.com/PHP-Open-Source-Saver/jwt-auth#migrating-from-tymondesignsjwt-auth

@jonkerw85
Copy link

@mfn I'm aware of that project. But to be honest, I do not have a lot of confidence in that project.

Two examples.

A bug as mentioned in PHP-Open-Source-Saver/jwt-auth#84 should not be able to happen. I feel like the feature was added to the project without understanding the ramifications. The problem is also not yet resolved which makes it a no-go for me to use it in my project.

Furthermore in PHP-Open-Source-Saver/jwt-auth#89 @Messhias mentions being in a turbulent time. So I'm unsure if he is able and willing to keep up with the maintenance burden.

@emielmolenaar
Copy link

I think it's very clear that this one won't be merged. Put your efforts in a fork or another solution, tymondesigns/jwt-auth is dead.

@sturtond
Copy link

sturtond commented Feb 9, 2022

Sad times for tymondesigns/jwt-auth

smartech7 pushed a commit to smartech7/php-jwt-authentication that referenced this pull request Aug 4, 2023
smartech7 pushed a commit to smartech7/php-jwt-authentication that referenced this pull request Aug 4, 2023
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.