Add Parser::parseUnencrypted()

[spawnia]

The following code currently works fine, but triggers a static analysis error due to the loose return type of Parser::parse():

$token = (new Parser(new JoseEncoder()))->parse($jwtString);
$token->claims();

// PHPStan says: Call to an undefined method Lcobucci\JWT\Token::claims().

I understand from previous discussion that Parser::parse() may also be used to handle encrypted tokens in the future.

Instead of handling other possible parse results in the application - and duplicating proper error handling - I think it is valuable for this library to offer a method that only deals with unencrypted tokens. This way, application code is simplified and proper error handling is ensured. Thus, I propose the following new method:

$token = (new Parser(new JoseEncoder()))->parseUnencrypted($unencryptedJwtString);
assert($token instanceof UnencryptedToken);

[Ocramius]

IMO OK - note that relying on the Parser interface still wouldn't give you a Plain token

[SvenRtbg]

I have the feeling that parse() only giving you a Token interface is intentional for some future feature, so relying on the one implementation to always returning a Plain token, or even enforcing that in the interface may counteract that intention.

[Ocramius]

Nah, perfectly valid for an implementation to return a more specific type.

The fact that the parser implementation may return new token types in future would become a BC break, which is also fine to have as an explicit transition.

[Slamdunk]

This has already been discussed:

1. Token\Parser: specific covariant return type #758
2. Unencrypted Token API Not Visible In Parsed Result #775
3. claims() missing in Token interface #786

[spawnia]

Thanks for pointing me towards this previous work. I took inspiration from your approach in JwtFacade::parse() and extended Parser itself with a method that expects to only handle unencrypted tokens.

[spawnia]

I don't know how to test this, other than repeating the tests for parse() - perhaps they should be parameterized with a data provider to test both parse() and parseUnencrypted()?

[spawnia]

This will lead to escaped mutants, can this be ignored for infection testing?

[lcobucci]

I left the extension point on purpose and would prefer not to remove it or provide different methods to handle this.

JWTs are usually user input and I believe it's responsibility of the users of this library to rely on the interface and verify they have the expected type of token.

The facade is there to offer a simplified API, it's only pending docs and a few more tests (that I couldn't do yet). Doesn't it satisfy your needs?

[spawnia]

JWTs are usually user input and I believe it's responsibility of the users of this library to rely on the interface and verify they have the expected type of token.

I can follow your reasoning, but I want to point out that it is somewhat arbitrary where the line is drawn. The following are all examples of a client not parsing a proper JWT:

• JWT is a string and not null: client must check or get a type error
• JWT has three dots: parser checks and throws if not
• JWT has some required claims, standard claims follow structure: parser checks and throws if not
• JWT is signed with a given key: client may check by invoking SignedWith
• JWT is unencrypted: ?

Calling Parser::parse() can be thought of as asking: "I have a string that I believe to be a JWT, please decode it and confirm that assumption to me.". The added Parser::parseUnencrypted() is asking a similar thing, with the added stipulation that the string is believed to be an unencrypted JWT string. I don't see a fundamental difference between those, and believe they are equally valid. Given that the library only supports unencrypted JWT anyway, it would be great if that use case would be made as simple as possible and require little extra code.

The facade is there to offer a simplified API, it's only pending docs and a few more tests (that I couldn't do yet).

The facade is doing something quite similar to this proposed API: it combines multiple assumptions about the passed in JWT. However, it does so at a more coarse level - requiring two additional constraints. I think this shows that there is a spectrum of how much constraints the library methods validate, and I believe this pull request is closing a gap between the currently existing two extremes.

Doesn't it satisfy your needs?

Due to reasons outside of my immediate control, we are currently forced to forgo time based validation of the tokens we handle.

Would you be more comfortable if I were to add this method as JwtFacade::parseUnencrypted()?

[lcobucci]

@spawnia sorry about my lack of response here (E_TOO_MANY_THINGS_HAPPENING_AT_THE_SAME 🤣).

Due to reasons outside of my immediate control, we are currently forced to forgo time based validation of the tokens we handle.

Then you could still rely on it but provide your own implementation of a ValidAt constraint that doesn't validate anything, right? IMHO that makes your specific use case explicit, whilst allowing for people to challenge that in the future.

I've posted on #822 some more thoughts on this encrypted/unencrypted thing. It would be nice to have your PoV there =)

[lcobucci]

Closing here for now as that's not the direction I'd like us to take.

The main "conflict point" is that in my understanding of the RFC an encrypted/nested JWT is still a JWT, therefore parsing should not provide guarantees but support both types.