From 2b36d33b8914cd8cf1ee1182b342a9b00ab992c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnter=20Grodotzki?= Date: Thu, 23 Apr 2026 23:24:24 +0200 Subject: [PATCH] Fix admin (#20) * Fix admin * fix audit --- handler/api_v1_clients.go | 11 ++- handler/api_v1_clients_test.go | 3 +- handler/api_v1_oidc.go | 3 +- handler/api_v1_oidc_test.go | 2 +- handler/api_v1_users.go | 22 ++++++ handler/api_v1_users_test.go | 129 +++++++++++++++++++++++++++++++++ handler/session.go | 4 + src/lib/api-client.ts | 8 +- 8 files changed, 176 insertions(+), 6 deletions(-) diff --git a/handler/api_v1_clients.go b/handler/api_v1_clients.go index f20758a..250a5db 100644 --- a/handler/api_v1_clients.go +++ b/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) } } diff --git a/handler/api_v1_clients_test.go b/handler/api_v1_clients_test.go index d4fa15d..03e8625 100644 --- a/handler/api_v1_clients_test.go +++ b/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) } diff --git a/handler/api_v1_oidc.go b/handler/api_v1_oidc.go index 71e97b7..34c9ada 100644 --- a/handler/api_v1_oidc.go +++ b/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 diff --git a/handler/api_v1_oidc_test.go b/handler/api_v1_oidc_test.go index 940ac5f..d86863a 100644 --- a/handler/api_v1_oidc_test.go +++ b/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) { diff --git a/handler/api_v1_users.go b/handler/api_v1_users.go index 7312790..93c95ca 100644 --- a/handler/api_v1_users.go +++ b/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 { diff --git a/handler/api_v1_users_test.go b/handler/api_v1_users_test.go index 954e91f..3211ff2 100644 --- a/handler/api_v1_users_test.go +++ b/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) +} diff --git a/handler/session.go b/handler/session.go index f5636f9..34a0dda 100644 --- a/handler/session.go +++ b/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 } diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index b703204..0363cda 100644 --- a/src/lib/api-client.ts +++ b/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( }); 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"); }