diff --git a/internal/app/users/ldap_interface_filter_test.go b/internal/app/users/ldap_interface_filter_test.go new file mode 100644 index 0000000..a9a63d5 --- /dev/null +++ b/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") +} diff --git a/internal/app/users/ldap_sync.go b/internal/app/users/ldap_sync.go index 7e77380..3797136 100644 --- a/internal/app/users/ldap_sync.go +++ b/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) diff --git a/internal/app/users/user_manager.go b/internal/app/users/user_manager.go index 06dbc11..7743765 100644 --- a/internal/app/users/user_manager.go +++ b/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 }