Document and enforce JWT payload type to be either String or Map<String, dynamic>
#71
Conversation
tp
changed the title
JWT payload type to be either String or Map<String, dynamic>JWT payload type to be either String or Map<String, dynamic>
tp
force-pushed
the
clarify-payload-type
branch
from
595289c to
6d95e98
Compare
| (jsonBase64.decode(base64Padded(parts[1])) as Map<String, dynamic>); | ||
|
|
||
| final audiance = _parseAud(payload['aud']); | ||
| final issuer = payload['iss']?.toString(); |
There was a problem hiding this comment.
At this point, any non-Map payload would've failed anyway.
| var payload = this.payload; | ||
| if (payload is Map<String, dynamic>) { | ||
| try { | ||
| payload = Map<String, dynamic>.from(payload); |
There was a problem hiding this comment.
I think it made sense that this was taking a copy of the map (as it might've been immutable or used again in the previous variant which was not holding it's own copy), but I wonder if the "writing back" to payload was intentional?
Now we don't write back to the JWT object (this) (and the local reference is also needed to get the type checking work properly, as it's now based on Object instead of the "always working" dynamic).
…ap<String, dynamic>`
tp
force-pushed
the
clarify-payload-type
branch
from
6d95e98 to
16c496e
Compare
|
@jonasroussel Thanks for your reviews on these PRs over the last few days. Is there anything I can/should do now, or will you just wait with the merge until you want to make another release? |
|
Thanks to you for your contribution ! If you have nothing more to add, I will merge all the PRs into main and soon make a new release. |
|
@jonasroussel Great. Yes, this would be everything from my side right now. There were a few more ideas (like an immutable |
|
this was a breaking change that had a minor version pump, leading to other libraries breaking |
|
@ahmednfwela Sorry I missed that, even though we explicitly tried to contain the changes… But migrating away from Then we might as well go for a fully type-safe route and move away from |
451: Type casting for JWT claims in tenant token tests r=curquiza a=ahmednfwela # Pull Request ## Related issue Fixes #450 ## What does this PR do? - Fixes a breaking change that was made in jonasroussel/dart_jsonwebtoken#71 ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved type safety when verifying JWT token payloads in the test suite. * **Chores** * Updated development lint dependency constraint to the 6.x release line for dev tooling consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: ahmednfwela <[email protected]>
3 participants
JWTfields final #68 was rejected, this needs to be enforced for both the constructor and the field setterMaps toMap<String, dynamic>, this has 2 reasonsMap<String, String>(for example), so we need to convert those for the doc comments to be correctMap<String, dynamic>, as some would then stay "in sync" with the original map (if it was ever changed) and some would not. (That's why feat!: MarkJWTfields final #68 also made them read-only, to bring any such late modifications to attention and err.)Closes #67