-
Notifications
You must be signed in to change notification settings - Fork 57
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
updates to verify #421
updates to verify #421
Conversation
TBDocs Report 🛑 Errors: 0 @web5/api
@web5/crypto
@web5/crypto-aws-kms
@web5/dids
@web5/credentials
TBDocs Report Updated at 2024-03-14T21:46:00Z |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
- Coverage 93.35% 93.34% -0.01%
==========================================
Files 80 80
Lines 23839 23968 +129
Branches 1891 1916 +25
==========================================
+ Hits 22255 22374 +119
- Misses 1544 1551 +7
- Partials 40 43 +3
|
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.
lgtm
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.
@nitro-neal i have vectors herethat may be worth using
Let me add these test vectors to double check on this impl |
} | ||
|
||
// TODO: iss is optional? | ||
if (!iss) throw new Error('Verification failed: iss claim is required'); |
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.
not optional!
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.
lol ok, just saw this: - https://www.w3.org/TR/vc-data-model/#jwt-decoding
If iss is present, the value MUST be used to set the issuer [property](https://www.w3.org/TR/vc-data-model/#dfn-property) of the new [credential](https://www.w3.org/TR/vc-data-model/#dfn-credential) JSON object or the holder [property](https://www.w3.org/TR/vc-data-model/#dfn-property) of the new [presentation](https://www.w3.org/TR/vc-data-model/#dfn-presentations) JSON object.
} | ||
|
||
// jti MUST represent the id property of the verifiable credential or verifiable presentation. | ||
if(jti && jti !== vcTyped.id) { |
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.
what if there's no jti? shoudl we err?
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.
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.
complex stuff, re-approving. nice job @nitro-neal
@mistermoe I added your test vectors - https://github.com/TBD54566975/web5-spec/tree/main/test-vectors/vc_jwt |
* updates to verify * merge * updates * remove comment * merge and timestamp update * lint update * updates
* updates to verify * merge * updates * remove comment * merge and timestamp update * lint update * updates
* updates to verify * merge * updates * remove comment * merge and timestamp update * lint update * updates
Addressing:
#420
Made sure we are explicitly checking these:
updated the iss to be the did's id and not the vcDataModel issuer
Examples:
vcJwt:
Decoded JWT:
ParsedJwt (VcDataModel):
Example processing rules:
jti
jti: 123
payload.vc.id: 123
valid? ✅
jti: 123
payload.vc.id: undefined
valid? ✅
jti: 123
payload.vc.id: 321
valid? ❌
jti: 123
payload.vc.id: “” // empty string
valid? ❌