Merge remote-tracking branch 'origin/main' into fix_update_settings_value_aware

client/mdm/conflicts.go

@@ -1,6 +1,10 @@
 package mdm
 
-import "net/url"
+import (
+	"net/url"
+
+	"github.com/netbirdio/netbird/util"
+)
 
 // PreSharedKeyRedactedSentinel is the redaction mask returned in place of a
 // real pre-shared key; an incoming value equal to it is a round-trip echo,
@@ -44,8 +48,9 @@ func ConflictStringPtr(key string, p *string) ConflictCheck {
 	}
 }
 
-// ConflictURL builds a ConflictCheck for a URL-typed MDM key; both sides are
-// normalized via CanonicalURL before comparison.
+// ConflictURL builds a ConflictCheck for a URL-typed MDM key. The two sides are
+// compared as the endpoints they address, not as strings: see
+// util.SameServiceURL.
 func ConflictURL(key, got string) ConflictCheck {
 	return ConflictCheck{
 		Key: key,
@@ -54,7 +59,7 @@ func ConflictURL(key, got string) ConflictCheck {
 				return true
 			}
 			want, ok := pol.GetString(key)
-			return ok && CanonicalURL(want) == CanonicalURL(got)
+			return ok && util.SameServiceURLStrings(want, got)
 		},
 	}
 }

client/mdm/conflicts_test.go

@@ -0,0 +1,40 @@
+package mdm
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The same spellings, through the conflict check that decides whether a request
+// is refused. An enforced URL restated in another spelling addresses the very
+// server the policy names, so it must not be reported as a conflict.
+func TestConflictURLComparesEndpoints(t *testing.T) {
+	policy := NewPolicy(map[string]any{KeyManagementURL: "https://mgmt.example.com"})
+	require.True(t, policy.HasKey(KeyManagementURL))
+
+	for _, restated := range []string{
+		"https://mgmt.example.com",
+		"https://mgmt.example.com:443",
+		"https://mgmt.example.com/",
+		"https://MGMT.example.com",
+		"https://mgmt.example.com:0443",
+	} {
+		conflicts := ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, restated)})
+		assert.Empty(t, conflicts, "%q is the enforced endpoint written differently", restated)
+	}
+
+	for _, diverging := range []string{
+		"https://other.example.com",
+		"http://mgmt.example.com",
+		"https://mgmt.example.com:8443",
+		"https://mgmt.example.com/other",
+	} {
+		conflicts := ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, diverging)})
+		assert.Equal(t, []string{KeyManagementURL}, conflicts, "%q addresses another endpoint", diverging)
+	}
+
+	// An unset field is not a request to change anything.
+	assert.Empty(t, ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, "")}))
+}

client/mdm/policy.go

@@ -235,6 +235,8 @@ func (p *Policy) GetBool(key string) (bool, bool) {
 		return t != 0, true
 	case int64:
 		return t != 0, true
+	case float64:
+		return t != 0, true
 	}
 	return false, false
 }

client/mdm/policy_test.go

@@ -96,7 +96,8 @@ func TestPolicy_GetBool(t *testing.T) {
 		{"int64 nonzero", int64(2), true, true},
 		{"int64 zero", int64(0), false, true},
 		{"string garbage", "maybe", false, false},
-		{"float unsupported", 1.0, false, false},
+		{"float nonzero", 1.0, true, true},
+		{"float zero", 0.0, false, true},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
@@ -156,6 +157,20 @@ func TestPolicy_GetStringSlice(t *testing.T) {
 	})
 }
 
+// encoding/json decodes every JSON number into float64, so the mobile
+// loaders never see int.
+func TestJSONLoader_BoolFromNumber(t *testing.T) {
+	p := NewJSONLoader(func() string { return `{"blockInbound":1,"disableProfiles":0}` }).Load()
+
+	got, ok := p.GetBool(KeyBlockInbound)
+	assert.True(t, ok)
+	assert.True(t, got)
+
+	got, ok = p.GetBool(KeyDisableProfiles)
+	assert.True(t, ok)
+	assert.False(t, got)
+}
+
 func TestLoader_NilFetcherReturnsEmpty(t *testing.T) {
 	// Loader.Load with no fetcher (desktop construction) must degrade
 	// gracefully and never return nil; on linux loadPlatform is a stub

util/serviceurl.go

@@ -0,0 +1,69 @@
+package util
+
+import (
+	"net/url"
+	"strconv"
+	"strings"
+)
+
+// SameServiceURL reports whether two service URLs address the same endpoint.
+// One endpoint can be written several ways, and every spelling below reaches
+// the same server, so none of them is a divergence from another:
+//
+//	an implicit default port    https://mgmt.example.com   :443
+//	a zero-padded port          https://mgmt.example.com:0443
+//	a different host case       https://MGMT.example.com
+//	a trailing slash            https://mgmt.example.com/
+//
+// A path is otherwise part of the identity: https://mgmt.example.com and
+// https://mgmt.example.com/other are two endpoints.
+//
+// It lives here rather than next to any one caller because several of them
+// compare the same kind of URL — an MDM-enforced management URL against a
+// requested one, a stored profile URL against a command-line one — and every
+// copy of these rules that drifts turns an equivalent URL into a refused
+// request.
+func SameServiceURL(a, b *url.URL) bool {
+	if a == nil || b == nil {
+		return a == b
+	}
+
+	return strings.EqualFold(a.Hostname(), b.Hostname()) &&
+		strings.EqualFold(a.Scheme, b.Scheme) &&
+		ServiceURLPort(a) == ServiceURLPort(b) &&
+		strings.TrimSuffix(a.Path, "/") == strings.TrimSuffix(b.Path, "/")
+}
+
+// SameServiceURLStrings is SameServiceURL for unparsed input. Input that does
+// not parse falls back to string equality, which is the strictest thing left
+// to do with it.
+func SameServiceURLStrings(a, b string) bool {
+	ua, errA := url.ParseRequestURI(a)
+	ub, errB := url.ParseRequestURI(b)
+	if errA != nil || errB != nil {
+		return a == b
+	}
+
+	return SameServiceURL(ua, ub)
+}
+
+// ServiceURLPort is the port a URL addresses: the one it carries, normalized
+// numerically so ":0443" and ":443" are one port, or the scheme's default.
+func ServiceURLPort(u *url.URL) string {
+	port := u.Port()
+	if port == "" {
+		switch strings.ToLower(u.Scheme) {
+		case "https":
+			return "443"
+		case "http":
+			return "80"
+		default:
+			return ""
+		}
+	}
+
+	if n, err := strconv.Atoi(port); err == nil {
+		return strconv.Itoa(n)
+	}
+	return port
+}

util/serviceurl_test.go

@@ -0,0 +1,73 @@
+package util
+
+import (
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSameServiceURLSpellings(t *testing.T) {
+	tests := []struct {
+		a, b string
+		want bool
+	}{
+		// One endpoint, written several ways.
+		{a: "https://mgmt.example.com", b: "https://mgmt.example.com:443", want: true},
+		{a: "https://mgmt.example.com", b: "https://mgmt.example.com/", want: true},
+		{a: "https://mgmt.example.com/", b: "https://mgmt.example.com:443/", want: true},
+		{a: "https://MGMT.example.com", b: "https://mgmt.example.com", want: true},
+		{a: "https://mgmt.example.com:0443", b: "https://mgmt.example.com:443", want: true},
+		{a: "http://mgmt.example.com", b: "http://mgmt.example.com:80", want: true},
+		{a: "HTTPS://mgmt.example.com", b: "https://mgmt.example.com", want: true},
+
+		// Different endpoints.
+		{a: "https://mgmt.example.com", b: "http://mgmt.example.com", want: false},
+		{a: "https://mgmt.example.com", b: "https://mgmt.example.com:8443", want: false},
+		{a: "https://mgmt.example.com", b: "https://other.example.com", want: false},
+		{a: "https://mgmt.example.com", b: "https://mgmt.example.com/other", want: false},
+
+		// Unparseable input falls back to string equality.
+		{a: "mgmt.example.com", b: "mgmt.example.com", want: true},
+		{a: "mgmt.example.com", b: "https://mgmt.example.com", want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.a+" vs "+tt.b, func(t *testing.T) {
+			assert.Equal(t, tt.want, SameServiceURLStrings(tt.a, tt.b))
+			assert.Equal(t, tt.want, SameServiceURLStrings(tt.b, tt.a), "the comparison must be symmetric")
+		})
+	}
+}
+
+// The parsed form is the primitive the string form delegates to, so it must
+// answer the same for a spelling that only the parser can tell apart.
+func TestSameServiceURLParsed(t *testing.T) {
+	parse := func(raw string) *url.URL {
+		t.Helper()
+		u, err := url.ParseRequestURI(raw)
+		require.NoError(t, err)
+		return u
+	}
+
+	assert.True(t, SameServiceURL(parse("https://mgmt.example.com:0443/"), parse("https://MGMT.example.com")))
+	assert.False(t, SameServiceURL(parse("https://mgmt.example.com"), parse("https://mgmt.example.com:8443")))
+
+	assert.True(t, SameServiceURL(nil, nil), "two absent URLs are the same absence")
+	assert.False(t, SameServiceURL(nil, parse("https://mgmt.example.com")))
+}
+
+func TestServiceURLPort(t *testing.T) {
+	parse := func(raw string) *url.URL {
+		t.Helper()
+		u, err := url.ParseRequestURI(raw)
+		require.NoError(t, err)
+		return u
+	}
+
+	assert.Equal(t, "443", ServiceURLPort(parse("https://mgmt.example.com")))
+	assert.Equal(t, "80", ServiceURLPort(parse("http://mgmt.example.com")))
+	assert.Equal(t, "443", ServiceURLPort(parse("https://mgmt.example.com:0443")))
+	assert.Equal(t, "8443", ServiceURLPort(parse("https://mgmt.example.com:8443")))
+}