Fix admin (#20)

* Fix admin

* fix audit

handler/api_v1_clients.go

@@ -405,13 +405,22 @@ func APIDeleteClient(db store.IStore, cw *ConfigWriter) echo.HandlerFunc {
 			return apiBadRequest(c, "Invalid client ID")
 		}
 
+		// capture details before deletion for audit log
+		clientData, err := db.GetClientByID(clientID, model.QRCodeSettings{Enabled: false})
+		if err != nil {
+			return apiNotFound(c, "Client not found")
+		}
+
 		if err := db.DeleteClient(clientID); err != nil {
 			return apiInternalError(c, "Cannot delete client")
 		}
 
 		cw.Trigger()
 		log.Infof("Deleted wireguard client: %s", clientID)
-		auditLogEvent(c, "client.delete", "client", clientID, nil)
+		auditLogEvent(c, "client.delete", "client", clientID, map[string]string{
+			"name":  clientData.Client.Name,
+			"email": clientData.Client.Email,
+		})
 		return c.NoContent(http.StatusNoContent)
 	}
 }

handler/api_v1_clients_test.go

@@ -2029,7 +2029,8 @@ func TestAPIDeleteClient_DBError(t *testing.T) {
 	c.SetParamValues(id)
 	err := APIDeleteClient(db, env.cw)(c)
 	require.NoError(t, err)
-	assert.Equal(t, http.StatusInternalServerError, rec.Code)
+	// errStore fails on GetClientByID (lookup before delete), returns 404
+	assert.Equal(t, http.StatusNotFound, rec.Code)
 }
 
 

handler/api_v1_oidc.go

@@ -111,7 +111,8 @@ func APIHandleOIDCCallback(oidcProvider *OIDCProvider, db store.IStore) echo.Han
 		if errParam := c.QueryParam("error"); errParam != "" {
 			errDesc := c.QueryParam("error_description")
 			log.Errorf("OIDC error: %s - %s", errParam, errDesc)
-			return apiError(c, http.StatusUnauthorized, "OIDC_ERROR", fmt.Sprintf("Authentication failed: %s", errDesc))
+			// return 403 (not 401) to avoid the SPA redirect loop — 401 triggers OIDC login again
+			return apiError(c, http.StatusForbidden, "OIDC_ERROR", fmt.Sprintf("Authentication failed: %s", errDesc))
 		}
 
 		// exchange code for token

handler/api_v1_oidc_test.go

@@ -288,7 +288,7 @@ func TestAPIHandleOIDCCallback_ErrorParam(t *testing.T) {
 		req2.AddCookie(cookie)
 	}
 	env.echo.ServeHTTP(rec2, req2)
-	assert.Equal(t, http.StatusUnauthorized, rec2.Code)
+	assert.Equal(t, http.StatusForbidden, rec2.Code)
 }
 
 func TestAPIHandleOIDCCallback_TokenExchangeFailure(t *testing.T) {

handler/api_v1_users.go

@@ -41,6 +41,28 @@ func APIPatchUserAdmin(db store.IStore) echo.HandlerFunc {
 			return apiNotFound(c, "User not found")
 		}
 
+		// prevent demoting yourself
+		if !body.Admin && username == currentUser(c) {
+			return apiBadRequest(c, "Cannot remove your own admin role")
+		}
+
+		// prevent removing the last admin
+		if !body.Admin && user.Admin {
+			users, err := db.GetUsers()
+			if err != nil {
+				return apiInternalError(c, "Cannot verify admin count")
+			}
+			adminCount := 0
+			for _, u := range users {
+				if u.Admin {
+					adminCount++
+				}
+			}
+			if adminCount <= 1 {
+				return apiBadRequest(c, "Cannot remove the last admin")
+			}
+		}
+
 		user.Admin = body.Admin
 		user.UpdatedAt = time.Now().UTC()
 		if err := db.SaveUser(user); err != nil {

handler/api_v1_users_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/labstack/echo-contrib/session"
 	"github.com/labstack/echo/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -158,7 +159,9 @@ func TestAPIPatchUserAdmin_DemoteSuccess(t *testing.T) {
 	env := setupTestEnv(t)
 	now := time.Now().UTC()
 
+	// need at least 2 admins so demoting one is allowed
 	env.db.SaveUser(model.User{Username: "demoteuser", Email: "demote@test.com", Admin: true, OIDCSub: "sub-demote", CreatedAt: now, UpdatedAt: now})
+	env.db.SaveUser(model.User{Username: "otheradmin", Email: "other@test.com", Admin: true, OIDCSub: "sub-other", CreatedAt: now, UpdatedAt: now})
 
 	body := map[string]interface{}{"admin": false}
 	req, rec := jsonRequest(http.MethodPatch, "/api/v1/users/demoteuser/admin", body)
@@ -253,3 +256,129 @@ func TestAPIPatchUserAdmin_SaveUserFails(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, http.StatusInternalServerError, rec.Code)
 }
+
+// Regression: an admin must not be able to demote themselves
+func TestAPIPatchUserAdmin_CannotDemoteSelf(t *testing.T) {
+	env := setupTestEnv(t)
+
+	// disable the DisableLogin bypass so currentUser reads from session
+	origDisableLogin := util.DisableLogin
+	util.DisableLogin = false
+	defer func() { util.DisableLogin = origDisableLogin }()
+
+	now := time.Now().UTC()
+	env.db.SaveUser(model.User{
+		Username: "selfadmin", Email: "self@test.com", Admin: true,
+		OIDCSub: "sub-self", CreatedAt: now, UpdatedAt: now,
+	})
+	env.db.SaveUser(model.User{
+		Username: "otheradmin", Email: "other@test.com", Admin: true,
+		OIDCSub: "sub-other", CreatedAt: now, UpdatedAt: now,
+	})
+
+	body := map[string]interface{}{"admin": false}
+	req, rec := jsonRequest(http.MethodPatch, "/api/v1/users/selfadmin/admin", body)
+
+	// set up a real session via the Echo router so the session middleware runs
+	env.echo.PATCH("/api/v1/users/:username/admin", func(c echo.Context) error {
+		// write session first, then call the handler
+		sess, _ := session.Get("session", c)
+		sess.Values["username"] = "selfadmin"
+		sess.Values["admin"] = true
+		sess.Values["session_token"] = "tok"
+		sess.Save(c.Request(), c.Response())
+		req.AddCookie(&http.Cookie{Name: "session_token", Value: "tok"})
+		return APIPatchUserAdmin(env.db)(c)
+	})
+	env.echo.ServeHTTP(rec, req)
+
+	assert.Equal(t, http.StatusBadRequest, rec.Code)
+	assert.Contains(t, rec.Body.String(), "Cannot remove your own admin role")
+
+	// verify user is still admin in DB
+	user, _ := env.db.GetUserByName("selfadmin")
+	assert.True(t, user.Admin)
+}
+
+// Regression: cannot demote the last remaining admin
+func TestAPIPatchUserAdmin_CannotDemoteLastAdmin(t *testing.T) {
+	env := setupTestEnv(t)
+	now := time.Now().UTC()
+
+	// only one admin exists
+	env.db.SaveUser(model.User{
+		Username: "onlyadmin", Email: "only@test.com", Admin: true,
+		OIDCSub: "sub-only", CreatedAt: now, UpdatedAt: now,
+	})
+
+	body := map[string]interface{}{"admin": false}
+	req, rec := jsonRequest(http.MethodPatch, "/api/v1/users/onlyadmin/admin", body)
+	c := env.echo.NewContext(req, rec)
+	c.SetParamNames("username")
+	c.SetParamValues("onlyadmin")
+
+	err := APIPatchUserAdmin(env.db)(c)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusBadRequest, rec.Code)
+	assert.Contains(t, rec.Body.String(), "Cannot remove the last admin")
+
+	// verify still admin
+	user, _ := env.db.GetUserByName("onlyadmin")
+	assert.True(t, user.Admin)
+}
+
+// Promoting a non-admin when you're the only admin should work fine
+func TestAPIPatchUserAdmin_PromoteWhenOnlyAdmin(t *testing.T) {
+	env := setupTestEnv(t)
+	now := time.Now().UTC()
+
+	env.db.SaveUser(model.User{
+		Username: "admin1", Email: "a1@test.com", Admin: true,
+		OIDCSub: "sub-a1", CreatedAt: now, UpdatedAt: now,
+	})
+	env.db.SaveUser(model.User{
+		Username: "regular", Email: "reg@test.com", Admin: false,
+		OIDCSub: "sub-reg", CreatedAt: now, UpdatedAt: now,
+	})
+
+	body := map[string]interface{}{"admin": true}
+	req, rec := jsonRequest(http.MethodPatch, "/api/v1/users/regular/admin", body)
+	c := env.echo.NewContext(req, rec)
+	c.SetParamNames("username")
+	c.SetParamValues("regular")
+
+	err := APIPatchUserAdmin(env.db)(c)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, rec.Code)
+
+	user, _ := env.db.GetUserByName("regular")
+	assert.True(t, user.Admin)
+}
+
+// Demoting when 2 admins exist should succeed
+func TestAPIPatchUserAdmin_DemoteWithMultipleAdmins(t *testing.T) {
+	env := setupTestEnv(t)
+	now := time.Now().UTC()
+
+	env.db.SaveUser(model.User{
+		Username: "admin1", Email: "a1@test.com", Admin: true,
+		OIDCSub: "sub-a1", CreatedAt: now, UpdatedAt: now,
+	})
+	env.db.SaveUser(model.User{
+		Username: "admin2", Email: "a2@test.com", Admin: true,
+		OIDCSub: "sub-a2", CreatedAt: now, UpdatedAt: now,
+	})
+
+	body := map[string]interface{}{"admin": false}
+	req, rec := jsonRequest(http.MethodPatch, "/api/v1/users/admin2/admin", body)
+	c := env.echo.NewContext(req, rec)
+	c.SetParamNames("username")
+	c.SetParamValues("admin2")
+
+	err := APIPatchUserAdmin(env.db)(c)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, rec.Code)
+
+	user, _ := env.db.GetUserByName("admin2")
+	assert.False(t, user.Admin)
+}

handler/session.go

@@ -9,6 +9,7 @@ import (
 	"github.com/gorilla/sessions"
 	"github.com/labstack/echo-contrib/session"
 	"github.com/labstack/echo/v4"
+	"github.com/labstack/gommon/log"
 	"github.com/rs/xid"
 )
 
@@ -51,6 +52,7 @@ func isValidSession(c echo.Context) bool {
 	sess, _ := session.Get("session", c)
 	cookie, err := c.Cookie("session_token")
 	if err != nil || sess.Values["session_token"] != cookie.Value {
+		log.Debugf("session invalid: token cookie mismatch (err=%v)", err)
 		return false
 	}
 
@@ -66,6 +68,7 @@ func isValidSession(c echo.Context) bool {
 	expiration := updatedAt + int64(maxAge)
 	now := time.Now().UTC().Unix()
 	if updatedAt > now || expiration < now || createdAt+util.SessionMaxDuration < now {
+		log.Debugf("session invalid: time bounds (updatedAt=%d, expiration=%d, maxDuration=%d, now=%d)", updatedAt, expiration, createdAt+util.SessionMaxDuration, now)
 		return false
 	}
 
@@ -76,6 +79,7 @@ func isValidSession(c echo.Context) bool {
 	uHash, ok := util.DBUsersToCRC32[username]
 	util.DBUsersToCRC32Mutex.RUnlock()
 	if !ok || userHash != uHash {
+		log.Debugf("session invalid: user hash mismatch (user=%s, ok=%v, sessHash=%d, dbHash=%d)", username, ok, userHash, uHash)
 		return false
 	}
 

src/lib/api-client.ts

@@ -1,5 +1,7 @@
 export const API_BASE = "/api/v1";
 
+let redirectingToLogin = false;
+
 export class ApiError extends Error {
   status: number;
   code: string;
@@ -25,8 +27,10 @@ export async function apiFetch<T>(
   });
 
   if (res.status === 401) {
-    // redirect to OIDC login
-    window.location.href = `${API_BASE}/auth/oidc/login`;
+    if (!redirectingToLogin) {
+      redirectingToLogin = true;
+      window.location.href = `${API_BASE}/auth/oidc/login`;
+    }
     throw new ApiError(401, "UNAUTHORIZED", "Not authenticated");
   }