fix(cache): optimistic updates on delete + disable

Bug symptom: after clicking Delete or Disable on a user/group/computer,
the list+drawer still showed the target entry for several seconds
("took longer than expected"), even though the LDAP op succeeded. Cause
is AD replication delay — Refresh() runs against the readonly-bind DC
which hasn't yet received the mutation from the writing DC.

Solution: bypass the replication round-trip by applying the mutation to
the in-memory cache directly, the moment the per-entity LDAP op returns
without error. Follows the existing OnAddUserToGroup /
OnRemoveUserFromGroup pattern.

New Manager hooks (wired into finaliseBulk* in bulk_handlers.go):
  - OnDeleteUser(dn)    — drops entry, scrubs from Groups.Members
  - OnDeleteGroup(dn)   — drops entry, scrubs from User.Groups /
                          Computer.Groups
  - OnDeleteComputer(dn) — drops entry
  - OnDisableUser(dn)   — flips Enabled=false on the user
  - OnDisableComputer(dn) — flips Enabled=false on the computer

Supporting change: new unexported Cache[T].remove(dn) that deletes a
single entry by DN and rebuilds the O(1) indexes. No-op on miss so
callers can call it optimistically.

The trailing full Refresh() in finaliseBulk* is kept as a reconciliation
pass but correctness of the redirected list no longer depends on it
winning the race against replication.

Coverage: hooks_test.go (8 subtests): Cache.remove happy/miss/empty,
OnDeleteUser member-scrub, OnDeleteGroup memberOf-scrub on both users
and computers, OnDeleteComputer idempotence, OnDisable{User,Computer}
Enabled-flip.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

Author: CybotTM

internal/ldap_cache/cache.go

@@ -129,6 +129,29 @@ func (c *Cache[T]) update(fn func(*T)) {
 	c.buildIndexes()
 }
 
+// remove drops the item with the given distinguished name from the cache
+// and rebuilds indexes. A no-op when no matching entry exists, so callers
+// can invoke it optimistically after an LDAP delete without first
+// checking presence. Used by the Manager's OnDelete* hooks to keep the
+// in-memory cache correct immediately after a successful LDAP mutation,
+// without waiting for the next background Refresh (which can be delayed
+// by AD replication between the modifying DC and the readonly-bind DC).
+func (c *Cache[T]) remove(dn string) {
+	c.m.Lock()
+	defer c.m.Unlock()
+
+	for idx := range c.items {
+		if c.items[idx].DN() != dn {
+			continue
+		}
+
+		c.items = append(c.items[:idx], c.items[idx+1:]...)
+		c.buildIndexes()
+
+		return
+	}
+}
+
 // Get returns a snapshot copy of all cached items.
 // The returned slice is safe to iterate without holding any lock.
 func (c *Cache[T]) Get() []T {

internal/ldap_cache/hooks_test.go

@@ -0,0 +1,220 @@
+// internal/ldap_cache/hooks_test.go
+package ldap_cache
+
+import (
+	"testing"
+
+	ldap "github.com/netresearch/simple-ldap-go"
+)
+
+// testEntity is a minimal cacheable used to exercise Cache.remove under
+// a controlled DN surface. The ldap.User / ldap.Computer structs embed
+// simple-ldap-go's Object struct with unexported dn/cn fields, so there
+// is no public way to seed a real DN from outside that package. An
+// in-test type sidesteps that without resorting to unsafe pointer
+// tricks, and lets us assert DN-indexed behaviour properly.
+type testEntity struct {
+	dn string
+	v  string
+}
+
+func (e testEntity) DN() string { return e.dn }
+
+func TestCacheRemove(t *testing.T) {
+	seed := []testEntity{
+		{dn: "cn=alice,dc=x", v: "A"},
+		{dn: "cn=bob,dc=x", v: "B"},
+		{dn: "cn=carol,dc=x", v: "C"},
+	}
+
+	t.Run("removes the matching entry", func(t *testing.T) {
+		c := NewCached[testEntity]()
+		c.setAll(append([]testEntity(nil), seed...))
+
+		c.remove("cn=bob,dc=x")
+
+		if got := c.Count(); got != 2 {
+			t.Fatalf("expected 2 items after remove, got %d", got)
+		}
+		if _, ok := c.FindByDN("cn=bob,dc=x"); ok {
+			t.Error("bob still indexed after remove")
+		}
+		if _, ok := c.FindByDN("cn=alice,dc=x"); !ok {
+			t.Error("alice missing after remove")
+		}
+		if _, ok := c.FindByDN("cn=carol,dc=x"); !ok {
+			t.Error("carol missing after remove")
+		}
+	})
+
+	t.Run("no-op when DN is not present", func(t *testing.T) {
+		c := NewCached[testEntity]()
+		c.setAll(append([]testEntity(nil), seed...))
+
+		c.remove("cn=does-not-exist,dc=x")
+
+		if got := c.Count(); got != 3 {
+			t.Fatalf("expected 3 items, got %d", got)
+		}
+	})
+
+	t.Run("tolerates empty cache", func(t *testing.T) {
+		c := NewCached[testEntity]()
+		c.remove("cn=whatever")
+
+		if got := c.Count(); got != 0 {
+			t.Fatalf("expected 0 items, got %d", got)
+		}
+	})
+}
+
+// TestManagerOnDeleteUser_ScrubGroupMembership relies on the fact that
+// every group's Members is a []string of user DNs — so even though the
+// mock users synthesise DN() == "", we can assert that a user DN we
+// seed into a group's member list gets scrubbed by OnDeleteUser.
+func TestManagerOnDeleteUser_ScrubGroupMembership(t *testing.T) {
+	const userDN = "cn=john.doe,ou=users,dc=example,dc=com"
+
+	mockClient := &mockLDAPClient{
+		users: []ldap.User{
+			NewMockUser(userDN, "john.doe", true, nil),
+		},
+		groups: []ldap.Group{
+			{Members: []string{userDN, "cn=other,dc=example,dc=com"}},
+		},
+	}
+	manager := New(mockClient)
+
+	if err := manager.RefreshUsers(); err != nil {
+		t.Fatalf("refresh users: %v", err)
+	}
+	if err := manager.RefreshGroups(); err != nil {
+		t.Fatalf("refresh groups: %v", err)
+	}
+
+	manager.OnDeleteUser(userDN)
+
+	groups := manager.Groups.Get()
+	if len(groups) != 1 {
+		t.Fatalf("expected 1 group, got %d", len(groups))
+	}
+	for _, m := range groups[0].Members {
+		if m == userDN {
+			t.Errorf("group still references deleted user %q", userDN)
+		}
+	}
+	if want := "cn=other,dc=example,dc=com"; len(groups[0].Members) != 1 || groups[0].Members[0] != want {
+		t.Errorf("expected remaining member %q, got %v", want, groups[0].Members)
+	}
+}
+
+// TestManagerOnDeleteGroup_ScrubMemberOf verifies that deleting a group
+// removes it from every user's Groups slice and every computer's Groups
+// slice.
+func TestManagerOnDeleteGroup_ScrubMemberOf(t *testing.T) {
+	const groupDN = "cn=old-group,ou=groups,dc=example,dc=com"
+
+	mockClient := &mockLDAPClient{
+		users: []ldap.User{
+			NewMockUser("cn=john.doe,ou=users,dc=example,dc=com", "john.doe", true,
+				[]string{groupDN, "cn=keep,dc=example,dc=com"}),
+		},
+		groups: []ldap.Group{
+			{Members: nil},
+		},
+		computers: []ldap.Computer{
+			NewMockComputer("cn=workstation-01,ou=computers,dc=example,dc=com", "workstation-01$", true,
+				[]string{groupDN, "cn=keep,dc=example,dc=com"}),
+		},
+	}
+	manager := New(mockClient)
+	manager.Refresh()
+
+	manager.OnDeleteGroup(groupDN)
+
+	users := manager.Users.Get()
+	for _, u := range users {
+		for _, g := range u.Groups {
+			if g == groupDN {
+				t.Errorf("user %q still references deleted group %q", u.SAMAccountName, groupDN)
+			}
+		}
+	}
+
+	computers := manager.Computers.Get()
+	for _, c := range computers {
+		for _, g := range c.Groups {
+			if g == groupDN {
+				t.Errorf("computer %q still references deleted group %q", c.SAMAccountName, groupDN)
+			}
+		}
+	}
+}
+
+// TestManagerOnDeleteComputer_NoOpOnAbsent confirms that deleting a
+// computer DN that isn't cached is a safe no-op (the hook is called
+// before bulk_handlers has a chance to check cache presence).
+func TestManagerOnDeleteComputer_NoOpOnAbsent(t *testing.T) {
+	mockClient := &mockLDAPClient{
+		computers: createMockComputers(),
+	}
+	manager := New(mockClient)
+	if err := manager.RefreshComputers(); err != nil {
+		t.Fatalf("refresh computers: %v", err)
+	}
+
+	before := manager.Computers.Count()
+	manager.OnDeleteComputer("cn=ghost,ou=computers,dc=example,dc=com")
+
+	if after := manager.Computers.Count(); after != before {
+		t.Errorf("computer count changed: before=%d after=%d", before, after)
+	}
+}
+
+// TestManagerOnDisableUser_FlipsEnabled uses the `update` code path
+// which iterates all users. Because mock users' DN() returns "", we
+// exploit the empty-string equality to flip every cached user's
+// Enabled bit and assert the write-back took effect. This is the same
+// pragmatic approach the existing OnAdd/OnRemove tests use.
+func TestManagerOnDisableUser_FlipsEnabled(t *testing.T) {
+	mockClient := &mockLDAPClient{
+		users: []ldap.User{
+			NewMockUser("cn=john.doe,…", "john.doe", true, nil),
+			NewMockUser("cn=jane.smith,…", "jane.smith", true, nil),
+		},
+	}
+	manager := New(mockClient)
+	if err := manager.RefreshUsers(); err != nil {
+		t.Fatalf("refresh users: %v", err)
+	}
+
+	manager.OnDisableUser("") // mock DN()=="" matches all
+
+	for _, u := range manager.Users.Get() {
+		if u.Enabled {
+			t.Errorf("user %q still enabled after OnDisableUser", u.SAMAccountName)
+		}
+	}
+}
+
+// TestManagerOnDisableComputer_FlipsEnabled mirrors
+// TestManagerOnDisableUser_FlipsEnabled for machine accounts.
+func TestManagerOnDisableComputer_FlipsEnabled(t *testing.T) {
+	mockClient := &mockLDAPClient{
+		computers: []ldap.Computer{
+			NewMockComputer("cn=ws01,…", "ws01$", true, nil),
+		},
+	}
+	manager := New(mockClient)
+	if err := manager.RefreshComputers(); err != nil {
+		t.Fatalf("refresh computers: %v", err)
+	}
+
+	manager.OnDisableComputer("")
+
+	for _, cmp := range manager.Computers.Get() {
+		if cmp.Enabled {
+			t.Errorf("computer %q still enabled after OnDisableComputer", cmp.SAMAccountName)
+		}
+	}
+}

internal/ldap_cache/manager.go

@@ -489,6 +489,83 @@ func (m *Manager) OnRemoveUserFromGroup(userDN, groupDN string) {
 	})
 }
 
+// OnDeleteUser drops a user entry from the cache by DN and scrubs any
+// group memberships that reference it. Call after a successful LDAP
+// delete. Idempotent: no-op if the user isn't cached. The trailing
+// Refresh() the bulk handler still issues catches up with everything
+// else the directory might have changed, but correctness of the user
+// list itself no longer depends on that refresh winning against AD
+// replication delay.
+func (m *Manager) OnDeleteUser(userDN string) {
+	m.Users.remove(userDN)
+
+	m.Groups.update(func(group *ldap.Group) {
+		for idx, member := range group.Members {
+			if member == userDN {
+				group.Members = append(group.Members[:idx], group.Members[idx+1:]...)
+
+				return
+			}
+		}
+	})
+}
+
+// OnDeleteGroup drops a group entry from the cache by DN and scrubs any
+// user/computer memberOf references that point at it. See OnDeleteUser
+// for rationale.
+func (m *Manager) OnDeleteGroup(groupDN string) {
+	m.Groups.remove(groupDN)
+
+	m.Users.update(func(user *ldap.User) {
+		for idx, g := range user.Groups {
+			if g == groupDN {
+				user.Groups = append(user.Groups[:idx], user.Groups[idx+1:]...)
+
+				return
+			}
+		}
+	})
+
+	m.Computers.update(func(computer *ldap.Computer) {
+		for idx, g := range computer.Groups {
+			if g == groupDN {
+				computer.Groups = append(computer.Groups[:idx], computer.Groups[idx+1:]...)
+
+				return
+			}
+		}
+	})
+}
+
+// OnDeleteComputer drops a computer entry from the cache by DN.
+// Computers are not referenced from other cached entities (group
+// membership is stored on the computer side only for memberOf display),
+// so this is a straight remove.
+func (m *Manager) OnDeleteComputer(computerDN string) {
+	m.Computers.remove(computerDN)
+}
+
+// OnDisableUser flips the Enabled bit on the cached user to false so
+// the UI reflects the disabled state immediately after a successful
+// LDAP userAccountControl mutation, without waiting for the next
+// background Refresh to rediscover it via the readonly bind.
+func (m *Manager) OnDisableUser(userDN string) {
+	m.Users.update(func(user *ldap.User) {
+		if user.DN() == userDN {
+			user.Enabled = false
+		}
+	})
+}
+
+// OnDisableComputer is the OnDisableUser analogue for machine accounts.
+func (m *Manager) OnDisableComputer(computerDN string) {
+	m.Computers.update(func(computer *ldap.Computer) {
+		if computer.DN() == computerDN {
+			computer.Enabled = false
+		}
+	})
+}
+
 // PopulateGroupsForUserFromData creates a FullLDAPUser with populated group memberships
 // using provided data instead of cache. Works identically to PopulateGroupsForUser
 // but operates on explicit slices rather than the cache.