fix: query for qualified/unqualified forms in revocation notification #1866

genaris · 2024-05-15T18:27:48Z

Now querying in both qualified and unqualified forms for revocation registry id. Didn't add any specific test for this yet.

@bryce-mcmath it would be great if by chance you can verify that this properly solves your issue with legacy indy credentals.

Fixes #1864

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

wadeking98 · 2024-05-15T20:53:56Z

could we also do this for credentials and proofs?

bryce-mcmath · 2024-05-15T23:22:17Z

Still getting no records with the updated query. Here's what the query ends up looking like:

"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    },
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsUnqualifiedRevocationRegistryId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    }
  ]

I thought maybe the values passed were incorrect so I tried switching them, but still retrieved no records. Here's what that query looked like:

"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    },
    {
      "anonCredsUnqualifiedRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    }
  ]

Not sure if I mentioned this already but it appears to be using the 'V1' revocation notification process, if that's helpful. Let me know what further info I could provide that might help.

bryce-mcmath · 2024-05-15T23:24:04Z

@TimoGlastra please let me know if you see anything odd about the above that could cause no records to be found

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2024-05-15T23:53:37Z

Still getting no records with the updated query. Here's what the query ends up looking like:

"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    },
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsUnqualifiedRevocationRegistryId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    }
  ]

I thought maybe the values passed were incorrect so I tried switching them, but still retrieved no records. Here's what that query looked like:

"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    },
    {
      "anonCredsUnqualifiedRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    }
  ]

Not sure if I mentioned this already but it appears to be using the 'V1' revocation notification process, if that's helpful. Let me know what further info I could provide that might help.

Yeah the query parameters were mixed up in previous commit. Sad that once you updated the query it is still not finding any record. Looking here, it seems that Credo is not storing legacy identifiers because the issuer is not a qualified did:indy. I think this should be done either if it is a qualified or an unqualified did:indy. @auer-martin can you confirm?

TimoGlastra · 2024-05-16T03:23:02Z

...dules/credentials/protocol/revocation-notification/services/RevocationNotificationService.ts

@@ -51,7 +51,21 @@ export class RevocationNotificationService {
    comment?: string
  ) {
    // TODO: can we extract support for this revocation notification handler to the anoncreds module?
-    const query = { anonCredsRevocationRegistryId, anonCredsCredentialRevocationId, connectionId: connection.id }
+    // Search for the revocation registry in both qualified and unqualified forms
+    const query = {


Do we already store it in both formats? We MIGHT have to query the credential itself first and based on that find the exchange record (as we definitely store both qualified and unqualified on the w3c record)

I think the idea in 0.5 was to store both in the w3c record, but up to now it is doing so only in case the issuer is a qualified did:indy, which is not the case in BCGov demo for example, so unqualified form is not being recorded there.

But something I'm thinking while inspectic the code is the following: LegacyIndyCredentialFormatService is supposed to work only with unqualified identifiers, right? In that case, to fix this particular issue shouldn't it suffice to simply store anonCredsRevocationRegistryId tag and metadata in unqualified format?

But something I'm thinking while inspectic the code is the following: LegacyIndyCredentialFormatService is supposed to work only with unqualified identifiers, right? In that case, to fix this particular issue shouldn't it suffice to simply store anonCredsRevocationRegistryId tag and metadata in unqualified format?

I think for the migration it's easier if we can store as much as possible as qualified identifiers. It's very simple to go from qualified -> unqualified, not the other way around. In the future it would be nice if you could send a revocation notification with a qualified identifier for a credential that was issued as unqualified.

auer-martin · 2024-05-16T09:55:18Z

Still getting no records with the updated query. Here's what the query ends up looking like:
"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    },
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsUnqualifiedRevocationRegistryId": "14",
      "connectionId": "c8b60a89-4849-4697-9d8b-326e1f1ddc5c"
    }
  ]
I thought maybe the values passed were incorrect so I tried switching them, but still retrieved no records. Here's what that query looked like:
"$or": [
    {
      "anonCredsRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    },
    {
      "anonCredsUnqualifiedRevocationRegistryId": "M6dhuFj5UwbhWkSLmvYSPc:4:M6dhuFj5UwbhWkSLmvYSPc:3:CL:612088:student_card:CL_ACCUM:5f6b196c-2051-4c30-92e1-e22af703ba8f",
      "anonCredsCredentialRevocationId": "13",
      "connectionId": "90da9419-b550-4616-b087-0e48efd78b0d"
    }
  ]
Not sure if I mentioned this already but it appears to be using the 'V1' revocation notification process, if that's helpful. Let me know what further info I could provide that might help.
Yeah the query parameters were mixed up in previous commit. Sad that once you updated the query it is still not finding any record. Looking here, it seems that Credo is not storing legacy identifiers because the issuer is not a qualified did:indy. I think this should be done either if it is a qualified or an unqualified did:indy. @auer-martin can you confirm?

Yeah, I think that's a mistake.
I'm wondering if we should do it for qualified and unqualified Indy Dids, as we want to get rid of the unqualified variants eventually. But I don't think it will hurt if we do it for qualified and unqualified atm.

bryce-mcmath · 2024-05-16T18:35:04Z

Just tried one of the approaches suggested; removing the (isIndyDid(issuerId) from here, keeping the other change from this PR as well (or query).

It still unfortunately did not find the record, not sure why. Here's what my resulting w3cRecordAnonCredsTags looked like:

{
  "anonCredsLinkSecretId": "790feabb-8054-41c9-8df5-197e3f646979",
  "anonCredsCredentialDefinitionId": "did:indy:bcovrin:test:QEquAHkM35w4XVT3Ku5yat/anoncreds/v0/CLAIM_DEF/567943/student_card",
  "anonCredsSchemaId": "did:indy:bcovrin:test:QEquAHkM35w4XVT3Ku5yat/anoncreds/v0/SCHEMA/student_card/1.4",
  "anonCredsSchemaName": "student_card",
  "anonCredsSchemaIssuerId": "did:indy:bcovrin:test:QEquAHkM35w4XVT3Ku5yat",
  "anonCredsSchemaVersion": "1.4",
  "anonCredsMethodName": "indy",
  "anonCredsRevocationRegistryId": "did:indy:bcovrin:test:QEquAHkM35w4XVT3Ku5yat/anoncreds/v0/REV_REG_DEF/567943/student_card/ac6d918c-6532-45a8-b74c-4a2533e596a2",
  "anonCredsCredentialRevocationId": "1",
  "anonCredsUnqualifiedIssuerId": "QEquAHkM35w4XVT3Ku5yat",
  "anonCredsUnqualifiedCredentialDefinitionId": "QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card",
  "anonCredsUnqualifiedSchemaId": "QEquAHkM35w4XVT3Ku5yat:2:student_card:1.4",
  "anonCredsUnqualifiedSchemaIssuerId": "QEquAHkM35w4XVT3Ku5yat",
  "anonCredsUnqualifiedRevocationRegistryId": "QEquAHkM35w4XVT3Ku5yat:4:QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card:CL_ACCUM:ac6d918c-6532-45a8-b74c-4a2533e596a2"
}

and here's what my query looked like:

query: {
  "$or": [
    {
      "anonCredsRevocationRegistryId": "QEquAHkM35w4XVT3Ku5yat:4:QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card:CL_ACCUM:ac6d918c-6532-45a8-b74c-4a2533e596a2",
      "anonCredsCredentialRevocationId": "1",
      "connectionId": "985d47bb-a7e7-4b1b-8ac8-02e407b5e0a1"
    },
    {
      "anonCredsUnqualifiedRevocationRegistryId": "QEquAHkM35w4XVT3Ku5yat:4:QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card:CL_ACCUM:ac6d918c-6532-45a8-b74c-4a2533e596a2",
      "anonCredsCredentialRevocationId": "1",
      "connectionId": "985d47bb-a7e7-4b1b-8ac8-02e407b5e0a1"
    }
  ]
}

Let me know if anything here stands out.

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2024-05-16T19:01:03Z

Just tried one of the approaches suggested; removing the (isIndyDid(issuerId) from here, keeping the other change from this PR as well (or query).

It still unfortunately did not find the record, not sure why. Here's what my resulting w3cRecordAnonCredsTags looked like:

Let me know if anything here stands out.

The issue is that in RevocationNotificationService we are looking at the tag in the CredentialExchangeRecord (i.e. the record holding the credential issuance flow) while these new unqualified tags are being stored into the CredentialRecord itself.

So for the moment I've updated LegacyIndyCredentialFormatService to set the unqualified identifier as a tag of the CredentialExchangeRecord (in addition to the qualified one), which seems to be working fine for BC Wallet showcase demo credentials.

bryce-mcmath · 2024-05-16T20:56:32Z

packages/anoncreds/src/formats/LegacyIndyCredentialFormatService.ts

@@ -399,6 +399,7 @@ export class LegacyIndyCredentialFormatService implements CredentialFormatServic
      })
      credentialRecord.setTags({
        anonCredsRevocationRegistryId: credential.revocationRegistryId,
+        anonCredsUnqualifiedRevocationRegistryId: anonCredsCredential.rev_reg_id,


Is this addition also needed in the acceptRequest method? (Line 277)

That's for the issuer part, but yes, it should be added also here if we follow this approach.

bryce-mcmath · 2024-05-16T21:06:52Z

I have one comment above just asking if we need an extra addition but as it is currently revocation works! Thank you all for your help this far. Safe to create a patch with these changes?

bryce-mcmath · 2024-05-16T21:10:21Z

Oh one other question. Will this only solve the problem for newly issued credentials, credentials issued with AFJ 0.4.2 will still fail revocation, no?

genaris · 2024-05-16T22:12:49Z

Oh one other question. Will this only solve the problem for newly issued credentials, credentials issued with AFJ 0.4.2 will still fail revocation, no?

From my understanding, credentials issued with 0.4.2 should work, since at the moment we are not touching any tag in CredentialExchangeRecord during record migration. So I guess it will have anonCredsRevocationRegistryId: "QEquAHkM35w4XVT3Ku5yat:4:QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card:CL_ACCUM:ac6d918c-6532-45a8-b74c-4a2533e596a2" (i.e. the unqualified one, matching what you are sending to it).

BTW for consistency, I think if we follow this anonCredsUnqualifiedRevocationRegistryId approach, it will be nice to also create this tag in the migration scripts.

bryce-mcmath · 2024-05-16T22:34:23Z

From my understanding, credentials issued with 0.4.2 should work, since at the moment we are not touching any tag in CredentialExchangeRecord during record migration

I can confirm that revocation in 0.5.2 for credentials issued in 0.4.2 work with these changes.

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2024-05-17T00:15:52Z

packages/anoncreds/src/formats/LegacyIndyCredentialFormatService.ts

      credentialOffer,
      credentialRequest,
      credentialValues: convertAttributesToCredentialValues(credentialAttributes),
    })

-    if (credential.rev_reg_id) {


I removed it from here because we don't actually support revocation with legacy indy credentials (but we do in AnonCreds, so tags are added to issued credentials there)

WDYM? A credential with legacy identifiers can't use revocation? Is that because it's not implemented in the legacy credential format service?

Yes, we do not currently support issuing revocable legacy indy credentials, so I don't see how this if statement can be true.

what do you mean by "legacy indy credentials"? is there a short documentation about what is considered legacy? Was revocation notification supported before for "legacy indy credentials"?

@genaris I know this is already merged but could I get a bit of clarification here; is this method only for issuing credentials? The name acceptRequest implies it could be accepting a credential issued by another agent that does support issuing legacy creds, no? Currently our ecosystem uses revocation with credentials that don't start with did:indy etc. ie. "legacy" credentials as I understand it, so it seems like we would need to keep this if block. Let me know what you think

The naming here is mainly based on DIDComm Issue Credential protocols, where the steps are propose -> offer -> request -> issue (see here). This acceptRequest is about responding to the request-credential message, which is sent from the holder to the issuer to ask for the actual credential issuance.

So this method is only called in issuer side. As you can see some lines before, we are calling anoncredsIssuerService.createCredential without any revocation-related CreateCredentialOptions parameter (revocationRegistryDefinitionId, revocationRegistryIndex, revocationStatusList), like we do in AnonCredsCredentialFormatService.

Ah, ok. Got it. Thank you!

TimoGlastra

It looks good 👍 but it's also complex logic with all this qualified vs unqualified so I might miss things 😅

TimoGlastra · 2024-05-17T04:57:17Z

packages/anoncreds/src/updates/0.4-0.5/anonCredsCredentialRecord.ts

+      // If using unqualified dids, store both qualified/unqualified revRegId forms
+      // to allow retrieving it from revocation notification service
+      if (qualifiedRevocationRegistryId && indyNamespace) {


This comment indicates we're using unqualified, but the parameter you're checking is for the qualified id?

It was mostly to optimize a bit the code but I'll change it, since it is not clear

TimoGlastra · 2024-05-17T04:58:27Z

packages/anoncreds/src/utils/w3cAnonCredsUtils.ts

@@ -199,7 +200,7 @@ export function getW3cRecordAnonCredsTags(options: {
    anonCredsMethodName: methodName,
    anonCredsRevocationRegistryId: revocationRegistryId,
    anonCredsCredentialRevocationId: credentialRevocationId,
-    ...(isIndyDid(issuerId) && {
+    ...((isIndyDid(issuerId) || isUnqualifiedIndyDid(issuerId)) && {


Are the non unqualified tags now always using qualified identifiers? Even if the credential was issued as unqualified?

I've understood from a previous comment that it was better to store the qualified form because it was not possible to go the other way around. Now I'm confused!

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

bryce-mcmath · 2024-05-23T21:02:45Z

could we also do this for credentials and proofs?

@wadeking98 do you still want this? @genaris see any issue doing this?

genaris added 2 commits May 15, 2024 15:25

fix: query for qualified/unqualified forms in revocation notification

73d68b0

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Merge branch 'main' into fix/revocation-registry-query

82456b2

fix: mixed up query parameters

f225420

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

TimoGlastra reviewed May 16, 2024

View reviewed changes

fix: store unqualified identifiers

4dc7059

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

bryce-mcmath reviewed May 16, 2024

View reviewed changes

Merge branch 'main' into fix/revocation-registry-query

11f929a

genaris added 2 commits May 16, 2024 20:48

fix: add unqualified identifier to issuer and migration

29dadb6

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: revocation tags

6cef96a

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris commented May 17, 2024

View reviewed changes

TimoGlastra approved these changes May 17, 2024

View reviewed changes

genaris added 3 commits May 17, 2024 12:12

fix: qualified/unqualified in migration

475c6c1

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Merge branch 'main' into fix/revocation-registry-query

df704b7

Merge branch 'main' into fix/revocation-registry-query

564bf97

genaris merged commit dcb363d into openwallet-foundation:main May 23, 2024
12 checks passed

genaris deleted the fix/revocation-registry-query branch May 23, 2024 14:13

bryce-mcmath mentioned this pull request Jun 6, 2024

fix: patch anoncreds and update bifold, add patch notes bcgov/bc-wallet-mobile#2003

Merged

github-actions bot mentioned this pull request Jun 21, 2024

chore(release): new version #1910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: query for qualified/unqualified forms in revocation notification #1866

fix: query for qualified/unqualified forms in revocation notification #1866

genaris commented May 15, 2024

wadeking98 commented May 15, 2024

bryce-mcmath commented May 15, 2024

bryce-mcmath commented May 15, 2024

genaris commented May 15, 2024

TimoGlastra May 16, 2024

genaris May 16, 2024 •

edited

Loading

TimoGlastra May 16, 2024

auer-martin commented May 16, 2024 •

edited

Loading

bryce-mcmath commented May 16, 2024

genaris commented May 16, 2024

bryce-mcmath May 16, 2024

genaris May 16, 2024

bryce-mcmath commented May 16, 2024 •

edited

Loading

bryce-mcmath commented May 16, 2024

genaris commented May 16, 2024

bryce-mcmath commented May 16, 2024

genaris May 17, 2024

TimoGlastra May 17, 2024

genaris May 17, 2024

cvarjao May 17, 2024 •

edited

Loading

bryce-mcmath May 23, 2024

genaris May 23, 2024 •

edited

Loading

bryce-mcmath May 23, 2024

TimoGlastra left a comment

TimoGlastra May 17, 2024

genaris May 17, 2024

TimoGlastra May 17, 2024

genaris May 17, 2024

bryce-mcmath commented May 23, 2024

fix: query for qualified/unqualified forms in revocation notification #1866

fix: query for qualified/unqualified forms in revocation notification #1866

Conversation

genaris commented May 15, 2024

wadeking98 commented May 15, 2024

bryce-mcmath commented May 15, 2024

bryce-mcmath commented May 15, 2024

genaris commented May 15, 2024

Choose a reason for hiding this comment

genaris May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auer-martin commented May 16, 2024 • edited Loading

bryce-mcmath commented May 16, 2024

genaris commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-mcmath commented May 16, 2024 • edited Loading

bryce-mcmath commented May 16, 2024

genaris commented May 16, 2024

bryce-mcmath commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvarjao May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genaris May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra 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

bryce-mcmath commented May 23, 2024

genaris May 16, 2024 •

edited

Loading

auer-martin commented May 16, 2024 •

edited

Loading

bryce-mcmath commented May 16, 2024 •

edited

Loading

cvarjao May 17, 2024 •

edited

Loading

genaris May 23, 2024 •

edited

Loading