search

awslabs / aws-jwt-verify

JS library for verifying JWTs signed by Amazon Cognito, and any OIDC-compatible IDP that signs JWTs with RS256, RS384, RS512, ES256, ES384, and ES512
Apache License 2.0
620 stars 44 forks source link

Document decomposeJwt #117

Closed ottokruse closed 1 year ago

ottokruse commented 1 year ago

Issue #, if available: #82

Description of changes: Document the decomposeJwt function

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ottokruse commented 1 year ago

Thoughts on leaving decomposeJwt and marking as @deprecated then having it call into decomposeUnverifiedJwt ?

Valid idea for sure. We would eventually remove decomposeJwt, so this would just push that major version update to the future. Perhaps we'd have other breaking changes then, and we combine several in a new major version. OTOH I think we should be liberal with new major versions in those cases where it's needed and better for the library's design--and I think this is such a case. Avoiding it adds maintenance burden on us. We are in awslabs after all. Also, this renaming isn't a total revamp of the lib, it will be easy for users to change their code and I suspect 99% of our current users do not use decomposeJwt as it was undocumented.

Should we update verifyJwt calls to take either a jwt: string or a decomposedJwt: DecomposedJwt to avoid double parsing, or does it not really help peformance since we would need to validate the DecomposedJwt parts were valid anyway?

Mmmm good thought. It would shave a few cycles off, but only synchronous work. You're gonna have a hard time even measuring that improvement I think. And It would be at the expense of making the interface more complicated. Think I'm not inclined to it.

Also, should we have a top level import like JwtParser (next to JwtRsaVerifier and CognitoJwtVerifier). That would have a method decomposeUntrusted(jwt: string): DecomposedJwt to be the primary interface? Supporting issue https://github.com/awslabs/aws-jwt-verify/issues/82 will probably require this pre-parsing and using some value of the payload to find the correct issuer for verification. This could come in a later PR or major version.

Interesting idea! Would also make it easier to support verifying ALB JWTs. We'd have to play around with the idea and do some sample implementations and see what works nicely.

hakanson commented 1 year ago

I'm fine with the major version and keeping the interface less complicated. We can roll the other ideas into the next minor release.