VCR: Don't check VC existence when revoking (#1913)

* VCR: Don't check VC existence when revoking

* Release notes

* godoc

* Add check for credential ID format

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) {

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)
 ***********************

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(),
 	}
 }

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)

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,

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)

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 <https://www.w3.org/2018/credentials#issuer> <did:nuts:123> .
 			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 <https://www.w3.org/2018/credentials#issuer> <did:nuts:123> .
 			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) {