mirror of
https://github.com/renorris/openfsd
synced 2026-08-10 19:36:08 +08:00
web: address review follow-ups for account self-service
Revalidate optionalSession on landing, harden PE assertions for CSRF rotation and soft-delete cookie clear, add I1 sweatbox API + delete CSRF tests, fix sweatbox manual/README authz copy, drop dead permanentDisabled assignment and misleading CSRF re-issue after password change.
This commit is contained in:
@@ -10,13 +10,14 @@ JSON under `/api/v1` for external tools and map polling. First-party UI is a pro
|
||||
| Page | Routes | Authz |
|
||||
|------|--------|-------|
|
||||
| Login | `GET/POST /login`, `POST /logout` | public / session |
|
||||
| Dashboard | `GET /dashboard` | session; **server-rendered connection summary** (table/counts from FSD service). Leaflet map is PE only (`credentials: 'same-origin'`) |
|
||||
| Users (directory) | `GET /usereditor[?q&rating&sort&dir&page&cid&new&flash]`, `POST /usereditor/create`, `POST /usereditor/update` | Instructor1+: directory + rating adjust; Supervisor+: create + name/password. CSRF; URL-owned filters; `dir_*` on POST for PRG |
|
||||
| Dashboard | `GET /dashboard` | any session (OBS+); **server-rendered connection summary** (table/counts from FSD service). Leaflet map is PE only (`credentials: 'same-origin'`) |
|
||||
| Account | `GET /account`, `POST /account/password`, `POST /account/delete` | any session; change password (current required) + soft-delete account (optional hard-delete via `ALLOW_PERMANENT_ACCOUNT_DELETE`, default false). CSRF; password step-up on delete |
|
||||
| Users (directory) | `GET /usereditor[?q&rating&sort&dir&page&cid&new&flash]`, `POST /usereditor/create`, `POST /usereditor/update` | **Supervisor+**; create + name/password + ratings (network ceiling ≤ actor; full pilot scale). CSRF; URL-owned filters; `dir_*` on POST for PRG |
|
||||
| Config editor | `GET/POST /configeditor`, `POST /configeditor/create-token`, `POST /configeditor/reset-secret` | Administrator; CSRF on mutations |
|
||||
| Sweatbox | `GET /sweatbox`, form POSTs under `/sweatbox/*` | Administrator; CSRF on mutations; proxies FSD service HTTP |
|
||||
| Sweatbox | `GET /sweatbox`, form POSTs under `/sweatbox/*` | **Instructor1+**; CSRF on mutations; proxies FSD service HTTP |
|
||||
| Airport editor | `GET /airport-editor`, `POST /airport-editor/download-apt`, `POST /airport-editor/download-air` | Administrator; CSRF on download; **echo-only** (no disk/DB persistence of `.apt`/`.air`) |
|
||||
|
||||
JSON under `/api/v1` remains for external consumers and map polling. Admin mutations work with **cookie + CSRF only** (no `Authorization` header required).
|
||||
JSON under `/api/v1` remains for external consumers and map polling. Session dual-accept mutations work with **cookie + CSRF only** (no `Authorization` header required).
|
||||
|
||||
### Airport editor validation
|
||||
|
||||
@@ -41,7 +42,7 @@ The **airport editor** (`/airport-editor`) is a second complexity-gate exception
|
||||
- `POST /logout` clears the session cookie
|
||||
- Cookie-authenticated API mutations require a CSRF synchronizer token (`csrf_token` form field or `X-CSRF-Token` header matching the `openfsd_csrf` cookie)
|
||||
- Suspended/inactive ratings cannot open a web session (same as FSD policy)
|
||||
- **Rating in the cookie is fixed until expiry.** Demotion/suspension does not revoke existing sessions until `exp` unless the JWT secret is rotated (Configure Server → Reset JWT secret). Prefer shorter TTL if faster revoke is required.
|
||||
- **Session cookies are revalidated against the DB on every use** (HTML + dual-accept API): missing or inactive/suspended certificates clear cookies and are rejected; claims (network rating + names) are overlaid from the DB so demotions take effect immediately. Residual window after soft-delete is **Bearer access tokens only** (15m TTL).
|
||||
|
||||
### Cookie `Secure` flag (`COOKIE_SECURE`)
|
||||
| Condition | Secure |
|
||||
@@ -67,10 +68,10 @@ Authorization: Bearer <access_token>
|
||||
|
||||
## Network Ratings
|
||||
The API enforces role-based access control using `NetworkRating` values defined in `pkg/protocol`. Key thresholds:
|
||||
- **Instructor1–3 (8–10)**: Can open the Users directory and adjust network/pilot ratings up to their own ceilings (any target). Cannot create users or change name/password.
|
||||
- **Supervisor (11)**: Full user mutation (create, name, password) when target network rating ≤ own; rating adjust as above; kick active connections.
|
||||
- **Administrator (12)**: Can manage server configuration, reset JWT secret keys, and create API tokens.
|
||||
- **Suspended (0) / Inactive (-1)**: Cannot log in to the web UI or obtain FSD JWTs.
|
||||
- **Instructor1–3 (8–10)**: Sweatbox instructor UI + JSON proxies. Cannot open the Users directory.
|
||||
- **Supervisor (11)**: User editor (create, name, password, ratings); kick active connections. Network rating assignments capped at own rating; pilot ratings use the full official scale.
|
||||
- **Administrator (12)**: Server configuration, JWT secret reset, API tokens, airport editor.
|
||||
- **Suspended (0) / Inactive (-1)**: Cannot log in to the web UI or obtain FSD JWTs; existing session cookies are rejected on revalidation.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/renorris/openfsd/internal/db"
|
||||
"github.com/renorris/openfsd/internal/serviceapi"
|
||||
"github.com/renorris/openfsd/pkg/protocol"
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -30,6 +31,32 @@ func TestAPISweatboxStateForbiddenForObserver(t *testing.T) {
|
||||
require.Equal(t, http.StatusForbidden, w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
func TestAPISweatboxStateAllowedForInstructor(t *testing.T) {
|
||||
m := &sweatboxMock{}
|
||||
fsd := httptest.NewServer(m.handler())
|
||||
t.Cleanup(fsd.Close)
|
||||
|
||||
env := setupTestAPI(t)
|
||||
env.server.cfg.FsdHttpServiceAddress = fsd.URL
|
||||
|
||||
i1Pass := "i1pass123"
|
||||
i1 := &db.User{
|
||||
Password: i1Pass,
|
||||
FirstName: strPtr("Inst"),
|
||||
LastName: strPtr("One"),
|
||||
NetworkRating: int(protocol.NetworkRatingInstructor1),
|
||||
}
|
||||
require.NoError(t, env.server.dbRepo.UserRepo.CreateUser(i1))
|
||||
|
||||
access, _ := env.login(t, i1.CID, i1Pass)
|
||||
w := env.doJSON(t, http.MethodGet, "/api/v1/sweatbox/state", nil, access)
|
||||
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
|
||||
|
||||
var st serviceapi.SweatboxStateJSON
|
||||
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &st))
|
||||
assert.Equal(t, "KBTV", st.ICAO)
|
||||
}
|
||||
|
||||
func TestAPISweatboxStateProxiesFSD(t *testing.T) {
|
||||
m := &sweatboxMock{}
|
||||
fsd := httptest.NewServer(m.handler())
|
||||
|
||||
@@ -111,7 +111,8 @@ func (s *Server) handleFrontendAccountPassword(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
// KD-7: re-issue session with rememberMe=false (24h); rotate CSRF.
|
||||
// KD-7: re-issue session with rememberMe=false (24h); clear CSRF so the
|
||||
// following GET re-issues a fresh token (issueCSRFToken reuses request cookies).
|
||||
if err := s.setSessionCookie(c, user, false); err != nil {
|
||||
slog.Error("account password session reissue failed", "cid", claims.CID, "err", err)
|
||||
page.FormError = "Password updated but session could not be refreshed; please log in again"
|
||||
@@ -119,7 +120,6 @@ func (s *Server) handleFrontendAccountPassword(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
s.clearCSRFCookie(c)
|
||||
s.issueCSRFToken(c)
|
||||
|
||||
slog.Info("account password changed",
|
||||
"cid", claims.CID,
|
||||
@@ -200,7 +200,6 @@ func (s *Server) handleFrontendAccountDelete(c *gin.Context) {
|
||||
"event", "account_deleted",
|
||||
"mode", "soft",
|
||||
)
|
||||
_ = permanentDisabled
|
||||
}
|
||||
|
||||
s.clearSessionCookie(c)
|
||||
|
||||
@@ -88,22 +88,33 @@ func TestChangePasswordSuccess(t *testing.T) {
|
||||
t.Fatal("expected session Set-Cookie on password change")
|
||||
}
|
||||
}
|
||||
// CSRF rotated (clear + new issue → different value from old)
|
||||
newCSRF := csrfFromCookies(cookies)
|
||||
if newCSRF == "" {
|
||||
// May need GET after rotate
|
||||
w2, cookies2 := authedGET(t, ts, "/account", cookies)
|
||||
_ = w2
|
||||
newCSRF = csrfFromCookies(cookies2)
|
||||
cookies = cookies2
|
||||
// CSRF cleared on password change response; next GET re-issues a fresh token.
|
||||
clearedCSRF := false
|
||||
for _, sc := range w.Result().Header.Values("Set-Cookie") {
|
||||
if strings.HasPrefix(sc, csrfCookieName+"=") &&
|
||||
(strings.Contains(sc, "Max-Age=0") || strings.Contains(sc, "Max-Age=-1") || strings.Contains(sc, csrfCookieName+"=;")) {
|
||||
clearedCSRF = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !clearedCSRF {
|
||||
// mergeCookies drops empty/cleared values; post-merge CSRF must not equal old.
|
||||
if csrfFromCookies(cookies) == oldCSRF && oldCSRF != "" {
|
||||
t.Fatal("expected CSRF clear/rotation after password change")
|
||||
}
|
||||
}
|
||||
w2, cookies2 := authedGET(t, ts, "/account", cookies)
|
||||
if w2.Code != http.StatusOK {
|
||||
t.Fatalf("GET /account after password change: %d", w2.Code)
|
||||
}
|
||||
newCSRF := csrfFromCookies(cookies2)
|
||||
if newCSRF == "" {
|
||||
t.Fatal("expected CSRF after password change")
|
||||
t.Fatal("expected CSRF after password change follow-up GET")
|
||||
}
|
||||
if oldCSRF != "" && newCSRF == oldCSRF {
|
||||
// Rotation: clearCSRF then issueCSRFToken on same response may set empty then new.
|
||||
// Accept if session still works with new CSRF.
|
||||
t.Fatal("CSRF token must change after password change (KD-7)")
|
||||
}
|
||||
cookies = cookies2
|
||||
|
||||
// Old password fails login
|
||||
csrf, loginCookies := getLoginCSRF(t, ts)
|
||||
@@ -225,6 +236,33 @@ func TestChangePasswordCSRF(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteAccountCSRF(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingObserver))
|
||||
cookies := formLogin(t, ts, user.CID, "pw")
|
||||
|
||||
form := url.Values{}
|
||||
form.Set("current_password", "pw")
|
||||
form.Set("confirm_cid", itoa(user.CID))
|
||||
form.Set("csrf_token", "not-the-real-token")
|
||||
req := httptest.NewRequest(http.MethodPost, "/account/delete", strings.NewReader(form.Encode()))
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
req.Header.Set("Cookie", cookieHeader(cookies))
|
||||
w := httptest.NewRecorder()
|
||||
ts.engine.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Fatalf("status %d want 403", w.Code)
|
||||
}
|
||||
// Account must not be deleted
|
||||
u, err := ts.dbRepo.UserRepo.GetUserByCID(user.CID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if u.NetworkRating != int(protocol.NetworkRatingObserver) {
|
||||
t.Fatalf("rating=%d want OBS after CSRF reject", u.NetworkRating)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteAccountSoft(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
user := createTestUser(t, ts, "delete-me1", int(protocol.NetworkRatingObserver))
|
||||
@@ -390,18 +428,33 @@ func TestSessionRejectedAfterSoftDelete(t *testing.T) {
|
||||
if loc := w.Header().Get("Location"); loc != "/login" {
|
||||
t.Fatalf("Location=%q want /login", loc)
|
||||
}
|
||||
// Session cookie cleared
|
||||
// Session cookie cleared on the soft-delete reject response.
|
||||
cleared := false
|
||||
for _, sc := range w.Result().Header.Values("Set-Cookie") {
|
||||
if strings.HasPrefix(sc, sessionCookieName+"=") &&
|
||||
(strings.Contains(sc, "Max-Age=0") || strings.Contains(sc, "Max-Age=-1")) {
|
||||
cleared = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !cleared {
|
||||
// mergeCookies should drop empty/cleared
|
||||
if csrfFromCookies(cookies2) != "" || extractCookie(w.Result(), sessionCookieName) != "" {
|
||||
// extractCookie may still return empty value for cleared cookie
|
||||
t.Fatal("expected session cookie Max-Age=0/-1 clear after soft-delete revalidation")
|
||||
}
|
||||
if extractCookie(w.Result(), sessionCookieName) != "" {
|
||||
// extractCookie returns the value part; empty value is OK for clear.
|
||||
val := extractCookie(w.Result(), sessionCookieName)
|
||||
if val != "" {
|
||||
// Some clients may still parse Max-Age=-1 with empty value only.
|
||||
for _, sc := range w.Result().Header.Values("Set-Cookie") {
|
||||
if strings.HasPrefix(sc, sessionCookieName+"=") &&
|
||||
(strings.Contains(sc, "Max-Age=0") || strings.Contains(sc, "Max-Age=-1")) {
|
||||
val = ""
|
||||
break
|
||||
}
|
||||
}
|
||||
if val != "" {
|
||||
t.Fatalf("session cookie still has value after clear: %q", val)
|
||||
}
|
||||
}
|
||||
}
|
||||
// Follow-up without re-login fails
|
||||
|
||||
@@ -163,11 +163,20 @@ func (s *Server) parseSessionCookie(c *gin.Context) (*auth.CustomClaims, error)
|
||||
}
|
||||
|
||||
// optionalSession loads session claims into the gin context when present (no redirect).
|
||||
// Revalidates against the DB (KD-9): inactive/missing users get cookies cleared and
|
||||
// are treated as signed-out so landing does not show stale elevated nav.
|
||||
func (s *Server) optionalSession(c *gin.Context) *auth.CustomClaims {
|
||||
claims, err := s.parseSessionCookie(c)
|
||||
if err != nil {
|
||||
return nil
|
||||
}
|
||||
claims, user, err := s.revalidateSessionFromDB(claims)
|
||||
if err != nil {
|
||||
s.clearSessionCookie(c)
|
||||
s.clearCSRFCookie(c)
|
||||
return nil
|
||||
}
|
||||
setJwtContext(c, claims)
|
||||
c.Set(dbUserContextKey, user)
|
||||
return claims
|
||||
}
|
||||
|
||||
@@ -111,7 +111,7 @@
|
||||
<section id="workflow">
|
||||
<h2>4. Typical workflow</h2>
|
||||
<ol>
|
||||
<li>Enable sweatbox on FSD and open <code>/sweatbox</code> as Administrator.</li>
|
||||
<li>Enable sweatbox on FSD and open <code>/sweatbox</code> as Instructor1+.</li>
|
||||
<li>
|
||||
Load an <strong>airport</strong> (<code>.apt</code>). Use
|
||||
<a href="/airport-editor">Airport Editor</a> to author/download files if needed
|
||||
|
||||
Reference in New Issue
Block a user