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

Should returning ProblemDetail objects be mandatory? #414

Open
msporny opened this issue Aug 20, 2024 · 6 comments
Open

Should returning ProblemDetail objects be mandatory? #414

msporny opened this issue Aug 20, 2024 · 6 comments
Assignees
Labels
pr exists A Pull Request exists to address this issue.

Comments

@msporny
Copy link
Contributor

msporny commented Aug 20, 2024

The VCDM, DI, BSL all have normative examples of ProblemDetail objects in their error handling sections. The verification algorithms have normative statements about exposing errors as well. The VC API doesn't specify how these objects are exposed to callers. Should the API do this?

@msporny
Copy link
Contributor Author

msporny commented Aug 20, 2024

The group discussed this on 2024-08-20:

@PatStLouis noted that the VCDM, DI, BSL all have normative examples of ProblemDetail objects in their error handling sections. The verification algorithms have normative statements about exposing errors as well. The VC API doesn't specify how these objects are exposed to callers. Should the API do this? @msporny noted that the reason we say MUST raise and error but SHOULD for the error is because we didn't want to raise burden on implementers. @PatStLouis noted that it's not easy to know why an error was thrown, including more information helps understand why the error was thrown. The VC API spec does currently speak to ProblemDetails, but there is no normative language related to including the data.

Can verification response be more dynamic and return more information? @dlongley noted that it would be good to have client request specific type of error or warning response, that would allow people to layer changes on top (additional feature to include ProblemDetails). @PatStLouis wondered if that is a "verbose" response flag, or something more specific? @dlongley didn't want to bikeshed, maybe we want something more semantically specific, like "ProblemDetails": "true".

@PatStLouis noted that for BitstringStatusList, the bitvalue that was fetched could be included in a check. Maybe the same for proofs, VC with set of proofs, return a property called proof which has property of proof that was verified w/ true/false for verified or not. @msporny noted concerns around complexity of checks and the complexity of data returned. @dlongley noted that we might be conflating running various things is "verification", what checks are run are a part of verification instance configuration. You want to be able to call a verifier instance, which might include running status checks, and it will provide the information back to the validation process and what one does with that information is part of the validation process. @PatStLouis noted that options for checks, should test suite inject that? Maybe people put that in their implementation (in options field, customized checks). If you tag a verifier instance w/ BSL checking, the status list is something the verifier will do.

@PatStLouis asked about the "option" to return "ProblemDetails". @dlongley noted that an input parameter might be needed to figure out what you want checked "check all status", or "check status of this type and purpose (e.g., revocation)" (how "strict" you want the verification to be). "As soon as you get an error, return HTTP 400 and exit immediately" or "Go as far as you can and return all errors". Do we need "strict mode" or "detail.

@msporny
Copy link
Contributor Author

msporny commented Sep 3, 2024

The group discussed this on 2024-09-03:

@PatStLouis noted that an option to get "verbose details" back as "ProblemDetails" is useful to understand what happened when a verification was performed; the option could be used to assert details on the verification process. With respect to BitstringStatusList, you return "credentialStatus" and have a list of credentialStatus Entry w/ value of the bit at the index (index value). @dlongley noted that we wanted "This is the status under status property" and bitfield value was, verified true/false, sibling property to status property. @PatStLouis noted "statusCredential" object, that contains same entries in VC with new element "verified: true/false" or bit value, leaning towards the bit value. Status message true/false might not be accurate. There are edge cases where status messages might have more than one index that might complicate things. For suspension/revocation, could provide simpler check.

@mburchil noted that implementations are leaving up to requester/wallet to provide at request time, wallets note whether they care whether or not something is suspended/revoked, and then true/false comes back in information provided by them. Don't provide information beyond true/false, relied on requester to state current status. In verification request, they state whether or not they care. @PatStLouis noted that endpoint will do the check, in light of different test suites... verify will return information, but not take action on it. Verify not meant to be called by wallet, component of software and it should inform software of verification information so software can take decisions based on information found. Verifier would return property that can be acted upon by coordinator. Coordinator might not care if a credential has been revoked. @mburchil noted that Entra is doing it the other way, field for "verification" whether or not you care if something is revoked or not. @PatStLouis noted that when someone provides endpoint, customized parameters if implementation checks status -- this question is about ProblemDetails.

... long discussion on what to do ensued ...

The group came to the following conclusions:

  • Issuance and verification endpoints are instances that are specific to certain types of VCs.
  • Issuance and verification endpoints MAY allow options that change the endpoints behavior.
  • When a credentialStatus is checked at a verification endpoint, an object of the following type is returned: {verified, statusResult: [{result, credentialStatus}, {result, credentialStatus}, ...]}

Return object should have:

  • warnings [array ProblemDetails]
  • errors [array ProblemDetails]
  • result {credentialStatus [{credentialStatus, result}], credentialSchema [{credentialSchema, result}], proof [{proof, result}] }

@PatStLouis PatStLouis self-assigned this Sep 10, 2024
@msporny msporny added ready for PR Issue ready to be resolved via a Pull Request pr exists A Pull Request exists to address this issue. and removed ready for PR Issue ready to be resolved via a Pull Request labels Sep 10, 2024
@robdefeo
Copy link

Where I had some confusion which lead me to not know about how to use the problem details was that the endpoints https://w3c-ccg.github.io/vc-api/#verify-credential and https://w3c-ccg.github.io/vc-api/#verify-presentation both return an errors array "Errors Each item in the errors array MUST be a string.".

Although it mentions https://w3c-ccg.github.io/vc-api/#verification-response that ProblemDetails should be raised, I assumed this was for libraries rather than API calls. Though problem details was intended for API responses. The RFC https://www.rfc-editor.org/rfc/rfc9457 seems to intend to return a single type of error, though there is an option to return multiple errors they are of a single type. In verification we perform distinct checks that return potentially different types of errors so, its hard to use as per rfc9457.

Error codes as a number are limiting, having known errors which can be easily identified is helpful, preferably it would be a namespaced maybe with a delimeted string to prevent or at least reduce clashes.

@PatStLouis
Copy link
Collaborator

@robdefeo thank you for this valuable feedback as an implementer reading the specification. The errors/warnings array where the legacy response objects prior to the introduction of problem details in vcdm 2.0. #418 is looking to clarify the response objects and update according to the specification. Please note that these are designed to be optional and enabled through configuration, such as an option returnProblemDetails. The goal is to enable these endpoints to return more verbose responses. One of our use case for this is to better assert implementation responses during test-suite conformance. As more features are being looked at such as proof chains, multi-statuses and such, it becomes valuable to provide test clients sufficient information to assert an implementation's rejection of a request. This feature can also assist an implementer during development phases.

Please let us know how we can improve documentation around this and make it as seamless as possible to implement.

@robdefeo
Copy link

This clarification is helpful, what I think readers will benefit from is the spec text being updated inline with the change to the yml, see

These sections make no mention of problem details in the response, in fact it explicitly states string
image

The reference PR updates the openAPI but I couldn't see if update the index.html, but that seems to be happening via some imports of the spec so it may well be solved, but its hard for me to tell.

@PatStLouis
Copy link
Collaborator

@robdefeo Thank you for pointing this out and yes you are correct, we very much want the spec text to be updated to reflect this. There seems to be some problem with linting I will have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr exists A Pull Request exists to address this issue.
Projects
None yet
Development

No branches or pull requests

3 participants