diff --git a/internal/apirouter/destination_credentials_test.go b/internal/apirouter/destination_credentials_test.go index 6aab079d4..c1a4ef516 100644 --- a/internal/apirouter/destination_credentials_test.go +++ b/internal/apirouter/destination_credentials_test.go @@ -90,6 +90,42 @@ func TestDestinationCredentials_SecretRotationViaAPI(t *testing.T) { "previous_secret should be the old secret") assert.NotEmpty(t, rotated.Credentials["previous_secret_invalid_at"], "previous_secret_invalid_at should be set") + + // Rotate again with an explicit invalidation window + customInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + secondRotateReq := h.jsonReq(http.MethodPatch, "/api/v1/tenants/t1/destinations/d1", map[string]any{ + "credentials": map[string]any{ + "rotate_secret": true, + "previous_secret_invalid_at": customInvalidAt, + }, + }) + secondRotateResp := h.do(h.withAPIKey(secondRotateReq)) + require.Equal(t, http.StatusOK, secondRotateResp.Code) + + var secondRotated destregistry.DestinationDisplay + require.NoError(t, json.Unmarshal(secondRotateResp.Body.Bytes(), &secondRotated)) + assert.Equal(t, customInvalidAt, secondRotated.Credentials["previous_secret_invalid_at"], + "explicit previous_secret_invalid_at should be respected") + + // Rotate once more without previous_secret_invalid_at: the 24h default + // must be re-applied, not the previously stored window carried forward + thirdRotateReq := h.jsonReq(http.MethodPatch, "/api/v1/tenants/t1/destinations/d1", map[string]any{ + "credentials": map[string]any{ + "rotate_secret": true, + }, + }) + thirdRotateResp := h.do(h.withAPIKey(thirdRotateReq)) + require.Equal(t, http.StatusOK, thirdRotateResp.Code) + + var thirdRotated destregistry.DestinationDisplay + require.NoError(t, json.Unmarshal(thirdRotateResp.Body.Bytes(), &thirdRotated)) + assert.Equal(t, secondRotated.Credentials["secret"], thirdRotated.Credentials["previous_secret"], + "previous_secret should be the secret from the previous rotation") + + invalidAt, err := time.Parse(time.RFC3339, thirdRotated.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, time.Minute, + "omitting previous_secret_invalid_at should default to now+24h") } func TestDestinationCredentials_TenantCannotSetCustomSecret(t *testing.T) { diff --git a/internal/apirouter/destination_handlers.go b/internal/apirouter/destination_handlers.go index 3339a7500..38e7ff635 100644 --- a/internal/apirouter/destination_handlers.go +++ b/internal/apirouter/destination_handlers.go @@ -121,6 +121,10 @@ func (h *DestinationHandlers) Create(c *gin.Context) { } if err := h.registry.PreprocessDestination(&destination, nil, &destregistry.PreprocessDestinationOpts{ Role: mustRoleFromContext(c), + Request: destregistry.PreprocessRequest{ + Config: destination.Config, + Credentials: destination.Credentials, + }, }); err != nil { AbortWithValidationError(c, err) return @@ -193,7 +197,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Config (merge-patch) - configResult, configChanged, err := applyMergePatchStringMap(originalDestination.Config, input.Config) + configResult, configRequest, configChanged, err := applyMergePatchStringMap(originalDestination.Config, input.Config) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid config: %w", err)) return @@ -204,7 +208,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Credentials (merge-patch) - credsResult, credsChanged, err := applyMergePatchStringMap(originalDestination.Credentials, input.Credentials) + credsResult, credsRequest, credsChanged, err := applyMergePatchStringMap(originalDestination.Credentials, input.Credentials) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid credentials: %w", err)) return @@ -229,7 +233,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // DeliveryMetadata (merge-patch) - dmResult, dmChanged, err := applyMergePatchStringMap(originalDestination.DeliveryMetadata, input.DeliveryMetadata) + dmResult, _, dmChanged, err := applyMergePatchStringMap(originalDestination.DeliveryMetadata, input.DeliveryMetadata) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid delivery_metadata: %w", err)) return @@ -239,7 +243,7 @@ func (h *DestinationHandlers) Update(c *gin.Context) { } // Metadata (merge-patch) - metaResult, metaChanged, err := applyMergePatchStringMap(originalDestination.Metadata, input.Metadata) + metaResult, _, metaChanged, err := applyMergePatchStringMap(originalDestination.Metadata, input.Metadata) if err != nil { AbortWithValidationError(c, fmt.Errorf("invalid metadata: %w", err)) return @@ -279,6 +283,10 @@ func (h *DestinationHandlers) Update(c *gin.Context) { // Always preprocess before updating if err := h.registry.PreprocessDestination(&updatedDestination, originalDestination, &destregistry.PreprocessDestinationOpts{ Role: mustRoleFromContext(c), + Request: destregistry.PreprocessRequest{ + Config: configRequest, + Credentials: credsRequest, + }, }); err != nil { AbortWithValidationError(c, err) return @@ -507,26 +515,28 @@ func isJSONNull(raw json.RawMessage) bool { } // applyMergePatchStringMap applies RFC 7396 merge-patch semantics for a map[string]string field. -// Returns (result, changed, error): +// Returns (result, request, changed, error), where request is the patch as +// sent by the caller (string-coerced, nulls dropped): // - raw is nil (field omitted): returns original unchanged // - raw is "null": returns nil (clear field) // - raw is "{}": returns original unchanged (empty merge = no-op) // - raw is an object: merge-patch into original -func applyMergePatchStringMap(original map[string]string, raw json.RawMessage) (map[string]string, bool, error) { +func applyMergePatchStringMap(original map[string]string, raw json.RawMessage) (map[string]string, map[string]string, bool, error) { if raw == nil { - return original, false, nil + return original, nil, false, nil } if isJSONNull(raw) { - return nil, true, nil + return nil, nil, true, nil } var patch map[string]any if err := json.Unmarshal(raw, &patch); err != nil { - return nil, false, err + return nil, nil, false, err } if len(patch) == 0 { - return original, false, nil + return original, nil, false, nil } - return maputil.MergePatchStringMap(original, patch), true, nil + request := maputil.MergePatchStringMap(nil, patch) + return maputil.MergePatchStringMap(original, patch), request, true, nil } // tenantSnapshot captures the tenant's derived state before a destination mutation. diff --git a/internal/destregistry/providers/destwebhook/destwebhook.go b/internal/destregistry/providers/destwebhook/destwebhook.go index 21ebef459..0162a2e23 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook.go +++ b/internal/destregistry/providers/destwebhook/destwebhook.go @@ -443,7 +443,7 @@ func (d *WebhookDestination) resolveConfig(ctx context.Context, destination *mod } // rotateSecret handles secret rotation and returns clean credentials -func (d *WebhookDestination) rotateSecret(newDest, origDest *models.Destination) (map[string]string, error) { +func (d *WebhookDestination) rotateSecret(origDest *models.Destination, opts *destregistry.PreprocessDestinationOpts) (map[string]string, error) { if origDest == nil { return nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{ { @@ -474,9 +474,11 @@ func (d *WebhookDestination) rotateSecret(newDest, origDest *models.Destination) } creds["secret"] = secret - // Keep custom invalidation time if provided, otherwise set default - if newDest.Credentials["previous_secret_invalid_at"] != "" { - creds["previous_secret_invalid_at"] = newDest.Credentials["previous_secret_invalid_at"] + // Keep custom invalidation time if provided, otherwise set default. + // The merged credentials can't tell us whether the caller sent the + // field — the raw request credentials can. + if invalidAt := opts.Request.Credentials["previous_secret_invalid_at"]; invalidAt != "" { + creds["previous_secret_invalid_at"] = invalidAt } else { creds["previous_secret_invalid_at"] = time.Now().Add(24 * time.Hour).Format(time.RFC3339) } @@ -607,7 +609,7 @@ func (d *WebhookDestination) Preprocess(newDestination *models.Destination, orig var cleanCredentials map[string]string var err error if isTruthy(newDestination.Credentials["rotate_secret"]) { - cleanCredentials, err = d.rotateSecret(newDestination, originalDestination) + cleanCredentials, err = d.rotateSecret(originalDestination, opts) } else { cleanCredentials, err = d.updateSecret(newDestination, originalDestination, opts) // For new destinations, ensure credentials are initialized if needed diff --git a/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go b/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go index d85dbe27c..860715d35 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go +++ b/internal/destregistry/providers/destwebhook/destwebhook_validate_test.go @@ -606,17 +606,65 @@ func TestWebhookDestination_Preprocess(t *testing.T) { }), ) - // Merge both config and credentials to simulate handler behavior + // Merge both config and credentials to simulate handler behavior, + // passing the raw request credentials via opts as the handler does + requestCredentials := newDestination.Credentials newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) - err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{}) + err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) require.NoError(t, err) // Verify that the custom invalidation time was preserved assert.Equal(t, customInvalidAt, newDestination.Credentials["previous_secret_invalid_at"]) }) + t.Run("should apply 24h default on rotation when request omits invalid_at", func(t *testing.T) { + t.Parallel() + storedInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + originalDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": "current-secret", + "previous_secret": "older-secret", + "previous_secret_invalid_at": storedInvalidAt, + }), + ) + + newDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "rotate_secret": "true", + }), + ) + + // Merge both config and credentials to simulate handler behavior: + // the stored previous_secret_invalid_at is merged into newDestination + // even though the caller's request didn't contain it. + requestCredentials := newDestination.Credentials + newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) + newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) + + err := webhookDestination.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) + require.NoError(t, err) + + // The merged-in stored value must not be carried forward; the + // default window applies. + invalidAt, err := time.Parse(time.RFC3339, newDestination.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, 5*time.Second) + }) + t.Run("should set default previous_secret_invalid_at when previous_secret is provided", func(t *testing.T) { t.Parallel() originalDestination := testutil.DestinationFactory.Any( diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go index 62acf0fb5..fc4f098b1 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go @@ -344,7 +344,7 @@ func (d *StandardWebhookDestination) resolveConfig(ctx context.Context, destinat } // rotateSecret handles secret rotation and returns clean credentials -func (d *StandardWebhookDestination) rotateSecret(newDest, origDest *models.Destination) (map[string]string, error) { +func (d *StandardWebhookDestination) rotateSecret(origDest *models.Destination, opts *destregistry.PreprocessDestinationOpts) (map[string]string, error) { if origDest == nil { return nil, destregistry.NewErrDestinationValidation([]destregistry.ValidationErrorDetail{ { @@ -375,9 +375,11 @@ func (d *StandardWebhookDestination) rotateSecret(newDest, origDest *models.Dest } creds["secret"] = secret - // Keep custom invalidation time if provided, otherwise set default - if newDest.Credentials["previous_secret_invalid_at"] != "" { - creds["previous_secret_invalid_at"] = newDest.Credentials["previous_secret_invalid_at"] + // Keep custom invalidation time if provided, otherwise set default. + // The merged credentials can't tell us whether the caller sent the + // field — the raw request credentials can. + if invalidAt := opts.Request.Credentials["previous_secret_invalid_at"]; invalidAt != "" { + creds["previous_secret_invalid_at"] = invalidAt } else { creds["previous_secret_invalid_at"] = time.Now().Add(24 * time.Hour).Format(time.RFC3339) } @@ -508,7 +510,7 @@ func (d *StandardWebhookDestination) Preprocess(newDestination *models.Destinati var cleanCredentials map[string]string var err error if isTruthy(newDestination.Credentials["rotate_secret"]) { - cleanCredentials, err = d.rotateSecret(newDestination, originalDestination) + cleanCredentials, err = d.rotateSecret(originalDestination, opts) } else { cleanCredentials, err = d.updateSecret(newDestination, originalDestination, opts) // For new destinations, ensure credentials are initialized if needed diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go index 0d1c23d8e..115a47191 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard_validate_test.go @@ -553,17 +553,65 @@ func TestStandardWebhookDestination_Preprocess(t *testing.T) { }), ) - // Merge both config and credentials to simulate handler behavior + // Merge both config and credentials to simulate handler behavior, + // passing the raw request credentials via opts as the handler does + requestCredentials := newDestination.Credentials newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) - err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{}) + err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) require.NoError(t, err) // Verify that the custom invalidation time was preserved assert.Equal(t, customInvalidAt, newDestination.Credentials["previous_secret_invalid_at"]) }) + t.Run("should apply 24h default on rotation when request omits invalid_at", func(t *testing.T) { + t.Parallel() + storedInvalidAt := time.Now().Add(1 * time.Hour).Format(time.RFC3339) + originalDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": "whsec_CurrentSecretBase64EncodedString", + "previous_secret": "whsec_OlderSecretBase64EncodedStringgg", + "previous_secret_invalid_at": storedInvalidAt, + }), + ) + + newDestination := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{ + "url": "https://example.com", + }), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "rotate_secret": "true", + }), + ) + + // Merge both config and credentials to simulate handler behavior: + // the stored previous_secret_invalid_at is merged into newDestination + // even though the caller's request didn't contain it. + requestCredentials := newDestination.Credentials + newDestination.Config = maputil.MergeStringMaps(originalDestination.Config, newDestination.Config) + newDestination.Credentials = maputil.MergeStringMaps(originalDestination.Credentials, newDestination.Credentials) + + err := provider.Preprocess(&newDestination, &originalDestination, &destregistry.PreprocessDestinationOpts{ + Request: destregistry.PreprocessRequest{Credentials: requestCredentials}, + }) + require.NoError(t, err) + + // The merged-in stored value must not be carried forward; the + // default window applies. + invalidAt, err := time.Parse(time.RFC3339, newDestination.Credentials["previous_secret_invalid_at"]) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), invalidAt, 5*time.Second) + }) + t.Run("should set default previous_secret_invalid_at when previous_secret is provided", func(t *testing.T) { t.Parallel() originalDestination := testutil.DestinationFactory.Any( diff --git a/internal/destregistry/registry.go b/internal/destregistry/registry.go index 7bf6ba840..385c6d70b 100644 --- a/internal/destregistry/registry.go +++ b/internal/destregistry/registry.go @@ -19,6 +19,19 @@ import ( // PreprocessDestinationOpts contains options for preprocessing a destination type PreprocessDestinationOpts struct { Role string + // Request holds the destination fields exactly as the caller sent them in + // the API request. On updates, newDestination carries the result of + // merge-patching the request into the stored values, so it cannot answer + // "did the caller provide this field" — the request can. + Request PreprocessRequest +} + +// PreprocessRequest is the caller's view of the provider-owned destination +// fields, before any merging with stored state. Maps are nil when the request +// did not contain the corresponding field. +type PreprocessRequest struct { + Config map[string]string + Credentials map[string]string } // Registry manages providers, their metadata, and publishers