fix(api): admins see all libraries in list/browse views#1310
Open
vavallee wants to merge 1 commit into
Open
Conversation
A multi-admin instance (e.g. an SSO-provisioned second admin) split authors and books by owner_user_id, but the list endpoints scoped strictly to the caller's user id with no admin bypass — so one admin could not see another admin's authors/books, and adding them hit the global foreign_id UNIQUE constraint as "author already exists". This was inconsistent with auth.CheckOwnership, which already treats admins (and API-key / no-tenancy callers) as able to access any item by id. Add auth.ListScopeUserID — unscoped (0) for admin role, disabled tenancy, or API-key callers; the caller's id otherwise — and route the owner-scoped list/browse handlers (authors, books, ListByAuthorAndUser, OPDS) through it. Also add db.QueryScopeForIncludingNull so the books list matches the authors list and includes owner-NULL rows (CheckOwnership treats unowned rows as visible to all), fixing books that pre-date the multi-user migration or were imported without an owner being hidden from a logged-in user. Non-admin isolation is preserved and tested: a non-admin still sees only their own rows plus unowned ones, never another non-admin's. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause (verified against prod)
A multi-admin instance had authors/books split by
owner_user_id(user 1admin, user 2akadmin— the latter auto-provisioned when SSO was enabled). The list endpoints scoped strictly to the caller's id (AuthorHandler.List→filter.UserID = auth.UserIDFromContext(ctx)) with no admin bypass, whileauth.CheckOwnershipalready treats admins (and API-key / no-tenancy callers) as able to access any item by id. So admin user 2 could not see user 1's authors/books in lists, and "Add Author" hit the globalforeign_idUNIQUE constraint → dead-end "author already exists".Confirmed on the live DB: author
Cory Doctorow(id 41) isowner_user_id = 1; browsing as admin user 2 hid it; owner spread was NULL=25 / u1=13 / u2=14.Fix
auth.ListScopeUserID(ctx)— returns 0 (unscoped) for admin role, disabled tenancy, or API-key callers; the caller's id otherwise. MirrorsCheckOwnership's bypasses.ListByAuthorAndUser, OPDS.db.QueryScopeForIncludingNull— books list now uses(owner = ? OR owner IS NULL)like the authors list, so owner-NULL rows (pre-multi-user / imported without an owner) are visible to a logged-in user instead of hidden.userID == 0still means unscoped.Security
No real access widening: admins can already open any item by id via
CheckOwnership; this only makes lists consistent with that. Non-admin isolation is preserved and tested — a non-admin sees only their own rows plus unowned, never another non-admin's.Tests
internal/api/list_scope_test.go,internal/db/list_scope_test.go: admin-sees-everything, non-admin-isolated-plus-global, API-key unscoped, tenancy-disabled unscoped, books-includes-null-owner. Admin-sees-everything fails pre-fix. Full api/db/auth suites pass; targeted scope tests clean under-race(0 data races; the full-package-raceonly timed out on a constrained box).🤖 Generated with Claude Code