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

fix: normalizeCredential overwrites "evidence" when "vc" is missing #86

Conversation

joshbax189
Copy link
Contributor

Occurs when calling normalizeCredential with an argument of type VerifiableCredential (i.e. has credentialSubject instead of vc).

For example

{
  credentialSubject: {
    id: 'did:example:ebfeb1f712ebc6f1c276e12ec21',
  },
    evidence: {
      foo: 'bar',
    },
 }

after normalization becomes

{
 credentialSubject: {
   id: 'did:example:ebfeb1f712ebc6f1c276e12ec21',
 },
   evidence: undefined
}

@joshbax189 joshbax189 force-pushed the fix/normalize-overwites-w3c-props branch from ef641f1 to c162a6c Compare July 30, 2021 04:05
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. It looks great!

I think it would be wise to also add the symmetrical operation when transforming from W3C JSON to JWT payload.
Do you think you can add that to this PR as well?

@mirceanis mirceanis self-assigned this Aug 4, 2021
@joshbax189
Copy link
Contributor Author

Good observation! I actually fixed the W3C -> JWT transform in PR #78 but didn't fix this direction (JWT -> W3C).

Taken together the changes should move ['evidence', 'termsOfUse', 'refreshService', 'credentialSchema', 'credentialStatus'] into the vc object for the W3C -> JWT transform, and back to the top level in the JWT -> W3C transform, unless those props already exist in the input.

@mirceanis
Copy link
Member

Taken together the changes should move ['evidence', 'termsOfUse', 'refreshService', 'credentialSchema', 'credentialStatus'] into the vc object for the W3C -> JWT transform, and back to the top level in the JWT -> W3C transform

yes, that's the general idea

unless those props already exist in the input.

For the moment, this should hold true. In a future version, there should not be any ambiguity here. Either an error should be thrown or the existing props overridden. (#64)

@stale
Copy link

stale bot commented Oct 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 19, 2021
@mirceanis mirceanis removed the stale label Oct 19, 2021
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

It would be great to have the symmetric transformation included here as well.

Do you think you can add them?

@joshbax189 joshbax189 force-pushed the fix/normalize-overwites-w3c-props branch from c162a6c to 4a03bd0 Compare November 17, 2021 01:21
@joshbax189
Copy link
Contributor Author

Hey sorry @mirceanis I think I explained myself poorly (and then forgot to clear it up!).

Commit 1284973 inside #78 already implements the fix within transformCredentialInput function, which produces JwtCredentialPayload. This PR just copies the same pattern to normalizeJwtCredentialPayload (which is called by normalizeCredential, transforming JWT to W3C).

Can you double check the test cases in converters.tests.ts under "other W3C fields" sections do what you expect? There should be one section for each of transformCredentialInput and normalizeCredential.

@mirceanis
Copy link
Member

I see. My bad for not looking thoroughly enough 😅
I'll take another look and merge it in if all is good.
Thanks for contributing!

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

looks great, thank you for the contribution!

@mirceanis mirceanis merged commit 0039298 into decentralized-identity:master Nov 23, 2021
uport-automation-bot pushed a commit that referenced this pull request Nov 23, 2021
## [2.1.8](2.1.7...2.1.8) (2021-11-23)

### Bug Fixes

* normalizeCredential overwrite of "evidence" when "vc" is missing ([#86](#86)) ([0039298](0039298))
@uport-automation-bot
Copy link
Collaborator

🎉 This PR is included in version 2.1.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants