fix: address review feedback for PE admin forms

Enforce updateUser API rating ceiling; form elevation tests; db.Config* keys.

internal/web/config.go

@@ -76,6 +76,7 @@ func (s *Server) handleUpdateConfig(c *gin.Context) {
 		return
 	}
 
+	// Best-effort multi-key write (no multi-Set transaction on ConfigRepo).
 	for i := range reqBody.KeyValuePairs {
 		kv := reqBody.KeyValuePairs[i]
 		if !isEditableConfigKey(kv.Key) {

internal/web/pagemodel.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"github.com/renorris/openfsd/internal/auth"
+	"github.com/renorris/openfsd/internal/db"
 	"github.com/renorris/openfsd/pkg/protocol"
 )
 
@@ -165,11 +166,18 @@ func networkRatingLabel(val int) string {
 	}
 }
 
-// allRatingOptions returns every network rating for select elements.
-func allRatingOptions(selected int) []ratingOption {
-	vals := []int{-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}
-	out := make([]ratingOption, 0, len(vals))
-	for _, v := range vals {
+// ratingOptionsUpTo returns network rating select options from Inactive (−1)
+// through maxInclusive (clamped to Administrator). Actors only see ratings
+// they are allowed to assign (server still enforces the ceiling).
+func ratingOptionsUpTo(maxInclusive int, selected int) []ratingOption {
+	if maxInclusive > int(protocol.NetworkRatingAdministator) {
+		maxInclusive = int(protocol.NetworkRatingAdministator)
+	}
+	if maxInclusive < int(protocol.NetworkRatingInactive) {
+		maxInclusive = int(protocol.NetworkRatingInactive)
+	}
+	out := make([]ratingOption, 0, maxInclusive-int(protocol.NetworkRatingInactive)+1)
+	for v := int(protocol.NetworkRatingInactive); v <= maxInclusive; v++ {
 		out = append(out, ratingOption{
 			Value:    v,
 			Label:    networkRatingLabel(v),
@@ -180,6 +188,7 @@ func allRatingOptions(selected int) []ratingOption {
 }
 
 // editableConfigKeys is the allowlist of config keys shown/mutated via the form UI.
+// Keys use db.Config* constants so the form allowlist cannot drift from the repository.
 var editableConfigKeys = []struct {
 	Key         string
 	Label       string
@@ -187,31 +196,31 @@ var editableConfigKeys = []struct {
 	Placeholder string
 }{
 	{
-		Key:         "WELCOME_MESSAGE",
+		Key:         db.ConfigWelcomeMessage,
 		Label:       "Welcome Message",
 		Description: "Welcome message sent to FSD clients after they connect",
 		Placeholder: "Welcome to my FSD server!",
 	},
 	{
-		Key:         "FSD_SERVER_HOSTNAME",
+		Key:         db.ConfigFsdServerHostname,
 		Label:       "FSD Server Hostname",
 		Description: "Server hostname advertised to clients",
 		Placeholder: "myfsdserver.com",
 	},
 	{
-		Key:         "FSD_SERVER_IDENT",
+		Key:         db.ConfigFsdServerIdent,
 		Label:       "FSD Server Ident",
 		Description: "Server ident advertised to clients",
 		Placeholder: "MY-FSD-SERVER",
 	},
 	{
-		Key:         "FSD_SERVER_LOCATION",
+		Key:         db.ConfigFsdServerLocation,
 		Label:       "FSD Server Location",
 		Description: "Geographical server location advertised to clients",
 		Placeholder: "East US",
 	},
 	{
-		Key:         "API_SERVER_BASE_URL",
+		Key:         db.ConfigApiServerBaseURL,
 		Label:       "API Server Base URL",
 		Description: "API server base URL advertised to clients",
 		Placeholder: "https://example.com",

internal/web/pages_config.go

@@ -47,12 +47,6 @@ func (s *Server) handleFrontendConfigEditor(c *gin.Context) {
 	switch c.Query("flash") {
 	case "saved":
 		page.FlashSuccess = "Configuration saved"
-	case "secret_reset":
-		page.FlashSuccess = "JWT secret key reset. All sessions and API tokens are invalidated."
-	case "token_created":
-		// Token value is not put in the query string (too long / sensitive in logs).
-		// The create-token POST re-renders with CreatedToken instead of redirecting.
-		page.FlashSuccess = "API token created"
 	}
 	s.writeTemplate(c, "configeditor", page)
 }
@@ -79,6 +73,9 @@ func (s *Server) handleFrontendConfigUpdate(c *gin.Context) {
 		page.Fields[i].Value = c.PostForm("cfg_" + key)
 	}
 
+	// Best-effort multi-key save: ConfigRepo has no multi-Set transaction, so a
+	// mid-loop failure can leave earlier keys already written. Keys are allowlisted
+	// and low-risk; operators can re-submit the form to retry.
 	for i := range page.Fields {
 		f := page.Fields[i]
 		if !isEditableConfigKey(f.Key) {

internal/web/pages_user.go

@@ -14,18 +14,31 @@ import (
 
 func (s *Server) newUserEditorPage(c *gin.Context) userEditorPage {
 	claims := getJwtContext(c)
+	maxRating := int(claims.NetworkRating)
+	defaultRating := int(protocol.NetworkRatingObserver)
+	if defaultRating > maxRating {
+		defaultRating = maxRating
+	}
 	return userEditorPage{
 		basePage: basePage{
 			User:      pageUserFromClaims(claims),
 			CSRFToken: s.issueCSRFToken(c),
 		},
 		Create: userForm{
-			NetworkRating: int(protocol.NetworkRatingObserver),
+			NetworkRating: defaultRating,
 		},
-		RatingOptions: allRatingOptions(int(protocol.NetworkRatingObserver)),
+		// Only offer ratings the actor may assign (server still enforces ceiling).
+		RatingOptions: ratingOptionsUpTo(maxRating, defaultRating),
 	}
 }
 
+func actorMaxRating(page *userEditorPage) int {
+	if page.User == nil {
+		return int(protocol.NetworkRatingObserver)
+	}
+	return page.User.NetworkRating
+}
+
 // handleFrontendUserEditor renders the supervisor user editor.
 // GET /usereditor?cid=N loads that user into the edit form (server-rendered).
 func (s *Server) handleFrontendUserEditor(c *gin.Context) {
@@ -68,7 +81,9 @@ func (s *Server) loadUserIntoEditForm(page *userEditorPage, cidStr string) {
 		LastName:      safeStr(user.LastName),
 		NetworkRating: user.NetworkRating,
 	}
-	page.RatingOptions = allRatingOptions(user.NetworkRating)
+	// Keep create/edit selects capped at actor max; selected value is the loaded rating
+	// (may appear only via value compare in template if above max — rare for higher targets).
+	page.RatingOptions = ratingOptionsUpTo(actorMaxRating(page), user.NetworkRating)
 }
 
 // handleFrontendUserCreate processes POST /usereditor/create (no-JS form path).
@@ -90,15 +105,17 @@ func (s *Server) handleFrontendUserCreate(c *gin.Context) {
 	page.Create.LastName = lastName
 	page.Create.Password = "" // never re-render password
 
+	maxRating := int(claims.NetworkRating)
 	rating, err := strconv.Atoi(ratingStr)
 	if err != nil {
 		page.Create.RatingError = "Select a network rating"
 		page.Create.NetworkRating = int(protocol.NetworkRatingObserver)
+		page.RatingOptions = ratingOptionsUpTo(maxRating, page.Create.NetworkRating)
 		s.writeTemplate(c, "usereditor", page)
 		return
 	}
 	page.Create.NetworkRating = rating
-	page.RatingOptions = allRatingOptions(rating)
+	page.RatingOptions = ratingOptionsUpTo(maxRating, rating)
 
 	if len(password) < 8 {
 		page.Create.PasswordError = "Password must be at least 8 characters"
@@ -115,7 +132,7 @@ func (s *Server) handleFrontendUserCreate(c *gin.Context) {
 		s.writeTemplate(c, "usereditor", page)
 		return
 	}
-	if claims.NetworkRating < protocol.NetworkRatingSupervisor || rating > int(claims.NetworkRating) {
+	if claims.NetworkRating < protocol.NetworkRatingSupervisor || rating > maxRating {
 		page.Create.Error = "You cannot create a user with that rating"
 		s.writeTemplate(c, "usereditor", page)
 		return
@@ -175,16 +192,17 @@ func (s *Server) handleFrontendUserUpdate(c *gin.Context) {
 		return
 	}
 
+	maxRating := int(claims.NetworkRating)
 	rating, err := strconv.Atoi(ratingStr)
 	if err != nil || rating < -1 || rating > 12 {
 		page.Edit.RatingError = "Invalid network rating"
 		page.Edit.NetworkRating = int(protocol.NetworkRatingObserver)
-		page.RatingOptions = allRatingOptions(page.Edit.NetworkRating)
+		page.RatingOptions = ratingOptionsUpTo(maxRating, page.Edit.NetworkRating)
 		s.writeTemplate(c, "usereditor", page)
 		return
 	}
 	page.Edit.NetworkRating = rating
-	page.RatingOptions = allRatingOptions(rating)
+	page.RatingOptions = ratingOptionsUpTo(maxRating, rating)
 
 	if password != "" {
 		if len(password) < 8 {

internal/web/pe_admin_test.go

@@ -407,3 +407,180 @@ func TestConfigUpdateViaAPIStillRequiresAuth(t *testing.T) {
 		t.Fatalf("status %d want 401, body %s", w.Code, body)
 	}
 }
+
+// TestAPIUpdateUserCannotElevateRatingAboveActor is the Issue 1 regression:
+// supervisor cannot PATCH an observer to Administrator via the JSON API.
+func TestAPIUpdateUserCannotElevateRatingAboveActor(t *testing.T) {
+	ts := newTestServer(t)
+	sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor))
+	target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver))
+
+	// Obtain Bearer access token
+	loginBody := `{"cid":` + itoa(sup.CID) + `,"password":"sup-pass"}`
+	req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/login", strings.NewReader(loginBody))
+	req.Header.Set("Content-Type", "application/json")
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusOK {
+		t.Fatalf("auth login %d %s", w.Code, w.Body.String())
+	}
+	respBody := w.Body.String()
+	marker := `"access_token":"`
+	i := strings.Index(respBody, marker)
+	if i < 0 {
+		t.Fatalf("no access token in %s", respBody)
+	}
+	rest := respBody[i+len(marker):]
+	j := strings.Index(rest, `"`)
+	token := rest[:j]
+
+	// Attempt privilege escalation: set rating to Administrator (12)
+	payload := `{"cid":` + itoa(target.CID) + `,"network_rating":12}`
+	req = httptest.NewRequest(http.MethodPatch, "/api/v1/user/update", strings.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Authorization", "Bearer "+token)
+	w = httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusForbidden {
+		t.Fatalf("elevate via API status %d want 403, body %s", w.Code, w.Body.String())
+	}
+
+	// DB must be unchanged
+	u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if u.NetworkRating != int(protocol.NetworkRatingObserver) {
+		t.Fatalf("target rating = %d, want still Observer", u.NetworkRating)
+	}
+}
+
+// Cookie+CSRF path of the same elevation hole.
+func TestAPIUpdateUserCannotElevateViaCookieSession(t *testing.T) {
+	ts := newTestServer(t)
+	sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor))
+	target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver))
+	cookies := formLogin(t, ts, sup.CID, "sup-pass")
+
+	// Ensure CSRF cookie
+	_, cookies = authedGET(t, ts, "/dashboard", cookies)
+	csrf := csrfFromCookies(cookies)
+	if csrf == "" {
+		t.Fatal("missing csrf after dashboard")
+	}
+
+	payload := `{"cid":` + itoa(target.CID) + `,"network_rating":12}`
+	req := httptest.NewRequest(http.MethodPatch, "/api/v1/user/update", strings.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	req.Header.Set(csrfHeaderName, csrf)
+	// No Authorization — cookie session only
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusForbidden {
+		t.Fatalf("cookie elevate status %d want 403, body %s", w.Code, w.Body.String())
+	}
+
+	u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if u.NetworkRating != int(protocol.NetworkRatingObserver) {
+		t.Fatalf("target rating = %d after cookie elevate attempt", u.NetworkRating)
+	}
+}
+
+func TestSupervisorCannotCreateAdminViaForm(t *testing.T) {
+	ts := newTestServer(t)
+	sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor))
+	cookies := formLogin(t, ts, sup.CID, "sup-pass")
+
+	// UI should not offer Administrator option for supervisor
+	w, cookies := authedGET(t, ts, "/usereditor", cookies)
+	if w.Code != http.StatusOK {
+		t.Fatalf("GET usereditor %d", w.Code)
+	}
+	body := w.Body.String()
+	if strings.Contains(body, `value="12"`) {
+		t.Fatal("supervisor usereditor should not list Administrator (12) option")
+	}
+
+	form := url.Values{}
+	form.Set("first_name", "Elevated")
+	form.Set("password", "password99")
+	form.Set("network_rating", "12") // forced POST bypassing UI
+	w, _ = formPOST(t, ts, "/usereditor/create", form, cookies)
+	// Server re-renders with error (200), not redirect success
+	if w.Code == http.StatusSeeOther {
+		t.Fatalf("create admin must not redirect success, Location=%s", w.Header().Get("Location"))
+	}
+	if w.Code != http.StatusOK {
+		t.Fatalf("status %d want 200 re-render", w.Code)
+	}
+	if !strings.Contains(w.Body.String(), "You cannot create a user with that rating") {
+		t.Fatalf("expected rating ceiling error, body=%s", clip(w.Body.String(), 500))
+	}
+}
+
+func TestSupervisorCannotPromoteToAdminViaForm(t *testing.T) {
+	ts := newTestServer(t)
+	sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor))
+	target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver))
+	cookies := formLogin(t, ts, sup.CID, "sup-pass")
+
+	form := url.Values{}
+	form.Set("cid", itoa(target.CID))
+	form.Set("first_name", "Still")
+	form.Set("last_name", "Observer")
+	form.Set("network_rating", "12")
+	form.Set("password", "")
+	w, _ := formPOST(t, ts, "/usereditor/update", form, cookies)
+	if w.Code == http.StatusSeeOther {
+		t.Fatalf("promote to admin must not succeed, Location=%s", w.Header().Get("Location"))
+	}
+	if w.Code != http.StatusOK {
+		t.Fatalf("status %d want 200 re-render", w.Code)
+	}
+	if !strings.Contains(w.Body.String(), "Cannot set rating above your own") {
+		t.Fatalf("expected ceiling error, body=%s", clip(w.Body.String(), 500))
+	}
+
+	u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if u.NetworkRating != int(protocol.NetworkRatingObserver) {
+		t.Fatalf("DB rating = %d, want Observer still", u.NetworkRating)
+	}
+}
+
+func TestSupervisorCannotUpdateHigherRatedUserViaForm(t *testing.T) {
+	ts := newTestServer(t)
+	// Create admin first so CID ordering is fine; login as supervisor
+	admin := createTestUser(t, ts, "admin-pass", int(protocol.NetworkRatingAdministator))
+	sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor))
+	cookies := formLogin(t, ts, sup.CID, "sup-pass")
+
+	form := url.Values{}
+	form.Set("cid", itoa(admin.CID))
+	form.Set("first_name", "Hacked")
+	form.Set("last_name", "Admin")
+	form.Set("network_rating", "11")
+	form.Set("password", "")
+	w, _ := formPOST(t, ts, "/usereditor/update", form, cookies)
+	if w.Code == http.StatusSeeOther {
+		t.Fatalf("must not update higher-rated user, Location=%s", w.Header().Get("Location"))
+	}
+	body := w.Body.String()
+	if !strings.Contains(body, "Cannot update user with higher network rating") {
+		t.Fatalf("expected higher-target error, body=%s", clip(body, 500))
+	}
+
+	u, err := ts.dbRepo.UserRepo.GetUserByCID(admin.CID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if safeStr(u.FirstName) == "Hacked" {
+		t.Fatal("admin first name must not change")
+	}
+}

internal/web/user.go

@@ -104,7 +104,14 @@ func (s *Server) updateUser(c *gin.Context) {
 	if reqBody.LastName != nil {
 		targetUser.LastName = reqBody.LastName
 	}
+	// Ceiling matches createUser and the no-JS form path: cannot assign a
+	// network_rating above the actor's own rating (privilege escalation).
 	if reqBody.NetworkRating != nil {
+		if *reqBody.NetworkRating > int(claims.NetworkRating) {
+			res := newAPIV1Failure("cannot set rating above your own")
+			writeAPIV1Response(c, http.StatusForbidden, &res)
+			return
+		}
 		targetUser.NetworkRating = *reqBody.NetworkRating
 	}