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

feat: 3.x migrate to "lcobucci/jwt" v5 #1157

Open
wants to merge 3 commits into
base: 3.x
Choose a base branch
from

Conversation

Chris53897
Copy link

feat: add support for psr/clock

To be honest i do not have much knowledge about this topic.

I just followed this mirgration guide https://lcobucci-jwt.readthedocs.io/en/latest/upgrading/
V5 does use psr/clock, instead of v4 with there own clock implementation.

As this is a new major version, i see no need to support versions <5

I added this PR for 3.x Branch as well. But could be a seperate PR if you want to.
#1155

feat: add support for psr/clock
feat: add support for psr/clock
@Chris53897
Copy link
Author

Chris53897 commented Sep 4, 2023

First run failed because of missing psr/clock implementation. I just added this to the dev section and let the users decide, which to use. I am not sure for runtime checks if the users to not have an implementation installed.

PHP 8.2 failed because of #1152
PHP 8.3 fails because lcobucci/jwt does not allow PHP 8.3 at the moment.

feat: add support for psr/clock
@Chris53897
Copy link
Author

Can the fix from https://github.com/lexik/LexikJWTAuthenticationBundle/pull/1119/files or 2.x merged upstream for 3.x?

@Chris53897
Copy link
Author

I just noticed that @maxhelias used an different approach for 2.x #1125

@Chris53897 Chris53897 changed the title feat: migrate to "lcobucci/jwt" v5 feat: 3.x migrate to "lcobucci/jwt" v5 Sep 4, 2023
@maxhelias
Copy link
Contributor

Hi @Chris53897, 2.x should be merge on 3.x to correct compatibility

@Chris53897
Copy link
Author

After that is done, we could discuss what are the best options.

  • Keep support of version 3 and 4?
  • Use jwt/clock as hard requirement or let the user decide which implementation of psr/clock the use?

@chalasr
Copy link
Collaborator

chalasr commented Nov 30, 2023

@Chris53897 Thanks for the PR, I'm gonna review and merge soon. I'd plan to finish and release v3.0 somewhere around 2024' first quarter.
Also I'd like to backport this to the 2.x branch with backward compatiblity layers for the older versions of lcobucci/jwt that 2.x supports today. If you feel brave enough to do so, additional PR welcome :)

@Chris53897
Copy link
Author

Thanks. The chance for additional PRs for the tasks is not so high. My actual main focus is on PRs to support symfony 7 and php 8.3 for repos.

@chalasr
Copy link
Collaborator

chalasr commented Nov 30, 2023

That's fine, thanks for telling me.

@@ -53,6 +53,7 @@
"require-dev": {
"phpunit/phpunit": "^9.5",
"symfony/browser-kit": "^5.4|^6.0",
"symfony/clock": "^6.3",
Copy link

Choose a reason for hiding this comment

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

as you instantiate a Symfony Clock when no clock is provided, this is not really an optional dependency.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

I am not sure if this should be tied to "symfony/clock" or if it is possible to just add the requirement that it needs a repo that implements "psr/clock" and let the user decide, which one to chose. require psr/clock-implementation?
https://packagist.org/?query=clock

The linked PR from @Dean151 makes the dependency optional.

Copy link

Choose a reason for hiding this comment

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

Well, your implementation is the one making it non-optional.

However, as this package is a Symfony bundle, I would not be too concerned about being bring a dependency on the Symfony component.

@@ -49,10 +47,10 @@ class LcobucciJWSProvider implements JWSProviderInterface
/**
* @throws \InvalidArgumentException If the given crypto engine is not supported
*/
public function __construct(KeyLoaderInterface $keyLoader, string $signatureAlgorithm, ?int $ttl, ?int $clockSkew, bool $allowNoExpiration = false, Clock $clock = null)
public function __construct(KeyLoaderInterface $keyLoader, string $signatureAlgorithm, ?int $ttl, ?int $clockSkew, bool $allowNoExpiration = false, ClockInterface $clock = null)
Copy link

Choose a reason for hiding this comment

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

You should also update the service definition to inject the clock service defined by FrameworkBundle when it is available (using on-invalid="null" to handle cases where the service is disabled) to use the clock configured in the container.

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.

5 participants