-
Notifications
You must be signed in to change notification settings - Fork 54
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
Dynamic Credential Verification #217
Dynamic Credential Verification #217
Conversation
Option interface{} | ||
} | ||
|
||
// GetVerificationOption returns a verification option given an ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing issue in comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there ever be a case where we would not run all verifiers?
Would a system admin like omit some verifiers on certain creds?
// NewCredentialVerifier creates a new credential verifier which executes in the order of the verifiers provided | ||
func NewCredentialVerifier(verifiers []Verifier) (*CredentialVerifier, error) { | ||
// dedupe | ||
var res []Verifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean this variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
credential/verification/verifiers.go
Outdated
|
||
var KnownVerifiers = []Verifier{ | ||
{ | ||
ID: "Go Validation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of Go Validation maybe like Object Validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, updated
if err != nil { | ||
// if the credential does not have a schema property, we cannot perform this check | ||
if !hasSchemaProperty { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this expected functionality?
If we you VerifyJSONSchema and it has no schema it will return nil (no error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a test for this case - if you do not pass in a schema AND the credential does not have a schema this will pass
here are the cases:
- no schema property, no schema provided = no error
- no schema property, schema provided = error
- schema property, no schema provided = error
- schema property, schema provided = depends on check
I think this is expected.
I think so. The go object model may not encapsulate all the fields that a verifier wishes to check, so they may deviate here. There also may be use cases where a verifier wants a credential even if it is expired - to prove you once had a credential, perhaps for a re-issuance flow.
I think so, yes. I imagine there can be verifiers defined per credential grouping. How that grouping is defined...not exactly sure. Maybe by schema, or by issuer. This could be functionality we add to the service. |
Codecov Report
@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 57.68% 58.01% +0.34%
==========================================
Files 33 35 +2
Lines 3901 3975 +74
==========================================
+ Hits 2250 2306 +56
- Misses 1272 1285 +13
- Partials 379 384 +5
|
merging, if you have follow ups I can take them in another PR @nitro-neal |
#213
Left out signing, covered everything else.