diff --git a/auth/services/oauth/oauth_test.go b/auth/services/oauth/oauth_test.go index 79c2a6eff6..4d77350157 100644 --- a/auth/services/oauth/oauth_test.go +++ b/auth/services/oauth/oauth_test.go @@ -467,7 +467,7 @@ func TestService_validateAuthorizationCredentials(t *testing.T) { require.NoError(t, err) assert.NotNil(t, tokenCtx.credentialIDs) - assert.Equal(t, "did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#1", tokenCtx.credentialIDs[0]) + assert.Equal(t, "did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#38E90E8C-F7E5-4333-B63A-F9DD155A0272", tokenCtx.credentialIDs[0]) }) t.Run("ok - no authorization credentials", func(t *testing.T) { @@ -518,7 +518,7 @@ func TestService_validateAuthorizationCredentials(t *testing.T) { err := ctx.oauthService.validateAuthorizationCredentials(tokenCtx) - assert.EqualError(t, err, "credentialSubject.ID did:nuts:B8PUHs2AUHbFF1xLLK4eZjgErEcMXHxs68FteY7NDtCY of authorization credential with ID: did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#1 does not match jwt.iss: unknown") + assert.EqualError(t, err, "credentialSubject.ID did:nuts:B8PUHs2AUHbFF1xLLK4eZjgErEcMXHxs68FteY7NDtCY of authorization credential with ID: did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#38E90E8C-F7E5-4333-B63A-F9DD155A0272 does not match jwt.iss: unknown") }) t.Run("error - jwt.sub <> issuer mismatch", func(t *testing.T) { @@ -529,7 +529,7 @@ func TestService_validateAuthorizationCredentials(t *testing.T) { err := ctx.oauthService.validateAuthorizationCredentials(tokenCtx) - assert.EqualError(t, err, "issuer did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW of authorization credential with ID: did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#1 does not match jwt.sub: unknown") + assert.EqualError(t, err, "issuer did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW of authorization credential with ID: did:nuts:GvkzxsezHvEc8nGhgz6Xo3jbqkHwswLmWw3CYtCm7hAW#38E90E8C-F7E5-4333-B63A-F9DD155A0272 does not match jwt.sub: unknown") }) t.Run("error - invalid credential", func(t *testing.T) { diff --git a/docs/_static/vcr/v2.yaml b/docs/_static/vcr/v2.yaml index b3c05b3ab1..d7c4a2a650 100644 --- a/docs/_static/vcr/v2.yaml +++ b/docs/_static/vcr/v2.yaml @@ -194,7 +194,6 @@ paths: error returns: * 400 - Credential can't be revoked. Most likely due to a missing private key. - * 404 - Credential is not found * 409 - Credential has already been revoked * 500 - An error occurred while processing the request operationId: "revokeVC" diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 39cde0a7b9..968eda0457 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -18,6 +18,19 @@ Release date: **DRAFT** **Full Changelog**: https://github.com/nuts-foundation/nuts-node/compare/v5.0.0...v5.1.0 +*********************** +Coconut update (v5.0.9) +*********************** + +Release date: 2023-02-21 + +This patch release fixes the following: + +- Validations performed when revoking a VC are now more lenient: don't check whether it can actually find the VC in the issuer's database. + Enables issuers to revoke VCs even if they've lost track of them (e.g. incorrect database backup/restore). + +**Full Changelog**: https://github.com/nuts-foundation/nuts-node/compare/v5.0.8...v5.0.9 + *********************** Coconut update (v5.0.8) *********************** diff --git a/vcr/credential/revocation.go b/vcr/credential/revocation.go index 23395c81d7..51ba684821 100644 --- a/vcr/credential/revocation.go +++ b/vcr/credential/revocation.go @@ -52,13 +52,13 @@ var nowFunc = time.Now var RevocationType = ssi.MustParseURI("CredentialRevocation") // BuildRevocation generates a revocation based on the credential -func BuildRevocation(credential vc.VerifiableCredential) Revocation { +func BuildRevocation(issuer ssi.URI, subject ssi.URI) Revocation { nutsCredentialContext := ssi.MustParseURI("https://nuts.nl/credentials/v1") return Revocation{ Context: []ssi.URI{nutsCredentialContext}, Type: []ssi.URI{RevocationType}, - Issuer: credential.Issuer, - Subject: *credential.ID, + Issuer: issuer, + Subject: subject, Date: nowFunc(), } } diff --git a/vcr/credential/revocation_test.go b/vcr/credential/revocation_test.go index 68363edcc8..b38ca71515 100644 --- a/vcr/credential/revocation_test.go +++ b/vcr/credential/revocation_test.go @@ -43,7 +43,7 @@ func TestBuildRevocation(t *testing.T) { nowFunc = time.Now }() - r := BuildRevocation(target) + r := BuildRevocation(target.Issuer, *target.ID) assert.Equal(t, *target.ID, r.Subject) assert.Equal(t, target.Issuer, r.Issuer) diff --git a/vcr/credential/test.go b/vcr/credential/test.go index 8331120320..c6e0e6e2a0 100644 --- a/vcr/credential/test.go +++ b/vcr/credential/test.go @@ -30,7 +30,7 @@ import ( ) func ValidNutsAuthorizationCredential() *vc.VerifiableCredential { - id := stringToURI(vdr.TestDIDA.String() + "#1") + id := stringToURI(vdr.TestDIDA.String() + "#38E90E8C-F7E5-4333-B63A-F9DD155A0272") return &vc.VerifiableCredential{ Context: []ssi.URI{vc.VCContextV1URI(), NutsV1ContextURI}, ID: &id, diff --git a/vcr/issuer/issuer.go b/vcr/issuer/issuer.go index c45f2d24d3..5257b9392c 100644 --- a/vcr/issuer/issuer.go +++ b/vcr/issuer/issuer.go @@ -172,12 +172,10 @@ func (i issuer) buildVC(ctx context.Context, credentialOptions vc.VerifiableCred } func (i issuer) Revoke(ctx context.Context, credentialID ssi.URI) (*credential.Revocation, error) { - // first find it using a query on id. - credentialToRevoke, err := i.store.GetCredential(credentialID) - if err != nil { - return nil, fmt.Errorf("could not revoke (id=%s): %w", credentialID, err) - } - + // Previously we first tried to resolve the credential, but that's not necessary: + // if the credential doesn't actually exist the revocation doesn't apply to anything, no harm done. + // Although it is a bit ugly, it helps issuers to revoke credentials that they don't have anymore, + // for whatever reason (e.g. incorrect database backup/restore). isRevoked, err := i.isRevoked(credentialID) if err != nil { return nil, fmt.Errorf("error while checking revocation status: %w", err) @@ -186,7 +184,7 @@ func (i issuer) Revoke(ctx context.Context, credentialID ssi.URI) (*credential.R return nil, vcr.ErrRevoked } - revocation, err := i.buildRevocation(ctx, *credentialToRevoke) + revocation, err := i.buildRevocation(ctx, credentialID) if err != nil { return nil, err } @@ -202,14 +200,24 @@ func (i issuer) Revoke(ctx context.Context, credentialID ssi.URI) (*credential.R } log.Logger(). - WithField(core.LogFieldCredentialID, credentialToRevoke.ID). + WithField(core.LogFieldCredentialID, credentialID). Info("Verifiable Credential revoked") return revocation, nil } -func (i issuer) buildRevocation(ctx context.Context, credentialToRevoke vc.VerifiableCredential) (*credential.Revocation, error) { - // find issuer - issuerDID, err := did.ParseDID(credentialToRevoke.Issuer.String()) +func (i issuer) buildRevocation(ctx context.Context, credentialID ssi.URI) (*credential.Revocation, error) { + // Sanity check: since we don't check existence of the VC, at least somewhat guard against mistyped credential IDs + // (although nobody should be typing those in). + _, err := uuid.Parse(credentialID.Fragment) + if err != nil { + return nil, core.InvalidInputError("invalid credential ID") + } + + // find issuer from credential ID + issuer := credentialID + issuer.Path = "" + issuer.Fragment = "" + issuerDID, err := did.ParseDID(issuer.String()) if err != nil { return nil, fmt.Errorf("failed to extract issuer: %w", err) } @@ -219,12 +227,12 @@ func (i issuer) buildRevocation(ctx context.Context, credentialToRevoke vc.Verif const errString = "failed to revoke credential (%s): could not resolve an assertionKey for issuer: %w" // Differentiate between a DID document not found and some other error: if errors.Is(err, vdr.ErrNotFound) { - return nil, core.InvalidInputError(errString, credentialToRevoke.ID, err) + return nil, core.InvalidInputError(errString, credentialID, err) } - return nil, fmt.Errorf(errString, credentialToRevoke.ID, err) + return nil, fmt.Errorf(errString, credentialID, err) } // set defaults - revocation := credential.BuildRevocation(credentialToRevoke) + revocation := credential.BuildRevocation(issuerDID.URI(), credentialID) revocationAsMap := map[string]interface{}{} b, _ := json.Marshal(revocation) diff --git a/vcr/issuer/issuer_test.go b/vcr/issuer/issuer_test.go index 89bcaf51ad..e088053385 100644 --- a/vcr/issuer/issuer_test.go +++ b/vcr/issuer/issuer_test.go @@ -308,7 +308,7 @@ func Test_issuer_buildRevocation(t *testing.T) { Issuer: issuerDID.URI(), ID: &credentialID, } - revocation, err := sut.buildRevocation(ctx, credentialToRevoke) + revocation, err := sut.buildRevocation(ctx, *credentialToRevoke.ID) assert.NoError(t, err) t.Logf("revocation %+v", revocation) }) @@ -350,7 +350,7 @@ _:c14n0 . keyResolverMock.EXPECT().ResolveAssertionKey(issuerDID).Return(nil, errors.New("b00m!")) sut := issuer{keyResolver: keyResolverMock} - _, err := sut.buildRevocation(ctx, testVC) + _, err := sut.buildRevocation(ctx, *testVC.ID) assert.EqualError(t, err, fmt.Sprintf("failed to revoke credential (%s): could not resolve an assertionKey for issuer: b00m!", testVC.ID)) }) @@ -361,13 +361,13 @@ _:c14n0 . keyResolverMock.EXPECT().ResolveAssertionKey(issuerDID).Return(nil, vdr.ErrNotFound) sut := issuer{keyResolver: keyResolverMock} - _, err := sut.buildRevocation(ctx, testVC) + _, err := sut.buildRevocation(ctx, *testVC.ID) assert.ErrorIs(t, err, core.InvalidInputError("failed to revoke credential: could not resolve an assertionKey for issuer: unable to find the DID document")) }) }) } func Test_issuer_Revoke(t *testing.T) { - credentialID := "did:nuts:123#abc" + credentialID := "did:nuts:123#38E90E8C-F7E5-4333-B63A-F9DD155A0272" credentialURI := ssi.MustParseURI(credentialID) issuerID := "did:nuts:123" issuerURI := ssi.MustParseURI(issuerID) @@ -378,16 +378,8 @@ func Test_issuer_Revoke(t *testing.T) { ctx := audit.TestContext() t.Run("for a known credential", func(t *testing.T) { - credentialToRevoke := func() *vc.VerifiableCredential { - return &vc.VerifiableCredential{ - ID: &credentialURI, - Issuer: issuerURI, - } - } - storeWithActualCredential := func(c *gomock.Controller) *MockStore { store := NewMockStore(c) - store.EXPECT().GetCredential(credentialURI).Return(credentialToRevoke(), nil) store.EXPECT().GetRevocation(credentialURI).Return(nil, vcr.ErrNotFound) return store } @@ -430,19 +422,47 @@ func Test_issuer_Revoke(t *testing.T) { assert.Equal(t, kid, revocation.Proof.VerificationMethod) }) - t.Run("it handles a buildRevocation error", func(t *testing.T) { + t.Run("error - unable to check revocation status", func(t *testing.T) { ctrl := gomock.NewController(t) - // the credential does not contain a valid issuer: - invalidCredential := vc.VerifiableCredential{} + // GetRevocation fails store := NewMockStore(ctrl) - store.EXPECT().GetCredential(credentialURI).Return(&invalidCredential, nil) - store.EXPECT().GetRevocation(credentialURI).Return(nil, vcr.ErrNotFound) + store.EXPECT().GetRevocation(credentialURI).Return(nil, errors.New("oops")) sut := issuer{ store: store, } revocation, err := sut.Revoke(ctx, credentialURI) + assert.EqualError(t, err, "error while checking revocation status: oops") + assert.Nil(t, revocation) + }) + + t.Run("error - invalid credential ID (fragment is not a UUID)", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + store := NewMockStore(ctrl) + store.EXPECT().GetRevocation(gomock.Any()).Return(nil, vcr.ErrNotFound) + + sut := issuer{ + store: store, + } + revocation, err := sut.Revoke(ctx, ssi.MustParseURI("did:nuts:123#invalid")) + assert.EqualError(t, err, "invalid credential ID") + assert.Nil(t, revocation) + }) + + t.Run("error - invalid DID", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + store := NewMockStore(ctrl) + store.EXPECT().GetRevocation(gomock.Any()).Return(nil, vcr.ErrNotFound) + + sut := issuer{ + store: store, + } + revocation, err := sut.Revoke(ctx, ssi.MustParseURI("a#38E90E8C-F7E5-4333-B63A-F9DD155A0272")) assert.EqualError(t, err, "failed to extract issuer: invalid DID: input length is less than 7") assert.Nil(t, revocation) }) @@ -475,7 +495,6 @@ func Test_issuer_Revoke(t *testing.T) { publisher.EXPECT().PublishRevocation(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().StoreRevocation(gomock.Any()).Return(nil) // 2nd revocation - store.EXPECT().GetCredential(credentialURI).Return(credentialToRevoke(), nil) store.EXPECT().GetRevocation(credentialURI).Return(&credential.Revocation{}, nil) sut := issuer{ @@ -494,26 +513,6 @@ func Test_issuer_Revoke(t *testing.T) { assert.Nil(t, revocation) }) }) - - t.Run("for an unknown credential", func(t *testing.T) { - storeWithoutCredential := func(c *gomock.Controller) Store { - store := NewMockStore(c) - store.EXPECT().GetCredential(credentialURI).Return(nil, vcr.ErrNotFound) - return store - } - - t.Run("it returns an error", func(t *testing.T) { - ctrl := gomock.NewController(t) - - sut := issuer{ - store: storeWithoutCredential(ctrl), - } - - revocation, err := sut.Revoke(ctx, credentialURI) - assert.EqualError(t, err, "could not revoke (id=did:nuts:123#abc): credential not found") - assert.Nil(t, revocation) - }) - }) } func TestIssuer_isRevoked(t *testing.T) {