fix(ldap): do not create interfaces from interface_filter entries (#746)

updateInterfaceLdapFilters saved the matched users with SaveInterface,
which creates the interface when it is missing.

On first start that panics. The sync runs immediately (main.go:85) and
the importer later (main.go:116), so the sync creates a stub row for
every interface_filter key, and the importer, which snapshotted the
interface list before its device round-trips, then fails with "interface
already exists". The window is GetInterfaces plus GetPeers, so a
directory on localhost loses the race and a slower one hides it.

The stub rows are wrong anyway. They have no backend, so a typo in an
interface_filter key quietly created an interface attached to no
controller.

Look the interface up and skip with a warning when it is absent. The
filter is applied on the next sync once the importer has created it. A
lookup error that is not ErrNotFound also skips.

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

internal/app/users/ldap_interface_filter_test.go

@@ -0,0 +1,125 @@
+package users
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/h44z/wg-portal/internal/config"
+	"github.com/h44z/wg-portal/internal/domain"
+)
+
+// fakeInterfaceRepo records what the sync asked of the interface store.
+type fakeInterfaceRepo struct {
+	existing map[domain.InterfaceIdentifier]*domain.Interface
+	getErr   error
+	saved    map[domain.InterfaceIdentifier]*domain.Interface
+}
+
+func (f *fakeInterfaceRepo) GetInterface(
+	_ context.Context,
+	id domain.InterfaceIdentifier,
+) (*domain.Interface, error) {
+	if f.getErr != nil {
+		return nil, f.getErr
+	}
+	if iface, ok := f.existing[id]; ok {
+		return iface, nil
+	}
+	return nil, domain.ErrNotFound
+}
+
+func (f *fakeInterfaceRepo) SaveInterface(
+	_ context.Context,
+	id domain.InterfaceIdentifier,
+	updateFunc func(i *domain.Interface) (*domain.Interface, error),
+) error {
+	existing, ok := f.existing[id]
+	if !ok {
+		// Mirror the real repo, which creates the row when it is missing. The
+		// point of the guard under test is that we never get here for an
+		// interface that does not exist.
+		existing = &domain.Interface{Identifier: id}
+	}
+	updated, err := updateFunc(existing)
+	if err != nil {
+		return err
+	}
+	if f.saved == nil {
+		f.saved = make(map[domain.InterfaceIdentifier]*domain.Interface)
+	}
+	f.saved[id] = updated
+	return nil
+}
+
+func newFilterTestManager(repo *fakeInterfaceRepo) Manager {
+	return Manager{cfg: &config.Config{}, interfaces: repo}
+}
+
+// An interface_filter entry naming an interface wg-portal does not know about
+// must not bring that interface into existence. Creating it races the startup
+// importer, which then fails with "interface already exists", and the row it
+// leaves behind has no backend.
+func TestApplyInterfaceLdapFilterDoesNotCreateInterfaces(t *testing.T) {
+	repo := &fakeInterfaceRepo{existing: map[domain.InterfaceIdentifier]*domain.Interface{}}
+	m := newFilterTestManager(repo)
+
+	m.applyInterfaceLdapFilter(context.Background(), "wg0", "ipa",
+		[]domain.UserIdentifier{"alice"})
+
+	assert.Empty(t, repo.saved, "an unknown interface must not be created or written to")
+}
+
+func TestApplyInterfaceLdapFilterUpdatesExistingInterfaces(t *testing.T) {
+	repo := &fakeInterfaceRepo{existing: map[domain.InterfaceIdentifier]*domain.Interface{
+		"wg0": {Identifier: "wg0", Backend: "opnsense1"},
+	}}
+	m := newFilterTestManager(repo)
+
+	m.applyInterfaceLdapFilter(context.Background(), "wg0", "ipa",
+		[]domain.UserIdentifier{"alice", "bob"})
+
+	require.Contains(t, repo.saved, domain.InterfaceIdentifier("wg0"))
+	assert.Equal(t, []domain.UserIdentifier{"alice", "bob"},
+		repo.saved["wg0"].LdapAllowedUsers["ipa"])
+	assert.Equal(t, domain.InterfaceBackend("opnsense1"), repo.saved["wg0"].Backend,
+		"the existing interface must be updated in place, not replaced")
+}
+
+// Several providers may filter the same interface; one must not clobber another.
+func TestApplyInterfaceLdapFilterKeepsOtherProviders(t *testing.T) {
+	repo := &fakeInterfaceRepo{existing: map[domain.InterfaceIdentifier]*domain.Interface{
+		"wg0": {
+			Identifier: "wg0",
+			LdapAllowedUsers: map[string][]domain.UserIdentifier{
+				"other": {"carol"},
+			},
+		},
+	}}
+	m := newFilterTestManager(repo)
+
+	m.applyInterfaceLdapFilter(context.Background(), "wg0", "ipa",
+		[]domain.UserIdentifier{"alice"})
+
+	saved := repo.saved["wg0"].LdapAllowedUsers
+	assert.Equal(t, []domain.UserIdentifier{"alice"}, saved["ipa"])
+	assert.Equal(t, []domain.UserIdentifier{"carol"}, saved["other"],
+		"another provider's allowed users must be left alone")
+}
+
+// A lookup failure that is not "not found" must also not fall through to a
+// write, or a transient database error would silently create the interface.
+func TestApplyInterfaceLdapFilterSkipsOnLookupError(t *testing.T) {
+	repo := &fakeInterfaceRepo{
+		existing: map[domain.InterfaceIdentifier]*domain.Interface{},
+		getErr:   assert.AnError,
+	}
+	m := newFilterTestManager(repo)
+
+	m.applyInterfaceLdapFilter(context.Background(), "wg0", "ipa",
+		[]domain.UserIdentifier{"alice"})
+
+	assert.Empty(t, repo.saved, "a lookup error must not result in a write")
+}

internal/app/users/ldap_sync.go

@@ -311,30 +311,55 @@ func (m Manager) updateInterfaceLdapFilters(
 			}
 		}
 
-		// Save the interface
-		err = m.interfaces.SaveInterface(ctx, ifaceId, func(i *domain.Interface) (*domain.Interface, error) {
-			if i.LdapAllowedUsers == nil {
-				i.LdapAllowedUsers = make(map[string][]domain.UserIdentifier)
-			}
-			i.LdapAllowedUsers[provider.ProviderName] = matchedUserIds
-			return i, nil
-		})
-		if err != nil {
-			slog.Error("failed to save interface ldap allowed users",
-				"interface", ifaceId,
-				"provider", provider.ProviderName,
-				"error", err)
-		} else {
-			slog.Debug("updated interface ldap allowed users",
-				"interface", ifaceId,
-				"provider", provider.ProviderName,
-				"matched_count", len(matchedUserIds))
-		}
+		m.applyInterfaceLdapFilter(ctx, ifaceId, provider.ProviderName, matchedUserIds)
 	}
 
 	return nil
 }
 
+// applyInterfaceLdapFilter stores the users an LDAP filter matched onto an
+// existing interface.
+//
+// It deliberately does not create the interface. SaveInterface would, which is
+// wrong twice over: an interface_filter entry is a statement about who may use
+// an interface, not a reason to bring one into existence, and the row it
+// creates has no backend. It also races the startup importer, which snapshots
+// the interface list before its device round-trips and then fails with
+// "interface already exists" once it finds the row.
+func (m Manager) applyInterfaceLdapFilter(
+	ctx context.Context,
+	ifaceId domain.InterfaceIdentifier,
+	providerName string,
+	matchedUserIds []domain.UserIdentifier,
+) {
+	if _, err := m.interfaces.GetInterface(ctx, ifaceId); err != nil {
+		if errors.Is(err, domain.ErrNotFound) {
+			slog.Warn("skipping interface filter for unknown interface",
+				"interface", ifaceId, "provider", providerName)
+		} else {
+			slog.Error("failed to look up interface for ldap filter",
+				"interface", ifaceId, "provider", providerName, "error", err)
+		}
+		return
+	}
+
+	err := m.interfaces.SaveInterface(ctx, ifaceId, func(i *domain.Interface) (*domain.Interface, error) {
+		if i.LdapAllowedUsers == nil {
+			i.LdapAllowedUsers = make(map[string][]domain.UserIdentifier)
+		}
+		i.LdapAllowedUsers[providerName] = matchedUserIds
+		return i, nil
+	})
+	if err != nil {
+		slog.Error("failed to save interface ldap allowed users",
+			"interface", ifaceId, "provider", providerName, "error", err)
+		return
+	}
+
+	slog.Debug("updated interface ldap allowed users",
+		"interface", ifaceId, "provider", providerName, "matched_count", len(matchedUserIds))
+}
+
 func ldapUserIdentifier(rawUser map[string]any, field string) domain.UserIdentifier {
 	identifier := internal.MapDefaultString(rawUser, field, "")
 	identifier = domain.SanitizeIdentifier(identifier, 256)

internal/app/users/user_manager.go

@@ -40,6 +40,8 @@ type PeerDatabaseRepo interface {
 }
 
 type InterfaceDatabaseRepo interface {
+	// GetInterface returns the interface with the given identifier.
+	GetInterface(ctx context.Context, id domain.InterfaceIdentifier) (*domain.Interface, error)
 	// SaveInterface saves the interface with the given identifier.
 	SaveInterface(ctx context.Context, id domain.InterfaceIdentifier, updateFunc func(i *domain.Interface) (*domain.Interface, error)) error
 }