Feat/revocation notification v2

[TheTechmage]

This PR implements Revocation Notification V2, as described in hyperledger/aries-rfcs#721. At this time, V1 takes precedence over V2 when a revocation occurs and V2 never gets fired off. After talking with @dbluhm, I plan to implement a config option to disable/exclude the loading of specific plugins so that, in terms of configuration, one can disable V1 notifications to allow V2 notifications to be triggered. Since Indy-Anoncreds are currently the only standard to be implemented at this time, there are a few hard coded values pointing to "indy-anoncreds" until such a time where a second credential revocation format needs to be implemented.

[dbluhm]

@andrewwhitehead are the tests that are failing related to the Askar update you mentioned on the ACA-PUG call today?

[ianco]

This PR implements Revocation Notification V2, as described in hyperledger/aries-rfcs#721. At this time, V1 takes precedence over V2 when a revocation occurs and V2 never gets fired off. After talking with @dbluhm, I plan to implement a config option to disable/exclude the loading of specific plugins so that, in terms of configuration, one can disable V1 notifications to allow V2 notifications to be triggered. Since Indy-Anoncreds are currently the only standard to be implemented at this time, there are a few hard coded values pointing to "indy-anoncreds" until such a time where a second credential revocation format needs to be implemented.

So, just to clarify, the V1 revocation notification will run by default (and the V2 revocation notification won't get sent) unless the V1 notification protocol is disabled using this PR? #1737

[TheTechmage]

That is correct. The reason for this is that revocation notification loads in first (due to the definition.py), and they both work off the same DB entries from revocation notifications. V1 deletes the DB entry before V2 can get to it, and thus, is essentially disabled

[codecov-commenter]

Codecov Report

Merging #1734 (ac4fe65) into main (a4013f4) will increase coverage by 0.01%.
The diff coverage is 98.61%.

@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
+ Coverage   95.19%   95.21%   +0.01%     
==========================================
  Files         525      530       +5     
  Lines       33091    33235     +144     
==========================================
+ Hits        31502    31644     +142     
- Misses       1589     1591       +2     

[ianco]

Per the discussion in this PR: #1737

• Add a parameter to the "revocation" Admin API call to say what version of the notification protocol to use
• Add a (yet another) startup parameter to always use a given notification protocol version.

I vote we add these options to the revocation v2 PR (#1734)

This solves the immediate problem where revocation notification V1 and V2 don't work together:

• the new startup parameter is the default version of the notification to use
• the new parameter in the API endpoint can override the notification version

This is consistent with how we've done this kind of thing in other endpoints.

[TheTechmage]

I'm not exactly happy with adding another CLI argument for this, but I don't have any real problems with it either. If agreed upon that these changes are to be made then I'll get started on making these changes.

[ianco]

I'm not exactly happy with adding another CLI argument for this

We should come up with a rule that if you add a new CLI argument you have to remove one as well :-D

[swcurran]

Would a GUI generator be possible to allow for interactively creating a the config? Or would we just need a config for that? :-)

[ianco]

Would a GUI generator be possible to allow for interactively creating a the config? Or would we just need a config for that? :-)

We have the capability to use config files (--arg-file <something.yml>) so I don't think we need to build a gui, we just need to provide some config.yml examples

... and I don't think it's a real issue to have lots and lots of CLI parameters, as long as they all have reasonable defaults

[ianco]

Discussed in the aca-pug call today and the consensus was to:

• add a (required) parameter to the revocation endpoint to specify which notification version to send
• no new CLI parameter

(Eventually we expect the notification version to be determined through feature discovery)

@frostyfrog

[ianco]

See comment re adding a version parameter to the endpoint

[TheTechmage]

Sounds good! I'll get those changes made.

[etschelp]

Just as a remark, I tested this and it is not backwards compatible. I was using the config from aca-py 0.7.3, meaning:

notify-revocation: true
monitor-revocation-notification: true

and also in the rest api "notify": true

After upgrading to the latest revision from main revocation notifications were not sent any more until I set a version either v1 or v2 in the rest api. Meaning for anyone who is not aware of this revocation notifications will stop working.

[TheTechmage]

I wonder why that's happening. My last commit was supposed to set a default value if nothing was provided in the API call. What you're suggesting, however, is that the notify_version = body.get("notify_version", "v1_0") and notify_version = data.get("notify_version", "v1_0") are not providing a default of v1_0 when notify_version is missing. The integration tests are testing for this very thing by leaving out the notify_version key. Is the problem that you are encountering simple to reproduce?

[ianco]

Could be setting the attribute to an empty string vs null value?

[TheTechmage]

I'll see if I can take a look at that. I feel that we should not be getting an empty string when the key does not exist.

[etschelp]

If I use the following payload the issuer will not send the revocation notification:

{
    "connection_id": "3a0060a9-2774-42a5-b5bd-80bb65f8538e",
    "cred_rev_id": "20",
    "notify": true,
    "publish": true,
    "rev_reg_id": "F6dB7dMVHUQSC64qemnBi7:4:F6dB7dMVHUQSC64qemnBi7:3:CL:571:phil-test-83-multi:CL_ACCUM:65514d6b-9c4a-473b-869a-c22b9fc21b24"
}

Together with config flags:

notify-revocation: true
monitor-revocation-notification: true

If I use the same payload but add the notify_version either 1 or 2 the event will be sent.

[ianco]

I'll see if I can take a look at that. I feel that we should not be getting an empty string when the key does not exist.

I think you can say default=None or even default="v1_0"