mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 03:01:21 +02:00
fix(auth): let the sidebar read install flags without settings:read (issue #3023)
cost_centers:read_own exists so a non-admin can see their own wallet, balance and cost-centre spend, and the Finance page honoured it -- typing the URL worked and rendered their balance. The sidebar never offered the entry. It decides whether to show Finance by reading billing_enabled from GET /settings, which requires SETTINGS_READ. A non-admin gets 403 there, so the value arrived undefined, `undefined !== true` held, and the entry was hidden from precisely the users the permission was written for. The permission map and the route guard were both already right; only discovery was broken. Three more fields came from that same 403, and one of them failed the other way up. The Notifications gate tests `=== false`, which undefined never satisfies, so an administrator who switched user notifications off still left the entry showing to the non-admins it governs. Nobody reported that one, and no administrator could have reproduced either: administrators can read /settings. The remaining two were quieter -- the sponsor prompt fell back to EUR whatever the install uses, and the update check ran where it had been turned off. SETTINGS_READ cannot be the price of knowing whether billing is on. It also grants sight of the SMTP, LDAP and MQTT credentials, which is the reason /settings/ui-preferences exists at all. So: a second endpoint, GET /settings/ui-flags, carrying those four fields and asking only that the caller be signed in, via the existing require_auth_if_enabled. Layout drops its /settings query altogether, which closes the class rather than the two instances that happened to be visible. Deliberately not four more fields on /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 make anyone adding to it stop and think. These fields are not defaults -- they say how this deployment is configured -- so they get their own endpoint at their own trust level instead of stretching that charter to fit them. require_auth_if_enabled also keeps the auth-disabled case that /ui-preferences was ungated for: "works when there is no auth" and "readable by anyone" are different statements, and conflating them is what put a settings read in front of a permission that never needed one. Twelve tests. Backend pins that the operator can read the flags, that the same operator still gets 403 from /settings, that an anonymous caller is refused when auth is on, that it answers when auth is off, the exact field set, that no credential ever appears, and that the public endpoint did not quietly gain these fields. Frontend pins Finance visible for cost_centers:read_own with /settings returning 403, and Notifications hidden when the flag is off -- each waiting on a positive signal before asserting an absence, so the negative cases cannot pass before the query resolves. Reported by @lonix, who traced it to the queryKey and the route gate.
This commit is contained in:
maziggy
@@ -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),
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user