feat(security): library visibility enforced at the source; per-library series; annotation ownership; admin user creation

The family-lens hardening: library_visibility was only enforced where
file bytes flowed (download/reader/OPDS) — metadata surfaces leaked
hidden libraries to any authenticated account, and registration was
open by default with no admin-side user creation.

- Series are grouped PER LIBRARY now (same name in two libraries =
  two entries: manga adaptations / LN splits no longer merge);
  /api/series aggregates only the requester's VISIBLE libraries and
  /api/series/books REQUIRES a visible library_id (breaking contract;
  web + app clients updated in the same change).
- /api/media-items, item detail, and the reader content endpoints
  (chapters/panels/outline/THUMBNAILS — page images leaked unguarded
  next to a gated GetPage) all enforce the visible set; resolveLibrary
  no longer honors a hand-typed library_id outside the visible set.
- Annotation/note/bookmark mutations (update/delete/tombstone, sync
  LWW paths incl. conflict resolution) carry ownership predicates:
  another account's bare row id is a 404, not a silent delete.
- Admin user creation: POST /api/auth/users (AdminMiddleware) + a
  Create User card in the admin Users UI — accounts stay creatable
  with self-service registration OFF, which is now the schema default
  (fresh installs; the first-admin bootstrap exception is unchanged).
- Tests: series contract re-pinned (per-library, visibility 403s),
  new ownership + admin-create integration suites, OPDS all=1 test
  now passes an explicit page size (shared-DB growth made the fresh
  books fall off page 1), device-cap helper switched to the admin
  endpoint.
This commit is contained in:
John O'Keefe committed 2026-10-13 12:03:01 -04:00
1 parent 97fbdf19fb
commit 431fec05ad
60 files changed
+1573 -691

No files matched your search

+9 -4
View File
@@ -336,7 +336,7 @@ func createTestUserForMaxDevices(t *testing.T, ts *httptest.Server, adminToken s
}
jsonData, _ := json.Marshal(createUserPayload)
req, _ := http.NewRequest("POST", ts.URL+"/api/auth/register", bytes.NewBuffer(jsonData))
req, _ := http.NewRequest("POST", ts.URL+"/api/auth/users", bytes.NewBuffer(jsonData))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+adminToken)
@@ -416,14 +416,19 @@ func createTestUserForMaxDevices(t *testing.T, ts *httptest.Server, adminToken s
err = json.NewDecoder(resp.Body).Decode(&result)
require.NoError(t, err)
// Check if user creation was successful
if result["user"] == nil {
// Admin create returns the user object at the top level; the legacy
// register endpoint nested it under "user". Accept both.
userObj, ok := result["user"].(map[string]interface{})
if !ok {
userObj = result
}
if userObj["id"] == nil {
// User creation failed for another reason
t.Logf("User creation failed, response: %+v", result)
return uuid.UUID{}
}
userIDStr := result["user"].(map[string]interface{})["id"].(string)
userIDStr, _ := userObj["id"].(string)
userID, _ := uuid.Parse(userIDStr)
return userID
+5 -1
View File
@@ -505,7 +505,11 @@ func TestOPDSLibraryFolders(t *testing.T) {
})
t.Run("all=1 keeps the flat cross-library catalog", func(t *testing.T) {
status, contentType, body := getFeed(t, base+"/catalog?all=1")
// Explicit large page: the shared test DB accumulates items, and
// the freshly created books sort last — without a generous
// per_page they fall onto a later page and the assertion reads
// tea leaves instead of the contract.
status, contentType, body := getFeed(t, base+"/catalog?all=1&per_page=200")
require.Equal(t, http.StatusOK, status)
assert.Contains(t, contentType, "kind=acquisition")
assert.Contains(t, body, "OPDS Folder Book 1")
@@ -0,0 +1,217 @@
package main
import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"testing"
"time"
"bookhoard/internal/database"
"github.com/google/uuid"
"github.com/jackc/pgx/v5/pgtype"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
)
// OwnershipAndAdminTestSuite pins the 2026-10-13 hardening: per-user
// annotation mutations are ownership-scoped (no cross-user delete/update by
// bare row id), and admins can create users even with self-service
// registration disabled.
type OwnershipAndAdminTestSuite struct {
suite.Suite
setup *TestServerSetup
token string
itemID string
}
func (s *OwnershipAndAdminTestSuite) SetupSuite() {
s.setup = setupTestServer(s.T())
s.token = s.setup.Token
s.itemID = s.createItemViaDB()
// createTestMediaItemID's t.Cleanup path can't be used at suite level
// (it fires after TearDownSuite has closed the server), so the item is
// created directly through the DB handle; suite cleanup relies on the
// standard "@tests.bookhoard.internal / test-named rows" passes of the
// next run's setupTestServer.
}
// createItemViaDB makes a throwaway library + media item directly through
// the test DB handle — no HTTP, no t.Cleanup entanglement.
func (s *OwnershipAndAdminTestSuite) createItemViaDB() string {
ctx := s.setup.QueueCtx
db := s.setup.DB
libraryTypeRow, err := db.GetLibraryTypeByName(ctx, "ebooks")
require.NoError(s.T(), err, "should find ebooks library type")
admin, err := db.GetUserByEmail(ctx, "testuser@tests.bookhoard.internal")
require.NoError(s.T(), err, "dev admin user should exist")
library, err := db.CreateLibrary(ctx, database.CreateLibraryParams{
Name: fmt.Sprintf("Ownership Suite Library %d", time.Now().UnixNano()),
Description: pgtype.Text{String: "annotation ownership suite", Valid: true},
LibraryTypeID: libraryTypeRow.ID,
CreatedByAdminID: admin.ID,
})
require.NoError(s.T(), err)
item, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{
LibraryID: library.ID,
Title: "Ownership Suite Item",
Author: pgtype.Text{String: "Suite Author", Valid: true},
FilePath: fmt.Sprintf("/tmp/ownership-suite-%d.epub", time.Now().UnixNano()),
MimeType: pgtype.Text{String: "application/epub+zip", Valid: true},
})
require.NoError(s.T(), err)
return uuid.UUID(item.ID.Bytes).String()
}
func (s *OwnershipAndAdminTestSuite) TearDownSuite() {
s.setup.Close()
}
func (s *OwnershipAndAdminTestSuite) getItem(itemID string) map[string]interface{} {
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/media-items/"+itemID, nil)
req.Header.Set("Authorization", "Bearer "+s.token)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
require.Equal(s.T(), http.StatusOK, resp.StatusCode)
var body map[string]interface{}
require.NoError(s.T(), json.NewDecoder(resp.Body).Decode(&body))
return body
}
func (s *OwnershipAndAdminTestSuite) createHighlight(token string) map[string]interface{} {
payload, _ := json.Marshal(map[string]interface{}{
"selection_text": "owned annotation text",
"start_position": "p1",
"end_position": "p1",
"color": "#ffff00",
})
req, _ := http.NewRequest("POST", s.setup.Server.URL+"/api/media-items/"+s.itemID+"/highlights", bytes.NewBuffer(payload))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+token)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
require.Equal(s.T(), http.StatusCreated, resp.StatusCode, "highlight create should succeed")
var body map[string]interface{}
require.NoError(s.T(), json.NewDecoder(resp.Body).Decode(&body))
return body
}
func (s *OwnershipAndAdminTestSuite) highlightExists(token, highlightID string) bool {
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/media-items/"+s.itemID+"/highlights", nil)
req.Header.Set("Authorization", "Bearer "+token)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
var body []map[string]interface{}
require.NoError(s.T(), json.NewDecoder(resp.Body).Decode(&body))
for _, h := range body {
if h["id"] == highlightID {
return true
}
}
return false
}
// TestHighlightDelete_CrossUserNoOp pins the ownership predicate: user B's
// DELETE of user A's highlight by bare id must not remove A's row.
func (s *OwnershipAndAdminTestSuite) TestHighlightDelete_CrossUserNoOp() {
regular := s.setup.RegularToken
created := s.createHighlight(regular)
highlightID, ok := created["id"].(string)
require.True(s.T(), ok, "create response carries the highlight id")
// The ADMIN (a different user) tries to delete it by id.
req, _ := http.NewRequest("DELETE", s.setup.Server.URL+"/api/media-items/"+s.itemID+"/highlights/"+highlightID, nil)
req.Header.Set("Authorization", "Bearer "+s.token)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
io.Copy(io.Discard, resp.Body)
resp.Body.Close()
assert.True(s.T(), s.highlightExists(regular, highlightID),
"another user's delete-by-id must not remove the owner's highlight")
// The owner deletes their own highlight — gone.
req, _ = http.NewRequest("DELETE", s.setup.Server.URL+"/api/media-items/"+s.itemID+"/highlights/"+highlightID, nil)
req.Header.Set("Authorization", "Bearer "+regular)
resp, err = (&http.Client{}).Do(req)
require.NoError(s.T(), err)
io.Copy(io.Discard, resp.Body)
resp.Body.Close()
assert.False(s.T(), s.highlightExists(regular, highlightID), "owner delete removes the row")
}
// TestAdminCreateUser pins the admin counterpart to registration: admins
// create accounts even with self-service registration disabled, regular
// users cannot, and duplicates conflict.
func (s *OwnershipAndAdminTestSuite) TestAdminCreateUser() {
suffix := fmt.Sprintf("%d", s.setup.DBPool.Config().MaxConns) // stable-ish uniquifier
unique := fmt.Sprintf("admincreated%s", suffix)
// Regular user token → 403 (AdminMiddleware).
payload, _ := json.Marshal(map[string]interface{}{
"email": unique + "@tests.bookhoard.internal",
"username": unique,
"password": "Test@Pass123!",
})
req, _ := http.NewRequest("POST", s.setup.Server.URL+"/api/auth/users", bytes.NewBuffer(payload))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+s.setup.RegularToken)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
io.Copy(io.Discard, resp.Body)
resp.Body.Close()
require.Equal(s.T(), http.StatusForbidden, resp.StatusCode)
// Admin → 201, and the account is immediately usable (login works)
// even though self-service registration is off for new servers.
req, _ = http.NewRequest("POST", s.setup.Server.URL+"/api/auth/users", bytes.NewBuffer(payload))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+s.token)
resp, err = (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
require.Equal(s.T(), http.StatusCreated, resp.StatusCode)
var created map[string]interface{}
require.NoError(s.T(), json.NewDecoder(resp.Body).Decode(&created))
assert.Equal(s.T(), unique, created["username"])
assert.Equal(s.T(), "user", created["role"])
// Duplicate username → 409.
req, _ = http.NewRequest("POST", s.setup.Server.URL+"/api/auth/users", bytes.NewBuffer(payload))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+s.token)
resp, err = (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
assert.Equal(s.T(), http.StatusConflict, resp.StatusCode)
// The created user can log in.
loginPayload, _ := json.Marshal(map[string]interface{}{
"login": unique + "@tests.bookhoard.internal",
"password": "Test@Pass123!",
})
req, _ = http.NewRequest("POST", s.setup.Server.URL+"/api/auth/login", bytes.NewBuffer(loginPayload))
req.Header.Set("Content-Type", "application/json")
resp, err = (&http.Client{}).Do(req)
require.NoError(s.T(), err)
defer resp.Body.Close()
assert.Equal(s.T(), http.StatusOK, resp.StatusCode)
}
func TestOwnershipAndAdminTestSuite(t *testing.T) {
suite.Run(t, new(OwnershipAndAdminTestSuite))
}
+4 -2
View File
@@ -667,14 +667,16 @@ func TestDeletedUserTokenRejected(t *testing.T) {
ctx := context.Background()
// Create a dedicated user, then log in as it to get a fresh token.
createResp := doReq(t, "POST", setup.Server.URL+"/api/auth/register", map[string]interface{}{
// Admin-created (not self-registered) so the test stays deterministic
// regardless of the registration_enabled setting.
createResp := doReq(t, "POST", setup.Server.URL+"/api/auth/users", map[string]interface{}{
"email": "ghost-test@tests.bookhoard.internal",
"username": "ghosttest",
"password": "Ghost@Pass123!",
}, setup.Token)
defer createResp.Body.Close()
if createResp.StatusCode != http.StatusCreated {
t.Skipf("registration unavailable (status %d)", createResp.StatusCode)
t.Skipf("admin user creation unavailable (status %d)", createResp.StatusCode)
}
loginResp := doReq(t, "POST", setup.Server.URL+"/api/auth/login", map[string]interface{}{
+101 -21
View File
@@ -32,11 +32,11 @@ func (s *SeriesIntegrationTestSuite) TearDownSuite() {
}
func (s *SeriesIntegrationTestSuite) TestGetSeries_RequiresLibraryID() {
// Ground truth: library_id is OPTIONAL on /api/series by design —
// GetDistinctSeries matches (narg IS NULL OR library_id = narg), so a
// missing library_id aggregates series across all libraries. The
// former "library_id required → 400" expectation asserted validation
// the shipped query never had.
// Ground truth (2026-10-13 visibility hardening): library_id stays
// optional, but the unscoped aggregate now covers only the requester's
// VISIBLE libraries — hidden libraries contribute nothing. Series are
// grouped per library (same name in two libraries = two entries), so
// every row carries a library_id.
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/series", nil)
req.Header.Set("Authorization", "Bearer "+s.token)
@@ -54,6 +54,13 @@ func (s *SeriesIntegrationTestSuite) TestGetSeries_RequiresLibraryID() {
require.NoError(s.T(), err)
assert.Contains(s.T(), body, "series", "response carries the series array")
assert.Contains(s.T(), body, "total")
if rows, ok := body["series"].([]interface{}); ok {
for _, r := range rows {
row, ok := r.(map[string]interface{})
require.True(s.T(), ok)
assert.NotEmpty(s.T(), row["library_id"], "series rows carry their library scope")
}
}
}
func (s *SeriesIntegrationTestSuite) TestGetSeries_InvalidLibraryID() {
@@ -165,14 +172,13 @@ func (s *SeriesIntegrationTestSuite) TestGetSeries_LimitClampedTo100() {
assert.Equal(s.T(), float64(100), limit, "limit should be clamped to 100")
}
func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_NameOnlyContract() {
// Ground truth: /api/series/books requires only `name` — the app
// client sends name-only (BookhoardApi.kt) and the web SSR page too
// (frontend.go). The query is intentionally library-agnostic (it
// matches the series name across libraries; library_id, if sent, is
// ignored — nothing validates it). The former "library_id required /
// bad uuid → 400" expectations asserted validation that never
// shipped.
func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_LibraryIDRequired() {
// Ground truth (2026-10-13 visibility hardening): /api/series/books is
// per-library now — library_id is REQUIRED, validated, and must be
// visible to the requester. The former name-only contract let any
// authenticated account pull series books across ALL libraries
// (including hidden ones), and merged same-name series across
// libraries (manga adaptations / LN splits).
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/series/books?name=Test+Series", nil)
req.Header.Set("Authorization", "Bearer "+s.token)
@@ -183,14 +189,12 @@ func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_NameOnlyContract() {
_ = Body.Close()
}(resp.Body)
assert.Equal(s.T(), http.StatusOK, resp.StatusCode)
assert.Equal(s.T(), http.StatusBadRequest, resp.StatusCode)
var body map[string]interface{}
var body map[string]string
err = json.NewDecoder(resp.Body).Decode(&body)
require.NoError(s.T(), err)
assert.Equal(s.T(), "Test Series", body["name"])
assert.Contains(s.T(), body, "books", "response carries the books array")
assert.Contains(s.T(), body, "total")
assert.Equal(s.T(), "library_id required", body["error"])
}
func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_RequiresName() {
@@ -214,8 +218,7 @@ func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_RequiresName() {
}
func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_InvalidLibraryID() {
// Same name-only contract: an unparseable library_id is ignored, not
// validated (nothing in the shipped pipeline ever reads it here).
// library_id is now validated: an unparseable uuid is a 400.
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/series/books?library_id=bad-uuid&name=Test", nil)
req.Header.Set("Authorization", "Bearer "+s.token)
@@ -226,7 +229,7 @@ func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_InvalidLibraryID() {
_ = Body.Close()
}(resp.Body)
assert.Equal(s.T(), http.StatusOK, resp.StatusCode)
assert.Equal(s.T(), http.StatusBadRequest, resp.StatusCode)
}
func (s *SeriesIntegrationTestSuite) TestGetSeriesBooks_NonexistentSeries() {
@@ -313,6 +316,83 @@ func (s *SeriesIntegrationTestSuite) TestGetSeries_ResponseStructure() {
assert.True(s.T(), ok, "'series' should be an array")
}
func (s *SeriesIntegrationTestSuite) setLibraryVisibility(token, libraryID string, visible bool) *http.Response {
payload, _ := json.Marshal(map[string]interface{}{
"library_id": libraryID,
"is_visible": visible,
})
req, _ := http.NewRequest("POST", s.setup.Server.URL+"/api/libraries/visibility", bytes.NewBuffer(payload))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Authorization", "Bearer "+token)
resp, err := (&http.Client{}).Do(req)
require.NoError(s.T(), err)
return resp
}
// TestGetSeries_HiddenLibraryScopedOut pins the family-lens contract: a
// library hidden from a user contributes NOTHING to that user's series
// views, and naming it explicitly is a 403, not a silent pass — across
// /api/series, /api/series/books, and /api/media-items.
func (s *SeriesIntegrationTestSuite) TestGetSeries_HiddenLibraryScopedOut() {
regular := s.setup.RegularToken
// Baseline: the regular user can see the suite library.
resp := s.setLibraryVisibility(regular, s.libraryID, true)
defer resp.Body.Close()
require.Equal(s.T(), http.StatusOK, resp.StatusCode)
// Hide it from the regular user.
resp = s.setLibraryVisibility(regular, s.libraryID, false)
require.Equal(s.T(), http.StatusOK, resp.StatusCode)
defer func() {
r := s.setLibraryVisibility(regular, s.libraryID, true)
_ = r.Body.Close()
}()
client := &http.Client{}
// Explicit hidden library → 403.
for _, path := range []string{
"/api/series?library_id=" + s.libraryID,
"/api/series/books?library_id=" + s.libraryID + "&name=Test",
"/api/media-items?library_id=" + s.libraryID,
} {
req, _ := http.NewRequest("GET", s.setup.Server.URL+path, nil)
req.Header.Set("Authorization", "Bearer "+regular)
r, err := client.Do(req)
require.NoError(s.T(), err)
body, _ := io.ReadAll(r.Body)
r.Body.Close()
require.Equalf(s.T(), http.StatusForbidden, r.StatusCode, "%s must be 403 for a hidden library (body: %s)", path, body)
}
// Unscoped aggregate: the hidden library's series never appear.
req, _ := http.NewRequest("GET", s.setup.Server.URL+"/api/series", nil)
req.Header.Set("Authorization", "Bearer "+regular)
r, err := client.Do(req)
require.NoError(s.T(), err)
defer r.Body.Close()
require.Equal(s.T(), http.StatusOK, r.StatusCode)
var body map[string]interface{}
require.NoError(s.T(), json.NewDecoder(r.Body).Decode(&body))
if rows, ok := body["series"].([]interface{}); ok {
for _, row := range rows {
m, ok := row.(map[string]interface{})
require.True(s.T(), ok)
assert.NotEqual(s.T(), s.libraryID, m["library_id"], "hidden library must not appear in the aggregate")
}
}
// The admin still sees the library (default-allow; visibility rows are
// per-user).
req, _ = http.NewRequest("GET", s.setup.Server.URL+"/api/series?library_id="+s.libraryID, nil)
req.Header.Set("Authorization", "Bearer "+s.token)
r, err = client.Do(req)
require.NoError(s.T(), err)
defer r.Body.Close()
assert.Equal(s.T(), http.StatusOK, r.StatusCode)
}
func (s *SeriesIntegrationTestSuite) TestRestoreSystemCollection_ContinueSeries() {
reqBody := map[string]interface{}{
"collection_name": "Continue Series",