diff --git a/CHANGELOG.md b/CHANGELOG.md index 8772c2f60..acb68af18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ All notable changes to Bambuddy will be documented in this file. - **Every FTP session Bambuddy opens now records how it closed (#3009, reported by @grengojbo)** — the report traced a print completion that opened two FTP connections to the printer, deleted one file and then, as far as the log showed, did nothing else until the printer was powered off 21 minutes later, and concluded the connections were being left open. They were not: the post-print SD-card cleanup opens one connection per candidate filename and closes each in a `finally`, which a run against a real FTPS server confirms at the server end for both the delete and the 550 not-here case. The trouble is that nothing in the log could have said so. Neither the clean close nor the hard socket drop logged anything at any level, so a session closed properly and a socket genuinely abandoned produced the same output — none — and the only way to tell them apart was to read the source. Both now log one DEBUG line naming the printer, whether QUIT was acknowledged or the socket had to be dropped without it, why, and how long the session was held. Every connect in a debug log is now paired with a close, so the next person suspecting a leaked FTP connection can settle it from a support bundle rather than by inference. Nothing about the connection handling itself changed, and at default log level nothing new is printed. This does not explain the SD-card read/write error in that report or in #645; it only removes one theory from the list by making it checkable. ### Fixed +- **The Finance sidebar entry was hidden from every non-admin, whatever their permissions (#3023, reported by @lonix)** — `cost_centers:read_own` exists so a user can see their own wallet, balance and cost-centre spend, and the page itself always worked if you typed the URL. The sidebar hid it anyway. It decides whether to offer Finance by reading `billing_enabled` from `GET /settings`, which requires `settings:read`; a non-admin gets 403 there, so the value arrived undefined and the entry was suppressed for exactly the people the permission was written for. The shell read three more fields from that same 403, and one of them failed the other way: the Notifications gate tests for `false`, which an undefined value never is, so an administrator who switched user notifications **off** still left the entry showing to the non-admins it governs — a fault no administrator could reproduce, because administrators can read `/settings`. The sponsor prompt also showed EUR regardless of the configured currency, and the update check ran on installs where it had been turned off. `settings:read` cannot be the price of any of this: it also grants sight of the SMTP, LDAP and MQTT credentials. These four now come from a new `GET /settings/ui-flags`, which asks only that the caller be signed in. It is deliberately not more fields on the existing public `/settings/ui-preferences`: that endpoint is served to anyone at all because its contents are defaults shipped with the app, while these describe how your particular install is configured, so they sit behind authentication instead. - **A queued print showed "ASAP" in the queue even when Queue was the option chosen (#3018, reported by @kilrah; also #2557, reported by @ddavidebor)** — the Print dialog offers ASAP, Queue and Schedule, but ASAP and Queue differ only in where the item lands in the list, and neither is recorded on the item itself. The queue's time column had nothing to read but the scheduled time, so it labelled every unscheduled item "ASAP" — the name of the one mode the user may well not have picked. Someone who chose Queue, watched the row appear as ASAP and then watched it start, reasonably concluded Bambuddy had overridden them. That column answers when an item runs, so it now says that: **When a printer is free**, in all fourteen languages. The dispatch itself was correct and is unchanged — a print scheduled for later does not reserve the printer until then, so an unscheduled item behind it uses the printer in the meantime rather than leaving it idle until the scheduled time. The queue's own per-printer log line was no help in telling those apart either: it called a printer "not available" whether the queue could not use it or had just picked it for this pass, and printed printer state read at the moment of logging rather than the state the decision was made on — so a support bundle could show "printer 1 not available — state=IDLE" immediately above that printer receiving a job. Each printer now reports which of the two it was, and why. - **An archive left empty by a printer refusing FTPS blamed the slicer (#2780, reported by @AntonPalmqvist)** — when a printer's file service answers port 990 with something that is not TLS, Bambuddy pauses transfers to it and the print in flight is archived with its name and timing only. That cause has been recorded on the archive since #2957, but the Archives banner was never taught about it: it ranked the three causes it knew, found none of them, and fell back to the original wording — the slicer did not leave the file on the card, go and switch on "Store sent files on external storage", here is a link to installation step 4. Every part of that is wrong for a refused handshake. The slicer did write the file, the reporter could see it on the stick from his own computer, the setting was already on, and there is nothing on his side to change. Worse, the banner dismisses one-shot into browser storage, so being shown the wrong explanation once meant never being shown the right one. It now has its own wording, in all fourteen languages, saying that the printer refused the connection, that this is not a slicer setting and not something the operator did, that Bambuddy comes back for the file after the five-minute pause so a brief episode fills itself in, and that a card still empty means the refusal outlasted the retry. It links to the troubleshooting entry for the handshake failure rather than to the installation guide. This cause now outranks the other three rather than falling to the bottom: the other three describe an install working as configured and each ends in something the operator can change, while this one reports a fault we cannot yet explain — and given the one-shot dismissal, ranking it below anything else would have hidden it permanently. It cannot mask a permanent cause in return, because the retry clears the marker when it succeeds, so a card still carrying this one is a printer whose file service is still refusing. The wiki page these reports are pointed at has also been corrected: it told people to power-cycle the printer, which the reporter who prompted that advice did on both of his printers to no effect, and which the code stopped saying for that reason. It now states what has been measured, says plainly that the trigger is unknown, and names the one log line worth collecting. - **Custom filament profiles arrived in the slicer as Generic, or as the Bambu profile they were built on (#3003, reported by @marivo)** — the slot's filament id is the one field a custom profile travels in, and it holds eight characters on the printer. Bambuddy was putting an eighteen-character preset identifier in it whenever it could not find a real filament id. The printer stored the first eight and reported success, so the slot pointed at something that resolves nowhere: the slicer showed Generic and the printer's calibration table, keyed by the same field, no longer had a slot to key. Three printers in the support archive show it happening — an A1, a P1S and an H2D — so it was never specific to one model. Bambuddy now sends the slot's existing filament id, or the generic one for the material, both of which fit. Orca Cloud profiles were additionally never looked up at all, so theirs went in as a thirty-six-character identifier and fared worse still; they are now resolved like every other source. Also fixed on the way through: a failed Orca Cloud sign-in left its HTTP connection behind instead of closing it, which went unnoticed while only the settings page could trigger it and would have repeated on every spool assignment once the lookup above started using the same code. A profile that carries no filament id of its own — which is every profile created in OrcaSlicer, whose preset format has no such field — still cannot be told apart from the one it inherits from. diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index a5155f953..70ec145b4 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -11,7 +11,12 @@ from pydantic import BaseModel, Field from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession -from backend.app.core.auth import RequirePermissionIfAuthEnabled, caller_is_api_key, require_energy_cost_update +from backend.app.core.auth import ( + RequirePermissionIfAuthEnabled, + caller_is_api_key, + require_auth_if_enabled, + require_energy_cost_update, +) from backend.app.core.config import settings as app_settings from backend.app.core.database import get_db from backend.app.core.permissions import Permission @@ -490,6 +495,57 @@ async def get_ui_preferences(db: AsyncSession = Depends(get_db)): return {key: dumped[key] for key in _UI_PREFERENCE_FIELDS if key in dumped} +# Install configuration the app shell reads before it can render correctly. +# +# Deliberately a second list rather than more entries in _UI_PREFERENCE_FIELDS. +# That one is served to anyone at all, on the recorded grounds that its contents +# are "public defaults that ship with the app" (test_route_auth_coverage.py), and +# its field set is pinned by a test written to make anyone adding to it stop and +# think. These fields are not defaults -- they are facts about how this +# particular deployment is configured -- so they get their own endpoint at their +# own trust level instead of stretching that charter to fit them. +_UI_FLAG_FIELDS: tuple[str, ...] = ( + # The sidebar hides Finance unless billing is on. Layout read this from + # GET /settings, which requires SETTINGS_READ, so for a non-admin the query + # 403'd, the value arrived undefined, `undefined !== true` held, and the + # entry was hidden from exactly the users cost_centers:read_own exists to + # serve. The page itself was reachable by URL the whole time (#3023). + "billing_enabled", + # Same 403, opposite outcome. That gate tests `=== false`, which undefined + # never satisfies, so an administrator who turned user notifications off + # still left the entry showing -- to precisely the non-admins it governs. + "user_notifications_enabled", + # Not gates, but read by the shell and equally undefined for a non-admin: + # the sponsor prompt fell back to EUR whatever the install uses, and the + # update check ran even where it had been switched off. + "currency", + "check_updates", +) + + +@router.get("/ui-flags") +async def get_ui_flags( + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_auth_if_enabled), +): + """Install configuration the app shell needs, for any signed-in user. + + Gated on being authenticated rather than on ``SETTINGS_READ``. The sidebar + has to know whether billing is enabled before it can decide whether to offer + Finance, and ``SETTINGS_READ`` cannot be the price of knowing that -- it also + grants sight of the SMTP, LDAP and MQTT credentials. + + ``require_auth_if_enabled`` returns ``None`` when auth is switched off + entirely, which is the case /ui-preferences was left ungated for. That is the + distinction the two endpoints draw: "works when there is no auth" is not the + same statement as "readable by anyone", and conflating them is what put a + settings read in front of a permission that was never meant to require one. + """ + full = await _build_settings_response(db, is_api_key=False) + dumped = full.model_dump() + return {key: dumped[key] for key in _UI_FLAG_FIELDS if key in dumped} + + @router.get("/check-ffmpeg") async def check_ffmpeg( _: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_READ), diff --git a/backend/tests/integration/test_settings_ui_flags_3023.py b/backend/tests/integration/test_settings_ui_flags_3023.py new file mode 100644 index 000000000..ec6192aff --- /dev/null +++ b/backend/tests/integration/test_settings_ui_flags_3023.py @@ -0,0 +1,169 @@ +"""The app shell can read install configuration without settings:read (#3023). + +Reporter @lonix: a user holding `cost_centers:read_own` never saw the Finance +entry in the sidebar. The permission map was right and the route guard was +right -- navigating to /finance directly worked and showed their balance. What +hid it was an extra condition, `billing_enabled !== true`, read from +GET /settings, which requires SETTINGS_READ. A non-admin gets 403 there, so the +value arrived undefined and the entry was hidden from exactly the users the +permission exists to serve. + +SETTINGS_READ cannot be the price of knowing whether billing is on: it also +grants sight of the SMTP, LDAP and MQTT credentials. Hence /settings/ui-flags, +which asks only that the caller be signed in. + +It is deliberately not more fields on /settings/ui-preferences. That endpoint is +served to anyone at all, on the recorded grounds that its contents are "public +defaults that ship with the app" (test_route_auth_coverage.py), and its field +set is pinned by a test written to stop exactly this kind of addition. These +fields are not defaults -- they say how this deployment is configured -- so the +last test here pins that they did not leak into it. +""" + +import secrets + +import pytest +from httpx import AsyncClient + +from backend.app.models.settings import Settings + +FLAGS_URL = "/api/v1/settings/ui-flags" +_FIXTURE_PW = "Aa1!" + secrets.token_urlsafe(12) # pragma: allowlist secret + + +async def _setup_admin(async_client: AsyncClient, username: str) -> str: + await async_client.post( + "/api/v1/auth/setup", + json={"auth_enabled": True, "admin_username": username, "admin_password": _FIXTURE_PW}, + ) + login = await async_client.post( + "/api/v1/auth/login", + json={"username": username, "password": _FIXTURE_PW}, + ) + assert login.status_code == 200, login.text + return login.json()["access_token"] + + +async def _create_operator( + async_client: AsyncClient, + admin_token: str, + *, + username: str, + permissions: list[str], +) -> str: + """A non-admin holding exactly `permissions` -- never settings:read.""" + headers = {"Authorization": f"Bearer {admin_token}"} + grp = await async_client.post( + "/api/v1/groups/", + headers=headers, + json={"name": f"ui_flags_test_{username}", "permissions": permissions}, + ) + assert grp.status_code == 201, grp.text + user = await async_client.post( + "/api/v1/users/", + headers=headers, + json={ + "username": username, + "password": _FIXTURE_PW, + "role": "user", + "group_ids": [grp.json()["id"]], + }, + ) + assert user.status_code == 201, user.text + assert user.json()["is_admin"] is False + login = await async_client.post( + "/api/v1/auth/login", + json={"username": username, "password": _FIXTURE_PW}, + ) + assert login.status_code == 200, login.text + return login.json()["access_token"] + + +@pytest.mark.integration +class TestTheUserTheEndpointExistsFor: + """A non-admin with cost_centers:read_own and nothing else.""" + + @pytest.mark.asyncio + async def test_they_can_read_the_flags(self, async_client: AsyncClient): + admin = await _setup_admin(async_client, "flagadmin1") + op = await _create_operator(async_client, admin, username="flagop1", permissions=["cost_centers:read_own"]) + + resp = await async_client.get(FLAGS_URL, headers={"Authorization": f"Bearer {op}"}) + assert resp.status_code == 200, resp.text + assert "billing_enabled" in resp.json() + + @pytest.mark.asyncio + async def test_they_still_cannot_read_settings(self, async_client: AsyncClient): + """The fix must not have widened SETTINGS_READ to get there.""" + admin = await _setup_admin(async_client, "flagadmin2") + op = await _create_operator(async_client, admin, username="flagop2", permissions=["cost_centers:read_own"]) + + resp = await async_client.get("/api/v1/settings/", headers={"Authorization": f"Bearer {op}"}) + assert resp.status_code == 403, resp.text + + @pytest.mark.asyncio + async def test_billing_enabled_carries_the_configured_value(self, async_client: AsyncClient, db_session): + """The whole point: the sidebar tests this for `true`, so it has to be + the real value and a real bool, not a truthy string.""" + admin = await _setup_admin(async_client, "flagadmin3") + op = await _create_operator(async_client, admin, username="flagop3", permissions=["cost_centers:read_own"]) + db_session.add(Settings(key="billing_enabled", value="true")) + await db_session.commit() + + resp = await async_client.get(FLAGS_URL, headers={"Authorization": f"Bearer {op}"}) + assert resp.json()["billing_enabled"] is True + + +@pytest.mark.integration +class TestTheBoundaryItDraws: + """Signed in is required; settings:read is not.""" + + @pytest.mark.asyncio + async def test_an_anonymous_caller_is_refused_when_auth_is_on(self, async_client: AsyncClient): + """This is the reason it is a separate endpoint rather than four more + fields on the public one.""" + await _setup_admin(async_client, "flagadmin4") + + resp = await async_client.get(FLAGS_URL) + assert resp.status_code in (401, 403), resp.text + + @pytest.mark.asyncio + async def test_it_answers_when_auth_is_switched_off(self, async_client: AsyncClient): + """An install with no auth has no user to authenticate, and the shell + still has to render. require_auth_if_enabled returns None there.""" + resp = await async_client.get(FLAGS_URL) + assert resp.status_code == 200, resp.text + + +@pytest.mark.integration +class TestWhatItExposes: + @pytest.mark.asyncio + async def test_the_field_set_is_exactly_these_four(self, async_client: AsyncClient): + """Pinned like the /ui-preferences set: anything added here is readable + by every signed-in user, so adding one should require editing this.""" + resp = await async_client.get(FLAGS_URL) + assert set(resp.json().keys()) == { + "billing_enabled", + "user_notifications_enabled", + "currency", + "check_updates", + } + + @pytest.mark.asyncio + async def test_no_credential_ever_appears(self, async_client: AsyncClient, db_session): + for i, key in enumerate( + ("smtp_password", "ldap_bind_password", "mqtt_password", "ha_token", "prometheus_token") + ): + db_session.add(Settings(key=key, value=f"SECRET_VALUE_{i}_DO_NOT_LEAK")) + await db_session.commit() + + body = (await async_client.get(FLAGS_URL)).text + assert "DO_NOT_LEAK" not in body + + @pytest.mark.asyncio + async def test_the_public_endpoint_did_not_gain_them(self, async_client: AsyncClient): + """These describe the deployment, not app defaults, so they must not + have been added to the endpoint that serves anyone at all.""" + public = (await async_client.get("/api/v1/settings/ui-preferences")).json() + assert "billing_enabled" not in public + assert "user_notifications_enabled" not in public diff --git a/frontend/src/__tests__/components/Layout.test.tsx b/frontend/src/__tests__/components/Layout.test.tsx index a2574f420..7dda8433f 100644 --- a/frontend/src/__tests__/components/Layout.test.tsx +++ b/frontend/src/__tests__/components/Layout.test.tsx @@ -2,10 +2,11 @@ * Tests for the Layout component. */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { waitFor } from '@testing-library/react'; import { render } from '../utils'; import { Layout } from '../../components/Layout'; +import { getAuthToken, setAuthToken } from '../../api/client'; import { http, HttpResponse } from 'msw'; import { server } from '../mocks/server'; import { SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, SIDEBAR_ORDER_KEY } from '../../utils/sidebarLayout'; @@ -39,6 +40,16 @@ describe('Layout', () => { auto_archive: true, }); }), + // What the sidebar actually gates on. Layout used to read these from + // /settings/, which a non-admin cannot fetch (#3023). + http.get('/api/v1/settings/ui-flags', () => { + return HttpResponse.json({ + check_updates: false, + billing_enabled: false, + user_notifications_enabled: true, + currency: 'EUR', + }); + }), http.get('/api/v1/external-links/', () => { return HttpResponse.json([]); }), @@ -173,12 +184,12 @@ describe('Layout', () => { it('appears between Statistics and Settings once billing is on', async () => { server.use( - http.get('/api/v1/settings/', () => + http.get('/api/v1/settings/ui-flags', () => HttpResponse.json({ check_updates: false, - check_printer_firmware: false, - auto_archive: true, billing_enabled: true, + user_notifications_enabled: true, + currency: 'EUR', }), ), ); @@ -196,6 +207,127 @@ describe('Layout', () => { }); }); + describe('Sidebar gates survive a user who cannot read /settings (#3023)', () => { + // Every gate below used to be fed by GET /settings, which requires + // settings:read. A non-admin gets 403 there, so the value arrived + // undefined and each gate silently took its fallback -- in opposite + // directions, which is why only one of the two was ever reported. + let priorToken: string | null = null; + + const asNonAdmin = (permissions: string[]) => { + server.use( + http.get('/api/v1/auth/status', () => + HttpResponse.json({ auth_enabled: true, requires_setup: false }), + ), + http.get('/api/v1/auth/me', () => + HttpResponse.json({ + id: 2, + username: 'operator', + role: 'user', + is_active: true, + is_admin: false, + groups: [{ id: 2, name: 'Operators' }], + permissions, + created_at: '2026-01-01T00:00:00Z', + }), + ), + // The 403 that started it. Layout must not need this call at all. + http.get('/api/v1/settings/', () => + HttpResponse.json({ detail: 'Not enough permissions' }, { status: 403 }), + ), + ); + // localStorage is a no-op mock in setup.ts, so writing the key there + // authenticates nobody. Set the client's token directly. + priorToken = getAuthToken(); + setAuthToken('test-token', 'session'); + }; + + afterEach(() => { + setAuthToken(priorToken, 'session'); + priorToken = null; + }); + + it('shows Finance to a user with cost_centers:read_own and no settings:read', async () => { + asNonAdmin(['cost_centers:read_own']); + server.use( + http.get('/api/v1/settings/ui-flags', () => + HttpResponse.json({ billing_enabled: true, user_notifications_enabled: true }), + ), + ); + + render(); + + await waitFor(() => { + expect(document.querySelector('aside a[href="/finance"]')).toBeInTheDocument(); + }); + }); + + it('still hides Finance from that user when billing is off', async () => { + // Waits on Notifications appearing rather than on the sidebar existing. + // Asserting absence the moment