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

Ignore token not having the getCredentials() method during decode #1244

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

Conversation

fmarchalemisys
Copy link

@fmarchalemisys fmarchalemisys commented Nov 2, 2024

The getCredentials() method was removed from TokenInterface in Symfony 6. Beside, some tokens, such as TestBrowserToken, return null instead of the expected string type. See issue #1040.

Decoding a jwt token is only meaningful when the token is a JWTPostAuthenticationToken. Any other token can be ignored.

To fix the deprecation, the user's code must do something like this:

if (!$token instanceof JWTPostAuthenticationToken) {
    return;
}
try {
    $jwtPayload = $this->jwtManager->decode($token);
} catch (JWTDecodeFailureException) {
    return;
}
if ($jwtPayload === false) {
    return;
}
// process $jwtPayload…    

Now, the questions:

  1. Do we deprecate the call with anything other than a JWTPostAuthenticationToken with the aim of restricting the type to public function decode(JWTPostAuthenticationToken $token): array|bool later on?
  2. Do we simply ignore tokens other than JWTPostAuthenticationToken without deprecation to make the use of the decode method easier?

This PR is currently aiming at the first solution but solution 2 simply requires to remove the trigger_deprecation call.

The TokenInterface doesn't have a getCredentials() method anymore since
Symfony 6. Beside, some tokens, such as TestBrowserToken returns null.

Decoding a jwt token is only meaningful when the token is a
JWTPostAuthenticationToken. Any other token can be ignored.
@chalasr
Copy link
Collaborator

chalasr commented Nov 10, 2024

Thanks for pushing the topic and doing the PR, that's much appreciated.
However, I'm not sure ignoring any other token type is the right approach as we'll have to stop implementing getCredentials() in JWTPostAuthenticationToken anyway since it is deprecated.
Here is a PR for 2.x (which is still maintained for bugfixes) that makes use of TokenInterface::get/setAttribute() instead: #1251.
For 3.x, I'm thinking about deprecating JWTManager::decode() entirely, suggesting parse() as replacement. Both JWTPostAuthenticationToken + decode(TokenInterface) feels overcomplicated. Any thought on this is welcome

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.

2 participants