hyperledger-labs / business-partner-agent

The Business Partner Agent is a SSI wallet and controller based on aries cloud agent python.
https://labs.hyperledger.org/business-partner-agent/
Apache License 2.0
56 stars 49 forks source link

Cannot sattistfy request with String value == restriction. #591

Open Jsyro opened 3 years ago

Jsyro commented 3 years ago

This proof request

{
   "id":"88719e13-4129-41a4-8434-f08e7a1e616b",
   "partnerId":"19b0d483-905b-46a3-ad19-938de8b5dd0f",
   "exchangeVersion":"V1",
   "state":"request_received",
   "role":"prover",
   "updatedAt":1630441579762,
   "stateToTimestamp":{
      "request_received":1630441579761
   }"typeLabel":"BC MInes Act Permit",
   "proofRequest":{
      "name":"BC MInes Act Permit",
      "version":"1.0",
      "nonce":"113436436443681711055586",
      "requestedAttributes":{
         "QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0":{
            "names":[
               "Permitee",
               "Permit-number"
            ]"nonRevoked":{
               "from":1630441579,
               "to":1630441579,
               "set":true
            }"restrictions":[
               {
                  "schema_id":"QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0",
                  "issuer_did":"QTNLwTnFaqtVTgjj2DZuC9",
                  "attr::Permit-number::value":"P-100"
               }
            ]
         }
      }"requestedPredicates":{

      }
   }"canBeFullfilled":false
}

Is saying it cannot be fulfilled, here is the matching credential in my wallet that should satisfy it.

{
   "id":"d0441610-4782-487e-a95f-079526df3778",
   "issuedAt":1630441206651,
   "state":"credential_acked",
   "isPublic":false,
   "issuer":"gov",
   "schemaId":"QVdfoxctydwAvScxGkUyor:2:mines_act-permit:1.0",
   "credentialDefinitionId":"QTNLwTnFaqtVTgjj2DZuC9:3:CL:1902:test",
   "connectionId":"10026690-25fa-4c56-94c3-a8eb7e4b4eff",
   "revocable":false,
   "exchangeVersion":"V1",
   "typeLabel":"mines_act-permit",
   "credentialData":{
      "Permitee":"Big Mining Inc",
      "Permit-number":"P-100"
   }"label":""
}

if i manually enable the button and click it. I get cannot get credentialInfo of undefined, PresentationExList.vue line 283.

This is the JSON of the proof request template that made the presentaitonRequest.

{
  "id": "4ac1c81f-33b7-47e5-966e-6cb99272b94a",
  "createdAt": "2021-08-31T20:26:19.380+0000",
  "name": "BC MInes Act Permit",
  "attributeGroups": [
    {
      "schemaId": "ecca792a-ef4f-403e-a37e-34e6bbc31841",
      "attributes": [
        {
          "name": "Permitee",
          "conditions": []
        },
        {
          "name": "Permit-number",
          "conditions": [
            {
              "operator": "==",
              "value": "P-100"
            }
          ]
        }
      ],
      "nonRevoked": true,
      "schemaLevelRestrictions": {
        "issuerDid": "QTNLwTnFaqtVTgjj2DZuC9"
      }
    }
  ]
}

Possibly related is that i cannot inspect the details of the proof template (picture below is of the verifier) same screen as the above json blob.

image

Jsyro commented 3 years ago

I tried different variations of restrictions and Cred Def id restriction also failed.

Schema id is redundant as it's added automatically anyways.

More testing results will be posted later.

domwoe commented 3 years ago

Thanks for reporting. Could you provide the raw data from the presentation exchange dialog? If there's no matching credential in this data it would be helpful if you'd try the matching credentials endpoint of the BPA API directly.

etschelp commented 3 years ago

I reconstructed the above and aca-py returns an empty array for the wallet matches in this case

etschelp commented 3 years ago

I played around some more and it seems that upper case characters in a schema attribute will cause this none match. The following example works:

        "proofRequest": {
            "name": "Permit dash underscore",
            "version": "1.0",
            "nonce": "1034732971840382951225191",
            "requestedAttributes": {
                "EraYCDJUPsChbkw7S1vV96:2:mines_act-permit:3.0": {
                    "names": [
                        "permitee",
                        "permit-number"
                    ],
                    "nonRevoked": {
                        "from": 1630486421,
                        "to": 1630486421,
                        "set": true
                    },
                    "restrictions": [
                        {
                            "schema_id": "EraYCDJUPsChbkw7S1vV96:2:mines_act-permit:3.0",
                            "attr::permit-number::value": "p-100"
                        }
                    ]
                }
            },
            "requestedPredicates": {}
        }

I guess the easiest solution to this is to sanitize all attribute names when we create the schema. Meaning allowing only a-z,0-9,_-

domwoe commented 3 years ago

Looking at https://github.com/hyperledger/indy-sdk/blob/113b79cd64a238130d20e19b972326f72047c550/libindy/src/api/anoncreds.rs#L1529 it should not matter.

It might be a bug in ACA-Py. The normalize method here should probably include making the names lowercase.

etschelp commented 3 years ago

I think validation and attribute names should follow the spec, if the spec allows it upper case attributes should not be normalized. This looks like there is some normalization going on during request matching and then the wallet value is not found because permit != Permit

swcurran commented 3 years ago

I was never certain whether the normalization was happening in ACA-Py (seems wrong), or because the Indy-SDK required it (also seems wrong). Unfortunately, the developer that knew about this (@sklump) is not with the project anymore and so not available to answer. @ianco or @andrewwhitehead -- any ideas?

domwoe commented 3 years ago

I did a small test.

  1. I created a schema with attribute MyAttrib and issued a credential based on this.
  2. I tried to fetch the credential with ACA-Py's GET: /credentials endpoint using a WQL query
    • Query { "attr::MyAttrib::marker": "1"} provides an empty result
    • Query { "attr::myattrib::marker": "1"} provides the credential

Based on this I think we should change ACA-Py's normalize function to include making the attributes lowercase.

etschelp commented 3 years ago

@domwoe I see some issues with that. 1. The normalize function also gets called when a credential gets loaded, which seems odd. 2. I think that this does not fix the root cause. According to your description the root issue seems to be in the wallet query. 3. Any change needs to happen in a backwards compatible way, and then you need indeed normalize when loading credentials.

domwoe commented 3 years ago

I got confused...

We have different behavior depending on usage of indy-sdk or askar/credx:

Indy SDK

Result: attr::{attribute}::marker and attr::{attribute}::value restrictions do not work for attributes containing upper case letters or spaces.

Askar/Credx

Result: attr::{attribute}::marker and attr::{attribute}::value restrictions do not work for attributes containing spaces.

Another thing we noticed. The get_credentials function is not implemented in askar/credx. Hence, ACA-Py'sGET: /credentials endpoint will always return null in this case.

Suggestion

If normalization is necessary, normalize the restrictions/WQL query accoding to the implemented normalization function which differs between indy-sdk and askar.

swcurran commented 3 years ago

Thanks for pushing on this. @andrewwhitehead -- FYI on this.

What is the right handling at the storage level, lower(), strip_spaces(), nothing, both, something else?

Presumably, the corresponding handling should be at the lowest level possible in using the attributes. Ideally at the WQL level, but if we don't want to change the indy-sdk, then at the ACA-Py level, at least for when Indy is the backend.

ianco commented 3 years ago

I recommend making the change in aca-py, so we can ensure the behaviour is consistent across the underlying sdk's. (Also it is a challenge to release a new indy-sdk version.)

andrewwhitehead commented 3 years ago

Yep, sounds like we need to do more processing on the WQL to normalize the attribute names. For the record Indy uses this method to normalize them (not sure why it's not applied to the WQL): https://github.com/hyperledger/indy-sdk/blob/dbd89cf94a73e7a62611c4150a874c38b810ff8d/libindy/src/services/anoncreds/helpers.rs#L20