mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-08 23:21:58 +02:00
fix(auth): preserve manually-assigned groups across LDAP logins (#1292)
_sync_ldap_user used to replace user.groups entirely on every login, wiping manual admin assignments to groups outside the LDAP mapping. Now partitions on LDAP-managed group names (mapping values + default group) and only rebuilds that slice from LDAP truth. Manual assignments to non-managed groups are preserved; revocation in LDAP still propagates for managed groups.
This commit is contained in:
maziggy
@@ -32,6 +32,8 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **Copy spool — duplicate any spool's settings into a fresh inventory row in two clicks** ([#1234](https://github.com/maziggy/bambuddy/issues/1234), [PR #1246](https://github.com/maziggy/bambuddy/pull/1246) by @MiguelAngelLV) — Adds a copy button (`Copy` icon) next to the existing edit button on every spool in the inventory page across all three views (table row, card, grouped table inner row). Clicking it opens the existing `SpoolFormModal` pre-filled with every field from the source spool — material, brand, color, slicer preset, label/core/cost, K-profiles, all of it — except `weight_used` which is reset to 0 (since the new spool starts full) and the RFID identity fields (`tag_uid`, `tray_uuid`, `tag_type`, `data_origin`) which aren't part of the form payload anyway, so the new spool is its own physical roll. Save calls `api.createSpool` (or `api.createSpoolmanInventorySpool` in Spoolman mode — both inherit the dispatch routing for free). Closes the long-running gap where users with many near-identical spools (e.g. five 1 kg PETG-CF rolls bought in a single order) had to re-enter every field from scratch on each one. **Implementation shape:** `SpoolFormModalProps.mode: 'create' | 'edit' | 'copy'` (exported as `SpoolFormMode`) replaces the previous `isEditing = !!spool` heuristic — every existing call site in `InventoryPage.tsx` was updated to pass the explicit mode, and the modal's title / submit-button label / weight-reset gate / submit-route branching all key on `mode` directly. The `onCopy` callback is optional on `SpoolCard`, `SpoolTableRow`, and `SpoolTableGroup` (matches the existing `onPrintLabel?` pattern), so the button is conditionally rendered and other consumers of those subcomponents don't get a copy affordance forced on them. Card-view and table-row buttons stop click propagation so clicking copy doesn't also fire the parent row's edit handler. **Quick Add interaction:** the Quick Add toggle is gated `mode === 'create'` (was `!isEditing`), so it stays out of copy mode — otherwise a user could enable Quick Add and bump quantity to N under the singular "Copy Spool" title and silently bulk-create N copies via `bulkCreateMutation`. **i18n:** new `inventory.copySpool` key across all 8 locales (en + de translated, fr/it/ja/pt-BR/zh-CN/zh-TW seeded with English fallback per project flow). **Tests:** 3 new in `SpoolFormModal.test.tsx` (`SpoolFormModal copy mode` describe block — title shows "Copy Spool", save calls `createSpool` not `updateSpool`, `weight_used` reset to 0 in the create payload when copying a spool with non-zero usage), 2 new in `InventoryPageCopyButton.test.tsx` (table-row copy button click → "Copy Spool" heading, cards-view copy button click → same heading after switching view modes) — guards against the three call sites drifting apart. Existing `SpoolFormBulk.test.tsx` and `SpoolFormModal.test.tsx` renders that omitted the `mode` prop were updated with the explicit `mode="create"` so the tightened Quick Add gate doesn't hide the toggle from them. Both `InventoryPageCopyButton.test.tsx` and `InventoryPageDeepLink.test.tsx` gained MSW handlers for the modal's open-time fetches (`/api/v1/cloud/status`, `/api/v1/cloud/local-presets`, `/api/v1/cloud/builtin-filaments`, `/api/v1/inventory/color-catalog`, `/api/v1/inventory/spool-catalog`, `/api/v1/printers/`) — without them MSW passes through to the real network, ECONNREFUSEs, and the rejected fetch resolves after the test environment is torn down, surfacing as a flaky "window is not defined" unhandled rejection in the modal's `setLoadingCloudPresets(false)` finally block (pre-existing flake hit ~1 in 3 full-suite runs at PR head).
|
||||
|
||||
### Fixed
|
||||
- **LDAP user logins wiped manually-assigned BamBuddy groups** ([#1292](https://github.com/maziggy/bambuddy/issues/1292), reported by @Fuechslein) — When an admin assigned an LDAP-authenticated user a BamBuddy group that wasn't mapped from LDAP (e.g. "Administrators" while the LDAP mapping only covered "Users"), the assignment vanished on the user's next login. The reporter's observation matched the code exactly: assigning a group while the user was logged in held until the next login because `user.groups` was just mutated in memory; on next login, `_sync_ldap_user` in `backend/app/api/routes/auth.py:1187` rebuilt `user.groups` from LDAP state alone and blew away the manual assignment. The design intent (LDAP truth must propagate, including revocation) was correct, but the implementation was over-broad — every BamBuddy group got wiped, not just LDAP-mapped ones. **Fix:** `_sync_ldap_user` now computes the set of "LDAP-managed" BamBuddy group names = values of `ldap_group_mapping` ∪ `{ldap_default_group}`. Groups inside that set are still rebuilt from LDAP truth on each login (so revocation works). Groups outside that set are treated as manual admin assignments and preserved. The partition happens via a list comprehension over `user.groups`; no schema or DDL change. Edge case explicitly tested: a manual assignment to a group that IS in the LDAP mapping is still overridden by LDAP state — once an assignment is in the user_groups table you can't tell manual-but-mapped from LDAP-derived, so LDAP wins for any group it has authority over. Regression tests in `backend/tests/integration/test_ldap_group_sync.py` cover: manual group survives login (the reporter's exact scenario), revocation still propagates for LDAP-managed groups, default_group persists across empty-LDAP logins, manual assignment to a managed group is overridden, and the realistic mixed case where a user has multiple manual + multiple LDAP groups at once.
|
||||
|
||||
- **Internal inventory: `storage_location` field was silently dropped on save and never shown in the table** ([#1291](https://github.com/maziggy/bambuddy/issues/1291), reported by @needo37) — The `storage_location` column existed on the Spool ORM model (`backend/app/models/spool.py:57`) but was missing from the Pydantic schemas in `backend/app/schemas/spool.py` (`SpoolBase`, `SpoolUpdate`, and by extension `SpoolResponse`). Pydantic silently strips unknown fields, so PATCH writes to `/inventory/spools/{id}` reached the update route's `model_dump(exclude_unset=True)` already missing the field, the `setattr` loop never touched the DB column, and GET responses left it out — the inventory table always showed "—" in the Storage Location column even when the user had typed and saved a value. Only the internal inventory was affected; Spoolman mode worked because it goes through a separate proxy backend with its own schema. Fix is two added fields in `schemas/spool.py`: one on `SpoolBase` (covers `SpoolCreate` + `SpoolResponse` via inheritance) and one on `SpoolUpdate` (standalone). Both constrained to `max_length=255` to match the DB column's `String(255)`. No route changes needed — the update handler at `inventory.py:961` already uses the generic dump-then-setattr pattern that picks up any new schema field automatically. Note on UX intent: `storage_location` is the user-defined free-text label ("Drybox #1", "Top shelf"), distinct from `location` which is the AMS slot assignment ("AMS-A slot 3") — keeping both is the right call. Regression tests in `test_spool_schemas_storage_location.py` lock in: create/update accept the field, the response surfaces it, explicit-null clears via `exclude_unset` round-trip, omitted-on-PATCH is left untouched (doesn't accidentally clear), and `max_length=255` is enforced (so the API returns a clean 422 instead of a SQLAlchemy column-length error).
|
||||
|
||||
- **Archives page didn't auto-refresh when a slicer sent a print to a Virtual Printer — the new card only appeared after switching tabs** ([#1282](https://github.com/maziggy/bambuddy/issues/1282), reported by @kleinwareio) — Real-printer prints broadcast `archive_created` over the WebSocket from `main.py`'s MQTT `print_start` handler, and the Archives page listens for that event in `frontend/src/hooks/useWebSocket.ts:241` to invalidate its react-query cache. The VP file-receive paths in `backend/app/services/virtual_printer/manager.py` (`_archive_file` for immediate mode and `_add_to_print_queue` for queue mode) created the archive and committed it to the DB but never broadcast the event — so the page stayed stale until the user clicked another tab and back, which triggered a refetch on focus. **Fix:** factored a small `_broadcast_archive_created(archive)` helper onto `VirtualPrinterInstance` that imports `ws_manager` lazily (matches the file's existing late-import convention for archive/queue imports) and emits the same `{id, printer_id, filename, print_name, status}` payload shape `main.py` uses. Called from both VP paths immediately after the archive is logged (`_archive_file`) and after the queue item is committed (`_add_to_print_queue`). Broadcast failures are swallowed at debug level so a transient WebSocket issue can't break the file-receive flow. The review mode path (`_queue_file`) is intentionally untouched — it creates a `PendingUpload`, not a `PrintArchive`, and renders on a different page. **Tests:** `test_archive_file_broadcasts_archive_created` and `test_add_to_print_queue_broadcasts_archive_created` patch `ws_manager.send_archive_created` and assert it's called once with the right payload shape. **Affects:** every Bambuddy install using a VP in `immediate` or `print_queue` mode; review mode and proxy mode are unaffected.
|
||||
|
||||
@@ -1185,7 +1185,15 @@ async def _provision_ldap_user(db: AsyncSession, ldap_user, ldap_config) -> User
|
||||
|
||||
|
||||
async def _sync_ldap_user(db: AsyncSession, user: User, ldap_user, ldap_config) -> None:
|
||||
"""Sync LDAP user attributes (email, groups) on each login."""
|
||||
"""Sync LDAP user attributes (email, groups) on each login.
|
||||
|
||||
Group sync only touches BamBuddy groups that LDAP is configured to manage —
|
||||
that is, the values of `group_mapping` plus `default_group`. Any group
|
||||
outside that set is assumed to be a manual admin assignment and is
|
||||
preserved across logins (#1292). Manual assignments to a BamBuddy group
|
||||
that IS LDAP-managed are still overridden by LDAP truth, because revoking
|
||||
access in LDAP must propagate to BamBuddy on next login.
|
||||
"""
|
||||
import logging
|
||||
|
||||
from backend.app.services.ldap_service import resolve_group_mapping
|
||||
@@ -1199,9 +1207,13 @@ async def _sync_ldap_user(db: AsyncSession, user: User, ldap_user, ldap_config)
|
||||
user.email = ldap_user.email
|
||||
changed = True
|
||||
|
||||
# Sync group mappings — always update to match LDAP state (including revocation).
|
||||
# Fall back to the configured default group when the user has no mapped groups,
|
||||
# so authenticated LDAP users are never left permission-less.
|
||||
# Compute the set of BamBuddy groups LDAP is allowed to manage. Anything
|
||||
# outside this set is left alone so manual admin assignments survive logins.
|
||||
ldap_managed_names: set[str] = set(ldap_config.group_mapping.values())
|
||||
if ldap_config.default_group:
|
||||
ldap_managed_names.add(ldap_config.default_group)
|
||||
|
||||
# Resolve what LDAP says the user should currently be in.
|
||||
mapped_group_names = resolve_group_mapping(ldap_user.groups, ldap_config.group_mapping)
|
||||
if not mapped_group_names and ldap_config.default_group:
|
||||
mapped_group_names = [ldap_config.default_group]
|
||||
@@ -1210,11 +1222,18 @@ async def _sync_ldap_user(db: AsyncSession, user: User, ldap_user, ldap_config)
|
||||
user.username,
|
||||
ldap_config.default_group,
|
||||
)
|
||||
|
||||
if mapped_group_names:
|
||||
groups_result = await db.execute(select(Group).where(Group.name.in_(mapped_group_names)))
|
||||
new_groups = list(groups_result.scalars().all())
|
||||
new_ldap_groups = list(groups_result.scalars().all())
|
||||
else:
|
||||
new_groups = []
|
||||
new_ldap_groups = []
|
||||
|
||||
# Preserve manual assignments to non-LDAP-managed groups; replace only
|
||||
# the LDAP-managed slice with the resolved set.
|
||||
preserved_manual_groups = [g for g in user.groups if g.name not in ldap_managed_names]
|
||||
new_groups = preserved_manual_groups + new_ldap_groups
|
||||
|
||||
current_group_ids = {g.id for g in user.groups}
|
||||
new_group_ids = {g.id for g in new_groups}
|
||||
if current_group_ids != new_group_ids:
|
||||
|
||||
@@ -0,0 +1,196 @@
|
||||
"""Regression tests for LDAP user group sync behavior (#1292).
|
||||
|
||||
Reporter @Fuechslein: when an admin manually assigned a BamBuddy group to an
|
||||
LDAP user, the assignment was silently wiped on the user's next login. Cause
|
||||
was that _sync_ldap_user used to replace `user.groups` entirely on every login,
|
||||
overwriting anything not derived from LDAP state.
|
||||
|
||||
The fix partitions the user's groups into "LDAP-managed" (anything in the
|
||||
ldap_group_mapping config values + the default_group) and "manual". Only the
|
||||
LDAP-managed slice is rebuilt from LDAP truth; manual assignments survive.
|
||||
"""
|
||||
|
||||
from dataclasses import dataclass
|
||||
|
||||
import pytest
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from backend.app.api.routes.auth import _sync_ldap_user
|
||||
from backend.app.models.group import Group
|
||||
from backend.app.models.user import User
|
||||
|
||||
|
||||
@dataclass
|
||||
class _FakeLdapUser:
|
||||
"""Stand-in for backend.app.services.ldap_service.LDAPUserInfo."""
|
||||
|
||||
username: str
|
||||
email: str | None
|
||||
groups: list[str]
|
||||
|
||||
|
||||
@dataclass
|
||||
class _FakeLdapConfig:
|
||||
"""Stand-in for backend.app.services.ldap_service.LDAPConfig — only the
|
||||
fields _sync_ldap_user actually reads."""
|
||||
|
||||
group_mapping: dict[str, str]
|
||||
default_group: str = ""
|
||||
|
||||
|
||||
async def _make_group(db: AsyncSession, name: str) -> Group:
|
||||
group = Group(name=name, description=f"Test group {name}")
|
||||
db.add(group)
|
||||
await db.commit()
|
||||
await db.refresh(group)
|
||||
return group
|
||||
|
||||
|
||||
async def _make_ldap_user(db: AsyncSession, username: str, groups: list[Group]) -> User:
|
||||
user = User(
|
||||
username=username,
|
||||
email=f"{username}@example.com",
|
||||
password_hash=None,
|
||||
role="user",
|
||||
auth_source="ldap",
|
||||
is_active=True,
|
||||
)
|
||||
user.groups = groups
|
||||
db.add(user)
|
||||
await db.commit()
|
||||
await db.refresh(user, attribute_names=["groups"])
|
||||
return user
|
||||
|
||||
|
||||
class TestLdapGroupSyncPreservesManualAssignments:
|
||||
"""The #1292 fix: groups outside the LDAP-managed set must survive logins."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_manual_group_survives_login(self, db_session: AsyncSession):
|
||||
"""Admin assigns 'Administrators' to an LDAP user. 'Administrators' is
|
||||
NOT in the LDAP group_mapping. Next login must keep it."""
|
||||
admins = await _make_group(db_session, "Administrators")
|
||||
users = await _make_group(db_session, "Users")
|
||||
|
||||
user = await _make_ldap_user(db_session, "alice", [admins])
|
||||
assert {g.name for g in user.groups} == {"Administrators"}
|
||||
|
||||
ldap_user = _FakeLdapUser(
|
||||
username="alice", email="alice@example.com", groups=["cn=staff,ou=groups,dc=example,dc=com"]
|
||||
)
|
||||
ldap_config = _FakeLdapConfig(
|
||||
group_mapping={"cn=staff,ou=groups,dc=example,dc=com": "Users"},
|
||||
default_group="",
|
||||
)
|
||||
|
||||
await _sync_ldap_user(db_session, user, ldap_user, ldap_config)
|
||||
await db_session.refresh(user, attribute_names=["groups"])
|
||||
|
||||
assert {g.name for g in user.groups} == {"Administrators", "Users"}, (
|
||||
"Manual 'Administrators' assignment must be preserved; LDAP-mapped 'Users' must be added"
|
||||
)
|
||||
# Use the local refs to silence linters about unused locals
|
||||
assert admins.id != users.id
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_default_group_not_treated_as_manual(self, db_session: AsyncSession):
|
||||
"""The default_group is LDAP-managed even though it's not in the mapping
|
||||
values — it gets added when no mapped groups resolve. So if LDAP later
|
||||
revokes all group memberships, the default group stays; if a different
|
||||
default_group is configured, the old one is dropped from the user."""
|
||||
guest = await _make_group(db_session, "Guests")
|
||||
await _make_group(db_session, "Users")
|
||||
|
||||
# User has the (LDAP-managed) Guests group as their default — no manual groups.
|
||||
user = await _make_ldap_user(db_session, "bob", [guest])
|
||||
|
||||
ldap_user = _FakeLdapUser(username="bob", email="bob@example.com", groups=[])
|
||||
ldap_config = _FakeLdapConfig(group_mapping={}, default_group="Guests")
|
||||
|
||||
await _sync_ldap_user(db_session, user, ldap_user, ldap_config)
|
||||
await db_session.refresh(user, attribute_names=["groups"])
|
||||
assert {g.name for g in user.groups} == {"Guests"}, "Default group should persist"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_revocation_in_ldap_still_propagates(self, db_session: AsyncSession):
|
||||
"""The original design intent — revocation in LDAP must flow through — must
|
||||
still work for LDAP-managed groups. User was in 'Users' (LDAP-mapped); LDAP
|
||||
no longer reports the mapped group; sync must remove 'Users'."""
|
||||
users = await _make_group(db_session, "Users")
|
||||
|
||||
user = await _make_ldap_user(db_session, "charlie", [users])
|
||||
assert {g.name for g in user.groups} == {"Users"}
|
||||
|
||||
ldap_user = _FakeLdapUser(username="charlie", email="charlie@example.com", groups=[])
|
||||
ldap_config = _FakeLdapConfig(
|
||||
group_mapping={"cn=staff,ou=groups,dc=example,dc=com": "Users"},
|
||||
default_group="",
|
||||
)
|
||||
|
||||
await _sync_ldap_user(db_session, user, ldap_user, ldap_config)
|
||||
await db_session.refresh(user, attribute_names=["groups"])
|
||||
assert {g.name for g in user.groups} == set(), (
|
||||
"LDAP-managed groups must be removed when LDAP no longer reports the user in them"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_manual_assignment_to_managed_group_still_overridden(self, db_session: AsyncSession):
|
||||
"""If an admin manually assigns a group that IS in the LDAP mapping, LDAP
|
||||
truth still wins — otherwise revoking access in LDAP wouldn't work for
|
||||
users who happened to have manual assignments to the same group. Cannot
|
||||
distinguish manual-but-mapped from LDAP-derived once the assignment is
|
||||
in the DB; resolved by treating any group in the LDAP-managed set as
|
||||
authoritative-by-LDAP."""
|
||||
users = await _make_group(db_session, "Users")
|
||||
|
||||
# Manually assign 'Users' (which IS in the LDAP mapping) to an LDAP user.
|
||||
user = await _make_ldap_user(db_session, "dave", [users])
|
||||
|
||||
# LDAP says the user is in no mapped groups.
|
||||
ldap_user = _FakeLdapUser(username="dave", email="dave@example.com", groups=[])
|
||||
ldap_config = _FakeLdapConfig(
|
||||
group_mapping={"cn=staff,ou=groups,dc=example,dc=com": "Users"},
|
||||
default_group="",
|
||||
)
|
||||
|
||||
await _sync_ldap_user(db_session, user, ldap_user, ldap_config)
|
||||
await db_session.refresh(user, attribute_names=["groups"])
|
||||
assert {g.name for g in user.groups} == set(), (
|
||||
"Manual assignment to an LDAP-managed group is overridden by LDAP state"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mixed_manual_and_ldap_groups(self, db_session: AsyncSession):
|
||||
"""Most realistic scenario: user has multiple manual assignments AND LDAP
|
||||
mapped groups. Manual groups survive; LDAP-managed slice gets rebuilt."""
|
||||
admins = await _make_group(db_session, "Administrators")
|
||||
ops = await _make_group(db_session, "PrintOps")
|
||||
users = await _make_group(db_session, "Users")
|
||||
await _make_group(db_session, "Power Users")
|
||||
|
||||
# User has two manual groups (Administrators, PrintOps) plus one LDAP
|
||||
# group (Users) at the start.
|
||||
user = await _make_ldap_user(db_session, "eve", [admins, ops, users])
|
||||
|
||||
# LDAP login: user is now in two LDAP-mapped groups.
|
||||
ldap_user = _FakeLdapUser(
|
||||
username="eve",
|
||||
email="eve@example.com",
|
||||
groups=["cn=staff,ou=groups,dc=example,dc=com", "cn=power,ou=groups,dc=example,dc=com"],
|
||||
)
|
||||
ldap_config = _FakeLdapConfig(
|
||||
group_mapping={
|
||||
"cn=staff,ou=groups,dc=example,dc=com": "Users",
|
||||
"cn=power,ou=groups,dc=example,dc=com": "Power Users",
|
||||
},
|
||||
default_group="",
|
||||
)
|
||||
|
||||
await _sync_ldap_user(db_session, user, ldap_user, ldap_config)
|
||||
await db_session.refresh(user, attribute_names=["groups"])
|
||||
assert {g.name for g in user.groups} == {
|
||||
"Administrators", # manual, preserved
|
||||
"PrintOps", # manual, preserved
|
||||
"Users", # LDAP-managed, retained from LDAP
|
||||
"Power Users", # LDAP-managed, newly added from LDAP
|
||||
}
|
||||
Reference in New Issue
Block a user