-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Use TokenInterface::getAttribute() instead of deprecated getCredentials() #1251
base: 2.x
Are you sure you want to change the base?
Conversation
e774a3c
to
8a19e6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -116,7 +116,7 @@ private function generateJwtStringAndDispatchEvents(UserInterface $user, array $ | |||
*/ | |||
public function decode(TokenInterface $token) | |||
{ | |||
if (!($payload = $this->jwtEncoder->decode($token->getCredentials()))) { | |||
if (!($payload = $this->jwtEncoder->decode($token->getAttribute('token') ?? $token->getCredentials()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line doesn't fix issue #1040.
Both getCredentials()
and the "token" attribute only exists on JWTPostAuthenticationToken
. Other TokenInterface
have neither. It means the code still crashes badly when the received token is not a
JWTPostAuthenticationToken
.
Beside, other classes implementing a valid TokenInterface
might have an attribute named token
for their own purpose. That token will certainly not be a JWT token.
In other words, you can't assume any TokenInterface
will work. You must either reject tokens based on their instance type (JWTPostAuthenticationToken
or some, yet to be created, JWTAuthenticationTokenInterface
) or strongly type the $token
argument of the decode
method with one of these two types. That's what #1244 does.
@@ -281,8 +282,10 @@ public function testCreateAuthenticatedToken() | |||
$user = $this->createMock(UserInterface::class); | |||
$user->method('getRoles')->willReturn(['ROLE_USER']); | |||
|
|||
$expectedToken = new JWTPostAuthenticationToken($user, 'dummy', ['ROLE_USER'], 'dummytoken'); | |||
$expectedToken->setAttribute('token', 'dummytoken'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #1040 is not about how the JWT payload is stored in the token but about what other authentication systems are used by the project.
For instance, we authenticate users with jwt when they interact with our api but users can also login on a web interface using their username/password/2FA. That authentication creates a UsernamePasswordToken
. Unit tests create a TestBrowserToken
.
See https://github.com/lexik/LexikJWTAuthenticationBundle/pull/1244/files#diff-89a63cacb64b31d43879018f8ede57c4f464869da9cc0115a00407187058f3e0 about how to properly cover issue #1040.
No description provided.