mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-01 11:47:46 +02:00
29311cab2bf0fcb58c5f175feeab1c7f96cd2d21
15
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jmoore-skild
|
bbd991510f |
fix(backup): refuse prometheus_enabled when the backup has no token either (#2656)
The companion-credential rule has five conditions, and the second one -- "the backup itself carried a usable credential" -- was applied to all five pairs. It should not be. It is what stops the rule over-refusing an anonymous MQTT broker or an anonymous LDAP bind, both of which are working configs: there, an empty credential in the backup means the restore is not producing anything weaker than what was backed up. For prometheus_enabled it does not transfer. An empty prometheus_token removes /api/v1/metrics' only gate, so the exposure is a property of the toggle, not of a downgrade relative to the backup -- and prometheus_token is optional, so a backup taken on an instance that enabled Prometheus without ever setting one carries the toggle and no usable token. That payload skipped the refusal entirely: not a candidate, so the local-state pass never ran, and the blocklist quietly dropped the token key. On a token-less target the result was prometheus_enabled=true, no token row anywhere, and a full unauthenticated metrics dump -- the same hole the rule was written to close, reached from the likelier of the two directions. So condition 2 is now per-pair: an exposure class (prometheus) that skips it and is judged on local state alone, and an availability class (mqtt, ldap, ha, virtual_printer) that keeps it. Nothing else changes -- the local-state pass already stands down when the instance has its own credential, when HA_TOKEN is in the environment, and when the toggle is already on locally, so "the exposure pre-dates this restore" still holds and refusals still get no tally increment. One wording consequence: an exposure toggle can now be refused on a payload with no credential-like key in it at all, where the shared caveat would have read "0 credential-like key(s) will be skipped". That case gets its own preview detail code, settingsCompanionOnlyWillSkip, in all 13 locales. Tests: 6 that fail pre-fix -- the token key absent and blank at the unit level, the new preview wording, and the integration test through the real endpoint for both payloads (200 with a full metrics body before this, 404 after). Plus 3 controls, because over-refusal is still the real risk: the exposure route must still stand down for a local token and for an already-on toggle, and the availability class must still let a credential-less mqtt/ldap/virtual_printer toggle through. The anonymous-broker and anonymous-bind controls are unchanged and still pass. |
||
|
jmoore-skild
|
733894aee6 |
docs(backup): give the real reason cloud profiles are not restorable (#2656)
Both docstrings said the collector never writes `cloud_profiles/*.json`. That was true when they were written and stopped being true at `455a9e4b` (#2717, "collect cloud profiles from every connected account"), which is this branch's rebase base — so the PR was shipping a stated reason its own base had invalidated. The real reason is the one the PR body now gives: restoring a preset means writing to a Bambu or Orca Cloud account, which is a different operation from every other category here. Those land in the local database, or on a printer the instance already owns. Checked that nothing in the restore path is confused by the new files — the category globs don't reach `cloud_profiles/`, and it stays out of `RestoreCategory`. |
||
|
jmoore-skild
|
cb6a4e6d88 |
fix(backup): stop two backed-up K-profiles claiming one live slot (#2656)
`_match_kprofile` ends in a single-candidate fallback, and the per-nozzle loop called it once per entry with no record of which live profiles were already taken. Two backup entries sharing a `filament_id` and matching on neither `setting_id` nor `name` both resolved to the same live profile, both got the same `cali_idx`, and both went into the batch — so the second overwrote the first on the printer while the tally counted two restored. Reachable in the ordinary way: the user deletes one of a pair after the backup, and the delete-then-add re-key this code already reasons about is exactly what strips the `setting_id` match. Fix: thread a `claimed` set of slot ids through the loop; a live profile can only stand in for one entry. A displaced entry falls through to `cali_idx: -1` — add-as-new is the safe outcome — and folds into the existing `kprofilesUnmatched` note rather than earning a new code. The single-candidate fallback is still judged against every candidate rather than the unclaimed ones. Two live profiles for one filament are ambiguous whether or not another entry has taken one, and narrowing to "available" would turn a guess the code deliberately refuses into a match. Tests: 2 regression (the displaced entry is added rather than aliased, and keeps its own setting_id) + 2 controls (two genuine matches keep their own slots; a claimed slot does not make an ambiguous pair matchable). Both regressions confirmed failing against the pre-fix service. |
||
|
jmoore-skild
|
be8e8d545f |
fix(backup): don't let an old backup commit blank an archive's owner (#2656)
`created_by_id` and `deleted_at` both went into the archive `fields` dict unconditionally, via `entry.get(...)`. A backup commit taken before the collector wrote those keys carries neither, so `.get` yielded None for both and the overwrite branch — a blanket `setattr` over every key — wrote NULL over a live owner. That is exactly the failure carrying `created_by_id` was added to fix, only now inflicted on rows that were fine: `_ensure_archive_visible` fails closed on a NULL owner, so the archive 404s for the person who owns it. It emitted no note either, because `archivesOwnerCleared` only fires for an id that isn't in `valid_users`, not for an absent key — and the row still counted as restored. `deleted_at` had the mirror problem: an old commit silently un-deleted, since `archivesUndeleted` reads the same absent value. Absent is not the same as explicitly null. Both keys now only enter `fields` when the entry actually carries them, so an old commit leaves the column alone on overwrite and a current one can still say "this archive has no owner" or "this archive is live". Same shape as the tag-column rule: don't clear what the backup doesn't know about. Behaviour change to an existing test, called out deliberately: `test_overwrite_undeletes_a_locally_deleted_archive_and_says_so` now has to put `deleted_at: None` in the entry to mean it. Tests: 2 regression (owner and deleted_at both left alone by a key-less entry) + 2 controls (an explicit null is still honoured, with its note). Both regressions confirmed failing against the pre-fix service. |
||
|
jmoore-skild
|
158301ac8a |
fix(backup): halve the provider round-trips, and stop losing commit metadata (#2656)
The three remaining review items, all in the read path. E1 — four provider calls where two would do. preview() called list_commits twice: once inside _resolve_ref to turn HEAD into a SHA, once more at limit=20 purely to find the entry describing that same SHA. And list_tree's recursive tree GET was thrown away, so fetch_files immediately fetched the identical tree again to map path -> blob SHA. _resolve_ref now returns the entry it already has, and list_tree returns its blob_shas map for fetch_files to take as an optional argument. GitLab reads files by path and ignores it. E2 — `commit: null` for a ref outside the 20 most recent. Two causes, and the second is the one that actually bit: REF_PATTERN accepts a 7-character ref while providers return the full 40, so the exact `==` in the scan never matched an abbreviated SHA *even when the commit was in the window*. Fixed by prefix comparison, plus a get_commit(ref) on the GitHub and GitLab backends for the genuinely-outside-the-window case. Gitea and Forgejo inherit GitHub's. Still best-effort: it is a subject line and a date, so a failed lookup renders the preview without them rather than failing it. E7 — the two tree readers disagreed, and each was wrong in the other's direction. GitHub's recursive trees endpoint is not paginated and signals overflow with truncated=true, which _blob_shas_at hard-fails on. Gitea and Forgejo *do* page that endpoint, and inherited that single GET unchanged — so a large backup repo returned only the first page and every category beyond it looked absent from the commit. GiteaBackend now has its own paging _blob_shas_at. GitLab had the mirror-image bug the review did not name: at its 50-page cap it exited through the while condition and returned success: True with a silently partial path list. Both now fail loudly, which is what the GitHub version was always doing. Both halves of E7 are the same failure the module already refuses to allow: a restore that skips categories and calls it "not present in this backup commit". 24 new or changed tests, all failing against this commit's parent. |
||
|
jmoore-skild
|
945f50a1ab |
fix(backup): stop overwrite writing a spool's other tag key (#2656)
Four of the review's smaller items. E6, the substantive one. tag_uid and tray_uuid are both in the overwrite setattr loop, so a spool matched on one key got the backup's *other* key written onto it. Neither column has a unique constraint (models/spool.py, and no unique index in the migrations), so nothing errors — a duplicate tag simply appears, after which _find_spool's .scalars().first() is non-deterministic and an AMS tag lookup resolves to an arbitrary one of the two spools. The same loop could also clear a tag the user had scanned since the backup was taken, when the backup entry held None. _find_spool now reports which key matched, and _guard_tag_overwrite drops a tag column from the write when the incoming value is empty and the local row has one (the backup predates the scan, so the local tag is the newer fact) or when another local spool already holds it. Announced in the tally the way the archive un-delete case already announces itself, rather than done silently — the spoolTagKept locale key landed with the rest of the i18n block last commit. E5. The Restore button is hidden without github:restore. All three endpoints are gated on it server-side, so the modal 403s on its first preview; offering the button is offering an action that cannot work. Button only — the card stays visible, since configuring backups is a separate permission — and hasPermission returns true with auth off, so a single-user instance is unaffected. E3. models/github_backup.py: the trigger comment said manual/scheduled; this PR added a third value. E4. ha_token_from_env: recommending no change, with the reasoning recorded as a test rather than left in a review thread. It is built only in the settings GET response, is absent from AppSettingsUpdate, and is therefore never a Settings row — it cannot reach a backup, so an allowlist entry would be dead code. Worse, a name-shaped exception to a belt-and-braces denylist is a live hole: an attacker-authored settings/app_settings.json could get a *token*-named row written by choosing that name. 4 unit tests and 1 frontend test that fail against this commit's parent, plus 4 controls: a free tag is still written, an unchanged tag is not reported as kept, an insert is unaffected, and the button still shows with auth disabled. |
||
|
jmoore-skild
|
af8d14d796 |
i18n(backup): make the restore notes and preview details translatable (#2656)
A German user got a translated modal with "Not present in this backup commit" in
the middle of it. Every tally note and preview caveat was a server-built English
sentence rendered verbatim.
Follows the backup.pathCheck contract already in use one card down in the same
component, deliberately rather than inventing a second convention: the server
sends a `code` plus typed `params` and carries the English along as the
fallback, and the client renders
`t(`...${code}`, { ...params, defaultValue: message })`. The defaultValue arm is
what keeps a newer backend's unfamiliar code readable instead of printing the
raw key — covered by its own test.
Shapes:
- notes: list[str] -> list[GitHubRestoreNote] {code, params, message}. Breaking,
but the field is unreleased in this same PR.
- GitHubRestorePreviewCategory gains detail_code / detail_params; `detail` stays
as the English fallback.
- _CategoryTally.note(code, message, **params), deduped on (code, params) rather
than on the rendered text, so two offline printers both keep their names. The
20-note cap is unchanged.
28 new leaves across 13 locales: 20 notes.* and 8 details.*. `noData` collapses
the four per-category "No X data in this backup" strings into one, since the
category heading already renders beside it. Counts use single-form {{count}} in
the existing "N record(s)" style rather than i18next plural suffixes — nothing in
this block uses _one/_other and the parity script has extra rules for them.
Parity holds at 5737 leaves in all 13 locales.
Deliberately out of scope, and worth saying so rather than leaving it to look
like an oversight: result.message, the commit-picker subject lines and the HTTP
error strings stay English. Those also originate in the provider backends, so
code-ifying them widens the diff well past the restore service.
spoolTagKept is added here with the rest of the locale churn but is not emitted
until the next commit, so the 13-locale change lands once.
|
||
|
jmoore-skild
|
cfa82bfcfb |
fix(backup): carry the owner across, or restored archives are invisible (#2656)
Neither _collect_archives nor _restore_archives touched created_by_id, so every restored archive row landed NULL. That column is not attribution, it is what the access check runs on: _ensure_archive_visible (api/routes/archives.py) fails closed on NULL — a 404 for any caller without archives:read_all — and the list paths filter created_by_id == user.id. On a multi-user instance the tally therefore reported archives restored while the person who owns them could neither list nor open them. Same shape as the deleted_at fix, and the same remedy: the collector records the key next to deleted_at, the restore mirrors the printer_id/project_id pattern exactly — one hoisted select(User.id), a membership test per row, an unknown id coerced to None rather than failing the row, and one de-duplicated note. It is in the overwrite setattr loop too, so overwrite keeps meaning "make local match the backup". Additive on the backup side, so older backups still restore; they just cannot know the owner. Clearing the id is not silent-safe, so the note says what it costs: those archives are visible only to users with archives:read_all until an admin reassigns them. Caveat recorded in a comment and raised in the PR, not decided here: this is the one place the module reuses a raw backup id, against its own rule. Validating it means a *stale* id clears rather than pointing somewhere wrong, but a live id belonging to a different person on a different instance would still collide. Collecting username and resolving on that would close it. 6 unit tests and 1 integration test that all fail against the parent commit, plus 2 controls that pass either way — a backup with no created_by_id key still restores, and a second operator still gets a 404. |
||
|
jmoore-skild
|
ca93d4cf44 |
fix(backup): never restore a toggle whose credential can't come with it (#2656)
A settings restore refuses to write anything credential-shaped, but wrote the switches that depend on those credentials like any other key. Restoring the two halves apart is not a partial restore, it is a downgrade. The sharp case is Prometheus. /api/v1/metrics is on PUBLIC_API_ROUTES and its only gate is `if token:`, so an empty or absent token means no authentication at all. prometheus_token matches the `token` hint and is refused; prometheus_enabled is an ordinary key and was written. On an instance that never enabled Prometheus there is no local token row, so overwrite-*off* alone was enough to publish the whole metrics body to anyone who could reach the port. The new integration test shows exactly that: 200 with a full unauthenticated body before, 404 after. Four more pairs are the same shape and break an integration rather than open one: ldap_enabled/ldap_bind_password, mqtt_enabled/mqtt_password, ha_enabled/ha_token (with an HA_TOKEN env arm, since get_homeassistant_settings prefers the environment over the row), and virtual_printer_enabled/virtual_printer_access_code — the last largely vestigial post-migration, included for consistency. A toggle is refused only when all five hold: the payload value is truthy, the backup carried a non-empty companion credential, that credential is denylisted, this instance has no usable value for it, and the toggle is not already on locally. The second condition is what keeps the rule honest — an anonymous MQTT broker and an anonymous LDAP bind are legitimate configs that pass empty credentials straight through, and without it both would be false positives. With it, the rule fires only when the restore would produce a config weaker than both the backup and the local instance. A present-but-blank prometheus_token row counts as unusable, since that is precisely the `if token:` hole. The rule needs the payload *and* local database state, which the old static _count_items could not see, so preview and restore now share one classifier: _plan_settings() runs a single SELECT over both halves of every candidate pair before anything enters the session, and returns the three refusal buckets. preview() takes the session the route already has. _is_skipped_setting_key is gone rather than having its docstring corrected as asked: a name is no longer enough to decide, so the union predicate had no caller left. Also implements the review's third ruling — the tally counts what the preview counted, and refusals live in the notes. Two `skipped += 1` increments are dropped (blocked, protected) and the companion refusal adds none; the value-is- None and overwrite-off skips stay, because they depend on the run's flags, which the preview cannot see. restored + skipped + failed now equals the item count the user was shown — off by three before. Behaviour change called out for review: test_credential_keys_are_never_restored and test_auth_settings_are_never_restored asserted skipped == 2 and 4; both are now 0, which is the point of the ruling. 16 new unit tests plus 2 integration tests. Nine of them are controls, because over-refusal is the real risk of this change — the anonymous-broker and anonymous-bind guards are load-bearing, not decoration. |
||
|
jmoore-skild
|
07244b6a43 |
fix(backup): don't count a stale selection, and say when archive links are dropped (#2656)
Two smaller restore-path fixes from the same review. The modal's footer counted `selected` raw while the checkboxes rendered `selected && isAvailable`. Switching commits keeps `selected` on purpose (it is only pruned once the new preview lands), so for as long as the new commit's preview was in flight — with the category list replaced by its spinner — the footer still read "2 selected" over an enabled Restore button, and clicking it restored the newly-picked commit with the previous commit's categories, none of which the user had seen an item count for. The count and the POST body now come from one `selectedCategories` memo gated on availability, exactly as the checkboxes are, so both go empty until the preview lands. Restoring Spool inventory without Print archives leaves archive_id_map empty, so every usage -> archive link is nulled even where the archive exists locally. It can't be resolved here (the archives payload isn't fetched for a category that wasn't selected) and a later archives-only restore won't repair it either, since the usage dedupe key doesn't include archive_id and those rows read as already-present. So it gets a note naming the remedy while the user can still redo the run with both categories ticked. Carries the rebuilt bundle (index-C2LOlVCR.js -> index-C16HJNOV.js). |
||
|
jmoore-skild
|
e7495dd41b |
fix(backup): reconnect the MQTT relay after restoring mqtt_* settings (#2656)
The relay reads its broker config once, when configure() is called — which is
why the settings PUT handler reconfigures it after writing those rows
(api/routes/settings.py:246). The restore wrote the rows and stopped there, so
the relay stayed on the pre-restore broker until the next backend restart while
the UI showed the restored values: the one way a settings restore could look
applied without being applied.
_restore_settings now reports the keys it actually wrote, and run_restore
reconfigures the relay from the committed rows when any of them is an mqtt_ one.
Three details worth keeping:
* it runs after the commit, because configure() drops the connection and
rebuilds it — not something to do on values a later failure could roll back;
* it is keyed on written, not merely present: a key skipped for overwrite=off
or by the credential blocklist must not trigger a reconnect;
* mqtt_password is never restorable, so configure() gets the row already in the
database and an unchanged broker keeps working.
A broker that refuses the new config is noted on the settings tally ("restart
Bambuddy") rather than failing the restore, matching the PUT handler's
best-effort handling of the same call.
|
||
|
jmoore-skild
|
3ba89c60a1 |
fix(backup): release the SQLite writer before the K-profile MQTT phase (#2656)
_apply ran archives and spools first, which autoflushes their INSERTs and so opens SQLite's single write transaction, then called _restore_kprofiles — which awaits get_kprofiles per printer per nozzle at timeout=5.0 with max_retries=3, i.e. up to ~15 s each against a printer that ignores the request. The commit only came afterwards, in run_restore. busy_timeout is 15 s (core/database.py:21), so a restore covering a couple of unresponsive printers held the writer past it and unrelated writes elsewhere in the app failed with "database is locked". Commit the database categories before the MQTT phase starts. The K-profile work is not in that transaction anyway — it leaves over MQTT — so the only thing lost is rolling those categories back when a K-profile send fails, and that rollback was never the right behaviour: extrusion_cali_set has already reached the printer by then, so undoing the database half would just make the two disagree. |
||
|
jmoore-skild
|
737258202b |
fix(backup): never restore the auth policy settings from a backup (#2656)
_collect_settings exports every Settings row minus two credential keys, so auth_enabled / advanced_auth_enabled / local_login_enabled / setup_completed all travel in a backup, and none of them are credential-shaped enough for _SECRET_KEY_HINTS to catch. Writing them back was the one part of a settings restore that changed who can reach the instance rather than how it behaves: * auth_enabled=false — from any backup taken before auth was turned on — disabled authentication. core.auth caches only the enabled=True result, on a 30 s TTL, precisely so staleness fails closed; set_auth_enabled pairs its write with invalidate_auth_enabled_cache(). The restore did neither, so it left the stored value the open one. * local_login_enabled=false walked straight past the #1589 refusals in update_settings (no enabled OIDC provider / no OIDC link on the caller), which exist to stop exactly that lockout. * /github-backup/restore is gated on GITHUB_RESTORE alone, so honouring these keys made that permission a way to rewrite auth config without SETTINGS_UPDATE. Flipping an existing row needed overwrite_existing, so the odds were lower than the severity. Both refusals now share _is_skipped_setting_key so the preview's item count still matches what a restore writes, and the skipped keys get their own note pointing at the auth UI rather than being folded in with the credential ones. |
||
|
jmoore-skild
|
cbe412607f |
fix(backup): resolve K-profile cali_idx live instead of reusing the backup's (#2656)
Restoring K-profiles addressed extrusion_cali_set at the cali_idx recorded in the backup. If that slot no longer existed on the printer the write was silently dropped and the restore still reported the profile restored. Not an edge case: Bambuddy's own K-profile editor is what re-keys the slot. On a single-nozzle printer an edit is delete-then-add, so any edit between backup and restore reproduces it. Found testing on an X1E. Backup held cali_idx 8151; an edit through the UI re-keyed the profile to 4606; the restore published cali_idx 8151, the printer ignored it, and the tally read "1 restored" while the k-value stayed put. Resending the identical payload with cali_idx 4606 applied, isolating the stale index as the cause. Fix mirrors the natural-key matching spools and archives already use, which the module docstring already promised but scoped to spool.id and print_archives.id. Before writing, read the live profiles for the nozzle and match on filament_id + setting_id, falling back to filament_id + name, then to the sole candidate for that filament. Send that profile's current cali_idx; where nothing matches send -1 so the printer adds a new profile instead of addressing a dead slot, and say so in the tally. A failed read degrades to adding rather than aborting. Also corrects the tally note. The printer does acknowledge extrusion_cali_set -- it answers with a result/reason pair -- so "published without acknowledgement" was false. It reports "fail" on writes that land, though, so the note now says the acknowledgement is unreliable rather than absent. Consuming result is left to a follow-up. Re-verified on the same X1E: perturbed to k=0.061, restored from the commit carrying the stale slot, payload went out with cali_idx 4606 and the printer read back 0.027. |
||
|
jmoore-skild
|
6a239314dc |
feat(backup): restore selected categories from a Git backup commit (#2656)
The Git backup feature was push-only: there was no equivalent of the local backup's Restore button, so recovering meant hand-downloading JSON files from the repository. This adds the read side. Providers gain list_commits / list_tree / fetch_files on the GitProviderBackend ABC. GitHub implements them against the Git Data API and Gitea/Forgejo inherit that unchanged; GitLab overrides for its own REST shape, including tree pagination and subgroup path encoding. fetch_files is batched so the path -> blob SHA lookup happens once per restore rather than once per file, and uses the blobs API rather than contents because contents silently inlines only the first 1 MB. The new GitHubRestoreService resolves HEAD to a concrete SHA up front, so a preview and the restore that follows act on the same commit even if a scheduled backup lands in between. Categories are applied archives -> spools -> settings -> kprofiles: archives first because spool usage history references archive_id, K-profiles last because they leave the database and publish over MQTT. Restores never reuse the backup's primary keys. spool.id and print_archives.id are bare autoincrement columns, so ids from an old backup very likely belong to unrelated rows today; rows are matched on natural keys (tag_uid, then tray_uuid, then a descriptive composite for spools; content_hash or filename plus started_at for archives), inserted without an explicit id, and an old_id -> new_id map rewrites the foreign keys in spool usage history. created_at is carried across on insert so restoring the same backup twice matches instead of duplicating. Dangling printer/project links are cleared and reported rather than failing the row. Settings restore re-applies the collector's credential denylist on the read side, plus a pattern guard, because a backup taken before that denylist existed can still contain secrets. Restored archives are metadata-only: the 3MF and thumbnail bytes are not in a Git backup and print_archives.file_path is NOT NULL, so inserted rows get an empty path and the UI says so. Backup and restore take a mutex against each other; both write the same tables and talk to the same printers. Restores are logged as GitHubBackupLog rows with trigger="restore", which needs no migration and surfaces them in the existing History card. Cloud profiles are deliberately not a restore category. The collector never actually writes cloud_profiles/*.json - it reads a "setting" list key the Bambu Cloud API does not return - and the preset list it would write carries no setting payload. Filed separately. Permission github:restore already existed and is granted to Administrators, so no permission changes were needed. Tests: 125 new backend tests (provider reads across all four providers, the per-category appliers, the API endpoints) and 13 frontend tests. Full suites pass with no regressions; the 35 backend failures on Windows are byte-identical with and without this branch. |