fix: use *bool for RegisterOnFirstLogin to preserve Connect default

[ian-flores]

Summary

• Fixes a regression introduced in feat: add support for disabling Connect OAuth2.RegisterOnFirstLogin #87 where RegisterOnFirstLogin bool defaulted to false, causing the zero value to always be written as RegisterOnFirstLogin = false in the generated gcfg
• This silently overrode Connect's native default of true for all existing OIDC deployments
• Changes the field to *bool across ConnectOAuth2Config, ConnectSpec, and InternalConnectSpec so that nil (unset) produces no entry in the config, letting Connect use its own default
• Users who explicitly want to disable the behavior can set registerOnFirstLogin: false

Test plan

• TestConnectConfig_RegisterOnFirstLogin — verifies ptr.To(true) writes the value; nil omits it entirely
• TestSiteReconciler_RegisterOnFirstLoginPropagation — verifies explicit true propagates through Site → Connect
• TestSiteReconciler_RegisterOnFirstLoginDefaultFalse — verifies unset field stays nil (not false)
• Controller integration tests (require etcd, run in CI)

[claude]

Claude finished @ian-flores's task —— View job

---

PR Review: fix: use *bool for RegisterOnFirstLogin to preserve Connect default

• Read review guidelines
• Read PR diff and changed files
• Analyze changes against review checklist
• Submit review with inline comments

Review submitted. The bool → *bool change is correct and the gcfg serializer properly handles nil pointers. Key feedback:

1. Missing test for explicit false — ptr.To(false) should be tested to cover the opt-out path
2. Misleading test name — TestSiteReconciler_RegisterOnFirstLoginDefaultFalse now asserts Nil, not False
3. Suggestion — Deduplicate the nil-guard in the controller warning logic

[claude]

Good fix — the bool → *bool change is the correct approach for preserving Connect's native default. The gcfg reflection-based serializer correctly skips nil pointers (lines 234-243 in connect_config.go), so this works end-to-end. The deepcopy changes are properly generated and the Site → Connect propagation is a clean pointer pass-through.

A few items:

Missing test for explicit false (important)
api/core/v1beta1/connect_config_test.go — There's no test for ptr.To(false). The PR description says "Users who explicitly want to disable the behavior can set registerOnFirstLogin: false", but there's no coverage verifying that ptr.To(false) writes RegisterOnFirstLogin = false to the generated gcfg. This is worth adding since it's the key opt-out path.

Misleading test name
internal/controller/core/site_test.go:1039 — TestSiteReconciler_RegisterOnFirstLoginDefaultFalse now asserts Nil, not False. Consider renaming to TestSiteReconciler_RegisterOnFirstLoginDefaultNil to match the new semantics.

Duplicated nil-guard in controller logic (suggestion)
internal/controller/core/connect.go:271-273 — The c.Spec.RegisterOnFirstLogin != nil && *c.Spec.RegisterOnFirstLogin check is repeated across two branches. Consider:

if regOnFirst := c.Spec.RegisterOnFirstLogin; regOnFirst != nil && *regOnFirst {
    if c.Spec.Auth.Type == "" {
        l.Info("registerOnFirstLogin is set but no auth type is configured; ...")
    } else if c.Spec.Auth.Type != positcov1beta1.AuthTypeOidc {
        l.Info("registerOnFirstLogin is set but auth type is not oidc; ...", "authType", c.Spec.Auth.Type)
    }
}

This avoids evaluating the nil check twice and makes intent clearer.

Checklist (API changes)

• New field has sensible default (nil = omit → Connect uses its own default)
• omitempty tag present on JSON serialization
• DeepCopy generated correctly for pointer field
• Config flows from Site → Product correctly (line 149 in site_controller_connect.go)

Checklist (Controller changes)

• Reconciliation remains idempotent
• Unit tests exist for set/unset/ignored-auth scenarios
• Missing: test for explicit false value

[stevenolen]

thanks @ian-flores! lgtm.

@statik -- i think your pr in #87 was only intended to set this value as false for a small set of environments, rather than all, right?

[statik]

@stevenolen @ian-flores thanks for fixing! yes, it was intending to preserve the product default :(