OpenID4VCI: Validate credential_definition and compare with credential #2308
Conversation
gerardsn
changed the title
vcr/oidc4vci/validators.go
Outdated
| if isOffer { | ||
| return errors.New("invalid credential_definition: credentialSubject not allowed in offer") | ||
| } | ||
| // TODO: add credentialSubject validation. |
There was a problem hiding this comment.
we currently do not use the credentialSubject, and I am not sure what to validate here. We could add some basic validation but that might give us more issues later on if the validation turns out to be incorrect.
There was a problem hiding this comment.
then it's not a todo for the code. If we need to remember something, it should be listed in an issue not in the code.
There was a problem hiding this comment.
created issue #2320 and referenced in to TODO
vcr/oidc4vci/validators.go
Outdated
| ) | ||
|
|
||
| // ValidateCredentialDefinition validates the CredentialDefinition according to the VerifiableCredentialJSONLDFormat format | ||
| func ValidateCredentialDefinition(definition *CredentialDefinition, isOffer bool) error { |
There was a problem hiding this comment.
you could put this func on CredentialDefinition, making the nil check obsolete
There was a problem hiding this comment.
doesn't that just move the nil check outside this function before calling the validator on the CredentialDefinition?
There was a problem hiding this comment.
I've made it a function on the CredentialDefinition, but it still contains the nil check.
There was a problem hiding this comment.
That's because of the pointer receiver. Do we always need a definition on an offer. Then the offer can also remove the pointer from the definition. You'll get an empty struct but validation would fail for that as well. Less pointers equals easier life.
There was a problem hiding this comment.
the CredentialDefinition is optional since it is only part of the verifiable credential flows, not de mDL flows. We only support ldp_vc for now where it is required. So I could remove the pointer, but then we'd have to add an Empty() check to assert that it was provided.
There was a problem hiding this comment.
Unless you don't care about the specific error and just let it fail on missing @context. I prefer the current solution as it is closest to the spec and therefor easiest to understand/extend.
gerardsn
force-pushed
the
oidc/add-OfferedCredential-and-CredentialDefinition-types
branch
from
6f0dfbd to
5af0a00
Compare
gerardsn
deleted the
oidc/add-OfferedCredential-and-CredentialDefinition-types
branch
new approach to validation of credential and credential_definition #2037 and #2050
per commit:
OfferedCredentialandCredentialDefinitiontypes to make validation and parsing easierCredentialRequestandCredentialOffer