Просмотр исходного кода

fix(auth): setup 422'd on re-enable when admin user already exists

  The SetupRequest Pydantic schema enforced password complexity unconditionally,
  but the route ignores admin_password entirely when an admin user already
  exists (the common case for re-enabling auth after it was disabled, or for
  LDAP deployments where the local admin is a placeholder). A legitimate
  existing password that predated the complexity rule — or the placeholder the
  form sends in LDAP mode — hit the Pydantic validator before the route body
  could decide it wasn't needed, surfacing as:

      422 Value error, Password must contain at least one special character

  Move the complexity check out of the schema and into the route body, scoped
  to the branch that actually creates a new local admin. Re-enabling auth with
  an existing admin now accepts whatever is in the field; first-time setup
  still rejects weak passwords with a clear 400 including the specific rule
  that was violated.

  Regression coverage in test_auth_api.py::TestAuthSetupAPI:
  - test_setup_weak_password_rejected_when_creating_new_admin — fresh setup
    with "NoSpecial1" → 400, "special character" in detail
  - test_setup_reenable_with_existing_admin_ignores_password — seeds an admin,
    POSTs /setup with a complexity-failing password → 200, admin_created=false
maziggy 4 месяцев назад
Родитель
Сommit
991111327f

+ 1 - 0
CHANGELOG.md

@@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file.
 ## [0.2.4b1] - Unreleased
 
 ### Fixed
+- **Setup: re-enabling auth could 422 on a password the form no longer needs** — after disabling authentication and re-enabling it (common when switching between local auth and LDAP, or recovering from a bad config), the setup form still sends `admin_password` in the body even though the backend route ignores it when an admin user already exists. The `SetupRequest` Pydantic schema enforced password complexity (uppercase + lowercase + digit + special char) unconditionally, so any existing password that predated the complexity rule — or a legitimate LDAP-mode placeholder — triggered `422 Value error, Password must contain at least one special character` before the route body could decide to ignore the field. Complexity validation has moved out of the schema and into the route body, scoped to the branch that actually creates a new local admin. Re-enabling auth with an existing admin (or any LDAP user) now accepts whatever the form sends; fresh first-time setup still rejects weak passwords with a clear 400. Two regression tests added in `test_auth_api.py`: weak password rejected at setup when creating the first admin, weak/placeholder password accepted when an admin already exists.
 - **Queue: batch (quantity>1) double-dispatched onto the same printer** — scheduling an ASAP print with `quantity > 1` could end up with two queue items in `'printing'` status for the same printer, surfaced in the logs as `BUG: Multiple queue items in 'printing' status for printer N`. The scheduler's in-memory `busy_printers` set was seeded empty each tick and only populated after `_start_print` succeeded in the current iteration, so on the next tick (30 s later) `_is_printer_idle()` read the printer's live MQTT state — which on H2D / P1 series lags several seconds behind the print command and still reported `IDLE` / `FINISH` — and dispatched the second batch item onto the already-running printer. `check_queue()` now queries `PrintQueueItem` for `status='printing'` rows and seeds `busy_printers` with their printer IDs before iterating pending items, so any printer with an outstanding dispatched job is excluded regardless of what MQTT currently reports. Regression covered in `test_phantom_print_hardening.py` (`TestBusyPrinterSeedingFromPrintingItems`): seeding query returns printers with `'printing'` rows only, returns empty when none exist, and end-to-end `check_queue()` does not call `_start_print` for a pending item whose printer already has a `'printing'` row even when `_is_printer_idle()` is forced `True`.
 - **Queue: active-item progress bar flashed 100% before dropping to 0%** — immediately after a queue item was dispatched, the per-item progress bar on the Queue page showed 100% (or whatever the prior print's final `mc_percent` was) for the few seconds between dispatch and the printer's MQTT state transitioning to `RUNNING`. Frontend `QueuePage.tsx` read `status.progress` directly from the printer's live MQTT snapshot, which carries over the last reported value from the previous print until the new one starts ticking. The progress bar, remaining time, ETA, and layer counter are now gated on `status.state` being `RUNNING` or `PAUSE`; in any other state (including `FINISH` from the prior print, `IDLE`, or `PREPARE` while heating) the bar renders at 0% with no stale ETA/layer values.
 

+ 12 - 0
backend/app/api/routes/auth.py

@@ -53,6 +53,7 @@ from backend.app.schemas.auth import (
     TestSMTPRequest,
     TestSMTPResponse,
     UserResponse,
+    _validate_password_complexity,
 )
 from backend.app.services.email_service import (
     create_password_reset_link_email_from_template,
@@ -228,6 +229,17 @@ async def setup_auth(request: SetupRequest, db: AsyncSession = Depends(get_db)):
                         detail="Admin username and password are required when enabling authentication (no admin users exist)",
                     )
 
+                # Enforce password complexity only when actually creating a new admin.
+                # Schema-level validation was removed so that re-enabling auth with an
+                # existing admin (or LDAP) doesn't reject whatever placeholder the form sends.
+                try:
+                    _validate_password_complexity(request.admin_password)
+                except ValueError as exc:
+                    raise HTTPException(
+                        status_code=status.HTTP_400_BAD_REQUEST,
+                        detail=str(exc),
+                    )
+
                 # Check if username already exists (shouldn't happen if no admin users exist, but check anyway)
                 existing_user = await get_user_by_username(db, request.admin_username)
                 if existing_user:

+ 5 - 6
backend/app/schemas/auth.py

@@ -108,12 +108,11 @@ class SetupRequest(BaseModel):
     admin_username: str | None = Field(default=None, max_length=150)
     admin_password: str | None = Field(default=None, max_length=256)
 
-    @field_validator("admin_password")
-    @classmethod
-    def validate_admin_password(cls, v: str | None) -> str | None:
-        if v is not None:
-            _validate_password_complexity(v)
-        return v
+    # Password complexity is NOT validated at the schema layer. When re-enabling auth
+    # with an existing admin user (or when LDAP is the auth backend), the frontend
+    # still sends whatever is in the password field but the route ignores it.
+    # Enforcing complexity here would reject those legitimate flows. The route body
+    # applies the check only when a brand-new local admin is actually being created.
 
 
 class SetupResponse(BaseModel):

+ 49 - 0
backend/tests/integration/test_auth_api.py

@@ -70,6 +70,55 @@ class TestAuthSetupAPI:
         assert result["auth_enabled"] is True
         assert result["admin_created"] is True
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_setup_weak_password_rejected_when_creating_new_admin(self, async_client: AsyncClient):
+        """Complexity is enforced only when a new admin is being created."""
+        response = await async_client.post(
+            "/api/v1/auth/setup",
+            json={
+                "auth_enabled": True,
+                "admin_username": "weakpw_admin",
+                "admin_password": "NoSpecial1",
+            },
+        )
+
+        assert response.status_code == 400
+        assert "special character" in response.json()["detail"].lower()
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_setup_reenable_with_existing_admin_ignores_password(self, async_client: AsyncClient, db_session):
+        """Re-enabling auth when an admin already exists must not reject the placeholder
+        password the frontend still sends. Regression for the LDAP re-enable flow that
+        previously 422'd because the Pydantic schema enforced complexity unconditionally.
+        """
+        from backend.app.core.auth import get_password_hash
+        from backend.app.models.user import User
+
+        existing = User(
+            username="existing_admin",
+            password_hash=get_password_hash("DoesNotMatter1!"),
+            role="admin",
+            is_active=True,
+        )
+        db_session.add(existing)
+        await db_session.commit()
+
+        response = await async_client.post(
+            "/api/v1/auth/setup",
+            json={
+                "auth_enabled": True,
+                "admin_username": "irrelevant",
+                "admin_password": "Ihk88LimT",
+            },
+        )
+
+        assert response.status_code == 200
+        result = response.json()
+        assert result["auth_enabled"] is True
+        assert result["admin_created"] is False
+
 
 class TestAuthLoginAPI:
     """Integration tests for /api/v1/auth/login endpoint."""