fix(ldap): do not disable every user when a sync returns no identifiers (#744)

disable_missing tested absence against the raw sync result without
checking that the search returned anything usable, so a search that
succeeds and yields nothing looked like "every user has been removed".

Connection and search errors were already safe, since synchronizeLdapUsers
returns before the disable phase. The gap is the successful-but-empty
case: a base_dn or sync_filter that stops matching, an unpopulated
replica, a field_map user_identifier naming an attribute the server does
not return, or a bind account that lost read access to the user subtree.
LDAP gives nothing to tell those apart from a directory that is genuinely
empty; they all answer success with zero entries.

Acting on it is not a database flag. TopicUserDisabled removes each
user's peers from the WireGuard device, the successful search means no
error is logged, and every message on the path was Debug while log_level
defaults to info, so the whole event was silent. It also repeats every
sync interval.

Refuse to disable anyone when no usable identifier came back, logging the
provider, entry count and identifier field. The guard counts identifiers,
not entries, so it covers the field_map case too. The per-user disable
line moves from Debug to Warn so a mass disable is audible even where the
guard does not fire.

The cost is that a directory intentionally emptied of users now disables
nobody. That is documented, along with the workaround: leave one account
matching sync_filter and everyone else is disabled as before.

Signed-off-by: clark-ja <37738506+clark-ja@users.noreply.github.com>

docs/documentation/configuration/overview.md

@@ -827,6 +827,14 @@ Below are the properties for each LDAP provider entry inside `auth.ldap`:
 #### `disable_missing`
 - **Default:** `false`
 - **Description:** If `true`, any user **not** found in LDAP (during sync) is disabled in WireGuard Portal.
+- **Important**: As a safeguard, a synchronization that returns no usable user identifiers at all disables
+  nobody. LDAP cannot distinguish a directory that legitimately holds no matching users from a wrong `base_dn`,
+  a `sync_filter` whose group was renamed, a replica that is reachable but not yet populated, or a bind account
+  that has lost read access — all of them answer with success and zero entries. Disabling every user on that
+  signal would lock administrators out of a portal they may only reach over the VPN, so the sync logs an error
+  and skips the disable step instead.
+  If you intend to empty a group, keep one account matching `sync_filter`: the remaining users are then disabled
+  normally, and the account doubles as a canary, since its disappearance means the query itself is broken.
 
 #### `auto_re_enable`
 - **Default:** `false`

internal/app/users/ldap_sync.go

@@ -193,6 +193,42 @@ func (m Manager) disableMissingLdapUsers(
 	rawUsers []internal.RawLdapUser,
 	fields *config.LdapFields,
 ) error {
+	// Collect the identifiers the directory actually returned. A hard LDAP
+	// failure is already handled by the caller, but a search that *succeeds*
+	// and yields nothing usable is indistinguishable here from "every user was
+	// removed from the directory". In practice that means a wrong base DN, a
+	// sync_filter that no longer matches, a replica that is up but not yet
+	// populated, a field map pointing at an attribute the server does not
+	// return, or a bind account that has lost read access to the user subtree.
+	// LDAP offers nothing to tell those apart: a search that matches nobody and
+	// one the client is not allowed to answer both come back as success with
+	// zero entries, and a directory server may reply that way for a base DN
+	// that does not exist rather than disclose it to an unprivileged client.
+	//
+	// Acting on it disables every LDAP-sourced user at once, and through
+	// TopicUserDisabled that removes each of their peers from the WireGuard
+	// device rather than only flagging them. The search succeeded, so no error
+	// is logged, and every message on this path is Debug: at the default log
+	// level the whole thing is silent. It also repeats every sync interval.
+	// Refuse instead. The cost is that a directory intentionally emptied of all
+	// users disables nobody; leaving a single account in scope restores the
+	// normal behaviour for everyone else.
+	ldapUserIds := make(map[domain.UserIdentifier]struct{}, len(rawUsers))
+	for _, rawUser := range rawUsers {
+		if userId := ldapUserIdentifier(rawUser, fields.UserIdentifier); userId != "" {
+			ldapUserIds[userId] = struct{}{}
+		}
+	}
+	if len(ldapUserIds) == 0 {
+		slog.Error("refusing to disable missing LDAP users: directory returned no usable user identifiers",
+			"provider", providerName,
+			"raw-entries", len(rawUsers),
+			"identifier-field", fields.UserIdentifier,
+			"hint", "check base_dn, sync_filter and the bind account's read permissions; "+
+				"if the directory is intentionally empty, keep one account matching sync_filter")
+		return nil
+	}
+
 	allUsers, err := m.users.GetAllUsers(ctx)
 	if err != nil {
 		return err
@@ -215,20 +251,14 @@ func (m Manager) disableMissingLdapUsers(
 			continue // skip sync for this user
 		}
 
-		existsInLDAP := false
-		for _, rawUser := range rawUsers {
-			userId := ldapUserIdentifier(rawUser, fields.UserIdentifier)
-			if user.Identifier == userId {
-				existsInLDAP = true
-				break
-			}
-		}
-
-		if existsInLDAP {
+		if _, existsInLDAP := ldapUserIds[user.Identifier]; existsInLDAP {
 			continue
 		}
 
-		slog.Debug("user is missing in ldap provider, disabling", "user", user.Identifier, "provider", providerName)
+		// Warn, not Debug: this removes the user's peers from the device, and at
+		// the default log level a Debug line would make a mass disable silent.
+		slog.Warn("user is missing in ldap provider, disabling",
+			"user", user.Identifier, "provider", providerName)
 
 		now := time.Now()
 		user.Disabled = &now

internal/app/users/ldap_sync_disable_missing_test.go

@@ -0,0 +1,159 @@
+package users
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/h44z/wg-portal/internal"
+	"github.com/h44z/wg-portal/internal/config"
+	"github.com/h44z/wg-portal/internal/domain"
+)
+
+// fakeUserRepo is a minimal UserDatabaseRepo that records which users were
+// saved, so a test can assert whether the disable path ran at all.
+type fakeUserRepo struct {
+	users []domain.User
+	saved map[domain.UserIdentifier]*domain.User
+}
+
+func (f *fakeUserRepo) GetAllUsers(_ context.Context) ([]domain.User, error) {
+	return f.users, nil
+}
+
+func (f *fakeUserRepo) SaveUser(
+	_ context.Context,
+	id domain.UserIdentifier,
+	updateFunc func(u *domain.User) (*domain.User, error),
+) error {
+	existing := &domain.User{Identifier: id}
+	updated, err := updateFunc(existing)
+	if err != nil {
+		return err
+	}
+	if f.saved == nil {
+		f.saved = make(map[domain.UserIdentifier]*domain.User)
+	}
+	f.saved[id] = updated
+	return nil
+}
+
+func (f *fakeUserRepo) GetUser(_ context.Context, _ domain.UserIdentifier) (*domain.User, error) {
+	return nil, domain.ErrNotFound
+}
+func (f *fakeUserRepo) GetUserByEmail(_ context.Context, _ string) (*domain.User, error) {
+	return nil, domain.ErrNotFound
+}
+func (f *fakeUserRepo) GetUserByWebAuthnCredential(_ context.Context, _ string) (*domain.User, error) {
+	return nil, domain.ErrNotFound
+}
+func (f *fakeUserRepo) FindUsers(_ context.Context, _ string) ([]domain.User, error) {
+	return nil, nil
+}
+func (f *fakeUserRepo) DeleteUser(_ context.Context, _ domain.UserIdentifier) error { return nil }
+
+type fakeBus struct {
+	published []string
+}
+
+func (f *fakeBus) Publish(topic string, _ ...any) {
+	f.published = append(f.published, topic)
+}
+
+// ldapUser builds an existing wg-portal user that is sourced from LDAP and is
+// therefore a candidate for being disabled when missing.
+func ldapUser(id string) domain.User {
+	return domain.User{
+		Identifier: domain.UserIdentifier(id),
+		Authentications: []domain.UserAuthentication{
+			{Source: domain.UserSourceLdap, ProviderName: "testprovider"},
+		},
+	}
+}
+
+func newTestManager(repo *fakeUserRepo, bus *fakeBus) Manager {
+	return Manager{cfg: &config.Config{}, bus: bus, users: repo}
+}
+
+// A directory that answers successfully but returns nothing usable must not be
+// read as "every user was removed". Disabling every LDAP-sourced user at once
+// takes down the VPN an admin would need in order to fix the directory.
+func TestDisableMissingLdapUsers_RefusesOnEmptyDirectoryResult(t *testing.T) {
+	repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}}
+	bus := &fakeBus{}
+	m := newTestManager(repo, bus)
+
+	err := m.disableMissingLdapUsers(context.Background(), "testprovider",
+		[]internal.RawLdapUser{}, makeTestLdapFields())
+
+	require.NoError(t, err)
+	assert.Empty(t, repo.saved, "no user may be disabled when LDAP returned no entries")
+	assert.Empty(t, bus.published, "no disable events may be published")
+}
+
+// Same protection when the search returns entries but the configured identifier
+// attribute is absent from all of them -- a misconfigured field map yields no
+// usable identifiers, which would otherwise mark every user as missing.
+func TestDisableMissingLdapUsers_RefusesWhenNoUsableIdentifiers(t *testing.T) {
+	repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}}
+	bus := &fakeBus{}
+	m := newTestManager(repo, bus)
+
+	// Entries exist, but none carry the "uid" attribute the field map expects.
+	raw := []internal.RawLdapUser{
+		{"dn": "cn=alice,dc=example,dc=com", "cn": "alice"},
+		{"dn": "cn=bob,dc=example,dc=com", "cn": "bob"},
+	}
+
+	err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields())
+
+	require.NoError(t, err)
+	assert.Empty(t, repo.saved, "no user may be disabled when no identifiers could be extracted")
+	assert.Empty(t, bus.published)
+}
+
+// The guard must not suppress the feature: with a genuine directory result,
+// users absent from it are still disabled.
+func TestDisableMissingLdapUsers_DisablesGenuinelyMissingUsers(t *testing.T) {
+	repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}}
+	bus := &fakeBus{}
+	m := newTestManager(repo, bus)
+
+	// Only alice is still present in the directory.
+	raw := []internal.RawLdapUser{{"dn": "uid=alice,dc=example,dc=com", "uid": "alice"}}
+
+	err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields())
+
+	require.NoError(t, err)
+	assert.NotContains(t, repo.saved, domain.UserIdentifier("alice"), "alice is present in LDAP")
+	require.Contains(t, repo.saved, domain.UserIdentifier("bob"), "bob is missing and must be disabled")
+	assert.Equal(t, domain.DisabledReasonLdapMissing, repo.saved["bob"].DisabledReason)
+	assert.NotNil(t, repo.saved["bob"].Disabled)
+	assert.Len(t, bus.published, 1)
+}
+
+// Users the operator has pinned with PersistLocalChanges, and users already
+// disabled, must be left alone even when genuinely missing.
+func TestDisableMissingLdapUsers_SkipsPinnedAndAlreadyDisabled(t *testing.T) {
+	pinned := ldapUser("pinned")
+	pinned.PersistLocalChanges = true
+
+	alreadyDisabled := ldapUser("gone")
+	disabledAt := time.Now()
+	alreadyDisabled.Disabled = &disabledAt
+
+	repo := &fakeUserRepo{users: []domain.User{pinned, alreadyDisabled}}
+	bus := &fakeBus{}
+	m := newTestManager(repo, bus)
+
+	raw := []internal.RawLdapUser{{"dn": "uid=other,dc=example,dc=com", "uid": "other"}}
+
+	err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields())
+
+	require.NoError(t, err)
+	assert.Empty(t, repo.saved)
+	assert.Empty(t, bus.published)
+}