diff --git a/internal/web/api_users.go b/internal/web/api_users.go index 1dd2b3f..3025e32 100644 --- a/internal/web/api_users.go +++ b/internal/web/api_users.go @@ -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, diff --git a/internal/web/api_users_test.go b/internal/web/api_users_test.go index a22d7eb..417b968 100644 --- a/internal/web/api_users_test.go +++ b/internal/web/api_users_test.go @@ -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) +} diff --git a/internal/web/openapi/openapi.v1.yaml b/internal/web/openapi/openapi.v1.yaml index 6542ff2..62d8d8a 100644 --- a/internal/web/openapi/openapi.v1.yaml +++ b/internal/web/openapi/openapi.v1.yaml @@ -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 <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) diff --git a/internal/web/pages_user_query.go b/internal/web/pages_user_query.go index e8eafa6..5194eab 100644 --- a/internal/web/pages_user_query.go +++ b/internal/web/pages_user_query.go @@ -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 } diff --git a/internal/web/pages_user_query_test.go b/internal/web/pages_user_query_test.go index 8493f45..26f7afb 100644 --- a/internal/web/pages_user_query_test.go +++ b/internal/web/pages_user_query_test.go @@ -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) } diff --git a/internal/web/user.go b/internal/web/user.go index 78ae637..79566c0 100644 --- a/internal/web/user.go +++ b/internal/web/user.go @@ -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) }