fix: address review feedback for user directory JSON API

I1/SUP authz boundary tests, empty-result envelope, sort-order
assertions, deterministic list golden, password-key checks, API-only
page_size parse, OpenAPI coerce docs, updateUser via apiUserFromDB.
Rebased onto PR-2 Bearer revalidation (897f514).
This commit is contained in:
Reese Norris
2026-07-28 11:59:08 -04:00
parent 00e1038f60
commit 29ff4d0ae1
6 changed files with 246 additions and 67 deletions

View File

@@ -53,7 +53,7 @@ func (s *Server) handleAPIListUsers(c *gin.Context) {
return
}
q := parseUserDirectoryQuery(c.Request.URL.Query())
q := parseUserDirectoryQueryAPI(c.Request.URL.Query())
filter := db.UserListFilter{
Query: q.Q,

View File

@@ -17,6 +17,27 @@ func TestAPIListUsers_Authz(t *testing.T) {
adminAccess, _ := env.login(t, env.admin.CID, env.adminPass)
obsAccess, _ := env.login(t, env.observer.CID, env.observerPass)
// Critical SUP+ boundary: Instructor1 forbidden; true Supervisor allowed.
i1Pass := "i1pass1234"
i1 := &db.User{
Password: i1Pass,
FirstName: strPtr("Inst"),
LastName: strPtr("One"),
NetworkRating: int(protocol.NetworkRatingInstructor1),
}
require.NoError(t, env.server.dbRepo.UserRepo.CreateUser(i1))
i1Access, _ := env.login(t, i1.CID, i1Pass)
supPass := "suppass123"
sup := &db.User{
Password: supPass,
FirstName: strPtr("Super"),
LastName: strPtr("Visor"),
NetworkRating: int(protocol.NetworkRatingSupervisor),
}
require.NoError(t, env.server.dbRepo.UserRepo.CreateUser(sup))
supAccess, _ := env.login(t, sup.CID, supPass)
t.Run("unauthenticated", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users", nil, "")
assert.Equal(t, http.StatusUnauthorized, w.Code)
@@ -30,6 +51,24 @@ func TestAPIListUsers_Authz(t *testing.T) {
assert.Equal(t, "forbidden", *res.Err)
})
t.Run("instructor1 forbidden", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users", nil, i1Access)
assert.Equal(t, http.StatusForbidden, w.Code, w.Body.String())
})
t.Run("supervisor ok", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users", nil, supAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
res := decodeAPIV1(t, w)
require.Nil(t, res.Err)
data := decodeUserListData(t, res)
assert.GreaterOrEqual(t, data.Total, 2)
assert.Equal(t, 1, data.Page)
assert.Equal(t, 50, data.PageSize)
assert.NotEmpty(t, data.Items)
assertItemsNoPassword(t, w.Body.Bytes())
})
t.Run("admin ok", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users", nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
@@ -43,10 +82,7 @@ func TestAPIListUsers_Authz(t *testing.T) {
assert.Equal(t, 50, data.PageSize)
assert.GreaterOrEqual(t, data.Pages, 1)
assert.NotEmpty(t, data.Items)
// No password field on items (json omits unknown; ensure public fields present).
for _, it := range data.Items {
assert.GreaterOrEqual(t, it.CID, 1)
}
assertItemsNoPassword(t, w.Body.Bytes())
})
}
@@ -54,12 +90,26 @@ func TestAPIListUsers_PaginationAndFilters(t *testing.T) {
env := setupTestAPI(t)
adminAccess, _ := env.login(t, env.admin.CID, env.adminPass)
// Seed enough users for multi-page lists.
for i := 0; i < 8; i++ {
// Seed named users for multi-page lists and sort-order checks.
// Names chosen so first_name order is Charlie < Alpha is wrong → Alpha, Bravo, Charlie.
type seed struct {
first, last string
}
seeds := []seed{
{"Alpha", "User"},
{"Bravo", "User"},
{"Charlie", "User"},
{"Delta", "User"},
{"Echo", "User"},
{"Foxtrot", "User"},
{"Golf", "User"},
{"Hotel", "User"},
}
for _, s := range seeds {
u := &db.User{
Password: "seedpass1",
FirstName: strPtr(fmt.Sprintf("Seed%d", i)),
LastName: strPtr("User"),
FirstName: strPtr(s.first),
LastName: strPtr(s.last),
NetworkRating: int(protocol.NetworkRatingObserver),
PilotRating: 0,
}
@@ -102,6 +152,67 @@ func TestAPIListUsers_PaginationAndFilters(t *testing.T) {
assert.GreaterOrEqual(t, data.Total, 2)
})
t.Run("empty result envelope", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users?q=zzznomatch999&page=5&page_size=25", nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
res := decodeAPIV1(t, w)
require.Nil(t, res.Err)
// Raw envelope: items must be [] not null.
var envelope map[string]any
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &envelope))
dataMap, ok := envelope["data"].(map[string]any)
require.True(t, ok)
items, ok := dataMap["items"].([]any)
require.True(t, ok, "items must be a JSON array, got %T", dataMap["items"])
assert.Empty(t, items)
data := decodeUserListData(t, res)
assert.Equal(t, 0, data.Total)
assert.Equal(t, 1, data.Page, "page clamps to 1 when total=0")
assert.Equal(t, 1, data.Pages)
assert.Equal(t, 25, data.PageSize)
assert.NotNil(t, data.Items)
assert.Empty(t, data.Items)
})
t.Run("sort cid desc order", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users?sort=cid&dir=desc&page_size=5", nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
data := decodeUserListData(t, decodeAPIV1(t, w))
require.GreaterOrEqual(t, len(data.Items), 2)
for i := 1; i < len(data.Items); i++ {
assert.GreaterOrEqual(t, data.Items[i-1].CID, data.Items[i].CID,
"cid desc: items[%d].CID=%d items[%d].CID=%d", i-1, data.Items[i-1].CID, i, data.Items[i].CID)
}
})
t.Run("sort name asc order", func(t *testing.T) {
// Filter to seeded *User last names so Admin/Obs do not interleave unpredictably.
w := env.doJSON(t, http.MethodGet, "/api/v1/users?q=User&sort=name&dir=asc&page_size=20", nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
data := decodeUserListData(t, decodeAPIV1(t, w))
require.GreaterOrEqual(t, len(data.Items), 3)
for i := 1; i < len(data.Items); i++ {
prev := data.Items[i-1].FirstName + " " + data.Items[i-1].LastName
cur := data.Items[i].FirstName + " " + data.Items[i].LastName
assert.LessOrEqual(t, prev, cur, "name asc order broken at %d: %q > %q", i, prev, cur)
}
})
t.Run("invalid sort falls back to cid order", func(t *testing.T) {
wNope := env.doJSON(t, http.MethodGet, "/api/v1/users?sort=nope&dir=asc&page_size=10", nil, adminAccess)
wCid := env.doJSON(t, http.MethodGet, "/api/v1/users?sort=cid&dir=asc&page_size=10", nil, adminAccess)
require.Equal(t, http.StatusOK, wNope.Code)
require.Equal(t, http.StatusOK, wCid.Code)
nope := decodeUserListData(t, decodeAPIV1(t, wNope))
cid := decodeUserListData(t, decodeAPIV1(t, wCid))
require.Equal(t, len(cid.Items), len(nope.Items))
for i := range cid.Items {
assert.Equal(t, cid.Items[i].CID, nope.Items[i].CID, "index %d", i)
}
})
t.Run("rating filter exact", func(t *testing.T) {
// Only admin is Administrator (12) among seeds (observers are rating 1).
w := env.doJSON(t, http.MethodGet, "/api/v1/users?rating=12", nil, adminAccess)
@@ -114,17 +225,17 @@ func TestAPIListUsers_PaginationAndFilters(t *testing.T) {
})
t.Run("q filter", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, "/api/v1/users?q=Seed0", nil, adminAccess)
w := env.doJSON(t, http.MethodGet, "/api/v1/users?q=Alpha", nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
data := decodeUserListData(t, decodeAPIV1(t, w))
assert.GreaterOrEqual(t, data.Total, 1)
found := false
for _, it := range data.Items {
if it.FirstName == "Seed0" {
if it.FirstName == "Alpha" {
found = true
}
}
assert.True(t, found, "expected Seed0 in results: %+v", data.Items)
assert.True(t, found, "expected Alpha in results: %+v", data.Items)
})
t.Run("page 1 of page_size 2", func(t *testing.T) {
@@ -143,6 +254,26 @@ func TestAPIGetUser_AuthzAndShape(t *testing.T) {
adminAccess, _ := env.login(t, env.admin.CID, env.adminPass)
obsAccess, _ := env.login(t, env.observer.CID, env.observerPass)
i1Pass := "i1pass1234"
i1 := &db.User{
Password: i1Pass,
FirstName: strPtr("Inst"),
LastName: strPtr("One"),
NetworkRating: int(protocol.NetworkRatingInstructor1),
}
require.NoError(t, env.server.dbRepo.UserRepo.CreateUser(i1))
i1Access, _ := env.login(t, i1.CID, i1Pass)
supPass := "suppass123"
sup := &db.User{
Password: supPass,
FirstName: strPtr("Super"),
LastName: strPtr("Visor"),
NetworkRating: int(protocol.NetworkRatingSupervisor),
}
require.NoError(t, env.server.dbRepo.UserRepo.CreateUser(sup))
supAccess, _ := env.login(t, sup.CID, supPass)
t.Run("unauthenticated", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, fmt.Sprintf("/api/v1/users/%d", env.observer.CID), nil, "")
assert.Equal(t, http.StatusUnauthorized, w.Code)
@@ -158,6 +289,7 @@ func TestAPIGetUser_AuthzAndShape(t *testing.T) {
assert.Equal(t, "Obs", u.FirstName)
assert.Equal(t, "Server", u.LastName)
assert.Equal(t, int(protocol.NetworkRatingObserver), u.NetworkRating)
assertGetNoPassword(t, w.Body.Bytes())
})
t.Run("observer cannot get other", func(t *testing.T) {
@@ -165,6 +297,18 @@ func TestAPIGetUser_AuthzAndShape(t *testing.T) {
assert.Equal(t, http.StatusForbidden, w.Code)
})
t.Run("instructor1 cannot get other", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, fmt.Sprintf("/api/v1/users/%d", env.observer.CID), nil, i1Access)
assert.Equal(t, http.StatusForbidden, w.Code, w.Body.String())
})
t.Run("supervisor can get other", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, fmt.Sprintf("/api/v1/users/%d", env.observer.CID), nil, supAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
u := decodeUserData(t, decodeAPIV1(t, w))
assert.Equal(t, env.observer.CID, u.CID)
})
t.Run("admin can get other", func(t *testing.T) {
w := env.doJSON(t, http.MethodGet, fmt.Sprintf("/api/v1/users/%d", env.observer.CID), nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
@@ -205,9 +349,9 @@ func TestAPIUsers_Goldens(t *testing.T) {
})
t.Run("users_list", func(t *testing.T) {
// Filter to a single known user so the fixture is stable.
// Unique first name "Obs" → exactly one row; golden real pagination fields.
w := env.doJSON(t, http.MethodGet,
fmt.Sprintf("/api/v1/users?q=%d&page_size=10&sort=cid&dir=asc", env.observer.CID),
"/api/v1/users?q=Obs&page_size=10&sort=cid&dir=asc",
nil, adminAccess)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
@@ -217,21 +361,15 @@ func TestAPIUsers_Goldens(t *testing.T) {
require.True(t, ok)
items, ok := dataMap["items"].([]any)
require.True(t, ok)
require.NotEmpty(t, items)
for _, raw := range items {
item, ok := raw.(map[string]any)
require.True(t, ok)
item["cid"] = float64(0)
}
// Stable pagination fields for golden: rewrite total/pages if filter matches exactly one.
dataMap["total"] = float64(1)
dataMap["pages"] = float64(1)
dataMap["page"] = float64(1)
dataMap["page_size"] = float64(10)
// Keep only first item if q matched more than one unexpectedly.
if len(items) > 1 {
dataMap["items"] = items[:1]
}
require.Equal(t, 1, len(items), "q=Obs must match exactly one user: %s", w.Body.String())
// Assert real pagination (do not rewrite total/pages/page/page_size).
assert.Equal(t, float64(1), dataMap["total"])
assert.Equal(t, float64(1), dataMap["pages"])
assert.Equal(t, float64(1), dataMap["page"])
assert.Equal(t, float64(10), dataMap["page_size"])
item, ok := items[0].(map[string]any)
require.True(t, ok)
item["cid"] = float64(0)
rewritten, err := json.Marshal(envelope)
require.NoError(t, err)
assertGoldenJSON(t, "2026-07-28/users_list.json", rewritten)
@@ -255,3 +393,30 @@ func decodeUserData(t *testing.T, res APIV1Response) apiUserData {
require.NoError(t, json.Unmarshal(b, &data), string(b))
return data
}
// assertItemsNoPassword checks raw list envelope items lack a password key.
func assertItemsNoPassword(t *testing.T, body []byte) {
t.Helper()
var envelope map[string]any
require.NoError(t, json.Unmarshal(body, &envelope))
dataMap, ok := envelope["data"].(map[string]any)
require.True(t, ok)
items, ok := dataMap["items"].([]any)
require.True(t, ok)
require.NotEmpty(t, items)
item, ok := items[0].(map[string]any)
require.True(t, ok)
_, has := item["password"]
assert.False(t, has, "list item must not include password key: %v", item)
}
// assertGetNoPassword checks raw get envelope data lacks a password key.
func assertGetNoPassword(t *testing.T, body []byte) {
t.Helper()
var envelope map[string]any
require.NoError(t, json.Unmarshal(body, &envelope))
dataMap, ok := envelope["data"].(map[string]any)
require.True(t, ok)
_, has := dataMap["password"]
assert.False(t, has, "get data must not include password key: %v", dataMap)
}

View File

@@ -267,28 +267,37 @@ paths:
- name: rating
in: query
required: false
description: Exact network rating filter (1…12). Invalid/out of range → all.
schema: { type: integer, minimum: -1, maximum: 12 }
description: |
Exact network rating filter (1…12). Server coerces invalid/out-of-range
values to "all ratings" (does not 400; never 500).
schema: { type: integer }
- name: sort
in: query
required: false
description: "cid | name | rating (default cid; unknown → cid)"
schema: { type: string, enum: [cid, name, rating], default: cid }
description: |
cid | name | rating (default cid). Server coerces unknown values to cid
(does not 400).
schema: { type: string, default: cid }
- name: dir
in: query
required: false
description: "asc | desc (default asc; unknown → asc)"
schema: { type: string, enum: [asc, desc], default: asc }
description: |
asc | desc (default asc). Server coerces unknown values to asc (does not 400).
schema: { type: string, default: asc }
- name: page
in: query
required: false
description: 1-based page (default 1; <1 or non-int → 1; clamped past last page)
schema: { type: integer, minimum: 1, default: 1 }
description: |
1-based page (default 1). Server coerces non-int or &lt;1 → 1; clamps past last
page after count (does not 400).
schema: { type: integer, default: 1 }
- name: page_size
in: query
required: false
description: Page size (default 50; ≤0 → 50; clamp [1, 200])
schema: { type: integer, minimum: 1, maximum: 200, default: 50 }
description: |
Page size (default 50). Server coerces ≤0/non-int → 50 and clamps to [1, 200]
(does not 400). JSON API only; HTML directory is fixed at 50.
schema: { type: integer, default: 50 }
responses:
"200":
description: Directory page (effective page / page_size / pages / total after clamp)

View File

@@ -6,19 +6,20 @@ import (
"strings"
)
// userDirectoryPageSize is the default directory page size (HTML omits page_size).
// userDirectoryPageSize is the fixed HTML directory page size and API default.
const userDirectoryPageSize = 50
// userDirectoryPageSizeMax matches db.UserListFilter hard cap.
const userDirectoryPageSizeMax = 200
// parseUserDirectoryQuery parses and clamps directory GET query parameters.
// Invalid values never 500: bad sort → cid, bad dir → asc, page < 1 → 1,
// rating outside [-1,12] or non-int → all (nil). Page clamping past the last
// page is applied later once Total is known (clampDirectoryPage).
// parseUserDirectoryQuery parses and clamps HTML/shared directory GET query
// parameters. Invalid values never 500: bad sort → cid, bad dir → asc,
// page < 1 → 1, rating outside [-1,12] or non-int → all (nil). Page clamping
// past the last page is applied later once Total is known (clampDirectoryPage).
//
// page_size (JSON API; HTML never sends it): missing/non-int/≤0 → 50;
// clamp to [1, 200] (UserListFilter hard cap).
// PageSize is always the fixed HTML default (50). Optional JSON API page_size
// is applied only via parseUserDirectoryQueryAPI so HTML pager links that omit
// page_size cannot drift after a manual ?page_size= bookmark.
func parseUserDirectoryQuery(values url.Values) userDirectoryQuery {
q := userDirectoryQuery{
Q: strings.TrimSpace(values.Get("q")),
@@ -60,7 +61,14 @@ func parseUserDirectoryQuery(values url.Values) userDirectoryQuery {
}
}
// page_size: optional (JSON directory API). HTML never sends it → stays 50.
return q
}
// parseUserDirectoryQueryAPI is the JSON directory query helper: same as
// parseUserDirectoryQuery plus optional page_size (missing/non-int/≤0 → 50;
// clamp to [1, 200]).
func parseUserDirectoryQueryAPI(values url.Values) userDirectoryQuery {
q := parseUserDirectoryQuery(values)
if psStr := strings.TrimSpace(values.Get("page_size")); psStr != "" {
if ps, err := strconv.Atoi(psStr); err == nil && ps > 0 {
if ps > userDirectoryPageSizeMax {
@@ -70,7 +78,6 @@ func parseUserDirectoryQuery(values url.Values) userDirectoryQuery {
}
// non-int or ≤0 → keep default 50 (never 500)
}
return q
}

View File

@@ -162,7 +162,20 @@ func TestParseUserDirectoryQuery(t *testing.T) {
}
}
func TestParseUserDirectoryQuery_PageSize(t *testing.T) {
func TestParseUserDirectoryQuery_PageSizeHTMLIgnores(t *testing.T) {
t.Parallel()
// HTML helper never applies page_size (fixed 50) so pager links stay consistent.
vals, err := url.ParseQuery("page_size=100")
if err != nil {
t.Fatal(err)
}
got := parseUserDirectoryQuery(vals)
if got.PageSize != userDirectoryPageSize {
t.Errorf("HTML PageSize=%d want %d", got.PageSize, userDirectoryPageSize)
}
}
func TestParseUserDirectoryQueryAPI_PageSize(t *testing.T) {
t.Parallel()
tests := []struct {
name string
@@ -186,7 +199,7 @@ func TestParseUserDirectoryQuery_PageSize(t *testing.T) {
if err != nil {
t.Fatalf("ParseQuery: %v", err)
}
got := parseUserDirectoryQuery(vals)
got := parseUserDirectoryQueryAPI(vals)
if got.PageSize != tt.want {
t.Errorf("PageSize=%d want %d", got.PageSize, tt.want)
}

View File

@@ -135,23 +135,8 @@ func (s *Server) updateUser(c *gin.Context) {
return
}
type ResponseBody struct {
CID int `json:"cid"`
FirstName string `json:"first_name"`
LastName string `json:"last_name"`
NetworkRating int `json:"network_rating"`
PilotRating int `json:"pilot_rating"`
}
resBody := ResponseBody{
CID: targetUser.CID,
FirstName: safeStr(targetUser.FirstName),
LastName: safeStr(targetUser.LastName),
NetworkRating: targetUser.NetworkRating,
PilotRating: targetUser.PilotRating,
}
res := newAPIV1Success(&resBody)
data := apiUserFromDB(targetUser)
res := newAPIV1Success(&data)
writeAPIV1Response(c, http.StatusOK, &res)
}