fix: adhere to Lcobucci\JWT v5 interface#209
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related issues
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Security/Jwt/Token.php (2)
35-41: Rename local variable for clarity.Using
$headersto store a genericCollectionobscures intent and is misleading ingetHeaders(). Renaming makes the purpose explicit and avoids confusion, especially when mirrored ingetClaims().- $headers = new Collection(); - - return $headers->add($this->token->headers()->all()); + $collection = new Collection(); + + return $collection->add($this->token->headers()->all());
42-47: Duplicate logic betweengetHeaders()andgetClaims().Both methods instantiate a
Collection, calladd(), then return it. Consider extracting this into a private helper to reduce duplication and future-proof changes to collection creation.private function buildCollection(array $items): CollectionInterface { return (new Collection())->add($items); }Then:
- $headers = new Collection(); - return $headers->add($this->token->claims()->all()); + return $this->buildCollection($this->token->claims()->all());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Security/Jwt/Token.php(1 hunks)
🔇 Additional comments (3)
src/Security/Jwt/Token.php (3)
25-25: Confirm composer dependency bump to v5.0 oflcobucci/jwt.The switch to
Lcobucci\JWT\UnencryptedTokenrequires the 5.x release line. Please verify thatcomposer.jsonhas been updated accordingly and that no other parts of the codebase still referenceLcobucci\JWT\Token\Plain.
31-33: Constructor change looks correct.The promoted property cleanly replaces the previous field; visibility and type are appropriate.
51-52: No issues withtoString()adaptation.The method simply delegates to the new token instance; behaviour is preserved.
kilatib
left a comment
kilatib
left a comment
There was a problem hiding this comment.
@kochen Thank you for the PR — it looks great and aligns well with v5! 🙌
However, it currently breaks compatibility with v4.
Would you be open to extending support for both versions? There are a few possible approaches:
-
Class detection in constructor
We could detect the token class here and allow both types.
This would require creating theLcobucci\JWT\Token\Plainnamespace manually if it doesn't exist. -
Separate token classes
CreateTokenV4andTokenV5classes that extend the base token.
Not ideal, but it would allow us to modify the logic here accordingly. -
Symfony DI with factory
Use Symfony's dependency injection to provide the correct token class via a factory.
This is cleaner but requires a bit more setup.
Let me know what you think — happy to help with implementation if needed!
| private $plainToken; | ||
|
|
||
| public function __construct(Plain $plainToken) | ||
| public function __construct(private UnencryptedToken $token) |
There was a problem hiding this comment.
| public function __construct(private UnencryptedToken $token) | |
| public function __construct(private UnencryptedToken|PlainToken $token) |
| namespace OAT\Library\Lti1p3Core\Security\Jwt; | ||
|
|
||
| use Lcobucci\JWT\Token\Plain; | ||
| use Lcobucci\JWT\UnencryptedToken; |
There was a problem hiding this comment.
use Lcobucci\JWT\UnencryptedToken
use Lcobucci\JWT\Token\Plain
Thanks @kilatib. I have our own setup and implementation working perfectly fine with the suggested code change and lcobucci/jwt v4 ( |
|
@kochen Thank you for your response. We will need some time to complete our internal testing and will get back to you as soon as possible. |
kilatib
left a comment
kilatib
left a comment
There was a problem hiding this comment.
Manual validation of the changes has been successfully completed with versions lcobucci/jwt 4.3 and 5.5. Everything is working perfectly — thank you for the PR!
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- New code is not subject to concurrency issues (if applicable)
- Feature is working correctly on my local machine (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
- Pull request's target is not
master - Commits are following conventional commits
- Changelog is updated according to changes (if applicable)
- Documentation is updated according to changes (if applicable)
* chore: validate PR oat-sa/lib-lti1p3-core#209 * chore: update lcobucci/jwt to version 5.5 * chore: update oat-sa/lib-lti1p3-core to stable 7.2.5 version with suppot lcobucci/jwt ^5 * chore: apply support lcobucci/jwt ^5
3 participants
closes #208
Summary by CodeRabbit