Alter discovery activation (#3418)

* change discovery activation API to fail immediately
* added sql table for refresh status and returned status+error in getServiceActivation

discovery/api/v1/api.go

@@ -24,6 +24,7 @@ import (
 	"github.com/labstack/echo/v4"
 	"github.com/nuts-foundation/nuts-node/audit"
 	"github.com/nuts-foundation/nuts-node/core"
+	"github.com/nuts-foundation/nuts-node/core/to"
 	"github.com/nuts-foundation/nuts-node/discovery"
 	"github.com/nuts-foundation/nuts-node/vcr/credential"
 	"github.com/nuts-foundation/nuts-node/vdr/didsubject"
@@ -46,6 +47,8 @@ func (w *Wrapper) ResolveStatusCode(err error) int {
 		return http.StatusNotFound
 	case errors.Is(err, didsubject.ErrSubjectNotFound):
 		return http.StatusNotFound
+	case errors.Is(err, discovery.ErrPresentationRegistrationFailed):
+		return http.StatusPreconditionFailed
 	default:
 		return http.StatusInternalServerError
 	}
@@ -108,12 +111,6 @@ func (w *Wrapper) ActivateServiceForSubject(ctx context.Context, request Activat
 	}
 
 	err := w.Client.ActivateServiceForSubject(ctx, request.ServiceID, request.SubjectID, parameters)
-	if errors.Is(err, discovery.ErrPresentationRegistrationFailed) {
-		// registration failed, but will be retried
-		return ActivateServiceForSubject202JSONResponse{
-			Reason: err.Error(),
-		}, nil
-	}
 	if err != nil {
 		// other error
 		return nil, err
@@ -123,12 +120,6 @@ func (w *Wrapper) ActivateServiceForSubject(ctx context.Context, request Activat
 
 func (w *Wrapper) DeactivateServiceForSubject(ctx context.Context, request DeactivateServiceForSubjectRequestObject) (DeactivateServiceForSubjectResponseObject, error) {
 	err := w.Client.DeactivateServiceForSubject(ctx, request.ServiceID, request.SubjectID)
-	if errors.Is(err, discovery.ErrPresentationRegistrationFailed) {
-		// deactivation succeeded, but not all Verifiable Presentation couldn't be removed from remote Discovery Server.
-		return DeactivateServiceForSubject202JSONResponse{
-			Reason: err.Error(),
-		}, nil
-	}
 	if err != nil {
 		return nil, err
 	}
@@ -141,12 +132,19 @@ func (w *Wrapper) GetServices(_ context.Context, _ GetServicesRequestObject) (Ge
 }
 
 func (w *Wrapper) GetServiceActivation(ctx context.Context, request GetServiceActivationRequestObject) (GetServiceActivationResponseObject, error) {
+	response := GetServiceActivation200JSONResponse{
+		Status: ServiceStatusActive,
+	}
 	activated, presentations, err := w.Client.GetServiceActivation(ctx, request.ServiceID, request.SubjectID)
 	if err != nil {
-		return nil, err
+		if !errors.As(err, &discovery.RegistrationRefreshError{}) {
+			return nil, err
+		}
+		response.Status = ServiceStatusError
+		response.Error = to.Ptr(err.Error())
 	}
-	return GetServiceActivation200JSONResponse{
-		Activated: activated,
-		Vp:        &presentations,
-	}, nil
+	response.Activated = activated
+	response.Vp = &presentations
+
+	return response, nil
 }

discovery/api/v1/api_test.go

@@ -71,17 +71,16 @@ func TestWrapper_ActivateServiceForSubject(t *testing.T) {
 		assert.NoError(t, err)
 		assert.IsType(t, ActivateServiceForSubject200Response{}, response)
 	})
-	t.Run("ok, but registration failed", func(t *testing.T) {
+	t.Run("but registration failed", func(t *testing.T) {
 		test := newMockContext(t)
 		test.client.EXPECT().ActivateServiceForSubject(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(discovery.ErrPresentationRegistrationFailed)
 
-		response, err := test.wrapper.ActivateServiceForSubject(nil, ActivateServiceForSubjectRequestObject{
+		_, err := test.wrapper.ActivateServiceForSubject(nil, ActivateServiceForSubjectRequestObject{
 			ServiceID: serviceID,
 			SubjectID: subjectID,
 		})
 
-		assert.NoError(t, err)
-		assert.IsType(t, ActivateServiceForSubject202JSONResponse{}, response)
+		assert.ErrorIs(t, err, discovery.ErrPresentationRegistrationFailed)
 	})
 	t.Run("other error", func(t *testing.T) {
 		test := newMockContext(t)
@@ -200,6 +199,24 @@ func TestWrapper_GetServiceActivation(t *testing.T) {
 		assert.NoError(t, err)
 		require.IsType(t, GetServiceActivation200JSONResponse{}, response)
 		assert.True(t, response.(GetServiceActivation200JSONResponse).Activated)
+		assert.Equal(t, ServiceStatusActive, string(response.(GetServiceActivation200JSONResponse).Status))
+		assert.Nil(t, response.(GetServiceActivation200JSONResponse).Error)
+		assert.Empty(t, response.(GetServiceActivation200JSONResponse).Vp)
+	})
+	t.Run("refresh failed", func(t *testing.T) {
+		test := newMockContext(t)
+		test.client.EXPECT().GetServiceActivation(gomock.Any(), serviceID, subjectID).Return(true, nil, discovery.RegistrationRefreshError{Underlying: assert.AnError})
+
+		response, err := test.wrapper.GetServiceActivation(nil, GetServiceActivationRequestObject{
+			SubjectID: subjectID,
+			ServiceID: serviceID,
+		})
+
+		assert.NoError(t, err)
+		require.IsType(t, GetServiceActivation200JSONResponse{}, response)
+		assert.True(t, response.(GetServiceActivation200JSONResponse).Activated)
+		assert.Equal(t, ServiceStatusError, string(response.(GetServiceActivation200JSONResponse).Status))
+		assert.NotNil(t, response.(GetServiceActivation200JSONResponse).Error)
 		assert.Empty(t, response.(GetServiceActivation200JSONResponse).Vp)
 	})
 	t.Run("error", func(t *testing.T) {

discovery/api/v1/types.go

@@ -31,3 +31,13 @@ type ServiceDefinition = discovery.ServiceDefinition
 
 // VerifiableCredential is a type alias for the VerifiableCredential from the go-did library.
 type VerifiableCredential = vc.VerifiableCredential
+
+// GetServiceActivation200JSONResponseStatus is a type alias for string, generated from an enum.
+type GetServiceActivation200JSONResponseStatus string
+
+const (
+	// ServiceStatusActive is the status for an active service.
+	ServiceStatusActive = "active"
+	// ServiceStatusError is the status for an inactive service.
+	ServiceStatusError = "error"
+)

discovery/client.go

@@ -92,23 +92,14 @@ func (r *defaultClientRegistrationManager) activate(ctx context.Context, service
 		return fmt.Errorf("%w: %w for %s", ErrPresentationRegistrationFailed, ErrDIDMethodsNotSupported, subjectID)
 	}
 
-	asSoonAsPossible := time.Now().Add(-10 * time.Second) // could be whatever, as long as it's a bit in the past to avoid issues with some weird clock skew scenarios when running a cluster
-	if err := r.store.updatePresentationRefreshTime(serviceID, subjectID, parameters, &asSoonAsPossible); err != nil {
-		return err
-	}
 	log.Logger().Debugf("Registering Verifiable Presentation on Discovery Service (service=%s, subject=%s)", service.ID, subjectID)
 
 	var registeredDIDs []string
 	var loopErrs []error
 	for _, subjectDID := range subjectDIDs {
 		err := r.registerPresentation(ctx, subjectDID, service, parameters)
 		if err != nil {
-			if !errors.Is(err, pe.ErrNoCredentials) { // ignore missing credentials
-				loopErrs = append(loopErrs, fmt.Errorf("%s: %w", subjectDID.String(), err))
-			} else {
-				// trace logging for missing credentials
-				log.Logger().Tracef("Missing credentials for Discovery Service (service=%s, subject=%s, did=%s): %s", service.ID, subjectID, subjectDID, err.Error())
-			}
+			loopErrs = append(loopErrs, fmt.Errorf("%s: %w", subjectDID.String(), err))
 		} else {
 			registeredDIDs = append(registeredDIDs, subjectDID.String())
 		}
@@ -132,6 +123,10 @@ func (r *defaultClientRegistrationManager) activate(ctx context.Context, service
 	if err := r.store.updatePresentationRefreshTime(serviceID, subjectID, parameters, &refreshVPAfter); err != nil {
 		return fmt.Errorf("unable to update Verifiable Presentation refresh time: %w", err)
 	}
+	// clear any previous presentationRefreshErrors here so it's triggered by both the refresh and API call
+	if err := r.store.setPresentationRefreshError(serviceID, subjectID, nil); err != nil {
+		return fmt.Errorf("unable to clear previous presentationRefreshError: %w", err)
+	}
 	return nil
 }
 
@@ -278,8 +273,8 @@ func (r *defaultClientRegistrationManager) refresh(ctx context.Context, now time
 	}
 	var loopErrs []error
 	for _, candidate := range refreshCandidates {
+		var loopErr error
 		if err = r.activate(ctx, candidate.ServiceID, candidate.SubjectID, candidate.Parameters); err != nil {
-			var loopErr error
 			if errors.Is(err, ErrDIDMethodsNotSupported) {
 				// DID method no longer supported, remove
 				err = r.store.updatePresentationRefreshTime(candidate.ServiceID, candidate.SubjectID, nil, nil)
@@ -296,9 +291,13 @@ func (r *defaultClientRegistrationManager) refresh(ctx context.Context, now time
 				}
 			} else {
 				loopErr = fmt.Errorf("failed to refresh Verifiable Presentation (service=%s, subject=%s): %w", candidate.ServiceID, candidate.SubjectID, err)
+				if err := r.store.setPresentationRefreshError(candidate.ServiceID, candidate.SubjectID, loopErr); err != nil {
+					loopErr = fmt.Errorf("failed to set refresh error for Verifiable Presentation (service=%s, subject=%s): %w. Original error: %w", candidate.ServiceID, candidate.SubjectID, err, loopErr)
+				}
 			}
 			loopErrs = append(loopErrs, loopErr)
 		}
+		// activate clears any presentationRefreshErrors
 	}
 	if len(loopErrs) > 0 {
 		return errors.Join(loopErrs...)

discovery/client_test.go

@@ -90,6 +90,12 @@ func Test_defaultClientRegistrationManager_activate(t *testing.T) {
 
 		require.ErrorIs(t, err, ErrPresentationRegistrationFailed)
 		assert.ErrorContains(t, err, "invoker error")
+
+		// check no refresh records are added
+		record, err := store.getPresentationRefreshRecord(testServiceID, aliceSubject)
+
+		require.NoError(t, err)
+		assert.Nil(t, record)
 	})
 	t.Run("DID method not supported", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
@@ -360,7 +366,13 @@ func Test_defaultClientRegistrationManager_refresh(t *testing.T) {
 
 		err := manager.refresh(audit.TestContext(), time.Now())
 
-		assert.EqualError(t, err, "failed to refresh Verifiable Presentation (service=usecase_v1, subject=bob): registration of Verifiable Presentation on remote Discovery Service failed: did:example:bob: remote error")
+		errStr := "failed to refresh Verifiable Presentation (service=usecase_v1, subject=bob): registration of Verifiable Presentation on remote Discovery Service failed: did:example:bob: remote error"
+		assert.EqualError(t, err, errStr)
+
+		// check for presentationRefreshError
+		refreshError, err := store.getPresentationRefreshError(testServiceID, bobSubject)
+		require.NoError(t, err)
+		assert.Contains(t, refreshError.Error, errStr)
 	})
 	t.Run("deactivate unknown subject", func(t *testing.T) {
 		store := setupStore(t, storageEngine.GetSQLDatabase())
@@ -376,7 +388,6 @@ func Test_defaultClientRegistrationManager_refresh(t *testing.T) {
 
 		assert.EqualError(t, err, "removed unknown subject (service=usecase_v1, subject=alice)")
 	})
-
 	t.Run("deactivate unsupported DID method", func(t *testing.T) {
 		store := setupStore(t, storageEngine.GetSQLDatabase())
 		ctrl := gomock.NewController(t)
@@ -391,9 +402,38 @@ func Test_defaultClientRegistrationManager_refresh(t *testing.T) {
 
 		// refresh clears the registration
 		require.NoError(t, err)
-		refreshTime, err := store.getPresentationRefreshTime(unsupportedServiceID, aliceSubject)
+		record, err := store.getPresentationRefreshRecord(unsupportedServiceID, aliceSubject)
 		assert.NoError(t, err)
-		assert.Nil(t, refreshTime)
+		assert.Nil(t, record)
+	})
+	t.Run("remove presentationRefreshError on success", func(t *testing.T) {
+		store := setupStore(t, storageEngine.GetSQLDatabase())
+		ctrl := gomock.NewController(t)
+		invoker := client.NewMockHTTPClient(ctrl)
+		gomock.InOrder(
+			invoker.EXPECT().Register(gomock.Any(), gomock.Any(), gomock.Any()),
+		)
+		wallet := holder.NewMockWallet(ctrl)
+		mockVCR := vcr.NewMockVCR(ctrl)
+		mockVCR.EXPECT().Wallet().Return(wallet).AnyTimes()
+		mockSubjectManager := didsubject.NewMockManager(ctrl)
+		manager := newRegistrationManager(testDefinitions(), store, invoker, mockVCR, mockSubjectManager)
+
+		// Alice
+		_ = store.setPresentationRefreshError(testServiceID, aliceSubject, assert.AnError)
+		_ = store.updatePresentationRefreshTime(testServiceID, aliceSubject, defaultRegistrationParams(aliceSubject), &time.Time{})
+		mockSubjectManager.EXPECT().ListDIDs(gomock.Any(), aliceSubject).Return([]did.DID{aliceDID}, nil)
+		wallet.EXPECT().BuildPresentation(gomock.Any(), gomock.Any(), gomock.Any(), &aliceDID, false).Return(&vpAlice, nil)
+		wallet.EXPECT().List(gomock.Any(), aliceDID).Return([]vc.VerifiableCredential{vcAlice}, nil)
+
+		err := manager.refresh(audit.TestContext(), time.Now())
+
+		require.NoError(t, err)
+
+		// check for presentationRefreshError
+		refreshError, err := store.getPresentationRefreshError(testServiceID, aliceSubject)
+		require.NoError(t, err)
+		assert.Nil(t, refreshError)
 	})
 }
 

discovery/interface.go

@@ -59,7 +59,6 @@ type Client interface {
 
 	// ActivateServiceForSubject causes a subject to be registered for a Discovery Service.
 	// Registration of all DIDs of the subject will be attempted immediately, and automatically refreshed.
-	// If the initial registration for all DIDs fails with ErrPresentationRegistrationFailed, registration will be retried.
 	// If the function is called again for the same service/DID combination, it will try to refresh the registration.
 	// parameters are added as credentialSubject to a DiscoveryRegistrationCredential holder credential.
 	// It returns an error if the service or subject is invalid/unknown.
@@ -77,6 +76,8 @@ type Client interface {
 	// GetServiceActivation returns the activation status of a subject on a Discovery Service.
 	// The boolean indicates whether the subject is activated on the Discovery Service (ActivateServiceForSubject() has been called).
 	// It also returns the Verifiable Presentations for all DIDs of the subject that are registered on the Discovery Service, if any.
+	// It returns a refreshRecordError if the last refresh of the service failed (activation status and VPs are still returned).
+	// The time of the last error is added in the error message.
 	GetServiceActivation(ctx context.Context, serviceID, subjectID string) (bool, []vc.VerifiablePresentation, error)
 }
 
@@ -96,3 +97,12 @@ type presentationVerifier func(definition ServiceDefinition, presentation vc.Ver
 
 // XForwardedHostContextKey is the context key for the X-Forwarded-Host header.
 type XForwardedHostContextKey struct{}
+
+// RegistrationRefreshError is returned from GetServiceRefreshError.
+type RegistrationRefreshError struct {
+	Underlying error
+}
+
+func (r RegistrationRefreshError) Error() string {
+	return r.Underlying.Error()
+}

discovery/module.go

@@ -379,9 +379,6 @@ func (m *Module) ActivateServiceForSubject(ctx context.Context, serviceID, subje
 
 	err := m.registrationManager.activate(ctx, serviceID, subjectID, parameters)
 	if err != nil {
-		if errors.Is(err, ErrPresentationRegistrationFailed) {
-			log.Logger().WithError(err).Warnf("Presentation registration failed, will be retried later (subject=%s,service=%s)", subjectID, serviceID)
-		}
 		return err
 	}
 
@@ -408,11 +405,11 @@ func (m *Module) Services() []ServiceDefinition {
 // GetServiceActivation is a Discovery Client function that retrieves the activation status of a service for a subject.
 // See interface.go for more information.
 func (m *Module) GetServiceActivation(ctx context.Context, serviceID, subjectID string) (bool, []vc.VerifiablePresentation, error) {
-	refreshTime, err := m.store.getPresentationRefreshTime(serviceID, subjectID)
+	refreshRecord, err := m.store.getPresentationRefreshRecord(serviceID, subjectID)
 	if err != nil {
 		return false, nil, err
 	}
-	if refreshTime == nil {
+	if refreshRecord == nil {
 		return false, nil, nil
 	}
 	// subject is activated for service
@@ -431,7 +428,16 @@ func (m *Module) GetServiceActivation(ctx context.Context, serviceID, subjectID
 	for _, vps := range vps2D {
 		results = append(results, vps...)
 	}
-	return true, results, nil
+
+	// check if the last refresh didn't result in an error
+	if refreshRecord.PresentationRefreshError.Error != "" {
+		// format a readable time using RFC3339
+		lastOccurrence := time.Unix(int64(refreshRecord.PresentationRefreshError.LastOccurrence), 0)
+		readableTime := lastOccurrence.Format(time.RFC3339)
+		err = RegistrationRefreshError{fmt.Errorf("[%s] %s", readableTime, refreshRecord.PresentationRefreshError.Error)}
+	}
+
+	return true, results, err
 }
 
 func loadDefinitions(directory string) (map[string]ServiceDefinition, error) {