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.

Sign up for GitHub

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

Jump to bottom

Didman: make organization search more robust #2114

Merged
merged 2 commits into from
May 8, 2023
Merged

Didman: make organization search more robust #2114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f09aef
Didman: make organization search more robust by only logging VC/DID d…
reinkrul May 8, 2023
a213daa
Removed obsolete error
reinkrul May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions didman/didman.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -420,22 +417,28 @@ 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 {
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) {
reinkrul marked this conversation as resolved.
Show resolved Hide resolved
// 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
Expand All @@ -445,10 +448,13 @@ 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) {
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 {
Expand Down
58 changes: 53 additions & 5 deletions didman/didman_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
})
}

Expand Down