ADFA-4949 fix(dashboard): close out the Kolibri pass — review nits, c… - #304
Merged
Merged
Conversation
…redential tests, books on the shared store
Second review follow-up. Finishes the dashboard side so the remaining work
(Android wishlist/service, Courses screen, catalog asset) does not have to come
back here.
login() no longer reports HTTP 400 as bad credentials. 400 is the serializer
rejecting the request — a missing field, an unknown facility — and calling that
"wrong password" sends the operator to change a password that was fine. It is now
'protocol' and carries the response body.
readStore() logs when it degrades. A corrupt or unreadable credentials.json
silently fell back to Admin/changeme, so the symptom surfaced later as an auth
failure with no stated cause. ENOENT stays quiet: no override yet is the normal
case.
deleteChannel()'s job id is now usable. It returns a Kolibri task id, and nothing
in our API could poll it. Added getKolibriTask() and GET /kolibri/task/:id, and
the delete response carries statusUrl.
authStatus() distinguishes upstream from ours. A fetch timeout or connection
failure means Kolibri is not ready (503), not that we broke (500); a
KolibriApiError propagates 401/403 and otherwise reports 502.
credentials.ts has tests: 16 covering the env -> override -> factory precedence,
that half a credential in the environment does not apply, 0600 permissions, no
leftover .tmp files from the atomic write, degradation on corrupt and
wrong-shaped JSON, input validation, that the username is trimmed and the
password is not, that describeCredential never returns the password, and that
isDefault still flags a hand-written Admin/changeme.
books moved onto the shared store. books.exec.ts and books.query.ts had the
credentials inline with a comment saying an override could be added later; this
is that override. Behaviour on an untouched device is identical — verified that
getCredential('calibre') resolves to Admin/changeme with no override present — but
a device whose Calibre-Web password changed now keeps working without a rebuild.
No inline credentials remain in the dashboard.
Verified: tsc --noEmit clean, build OK, npm test 64/64 (48 + 16 new). Runner
re-exercised against the mock across the happy, failed, empty-COMPLETED and
session-expiry scenarios; no change in behaviour.
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.
Second review follow-up on ADFA-4949. Closes out the dashboard side so the remaining work (Android wishlist/service, Courses screen, catalog asset) does not have to come back here.
login() no longer reports HTTP 400 as bad credentials — 400 is the serializer rejecting the request, and calling it "wrong password" sends the operator to change one that was fine. readStore() now logs when a corrupt credentials.json makes it fall back to the factory default, instead of surfacing later as an unexplained auth failure. deleteChannel()'s Kolibri task id is now pollable via GET /kolibri/task/:id. authStatus() returns 503 for a fetch timeout or connection failure rather than 500.
credentials.ts gets 16 tests: env → override → factory precedence, 0600 permissions, no leftover .tmp files, degradation on corrupt JSON, and that describeCredential never returns the password.
books.exec.ts and books.query.ts move onto the shared credential store — the override their own comment anticipated. Behaviour on an untouched device is identical; a device whose Calibre-Web password changed now keeps working without a rebuild. No inline credentials remain in the dashboard.
Verified: tsc --noEmit clean, npm test 64/64 (48 + 16 new). Runner re-exercised against the mock across the happy, failed, empty-COMPLETED and session-expiry scenarios.