Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added encrypted StatusList APIs + enhanced type safety #350

Merged
merged 39 commits into from
Aug 25, 2023

Conversation

Eengineer1
Copy link
Contributor

  • Adds encrypted / unencrypted status list distinction.
    • Enriched unencrypted.
    • Overall encrypted implementation.
    • Enhanced check APIs.
    • Performant search APIs.
  • Adds intermittent payment layer on demand.
  • Enhances type safety.
  • Monkey patches on globalThis replace issue.
  • Relevant documentation.

@Eengineer1 Eengineer1 changed the title Encrypted status list feat: Added encrypted StatusList APIs + enhanced type safety Aug 21, 2023
Copy link
Contributor

@ankurdotb ankurdotb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good PR overall and a delight to read through/review!

I did make some changes:

  • As discussed, I removed encrypted from the top-level response. Please cross-check I got it removed in all places (pretty sure I have, but worth a second pair of eyes).
  • I have made some changes by leveraging schema reuse a lot more widely within the Credential Status API Swagger docs. This makes it shorter and less verbose (especially large, repeated example blocks). I didn't do this for any of the APIs as it might make the PR harder to read/review if there are non Credential Status APIs to review, but probably worth a pass by Abdulla from a similar lens after this is done.

Next steps:

  • The only minor comment is the /check API, which asks for statusPurpose as a query parameter. This is redundant, since based on the DID, the status list name etc we already search/look it up to determine if it's got payment conditions or not. The input provided in statusPurpose therefore is already present in the resource metadata and the resource body under resourceType, in the body, and again in the metadata in body. So IMO this should be removed since user input is not required to determine the status purpose.
  • For the /update actions, the statusAction should match the resource type. For example, if the chosen action is revoke, but the resourceType / type within the resource provided is StatusList2021Suspension, then this action should fail. Now, I haven't checked did-provider-cheqd to see if it's enforced there instead, but if not, we should. Regardless of did-provider-cheqd, not sure whether we want to catch/handle the same case here (e.g., did-provider-cheqd might already reject, but is there a relevant error message surfaced to the user?)

src/types/shared.ts Outdated Show resolved Hide resolved
@Tweeddalex
Copy link
Contributor

Tweeddalex commented Aug 25, 2023

  • The only minor comment is the /check API, which asks for statusPurpose as a query parameter. This is redundant, since based on the DID, the status list name etc we already search/look it up to determine if it's got payment conditions or not. The input provided in statusPurpose therefore is already present in the resource metadata and the resource body under resourceType, in the body, and again in the metadata in body. So IMO this should be removed since user input is not required to determine the status purpose.

Hey @ankurdotb I'm not sure this is entirely correct. In my opinion the statusPurpose is required as a query parameter because there is a scenario where:

  1. I create a StatusListRevocation with the name: employee-credentials
  2. I ALSO create a StatusListSuspension with the name: employee-credentials
  3. I.e. there may be multiple resource types with the same resource name.
  4. In this scenario, without specifying the statusPurpose as a query parameter, there will be ambiguity whether I'm trying to check the Revocation list or the Suspension list.
  5. Therefore, DID, index, encrypted and statusListName are not enough on their own to specify a particular resource and status list where multiple resourceNames are the same, with different resourceTypes.

@Eengineer1
Copy link
Contributor Author

Underlying search operations are based on user input + validated against the desired action, therefore statusPurpose is not redundant.

On underlying did-provider-cheqd functionality, it's deterministically action based + takes relevant information from the credential body, namely the credential status index section, which acts as the single source of truth. Cross-purpose validation handles edge cases.

Having that in mind, both create + update, should provide the purpose / action top-level, which is utilised for a sanity check, if a relevant status list has been found, is encrypted / unencrypted.

So I don't think we should remove either.

@Eengineer1
Copy link
Contributor Author

I've enhanced the runtime removal of underlying encrypted property as well, pairing yesterday's conversation.

I'd scope further conversations to relevant changes / technical points required + take additional discussions elsewhere, if no objections.

@Eengineer1
Copy link
Contributor Author

Eengineer1 commented Aug 25, 2023

Swagger reusability type additions make sense + thanks!

@ankurdotb ankurdotb merged commit 951e2b4 into develop Aug 25, 2023
15 checks passed
@ankurdotb ankurdotb deleted the encrypted-status-list branch August 25, 2023 09:58
cheqd-bot bot pushed a commit that referenced this pull request Aug 25, 2023
## [2.9.0-develop.4](2.9.0-develop.3...2.9.0-develop.4) (2023-08-25)

### Features

* Added encrypted StatusList APIs + enhanced type safety ([#350](#350)) ([951e2b4](951e2b4))
@cheqd-bot
Copy link

cheqd-bot bot commented Aug 25, 2023

🎉 This PR is included in version 2.9.0-develop.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

cheqd-bot bot pushed a commit that referenced this pull request Aug 29, 2023
## [2.9.0](2.8.0...2.9.0) (2023-08-29)

### Features

* Added encrypted StatusList APIs + enhanced type safety ([#350](#350)) ([951e2b4](951e2b4))
* Change from `did/:did` to `did/search/:didUrl` endpoint and support query parameters for DID endpoint in Credential Service [DEV-3097] ([#331](#331)) ([4e34b76](4e34b76))

### Bug Fixes

* Credential Service Staging - Resolve issue with page not loading and broken login [DEV-3162] ([#358](#358)) ([419d2ec](419d2ec))
* Fix problem with agent initialize [DEV-3146] ([#346](#346)) ([45e1074](45e1074))
* Validate did access on operation ([#351](#351)) ([8bbfce1](8bbfce1))
@cheqd-bot
Copy link

cheqd-bot bot commented Aug 29, 2023

🎉 This PR is included in version 2.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cheqd-bot cheqd-bot bot added the released label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants