openwallet-foundation / credo-ts

Typescript framework for building decentralized identity and verifiable credential solutions
https://credo.js.org
Apache License 2.0
260 stars 197 forks source link

Unable to share revocable credential in 0.2.0-alpha.53 #713

Closed marcinkiewiczblazej closed 2 years ago

marcinkiewiczblazej commented 2 years ago

I'm using version 0.2.0-alpha.53. My setup: react native wallet (Android), acapy mediator, acapy agent and a backend that serves also as a tails server. I'm using Indicio test net ledger. I was able to issue a revocable credential. For sharing I was testing 2 scenarios:

  1. Proof request without non_revoked params. It worked fine, whole flow finished successfully.
  2. Proof request with non_revoked params. It seems that it gets stuck somewhere, but hard to tell where. Here are last log lines from the wallet.
 LOG  Handling proof request with id 1fd96be1-cffa-452a-b018-baff79f4fd1a from connection 0f7680e9-cc31-4654-baf9-88cc4a2c52af
 LOG  TRACE: Presentation is requesting proof of non revocation, getting revocation status for credential {
  "requestNonRevoked": {
    "from": 0,
    "to": 1650394955
  },
  "credentialRevocationId": "3",
  "revocationRegistryId": "ID"
}
 LOG  TRACE: Fetching Credential Revocation Status for Credential Revocation Id '3' with revocation interval with to '1650394955' & from '0'

I've checked logs from mediator and other agent and there were no messages sent. So the execution stopped earlier.

swcurran commented 2 years ago

I suspect this is because of the from and to values. Per best practices in Indy presentation requests (this Aries RFC 0441), the values should be the same for from and to.

The background is that the original implementation of AnonCreds tried to be too flexible in the implementation of the handling of "revocation intervals" and made things extremely complex. For sanity, the from and to in a presentation request should be the same -- or the from eliminated.

Could you give that a try and report back? I suspect that you are using vc-authn-oidc, which uses the form you are seeing. We are trying to sort out what to do about that. I believe there is concern that if we change from the form you provide in your example, the Trinsic wallet will not work, but I've yet to hear if that is actually the case.

Thanks!

marcinkiewiczblazej commented 2 years ago

@swcurran thanks it worked. Weird thing that there was no error in the console or in the app.

Now there's other issue. Wallet successfully fetched tails file. While reading it there's exception:

com.facebook.react.bridge.ReadableNativeMap cannot be cast to java.lang.String

The stacktrace contains only native java methods. Top entry is getString on ReadableNativeArray.

Side note. I'm not using vc-authn-oidc. It's in house implementation that is using aca-py agent directly, so we're free to update the values as needed.

TimoGlastra commented 2 years ago

It's weird it's not throwing an error for that. We do a check for the from and to values and the following error should be thrown: Presentation requests proof of non-revocation with an interval from: '${requestRevocationInterval.from}' that does not match the interval to: '${requestRevocationInterval.to}', as specified in Aries RFC 0441

Maybe we should also log an error message. How are you creating the presentation? Is it through auto-accept? In that case the function is sort-of allowed to fail (there's situations where we can't auto respond). Or are you responding using the proofs module?

marcinkiewiczblazej commented 2 years ago

Auto-accept is set to never

          autoAcceptConnections: true,
          autoAcceptCredentials: AutoAcceptCredential.Never,
          autoAcceptProofs: AutoAcceptProof.Never,

Proofs module is used, as we allow user to accept or reject the request and preview what data is requested, respectively agent.proofs.declineRequest or agent.proofs.acceptRequest. But the initial error happened before proofs module was called.

Current error with casting to String is happening after the proof request is accepted.

TimoGlastra commented 2 years ago

Hmm not so sure what's happening here, but almost haven't worked with revocation in AFJ myself yet. @JamesKEbert any thoughts?

marcinkiewiczblazej commented 2 years ago

After updating indy-sdk-react-native to version 0.1.21 it started working correctly. So the issue is resolved.

TimoGlastra commented 2 years ago

Ah good to know that did the trick. Great find!

JamesKEbert commented 2 years ago

I suspect this is because of the from and to values. Per best practices in Indy presentation requests (this Aries RFC 0441), the values should be the same for from and to.

Per our implementation we built AFJ to appropriately throw an error/error out when the intervals don't match the for-mentioned best practices, so I am curious as to why we didn't end up seeing those errors. It may have been that the logic for asserting the intervals occurred after the actual issue presented in this report. @TimoGlastra is there a test in the AATH to test a mis-matching interval as it might be good to verify the functionality here?

x0axz commented 1 year ago

We integrated the Aries React Native App with aries-framework-dotnet for Issuer & Verifier and aries-mediator-service for Mediator, and we encountered a similar issue with the app with the most recent version of indy-sdk-react-native. Presentation requests proof of non-revocation with an interval from: '${requestRevocationInterval.from}' that does not match the interval to: '${requestRevocationInterval.to}', as specified in Aries RFC 0441.

However, @swcurran's comment was helpful in finding a solution.

The background is that the original implementation of AnonCreds tried to be too flexible in the implementation of the handling of "revocation intervals" and made things extremely complex. For sanity, the from and to in a presentation request should be the same -- or the from eliminated.

In Verifier, the code was like this:

var now = (uint)DateTimeOffset.Now.ToUnixTimeSeconds();

NonRevoked = new RevocationInterval
{
    From = 0,
    To = now
}

We still get the same error after removing the from. According to my guess, the from default value is 0. But, when I gave the from and to the same values, it worked.

var now = (uint)DateTimeOffset.Now.ToUnixTimeSeconds();

NonRevoked = new RevocationInterval
{
    From = now,
    To = now
}

Thanks for the help.