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

Avoid setting exp claim from JWTManager #249

Merged

Conversation

chalasr
Copy link
Collaborator

@chalasr chalasr commented Oct 15, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations no
Fixed tickets n/a
Tests pass? yes

The encoder should be responsible of setting the exp claim and all other standard ones. @Spomky this might have an impact on the lexik_jose_bridge, excepted if you set this claim from the encoder anyway.

  • Inject the configured ttl in *JWSProvider rather than JWTManager
  • Update tests
  • Add an UPGRADE+CHANGELOG note

@Spomky
Copy link
Contributor

Spomky commented Oct 15, 2016

The lexik-jose-bridge bundle marks the exp claim as critical.

It does not include it as it seems that it is already set in the payload by the JWTManager service.
But if you remove it from the JWTManager then I will add it to my encoder as well.

@chalasr
Copy link
Collaborator Author

chalasr commented Oct 15, 2016

Is the change itself fine to you?
It sounds logical to me from a separation of concerns pov and allows to use the encoder directly without bad surprise.

@Spomky
Copy link
Contributor

Spomky commented Oct 15, 2016

Yes it is. The payload sent by the JWTManager should contain only the claims related to the user authentication.
Other claims such as jti, iat, exp and so on are not related to the user but the JWT lifecycle.

@chalasr chalasr force-pushed the feature/avoid_setting_standard_claims_from_manager branch from cc329c2 to 0d503a6 Compare October 16, 2016 07:52
@chalasr chalasr force-pushed the feature/avoid_setting_standard_claims_from_manager branch from 0d503a6 to d50d1cc Compare October 16, 2016 08:08
@chalasr chalasr merged commit 2d949b8 into lexik:2.0 Oct 16, 2016
@chalasr chalasr deleted the feature/avoid_setting_standard_claims_from_manager branch October 16, 2016 08:09
@@ -42,6 +42,12 @@ public function getConfigTreeBuilder()
->end()
->scalarNode('token_ttl')
->defaultValue(3600)
->validate()
->ifTrue(function ($ttl) {
return !is_numeric($ttl);

Choose a reason for hiding this comment

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

we should allow null values here as per #117

Choose a reason for hiding this comment

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

also why not simply make it an integerNode as I suspect that floats do not really make sense here ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this check has been moved at runtime in order to allow use of %env()% parameters in #325. The point of using is_numeric() is to support env vars too.
About allowing null, this has been changed accidentally and rediscussed in #250, having tokens with an infinite lifetime is not something we want to support regarding security

@chalasr chalasr mentioned this pull request May 18, 2017
@@ -15,7 +17,7 @@ For a diff between two versions https://github.com/lexik/LexikJWTAuthenticationB

* feature [\#218](https://github.com/lexik/LexikJWTAuthenticationBundle/pull/218) Add more flexibility in token extractors configuration ([chalasr](https://github.com/chalasr))

* feature [\#217](https://github.com/lexik/LexikJWTAuthenticationBundle/pull/217) Refactor TokenExtractors loading for easy overriding ([chalasr](https://github.com/chalasr))
* feature [\#217](https://github.com/lexik/LexikJWTAuthenticationBundle/pull/217) Refactor TokenExtractors loadi ng for easy overriding ([chalasr](https://github.com/chalasr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverted in #750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants