Paginate fronts history + fix step-up 401 silent-fail#62
Merged
Conversation
Two pieces; tagged together since they share a branch.
1) Pagination for /v1/fronts
GET /v1/fronts had limit + offset but no "is there more" signal, so the
frontend silently rendered only the first 50 entries and anyone with a
longer history was truncated without warning. Adds:
Backend (non-breaking):
- `cursor` query param (alternative to offset); when set, offset is
ignored. Cursor is base64url JSON of {started_at, id}, opaque.
- `include_total` opt-in param emitting X-Sheaf-Total-Count (single
COUNT on the system_id-indexed filter — off by default so cursor /
load-more callers don't pay).
- Response headers X-Sheaf-Has-More + X-Sheaf-Next-Cursor on every
list call.
- Has-more detection via limit+1 probe rather than a separate count;
response time stays flat regardless of total history length.
- ORDER BY started_at DESC, id DESC + Postgres row comparison
(started_at, id) < (cursor.started_at, cursor.id) gives stable
pagination across ties. Mobile and any other consumer continue to
work unchanged.
Frontend:
- View toggle in the History header (infinite icon vs numbered-pages
icon), persisted to URL search params (?view=paged&page=N&pageSize=L)
so refresh / bookmark / share preserves position.
- Infinite (default): useInfiniteQuery + Load older entries button.
- Numbered pages: useQuery with placeholderData=keepPreviousData
(page doesn't flash empty during navigation) + a new PageNav
component rendering First / Prev / windowed page numbers / Next /
Last with ellipsis for long lists. Page-size select offers 25 /
50 / 100.
- Per-user persistence: view + pageSize persist to client-settings/
web under a `fronts` key. URL still wins when present; settings
provide the default on bare /fronts visits. Page number itself is
transient.
Tests: 6 new integration tests covering the cursor + count surface
(has-more on truncation, has-more=false on short pages, exact-limit
boundary, full walk returns every entry once, invalid cursor → 400,
total-count opt-in, cursor wins over offset).
2) Step-up auth denials return 403 instead of 401
Bug surfaced when deleting a notification channel under a system with
delete_confirmation=password: wrong password returned 401 Incorrect
password, which the frontend's apiFetch read as "token stale" and
fired the silent refresh-retry. Refresh succeeded but the retried
DELETE still came back 401 — and a `resp.status !== 401` guard meant
to suppress double-toasting during normal refresh dances swallowed
the post-retry 401. End result: click Delete, nothing visible
happens.
Fix is two-sided:
- Backend: every "user is authenticated, step-up credential is wrong"
path now raises 403 instead of 401. 401 means "authenticate"; 403
means "authenticated but can't do this". The wrong-credential case
is the latter. Sites: services/system_safety.verify_destructive_auth
(covers member/channel/front/journal/group/tag/poll/message/safety
deletes), api/v1/admin.do_step_up, api/v1/systems.update_delete
_confirmation, api/v1/account.account_data, api/v1/export.create
_export_job, api/v1/auth.request_account_deletion. "Credential not
provided" branches stay 400/422 per-site.
- Frontend: apiFetch + apiFetchWithHeaders now distinguish pre-retry
401 (suppressed; refresh handles) from post-retry 401 (surfaced as
toast). Defensive measure on top of the backend fix.
Test asserts updated in test_system_safety, test_admin_step_up,
test_account_deletion, test_account_export_completeness to expect
403 on wrong-credentials paths. 45/45 affected tests pass.
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
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.
Two pieces on one branch — pagination work uncovered the step-up bug while testing, fix tagged in here rather than spun off.
1. Pagination for the fronts history
GET /v1/frontswas paginated by limit + offset but had no "is there more" signal, so the frontend silently rendered only the first 50 entries and anyone with a longer history was truncated without warning.Backend (non-breaking):
cursorquery param (alternative tooffset); when set,offsetis ignored.include_totalopt-in param that emitsX-Sheaf-Total-Count(one extraCOUNT(*)— off by default so cursor / load-more callers don't pay).X-Sheaf-Has-More: true|false, plusX-Sheaf-Next-Cursor: <opaque>when has-more is true.{started_at, id}, opaque to callers.ORDER BY started_at DESC, id DESC+ Postgres row comparison(started_at, id) < (cursor.started_at, cursor.id)gives stable pagination across ties.limit + 1probe rather than a separate count, so response time stays flat regardless of total history length. Mobile and any other consumer continue to work unchanged.Frontend:
?view=paged&page=N&pageSize=L) so refresh / bookmark / share preserves position.useInfiniteQuery+ Load older entries button.useQuerywithplaceholderData=keepPreviousData(page doesn't flash empty during navigation) + a newPageNavcomponent rendering First / Prev / windowed page numbers / Next / Last with ellipsis for long lists. Page-size select offers 25 / 50 / 100.client-settings/webunder afrontskey, so a one-time toggle sticks across sessions. URL still wins when present; settings just provide the default on bare/frontsvisits. Page number itself is transient.Tests: 6 new integration tests for the cursor + count surface (has-more on truncation, has-more=false on short pages, exact-limit boundary, full walk via cursor returns every entry once, invalid cursor → 400, total-count opt-in, cursor wins over offset). All 13 fronts tests pass.
2. Step-up auth denials return 403 instead of 401
Bug surfaced when deleting a notification channel under a system with
delete_confirmation=password: wrong password returned401 Incorrect password, which the frontend'sapiFetchread as "access token may be stale" and silently fired the refresh-and-retry path. Refresh succeeded but the retried DELETE still came back 401 — and aresp.status !== 401guard meant to suppress double-toasting during normal refresh dances swallowed the post-retry 401. End result: click Delete, nothing visibly happens.Fix is two-sided:
services/system_safety.verify_destructive_auth(covers member / channel / front / journal / group / tag / poll / message / safety deletes),api/v1/admin.do_step_up,api/v1/systems.update_delete_confirmation,api/v1/account.account_data,api/v1/export.create_export_job,api/v1/auth.request_account_deletion. The "credential not provided" branches stay 400 / 422 per-site.apiFetch+apiFetchWithHeadersnow distinguish pre-retry 401 (suppressed; refresh handles it) from post-retry 401 (surfaced via toast like any other error). Defensive measure on top of the backend fix.Test asserts updated in
test_system_safety,test_admin_step_up,test_account_deletion,test_account_export_completenessto expect 403 on wrong-credentials paths.Test plan
ruff check sheaf/cleannpx tsc --noEmit+npm run lintcleanpytest tests/test_fronts.py— 13/13pytest tests/test_system_safety.py tests/test_account_deletion.py tests/test_account_export_completeness.py tests/test_admin_step_up.py— 45 pass, 11 skip (admin-step-up tests need a differentADMIN_AUTH_LEVELconfig)