bcgov / aries-vcr

Hyperledger Aries Verifiable Credential Registry (VCR) is a set of application level software components designed to accelerate the adoption of trustworthy entity to entity communications.
Apache License 2.0
78 stars 67 forks source link

Investigate recommended alternative approach for verify #365

Closed nrempel closed 4 years ago

nrempel commented 4 years ago

1) current state of the 1893 PR. It doesn't solve the problem. It just ignore every restrictions by name aside from the current one disclosed attribute. 0) master branch behaviour - always return false for the case with multiple restrictions So 1) may have false-positive result, 0) false-negative That's why I haven't merged it yet 1 can be extended by application check that all attributes are from the same proof. To do it the app can just make sure that sub_proof_index are the same for whole group of attrs 2) extension for 1): Allow only the restrictions for attributes (by value) which are disclosed near the current one attrrib and does validation in libindy around that. It seems like a solution for your particular case, but not the general solution and may be unclear for users 3) introduce more concrete restriction which would force to use some attributes from the same credential. Kind of grouping of attributes.

proof req {
  requestedAttrs: {
    attrXref: { name: X, restrictions: { attr::X::value == valX, groupedWith == groupRef1 }
    attrYref: { name: Y, restrictions: { attr::Y::value == valY, groupedWith == groupRef1 }
   } 
}

For me the 3rd one seems generic enough and should solve your problem as well (if I understood it correctly) Let me know are the descriptions of all solutions and the difference are clear enough. Fill free to ask questions. I'd like to be aligned at understanding the options and then we can choose the right solution

nrempel commented 4 years ago

More discussion about a possible solution here: https://github.com/hyperledger/indy-sdk/pull/1893

nrempel commented 4 years ago

A solution was implemented in https://github.com/hyperledger/indy-sdk/pull/1958/files

I am testing these changes.

nrempel commented 4 years ago

I was blocked by build failures last week. I'm testing changes now with 1.13 stable release.

nrempel commented 4 years ago

This is ready and in review https://github.com/bcgov/indy-catalyst/pull/404.

@esune I'm going to leave the PR open for the moment. We may want to wait for this to land in a stable release before merging. If there is no rush, that is the least disruptive and least effort path.

esune commented 4 years ago

@nrempel I merged the PR and logged #412 to track the need to update the base image to an "official" release.

As far as my understanding goes, having the self-verify functionality work all the way through is required for the launch of ICOB so it is best if we merge the change and track future updates.

@swcurran and @WadeBarnes can confirm or deny my claims ;)

nrempel commented 4 years ago

Sounds good!

swcurran commented 4 years ago

Not strictly required for ICOB deployment, but definitely something we want ASAP.