fix(credential-w3c): correct verification of credentials given as objects with jwt proofs #1071
Conversation
…is modified outside of jwt proof note: this test *should* pass, but currently fails since there is a bug
…cts with jwt proofs
nickreynolds
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #1071 +/- ##
==========================================
- Coverage 80.25% 80.02% -0.23%
==========================================
Files 118 127 +9
Lines 4056 4606 +550
Branches 875 986 +111
==========================================
+ Hits 3255 3686 +431
- Misses 798 916 +118
- Partials 3 4 +1 |
| @@ -19,6 +19,7 @@ | |||
| "@veramo/key-manager": "^4.1.1", | |||
| "@veramo/kms-local": "^4.1.1", | |||
| "base64url": "^3.0.1", | |||
| "canonicalize": "^1.0.8", | |||
There was a problem hiding this comment.
I think this dependency should be declared in the @veramo/credential-w3c package
There was a problem hiding this comment.
@mirceanis I just had noticed that did-provider-ion used this dependency but didn't declare it explicitly. I also added it in credential-w3c package
There was a problem hiding this comment.
You're right. I hadn't noticed 👓
| @@ -285,6 +288,20 @@ export class CredentialPlugin implements IAgentPlugin { | |||
| }, | |||
| }) | |||
| verifiedCredential = verificationResult.verifiableCredential | |||
|
|
|||
| // if credential was presented with other fields, make sure those fields match what's in the JWT | |||
| if (!(typeof credential === 'string')) { | |||
There was a problem hiding this comment.
| if (!(typeof credential === 'string')) { | |
| if (typeof credential !== 'string') { |
nitpicking, but it makes it easier to see the !
| @@ -101,7 +190,8 @@ describe('credential-w3c full flow', () => { | |||
| expect(response.verified).toBe(true) | |||
| }) | |||
|
|
|||
| it.only('fails the verification of an expired credential', async () => { | |||
| import { LdDefaultContexts } from '../../../credential-ld/src/ld-default-contexts' | ||
| import { VeramoEd25519Signature2018 } from '../../../credential-ld/src/suites/Ed25519Signature2018' | ||
| import { VeramoEcdsaSecp256k1RecoverySignature2020 } from '../../../credential-ld/src/suites/EcdsaSecp256k1RecoverySignature2020' | ||
| import { VerifiableCredential } from '@veramo/core' |
There was a problem hiding this comment.
| import { VerifiableCredential } from '@veramo/core' | |
| import { VerifiableCredential } from '../../../core/src/' |
We have to use relative paths in tests to test against local source code instead of the packages from NPM
| expect(verifyResult.verified).toBeFalsy() | ||
| }) | ||
|
|
||
| // uncomment when we support `@vocab` in `@context` |
| @@ -19,6 +19,7 @@ | |||
| "@veramo/key-manager": "^4.1.1", | |||
| "@veramo/kms-local": "^4.1.1", | |||
| "base64url": "^3.0.1", | |||
| "canonicalize": "^1.0.8", | |||
There was a problem hiding this comment.
You're right. I hadn't noticed 👓
What issue is this PR fixing
fixes #1070
What is being changed
Update the
verifyCredentialfunction to check the other fields present in the credential object, to ensure they match what's in the JWT. Previously, only thecredential.proof.jwtitself was verified.Quality
Check all that apply:
yarn,yarn build,yarn test,yarn test:browserlocally.