Add an endpoint to verify presentations #630
Conversation
decentralgabe
requested review from
nitro-neal and
andresuribe87
as code owners
Codecov Report
@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 26.64% 26.54% -0.10%
==========================================
Files 55 55
Lines 5855 5876 +21
==========================================
Hits 1560 1560
- Misses 4023 4044 +21
Partials 272 272
|
decentralgabe
force-pushed
the
issue-615-verification-endpoint
branch
from
740a33a to
ba132b3
Compare
| kid, ok := jwtKID.(string) | ||
| if !ok { | ||
| return sdkutil.LoggingNewError("JWT kid is not a string") | ||
| issuerKey, err := did.GetKeyFromVerificationMethod(issuerDID.Document, issuerKID) |
There was a problem hiding this comment.
issuerPublicKey for clarity of future readers.
| // for each credential in the presentation, run a set of static verification checks | ||
| for _, cred := range vp.VerifiableCredential { | ||
| _, _, normalizedCred, err := parsing.ToCredential(cred) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "error parsing credential in presentation<%v>", cred) | ||
| } | ||
| if err = v.staticValidationChecks(ctx, *normalizedCred); err != nil { | ||
| return errors.Wrapf(err, "error running static validation checks on credential in presentation<%v>", normalizedCred.ID) | ||
| } |
There was a problem hiding this comment.
Consider reusing some of the stuff we have:
creds, err := NewCredentialContainerFromArray(vp.VerifiableCredential)
if err != nil {
return err
}
for _, cred := range creds {
if err := v.Verify(ctx, cred); err != nil {
return err
}
}There was a problem hiding this comment.
nice forgot about this. ideally some of this logic should move to the sdk to mirror the credential functionality
| Holder: holderDID.String(), | ||
| } | ||
|
|
||
| // invalid verifiable presentation with no credentials |
There was a problem hiding this comment.
Here and elsewhere, better to use t.Run("verifying verifiable presentation with no credentials returns false", func ... if possible.
There was a problem hiding this comment.
was following the pattern you had below...
| t.Run("Create returns error without input descriptors", func(tt *testing.T) { | ||
| s := test.ServiceStorage(tt) | ||
| pRouter, _ := setupPresentationRouter(tt, s) | ||
| tt.Run("Create returns error without input descriptors", func(ttt *testing.T) { |
There was a problem hiding this comment.
When I was doing the SQL implementation I realized why this approach isn't great. There is not reason why code should need to access the upper level *testing.T object. In fact, using the incorrect t leads to incorrect setup which is very difficult to catch.
Can you elaborate why it's preferred to use the t, tt, ttt approach?
There was a problem hiding this comment.
the idea is that each test runner should be able to act independently, if we look at what's provided by t *testing.T we see that it's a test context responsible for the lifecycle of a test...so separating it out per running test has a few benefits:
- a child failing test won't result in a parent failing test (we want to see all failures individually)
- individual tests can be skipped / toggled by using
t.Skip() - individual tests can have their own cleanup functions using
t.Cleanup()
probably more too. just better isolation/separation of concerns
separately -- I think it's a mistake to couple storage to these tests. I believe we should have a separate set of storage tests that do a kind of "steel thread" for the service's functionality instead of running every test for every storage provider
Co-authored-by: Andres Uribe <[email protected]>
|
feedback accepted - added an action in this ticket to add an integration test for all verification endpoints #615 |
fix #631