Skip to content

Fix issue with token generation via DateTimeImmutable instead of int (dependency with lcobucci/jwt ^5.0) - #1282

Open
RuslanKononov wants to merge 2 commits into
lexik:3.xfrom
RuslanKononov:3.x
Open

Fix issue with token generation via DateTimeImmutable instead of int (dependency with lcobucci/jwt ^5.0)#1282
RuslanKononov wants to merge 2 commits into
lexik:3.xfrom
RuslanKononov:3.x

Conversation

@RuslanKononov

Copy link
Copy Markdown

It was problem with generation of token from DTO that implement UserInterface and it was fixed via this changes.

Comment thread Subscriber/AdditionalAccessTokenClaimsAndHeaderSubscriber.php Outdated
Comment thread Subscriber/AdditionalAccessTokenClaimsAndHeaderSubscriber.php Outdated
Comment on lines +38 to +39
'iat' => $this->clock->now(),
'nbf' => $this->clock->now(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't these 2 entries have the same instance? Or is it irrelevant?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it's irrelevant (it was time() before) but you are right, it's more correct to make same nanoseconds time for "initialization" and "not before". maybe it help someone with unit testing

@maxhelias maxhelias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For me it's ok as the LcobucciJWSProvider class hasn't configured the clock injection either.

private ?ClockInterface $clock;

public function __construct(?int $ttl)
public function __construct(?int $ttl, ?ClockInterface $clock = null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For the record changing the public API in a patch is forbidden as per semver, as such this will land in a new minor version. For eventual future contributions, consider splitting in 2 PRs: one that fixes the bug at hand. (using the clock) and one that changes the public API (allowing to inject a clock)

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.

3 participants