-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor Jws
and Jwt
Verification
#46
Conversation
packages/web5/lib/src/jws/jws.dart
Outdated
signature = base64.decoder.convertNoPadding(parts[2]); | ||
} on Exception { | ||
throw Exception( | ||
'Malformed JWT. Invalid base64url encoding for JWT payload', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be ...for JWT signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! fixed ad71b40
final dsaName = | ||
DsaName.findByAlias(algorithm: header.alg, curve: publicKeyJwk!.crv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's a big worry here, but since we are reading the crv
with a !
there's a chance that would throw when trying to read it and you wouldn't get the nice expectation below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great point @wesbillman . i actually left this as is because i'm removing the entire concept of aliases in the next PR
throw Exception( | ||
'Verification failed. Expected header kid to dereference a verification method', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more specific exception here? Or are both of these changes for header kid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can get slightly more specific. the first exception is thrown if no did resource exists with the kid
provided. the second is thrown if the kid
provided doesnt point to a VerificationMethod
misc, | ||
}) : misc = misc ?? {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Overview
Refactored
Jws
andJwt
Verification into two distinct operations:Decode
andVerify
Rationale
we've run into multiple scenarios where we need to:
The way in which
Jws
andJwt
were initially implemented doesn't let us do this. Additionally, the implementation was a bit obtuse as noted in the TODO hereChanges
In order to remedy the above, this PR splits decoding and verifying into two distinct steps while still providing a single function call if desired
Approach 1
This approach allows to caller to call verify and then decide to perform additional logic using the claims within a JWT depending on the result of verification
Approach 2
This approach requires the caller to first decode the JWT, perform whatever business logic is needed and then optionally call verify when desired
the
Jws
public api surface is identical. Simply changeJwt
toJws
in the examples above.Warning
this PR doesn't include enough tests. looking to add some ASAP