test(web): close review gaps in the password-expiry view

Addresses the confirmed findings of the adversarial review of #626.

Testability seams. RequireAdmin now resolves its admin check per request, and
the App carries two nil-defaulted overrides — adminCheck and expiryResolver —
so tests can drive the handler behind the gate. Without them the handler had
0% coverage: the only route test stopped at the 403, because a DN-addressable
admin cache entry cannot be built outside the LDAP package. New handler tests
cover the 503 no-service-account guard, the admit path, the days/show/sort
query-param wiring, and the resolver-error branch. handlePasswordExpiryV2 goes
from 0% to 94.7%, RequireAdmin to 100%.

Untested guards. Added the empty-admin-group case (a directory returning an
empty group entry must not match an unset admin group) and the status-sort
column, both previously surviving mutants.

Undated rows now sort to the bottom in BOTH directions. The old sentinel kept
must-change/never/unknown last only under ascending; under descending they
floated to the top above the furthest concrete deadline. sortByDeadline pins
them at the bottom regardless of direction, and the dead expiryOrder sentinel
is gone.

Accessibility. Sort headers now carry aria-sort (ascending/descending/none) so
the active column and direction reach assistive technology rather than living
only in an aria-hidden arrow — a fix in the shared tableSortHeader helper, so
every V2 table gains it. The deadline cell for undated rows pairs the visual
em-dash with an sr-only "No expiry date" instead of announcing a lone dash.

Documented that AD's adminCount is sticky, so admin access via that marker
outlives de-privilege; prefer LDAP_ADMIN_GROUP where access should track
current privilege.

Not changed: the client-side filter does not announce "no results" via a live
region — a pre-existing gap in the shared search JS affecting every V2 list
table, out of scope here.

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

Author: CybotTM

README.md

@@ -118,7 +118,7 @@ When no readonly user is configured, the app uses per-user LDAP credentials for
 
 `/password-expiry` lists accounts whose LDAP password is expiring, resolved live via [simple-ldap-go](https://github.com/netresearch/simple-ldap-go)'s expiry API. It is **admin-only** and needs the service account.
 
-An admin is a member of `LDAP_ADMIN_GROUP` **or** an account carrying Active Directory's `adminCount=1`. On OpenLDAP there is no `adminCount`, so `LDAP_ADMIN_GROUP` is the only way to grant access — without it, the roster is reachable by no one. Group membership is read from the user's `memberOf`, which Active Directory populates automatically; an OpenLDAP deployment must have the `memberof` overlay enabled for the group gate to work.
+An admin is a member of `LDAP_ADMIN_GROUP` **or** an account carrying Active Directory's `adminCount=1`. Note that `adminCount` is *sticky*: Active Directory sets it when an account joins a protected group and never clears it on removal, so an account that was ever privileged keeps roster access. Prefer `LDAP_ADMIN_GROUP` membership where you want access to track current privilege. On OpenLDAP there is no `adminCount`, so `LDAP_ADMIN_GROUP` is the only way to grant access — without it, the roster is reachable by no one. Group membership is read from the user's `memberOf`, which Active Directory populates automatically; an OpenLDAP deployment must have the `memberof` overlay enabled for the group gate to work.
 
 The default view shows accounts due within a window (`?days=`, default 30, capped at 366); a **Show all accounts** toggle adds the never-expires and unknown accounts with a status badge. On OpenLDAP, expiry needs the `ppolicy` overlay; accounts the directory reports nothing about show as `unknown`.
 

internal/web/admin.go

@@ -47,13 +47,23 @@ func (a *App) userIsAdmin(user *ldap.User) bool {
 	return false
 }
 
+// resolveAdminCheck returns the admin predicate: the injected one in tests,
+// otherwise the real cache-backed isAdmin.
+func (a *App) resolveAdminCheck() func(string) bool {
+	if a.adminCheck != nil {
+		return a.adminCheck
+	}
+
+	return a.isAdmin
+}
+
 // RequireAdmin gates a route to administrators. It must sit behind RequireAuth,
 // which populates the viewer DN into c.Locals. A non-admin gets 403 rather than
 // a redirect: they are authenticated, just not permitted.
 func (a *App) RequireAdmin() fiber.Handler {
 	return func(c *fiber.Ctx) error {
 		userDN := GetUserDN(c)
-		if userDN == "" || !a.isAdmin(userDN) {
+		if userDN == "" || !a.resolveAdminCheck()(userDN) {
 			log.Warn().
 				Str("userDN", userDN).
 				Str("path", c.Path()).

internal/web/admin_test.go

@@ -45,6 +45,14 @@ func TestUserIsAdmin(t *testing.T) {
 			user: ldap.User{Groups: []string{adminGroup}},
 			want: false,
 		},
+		{
+			// The adminGroupDN != "" guard exists for exactly this: a directory
+			// that returns an empty-string group entry must not match an unset
+			// admin group, since IsMemberOf("") would otherwise be true.
+			name: "empty group entry with no admin group configured is not admin",
+			user: ldap.User{Groups: []string{""}},
+			want: false,
+		},
 	}
 
 	for _, tt := range tests {

internal/web/password_expiry_v2_handler.go

@@ -38,7 +38,8 @@ func (a *App) handlePasswordExpiryV2(c *fiber.Ctx) error {
 		return res
 	}
 
-	if a.ldapReadonly == nil {
+	resolver := a.effectiveExpiryResolver()
+	if resolver == nil {
 		// No service account => no enumeration path. RequireAdmin already
 		// returns false here, so this is defence in depth.
 		return c.Status(fiber.StatusServiceUnavailable).
@@ -51,7 +52,7 @@ func (a *App) handlePasswordExpiryV2(c *fiber.Ctx) error {
 	ctx := c.UserContext()
 	window := time.Duration(days) * 24 * time.Hour
 
-	rows, err := collectExpiryRows(ctx, a.ldapReadonly, window, showAll)
+	rows, err := collectExpiryRows(ctx, resolver, window, showAll)
 	if err != nil {
 		return handle500(c, err)
 	}
@@ -68,6 +69,22 @@ func (a *App) handlePasswordExpiryV2(c *fiber.Ctx) error {
 	return page.Render(c.UserContext(), c.Response().BodyWriter())
 }
 
+// effectiveExpiryResolver returns the roster's data source: the injected test
+// resolver, else the service-account client, else nil when neither is set.
+// Returning the concrete *ldap.LDAP only when non-nil avoids the typed-nil
+// interface trap — a nil *ldap.LDAP boxed into the interface would not compare
+// equal to nil and would panic on first use.
+func (a *App) effectiveExpiryResolver() expiryResolver {
+	if a.expiryResolver != nil {
+		return a.expiryResolver
+	}
+	if a.ldapReadonly != nil {
+		return a.ldapReadonly
+	}
+
+	return nil
+}
+
 // expiryResolver is the slice of the LDAP client the roster needs. Depending
 // on an interface rather than *ldap.LDAP lets the row-collection logic — the
 // due/show-all split, the disabled-skip, error propagation — be tested with a
@@ -167,10 +184,19 @@ func parseWindowDays(raw string) int {
 }
 
 // sortExpiryRows orders the rows in place by the requested column. The default
-// is by deadline ascending, so the most urgent accounts lead. Rows without a
-// concrete deadline (must-change, never, unknown) sort after dated ones on the
-// expires key, since they have no moment to compare.
+// is by deadline ascending, so the most urgent accounts lead.
+//
+// On the deadline column, rows without a concrete date (must-change, never,
+// unknown) always sort to the bottom regardless of direction — reversing the
+// direction reverses only the dated rows, keeping the undated ones out of the
+// way rather than letting them jump to the top under desc.
 func sortExpiryRows(rows []templates.ExpiryRow, key, dir string) {
+	if key != "name" && key != "status" {
+		sortByDeadline(rows, dir)
+
+		return
+	}
+
 	less := expiryLess(key)
 	sort.SliceStable(rows, func(i, j int) bool {
 		if dir == "desc" {
@@ -181,35 +207,38 @@ func sortExpiryRows(rows []templates.ExpiryRow, key, dir string) {
 	})
 }
 
-// expiryLess returns the ascending comparator for a sort column.
-func expiryLess(key string) func(a, b templates.ExpiryRow) bool {
-	switch key {
-	case "name":
-		return func(a, b templates.ExpiryRow) bool {
-			return lowerCN(a) < lowerCN(b)
+// sortByDeadline sorts dated rows by their deadline in the given direction and
+// pins undated rows to the bottom in both directions.
+func sortByDeadline(rows []templates.ExpiryRow, dir string) {
+	sort.SliceStable(rows, func(i, j int) bool {
+		a, b := rows[i], rows[j]
+		if a.HasDeadline != b.HasDeadline {
+			// The dated row always precedes the undated one.
+			return a.HasDeadline
 		}
-	case "status":
-		return func(a, b templates.ExpiryRow) bool {
-			return a.Status < b.Status
+		if !a.HasDeadline {
+			return false // both undated: keep stable order
 		}
-	default: // "expires"
-		return func(a, b templates.ExpiryRow) bool {
-			return expiryOrder(a) < expiryOrder(b)
+		if dir == "desc" {
+			return a.ExpiresAt > b.ExpiresAt
 		}
-	}
+
+		return a.ExpiresAt < b.ExpiresAt
+	})
 }
 
-// expiryOrder maps a row to a sortable deadline. Dated rows sort by their
-// timestamp; undated rows (must-change, never, unknown) sort last, keeping the
-// concrete deadlines — the ones an admin acts on — at the top.
-func expiryOrder(r templates.ExpiryRow) int64 {
-	if r.HasDeadline {
-		return r.ExpiresAt
+// expiryLess returns the ascending comparator for the non-deadline columns.
+// The deadline column is handled by sortByDeadline, which needs direction- and
+// undated-aware ordering the plain comparator cannot express.
+func expiryLess(key string) func(a, b templates.ExpiryRow) bool {
+	if key == "status" {
+		return func(a, b templates.ExpiryRow) bool {
+			return a.Status < b.Status
+		}
 	}
 
-	return 1<<62 - 1
-}
-
-func lowerCN(r templates.ExpiryRow) string {
-	return strings.ToLower(r.CN)
+	// "name"
+	return func(a, b templates.ExpiryRow) bool {
+		return strings.ToLower(a.CN) < strings.ToLower(b.CN)
+	}
 }

internal/web/password_expiry_v2_handler_test.go

@@ -3,6 +3,10 @@ package web
 import (
 	"context"
 	"errors"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -194,6 +198,52 @@ func TestSortExpiryRows_ByDeadlineUndatedLast(t *testing.T) {
 	}
 }
 
+// Undated rows must stay at the bottom under BOTH directions — reversing the
+// deadline sort reverses only the dated rows, it must not float must-change /
+// never / unknown accounts to the top.
+func TestSortExpiryRows_UndatedStayLastUnderDesc(t *testing.T) {
+	rows := []templates.ExpiryRow{
+		{CN: "must", Status: "must-change"},
+		{CN: "late", Status: "expires", HasDeadline: true, ExpiresAt: 2000},
+		{CN: "soon", Status: "expires", HasDeadline: true, ExpiresAt: 1000},
+		{CN: "never", Status: "never-expires"},
+	}
+
+	sortExpiryRows(rows, "expires", "desc")
+
+	// Dated rows first, newest deadline leading; undated rows keep their order
+	// at the bottom.
+	want := []string{"late", "soon", "must", "never"}
+	for i, cn := range want {
+		if rows[i].CN != cn {
+			t.Errorf("position %d = %q, want %q", i, rows[i].CN, cn)
+		}
+	}
+}
+
+func TestSortExpiryRows_ByStatus(t *testing.T) {
+	rows := []templates.ExpiryRow{
+		{CN: "c", Status: "unknown"},
+		{CN: "a", Status: "expires"},
+		{CN: "b", Status: "must-change"},
+	}
+
+	sortExpiryRows(rows, "status", "asc")
+
+	// Alphabetical by status string: expires < must-change < unknown.
+	want := []string{"expires", "must-change", "unknown"}
+	for i, s := range want {
+		if rows[i].Status != s {
+			t.Errorf("position %d status = %q, want %q", i, rows[i].Status, s)
+		}
+	}
+
+	sortExpiryRows(rows, "status", "desc")
+	if rows[0].Status != "unknown" {
+		t.Errorf("desc first = %q, want unknown", rows[0].Status)
+	}
+}
+
 func TestSortExpiryRows_NameDescending(t *testing.T) {
 	rows := []templates.ExpiryRow{
 		{CN: "Alice"}, {CN: "carol"}, {CN: "Bob"},
@@ -210,3 +260,105 @@ func TestSortExpiryRows_NameDescending(t *testing.T) {
 }
 
 func ptr[T any](v T) *T { return &v }
+
+// The injected admin check lets a request reach the handler behind
+// RequireAdmin — otherwise unreachable, since a DN-addressable admin cache
+// entry cannot be built outside the LDAP package.
+func adminApp(t *testing.T) (*App, []*http.Cookie) {
+	t.Helper()
+
+	app, _ := setupFullTestApp(t)
+	app.adminCheck = func(string) bool { return true }
+	cookies := simulatedSession(t, app)
+
+	return app, cookies
+}
+
+func getExpiry(t *testing.T, app *App, cookies []*http.Cookie, target string) *http.Response {
+	t.Helper()
+
+	req := httptest.NewRequest(http.MethodGet, target, nil)
+	for _, ck := range cookies {
+		req.AddCookie(ck)
+	}
+	resp, err := app.fiber.Test(req)
+	if err != nil {
+		t.Fatalf("request failed: %v", err)
+	}
+
+	return resp
+}
+
+// With no service account the roster has no enumeration path; the handler
+// returns 503 rather than 500 or a blank page.
+func TestHandlePasswordExpiry_NoServiceAccountIs503(t *testing.T) {
+	app, cookies := adminApp(t)
+	app.ldapReadonly = nil
+	app.expiryResolver = nil
+
+	resp := getExpiry(t, app, cookies, "/password-expiry")
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusServiceUnavailable {
+		t.Errorf("status = %d, want 503", resp.StatusCode)
+	}
+}
+
+// An admin reaches the handler and gets the rendered roster. This also
+// exercises the RequireAdmin admit path and the days/show/sort wiring.
+func TestHandlePasswordExpiry_AdminGetsRoster(t *testing.T) {
+	app, cookies := adminApp(t)
+	soon := ldap.PasswordExpiry{Status: ldap.PasswordExpires, At: time.Now().Add(48 * time.Hour)}
+	app.expiryResolver = &fakeExpiryResolver{
+		expiring: []ldap.ExpiringUser{{User: ptr(userWith("alice", true)), Expiry: soon}},
+	}
+
+	resp := getExpiry(t, app, cookies, "/password-expiry?days=14")
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("status = %d, want 200", resp.StatusCode)
+	}
+	body, _ := io.ReadAll(resp.Body)
+	html := string(body)
+	if !strings.Contains(html, "alice") {
+		t.Error("roster should render the expiring account")
+	}
+	if !strings.Contains(html, "expiring within 14 days") {
+		t.Errorf("count label should reflect ?days=14, got body without it")
+	}
+}
+
+// show=all takes the enumeration path and includes never/unknown rows.
+func TestHandlePasswordExpiry_ShowAllIncludesEveryState(t *testing.T) {
+	app, cookies := adminApp(t)
+	app.expiryResolver = &fakeExpiryResolver{
+		users: []ldap.User{userWith("never", true), userWith("quiet", true)},
+		perUser: map[string]ldap.PasswordExpiry{
+			"never": {Status: ldap.PasswordNeverExpires},
+			"quiet": {Status: ldap.PasswordExpiryUnknown},
+		},
+	}
+
+	resp := getExpiry(t, app, cookies, "/password-expiry?show=all")
+	defer func() { _ = resp.Body.Close() }()
+
+	body, _ := io.ReadAll(resp.Body)
+	html := string(body)
+	if !strings.Contains(html, "Never expires") || !strings.Contains(html, "Unknown") {
+		t.Error("show=all should render never-expires and unknown rows")
+	}
+}
+
+// A resolver error surfaces as a server error, not a blank 200.
+func TestHandlePasswordExpiry_ResolverErrorIsHandled(t *testing.T) {
+	app, cookies := adminApp(t)
+	app.expiryResolver = &fakeExpiryResolver{expiringErr: errors.New("ldap down")}
+
+	resp := getExpiry(t, app, cookies, "/password-expiry")
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode == http.StatusOK {
+		t.Errorf("status = %d, want a non-200 error for a resolver failure", resp.StatusCode)
+	}
+}

internal/web/server.go

@@ -34,19 +34,29 @@ import (
 // When ReadonlyUser is not configured, ldapReadonly and ldapCache are nil;
 // all interactive LDAP operations use the logged-in user's own credentials.
 type App struct {
-	ldapConfig    ldap.Config
-	ldapOpts      []ldap.Option       // LDAP client options (TLS, logging)
-	ldapReadonly  *ldap.LDAP          // Service account client (nil when not configured)
-	ldapCache     *ldap_cache.Manager // Background cache (nil when no service account)
-	adminGroupDN  string              // Group whose members may see the password-expiry roster (empty => adminCount only)
-	sessionStore  *session.Store
-	templateCache *TemplateCache
-	csrfHandler   fiber.Handler
-	fiber         *fiber.App
-	rateLimiter   *RateLimiter  // Rate limiter for authentication endpoints
-	stopCacheLog  chan struct{} // Stops periodicCacheLogging goroutine
-	pinnedStore   *PinnedStore  // Per-user pinned-items store (spec §6.5)
-	pinnedDB      *bolt.DB      // Underlying bbolt handle for pinnedStore (nil when in-memory)
+	ldapConfig   ldap.Config
+	ldapOpts     []ldap.Option       // LDAP client options (TLS, logging)
+	ldapReadonly *ldap.LDAP          // Service account client (nil when not configured)
+	ldapCache    *ldap_cache.Manager // Background cache (nil when no service account)
+	adminGroupDN string              // Group whose members may see the password-expiry roster (empty => adminCount only)
+
+	// adminCheck resolves whether a viewer DN is an admin. Nil in production,
+	// where isAdmin is used. Tests inject it to exercise the admit path and
+	// the handler behind RequireAdmin, which is otherwise unreachable because
+	// a DN-addressable admin cache entry cannot be built outside the LDAP
+	// package (User.DN is unexported).
+	adminCheck func(userDN string) bool
+	// expiryResolver overrides ldapReadonly as the roster's data source. Nil in
+	// production; tests inject a fake to drive the handler without a directory.
+	expiryResolver expiryResolver
+	sessionStore   *session.Store
+	templateCache  *TemplateCache
+	csrfHandler    fiber.Handler
+	fiber          *fiber.App
+	rateLimiter    *RateLimiter  // Rate limiter for authentication endpoints
+	stopCacheLog   chan struct{} // Stops periodicCacheLogging goroutine
+	pinnedStore    *PinnedStore  // Per-user pinned-items store (spec §6.5)
+	pinnedDB       *bolt.DB      // Underlying bbolt handle for pinnedStore (nil when in-memory)
 }
 
 // pinnedStorePath returns the bbolt file path for the per-user pinned