Fix Jamf plugin duration serialization (#65944)

* Fix Jamf plugin duration serialization

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

* Update comment, simplify `UnmarshalJSONPB`

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@gmail.com>

---------

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
Co-authored-by: Edoardo Spadolini <edoardo.spadolini@gmail.com>

api/proto/teleport/legacy/types/types.proto

@@ -9050,7 +9050,7 @@ message JamfSpecV1 {
   // Defaults to a random delay (a few minutes max).
   int64 sync_delay = 3 [
     (gogoproto.jsontag) = "sync_delay,omitempty",
-    (gogoproto.casttype) = "Duration"
+    (gogoproto.casttype) = "DurationStringForJamfSpecV1"
   ];
   // Jamf Pro API endpoint.
   // Example: "https://yourtenant.jamfcloud.com/api".
@@ -9075,7 +9075,7 @@ message JamfInventoryEntry {
   // Set to zero or negative to disable PARTIAL syncs.
   int64 sync_period_partial = 2 [
     (gogoproto.jsontag) = "sync_period_partial,omitempty",
-    (gogoproto.casttype) = "Duration"
+    (gogoproto.casttype) = "DurationStringForJamfSpecV1"
   ];
   // Sync period for FULL syncs.
   // Ideally sync_period_full is a multiple of sync_period_partial, so schedules
@@ -9083,7 +9083,7 @@ message JamfInventoryEntry {
   // Set to zero or negative to disable FULL syncs.
   int64 sync_period_full = 3 [
     (gogoproto.jsontag) = "sync_period_full,omitempty",
-    (gogoproto.casttype) = "Duration"
+    (gogoproto.casttype) = "DurationStringForJamfSpecV1"
   ];
   // on_missing is the trigger used on devices missing from the MDM view in a
   // FULL sync.

api/types/jamf_duration.go

@@ -14,13 +14,57 @@
 
 package types
 
-// DurationStringForJamfSpecV1 is a transitional type alias for Duration.
+import (
+	"encoding/json"
+	"time"
+
+	"github.com/gogo/protobuf/jsonpb" //nolint:depguard // needed to implement JSONPBUnmarshaler
+)
+
+var (
+	// json.Marshaler is what both gogo's jsonpb marshal (via json.Marshal) and
+	// ghodss/yaml call for fields of this type.
+	_ json.Marshaler = DurationStringForJamfSpecV1(0)
+	// json.Unmarshaler is called by encoding/json (e.g. via ghodss/yaml in tctl edit).
+	_ json.Unmarshaler = (*DurationStringForJamfSpecV1)(nil)
+	// jsonpb.JSONPBUnmarshaler is what makes this type's existence worthwhile:
+	// gogo's jsonpb checks it before the int64 quote-stripping path that breaks
+	// [Duration] roundtripping.
+	_ jsonpb.JSONPBUnmarshaler = (*DurationStringForJamfSpecV1)(nil)
+)
+
+// DurationStringForJamfSpecV1 is a [Duration]-like casttype used only by the
+// JamfSpecV1 and JamfInventoryEntry proto fields. It exists to work around a
+// bug in gogoproto's jsonpb where int64 casttype fields with a custom
+// MarshalJSON can't be roundtripped: marshal produces a quoted duration
+// string, but unmarshal strips the quotes for int64 fields before passing the
+// value to encoding/json, which then fails because the bare duration string
+// (e.g. `6h0m0s`) is not valid JSON.
 //
-// It exists so that enterprise call sites can switch to this name before the
-// proto casttype for the JamfSpecV1 / JamfInventoryEntry duration fields
-// changes in a follow-up PR. Once the casttype is switched, this alias will
-// be replaced by a distinct named type that implements jsonpb.JSONPBUnmarshaler
-// to work around a serialization bug in gogo's jsonpb.
+// Implementing JSONPBUnmarshaler bypasses the broken path because gogo's
+// jsonpb checks that interface before the int64 quote-stripping.
 //
 // See https://github.com/gravitational/teleport/issues/57747.
-type DurationStringForJamfSpecV1 = Duration
+type DurationStringForJamfSpecV1 time.Duration
+
+// MarshalJSON delegates to [Duration.MarshalJSON]. Called by encoding/json,
+// which is used by both gogo's jsonpb marshal (via json.Marshal) and
+// ghodss/yaml in tctl edit.
+func (d DurationStringForJamfSpecV1) MarshalJSON() ([]byte, error) {
+	return Duration(d).MarshalJSON()
+}
+
+// UnmarshalJSON delegates to [Duration.UnmarshalJSON]. Called by encoding/json
+// (e.g. when ghodss/yaml is used by tctl edit).
+func (d *DurationStringForJamfSpecV1) UnmarshalJSON(data []byte) error {
+	return (*Duration)(d).UnmarshalJSON(data)
+}
+
+// UnmarshalJSONPB intercepts gogo's jsonpb unmarshal before it strips quotes
+// from the string value of int64 fields, delegating to [Duration.UnmarshalJSON]
+// which correctly parses the properly quoted JSON string. We only need this
+// special handling during the unmarshaling, because marshaling will consistently
+// use [json.Marshaler.MarshalJSON] if available.
+func (d *DurationStringForJamfSpecV1) UnmarshalJSONPB(_ *jsonpb.Unmarshaler, data []byte) error {
+	return d.UnmarshalJSON(data)
+}

api/types/jamf_test.go

@@ -60,8 +60,8 @@ func TestValidateJamfSpecV1(t *testing.T) {
 				Inventory: []*types.JamfInventoryEntry{
 					{
 						FilterRsql:        `general.remoteManagement.managed==true and general.platform=="Mac"`,
-						SyncPeriodPartial: types.Duration(4 * time.Hour),
-						SyncPeriodFull:    types.Duration(48 * time.Hour),
+						SyncPeriodPartial: types.DurationStringForJamfSpecV1(4 * time.Hour),
+						SyncPeriodFull:    types.DurationStringForJamfSpecV1(48 * time.Hour),
 						OnMissing:         "DELETE",
 					},
 					{
@@ -106,8 +106,8 @@ func TestValidateJamfSpecV1(t *testing.T) {
 				spec.Inventory = []*types.JamfInventoryEntry{
 					validEntry,
 					{
-						SyncPeriodPartial: types.Duration(12 * time.Hour),
-						SyncPeriodFull:    types.Duration(8 * time.Hour),
+						SyncPeriodPartial: types.DurationStringForJamfSpecV1(12 * time.Hour),
+						SyncPeriodFull:    types.DurationStringForJamfSpecV1(8 * time.Hour),
 					},
 				}
 			}),
@@ -132,7 +132,7 @@ func TestValidateJamfSpecV1(t *testing.T) {
 					validEntry,
 					{
 						SyncPeriodPartial: -1,
-						SyncPeriodFull:    types.Duration(8 * time.Hour),
+						SyncPeriodFull:    types.DurationStringForJamfSpecV1(8 * time.Hour),
 					},
 				}
 			}),
@@ -143,7 +143,7 @@ func TestValidateJamfSpecV1(t *testing.T) {
 				spec.Inventory = []*types.JamfInventoryEntry{
 					validEntry,
 					{
-						SyncPeriodPartial: types.Duration(12 * time.Hour),
+						SyncPeriodPartial: types.DurationStringForJamfSpecV1(12 * time.Hour),
 						SyncPeriodFull:    -1,
 					},
 				}

lib/config/configuration_test.go

@@ -3883,13 +3883,13 @@ jamf_service:
 				Spec: &types.JamfSpecV1{
 					Enabled:     true,
 					Name:        "jamf2",
-					SyncDelay:   types.Duration(1 * time.Minute),
+					SyncDelay:   types.DurationStringForJamfSpecV1(1 * time.Minute),
 					ApiEndpoint: "https://yourtenant.jamfcloud.com",
 					Inventory: []*types.JamfInventoryEntry{
 						{
 							FilterRsql:        "1==1",
-							SyncPeriodPartial: types.Duration(4 * time.Hour),
-							SyncPeriodFull:    types.Duration(48 * time.Hour),
+							SyncPeriodPartial: types.DurationStringForJamfSpecV1(4 * time.Hour),
+							SyncPeriodFull:    types.DurationStringForJamfSpecV1(48 * time.Hour),
 							OnMissing:         "NOOP",
 							PageSize:          10,
 						},

lib/config/fileconf.go

@@ -3169,8 +3169,8 @@ func (j *JamfService) toJamfSpecV1() (*types.JamfSpecV1, error) {
 	for i, e := range j.Inventory {
 		inventory[i] = &types.JamfInventoryEntry{
 			FilterRsql:        e.FilterRSQL,
-			SyncPeriodPartial: types.Duration(e.SyncPeriodPartial),
-			SyncPeriodFull:    types.Duration(e.SyncPeriodFull),
+			SyncPeriodPartial: types.DurationStringForJamfSpecV1(e.SyncPeriodPartial),
+			SyncPeriodFull:    types.DurationStringForJamfSpecV1(e.SyncPeriodFull),
 			OnMissing:         e.OnMissing,
 			PageSize:          e.PageSize,
 		}
@@ -3178,7 +3178,7 @@ func (j *JamfService) toJamfSpecV1() (*types.JamfSpecV1, error) {
 	spec := &types.JamfSpecV1{
 		Enabled:     j.Enabled(),
 		Name:        j.Name,
-		SyncDelay:   types.Duration(j.SyncDelay),
+		SyncDelay:   types.DurationStringForJamfSpecV1(j.SyncDelay),
 		ApiEndpoint: j.APIEndpoint,
 		Inventory:   inventory,
 	}

lib/services/plugins_test.go

@@ -91,6 +91,48 @@ func TestUnmarshalPluginUnknownField(t *testing.T) {
 	})
 }
 
+// TestMarshalPluginJamfDurationRoundTrip checks that a PluginV1 with a Jamf
+// spec with non-zero duration fields roundtrips correctly through MarshalPlugin
+// and UnmarshalPlugin. See https://github.com/gravitational/teleport/issues/57747.
+func TestMarshalPluginJamfDurationRoundTrip(t *testing.T) {
+	spec := types.PluginSpecV1{
+		Settings: &types.PluginSpecV1_Jamf{
+			Jamf: &types.PluginJamfSettings{
+				JamfSpec: &types.JamfSpecV1{
+					ApiEndpoint: "https://test.jamfcloud.com",
+					SyncDelay:   types.DurationStringForJamfSpecV1(6 * time.Hour),
+					Inventory: []*types.JamfInventoryEntry{
+						{
+							FilterRsql:        "general.remoteManagement.managed==true",
+							SyncPeriodPartial: types.DurationStringForJamfSpecV1(6 * time.Hour),
+							SyncPeriodFull:    types.DurationStringForJamfSpecV1(24 * time.Hour),
+							OnMissing:         "DELETE",
+							PageSize:          50,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	creds := &types.PluginCredentialsV1{
+		Credentials: &types.PluginCredentialsV1_StaticCredentialsRef{
+			StaticCredentialsRef: &types.PluginStaticCredentialsRef{
+				Labels: map[string]string{"label": "value"},
+			},
+		},
+	}
+
+	plugin := types.NewPluginV1(types.Metadata{Name: "test-jamf"}, spec, creds)
+
+	payload, err := MarshalPlugin(plugin)
+	require.NoError(t, err)
+
+	unmarshaled, err := UnmarshalPlugin(payload)
+	require.NoError(t, err)
+	require.Empty(t, cmp.Diff(plugin, unmarshaled))
+}
+
 func TestMarshalPluginWithStatus(t *testing.T) {
 	spec := types.PluginSpecV1{
 		Settings: &types.PluginSpecV1_SlackAccessPlugin{