From 5b02246f4f599463d8f7bf15b9e68cd6611d222d Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 10 Jun 2026 09:03:07 +0200 Subject: [PATCH] Partial revert coderabbit added docstrings --- client/cmd/debug.go | 25 ++--- client/cmd/root.go | 24 ++--- client/internal/profilemanager/config.go | 9 +- client/mdm/policy.go | 26 ++--- client/mdm/policy_darwin.go | 25 ++--- client/mdm/policy_windows.go | 49 ++++----- client/mdm/ticker.go | 55 +++++----- client/server/mdm.go | 122 ++++++----------------- client/server/server.go | 15 +-- client/ui/client_ui.go | 27 ++--- 10 files changed, 133 insertions(+), 244 deletions(-) diff --git a/client/cmd/debug.go b/client/cmd/debug.go index 5c1b343b9..bc7b0e98c 100644 --- a/client/cmd/debug.go +++ b/client/cmd/debug.go @@ -104,14 +104,9 @@ var debugConfigCmd = &cobra.Command{ // Useful for verifying MDM enforcement end-to-end: the response's // mDMManagedFields array is the single source of truth for "which // fields is the daemon currently enforcing from the MDM source", and -// every config field side-by-side with that list confirms the -// merge result. Secrets in the response (e.g. PreSharedKey) are -// debugConfigDump requests the daemon for the resolved effective configuration and prints it as indented JSON. -// It resolves the active profile and current OS user, calls DaemonService.GetConfig with those values, and -// marshals the response using protojson with default/zero-valued fields included. -// debugConfigDump prints the daemon's effective configuration for the active profile and current OS user as indented JSON. -// It requests the configuration from the daemon and writes the protobuf response with default fields emitted to stdout. -// Returns an error if active profile or user lookup fails, the daemon RPC fails, or the response cannot be marshaled. +// every config field side-by-side with that list confirms the merge +// result. Secrets in the response (e.g. PreSharedKey) are already +// redacted by the daemon-side handler. func debugConfigDump(cmd *cobra.Command, _ []string) error { pm := profilemanager.NewProfileManager() activeProf, err := pm.GetActiveProfile() @@ -153,14 +148,12 @@ func debugConfigDump(cmd *cobra.Command, _ []string) error { return nil } -// debugBundle requests the daemon to create a debug bundle and prints the resulting -// local file path and, if uploaded, the uploaded file key. -// It uses the package flags (anonymize, system info, log file count, CLI version and -// optional upload URL) to configure the bundle request. Returns an error if the RPC -// debugBundle requests creation of a debug bundle from the daemon and prints -// the local bundle file path and, if uploading was enabled, the uploaded file key. -// It returns an error if the RPC fails, if the daemon reports an upload failure -// reason, or if establishing the connection fails. +// debugBundle requests the daemon to create a debug bundle and prints +// the resulting local file path and, if uploaded, the uploaded file +// key. It uses the package flags (anonymize, system info, log file +// count, CLI version, optional upload URL) to configure the bundle +// request. Returns an error if the RPC fails or if the daemon reports +// an upload failure reason. func debugBundle(cmd *cobra.Command, _ []string) error { conn, err := getClient(cmd) if err != nil { diff --git a/client/cmd/root.go b/client/cmd/root.go index 34ba2e8b7..ead29248e 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -105,20 +105,16 @@ func Execute() error { return rootCmd.Execute() } -// init initializes package-level defaults and the CLI command tree. -// init sets platform-specific default config and log directory paths and a default daemon address, -// registers persistent flags (daemon address, management/admin URLs, logging, setup key, preshared key, -// hostname, anonymize, config path), attaches top-level and nested subcommands to the root command, -// and configures `up` command specific flags (external IP maps, DNS resolver address, Rosenpass options, -// init initializes package-level defaults and configures the root Cobra command. -// -// It sets default configuration and log directory paths (including legacy Wiretrustee -// locations) based on the runtime OS, builds default config/log file paths, and selects -// a platform-appropriate default daemon address. It registers persistent CLI flags -// (including mutually exclusive setup-key and setup-key-file), attaches top-level -// commands and subcommands to the root command, and registers `up`-specific persistent -// flags for external IP mapping, custom DNS resolver address, Rosenpass options, -// auto-connect disabling, and lazy connection. +// init initialises package-level defaults and configures the root +// Cobra command tree. Sets platform-specific config / log directory +// paths (including legacy Wiretrustee fallbacks) and a default daemon +// address; registers persistent CLI flags (daemon address, +// management / admin URLs, logging, setup key (file and inline, +// mutually exclusive), preshared key, hostname, anonymise, config +// path); attaches top-level and nested subcommands to the root +// command; and registers `up`-specific persistent flags (external IP +// maps, custom DNS resolver address, Rosenpass options, auto-connect +// disabling, lazy connection). func init() { defaultConfigPathDir = "/etc/netbird/" defaultLogFileDir = "/var/log/netbird/" diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 8c612718b..550fc2ace 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -716,10 +716,11 @@ func (config *Config) applyMDMPolicy(policy *mdm.Policy) { } } -// parseURL parses and validates the URL for the named service. -// It requires the URL to use the http or https scheme and, if no port is present, -// appends ":443" for https or ":80" for http. On success it returns the parsed -// ":80" is appended. The `serviceName` is used to contextualize error messages. +// parseURL parses and validates the URL for the named service. The URL +// must use the http or https scheme; if no port is present, ":443" is +// appended for https or ":80" for http. The serviceName parameter is +// used to contextualise error messages. On success returns the parsed +// *url.URL; on failure returns a non-nil error. func parseURL(serviceName, serviceURL string) (*url.URL, error) { parsedMgmtURL, err := url.ParseRequestURI(serviceURL) if err != nil { diff --git a/client/mdm/policy.go b/client/mdm/policy.go index 165f8fb77..0f7ca6379 100644 --- a/client/mdm/policy.go +++ b/client/mdm/policy.go @@ -82,12 +82,9 @@ type Policy struct { values map[string]any } -// NewPolicy constructs a Policy from a key→value map. Pass nil or an empty -// NewPolicy constructs a Policy backed by the provided key→value map. -// If values is nil it is replaced with an empty map so the returned *Policy -// NewPolicy constructs a non-nil *Policy that wraps the provided key/value map. -// If values is nil it is replaced with an empty map so the returned Policy always -// represents "no MDM enforcement" when its values are empty. +// NewPolicy constructs a Policy from a key→value map. Pass nil or an +// empty map to construct an empty (no-enforcement) Policy. The returned +// *Policy is always non-nil. func NewPolicy(values map[string]any) *Policy { if values == nil { values = map[string]any{} @@ -95,20 +92,14 @@ func NewPolicy(values map[string]any) *Policy { return &Policy{values: values} } -// LoadPolicy reads the platform-native MDM configuration. Returns an empty -// (but non-nil) Policy when no source is present, the source is empty, or -// the platform is unsupported. +// LoadPolicy reads the platform-native MDM configuration. Returns an +// empty (but non-nil) Policy when no source is present, the source is +// empty, or the platform is unsupported. // // Diagnostic logging differentiates the three states: // - source absent / unsupported platform: trace log only // - source present, zero keys: info "MDM enrolled (no managed keys)" -// LoadPolicy loads MDM-managed configuration from the platform and returns a Policy representing the managed settings. -// If the platform loader fails or returns nil, LoadPolicy returns a non-nil empty Policy. -// LoadPolicy loads platform-managed MDM key/value pairs and returns a non-nil Policy. -// If the platform loader returns an error or a nil map, an empty Policy is returned. -// On loader error a trace-level message is emitted. When a map is returned, an -// informational message is logged either indicating enrollment with no managed keys -// or the count and a stable, sorted list of managed key names. +// - source present, N keys: info "MDM enrolled with N managed keys: [...]" func LoadPolicy() *Policy { values, err := loadPlatformPolicy() if err != nil { @@ -266,8 +257,7 @@ func (p *Policy) GetStringSlice(key string) ([]string, bool) { // sortedKeys returns the keys of m as a deterministic, lexicographically // sorted slice. Used internally by Policy.ManagedKeys and LoadPolicy's // diagnostic log line so callers see a stable key order across runs -// sortedKeys returns the keys of m as a lexicographically sorted slice. -// The sorted order provides a deterministic key ordering for diagnostics and enumeration. +// regardless of Go's randomised map iteration. func sortedKeys(m map[string]any) []string { out := make([]string, 0, len(m)) for k := range m { diff --git a/client/mdm/policy_darwin.go b/client/mdm/policy_darwin.go index a644e695c..57aa1168c 100644 --- a/client/mdm/policy_darwin.go +++ b/client/mdm/policy_darwin.go @@ -26,28 +26,19 @@ import ( const policyPlistPath = "/Library/Managed Preferences/io.netbird.client.plist" // loadPlatformPolicy reads the MDM-managed configuration from the macOS -// managed-preferences plist. Returns: +// managed-preferences plist at policyPlistPath. Returns: // - (nil, nil) when the plist is absent (device not MDM-enrolled for // NetBird, or admin has not yet pushed a payload) // - (map, nil) with N entries when N managed values are present // (N may be 0 — empty plist still signals enrollment to the caller) -// - (nil, err) on permission / parse / safety errors +// - (nil, err) on permission / parse / safety errors (including +// refusal to read a world-writable plist) // -// Value-type coercion mirrors the Windows loader: native plist types -// map naturally onto the Policy accessor expectations (GetString / -// GetBool / GetInt / GetStringSlice). Unknown top-level keys are -// logged and skipped so a stray entry in the payload does not block -// loadPlatformPolicy reads the managed-preferences plist at policyPlistPath and returns recognised MDM key/value pairs. -// -// If the plist file does not exist, it returns (nil, nil). It returns a wrapped error on open/stat/decode failures. -// The function refuses to read a world-writable plist and returns an error in that case. -// loadPlatformPolicy reads the device-level managed-preferences plist and returns its recognized keys. -// -// It looks for the plist at policyPlistPath and, if present, decodes it into a map[string]any. -// Top-level plist keys are canonicalized case-insensitively to the package's internal MDM key names; -// unknown keys are logged and ignored. If the plist file does not exist, it returns (nil, nil). -// The function refuses to read the file if it is world-writable and returns a wrapped error for -// failures to open, stat, or decode the plist. +// Top-level plist keys are canonicalised case-insensitively to the +// package's internal mdm.Key* names; unknown keys are logged and +// skipped so a stray entry in the payload does not block startup. +// Native plist value types map naturally onto the Policy accessor +// expectations (GetString / GetBool / GetInt / GetStringSlice). func loadPlatformPolicy() (map[string]any, error) { f, err := os.Open(policyPlistPath) if err != nil { diff --git a/client/mdm/policy_windows.go b/client/mdm/policy_windows.go index 27d4dbf85..0c2629f98 100644 --- a/client/mdm/policy_windows.go +++ b/client/mdm/policy_windows.go @@ -17,27 +17,17 @@ import ( // Listed in the project's docs/mdm/netbird.admx schema. const policyRegistryPath = `Software\Policies\NetBird` -// loadPlatformPolicy reads the MDM-managed configuration from the Windows -// registry under HKLM\Software\Policies\NetBird. Returns: -// - (nil, nil) when the key is absent (device not MDM-enrolled for NetBird) -// - (map, nil) with N entries when N managed values are set (N may be 0) -// - (nil, err) on any other registry error +// readRegistryValue reads a single value under policyRegistryPath and, +// on success, stores the type-coerced result in out[canonical]. Type +// coercion mirrors loadPlatformPolicy's documented mapping: +// - REG_SZ / REG_EXPAND_SZ -> string (REG_EXPAND_SZ is expanded by the API) +// - REG_DWORD / REG_QWORD -> int64 +// - REG_MULTI_SZ -> []string // -// Type coercion of registry value types into the Policy map: -// - REG_SZ -> string -// - REG_EXPAND_SZ -> string (expanded by the registry API) -// - REG_DWORD -> int64 (caller's GetBool handles 0/!=0 coercion) -// - REG_QWORD -> int64 -// - REG_MULTI_SZ -> []string -// -// Unsupported value types (REG_BINARY, REG_NONE, ...) are skipped with a -// loadPlatformPolicy reads managed NetBird policy values from HKLM\Software\Policies\NetBird. -// If the registry key does not exist it returns (nil, nil). -// It returns a map whose keys are canonical policy names and whose values are coerced from registry types: -// REG_SZ/REG_EXPAND_SZ -> string, REG_DWORD/REG_QWORD -> int64, REG_MULTI_SZ -> []string. -// readRegistryValue reads the registry value named by name from key k and, when the value is successfully read and its type is supported, stores the coerced Go value in out[canonical]. -// -// REG_SZ and REG_EXPAND_SZ are stored as string, REG_DWORD and REG_QWORD are stored as int64, and REG_MULTI_SZ is stored as []string; unknown value names, unsupported value types, and per-value read errors are logged and skipped. +// Unsupported value types and per-value read failures are logged at +// warn level and skipped — one malformed value must not block the +// surrounding loop. Extracted from loadPlatformPolicy to keep that +// function's cognitive complexity in check. func readRegistryValue(k registry.Key, name, canonical string, out map[string]any) { _, valType, err := k.GetValue(name, nil) if err != nil { @@ -71,16 +61,15 @@ func readRegistryValue(k registry.Key, name, canonical string, out map[string]an } } -// loadPlatformPolicy loads MDM-managed NetBird policy values from the Windows -// registry at HKLM\Software\Policies\NetBird. -// -// It returns a map that maps canonical policy names to coerced Go values: -// string for REG_SZ/REG_EXPAND_SZ, int64 for REG_DWORD/REG_QWORD, and []string -// for REG_MULTI_SZ. If the policy registry key does not exist, it returns -// (nil, nil). It returns an error when opening or enumerating the registry -// key fails. Individual values that are unknown, of unsupported types, or that -// fail to read are skipped and produce logged warnings; registry close failures -// are also logged. +// loadPlatformPolicy reads the MDM-managed configuration from the +// Windows registry under HKLM\Software\Policies\NetBird. Returns: +// - (nil, nil) when the key is absent (device not MDM-enrolled for NetBird) +// - (map, nil) with N entries when N managed values are set (N may be 0) +// - (nil, err) on open / enumerate registry errors +// +// Per-value type coercion + skip-on-error is delegated to +// readRegistryValue. Unknown value names are logged and skipped so a +// malformed deployment does not block startup. func loadPlatformPolicy() (map[string]any, error) { k, err := registry.OpenKey(registry.LOCAL_MACHINE, policyRegistryPath, registry.QUERY_VALUE) if err != nil { diff --git a/client/mdm/ticker.go b/client/mdm/ticker.go index 0d94deb55..143f4686b 100644 --- a/client/mdm/ticker.go +++ b/client/mdm/ticker.go @@ -24,10 +24,9 @@ const defaultReloadInterval = 1 * time.Minute const testReloadInterval = 1 * time.Second // reloadInterval returns the production cadence, or the accelerated test -// cadence when running under `go test`. Centralising the choice here keeps -// reloadInterval selects the polling interval used to re-read the OS-native MDM policy. -// reloadInterval selects the polling interval used for policy reloads. -// It returns testReloadInterval when running under `go test` (testing.Testing() == true), and defaultReloadInterval otherwise. +// cadence when running under `go test` (detected via testing.Testing()). +// Centralising the choice here keeps the prod/test split in one place +// and out of the ticker's call sites. func reloadInterval() time.Duration { if testing.Testing() { return testReloadInterval @@ -51,16 +50,15 @@ type Ticker struct { prev *Policy } -// NewTicker constructs a Ticker that re-reads the OS-native policy every -// reloadInterval() and invokes onChange on any diff. The cadence is owned by -// reloadInterval (production default, accelerated under `go test`); callers -// NewTicker creates a Ticker that polls the OS-native MDM policy at the package reload interval and invokes onChange when a policy change is detected. -// If onChange is nil the ticker will only log detected changes. -// NewTicker creates a Ticker that polls for policy changes and invokes onChange when a difference is detected. -// -// The provided onChange callback, if non-nil, is called with the previous and current Policy snapshots when a -// change is observed. The returned Ticker's polling interval is set via reloadInterval and its initial snapshot -// is populated by calling policyLoader. +// NewTicker constructs a Ticker that re-reads the OS-native policy +// every reloadInterval() and invokes onChange on any diff. The +// cadence is owned by reloadInterval (production default, accelerated +// under `go test`); callers do not supply it. onChange may be nil for +// a log-only ticker. The initial snapshot is populated by calling +// policyLoader at construction time so the first tick only fires +// onChange when the policy actually changed since boot — without +// this baseline the first tick would report every currently-managed +// key as "added" and trigger a spurious engine restart. func NewTicker(onChange func(prev, curr *Policy)) *Ticker { return &Ticker{ interval: reloadInterval(), @@ -98,11 +96,10 @@ func (t *Ticker) Run(ctx context.Context) { } } -// PoliciesEqual reports whether two Policy instances carry the same managed -// PoliciesEqual reports whether two Policy instances represent the same policy. -// It returns true when both policies are empty, returns false if one pointer is nil -// while the other is not, and otherwise compares the policies' underlying value -// maps for deep equality. +// PoliciesEqual reports whether two Policy instances carry the same +// managed key set with identical values. Nil and empty policies +// compare equal; one-nil/one-non-empty compare not equal; otherwise +// the underlying values maps are compared with reflect.DeepEqual. func PoliciesEqual(a, b *Policy) bool { if a.IsEmpty() && b.IsEmpty() { return true @@ -113,12 +110,10 @@ func PoliciesEqual(a, b *Policy) bool { return reflect.DeepEqual(a.values, b.values) } -// diffPolicies returns the keys added in curr, removed from prev, and whose -// diffPolicies reports keys that were added, removed, or changed between two policies. -// The returned slices contain keys present only in `curr` (added), only in `prev` (removed), -// and present in both but whose values differ (changed). Each slice is sorted -// lexicographically for stable logging output; value differences are determined -// associated values differ by deep equality. +// diffPolicies returns the keys added in curr, removed from prev, and +// whose values changed between prev and curr. Each slice is sorted +// lexicographically for stable log output; value differences are +// determined with reflect.DeepEqual. func diffPolicies(prev, curr *Policy) (added, removed, changed []string) { prevKeys := mapOf(prev) currKeys := mapOf(curr) @@ -140,13 +135,9 @@ func diffPolicies(prev, curr *Policy) (added, removed, changed []string) { return added, removed, changed } -// mapOf returns a (possibly empty, never nil) copy of the underlying values -// map of a Policy so callers outside this package can compare across the -// mapOf returns a non-nil copy of the given Policy's key/value map. -// If p is nil, mapOf returns an empty map; otherwise it returns a newly -// mapOf returns a non-nil copy of a Policy's values map. -// If p is nil it returns an empty map; otherwise it returns a newly -// allocated map containing the same key/value pairs as p.values. +// mapOf returns a (possibly empty, never nil) copy of the underlying +// values map of a Policy so callers outside this package can compare +// keys/values across the type boundary. Returns an empty map on nil p. func mapOf(p *Policy) map[string]any { if p == nil { return map[string]any{} diff --git a/client/server/mdm.go b/client/server/mdm.go index fd01f1e64..c1286b64e 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -170,13 +170,10 @@ type conflictCheck struct { check func(*mdm.Policy) (match bool) } -// conflictBool builds a check for a *bool field on an arbitrary request -// conflictBool builds a conflictCheck for a boolean MDM key. -// If p is nil the returned check treats the field as matching; otherwise the -// check returns true only when the policy contains the key and its boolean -// conflictBool constructs a conflictCheck that verifies a boolean MDM policy key matches a desired value. -// If p is nil the produced check treats the field as matching by definition. Otherwise the check returns -// true only if the policy has the key and its boolean value equals *p. +// conflictBool builds a conflictCheck for a boolean MDM key. If p is nil +// the field is treated as matching (no override requested); otherwise the +// check returns true only when the policy contains the key and its +// boolean value equals *p. func conflictBool(key string, p *bool) conflictCheck { return conflictCheck{ key: key, @@ -190,12 +187,10 @@ func conflictBool(key string, p *bool) conflictCheck { } } -// conflictString builds a check for a string field. Empty string ("") -// conflictString returns a conflictCheck for the MDM string key identified by `key`. -// If `got` is empty the field is treated as unset and will not be considered a conflict. -// conflictString constructs a conflictCheck for a string policy key. -// The check treats an empty requested value as matching. Otherwise it -// succeeds only when the policy contains the key and its value equals got. +// conflictString builds a conflictCheck for a string MDM key. An empty +// `got` is treated as "field not set" (no override requested); otherwise +// the check returns true only when the policy contains the key and its +// value equals got. func conflictString(key, got string) conflictCheck { return conflictCheck{ key: key, @@ -209,9 +204,9 @@ func conflictString(key, got string) conflictCheck { } } -// conflictInt64 builds a conflictCheck that verifies an *int64 field against the MDM policy key. -// conflictInt64 builds a conflictCheck that validates an int64 MDM policy key. -// If p is nil, the check always matches; otherwise the policy must contain the key and its integer value must equal *p. +// conflictInt64 builds a conflictCheck for an integer MDM key. If p is +// nil the field is treated as matching; otherwise the check returns +// true only when the policy contains the key and its int value equals *p. func conflictInt64(key string, p *int64) conflictCheck { return conflictCheck{ key: key, @@ -225,15 +220,11 @@ func conflictInt64(key string, p *int64) conflictCheck { } } -// resolveConflicts walks a list of per-field checks against the active -// MDM policy and returns the names of keys whose requested value -// diverges from the policy-enforced value. Keys not managed by MDM are -// skipped silently (the gate fires only for keys the admin has actually -// resolveConflicts identifies MDM-managed policy keys whose values differ from the provided checks. -// If the policy is empty, it returns nil. Only keys present in the policy are considered; for each -// resolveConflicts evaluates each conflictCheck against the provided MDM policy and returns -// a slice of policy keys whose checks report a mismatch. If the policy is empty, it returns nil. -// Checks whose key is not present in the policy are skipped. +// resolveConflicts walks the per-field checks against the active MDM +// policy and returns the names of keys whose requested value diverges +// from the policy-enforced value. Keys not present in the policy are +// skipped silently (the gate fires only for keys the admin has +// actually pushed). Returns nil for an empty policy. func resolveConflicts(policy *mdm.Policy, checks []conflictCheck) []string { if policy.IsEmpty() { return nil @@ -255,21 +246,9 @@ func resolveConflicts(policy *mdm.Policy, checks []conflictCheck) []string { // value. A field set to the same value the policy already enforces is // treated as a no-op echo (the GUI tray sends a full Config snapshot on // every toggle, so most fields in a typical request match the policy -// exactly and must NOT be flagged as conflicts). -// -// The redacted PreSharedKey sentinel that GetConfig returns is -// recognised and treated as no-op so the UI can safely round-trip it -// mdmManagedFieldConflicts reports which MDM-managed policy keys would be violated by -// the provided SetConfigRequest. -// -// If msg is nil, it returns nil. The function treats the PSK redaction sentinel -// ("**********") as an intentional no-op (equivalent to field not set). Only keys -// present in the supplied policy are considered; returned slice contains the policy -// mdmManagedFieldConflicts reports MDM-managed policy keys that would conflict with a SetConfigRequest. -// -// If msg is nil, it returns nil. The pre-shared key redaction sentinel ("**********") is treated as unset -// so it does not produce a false conflict. The returned slice contains policy key names whose values in -// the request differ from the active policy; an empty or nil slice indicates no conflicts. +// exactly and must NOT be flagged as conflicts). The redacted PSK +// sentinel ("**********") returned by GetConfig is recognised and +// treated as no-op so the UI can safely round-trip it. func mdmManagedFieldConflicts(msg *proto.SetConfigRequest, policy *mdm.Policy) []string { if msg == nil { return nil @@ -297,21 +276,14 @@ func mdmManagedFieldConflicts(msg *proto.SetConfigRequest, policy *mdm.Policy) [ } // setConfigRequestHasConfigOverrides reports whether the SetConfigRequest -// carries ANY field that would actually mutate the persisted config. The -// CLI builds the request unconditionally on every `netbird up` (see -// setupSetConfigReq in cmd/up.go), so a plain `netbird up` results in a -// SetConfig call with every field at its zero value; the gate must skip -// such no-op invocations or it would always fire even when the user did -// setConfigRequestHasConfigOverrides reports whether msg contains any fields that would mutate -// persisted daemon configuration rather than being purely authentication-only. -// It returns false if msg is nil; otherwise it returns true when any configuration-related -// field is present (for example: management/admin URLs, pre-shared key, DNS/NAT lists and -// cleaning flags, interface/port/MTU settings, auto-connect and routing toggles, DNS/firewall/IPv6 -// controls, SSH-related flags, notification/lazy-connection options, or other persistent config -// setConfigRequestHasConfigOverrides reports whether msg contains any fields that would modify persisted daemon configuration. -// It returns false for a nil message. The check includes management/admin URLs, pre-shared key, DNS/NAT lists and cleanup flags, -// interface and WireGuard settings, MTU, auto-connect, routing, DNS/firewall/IPv6 controls, SSH-related flags, notification and -// lazy-connection options, and other persistent network/security fields. +// carries ANY field that would actually mutate the persisted config. +// The CLI builds a SetConfigRequest unconditionally on every +// `netbird up` (see setupSetConfigReq in cmd/up.go) — a plain +// `netbird up` produces a request with every field at its zero value; +// the gate must skip such no-op invocations or it would always fire +// even when the user did not pass any --flag. Returns false on a nil +// msg; true when any management/admin URL, PSK, DNS/NAT list+clean +// flag, interface/port/MTU, or any optional bool/duration field is set. func setConfigRequestHasConfigOverrides(msg *proto.SetConfigRequest) bool { if msg == nil { return false @@ -354,11 +326,7 @@ func setConfigRequestHasConfigOverrides(msg *proto.SetConfigRequest) bool { // (as opposed to pure-auth fields like setupKey, hostname, hint, // profileName, username). Used by the Login handler to decide whether // the `--disable-update-settings` / MDM gates must run: a re-auth that -// loginRequestHasConfigOverrides reports whether a LoginRequest includes any fields that would change persisted daemon configuration. -// It returns true when the request carries any configuration-related values (for example: management/admin URLs, pre-shared key, -// DNS or NAT lists/cleanup flags, interface or WireGuard port, connection and policy toggles, route/DNS/firewall/notification flags, -// loginRequestHasConfigOverrides reports whether the given LoginRequest contains any fields that would modify the daemon's persisted configuration. -// It returns true when the request sets any configuration-related fields (management/admin URLs, pre-shared key, DNS/NAT settings, Rosenpass options, interface/WireGuard settings, auto-connect, routing/SSH/firewall/DNS controls, notifications, lazy-connection, block-inbound, or similar persistent toggles); it returns false if msg is nil or contains only authentication/identity fields. +// changes nothing about the configuration is always allowed. func loginRequestHasConfigOverrides(msg *proto.LoginRequest) bool { if msg == nil { return false @@ -394,19 +362,9 @@ func loginRequestHasConfigOverrides(msg *proto.LoginRequest) bool { // MDM-enforced value is a no-op echo, not a conflict; only a divergent // value is flagged. PSK has two proto fields — PreSharedKey (deprecated) // and OptionalPreSharedKey (current); either route trips the gate if it -// diverges from the MDM-enforced PSK. The redaction sentinel is treated -// loginRequestMDMConflicts reports MDM-managed keys that conflict between a LoginRequest and an active MDM policy. -// -// It returns a slice of policy keys that are managed by the given policy and whose values in the request -// differ from the policy. If msg is nil or the policy has no managed keys, it returns nil. The function -// prefers OptionalPreSharedKey over the legacy PreSharedKey when both are present and treats the redaction -// loginRequestMDMConflicts reports MDM-managed configuration keys that would -// conflict between a LoginRequest and the active MDM policy. -// -// It returns a slice of policy key names whose requested values differ from the -// policy. If msg is nil it returns nil. For pre-shared keys, OptionalPreSharedKey -// takes precedence over the deprecated PreSharedKey; a value equal to -// preSharedKeyRedactedSentinel ("**********") is treated as unset. +// diverges from the MDM-enforced PSK. OptionalPreSharedKey wins when +// both are set; the redaction sentinel ("**********") is accepted as +// a no-op echo. func loginRequestMDMConflicts(msg *proto.LoginRequest, policy *mdm.Policy) []string { if msg == nil { return nil @@ -445,22 +403,8 @@ func loginRequestMDMConflicts(msg *proto.LoginRequest, policy *mdm.Policy) []str // fields tries to change an MDM-enforced value to something else, and // nil otherwise. The whole request is rejected on any conflict; non- // conflicting fields in the same request are not applied either (no -// rejectMDMManagedFieldConflicts returns a gRPC FailedPrecondition error when any MDM-managed fields conflict. -// If `conflicts` is empty this function returns nil. When conflicts exist it produces a FailedPrecondition status -// whose message lists the conflicting fields and attempts to attach a `proto.MDMManagedFieldsViolation` detail; -// rejectMDMManagedFieldConflicts rejects requests that attempt to modify fields managed by MDM. -// If `conflicts` is empty, it does nothing. Otherwise it logs a warning and returns a gRPC -// FailedPrecondition error whose message lists the conflicting keys and which carries a -// `proto.MDMManagedFieldsViolation` detail with the `Fields` set to `conflicts`. If attaching -// the detail fails, the base FailedPrecondition status is returned. -// -// Parameters: -// - policy: the active MDM policy (unused here, present for call-site symmetry). -// - conflicts: list of MDM-managed keys that the request attempted to modify. -// -// Returns: -// - a gRPC error indicating the request was rejected due to MDM-managed fields, or nil when -// there are no conflicts. +// partial apply). The `policy` parameter is accepted for call-site +// symmetry with the *Conflicts helpers and is currently unused. func rejectMDMManagedFieldConflicts(policy *mdm.Policy, conflicts []string) error { if len(conflicts) == 0 { return nil diff --git a/client/server/server.go b/client/server/server.go index d12eb0e8a..e00a93fff 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -362,17 +362,10 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques // optional fields and the "empty / clean" semantics for the two slice // fields (DNS labels, NAT external IPs). Extracted from SetConfig to // keep the handler's cognitive complexity below the SonarCube -// threshold; the body of this function is intentionally linear and -// setConfigInputFromRequest builds a profilemanager.ConfigInput from a SetConfigRequest proto. -// -// It translates each provided proto field into the corresponding ConfigInput field, -// preserving the request's semantics for "clean" vs explicit-empty slices/bytes and -// converting optional numeric fields into typed pointers where applicable. -// -// msg: the incoming SetConfigRequest whose fields are mapped into the returned ConfigInput. -// -// Returns the constructed ConfigInput and a non-nil error if the active profile file path -// cannot be determined. +// threshold; the body is intentionally linear because each proto +// field is its own optional case. Returns the resolved ConfigInput +// and a non-nil error only when the active profile file path cannot +// be determined. func setConfigInputFromRequest(msg *proto.SetConfigRequest) (profilemanager.ConfigInput, error) { var config profilemanager.ConfigInput diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index d1a098de6..5814ad9b4 100644 --- a/client/ui/client_ui.go +++ b/client/ui/client_ui.go @@ -65,14 +65,14 @@ const ( mdmFieldSuffix = " (MDM)" ) -// main is the entry point for the UI tray/client binary. -// -// It parses CLI flags, initializes logging, creates the Fyne application and tray icons, -// and constructs the service client (which may open a requested UI window). If a window-mode -// flag is set the Fyne event loop runs and main returns; otherwise it ensures only one -// main is the program entry point that initializes logging and the Fyne UI, enforces single-instance behavior, creates the service client, and starts the system tray. -// -// It parses CLI flags, configures logging, constructs the Fyne application and service client (optionally showing a requested UI window), monitors theme/settings changes, ensures only a single instance runs (signaling an existing instance to show its window when present), sets up signal handling and default fonts, and finally runs the system tray loop. +// main is the entry point for the UI tray/client binary. Parses CLI +// flags, initialises logging, builds the Fyne application and tray +// icons, and constructs the service client (which may open a +// requested UI window). When a window-mode flag is set the Fyne event +// loop runs and main returns; otherwise main enforces single-instance +// behaviour (signalling an existing instance to show its window when +// present), sets up signal handling + default fonts, and runs the +// system tray loop. func main() { flags := parseFlags() @@ -1705,11 +1705,12 @@ func (s *serviceClient) applyMDMLocks(managed []string) { } // preSharedKeyPlaceholder returns the hint string shown in the PSK -// Entry's placeholder slot. The placeholder is the only signal the user -// gets that a PSK is configured, because the entry's Text is forced to -// empty to keep the password reveal toggle from leaking the -// preSharedKeyPlaceholder returns the placeholder text for the pre-shared key entry based on the daemon config. -// It returns an empty string if no pre-shared key is present, `MDM-managed` if the key is enforced by MDM, and `configured` otherwise. +// Entry's placeholder slot. The placeholder is the only signal the +// user gets that a PSK is configured, because the entry's Text is +// forced to empty to keep the password reveal toggle from leaking +// the daemon-returned "**********" redaction sentinel. Returns "" if +// no PSK is present, "MDM-managed" if the key is enforced by MDM, +// and "configured" otherwise. func preSharedKeyPlaceholder(cfg *proto.GetConfigResponse) string { if cfg == nil || cfg.PreSharedKey == "" { return ""