BREAKING: VCHolder multitenant binding

[jamshale]

I know this "soft binding" was done for a reason, but it appears the multitenant storage issue is happening because of it. It was causing credentials to be stored in the base wallet. It's the only class with the soft binding.

Also for some reason the tenant authentication method was missing on this endpoint for some reason and had no openapi examples :/

@dbluhm Do you have insight into the soft_binding? I can't really remember what it's for.

[PatStLouis]

@jamshale the missing tenant auth would be on me as I added this route. As for the soft binding I'm not aware of it's concept and seems to be at another layer in the code. Great find

[dbluhm]

The goal of the "soft binding" was to enable binding a separate vc holder implementation to, for example, delegate storage of credentials to an external service. This was a need that we had but it would seem I didn't consider implications on multi-tenancy deeply enough.

[dbluhm]

I'd like to take just a bit of time to understand how the soft binding caused this behavior. I'll report back very soon or else we can go ahead and merge with Patrick's approval.

[jamshale]

No problem, let's wait a bit then.

[dbluhm]

Discussed in discord but for posterity's sake recording the discussion here.

I pulled down Jamie's changes and dug around a bit and came up with a solution that enable us to retain the soft binding while making sure the correct profile was being used with the VC Holder interface.

I also did a bit of tuning on the VC routes open api, building off of the updates Jamie had already made, correcting a few more things that were causing the OpenAPI to not accurately reflect what the endpoints expected. One of those changes included making the return value of GET /vc/credentials an object with a results attribute rather than a flat list. This is mostly a quirk of the Marshmallow + OpenAPI generation framework we use but it doesn't support expressing a flat list (to my knowledge). Additionally, this aligns with conventions used elsewhere in ACA-Py.

This does technically constitute a breaking change and also means that this endpoint no longer perfectly aligns with https://w3c-ccg.github.io/vc-api/#get-credentials. @PatStLouis recommended we go ahead with this change regardless.

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[PatStLouis]

I will suggest the same change to the vc-api group so we maintain alignment.

[PatStLouis]

The intention here was to enable the controller to set tags and pass them through options if they want to build a custom tag querying integration. This can be revisited at a later time, and we would likely provide the tags directly to the store_credential function instead of the options object.

[swcurran]

Can someone please put into a comment a paragraph about the breaking change.

PR 3391 impacts those using…

[jamshale]

I'm going to squash and merge this soon. I can look into the backports after.

[dbluhm]

@swcurran

PR #3391 impacts those using the GET /vc/credentials endpoint; the response is now an object with a single results attribute where it was previously just a flat list.

[dbluhm]

We can safely ignore Sonar Cloud's complaint; most of the lines it's complaining about are just OpenAPI schemas and the remainder are covered by the scenario testing.

[swcurran]

Prepping the 0.12.lts release -- what has the bug that resulted in this change being made? The impact is clear, but not the why the change had to be made. Would also like to characterize what kind of ACA-Py deployment would be impacted. I think it is just those using VC-LD, right?

[jamshale]

@daniel can probably explain better but this was causing issues anywhere the VCHolder class was injected when in multitenancy mode. It's used in the dif and json_ld formats as well.

I think multitenancy holders aren't a very common use case. Likely why it wasn't tested and didn't get reported/found until now.

[jamshale]

We need to do the other patch releases as well. Wanted to do the oldest/hardest one first.

[swcurran]

I think it is that a VC received into one tenant wallet, and as a result was visible in all tenant wallets. Right?

[jamshale]

Yes. Kind of.

The VC would end up in the admin wallet. It was visible to the admin wallet. Not all wallets. It looked like it was available to all wallets in admin insecure mode.

[jamshale]

It was actually getting issued to the admin wallet instead of the tenant wallet.