From 7f09aef33c960afb3c9e4a1d679fa57798230357 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Mon, 8 May 2023 13:10:20 +0200 Subject: [PATCH 1/2] Didman: make organization search more robust by only logging VC/DID document processing errors --- didman/didman.go | 19 +++++++++++--- didman/didman_test.go | 58 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/didman/didman.go b/didman/didman.go index 104f94cd1d..85e7a08f79 100644 --- a/didman/didman.go +++ b/didman/didman.go @@ -427,15 +427,23 @@ func (d *didman) resolveOrganizationDIDDocuments(organizations []vc.VerifiableCr j := 0 for i, organization := range organizations { document, organizationDID, err := d.resolveOrganizationDIDDocument(organization) - if err != nil && !(errors.Is(err, types.ErrNotFound) || errors.Is(err, types.ErrDeactivated)) { - return nil, nil, err + if errors.Is(err, types.ErrDeactivated) || errors.Is(err, types.ErrNotFound) || errors.Is(err, did.ErrInvalidDID) { + // Just ignore deactivated DID documents or VCs that don't refer to an existing DID document. + // Log it on debug, because it might be useful for finding VCs that need to be revoked (since they're invalid). + log.Logger(). + WithError(err). + WithField(core.LogFieldCredentialID, organization.ID). + WithField(core.LogFieldDID, organizationDID.String()). + Debug("Unable to resolve organization DID document (invalid VC?)") + continue } if document == nil { - // DID Document might be deactivated, so just log a warning and omit this entry from the search. + // Some other error occurred, log a warning and omit this entry from the search. log.Logger(). WithError(err). + WithField(core.LogFieldCredentialID, organization.ID). WithField(core.LogFieldDID, organizationDID.String()). - Warn("Unable to resolve organization DID Document") + Warn("Unable to parse organization VC and/or subject DID document") continue } didDocuments[j] = document @@ -449,6 +457,9 @@ func (d *didman) resolveOrganizationDIDDocuments(organizations []vc.VerifiableCr } func (d *didman) resolveOrganizationDIDDocument(organization vc.VerifiableCredential) (*did.Document, did.DID, error) { + if len(organization.CredentialSubject) == 0 { + return nil, did.DID{}, errors.New("no credential subjects in organization credential") + } credentialSubject := make([]credential.BaseCredentialSubject, 0) err := organization.UnmarshalCredentialSubject(&credentialSubject) if err != nil { diff --git a/didman/didman_test.go b/didman/didman_test.go index 33deeef0b1..1a0115c0a4 100644 --- a/didman/didman_test.go +++ b/didman/didman_test.go @@ -711,7 +711,7 @@ func TestDidman_SearchOrganizations(t *testing.T) { assert.NotNil(t, actual) assert.Empty(t, actual) }) - t.Run("ok - DID document not found (logs, omits result)", func(t *testing.T) { + t.Run("ok - DID document not found (omits result)", func(t *testing.T) { ctx := newMockContext(t) ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil) ctx.docResolver.EXPECT().Resolve(testDIDB, nil).Return(nil, nil, types.ErrNotFound) @@ -722,7 +722,7 @@ func TestDidman_SearchOrganizations(t *testing.T) { assert.NotNil(t, actual) assert.Empty(t, actual) }) - t.Run("ok - DID document deactivated (logs, omits result)", func(t *testing.T) { + t.Run("ok - DID document deactivated (omits result)", func(t *testing.T) { ctx := newMockContext(t) ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil) ctx.docResolver.EXPECT().Resolve(testDIDB, nil).Return(nil, nil, types.ErrDeactivated) @@ -733,15 +733,63 @@ func TestDidman_SearchOrganizations(t *testing.T) { assert.NotNil(t, actual) assert.Empty(t, actual) }) - t.Run("error - other error while resolving DID document", func(t *testing.T) { + t.Run("ok - invalid subject ID (omits result)", func(t *testing.T) { + ctx := newMockContext(t) + credentialWithInvalidSubjectID := vc.VerifiableCredential{} + _ = json.Unmarshal([]byte(jsonld.TestOrganizationCredential), &credentialWithInvalidSubjectID) + credentialWithInvalidSubjectID.CredentialSubject = []interface{}{ + map[string]interface{}{ + "id": "90", + }, + } + + ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{credentialWithInvalidSubjectID}, nil) + + actual, err := ctx.instance.SearchOrganizations(reqCtx, "query", nil) + + assert.NoError(t, err) + assert.NotNil(t, actual) + assert.Empty(t, actual) + }) + t.Run("ok - missing subject ID (omits result)", func(t *testing.T) { + ctx := newMockContext(t) + credentialWithoutSubjectID := vc.VerifiableCredential{} + _ = json.Unmarshal([]byte(jsonld.TestOrganizationCredential), &credentialWithoutSubjectID) + credentialWithoutSubjectID.CredentialSubject = []interface{}{ + map[string]interface{}{}, + } + + ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{credentialWithoutSubjectID}, nil) + + actual, err := ctx.instance.SearchOrganizations(reqCtx, "query", nil) + + assert.NoError(t, err) + assert.NotNil(t, actual) + assert.Empty(t, actual) + }) + t.Run("ok - no subject (omits result)", func(t *testing.T) { + ctx := newMockContext(t) + credentialWithoutSubjectID := vc.VerifiableCredential{} + _ = json.Unmarshal([]byte(jsonld.TestOrganizationCredential), &credentialWithoutSubjectID) + credentialWithoutSubjectID.CredentialSubject = nil + + ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{credentialWithoutSubjectID}, nil) + + actual, err := ctx.instance.SearchOrganizations(reqCtx, "query", nil) + + assert.NoError(t, err) + assert.NotNil(t, actual) + assert.Empty(t, actual) + }) + t.Run("ok - other error while resolving DID document (just logged)", func(t *testing.T) { ctx := newMockContext(t) ctx.vcr.EXPECT().Search(reqCtx, searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil) ctx.docResolver.EXPECT().Resolve(testDIDB, nil).Return(nil, nil, io.EOF) actual, err := ctx.instance.SearchOrganizations(reqCtx, "query", nil) - assert.Error(t, err) - assert.Nil(t, actual) + assert.NoError(t, err) + assert.NotNil(t, actual) }) } From a213daaa892d4751dc2549f006248055e96c9b81 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Mon, 8 May 2023 14:00:47 +0200 Subject: [PATCH 2/2] Removed obsolete error --- didman/didman.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/didman/didman.go b/didman/didman.go index 85e7a08f79..1a6e6b2583 100644 --- a/didman/didman.go +++ b/didman/didman.go @@ -370,10 +370,7 @@ func (d *didman) SearchOrganizations(ctx context.Context, query string, didServi } // Retrieve DID Documents of found organizations var didDocuments []*did.Document - didDocuments, organizations, err = d.resolveOrganizationDIDDocuments(organizations) - if err != nil { - return nil, err - } + didDocuments, organizations = d.resolveOrganizationDIDDocuments(organizations) // If specified, filter on DID service type if didServiceType != nil && len(*didServiceType) > 0 { @@ -420,9 +417,7 @@ func (d *didman) SearchOrganizations(ctx context.Context, query string, didServi } // resolveOrganizationDIDDocuments takes a slice of organization VCs and tries to resolve the corresponding DID document for each. -// If a DID document isn't found or it is deactivated the organization is filtered from the slice (reslicing the given slice) and omitted from the DID documents slice. -// If any other error occurs, it is returned. -func (d *didman) resolveOrganizationDIDDocuments(organizations []vc.VerifiableCredential) ([]*did.Document, []vc.VerifiableCredential, error) { +func (d *didman) resolveOrganizationDIDDocuments(organizations []vc.VerifiableCredential) ([]*did.Document, []vc.VerifiableCredential) { didDocuments := make([]*did.Document, len(organizations)) j := 0 for i, organization := range organizations { @@ -453,7 +448,7 @@ func (d *didman) resolveOrganizationDIDDocuments(organizations []vc.VerifiableCr // Reslice to omit results which' DID Document could not be resolved didDocuments = didDocuments[:j] organizations = organizations[:j] - return didDocuments, organizations, nil + return didDocuments, organizations } func (d *didman) resolveOrganizationDIDDocument(organization vc.VerifiableCredential) (*did.Document, did.DID, error) {