Skip to content
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

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Refactor Jws and Jwt Verification #46

merged 3 commits into from
Feb 12, 2024

Conversation

mistermoe
Copy link
Member

Overview

Refactored Jws and Jwt Verification into two distinct operations: Decode and Verify

Rationale

we've run into multiple scenarios where we need to:

  1. verify a JWT or JWS and then use a number of fields from within the header or the payload/claims
  2. decode a JWT or JWS, check some stuff within the payload or headers, and then verify

The way in which Jws and Jwt were initially implemented doesn't let us do this. Additionally, the implementation was a bit obtuse as noted in the TODO here

Changes

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

decodedJwt = await Jwt.verify('test');

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

decodedJwt = Jwt.decode('test');
await decodedJwt.verify();

the Jws public api surface is identical. Simply change Jwt to Jws in the examples above.

Warning

this PR doesn't include enough tests. looking to add some ASAP

signature = base64.decoder.convertNoPadding(parts[2]);
} on Exception {
throw Exception(
'Malformed JWT. Invalid base64url encoding for JWT payload',
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! fixed ad71b40

Comment on lines +52 to +53
final dsaName =
DsaName.findByAlias(algorithm: header.alg, curve: publicKeyJwk!.crv);
Copy link
Member

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.

Copy link
Member Author

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

Comment on lines +46 to +48
throw Exception(
'Verification failed. Expected header kid to dereference a verification method',
);
Copy link
Member

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?

Copy link
Member Author

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

Comment on lines +69 to +70
misc,
}) : misc = misc ?? {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@mistermoe mistermoe merged commit e4c1dd7 into main Feb 12, 2024
1 check passed
@mistermoe mistermoe deleted the refactor-jwt branch February 12, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants