Merge branch 'main' into fix/upload-server

management/cmd/root.go

@@ -54,6 +54,15 @@ func Execute() error {
 	return rootCmd.Execute()
 }
 
+// Customize hands the fully built root command to fn so an embedding binary
+// can extend or adjust the command tree — most commonly attaching its own
+// subcommands next to (or under) the built-in ones — before calling Execute.
+// The root command is constructed in this package's init, so Customize may be
+// called from the embedding binary's main at any point before Execute.
+func Customize(fn func(root *cobra.Command)) {
+	fn(rootCmd)
+}
+
 func init() {
 	mgmtCmd.Flags().IntVar(&mgmtPort, "port", 80, "server port to listen on (defaults to 443 if TLS is enabled, 80 otherwise")
 	mgmtCmd.Flags().BoolVar(&disableLegacyManagementPort, "disable-legacy-port", false, "disabling the old legacy port (33073)")

management/cmd/root_test.go

@@ -0,0 +1,42 @@
+package cmd
+
+import (
+	"testing"
+
+	"github.com/spf13/cobra"
+)
+
+// TestCustomize verifies an embedding binary can extend the command tree: a
+// top-level command attached through the hook, and a subcommand attached under
+// the built-in admin group, are both resolvable exactly as Execute would
+// resolve them.
+func TestCustomize(t *testing.T) {
+	topLevel := &cobra.Command{Use: "some-extra", RunE: func(*cobra.Command, []string) error { return nil }}
+	nested := &cobra.Command{Use: "cluster", RunE: func(*cobra.Command, []string) error { return nil }}
+
+	Customize(func(root *cobra.Command) {
+		root.AddCommand(topLevel)
+		for _, c := range root.Commands() {
+			if c.Name() == "admin" {
+				c.AddCommand(nested)
+				return
+			}
+		}
+		t.Fatal("admin command not found in the root tree")
+	})
+	t.Cleanup(func() {
+		rootCmd.RemoveCommand(topLevel)
+		for _, c := range rootCmd.Commands() {
+			if c.Name() == "admin" {
+				c.RemoveCommand(nested)
+			}
+		}
+	})
+
+	if found, _, err := rootCmd.Find([]string{"some-extra"}); err != nil || found != topLevel {
+		t.Fatalf("top-level command not resolvable: found=%v err=%v", found, err)
+	}
+	if found, _, err := rootCmd.Find([]string{"admin", "cluster"}); err != nil || found != nested {
+		t.Fatalf("nested admin subcommand not resolvable: found=%v err=%v", found, err)
+	}
+}

management/internals/modules/reverseproxy/domain/manager/domain_test.go

@@ -66,8 +66,8 @@ func TestExtractClusterFromFreeDomain(t *testing.T) {
 
 func TestExtractClusterFromCustomDomains(t *testing.T) {
 	customDomains := []*domain.Domain{
-		{Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io"},
-		{Domain: "proxy.corp.io", TargetCluster: "us1.proxy.netbird.io"},
+		{Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io", Validated: true},
+		{Domain: "proxy.corp.io", TargetCluster: "us1.proxy.netbird.io", Validated: true},
 	}
 
 	tests := []struct {
@@ -120,19 +120,49 @@ func TestExtractClusterFromCustomDomains(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			cluster, ok := extractClusterFromCustomDomains(tc.domain, customDomains)
-			assert.Equal(t, tc.wantOK, ok)
-			if ok {
-				assert.Equal(t, tc.wantVal, cluster)
+			cluster, match := extractClusterFromCustomDomains(tc.domain, customDomains)
+			if !tc.wantOK {
+				assert.Equal(t, customDomainNoMatch, match, "unrelated domain should not match any custom domain")
+				return
 			}
+			assert.Equal(t, customDomainValidated, match, "validated custom domain should resolve a cluster")
+			assert.Equal(t, tc.wantVal, cluster)
 		})
 	}
 }
 
+// An unvalidated row must never yield a cluster: the account has not shown it
+// controls the name, so no service may be bound to it.
+func TestExtractClusterFromCustomDomains_UnvalidatedDomainRefused(t *testing.T) {
+	customDomains := []*domain.Domain{
+		{Domain: "example.com", TargetCluster: "eu1.proxy.netbird.io", Validated: false},
+	}
+
+	for _, serviceDomain := range []string{"example.com", "app.example.com"} {
+		t.Run(serviceDomain, func(t *testing.T) {
+			cluster, match := extractClusterFromCustomDomains(serviceDomain, customDomains)
+			assert.Equal(t, customDomainUnvalidated, match, "unvalidated row must be reported as such")
+			assert.Empty(t, cluster, "unvalidated row must not resolve a cluster")
+		})
+	}
+}
+
+// A more specific unvalidated row must not shadow a validated parent domain.
+func TestExtractClusterFromCustomDomains_ValidatedParentWinsOverUnvalidatedChild(t *testing.T) {
+	customDomains := []*domain.Domain{
+		{Domain: "example.com", TargetCluster: "cluster-generic", Validated: true},
+		{Domain: "app.example.com", TargetCluster: "cluster-app", Validated: false},
+	}
+
+	cluster, match := extractClusterFromCustomDomains("app.example.com", customDomains)
+	assert.Equal(t, customDomainValidated, match)
+	assert.Equal(t, "cluster-generic", cluster, "validated parent domain should provide the cluster")
+}
+
 func TestExtractClusterFromCustomDomains_OverlappingDomains(t *testing.T) {
 	customDomains := []*domain.Domain{
-		{Domain: "example.com", TargetCluster: "cluster-generic"},
-		{Domain: "app.example.com", TargetCluster: "cluster-app"},
+		{Domain: "example.com", TargetCluster: "cluster-generic", Validated: true},
+		{Domain: "app.example.com", TargetCluster: "cluster-app", Validated: true},
 	}
 
 	tests := []struct {
@@ -164,8 +194,8 @@ func TestExtractClusterFromCustomDomains_OverlappingDomains(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			cluster, ok := extractClusterFromCustomDomains(tc.domain, customDomains)
-			assert.True(t, ok)
+			cluster, match := extractClusterFromCustomDomains(tc.domain, customDomains)
+			assert.Equal(t, customDomainValidated, match)
 			assert.Equal(t, tc.wantVal, cluster)
 		})
 	}

management/internals/modules/reverseproxy/domain/manager/manager.go

@@ -26,6 +26,7 @@ type store interface {
 	GetAgentNetworkSettings(ctx context.Context, lockStrength nbstore.LockingStrength, accountID string) (*agentnetworkTypes.Settings, error)
 
 	GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error)
+	GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error)
 	ListFreeDomains(ctx context.Context, accountID string) ([]string, error)
 	ListCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error)
 	CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error)
@@ -150,6 +151,10 @@ func (m Manager) CreateDomain(ctx context.Context, accountID, userID, domainName
 		return nil, fmt.Errorf("target cluster %s is not available", targetCluster)
 	}
 
+	if err := m.checkDomainAvailable(ctx, domainName); err != nil {
+		return nil, err
+	}
+
 	// Attempt an initial validation against the specified cluster only
 	var validated bool
 	if m.validator.IsValid(ctx, domainName, []string{targetCluster}) {
@@ -166,6 +171,23 @@ func (m Manager) CreateDomain(ctx context.Context, accountID, userID, domainName
 	return d, nil
 }
 
+// checkDomainAvailable reports whether the domain is free to claim. The unique
+// index on the column is the real guard; this turns the violation into a
+// conflict the caller can act on instead of a database error, and says nothing
+// about which account holds the domain.
+func (m Manager) checkDomainAvailable(ctx context.Context, domainName string) error {
+	_, err := m.store.GetCustomDomainByName(ctx, domainName)
+	if err == nil {
+		return status.Errorf(status.AlreadyExists, "domain %s is already registered", domainName)
+	}
+
+	if sErr, ok := status.FromError(err); ok && sErr.Type() == status.NotFound {
+		return nil
+	}
+
+	return fmt.Errorf("look up domain: %w", err)
+}
+
 func (m Manager) DeleteDomain(ctx context.Context, accountID, userID, domainID string) error {
 	ok, ctx, err := m.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, modules.Services, operations.Delete)
 	if err != nil {
@@ -203,7 +225,9 @@ func (m Manager) ValidateDomain(ctx context.Context, accountID, userID, domainID
 		log.WithFields(log.Fields{
 			"accountID": accountID,
 			"domainID":  domainID,
-		}).WithError(err).Error("validate domain")
+			"userID":    userID,
+		}).Error("validate domain: permission denied")
+		return
 	}
 
 	log.WithFields(log.Fields{
@@ -298,9 +322,12 @@ func (m Manager) DeriveClusterFromDomain(ctx context.Context, accountID, domain
 		return "", fmt.Errorf("list custom domains: %w", err)
 	}
 
-	targetCluster, valid := extractClusterFromCustomDomains(domain, customDomains)
-	if valid {
+	targetCluster, match := extractClusterFromCustomDomains(domain, customDomains)
+	switch match {
+	case customDomainValidated:
 		return targetCluster, nil
+	case customDomainUnvalidated:
+		return "", status.Errorf(status.PreconditionFailed, "domain %s is not validated", domain)
 	}
 
 	return "", fmt.Errorf("domain %s does not match any available proxy cluster", domain)
@@ -363,19 +390,46 @@ func (m Manager) reservedGatewayAddress(ctx context.Context, accountID string) (
 	return settings.ProxyAddress, nil
 }
 
-func extractClusterFromCustomDomains(serviceDomain string, customDomains []*domain.Domain) (string, bool) {
+// customDomainMatch describes how a service domain relates to the account's
+// custom domain rows.
+type customDomainMatch int
+
+const (
+	customDomainNoMatch customDomainMatch = iota
+	customDomainUnvalidated
+	customDomainValidated
+)
+
+// extractClusterFromCustomDomains finds the longest custom domain covering the
+// service domain and reports its target cluster. Only a validated row yields a
+// cluster: until the CNAME check has passed the account has not shown it
+// controls the name, so no traffic may be routed for it.
+func extractClusterFromCustomDomains(serviceDomain string, customDomains []*domain.Domain) (string, customDomainMatch) {
 	bestCluster := ""
 	bestLen := -1
+	matched := false
 	for _, cd := range customDomains {
 		if serviceDomain != cd.Domain && !strings.HasSuffix(serviceDomain, "."+cd.Domain) {
 			continue
 		}
+		matched = true
+		if !cd.Validated {
+			continue
+		}
 		if l := len(cd.Domain); l > bestLen {
 			bestLen = l
 			bestCluster = cd.TargetCluster
 		}
 	}
-	return bestCluster, bestLen >= 0
+
+	switch {
+	case bestLen >= 0:
+		return bestCluster, customDomainValidated
+	case matched:
+		return "", customDomainUnvalidated
+	default:
+		return "", customDomainNoMatch
+	}
 }
 
 // ExtractClusterFromFreeDomain extracts the cluster address from a free domain.

management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go

@@ -0,0 +1,326 @@
+package manager
+
+import (
+	"context"
+	"fmt"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/otel/metric/noop"
+
+	"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
+	proxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy/manager"
+	"github.com/netbirdio/netbird/management/server/activity"
+	"github.com/netbirdio/netbird/management/server/mock_server"
+	"github.com/netbirdio/netbird/management/server/permissions"
+	nbstore "github.com/netbirdio/netbird/management/server/store"
+	"github.com/netbirdio/netbird/management/server/types"
+	"github.com/netbirdio/netbird/shared/management/status"
+)
+
+const (
+	testCluster    = "eu.proxy.test"
+	accountA       = "account-a"
+	accountAUser   = "account-a-admin"
+	accountB       = "account-b"
+	accountBUser   = "account-b-admin"
+	accountAMember = "account-a-member"
+)
+
+// stubResolver answers CNAME lookups from a table the test controls, so a
+// domain can point at the cluster or nowhere without touching a real resolver.
+type stubResolver struct {
+	mu     sync.Mutex
+	cnames map[string]string
+}
+
+func (r *stubResolver) LookupCNAME(_ context.Context, host string) (string, error) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	cname, ok := r.cnames[host]
+	if !ok {
+		return "", fmt.Errorf("lookup %s: no such host", host)
+	}
+	return cname + ".", nil
+}
+
+func (r *stubResolver) set(host, cname string) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	r.cnames[host] = cname
+}
+
+type domainTestEnv struct {
+	manager  Manager
+	store    nbstore.Store
+	resolver *stubResolver
+}
+
+// setupDomainTest builds the domain manager on a real SQLite store with two
+// accounts and one active public proxy cluster.
+func setupDomainTest(t *testing.T) *domainTestEnv {
+	t.Helper()
+
+	ctx := context.Background()
+	testStore, cleanup, err := nbstore.NewTestStoreFromSQL(ctx, "", t.TempDir())
+	require.NoError(t, err)
+	t.Cleanup(cleanup)
+
+	for accountID, userID := range map[string]string{accountA: accountAUser, accountB: accountBUser} {
+		users := map[string]*types.User{
+			userID: {
+				Id:        userID,
+				AccountID: accountID,
+				Role:      types.UserRoleAdmin,
+			},
+		}
+		if accountID == accountA {
+			// A real member of the account whose role denies Services:Create, so
+			// permission denial is exercised as ok=false rather than as a lookup
+			// error for a user who is not in the account at all.
+			users[accountAMember] = &types.User{
+				Id:        accountAMember,
+				AccountID: accountID,
+				Role:      types.UserRoleUser,
+			}
+		}
+
+		require.NoError(t, testStore.SaveAccount(ctx, &types.Account{
+			Id:        accountID,
+			CreatedBy: userID,
+			Settings:  &types.Settings{},
+			Users:     users,
+		}))
+	}
+
+	proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter(""))
+	require.NoError(t, err)
+
+	_, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", testCluster, "127.0.0.1", nil, nil)
+	require.NoError(t, err)
+
+	resolver := &stubResolver{cnames: make(map[string]string)}
+
+	mgr := Manager{
+		store:              testStore,
+		proxyManager:       proxyMgr,
+		validator:          domain.Validator{Resolver: resolver},
+		permissionsManager: permissions.NewManager(testStore),
+		accountManager: &mock_server.MockAccountManager{
+			StoreEventFunc: func(context.Context, string, string, string, activity.ActivityDescriber, map[string]any) {},
+		},
+	}
+
+	return &domainTestEnv{manager: mgr, store: testStore, resolver: resolver}
+}
+
+// storedDomain reads a domain row back through the store so assertions are made
+// on what was persisted rather than on the value the manager returned.
+func storedDomain(t *testing.T, s nbstore.Store, accountID, domainName string) *domain.Domain {
+	t.Helper()
+
+	domains, err := s.ListCustomDomains(context.Background(), accountID)
+	require.NoError(t, err)
+	for _, d := range domains {
+		if d.Domain == domainName {
+			return d
+		}
+	}
+	return nil
+}
+
+// A domain whose CNAME check fails is stored unvalidated and must not resolve a
+// cluster, which is what service creation gates on.
+func TestCreateDomain_FailedLookupIsNotServable(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "apps.example.com", testCluster)
+	require.NoError(t, err)
+	assert.False(t, created.Validated, "a domain whose CNAME lookup fails must not be created validated")
+
+	stored := storedDomain(t, env.store, accountA, "apps.example.com")
+	require.NotNil(t, stored, "domain row should exist")
+	assert.False(t, stored.Validated, "persisted row must be unvalidated")
+
+	cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "apps.example.com")
+	require.Error(t, err, "an unvalidated domain must not resolve a cluster")
+	assert.Empty(t, cluster)
+	assert.Contains(t, err.Error(), "not validated", "error should tell the caller what to fix")
+
+	sErr, ok := status.FromError(err)
+	require.True(t, ok, "error should be a typed status error")
+	assert.Equal(t, status.PreconditionFailed, sErr.Type())
+
+	_, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "sub.apps.example.com")
+	assert.Error(t, err, "subdomains of an unvalidated custom domain are not servable either")
+}
+
+// A second account claiming a registered domain gets a clean conflict, not a
+// database error surfaced as a 500.
+func TestCreateDomain_DuplicateIsAConflict(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	_, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "shared.example.com", testCluster)
+	require.NoError(t, err)
+
+	_, err = env.manager.CreateDomain(ctx, accountB, accountBUser, "shared.example.com", testCluster)
+	require.Error(t, err)
+
+	sErr, ok := status.FromError(err)
+	require.True(t, ok, "conflict must be a typed status error, not a raw database error")
+	assert.Equal(t, status.AlreadyExists, sErr.Type(), "conflict should map to 409, not 500")
+	assert.NotContains(t, sErr.Message, accountA, "the response must not reveal the holding account")
+
+	assert.Nil(t, storedDomain(t, env.store, accountB, "shared.example.com"), "no row should be written on conflict")
+}
+
+// The same account re-adding one of its own domains is a conflict too.
+func TestCreateDomain_SameAccountDuplicateIsAConflict(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	_, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "dup.example.com", testCluster)
+	require.NoError(t, err)
+
+	_, err = env.manager.CreateDomain(ctx, accountA, accountAUser, "dup.example.com", testCluster)
+	require.Error(t, err)
+
+	sErr, ok := status.FromError(err)
+	require.True(t, ok)
+	assert.Equal(t, status.AlreadyExists, sErr.Type())
+}
+
+// The negative control: a validated domain still derives its cluster, for the
+// bare name and for subdomains, exactly as before.
+func TestCreateDomain_ValidatedDomainDerivesCluster(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+	env.resolver.set("validation.valid.example.com", testCluster)
+
+	created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "valid.example.com", testCluster)
+	require.NoError(t, err)
+	require.True(t, created.Validated, "a matching CNAME should validate on create")
+
+	cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "valid.example.com")
+	require.NoError(t, err)
+	assert.Equal(t, testCluster, cluster)
+
+	cluster, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "app.valid.example.com")
+	require.NoError(t, err)
+	assert.Equal(t, testCluster, cluster, "subdomains of a validated custom domain resolve too")
+}
+
+// Validating a domain flips the gate: the same lookup that failed before now
+// resolves a cluster.
+func TestValidateDomain_UnlocksClusterDerivation(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "later.example.com", testCluster)
+	require.NoError(t, err)
+	require.False(t, created.Validated)
+
+	_, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "later.example.com")
+	require.Error(t, err)
+
+	env.resolver.set("validation.later.example.com", testCluster)
+	env.manager.ValidateDomain(ctx, accountA, accountAUser, created.ID)
+
+	require.True(t, storedDomain(t, env.store, accountA, "later.example.com").Validated)
+
+	cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "later.example.com")
+	require.NoError(t, err)
+	assert.Equal(t, testCluster, cluster)
+}
+
+// Free cluster domains are unaffected by the custom domain gate.
+func TestDeriveClusterFromDomain_FreeDomainUnaffected(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	cluster, err := env.manager.DeriveClusterFromDomain(ctx, accountA, "myapp.abc123."+testCluster)
+	require.NoError(t, err)
+	assert.Equal(t, testCluster, cluster)
+}
+
+// The manager pre-check exists to turn a conflict into a 409, but the unique
+// index on the column is what actually guarantees the domain is claimed once.
+//
+// Two requests can clear the pre-check concurrently and race to the insert.
+// Inserting twice through the store reaches the same code path the loser of
+// that race takes, without the nondeterminism of driving it from goroutines,
+// and the loser must still see a conflict rather than an internal error.
+func TestStore_DuplicateDomainRejectedByIndexAsConflict(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	_, err := env.store.CreateCustomDomain(ctx, accountA, "indexed.example.com", testCluster, false)
+	require.NoError(t, err)
+
+	_, err = env.store.CreateCustomDomain(ctx, accountB, "indexed.example.com", testCluster, false)
+	require.Error(t, err, "the unique index must reject the same domain in a second account")
+
+	sErr, ok := status.FromError(err)
+	require.True(t, ok, "the losing insert must return a typed status error")
+	assert.Equal(t, status.AlreadyExists, sErr.Type(), "a lost race is a 409, not a 500")
+}
+
+// Validation is what decides whether a domain routes traffic, so a caller
+// without permission to it must not be able to flip the flag. The check logged
+// the denial and then carried on, which was inert while nothing read Validated
+// and is not once cluster derivation gates on it.
+func TestValidateDomain_PermissionDeniedDoesNotValidate(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "guarded.example.com", testCluster)
+	require.NoError(t, err)
+	require.False(t, created.Validated)
+
+	// The CNAME is in place, so the only thing standing between this caller and
+	// a validated domain is the permission check.
+	env.resolver.set("validation.guarded.example.com", testCluster)
+
+	env.manager.ValidateDomain(ctx, accountA, accountAMember, created.ID)
+
+	stored := storedDomain(t, env.store, accountA, "guarded.example.com")
+	require.NotNil(t, stored)
+	assert.False(t, stored.Validated, "a caller without permission must not validate the domain")
+
+	_, err = env.manager.DeriveClusterFromDomain(ctx, accountA, "guarded.example.com")
+	assert.Error(t, err, "the domain must still be unservable")
+}
+
+// Validation runs asynchronously, so it can finish after the domain was
+// deleted and then write a stale row back. gorm's Save falls back to an insert
+// when an update affects no rows, which would resurrect the domain as
+// validated; UpdateCustomDomain avoids that by selecting explicit columns.
+// This pins that behaviour, since dropping the Select would reintroduce it.
+func TestUpdateCustomDomain_DoesNotResurrectDeletedDomain(t *testing.T) {
+	ctx := context.Background()
+	env := setupDomainTest(t)
+
+	created, err := env.manager.CreateDomain(ctx, accountA, accountAUser, "racy.example.com", testCluster)
+	require.NoError(t, err)
+
+	stale := storedDomain(t, env.store, accountA, "racy.example.com")
+	require.NotNil(t, stale)
+
+	require.NoError(t, env.manager.DeleteDomain(ctx, accountA, accountAUser, created.ID))
+	require.Nil(t, storedDomain(t, env.store, accountA, "racy.example.com"), "the domain should be gone")
+
+	// What an in-flight validation would write once its CNAME check succeeded.
+	// The write has to succeed for the assertion below to mean anything: a
+	// rejected write would leave the domain absent for the wrong reason.
+	stale.Validated = true
+	_, err = env.store.UpdateCustomDomain(ctx, accountA, stale)
+	require.NoError(t, err, "the update itself must succeed, so absence is not just a failed write")
+
+	assert.Nil(t, storedDomain(t, env.store, accountA, "racy.example.com"),
+		"a late validation write must not recreate a deleted domain")
+}

management/internals/modules/reverseproxy/domain/manager/manager_test.go

@@ -184,6 +184,10 @@ func (s *stubStore) GetCustomDomain(context.Context, string, string) (*domain.Do
 	panic("not used in allow-list tests")
 }
 
+func (s *stubStore) GetCustomDomainByName(context.Context, string) (*domain.Domain, error) {
+	panic("not used in allow-list tests")
+}
+
 func (s *stubStore) ListFreeDomains(context.Context, string) ([]string, error) {
 	panic("not used in allow-list tests")
 }

management/internals/modules/reverseproxy/service/manager/domain_validation_test.go

@@ -0,0 +1,127 @@
+package manager
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/otel/metric/noop"
+
+	domainmanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain/manager"
+	proxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy/manager"
+	rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
+	"github.com/netbirdio/netbird/management/server/activity"
+	"github.com/netbirdio/netbird/management/server/mock_server"
+	"github.com/netbirdio/netbird/management/server/permissions"
+	"github.com/netbirdio/netbird/management/server/store"
+	"github.com/netbirdio/netbird/shared/management/status"
+)
+
+const validationTestCluster = "eu.proxy.test"
+
+// withRealDomainManager swaps the stub cluster deriver for the real domain
+// manager backed by the same store, so service creation is gated by the actual
+// domain rows rather than by a test double that always agrees.
+func withRealDomainManager(t *testing.T, mgr *Manager, testStore store.Store) {
+	t.Helper()
+
+	ctx := context.Background()
+	proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter(""))
+	require.NoError(t, err)
+
+	_, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", validationTestCluster, "127.0.0.1", nil, nil)
+	require.NoError(t, err)
+
+	accountMgr := &mock_server.MockAccountManager{
+		StoreEventFunc: func(context.Context, string, string, string, activity.ActivityDescriber, map[string]any) {},
+	}
+	mgr.clusterDeriver = domainmanager.NewManager(testStore, proxyMgr, permissions.NewManager(testStore), accountMgr)
+}
+
+func newTestService(domain string) *rpservice.Service {
+	return &rpservice.Service{
+		Name:    "test-service",
+		Domain:  domain,
+		Enabled: true,
+		Mode:    rpservice.ModeHTTP,
+		Targets: []*rpservice.Target{{
+			Host:       "10.0.0.1",
+			Port:       8080,
+			Protocol:   "http",
+			TargetId:   testPeerID,
+			TargetType: "peer",
+			Enabled:    true,
+		}},
+	}
+}
+
+// A service must not bind to a domain the account has not validated, and
+// nothing may be persisted for the attempt.
+func TestCreateService_RefusesUnvalidatedDomain(t *testing.T) {
+	ctx := context.Background()
+	mgr, testStore := setupIntegrationTest(t)
+	withRealDomainManager(t, mgr, testStore)
+
+	_, err := testStore.CreateCustomDomain(ctx, testAccountID, "unproven.example.com", validationTestCluster, false)
+	require.NoError(t, err)
+
+	_, err = mgr.CreateService(ctx, testAccountID, testUserID, newTestService("unproven.example.com"))
+	require.Error(t, err, "an unvalidated domain must not bind a service")
+	assert.Contains(t, err.Error(), "not validated", "the API error should name the actual problem")
+
+	sErr, ok := status.FromError(err)
+	require.True(t, ok, "error should be a typed status error")
+	assert.Equal(t, status.PreconditionFailed, sErr.Type())
+
+	services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID)
+	require.NoError(t, err)
+	assert.Empty(t, services, "no service row should be written for a refused domain")
+}
+
+// The negative control: a validated domain still binds a service and derives
+// its cluster exactly as before.
+func TestCreateService_ValidatedDomainBindsService(t *testing.T) {
+	ctx := context.Background()
+	mgr, testStore := setupIntegrationTest(t)
+	withRealDomainManager(t, mgr, testStore)
+
+	_, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true)
+	require.NoError(t, err)
+
+	created, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.proven.example.com"))
+	require.NoError(t, err)
+	assert.Equal(t, validationTestCluster, created.ProxyCluster, "service should bind to the domain's target cluster")
+
+	services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, services, 1, "the service should be persisted")
+	assert.Equal(t, "app.proven.example.com", services[0].Domain)
+}
+
+// An update must not be a way around the creation gate: moving a live service
+// onto an unvalidated domain has to fail rather than silently keep the old
+// cluster and start serving the new hostname.
+func TestUpdateService_RefusesMoveToUnvalidatedDomain(t *testing.T) {
+	ctx := context.Background()
+	mgr, testStore := setupIntegrationTest(t)
+	withRealDomainManager(t, mgr, testStore)
+
+	_, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true)
+	require.NoError(t, err)
+	_, err = testStore.CreateCustomDomain(ctx, testAccountID, "unproven.example.com", validationTestCluster, false)
+	require.NoError(t, err)
+
+	created, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.proven.example.com"))
+	require.NoError(t, err)
+
+	moved := *created
+	moved.Domain = "app.unproven.example.com"
+	_, err = mgr.UpdateService(ctx, testAccountID, testUserID, &moved)
+	require.Error(t, err, "moving to an unvalidated domain must fail")
+	assert.Contains(t, err.Error(), "not validated")
+
+	stored, err := testStore.GetServiceByID(ctx, store.LockingStrengthNone, testAccountID, created.ID)
+	require.NoError(t, err)
+	assert.Equal(t, "app.proven.example.com", stored.Domain, "the service must keep its original domain")
+}

management/internals/modules/reverseproxy/service/manager/manager.go

@@ -606,16 +606,19 @@ func (m *Manager) resolveEffectiveCluster(ctx context.Context, accountID string,
 		return existing.ProxyCluster, nil
 	}
 
-	if m.clusterDeriver != nil {
-		derived, err := m.clusterDeriver.DeriveClusterFromDomain(ctx, accountID, svc.Domain)
-		if err != nil {
-			log.WithError(err).Warnf("could not derive cluster from domain %s", svc.Domain)
-		} else {
-			return derived, nil
-		}
+	if m.clusterDeriver == nil {
+		return existing.ProxyCluster, nil
 	}
 
-	return existing.ProxyCluster, nil
+	// Falling back to the old cluster here would let an update move a service
+	// onto a domain the account has not validated, bypassing the check that
+	// creation makes.
+	derived, err := m.clusterDeriver.DeriveClusterFromDomain(ctx, accountID, svc.Domain)
+	if err != nil {
+		return "", status.Errorf(status.PreconditionFailed, "could not derive cluster from domain %s: %v", svc.Domain, err)
+	}
+
+	return derived, nil
 }
 
 func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo, customPorts *bool, effectiveCluster string) error {

management/server/account.go

@@ -719,8 +719,10 @@ func (am *DefaultAccountManager) schedulePeerLoginExpiration(ctx context.Context
 		log.WithContext(ctx).Tracef("peer login expiration job for account %s is already scheduled", accountID)
 		return
 	}
+	// The job outlives the request that arms it, so it must not inherit the request's cancellation.
+	jobCtx := context.WithoutCancel(ctx)
 	if nextRun, ok := am.getNextPeerExpiration(ctx, accountID); ok {
-		go am.peerLoginExpiry.Schedule(ctx, nextRun, accountID, am.peerLoginExpirationJob(ctx, accountID))
+		go am.peerLoginExpiry.Schedule(jobCtx, nextRun, accountID, am.peerLoginExpirationJob(jobCtx, accountID))
 	}
 }
 
@@ -752,8 +754,9 @@ func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context
 // checkAndSchedulePeerInactivityExpiration periodically checks for inactive peers to end their sessions
 func (am *DefaultAccountManager) checkAndSchedulePeerInactivityExpiration(ctx context.Context, accountID string) {
 	am.peerInactivityExpiry.Cancel(ctx, []string{accountID})
+	jobCtx := context.WithoutCancel(ctx)
 	if nextRun, ok := am.getNextInactivePeerExpiration(ctx, accountID); ok {
-		go am.peerInactivityExpiry.Schedule(ctx, nextRun, accountID, am.peerInactivityExpirationJob(ctx, accountID))
+		go am.peerInactivityExpiry.Schedule(jobCtx, nextRun, accountID, am.peerInactivityExpirationJob(jobCtx, accountID))
 	}
 }
 

management/server/account_test.go

@@ -1920,6 +1920,154 @@ func TestDefaultAccountManager_MarkPeerConnected_PeerLoginExpiration(t *testing.
 	}
 }
 
+func TestDefaultAccountManager_SchedulePeerLoginExpiration_IncludesOfflinePeers(t *testing.T) {
+	manager, updateManager, err := createManager(t)
+	require.NoError(t, err, "unable to create account manager")
+
+	accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: userID})
+	require.NoError(t, err, "unable to create an account")
+
+	connectedKey, offlineKey := addExpiringPeers(t, manager)
+	_, err = manager.UpdateAccountSettings(context.Background(), accountID, userID, &types.Settings{
+		PeerLoginExpiration:        time.Hour,
+		PeerLoginExpirationEnabled: true,
+		Extra:                      &types.ExtraSettings{},
+	})
+	require.NoError(t, err, "expecting to update account settings successfully but got error")
+	manager.peerLoginExpiry.CancelAll(context.Background())
+
+	// The connected peer logged in just now, so a job computed from connected peers alone
+	// would be armed for an hour. The offline peer's login expires in two seconds; a
+	// reconnect of that peer must not have to wait for the connected peer's tick.
+	now := time.Now().UTC()
+	setPeerLogin(t, manager, accountID, connectedKey, true, now)
+	setPeerLogin(t, manager, accountID, offlineKey, false, now.Add(-time.Hour+2*time.Second))
+
+	offlinePeer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, offlineKey)
+	require.NoError(t, err)
+	updateManager.CreateChannel(context.Background(), offlinePeer.ID)
+
+	manager.peerLoginExpiry = NewDefaultScheduler()
+	t.Cleanup(func() { manager.peerLoginExpiry.CancelAll(context.Background()) })
+	manager.schedulePeerLoginExpiration(context.Background(), accountID)
+
+	// The flag is committed per peer before the disconnect fans out, so wait for both.
+	require.Eventually(t, func() bool {
+		peer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, offlineKey)
+		return err == nil && peer.Status.LoginExpired && !updateManager.HasChannel(offlinePeer.ID)
+	}, 10*time.Second, 100*time.Millisecond, "offline peer should be expired and disconnected at its own deadline")
+
+	connectedPeer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, connectedKey)
+	require.NoError(t, err)
+	assert.False(t, connectedPeer.Status.LoginExpired, "connected peer with a fresh login must not expire")
+}
+
+func TestDefaultAccountManager_SchedulePeerLoginExpiration_DetachesRequestContext(t *testing.T) {
+	manager, _, err := createManager(t)
+	require.NoError(t, err, "unable to create account manager")
+
+	accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: userID})
+	require.NoError(t, err, "unable to create an account")
+	connectedKey, _ := addExpiringPeers(t, manager)
+	setPeerLogin(t, manager, accountID, connectedKey, true, time.Now().UTC())
+
+	scheduled := make(chan context.Context, 1)
+	manager.peerLoginExpiry = &MockScheduler{
+		IsSchedulerRunningFunc: func(string) bool { return false },
+		ScheduleFunc: func(ctx context.Context, _ time.Duration, _ string, _ func() (time.Duration, bool)) {
+			scheduled <- ctx
+		},
+	}
+
+	requestCtx, cancel := context.WithCancel(context.Background())
+	manager.schedulePeerLoginExpiration(requestCtx, accountID)
+	cancel()
+
+	select {
+	case jobCtx := <-scheduled:
+		assert.NoError(t, jobCtx.Err(), "the expiration job must outlive the request that armed it")
+	case <-time.After(time.Second):
+		t.Fatal("timeout while waiting for the job to be scheduled")
+	}
+}
+
+func TestDefaultAccountManager_ExpireAndUpdatePeers_SkipsPeerThatLoggedInAgain(t *testing.T) {
+	manager, updateManager, err := createManager(t)
+	require.NoError(t, err, "unable to create account manager")
+
+	accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: userID})
+	require.NoError(t, err, "unable to create an account")
+
+	reloggedKey, staleKey := addExpiringPeers(t, manager)
+	_, err = manager.UpdateAccountSettings(context.Background(), accountID, userID, &types.Settings{
+		PeerLoginExpiration:        time.Hour,
+		PeerLoginExpirationEnabled: true,
+		Extra:                      &types.ExtraSettings{},
+	})
+	require.NoError(t, err, "expecting to update account settings successfully but got error")
+	manager.peerLoginExpiry.CancelAll(context.Background())
+
+	expiredLogin := time.Now().UTC().Add(-2 * time.Hour)
+	setPeerLogin(t, manager, accountID, reloggedKey, true, expiredLogin)
+	setPeerLogin(t, manager, accountID, staleKey, true, expiredLogin)
+
+	expiredPeers, err := manager.getExpiredPeers(context.Background(), accountID)
+	require.NoError(t, err)
+	require.Len(t, expiredPeers, 2, "both peers should be due for expiration")
+
+	// The job holds the candidate list while one peer completes a fresh login, which
+	// moves its deadline into the future and must win over the stale candidate entry.
+	setPeerLogin(t, manager, accountID, reloggedKey, true, time.Now().UTC())
+
+	reloggedPeer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, reloggedKey)
+	require.NoError(t, err)
+	stalePeer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, staleKey)
+	require.NoError(t, err)
+	updateManager.CreateChannel(context.Background(), reloggedPeer.ID)
+	updateManager.CreateChannel(context.Background(), stalePeer.ID)
+
+	err = manager.expireAndUpdatePeers(context.Background(), accountID, expiredPeers, peerExpirationSessionExpired)
+	require.NoError(t, err)
+
+	reloggedPeer, err = manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, reloggedKey)
+	require.NoError(t, err)
+	assert.False(t, reloggedPeer.Status.LoginExpired, "a peer that logged in again must not be flagged from the stale candidate list")
+	assert.True(t, reloggedPeer.Status.Connected, "the re-logged peer must keep its connected status")
+	assert.True(t, updateManager.HasChannel(reloggedPeer.ID), "the re-logged peer's update channel must stay open")
+
+	stalePeer, err = manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, staleKey)
+	require.NoError(t, err)
+	assert.True(t, stalePeer.Status.LoginExpired, "a peer that is still due must be flagged")
+	assert.False(t, updateManager.HasChannel(stalePeer.ID), "the expired peer's update channel must be closed")
+}
+
+// addExpiringPeers registers two SSO peers with login expiration enabled and returns their public keys.
+func addExpiringPeers(t *testing.T, manager *DefaultAccountManager) (string, string) {
+	t.Helper()
+	keys := make([]string, 0, 2)
+	for _, hostname := range []string{"connected-peer", "offline-peer"} {
+		key, err := wgtypes.GenerateKey()
+		require.NoError(t, err, "unable to generate WireGuard key")
+		_, _, _, _, err = manager.AddPeer(context.Background(), "", "", userID, &nbpeer.Peer{
+			Key:                    key.PublicKey().String(),
+			Meta:                   nbpeer.PeerSystemMeta{Hostname: hostname},
+			LoginExpirationEnabled: true,
+		}, false)
+		require.NoError(t, err, "unable to add peer")
+		keys = append(keys, key.PublicKey().String())
+	}
+	return keys[0], keys[1]
+}
+
+func setPeerLogin(t *testing.T, manager *DefaultAccountManager, accountID, peerKey string, connected bool, lastLogin time.Time) {
+	t.Helper()
+	peer, err := manager.Store.GetPeerByPeerPubKey(context.Background(), store.LockingStrengthNone, peerKey)
+	require.NoError(t, err)
+	peer.Status.Connected = connected
+	peer.LastLogin = &lastLogin
+	require.NoError(t, manager.Store.SavePeer(context.Background(), accountID, peer))
+}
+
 func TestDefaultAccountManager_MarkPeerDisconnected_SchedulesInactivityExpiration(t *testing.T) {
 	manager, _, err := createManager(t)
 	require.NoError(t, err, "unable to create account manager")
@@ -2702,7 +2850,7 @@ func TestAccount_GetNextPeerExpiration(t *testing.T) {
 			expectedNextExpiration: time.Duration(0),
 		},
 		{
-			name: "No connected peers, no expiration",
+			name: "Offline peer with expiration, return expiration",
 			peers: map[string]*nbpeer.Peer{
 				"peer-1": {
 					Status: &nbpeer.PeerStatus{
@@ -2721,8 +2869,33 @@ func TestAccount_GetNextPeerExpiration(t *testing.T) {
 			},
 			expiration:             time.Second,
 			expirationEnabled:      false,
-			expectedNextRun:        false,
-			expectedNextExpiration: time.Duration(0),
+			expectedNextRun:        true,
+			expectedNextExpiration: time.Second,
+		},
+		{
+			name: "Offline peer with the earliest deadline defines the next run",
+			peers: map[string]*nbpeer.Peer{
+				"peer-1": {
+					Status: &nbpeer.PeerStatus{
+						Connected: true,
+					},
+					LoginExpirationEnabled: true,
+					LastLogin:              util.ToPtr(time.Now().UTC()),
+					UserID:                 userID,
+				},
+				"peer-2": {
+					Status: &nbpeer.PeerStatus{
+						Connected: false,
+					},
+					LoginExpirationEnabled: true,
+					LastLogin:              util.ToPtr(time.Now().UTC().Add(-50 * time.Minute)),
+					UserID:                 userID,
+				},
+			},
+			expiration:             time.Hour,
+			expirationEnabled:      true,
+			expectedNextRun:        true,
+			expectedNextExpiration: 10 * time.Minute,
 		},
 		{
 			name: "Connected peers with disabled expiration, no expiration",

management/server/peer.go

@@ -1494,9 +1494,12 @@ func checkAuth(ctx context.Context, loginUserID string, peer *nbpeer.Peer) error
 
 func peerLoginExpired(ctx context.Context, peer *nbpeer.Peer, settings *types.Settings) bool {
 	expired, expiresIn := peer.LoginExpired(settings.PeerLoginExpiration)
-	expired = settings.PeerLoginExpirationEnabled && expired
-	if expired || peer.Status.LoginExpired {
-		log.WithContext(ctx).Debugf("peer's %s login expired %v ago", peer.ID, expiresIn)
+	if settings.PeerLoginExpirationEnabled && expired {
+		log.WithContext(ctx).Debugf("peer's %s login expired %v ago", peer.ID, -expiresIn)
+		return true
+	}
+	if peer.Status.LoginExpired {
+		log.WithContext(ctx).Debugf("peer's %s login is marked as expired", peer.ID)
 		return true
 	}
 	return false
@@ -1643,7 +1646,9 @@ func (am *DefaultAccountManager) UpdateAccountPeer(ctx context.Context, accountI
 
 // getNextPeerExpiration returns the minimum duration in which the next peer of the account will expire if it was found.
 // If there is no peer that expires this function returns false and a duration of 0.
-// This function only considers peers that haven't been expired yet and that are connected.
+// This function only considers peers that haven't been expired yet. Offline peers count too:
+// a running job is never re-armed on connect, so a peer that reconnects with an old login
+// must already be part of the scheduled run.
 func (am *DefaultAccountManager) getNextPeerExpiration(ctx context.Context, accountID string) (time.Duration, bool) {
 	peersWithExpiry, err := am.Store.GetAccountPeersWithExpiration(ctx, store.LockingStrengthNone, accountID)
 	if err != nil {
@@ -1663,8 +1668,7 @@ func (am *DefaultAccountManager) getNextPeerExpiration(ctx context.Context, acco
 
 	var nextExpiry *time.Duration
 	for _, peer := range peersWithExpiry {
-		// consider only connected peers because others will require login on connecting to the management server
-		if peer.Status.LoginExpired || !peer.Status.Connected {
+		if peer.Status.LoginExpired {
 			continue
 		}
 		_, duration := peer.LoginExpired(settings.PeerLoginExpiration)

management/server/scheduler.go

@@ -117,6 +117,7 @@ func (wm *DefaultScheduler) Schedule(ctx context.Context, in time.Duration, ID s
 	}
 
 	ticker := time.NewTicker(in)
+	period := in
 
 	wm.jobs[ID] = cancel
 	log.WithContext(ctx).Debugf("scheduled a job %s to run in %s. There are %d total jobs scheduled.", ID, in.String(), len(wm.jobs))
@@ -136,14 +137,18 @@ func (wm *DefaultScheduler) Schedule(ctx context.Context, in time.Duration, ID s
 				if !reschedule {
 					wm.mu.Lock()
 					defer wm.mu.Unlock()
-					delete(wm.jobs, ID)
+					// A Cancel during job() may have registered a replacement under this ID.
+					if current, ok := wm.jobs[ID]; ok && current == cancel {
+						delete(wm.jobs, ID)
+					}
 					log.WithContext(ctx).Debugf("job %s is not scheduled to run again", ID)
 					ticker.Stop()
 					return
 				}
 				// we need this comparison to avoid resetting the ticker with the same duration and missing the current elapsesed time
-				if runIn != in {
+				if runIn != period {
 					ticker.Reset(runIn)
+					period = runIn
 				}
 			case <-cancel:
 				log.WithContext(ctx).Debugf("job %s was canceled, stopping timer", ID)

management/server/scheduler_test.go

@@ -6,10 +6,12 @@ import (
 	"math/rand"
 	"runtime"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestScheduler_Performance(t *testing.T) {
@@ -150,3 +152,90 @@ func TestScheduler_Schedule(t *testing.T) {
 	scheduler.cancel(context.Background(), jobID)
 
 }
+
+func TestScheduler_Schedule_ResetsTickerAfterReturningInitialInterval(t *testing.T) {
+	jobID := "test-scheduler-job-2"
+	scheduler := NewDefaultScheduler()
+	defer scheduler.Cancel(context.Background(), []string{jobID})
+
+	initial := 30 * time.Millisecond
+	stretched := 400 * time.Millisecond
+	runs := make(chan time.Time, 3)
+	count := 0
+	// The first run stretches the period; the second returns the initial interval again,
+	// which must shrink the period back instead of keeping the stretched one.
+	job := func() (nextRunIn time.Duration, reschedule bool) {
+		count++
+		runs <- time.Now()
+		switch count {
+		case 1:
+			return stretched, true
+		case 2:
+			return initial, true
+		default:
+			return 0, false
+		}
+	}
+	scheduler.Schedule(context.Background(), initial, jobID, job)
+
+	var stamps []time.Time
+	for len(stamps) < 3 {
+		select {
+		case ts := <-runs:
+			stamps = append(stamps, ts)
+		case <-time.After(2 * time.Second):
+			t.Fatalf("timed out after %d runs", len(stamps))
+		}
+	}
+	assert.Less(t, stamps[2].Sub(stamps[1]), stretched/2, "returning the initial interval must reset the stretched ticker")
+}
+
+func TestScheduler_Schedule_StaleCompletionKeepsReplacement(t *testing.T) {
+	jobID := "test-scheduler-job-3"
+	scheduler := NewDefaultScheduler()
+	defer scheduler.Cancel(context.Background(), []string{jobID})
+
+	started := make(chan struct{})
+	release := make(chan struct{})
+	staleJob := func() (nextRunIn time.Duration, reschedule bool) {
+		close(started)
+		<-release
+		return 0, false
+	}
+	scheduler.Schedule(context.Background(), 10*time.Millisecond, jobID, staleJob)
+	select {
+	case <-started:
+	case <-time.After(time.Second):
+		t.Fatal("timed out waiting for the first job to start")
+	}
+
+	// Cancel the job while it is still executing and register a replacement under the
+	// same ID, as the expiration paths do on a settings change.
+	scheduler.Cancel(context.Background(), []string{jobID})
+	var replacementRuns atomic.Int32
+	scheduler.Schedule(context.Background(), 20*time.Millisecond, jobID, func() (nextRunIn time.Duration, reschedule bool) {
+		replacementRuns.Add(1)
+		return 20 * time.Millisecond, true
+	})
+	require.True(t, scheduler.IsSchedulerRunning(jobID), "replacement must be registered")
+
+	// The stale job now completes without rescheduling; its cleanup must leave the
+	// replacement's entry in place.
+	close(release)
+	assert.Never(t, func() bool { return !scheduler.IsSchedulerRunning(jobID) }, 200*time.Millisecond, 10*time.Millisecond,
+		"stale completion must not drop the replacement job")
+
+	var duplicateRuns atomic.Int32
+	scheduler.Schedule(context.Background(), 10*time.Millisecond, jobID, func() (nextRunIn time.Duration, reschedule bool) {
+		duplicateRuns.Add(1)
+		return 10 * time.Millisecond, true
+	})
+	assert.Never(t, func() bool { return duplicateRuns.Load() > 0 }, 100*time.Millisecond, 10*time.Millisecond,
+		"a duplicate schedule must be refused while the replacement is registered")
+
+	scheduler.Cancel(context.Background(), []string{jobID})
+	assert.False(t, scheduler.IsSchedulerRunning(jobID), "cancel must find and remove the replacement")
+	runsAfterCancel := replacementRuns.Load()
+	assert.Never(t, func() bool { return replacementRuns.Load() > runsAfterCancel+1 }, 150*time.Millisecond, 10*time.Millisecond,
+		"the replacement must stop after cancel")
+}

management/server/store/sql_store.go

@@ -5686,6 +5686,23 @@ func (s *SqlStore) ListCustomDomains(ctx context.Context, accountID string) ([]*
 	return domains, nil
 }
 
+// GetCustomDomainByName returns the custom domain row holding the given name,
+// regardless of which account owns it.
+func (s *SqlStore) GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) {
+	customDomain := &domain.Domain{}
+	result := s.db.Take(customDomain, "domain = ?", domainName)
+	if result.Error != nil {
+		if errors.Is(result.Error, gorm.ErrRecordNotFound) {
+			return nil, status.Errorf(status.NotFound, "custom domain %s not found", domainName)
+		}
+
+		log.WithContext(ctx).Errorf("failed to get custom domain by name from store: %v", result.Error)
+		return nil, status.Errorf(status.Internal, "failed to get custom domain from store")
+	}
+
+	return customDomain, nil
+}
+
 func (s *SqlStore) CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error) {
 	newDomain := &domain.Domain{
 		ID:            xid.New().String(), // Generate our own ID because gorm doesn't always configure the database to handle this for us.
@@ -5697,6 +5714,18 @@ func (s *SqlStore) CreateCustomDomain(ctx context.Context, accountID string, dom
 	}
 	result := s.db.Create(newDomain)
 	if result.Error != nil {
+		// The unique index is the last guard when two requests clear the
+		// manager's availability check at the same time. The one that loses the
+		// insert is a conflict, not an internal failure.
+		var count int64
+		if err := s.db.Model(&domain.Domain{}).Where("domain = ?", domainName).Count(&count).Error; err == nil && count > 0 {
+			// The insert error is logged even on this path: the name being taken
+			// is what the caller has to act on, but if the insert also failed for
+			// an unrelated reason the operator still needs to see it.
+			log.WithContext(ctx).Warnf("create reverse proxy custom domain %s rejected, name already registered: %v", domainName, result.Error)
+			return nil, status.Errorf(status.AlreadyExists, "domain %s is already registered", domainName)
+		}
+
 		log.WithContext(ctx).Errorf("failed to create reverse proxy custom domain to store: %v", result.Error)
 		return nil, status.Errorf(status.Internal, "failed to create reverse proxy custom domain to store")
 	}

management/server/store/store.go

@@ -302,6 +302,7 @@ type Store interface {
 	GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error)
 	ListFreeDomains(ctx context.Context, accountID string) ([]string, error)
 	ListCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error)
+	GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error)
 	CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error)
 	UpdateCustomDomain(ctx context.Context, accountID string, d *domain.Domain) (*domain.Domain, error)
 	DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error

management/server/store/store_mock.go

@@ -1941,6 +1941,21 @@ func (mr *MockStoreMockRecorder) GetCustomDomain(ctx, accountID, domainID any) *
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCustomDomain", reflect.TypeOf((*MockStore)(nil).GetCustomDomain), ctx, accountID, domainID)
 }
 
+// GetCustomDomainByName mocks base method.
+func (m *MockStore) GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "GetCustomDomainByName", ctx, domainName)
+	ret0, _ := ret[0].(*domain.Domain)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// GetCustomDomainByName indicates an expected call of GetCustomDomainByName.
+func (mr *MockStoreMockRecorder) GetCustomDomainByName(ctx, domainName any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCustomDomainByName", reflect.TypeOf((*MockStore)(nil).GetCustomDomainByName), ctx, domainName)
+}
+
 // GetCustomDomainsCounts mocks base method.
 func (m *MockStore) GetCustomDomainsCounts(ctx context.Context) (int64, int64, error) {
 	m.ctrl.T.Helper()

management/server/types/account.go

@@ -404,7 +404,7 @@ func (a *Account) GetExpiredPeers() []*nbpeer.Peer {
 
 // GetNextPeerExpiration returns the minimum duration in which the next peer of the account will expire if it was found.
 // If there is no peer that expires this function returns false and a duration of 0.
-// This function only considers peers that haven't been expired yet and that are connected.
+// This function only considers peers that haven't been expired yet, whether connected or not.
 func (a *Account) GetNextPeerExpiration() (time.Duration, bool) {
 	peersWithExpiry := a.GetPeersWithExpiration()
 	if len(peersWithExpiry) == 0 {
@@ -412,8 +412,7 @@ func (a *Account) GetNextPeerExpiration() (time.Duration, bool) {
 	}
 	var nextExpiry *time.Duration
 	for _, peer := range peersWithExpiry {
-		// consider only connected peers because others will require login on connecting to the management server
-		if peer.Status.LoginExpired || !peer.Status.Connected {
+		if peer.Status.LoginExpired {
 			continue
 		}
 		_, duration := peer.LoginExpired(a.Settings.PeerLoginExpiration)

management/server/user.go

@@ -1177,28 +1177,35 @@ func (am *DefaultAccountManager) expireAndUpdatePeers(ctx context.Context, accou
 	dnsDomain := am.networkMapController.GetDNSDomain(settings)
 
 	var peerIDs []string
-	for _, peer := range peers {
+	defer func() {
+		if len(peerIDs) == 0 {
+			return
+		}
+		// this will trigger peer disconnect from the management service
+		log.Debugf("Expiring %d peers for account %s", len(peerIDs), accountID)
+		am.networkMapController.DisconnectPeers(ctx, accountID, peerIDs)
+	}()
+	for _, candidate := range peers {
 		// nolint:staticcheck
-		ctx = context.WithValue(ctx, nbcontext.PeerIDKey, peer.Key)
+		peerCtx := context.WithValue(ctx, nbcontext.PeerIDKey, candidate.Key)
 
-		if peer.UserID == "" {
+		if candidate.UserID == "" {
 			// we do not want to expire peers that are added via setup key
 			continue
 		}
 
-		if peer.Status.LoginExpired {
+		peer, err := am.expirePeerIfStillDue(peerCtx, accountID, candidate.ID, settings, reason)
+		if err != nil {
+			return err
+		}
+		if peer == nil {
 			continue
 		}
 		peerIDs = append(peerIDs, peer.ID)
-		peer.MarkLoginExpired(true)
-
-		if err := am.Store.SavePeerStatus(ctx, accountID, peer.ID, *peer.Status); err != nil {
-			return err
-		}
 		meta := peer.EventMeta(dnsDomain)
 		meta["reason"] = string(reason)
 		am.StoreEvent(
-			ctx,
+			peerCtx,
 			peer.UserID, peer.ID, accountID,
 			activity.PeerLoginExpired, meta,
 		)
@@ -1215,15 +1222,53 @@ func (am *DefaultAccountManager) expireAndUpdatePeers(ctx context.Context, accou
 	if err != nil {
 		return fmt.Errorf("notify network map controller of peer update: %w", err)
 	}
-
-	if len(peerIDs) != 0 {
-		// this will trigger peer disconnect from the management service
-		log.Debugf("Expiring %d peers for account %s", len(peerIDs), accountID)
-		am.networkMapController.DisconnectPeers(ctx, accountID, peerIDs)
-	}
 	return nil
 }
 
+// expirePeerIfStillDue flags the peer as login-expired and returns its fresh copy, or nil
+// when it no longer qualifies. The candidate list is read without a lock, so a login that
+// landed in between would otherwise be overwritten with a stale expired status.
+func (am *DefaultAccountManager) expirePeerIfStillDue(ctx context.Context, accountID, peerID string, settings *types.Settings, reason peerExpirationReason) (*nbpeer.Peer, error) {
+	var expired *nbpeer.Peer
+	err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
+		peer, err := transaction.GetPeerByID(ctx, store.LockingStrengthUpdate, accountID, peerID)
+		if err != nil {
+			if s, ok := status.FromError(err); ok && s.Type() == status.NotFound {
+				return nil
+			}
+			return err
+		}
+		if peer.Status.LoginExpired || !peerExpirationDue(peer, settings, reason) {
+			return nil
+		}
+		peer.MarkLoginExpired(true)
+		if err := transaction.SavePeerStatus(ctx, accountID, peer.ID, *peer.Status); err != nil {
+			return err
+		}
+		expired = peer
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return expired, nil
+}
+
+// peerExpirationDue re-evaluates a time-based expiry against the peer's current state.
+// Administrative reasons expire the peer unconditionally.
+func peerExpirationDue(peer *nbpeer.Peer, settings *types.Settings, reason peerExpirationReason) bool {
+	switch reason {
+	case peerExpirationSessionExpired:
+		expired, _ := peer.LoginExpired(settings.PeerLoginExpiration)
+		return settings.PeerLoginExpirationEnabled && expired
+	case peerExpirationInactivity:
+		expired, _ := peer.SessionExpired(settings.PeerInactivityExpiration)
+		return settings.PeerInactivityExpirationEnabled && expired
+	default:
+		return true
+	}
+}
+
 func (am *DefaultAccountManager) deleteUserFromIDP(ctx context.Context, targetUserID, accountID string) error {
 	if am.userDeleteFromIDPEnabled {
 		log.WithContext(ctx).Debugf("user %s deleted from IdP", targetUserID)