Fix ui.phone_input.allowlist minItems:1 blocking explicit allow-all override- #5842

Fix ui.phone_input.allowlist minItems:1 blocking explicit allow-all override

Author: tung2744

.claude/skills/update-feature-config/SKILL.md

@@ -66,9 +66,76 @@ regression would silently produce the "right" value by accident and the test
 wouldn't catch it. See the existing `hook`/`collaborator`/`identity` cases in
 that file for the pattern.
 
+## Schema/runtime consistency
+
+Before adding a JSON schema constraint on a feature config field
+(`minItems`, `minLength`, `enum`, `required`, etc. in the
+`FeatureConfigSchema.Add(...)` block), check what the field's actual
+*consumer* code does with edge-case values (nil, empty, zero) — grep for
+where the field is read at runtime. A constraint that's stricter than the
+runtime semantics can silently make a legitimately meaningful value
+unreachable. Concrete case: `PhoneInputFeatureConfig.allowlist` had
+`"minItems": 1`, but `IntersectAllowlist` (`pkg/lib/config/utils.go`) already
+treated an empty allowlist as "no restriction" — the schema blocked the one
+input (`allowlist: []`) that would have cleanly expressed "clear this
+override," forcing an awkward, undiscoverable workaround
+(`phone_input: {}` with the field omitted) instead. Don't add a schema
+constraint "for safety" without confirming the runtime already needs it.
+
+## Don't tag a scalar field `omitempty` if its zero value is a real default
+
+`SetFieldDefaults` (`pkg/lib/config/default.go`) already makes every section
+pointer non-nil via its generic reflection walk, regardless of whether that
+section implements `SetDefaults()` — a section is never actually "absent" in
+a parsed/defaulted `FeatureConfig`. If a plain (non-pointer) `bool`/`string`/
+`int`/`float` field's zero value (`false`/`""`/`0`) *is* that field's real,
+intended default — not a stand-in for "not set" — tagging it `omitempty`
+doesn't skip anything meaningful when parsing, but it does hide that value
+from JSON *output*: `encoding/json` treats the zero value as "empty" and
+omits the key, so a fully-resolved section marshals as `{}` instead of e.g.
+`{"disabled": false}`. This makes the Site Admin API's
+`effective_plan_feature_config`/`effective_app_feature_config` (and any
+other JSON consumer of `FeatureConfig`) show a resolved section as if it
+were empty/unset. Fix: drop `omitempty` — plain `json:"disabled"`. This is
+always safe, because `Merge()` for these fields already operates on the
+*section's* pointer-nil-ness (see the field-level merge rule above), never
+the leaf scalar's zero value — removing `omitempty` never changes merge or
+validation behavior, only what the field looks like once marshaled.
+
+This is a different situation from the slice case in "Schema/runtime
+consistency" above (`PhoneInputFeatureConfig.allowlist`): there, `nil` and an
+explicit empty slice are two *different* meaningful values (inherit vs.
+explicitly cleared), so the fix was `omitzero` (which only omits the true
+zero value, `nil`), not simply dropping the tag. A plain scalar only has one
+value to begin with, so just remove `omitempty` entirely — don't reach for
+`omitzero` there, it would be a no-op.
+
+**Test with a real marshal, not `ShouldResemble` on parsed structs.** Every
+existing test in this package compares parsed Go *structs*, which can't tell
+`omitempty` apart from no tag at all — that's exactly why this went
+unnoticed for nine fields across five sections. Marshal with
+`encoding/json.Marshal` (or `sigs.k8s.io/yaml.Marshal`, which calls it
+internally — this is what `viewEffectiveResource`'s merge fold does) and
+assert on the resulting shape, e.g.
+`TestFeatureConfigDisabledFieldsSerializeExplicitly` in `feature_test.go`.
+
+**When auditing for this, grep the whole package by field, not file by
+file.** A file having a `SetDefaults()` for one field doesn't mean every
+field in that file is covered — `feature_identity.go` has one for
+`BiometricFeatureConfig` (a pointer-scalar field) while
+`LoginIDPhoneFeatureConfig.Disabled` and
+`OAuthSSOProviderFeatureConfig.Disabled` (plain-bool, single-field sections
+in that same file) still had the bug. Use:
+
+```
+grep -nE '^\s*[A-Z][A-Za-z0-9_]*\s+(bool|string|int|int32|int64|float32|float64)\s+`json:"[^"]*,omitempty"`' pkg/lib/config/feature_*.go
+```
+
 ## References
 
 - `pkg/lib/config/feature.go` — top-level `FeatureConfig.Merge` dispatcher
 - `pkg/lib/config/feature_*.go` — per-section `Merge` implementations
 - `pkg/lib/config/testdata/merge_feature.yaml` — shared merge test fixture
 - `pkg/lib/config/testdata/parse_feature_tests.yaml` — schema validation test fixture
+- `pkg/lib/config/default.go` — `SetFieldDefaults`, the generic reflection walk
+- `pkg/lib/config/feature_test.go`'s `TestFeatureConfigDisabledFieldsSerializeExplicitly` — marshal-based test pattern for the omitempty rule above

pkg/lib/config/config_test.go

@@ -82,6 +82,25 @@ func TestApplyFeatureConfigConstraints(t *testing.T) {
 			So(appConfig.UI.PhoneInput.AllowList, ShouldBeNil)
 		})
 
+		Convey("explicit empty feature allowlist means allow all, same as nil", func() {
+			appConfig := &config.AppConfig{
+				UI: &config.UIConfig{
+					PhoneInput: &config.PhoneInputConfig{
+						AllowList: []string{"SG", "MY", "TH"},
+					},
+				},
+			}
+			featureConfig := &config.FeatureConfig{
+				UI: &config.UIFeatureConfig{
+					PhoneInput: &config.PhoneInputFeatureConfig{
+						AllowList: []string{}, // explicit empty, not nil -- an app override clearing a plan restriction
+					},
+				},
+			}
+			config.ApplyFeatureConfigConstraints(appConfig, featureConfig)
+			So(appConfig.UI.PhoneInput.AllowList, ShouldResemble, []string{"SG", "MY", "TH"})
+		})
+
 		Convey("does not panic when phone input config is absent", func() {
 			So(func() {
 				config.ApplyFeatureConfigConstraints(&config.AppConfig{}, &config.FeatureConfig{})

pkg/lib/config/feature_authentication.go

@@ -72,5 +72,7 @@ var _ = FeatureConfigSchema.Add("AuthenticatorOOBOTBSMSFeatureConfig", `
 `)
 
 type AuthenticatorOOBOTBSMSFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }

pkg/lib/config/feature_collaborator.go

@@ -12,6 +12,13 @@ var _ = FeatureConfigSchema.Add("CollaboratorFeatureConfig", `
 `)
 
 type CollaboratorFeatureConfig struct {
+	// Maximum and SoftMaximum are deliberately left without a SetDefaults():
+	// nil means "unlimited" and is the current, intended behavior. Both
+	// pkg/portal/service/collaborator.go (checkQuotaInSend/checkQuotaInAccept)
+	// and portal/src/graphql/portal/PortalAdminsSettings.tsx skip
+	// quota enforcement/warnings entirely when these are nil/undefined. Giving
+	// either field a concrete default would start enforcing a collaborator
+	// cap (or showing a quota warning) that does not exist today.
 	Maximum     *int `json:"maximum,omitempty"`
 	SoftMaximum *int `json:"soft_maximum,omitempty"`
 }

pkg/lib/config/feature_custom_domain.go

@@ -11,7 +11,9 @@ var _ = FeatureConfigSchema.Add("CustomDomainFeatureConfig", `
 `)
 
 type CustomDomainFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ MergeableFeatureConfig = &CustomDomainFeatureConfig{}

pkg/lib/config/feature_google_tag_manager.go

@@ -11,7 +11,9 @@ var _ = FeatureConfigSchema.Add("GoogleTagManagerFeatureConfig", `
 `)
 
 type GoogleTagManagerFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ MergeableFeatureConfig = &GoogleTagManagerFeatureConfig{}

pkg/lib/config/feature_identity.go

@@ -86,7 +86,9 @@ var _ = FeatureConfigSchema.Add("LoginIDPhoneFeatureConfig", `
 `)
 
 type LoginIDPhoneFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ = FeatureConfigSchema.Add("OAuthSSOFeatureConfig", `
@@ -235,7 +237,9 @@ var _ = FeatureConfigSchema.Add("OAuthSSOProviderFeatureConfig", `
 `)
 
 type OAuthSSOProviderFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ = FeatureConfigSchema.Add("BiometricFeatureConfig", `

pkg/lib/config/feature_rate_limits.go

@@ -11,7 +11,9 @@ var _ = FeatureConfigSchema.Add("RateLimitsFeatureConfig", `
 `)
 
 type RateLimitsFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ MergeableFeatureConfig = &RateLimitsFeatureConfig{}

pkg/lib/config/feature_test.go

@@ -2,6 +2,7 @@ package config_test
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"io"
 
@@ -172,3 +173,129 @@ func TestFeatureConfigMigrate(t *testing.T) {
 		}})
 	})
 }
+
+// This mirrors configsource/resources.go's viewEffectiveResource exactly
+// (fold layers via Merge, then yaml.Marshal, then re-parse) rather than the
+// "merge feature config" Convey block above, which stops at
+// SetFieldDefaults/Migrate on the in-memory merged struct and so never
+// exercises the yaml.Marshal step -- the one place a field tagged
+// `omitempty` (as opposed to `omitzero`) can silently turn an explicit
+// empty slice back into an absent/nil field once re-parsed.
+func TestFeatureConfigEffectiveResourceRoundTrip(t *testing.T) {
+	Convey("an explicit empty phone_input.allowlist override survives the merge fold's marshal-and-reparse round trip", t, func() {
+		ctx := context.Background()
+
+		planYAML := []byte(`
+ui:
+  phone_input:
+    allowlist:
+      - US
+      - GB
+`)
+		appYAML := []byte(`
+ui:
+  phone_input:
+    allowlist: []
+`)
+
+		planCfg, err := config.ParseFeatureConfigWithoutDefaults(ctx, planYAML)
+		So(err, ShouldBeNil)
+		appCfg, err := config.ParseFeatureConfigWithoutDefaults(ctx, appYAML)
+		So(err, ShouldBeNil)
+
+		mergedConfig := &config.FeatureConfig{}
+		mergedConfig = mergedConfig.Merge(planCfg)
+		mergedConfig = mergedConfig.Merge(appCfg)
+
+		mergedYAML, err := yaml.Marshal(mergedConfig)
+		So(err, ShouldBeNil)
+
+		effective, err := config.ParseFeatureConfig(ctx, mergedYAML)
+		So(err, ShouldBeNil)
+
+		So(effective.UI, ShouldNotBeNil)
+		So(effective.UI.PhoneInput, ShouldNotBeNil)
+		So(effective.UI.PhoneInput.AllowList, ShouldResemble, []string{})
+	})
+
+	Convey("an app override that doesn't touch phone_input.allowlist inherits the plan's list", t, func() {
+		ctx := context.Background()
+
+		planYAML := []byte(`
+ui:
+  phone_input:
+    allowlist:
+      - US
+      - GB
+`)
+		// The app layer is present and non-empty, but never mentions
+		// phone_input -- this is the "not set" case, distinct from the
+		// explicit-empty case above, and must inherit the plan's list.
+		appYAML := []byte(`
+collaborator:
+  maximum: 5
+`)
+
+		planCfg, err := config.ParseFeatureConfigWithoutDefaults(ctx, planYAML)
+		So(err, ShouldBeNil)
+		appCfg, err := config.ParseFeatureConfigWithoutDefaults(ctx, appYAML)
+		So(err, ShouldBeNil)
+
+		mergedConfig := &config.FeatureConfig{}
+		mergedConfig = mergedConfig.Merge(planCfg)
+		mergedConfig = mergedConfig.Merge(appCfg)
+
+		mergedYAML, err := yaml.Marshal(mergedConfig)
+		So(err, ShouldBeNil)
+
+		effective, err := config.ParseFeatureConfig(ctx, mergedYAML)
+		So(err, ShouldBeNil)
+
+		So(effective.UI, ShouldNotBeNil)
+		So(effective.UI.PhoneInput, ShouldNotBeNil)
+		So(effective.UI.PhoneInput.AllowList, ShouldResemble, []string{"US", "GB"})
+	})
+}
+
+// These single-field sections have `false` as their real, correct default
+// (not disabled) -- `omitempty` hid that from JSON output, making a
+// fully-resolved section marshal as an empty object indistinguishable from
+// one with no fields at all. Sections stay non-nil with a `false` value
+// regardless, from SetFieldDefaults's generic pointer-to-struct
+// initialization (default.go) -- this test is specifically about the
+// *serialized shape*, which is what the Site Admin API's feature-config UI
+// (and any other JSON consumer of FeatureConfig) actually sees.
+func TestFeatureConfigDisabledFieldsSerializeExplicitly(t *testing.T) {
+	Convey("false-valued Disabled fields marshal explicitly, not as an empty object", t, func() {
+		ctx := context.Background()
+		cfg, err := config.ParseFeatureConfig(ctx, []byte(`{}`))
+		So(err, ShouldBeNil)
+
+		data, err := json.Marshal(cfg)
+		So(err, ShouldBeNil)
+
+		var raw map[string]any
+		err = json.Unmarshal(data, &raw)
+		So(err, ShouldBeNil)
+
+		So(raw["custom_domain"], ShouldResemble, map[string]any{"disabled": false})
+		So(raw["google_tag_manager"], ShouldResemble, map[string]any{"disabled": false})
+		So(raw["rate_limits"], ShouldResemble, map[string]any{"disabled": false})
+
+		ui, _ := raw["ui"].(map[string]any)
+		So(ui["white_labeling"], ShouldResemble, map[string]any{"disabled": false})
+
+		authentication, _ := raw["authentication"].(map[string]any)
+		secondaryAuthenticators, _ := authentication["secondary_authenticators"].(map[string]any)
+		So(secondaryAuthenticators["oob_otp_sms"], ShouldResemble, map[string]any{"disabled": false})
+
+		identity, _ := raw["identity"].(map[string]any)
+		loginID, _ := identity["login_id"].(map[string]any)
+		types, _ := loginID["types"].(map[string]any)
+		So(types["phone"], ShouldResemble, map[string]any{"disabled": false})
+
+		oauth, _ := identity["oauth"].(map[string]any)
+		providers, _ := oauth["providers"].(map[string]any)
+		So(providers["google"], ShouldResemble, map[string]any{"disabled": false})
+	})
+}

pkg/lib/config/feature_ui.go

@@ -51,21 +51,33 @@ var _ = FeatureConfigSchema.Add("WhiteLabelingFeatureConfig", `
 `)
 
 type WhiteLabelingFeatureConfig struct {
-	Disabled bool `json:"disabled,omitempty"`
+	// No omitempty: false is this field's real default, not an absence
+	// (see the update-feature-config skill).
+	Disabled bool `json:"disabled"`
 }
 
 var _ = FeatureConfigSchema.Add("PhoneInputFeatureConfig", `
 {
 	"type": "object",
 	"additionalProperties": false,
 	"properties": {
-		"allowlist": { "type": "array", "items": { "$ref": "#/$defs/ISO31661Alpha2" }, "minItems": 1 }
+		"allowlist": { "type": "array", "items": { "$ref": "#/$defs/ISO31661Alpha2" } }
 	}
 }
 `)
 
 type PhoneInputFeatureConfig struct {
-	AllowList []string `json:"allowlist,omitempty"`
+	// omitzero, not omitempty: nil (unset, inherit from plan) and an
+	// explicit empty slice (allow all countries) are semantically distinct
+	// here (see ApplyFeatureConfigConstraints/IntersectAllowlist). omitempty
+	// treats both as "empty" and drops the field either way, which loses
+	// the explicit-empty signal when this struct is re-marshaled during the
+	// merge fold's final yaml.Marshal step (configsource/resources.go's
+	// viewEffectiveResource) -- turning an explicit "allow all" override
+	// back into an absent field, indistinguishable from "not set", once
+	// re-parsed. omitzero only omits the true zero value (nil), preserving
+	// AllowList: []string{} through that round trip.
+	AllowList []string `json:"allowlist,omitzero"`
 }
 
 var _ = FeatureConfigSchema.Add("ISO31661Alpha2", phone.JSONSchemaString)