diff --git a/internal/ldap_cache/cache.go b/internal/ldap_cache/cache.go index 5f151fea..99a941f4 100644 --- a/internal/ldap_cache/cache.go +++ b/internal/ldap_cache/cache.go @@ -5,6 +5,7 @@ package ldap_cache import ( "reflect" + "slices" "sync" ) @@ -129,6 +130,63 @@ 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). +// +// Uses the dnIndex for O(1) location lookup and slices.Delete for the +// slice shrink (which zeroes the vacated slot, so pointer fields inside +// T do not keep the removed entry's data alive indefinitely). +func (c *Cache[T]) remove(dn string) { + c.m.Lock() + defer c.m.Unlock() + + ptr, ok := c.dnIndex[dn] + if !ok { + return + } + + for idx := range c.items { + if &c.items[idx] != ptr { + continue + } + + c.items = slices.Delete(c.items, idx, idx+1) + c.buildIndexes() + + return + } +} + +// updateByDN applies a mutation function to the single cached item with +// the given DN, using the dnIndex for O(1) location. Unlike update() +// this does NOT rebuild indexes — callers must only perform non-key +// mutations (flipping Enabled, touching Mail, etc.). DN-changing edits +// must go through setAll or remove+append. +// +// Returns true if the entry was found and the fn invoked. Used by the +// Manager's OnDisable* hooks to flip Enabled=false without a full +// linear scan. +func (c *Cache[T]) updateByDN(dn string, fn func(*T)) bool { + c.m.Lock() + defer c.m.Unlock() + + ptr, ok := c.dnIndex[dn] + if !ok { + return false + } + + // ptr points into c.items; fn mutates in place. buildIndexes is not + // needed because DN is stable across the mutation. + fn(ptr) + + return true +} + // 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 { diff --git a/internal/ldap_cache/hooks_test.go b/internal/ldap_cache/hooks_test.go new file mode 100644 index 00000000..e4f869df --- /dev/null +++ b/internal/ldap_cache/hooks_test.go @@ -0,0 +1,297 @@ +// 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) + } +} + +// TestManagerOnDeleteComputer_ScrubGroupMembership verifies that +// deleting a computer also removes its DN from every cached group's +// Members list. Computers can be group members in AD (machine accounts +// in security groups), and the UI derives computer group memberships +// by scanning group Members, so the scrub is necessary for the group +// member count to stay accurate after the delete. +func TestManagerOnDeleteComputer_ScrubGroupMembership(t *testing.T) { + const computerDN = "cn=workstation-01,ou=computers,dc=example,dc=com" + + mockClient := &mockLDAPClient{ + computers: []ldap.Computer{ + NewMockComputer(computerDN, "workstation-01$", true, nil), + }, + groups: []ldap.Group{ + {Members: []string{computerDN, "cn=other,dc=example,dc=com"}}, + }, + } + manager := New(mockClient) + if err := manager.RefreshComputers(); err != nil { + t.Fatalf("refresh computers: %v", err) + } + if err := manager.RefreshGroups(); err != nil { + t.Fatalf("refresh groups: %v", err) + } + + manager.OnDeleteComputer(computerDN) + + 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 == computerDN { + t.Errorf("group still references deleted computer %q", computerDN) + } + } + 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) + } +} + +// testMutable is a cacheable with both a DN and a mutable bool, used +// to exercise Cache.updateByDN under a controlled DN surface. The +// approach mirrors testEntity above; simple-ldap-go's unexported dn +// field makes it impractical to seed real DNs on ldap.User / +// ldap.Computer from outside that package. +type testMutable struct { + dn string + enabled bool +} + +func (m testMutable) DN() string { return m.dn } + +func TestCacheUpdateByDN(t *testing.T) { + t.Run("mutates the matching entry and returns true", func(t *testing.T) { + c := NewCached[testMutable]() + c.setAll([]testMutable{ + {dn: "cn=alice,dc=x", enabled: true}, + {dn: "cn=bob,dc=x", enabled: true}, + }) + + ok := c.updateByDN("cn=bob,dc=x", func(m *testMutable) { m.enabled = false }) + if !ok { + t.Fatal("updateByDN returned false for an existing DN") + } + + got, found := c.FindByDN("cn=bob,dc=x") + if !found { + t.Fatal("bob missing after updateByDN") + } + if got.enabled { + t.Error("bob still enabled after updateByDN") + } + + alice, _ := c.FindByDN("cn=alice,dc=x") + if !alice.enabled { + t.Error("alice got mutated — update leaked past the DN filter") + } + }) + + t.Run("returns false and does nothing for an unknown DN", func(t *testing.T) { + c := NewCached[testMutable]() + c.setAll([]testMutable{ + {dn: "cn=alice,dc=x", enabled: true}, + }) + + called := false + ok := c.updateByDN("cn=ghost,dc=x", func(_ *testMutable) { called = true }) + if ok { + t.Error("updateByDN returned true for a missing DN") + } + if called { + t.Error("fn was invoked for a missing DN") + } + }) +} + +// TestManagerOnDisable_NoOpOnAbsentDN verifies that calling +// OnDisableUser / OnDisableComputer with a DN that isn't cached is a +// safe no-op. The happy-path ("a known DN gets Enabled flipped") is +// covered structurally by TestCacheUpdateByDN above — seeding an +// ldap.User with a real DN requires reaching into simple-ldap-go's +// unexported Object fields. +func TestManagerOnDisable_NoOpOnAbsentDN(t *testing.T) { + mockClient := &mockLDAPClient{ + users: createMockUsers(), + computers: createMockComputers(), + } + manager := New(mockClient) + manager.Refresh() + + usersBefore := manager.Users.Count() + computersBefore := manager.Computers.Count() + + manager.OnDisableUser("cn=ghost-user,dc=example,dc=com") + manager.OnDisableComputer("cn=ghost-computer,dc=example,dc=com") + + if got := manager.Users.Count(); got != usersBefore { + t.Errorf("user count changed: before=%d after=%d", usersBefore, got) + } + if got := manager.Computers.Count(); got != computersBefore { + t.Errorf("computer count changed: before=%d after=%d", computersBefore, got) + } +} diff --git a/internal/ldap_cache/manager.go b/internal/ldap_cache/manager.go index 73c1f729..72049ec8 100644 --- a/internal/ldap_cache/manager.go +++ b/internal/ldap_cache/manager.go @@ -489,6 +489,79 @@ func (m *Manager) OnRemoveUserFromGroup(userDN, groupDN string) { }) } +// OnDeleteUser drops a user entry from the cache by DN and scrubs any +// group membership references that point at it. Call after a successful +// LDAP delete. Idempotent: no-op if the user isn't cached. +// +// The group-membership scrub is a full scan over Groups since there is +// no reverse index from user DN → group DN list. For typical bulk +// operations (tens of deletions, hundreds of groups) this is fine; a +// reverse index would only pay off for larger directories. +func (m *Manager) OnDeleteUser(userDN string) { + m.Users.remove(userDN) + + m.Groups.update(func(group *ldap.Group) { + group.Members = slices.DeleteFunc(group.Members, func(member string) bool { + return member == userDN + }) + }) +} + +// OnDeleteGroup drops a group entry from the cache by DN and scrubs +// memberOf references from every cached user and computer. See +// OnDeleteUser for the iteration-cost rationale. +func (m *Manager) OnDeleteGroup(groupDN string) { + m.Groups.remove(groupDN) + + m.Users.update(func(user *ldap.User) { + user.Groups = slices.DeleteFunc(user.Groups, func(g string) bool { + return g == groupDN + }) + }) + + m.Computers.update(func(computer *ldap.Computer) { + computer.Groups = slices.DeleteFunc(computer.Groups, func(g string) bool { + return g == groupDN + }) + }) +} + +// OnDeleteComputer drops a computer entry from the cache by DN and +// scrubs any group Members reference pointing at it. Computer group +// memberships are derived at display time by scanning ldap.Group.Members +// for the computer DN (see PopulateGroupsForComputerFromData), so those +// reverse references have to be removed or the group member count and +// the drawer's members list stay stale until the next full Refresh. +func (m *Manager) OnDeleteComputer(computerDN string) { + m.Computers.remove(computerDN) + + m.Groups.update(func(group *ldap.Group) { + group.Members = slices.DeleteFunc(group.Members, func(member string) bool { + return member == 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. +// +// Uses Cache.updateByDN for O(1) lookup via dnIndex instead of a full +// linear scan over every cached user. +func (m *Manager) OnDisableUser(userDN string) { + m.Users.updateByDN(userDN, func(user *ldap.User) { + user.Enabled = false + }) +} + +// OnDisableComputer is the OnDisableUser analogue for machine accounts. +func (m *Manager) OnDisableComputer(computerDN string) { + m.Computers.updateByDN(computerDN, func(computer *ldap.Computer) { + 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. diff --git a/internal/web/bulk_handlers.go b/internal/web/bulk_handlers.go index 27b936a0..e5385611 100644 --- a/internal/web/bulk_handlers.go +++ b/internal/web/bulk_handlers.go @@ -13,6 +13,8 @@ package web import ( "fmt" + "net/url" + "strings" "github.com/gofiber/fiber/v2" ldap "github.com/netresearch/simple-ldap-go" @@ -21,6 +23,74 @@ import ( "github.com/netresearch/ldap-manager/internal/web/templates" ) +// bulkRedirectAfter computes the post-action redirect target for a bulk +// handler. It preserves the originating filters (ou=, enabled=, member_of=, +// …) by reusing the Referer URL's query string. It also keeps or drops +// the ?panel= drawer-state parameter depending on whether the entity the +// drawer was showing is still there after the op: +// +// - dropPanel=true (delete): always strips ?panel= because the +// referenced entity is gone. If the Referer path was a single-entity +// detail page (fallbackList + "/:dn"), it is rewritten to the parent +// list so the user doesn't land on a dangling detail route. +// - dropPanel=false (disable et al.): keeps the full Referer verbatim +// so the drawer reopens on the same entity (now with updated state) +// and the filter chips stay applied. +// +// Only same-origin Referer values are honoured to avoid open-redirect +// risk; cross-origin or unparseable Referer falls back to fallbackList. +func bulkRedirectAfter(c *fiber.Ctx, fallbackList string, dropPanel bool) string { + ref := c.Get(fiber.HeaderReferer) + if ref == "" { + return fallbackList + } + + refURL, err := url.Parse(ref) + if err != nil { + return fallbackList + } + + // Reject cross-origin Referers. Allow relative Referers (empty Host), + // which some clients still emit for same-origin POSTs. + // + // Compare hostnames via url.URL.Hostname() (strips any port) so dev + // and proxy deployments on non-default ports (e.g. localhost:3000) + // are not incorrectly flagged as cross-origin against Fiber's + // port-less c.Hostname(). + if refURL.Host != "" && refURL.Hostname() != c.Hostname() { + return fallbackList + } + + // Use EscapedPath so percent-encoded DNs (e.g. /users/cn%3Dbob%2Cdc%3Dx) + // round-trip unchanged; refURL.Path decodes by default which would + // emit raw "=" and "," that the router then has to re-interpret. + path := refURL.EscapedPath() + q := refURL.Query() + + if dropPanel { + q.Del("panel") + // The Referer may have been a per-entity detail page + // (fallbackList + "/:dn"); after delete the entity is gone, so + // collapse back to the parent list route. + if path != fallbackList && strings.HasPrefix(path, fallbackList+"/") { + path = fallbackList + } + } + + // Only redirect to paths on the same list surface — defence in depth + // against an attacker setting a crafted Referer to bounce through the + // 303 into an unrelated route. + if path != fallbackList && !strings.HasPrefix(path, fallbackList+"/") { + return fallbackList + } + + if len(q) == 0 { + return path + } + + return path + "?" + q.Encode() +} + // bulkNotImplementedMessage is the response body for bulk actions that // require LDAP operations simple-ldap-go does not yet expose. The HTTP // status is 501. @@ -135,7 +205,7 @@ func (a *App) bulkAddToGroup(c *fiber.Ctx) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", false), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -169,7 +239,7 @@ func (a *App) bulkAddToGroup(c *fiber.Ctx) error { Str("group", groupDN). Msg("bulk add-to-group complete") - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", false), fiber.StatusSeeOther) } // bulkRemoveFromGroup removes each user in target_dn[] from the group_dn. @@ -185,7 +255,7 @@ func (a *App) bulkRemoveFromGroup(c *fiber.Ctx) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", false), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -219,7 +289,7 @@ func (a *App) bulkRemoveFromGroup(c *fiber.Ctx) error { Str("group", groupDN). Msg("bulk remove-from-group complete") - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", false), fiber.StatusSeeOther) } // bulkDeleteUsers deletes each user in target_dn[]. Per-entry failures @@ -228,7 +298,7 @@ func (a *App) bulkRemoveFromGroup(c *fiber.Ctx) error { func (a *App) bulkDeleteUsers(c *fiber.Ctx) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", true), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -251,41 +321,60 @@ func (a *App) bulkDeleteUsers(c *fiber.Ctx) error { continue } + // Optimistic cache update: drop the user and scrub any group + // memberships pointing at them, so the redirected list renders + // correctly on the next request even if AD replication to the + // readonly-bind DC hasn't caught up yet. + if a.ldapCache != nil { + a.ldapCache.OnDeleteUser(userDN) + } + deleted++ } a.finaliseBulkDelete(c, "user", deleted, len(targets), firstErr) - return c.Redirect("/users", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/users", true), fiber.StatusSeeOther) } // bulkDeleteGroups deletes each DN in target_dn[] as an LDAP entry // via simple-ldap-go's generic DeleteByDN (which performs a raw // ldap.Del on the DN). Flash summarises the result on the list page. func (a *App) bulkDeleteGroups(c *fiber.Ctx) error { - return a.bulkDeleteByDN(c, "group", "/groups") + return a.bulkDeleteByDN(c, "group", "/groups", func(dn string) { + if a.ldapCache != nil { + a.ldapCache.OnDeleteGroup(dn) + } + }) } // bulkDeleteComputers mirrors bulkDeleteGroups against the computers // list. Uses the same generic DeleteByDN because computers and groups // are both single-entry deletes without a type-specific helper. func (a *App) bulkDeleteComputers(c *fiber.Ctx) error { - return a.bulkDeleteByDN(c, "computer", "/computers") + return a.bulkDeleteByDN(c, "computer", "/computers", func(dn string) { + if a.ldapCache != nil { + a.ldapCache.OnDeleteComputer(dn) + } + }) } // bulkDeleteByDN is the shared body of bulkDeleteGroups / // bulkDeleteComputers: collect target_dn[], open a per-user LDAP // binding, DeleteByDN each target, count successes, flash a summary, // and redirect back to the list page at `redirectTo`. `kind` is the -// singular noun used in the flash and the log field. +// singular noun used in the flash and the log field. onCacheSuccess is +// invoked per successfully-deleted DN so the caller can apply an +// optimistic cache update (scrubbing the entity before the next +// Refresh() round-trip). // // Users have their own handler (bulkDeleteUsers) because we call the // type-specific `client.DeleteUser` which also fires cache-hook work // in simple-ldap-go that DeleteByDN bypasses. -func (a *App) bulkDeleteByDN(c *fiber.Ctx, kind, redirectTo string) error { +func (a *App) bulkDeleteByDN(c *fiber.Ctx, kind, redirectTo string, onCacheSuccess func(dn string)) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect(redirectTo, fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, redirectTo, true), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -308,12 +397,16 @@ func (a *App) bulkDeleteByDN(c *fiber.Ctx, kind, redirectTo string) error { continue } + if onCacheSuccess != nil { + onCacheSuccess(dn) + } + deleted++ } a.finaliseBulkDelete(c, kind, deleted, len(targets), firstErr) - return c.Redirect(redirectTo, fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, redirectTo, true), fiber.StatusSeeOther) } // bulkDisableUsers flips the ACCOUNTDISABLE bit (0x2) on each user DN @@ -321,27 +414,47 @@ func (a *App) bulkDeleteByDN(c *fiber.Ctx, kind, redirectTo string) error { // only — the caller in handleBulkUsers gates this on // a.ldapConfig.IsActiveDirectory so non-AD deployments never reach here. func (a *App) bulkDisableUsers(c *fiber.Ctx) error { - return a.bulkUACDisable(c, "user", "/users", func(client *ldap.LDAP, dn string) error { - return client.DisableUserContext(c.UserContext(), dn) - }) + return a.bulkUACDisable(c, "user", "/users", + func(client *ldap.LDAP, dn string) error { + return client.DisableUserContext(c.UserContext(), dn) + }, + func(dn string) { + if a.ldapCache != nil { + a.ldapCache.OnDisableUser(dn) + } + }) } // bulkDisableComputers mirrors bulkDisableUsers for computer entries. // AD-only, same gating in handleBulkComputers. func (a *App) bulkDisableComputers(c *fiber.Ctx) error { - return a.bulkUACDisable(c, "computer", "/computers", func(client *ldap.LDAP, dn string) error { - return client.DisableComputerContext(c.UserContext(), dn) - }) + return a.bulkUACDisable(c, "computer", "/computers", + func(client *ldap.LDAP, dn string) error { + return client.DisableComputerContext(c.UserContext(), dn) + }, + func(dn string) { + if a.ldapCache != nil { + a.ldapCache.OnDisableComputer(dn) + } + }) } // bulkUACDisable is the shared body for bulkDisableUsers / // bulkDisableComputers: open a per-user LDAP binding, run the given // per-DN disable op, count successes, flash "Disabled N / M s". +// onCacheSuccess is invoked per successfully-disabled DN so the caller +// can flip Enabled=false in the local cache without waiting for the +// next Refresh() to notice via the readonly-bind DC. // Pattern matches bulkDeleteByDN — different op, same batching. -func (a *App) bulkUACDisable(c *fiber.Ctx, kind, redirectTo string, op func(*ldap.LDAP, string) error) error { +func (a *App) bulkUACDisable( + c *fiber.Ctx, + kind, redirectTo string, + op func(*ldap.LDAP, string) error, + onCacheSuccess func(dn string), +) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect(redirectTo, fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, redirectTo, false), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -364,23 +477,32 @@ func (a *App) bulkUACDisable(c *fiber.Ctx, kind, redirectTo string, op func(*lda continue } + if onCacheSuccess != nil { + onCacheSuccess(dn) + } + disabled++ } a.finaliseBulkDisable(c, kind, disabled, len(targets), firstErr) - return c.Redirect(redirectTo, fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, redirectTo, false), fiber.StatusSeeOther) } // finaliseBulkDisable is the disable analogue of finaliseBulkDelete: -// refresh caches on any success, log the summary, flash the result. +// invalidate the rendered-template cache so the redirected list page +// is re-rendered, log the summary, and flash the result. +// +// The LDAP cache itself was already updated optimistically in the +// per-entity loop via OnDisable{User,Computer} — we intentionally do +// NOT call ldapCache.Refresh() here: Refresh() queries the readonly- +// bind DC, which under normal AD replication delay returns the +// pre-disable state (Enabled=true) and overwrites our optimistic +// update. The 30 s background refresh picks up the upstream state +// once replication has caught up. func (a *App) finaliseBulkDisable(c *fiber.Ctx, kind string, disabled, total int, firstErr error) { if disabled > 0 { a.invalidateTemplateCacheOnModification() - - if a.ldapCache != nil { - a.ldapCache.Refresh() - } } log.Info(). @@ -405,20 +527,21 @@ func (a *App) finaliseBulkDisable(c *fiber.Ctx, kind string, disabled, total int } // finaliseBulkDelete is the shared post-loop cleanup for all three -// bulk-delete handlers: invalidate the template + LDAP caches if any -// delete succeeded, and drop a "Deleted N / M s" flash on the -// next list page load. `kind` is the singular noun ("user", "group", -// "computer"); pluralisation is naive "s"-suffix which is correct for -// all three. +// bulk-delete handlers: invalidate the rendered-template cache and +// drop a "Deleted N / M s" flash on the next list page load. +// `kind` is the singular noun ("user", "group", "computer"); +// pluralisation is naive "s"-suffix which is correct for all three. +// +// The LDAP cache itself was already updated optimistically in the +// per-entity loop via OnDelete{User,Group,Computer} — we intentionally +// do NOT call ldapCache.Refresh() here: Refresh() queries the readonly- +// bind DC, which under normal AD replication delay still sees the +// just-deleted entity and overwrites our optimistic scrub. The 30 s +// background refresh picks up the upstream state once replication +// has caught up. func (a *App) finaliseBulkDelete(c *fiber.Ctx, kind string, deleted, total int, firstErr error) { if deleted > 0 { - // No dedicated OnDelete hook in the cache — force a full refresh - // so stale lists don't linger. a.invalidateTemplateCacheOnModification() - - if a.ldapCache != nil { - a.ldapCache.Refresh() - } } log.Info(). @@ -465,7 +588,7 @@ func (a *App) bulkAddMembersToGroups(c *fiber.Ctx) error { targets := collectTargetDNs(c) if len(targets) == 0 { - return c.Redirect("/groups", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/groups", false), fiber.StatusSeeOther) } client, err := a.getUserLDAP(c) @@ -499,7 +622,7 @@ func (a *App) bulkAddMembersToGroups(c *fiber.Ctx) error { Str("user", userDN). Msg("bulk add-members complete") - return c.Redirect("/groups", fiber.StatusSeeOther) + return c.Redirect(bulkRedirectAfter(c, "/groups", false), fiber.StatusSeeOther) } // collectTargetDNs extracts the target_dn[] list from both URL-encoded diff --git a/internal/web/bulk_redirect_test.go b/internal/web/bulk_redirect_test.go new file mode 100644 index 00000000..fddd0ee5 --- /dev/null +++ b/internal/web/bulk_redirect_test.go @@ -0,0 +1,138 @@ +// internal/web/bulk_redirect_test.go +package web + +import ( + "io" + "net/http/httptest" + "testing" + + "github.com/gofiber/fiber/v2" +) + +// TestBulkRedirectAfter exercises the helper that preserves query +// filters (and optionally ?panel=) in the post-action 303 target. +// Covers the bug where POSTing from /users?ou=Eng&enabled=true&panel=1 +// redirected to a bare /users, closing the drawer and clearing the +// filter chips on every disable/delete action. +func TestBulkRedirectAfter(t *testing.T) { + cases := []struct { + name string + referer string + fallbackList string + dropPanel bool + want string + }{ + { + name: "no referer falls back to list", + referer: "", + fallbackList: "/users", + dropPanel: false, + want: "/users", + }, + { + name: "relative list referer preserved (disable)", + referer: "/users?ou=Eng&enabled=true&panel=1", + fallbackList: "/users", + dropPanel: false, + want: "/users?enabled=true&ou=Eng&panel=1", + }, + { + name: "list referer strips panel on delete", + referer: "/users?ou=Eng&enabled=true&panel=1", + fallbackList: "/users", + dropPanel: true, + want: "/users?enabled=true&ou=Eng", + }, + { + name: "detail page referer preserved on disable", + referer: "/users/cn%3Dbob%2Cdc%3Dx?panel=1&ou=Eng", + fallbackList: "/users", + dropPanel: false, + want: "/users/cn%3Dbob%2Cdc%3Dx?ou=Eng&panel=1", + }, + { + name: "detail page referer collapses to list on delete", + referer: "/users/cn%3Dbob%2Cdc%3Dx?panel=1&ou=Eng", + fallbackList: "/users", + dropPanel: true, + want: "/users?ou=Eng", + }, + { + name: "unrelated referer falls back", + referer: "/groups?ou=Ops", + fallbackList: "/users", + dropPanel: false, + want: "/users", + }, + { + name: "cross-origin referer rejected", + referer: "https://evil.example.com/users?ou=Eng", + fallbackList: "/users", + dropPanel: false, + want: "/users", + }, + { + // httptest.NewRequest defaults Host to "example.com"; the + // absolute referer has the same hostname but an arbitrary + // port. Fiber's c.Hostname() is port-less, so we must + // compare against refURL.Hostname(), not refURL.Host — the + // latter would treat "example.com:3000" as a different + // origin and discard filters. This guards bug-fixed in the + // post-review follow-up. + name: "same-origin with explicit port preserved", + referer: "http://example.com:3000/users?ou=Eng", + fallbackList: "/users", + dropPanel: false, + want: "/users?ou=Eng", + }, + { + name: "unparseable referer falls back", + referer: "://not-a-url", + fallbackList: "/users", + dropPanel: false, + want: "/users", + }, + { + name: "plain list no query preserved", + referer: "/groups", + fallbackList: "/groups", + dropPanel: true, + want: "/groups", + }, + { + name: "panel key dropped even when only param", + referer: "/users?panel=1", + fallbackList: "/users", + dropPanel: true, + want: "/users", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app := fiber.New() + app.Get("/_probe", func(c *fiber.Ctx) error { + return c.SendString(bulkRedirectAfter(c, tc.fallbackList, tc.dropPanel)) + }) + + req := httptest.NewRequest("GET", "/_probe", nil) + if tc.referer != "" { + req.Header.Set(fiber.HeaderReferer, tc.referer) + } + + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if got := string(body); got != tc.want { + t.Errorf("got %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/web/templates/computers_v2.templ b/internal/web/templates/computers_v2.templ index 30636ec9..e64bfdc3 100644 --- a/internal/web/templates/computers_v2.templ +++ b/internal/web/templates/computers_v2.templ @@ -19,7 +19,7 @@ type ComputerDrawerVM struct { OUName string // plain-text OU name, e.g. "ou=Computers" (for display) OUPivotHref string // URL-encoded pivot link, e.g. "/computers?ou=ou%3DComputers" CSRFToken string // current-session CSRF token used by embedded POST forms - IsAD bool // true when backend is Active Directory — gates the Disable button + IsAD bool // true when backend is Active Directory — gates the Disable button (which also requires Computer.Enabled) } templ ComputersListV2(computers []ldap.Computer, ouFilter string, ous []string, flashes []Flash, palettePinned []PinnedEntry) { @@ -325,7 +325,7 @@ templ computerDrawerContentsCtx(vm ComputerDrawerVM, inDrawer bool) {

Actions

- if vm.IsAD { + if vm.IsAD && vm.Computer.Enabled {
diff --git a/internal/web/templates/drawer_disable_gating_test.go b/internal/web/templates/drawer_disable_gating_test.go new file mode 100644 index 00000000..3896dbc3 --- /dev/null +++ b/internal/web/templates/drawer_disable_gating_test.go @@ -0,0 +1,101 @@ +// internal/web/templates/drawer_disable_gating_test.go +package templates + +import ( + "bytes" + "context" + "strings" + "testing" + + ldap "github.com/netresearch/simple-ldap-go" + + "github.com/netresearch/ldap-manager/internal/ldap_cache" +) + +// TestUserDrawerDisableGating asserts that the Disable action form is only +// rendered when both (a) the backend is Active Directory and (b) the user +// is currently enabled. Bug: the drawer previously emitted the Disable +// button for an already-disabled user, so the account was told to disable +// itself again — visually contradictory and operationally useless. +func TestUserDrawerDisableGating(t *testing.T) { + const disableMarker = `action="/users/bulk?action=disable"` + + cases := []struct { + name string + isAD bool + enabled bool + wantDisable bool + wantStatusOK string + }{ + {"AD + enabled shows Disable", true, true, true, "Enabled"}, + {"AD + already disabled hides Disable", true, false, false, "Disabled"}, + {"OpenLDAP + enabled hides Disable", false, true, false, "Enabled"}, + {"OpenLDAP + disabled hides Disable", false, false, false, "Disabled"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + vm := UserDrawerVM{ + User: &ldap_cache.FullLDAPUser{ + User: ldap.User{Enabled: tc.enabled, SAMAccountName: "bob"}, + }, + IsAD: tc.isAD, + } + + var buf bytes.Buffer + if err := UserDrawerFragment(vm).Render(context.Background(), &buf); err != nil { + t.Fatalf("render drawer fragment: %v", err) + } + + html := buf.String() + hasDisable := strings.Contains(html, disableMarker) + + if hasDisable != tc.wantDisable { + t.Errorf("disable form present=%v, want=%v (IsAD=%v, Enabled=%v)", + hasDisable, tc.wantDisable, tc.isAD, tc.enabled) + } + if !strings.Contains(html, tc.wantStatusOK) { + t.Errorf("expected drawer to render status %q, missing in output", tc.wantStatusOK) + } + }) + } +} + +// TestComputerDrawerDisableGating mirrors TestUserDrawerDisableGating for +// the computer detail drawer. Same bug symptom (Disable shown on an +// already-disabled machine account), same fix shape. +func TestComputerDrawerDisableGating(t *testing.T) { + const disableMarker = `action="/computers/bulk?action=disable"` + + cases := []struct { + name string + isAD bool + enabled bool + wantDisable bool + }{ + {"AD + enabled shows Disable", true, true, true}, + {"AD + already disabled hides Disable", true, false, false}, + {"OpenLDAP + enabled hides Disable", false, true, false}, + {"OpenLDAP + disabled hides Disable", false, false, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + vm := ComputerDrawerVM{ + Computer: ldap.Computer{Enabled: tc.enabled, SAMAccountName: "pc01$"}, + IsAD: tc.isAD, + } + + var buf bytes.Buffer + if err := ComputerDrawerFragment(vm).Render(context.Background(), &buf); err != nil { + t.Fatalf("render drawer fragment: %v", err) + } + + hasDisable := strings.Contains(buf.String(), disableMarker) + if hasDisable != tc.wantDisable { + t.Errorf("disable form present=%v, want=%v (IsAD=%v, Enabled=%v)", + hasDisable, tc.wantDisable, tc.isAD, tc.enabled) + } + }) + } +} diff --git a/internal/web/templates/users_v2.templ b/internal/web/templates/users_v2.templ index 0ce18310..9750bfc5 100644 --- a/internal/web/templates/users_v2.templ +++ b/internal/web/templates/users_v2.templ @@ -26,7 +26,7 @@ type UserDrawerVM struct { CSRFToken string // current-session CSRF token used by embedded POST forms UnassignedGroups []ldap.Group // groups the user is NOT a member of (datalist for add-to-group) FlashError string // set by modify handlers when an LDAP op fails; shown inline - IsAD bool // true when the backend is Active Directory — gates the Disable button since OpenLDAP has no portable disable mechanism + IsAD bool // true when the backend is Active Directory — gates the Disable button (which also requires User.Enabled) since OpenLDAP has no portable disable mechanism } templ UsersListV2(users []ldap.User, showDisabled bool, ouFilter string, lastLogon string, memberOfDN, memberOfCN string, ous []string, flashes []Flash, palettePinned []PinnedEntry, adminDNs map[string]struct{}) { @@ -424,7 +424,7 @@ templ userDrawerContentsCtx(vm UserDrawerVM, inDrawer bool) {

Actions

- if vm.IsAD { + if vm.IsAD && vm.User.Enabled {