feat(verifyVC): add verifyVC method to Claims Service #592

whitneypurdum · 2022-06-30T08:41:19Z

Description

Moves verifyVc and verifyIssuer to Claims Service (as was in original ICL-210, no additional changes)

Contributor checklist

Breaking changes - check for any existing interfaces changes that are not backward compatible, removed method etc.
Documentation - document your code, add comments for method, remember to check if auto generated docs were updated.
Tests - add new or updated existed test for changes you made.
Migration guide - add migration guide for every breaking change.
Configuration correctness - check that any configuration changes are correct ex. default URLs, chain ids, smart contract verification on Volta explorer or EWC explorer.

src/modules/claims/claims.service.ts

jrhender

It looks like it's coming along, thanks @whitneypurdum . I think the biggest thing to do is to update the @energyweb/vc-verification package to use the latest library API.

Also, it seems like the changes to domains.service and verifiable-credentials-base.service are just styling. IMO, I would remove these changes from the PR to avoid having them included in the history and limit the PR to the relevant functional changes.

whitneypurdum · 2022-06-30T17:42:19Z

It looks like it's coming along, thanks @whitneypurdum . I think the biggest thing to do is to update the @energyweb/vc-verification package to use the latest library API.

Also, it seems like the changes to domains.service and verifiable-credentials-base.service are just styling. IMO, I would remove these changes from the PR to avoid having them included in the history and limit the PR to the relevant functional changes.

Sure thing, these have been removed from PR.

whitneypurdum · 2022-06-30T21:35:19Z

@JGiter @jrhender Hopefully my changes align with what you intended above! I removed the error handling from verifyIssuer because I think the method in ew-credentials throws descriptive errors already, plus it does not return boolean value anymore for conditional check. I have squashed my commits.

jrhender

Looks like strong progress to the PR, thank you for all of your efforts @whitneypurdum . However, I think there might be a couple tweaks and ideally some tests to add

src/modules/claims/claims.service.ts

jrhender · 2022-07-01T08:12:32Z

src/modules/claims/claims.service.ts

+    /**
+   * Verifies that credential was issued by authorized issuer
+   *
+   * @param {VerifiableCredential<RoleCredentialSubject} vc to be verified
+   */
+     async verifyVc(vc: VerifiableCredential<RoleCredentialSubject>) {


I think it would be good to:

Add the return type

Add to the documentation that the expected behaviour is that errors are thrown if verification passes (it would be good to include this in the tests as well)

src/modules/claims/claims.service.ts

whitneypurdum · 2022-07-03T18:46:36Z

src/modules/claims/claims.service.ts

@@ -1,17 +1,14 @@
 /* eslint-disable @typescript-eslint/no-unused-vars */
 import { providers, utils, Wallet } from 'ethers';
+import { verifyCredential } from 'didkit-wasm-node';


@jrhender @JGiter we will need to create separate services based on env. for claims service now, right? For 'didkit-wasm-node' and 'didkit-wasm' - like for VC Service?

@whitneypurdum We can use verifyCredential from VC service. Actually I added it already when updated vc-verification. You just need to merge develop

CLAassistant · 2022-07-04T13:43:32Z

All committers have signed the CLA.

src/modules/claims/claims.service.ts

jrhender

Some comments, I will continue to review further

jrhender · 2022-07-04T09:16:15Z

src/modules/claims/claims.service.ts

@@ -1354,6 +1350,23 @@ export class ClaimsService {
    );
  }

+  /**
+   * Verifies that credential was issued by authorized issuer


Suggested change

* Verifies that credential was issued by authorized issuer

* Verifies:

* - That credential proof is valid

* - That credential was issued by authorized issuer

src/modules/claims/claims.service.ts

jrhender

I think the API should be

resolveCredentialAndVerify(subjectDID, roleNamespace) {
  ...some code to resolve the credential 
  if (foundOffChainClaim) {
    return verifyOffChainClaim(offChainClaim);
  }
  if (foundVc) {
    return verifyVc(vc);
  }

where resolveCredentialAndVerify is a rename of the current verify method.

This allow us to have methods which can accept the offChainClaim and VCs directly without needing the credentials to be "resolvable"

How does this sound @whitneypurdum @nichonien ?

src/modules/claims/claims.service.ts

…entials

jrhender · 2022-07-11T11:22:14Z

src/modules/claims/claims.service.ts

+   * @param roleNamesapce The role to try to get a credential for. Should be a full role namespace (for example, "myrole.roles.myorg.auth.ewc") 
+   * @return Boolean indicating if verified and array of error messages
+   */
+     async verifyOffChainClaim(subjectDID: string, roleNamespace: string): Promise<CredentialVerificationResult> {


IMO, this should accept the OffChainClaim interface of ew-credentials https://github.com/energywebfoundation/ew-credentials/blob/55b84926b85a7e2261fffad[…]6/packages/vc-verification/src/models/verifiable-credentials.ts

The idea is that there should be some symmetry between async verifyVc(vc: VerifiableCredential<RoleCredentialSubject>): Promise<CredentialVerificationResult> (which accepts the actual VerifiableCredential) and this verifyOffChainClaim method (which will accept an actual offChainClaim)

Suggested change

async verifyOffChainClaim(subjectDID: string, roleNamespace: string): Promise<CredentialVerificationResult> {

async verifyOffChainClaim(offChainClaim: OffChainClaim): Promise<CredentialVerificationResult> {

Sure, can do, but the two methods used for offchain verification only use the subject did and role name. How can I resolve those in ICL from the claim?

whitneypurdum · 2022-07-11T11:27:35Z

e2e/claims.service.e2e.ts

+      expect(result.errors).toHaveLength(0);
+      expect(result.isVerified).toBeTruthy;
+    });
+    test('verifyVc should: not verify a credential and return an issuer error if the issuer is not authorized', async () => {


This passes

whitneypurdum · 2022-07-11T11:33:19Z

src/modules/claims/claims.service.ts

+   * @param roleNamesapce The role to try to get a credential for. Should be a full role namespace (for example, "myrole.roles.myorg.auth.ewc")
+   * @return void. Returns "Proof Not Verified" error if VC not verified. Returns error if issuer not verified
+   */
+  async resolveCredentialAndVerify(


@JGiter @jrhender This is the new verify code based on previous comments. I have a few of the tests passing, but questions on others - I think maybe I am misunderstanding the limitations of ew-credentials, but I don't think I need to mock anything else out as _verifyIssuer is verifying the credential successfully.

…erface

jrhender · 2022-07-11T13:09:14Z

e2e/claims.service.e2e.ts

+      console.log(result, 'THE RESOLVE RESULT');
+    });
+    test('verifyOffChainClaim should verify an issued off chain claim', async () => {
+      const roleName = `${verifyOffChainClaimRole}.${root}`;


@whitneypurdum Sorry, I think I made a mistake. I think that the signature of verifyOffChainClaim should be async verifyOffChainClaim(eip191JWT: string): Promise<CredentialVerificationResult>

And then you can use the jsonwebtoken package to decode the body.

iam-client-lib/src/modules/claims/claims.service.ts

Line 3 in 757de5a

import jsonwebtoken from 'jsonwebtoken';

where a decoded body should look something like:

{ "claimData": { "claimType": "myrole.roles.myorg.iam.ewc", "claimTypeVersion": 1 }, "signer": "did:ethr:volta:0xb764D50751fF83e66FCF5A08Fb0De3f830248728", "iss": "did:ethr:volta:0xb764D50751fF83e66FCF5A08Fb0De3f830248728", "sub": "did:ethr:volta:0x0fAe481148C2DF77886CE3874f0fb2E7118acDa0" }

jrhender

Great work, thanks for all of your efforts on this @whitneypurdum

src/modules/claims/claims.service.ts

Co-authored-by: John Henderson <[email protected]>

ewf-devops · 2022-07-13T23:25:33Z

🎉 This PR is included in version 6.0.0-alpha.29 🎉

The release is available on:

Your semantic-release bot 📦🚀

ewf-devops · 2022-08-15T09:26:25Z

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

whitneypurdum marked this pull request as draft June 30, 2022 08:41

whitneypurdum marked this pull request as ready for review June 30, 2022 10:40

whitneypurdum requested a review from JGiter June 30, 2022 10:52

whitneypurdum changed the title ~~feat(verifyVC): add verifyVC method to VC Base Service~~ feat(verifyVC): add verifyVC method to Claims Service Jun 30, 2022

JGiter requested changes Jun 30, 2022

View reviewed changes

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

jrhender reviewed Jun 30, 2022

View reviewed changes

whitneypurdum force-pushed the task/ICL-210_verify_ewf_vc branch from c62e14e to a573e51 Compare June 30, 2022 21:37

JGiter approved these changes Jul 1, 2022

View reviewed changes

Harasz mentioned this pull request Jul 1, 2022

feat: claim expieration #567

Merged

5 tasks

jrhender requested changes Jul 1, 2022

View reviewed changes

whitneypurdum commented Jul 3, 2022

View reviewed changes

whitneypurdum added 2 commits July 4, 2022 14:33

feat(verifyVC): add verifyVC method to VC Base Service

3187e5e

feat(verifyVC): make proof private; add error and return types

0da3229

whitneypurdum commented Jul 5, 2022

View reviewed changes

src/modules/claims/claims.service.ts Show resolved Hide resolved

whitneypurdum commented Jul 6, 2022

View reviewed changes

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

jrhender reviewed Jul 6, 2022

View reviewed changes

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

src/modules/claims/claims.service.ts Show resolved Hide resolved

whitneypurdum force-pushed the task/ICL-210_verify_ewf_vc branch from 9496e0b to 0da3229 Compare July 6, 2022 12:01

whitneypurdum added 3 commits July 6, 2022 15:37

feat(verifyVc): update resolve method and add method for offchain verify

800c903

feat(verifyVc): update ew-credentials version and use to resolve cred…

cd77eb0

…entials

feat(verifyVc): update method documentation

86e47e1

jrhender reviewed Jul 11, 2022

View reviewed changes

feat(verifyVc): add unit tests

c4390b1

whitneypurdum commented Jul 11, 2022

View reviewed changes

feat(verifyVc): update verifyOffChainClaim method to accept Claim int…

3034101

…erface

jrhender reviewed Jul 11, 2022

View reviewed changes

whitneypurdum added 2 commits July 13, 2022 13:52

feat(verifyVC): update ew-credentials

877fad6

feat(verifyVc): add jsonwebtoken and update package-lock

81ff25d

jrhender approved these changes Jul 13, 2022

View reviewed changes

src/modules/claims/claims.service.ts Outdated Show resolved Hide resolved

whitneypurdum and others added 6 commits July 13, 2022 16:15

feat(verifyVc): update verify method name

e5733c6

Co-authored-by: John Henderson <[email protected]>

feat(verifyVc): update method and params

9800557

feat(verifyVc): resolve conflicts and add formatting

b24f328

feat(verifyVc): update unit tests

eb0c7e0

feat(verifyVc): update unit tests

9d5e7ba

feat(verifyVc): remove comments and update test case descriptions

04f9c9f

whitneypurdum merged commit f34445f into develop Jul 13, 2022

ewf-devops added the released on @canary label Jul 13, 2022

ewf-devops added the released on @latest label Aug 15, 2022

jrhender deleted the task/ICL-210_verify_ewf_vc branch March 6, 2023 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(verifyVC): add verifyVC method to Claims Service #592

feat(verifyVC): add verifyVC method to Claims Service #592

whitneypurdum commented Jun 30, 2022 •

edited

Loading

jrhender left a comment

whitneypurdum commented Jun 30, 2022

whitneypurdum commented Jun 30, 2022 •

edited

Loading

jrhender left a comment

jrhender Jul 1, 2022

whitneypurdum Jul 3, 2022

JGiter Jul 4, 2022

CLAassistant commented Jul 4, 2022 •

edited

Loading

jrhender left a comment

jrhender Jul 4, 2022

jrhender left a comment

jrhender Jul 11, 2022

whitneypurdum Jul 11, 2022

whitneypurdum Jul 11, 2022

whitneypurdum Jul 11, 2022

jrhender Jul 11, 2022

jrhender left a comment

ewf-devops commented Jul 13, 2022

ewf-devops commented Aug 15, 2022

-   * Verifies that credential was issued by authorized issuer
+   * Verifies:
+   * - That credential proof is valid
+   * - That credential was issued by authorized issuer

	async verifyOffChainClaim(subjectDID: string, roleNamespace: string): Promise<CredentialVerificationResult> {
	async verifyOffChainClaim(offChainClaim: OffChainClaim): Promise<CredentialVerificationResult> {

feat(verifyVC): add verifyVC method to Claims Service #592

feat(verifyVC): add verifyVC method to Claims Service #592

Conversation

whitneypurdum commented Jun 30, 2022 • edited Loading

Description

Contributor checklist

jrhender left a comment

Choose a reason for hiding this comment

whitneypurdum commented Jun 30, 2022

whitneypurdum commented Jun 30, 2022 • edited Loading

jrhender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jul 4, 2022 • edited Loading

jrhender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhender left a comment

Choose a reason for hiding this comment

ewf-devops commented Jul 13, 2022

ewf-devops commented Aug 15, 2022

whitneypurdum commented Jun 30, 2022 •

edited

Loading

whitneypurdum commented Jun 30, 2022 •

edited

Loading

CLAassistant commented Jul 4, 2022 •

edited

Loading