Give backfilled group permissions once, not on every start (#3238)

Four permission backfills in seed_default_groups ran on every start, so
removing MakerWorld, clear plate, forecasting or pipelines from a group
was undone by the next restart. Each now runs once behind a settings
flag. On the upgrade that adds the flag, a backfill whose permission the
Administrators group already held at the start of seeding is recorded as
done without running, so nothing an admin removed comes back one last
time. The Operators/Viewers block stays per-start: those system groups
can't be edited, so it only keeps them complete.

CHANGELOG.md

@@ -117,6 +117,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 
 ### Fixed
+- **Permissions taken away from a group came back after a restart (#3238, reported by @Minebuddy)** — Turning off **View MakerWorld** and **Import MakerWorld** for a group and restarting Bambuddy turned them back on. Several permissions added in past releases were given to every matching group on each start, not just once on the upgrade that introduced them: MakerWorld, **Clear plate**, stock forecasting and slicer pipelines. Each is now given once, and a permission you remove stays removed. Updating doesn't re-enable anything you have already turned off. The built-in Administrators, Operators and Viewers groups, whose permissions can't be edited, are still kept complete on each start.
 - **A virtual printer with Save AMS mapping lost the slicer's external-spool pick (#3237, reported by @erabti)** — The slicer marks a filament fed from the external spool as -1 in its slot list, the same as a filament with no slot, and says which spool it is in a second list. Bambuddy saved only the first list, so the queued print sent that filament as unassigned. On an H2C the printer then stopped before the first layer with 0700-8012, "Failed to get AMS mapping table". The saved mapping now keeps the external spool, left or right on dual-nozzle printers, for the queued print and for reprints from the archive.
 - **A smart plug that reports energy in watt-seconds could not be set up (reported by @CLKRUN in #1251)** — A myStrom Switch reports energy in watt-seconds, and converting that to kWh needs a multiplier of about 0.000000278. The multiplier fields refused anything below 0.0001, so saving the plug failed. Any multiplier above zero is now accepted, for MQTT and REST plugs alike.
 - **The Statistics energy total stayed at 0 for Shelly and other REST plugs with only a lifetime counter (#3232, reported and contributed by @rojosinalma in #3233)** — With **Total consumption** tracking and no date filter, the Statistics page adds up each plug's lifetime counter, but for REST plugs it added the counter for today. Since #2539, a Shelly is set up with only **Lifetime Energy JSON Path**, as it has no counter for today, so its plug added nothing and the energy total and cost showed 0, while any date range showed the right figure. REST plugs now add their lifetime counter, the same as Tasmota and Home Assistant plugs. A plug with only a counter for today still adds that one.

backend/app/core/database.py

@@ -6235,6 +6235,28 @@ async def seed_default_groups():
         result = await session.execute(select(Group))
         existing_groups = {group.name: group for group in result.scalars().all()}
 
+        # The permission backfills below that reach custom groups run once
+        # each (#3238): an admin who takes a permission away from a group
+        # keeps it taken away. They used to run on every start, so a flag
+        # alone would hand everything back one last time on the upgrade that
+        # adds it. Whether a backfill already ran is read off the
+        # Administrators group as it was before this start changed anything:
+        # it holds every permission once a version that knew it has started,
+        # and each backfill shipped with the permission it is checked against.
+        # Taken now, because the Administrators sync below would otherwise make
+        # every later backfill look done on the very start that should run it.
+        # A wrong guess can only run a backfill once more, never skip one that
+        # is due.
+        admin_at_start = existing_groups.get("Administrators")
+        admin_perms_at_start = set(admin_at_start.permissions or []) if admin_at_start is not None else None
+
+        async def _backfill_due(flag_key: str, shipped_with: str) -> bool:
+            flag = (await session.execute(select(Settings).where(Settings.key == flag_key))).scalar_one_or_none()
+            if flag is not None:
+                return False
+            session.add(Settings(key=flag_key, value="true"))
+            return admin_perms_at_start is None or shipped_with not in admin_perms_at_start
+
         # Create default groups if they don't exist
         groups_created = []
         for group_name, group_config in DEFAULT_GROUPS.items():
@@ -6305,16 +6327,16 @@ async def seed_default_groups():
         await session.commit()
 
         # Migrate new permissions: grant printers:clear_plate to all groups with printers:control
-        result = await session.execute(select(Group))
-        all_groups = result.scalars().all()
-        for group in all_groups:
-            if (
-                group.permissions
-                and "printers:control" in group.permissions
-                and "printers:clear_plate" not in group.permissions
-            ):
-                group.permissions = [*group.permissions, "printers:clear_plate"]
-                logger.info("Added printers:clear_plate to group '%s' (has printers:control)", group.name)
+        if await _backfill_due("_backfill_446_clear_plate_permission_done", "printers:clear_plate"):
+            result = await session.execute(select(Group))
+            for group in result.scalars().all():
+                if (
+                    group.permissions
+                    and "printers:control" in group.permissions
+                    and "printers:clear_plate" not in group.permissions
+                ):
+                    group.permissions = [*group.permissions, "printers:clear_plate"]
+                    logger.info("Added printers:clear_plate to group '%s' (has printers:control)", group.name)
         await session.commit()
 
         # Migrate new permissions for MakerWorld integration: groups that
@@ -6323,24 +6345,25 @@ async def seed_default_groups():
         # groups that only have library:read get makerworld:view (browse
         # only). Matches the intent of DEFAULT_GROUPS without clobbering
         # any user-customised permission lists.
-        result = await session.execute(select(Group))
-        for group in result.scalars().all():
-            if not group.permissions:
-                continue
-            perms = list(group.permissions)
-            changed = False
-            if "library:upload" in perms:
-                for new_perm in ("makerworld:view", "makerworld:import"):
-                    if new_perm not in perms:
-                        perms.append(new_perm)
-                        changed = True
-                        logger.info("Added %s to group '%s' (has library:upload)", new_perm, group.name)
-            elif "library:read" in perms and "makerworld:view" not in perms:
-                perms.append("makerworld:view")
-                changed = True
-                logger.info("Added makerworld:view to group '%s' (has library:read)", group.name)
-            if changed:
-                group.permissions = perms
+        if await _backfill_due("_backfill_1099_makerworld_permissions_done", "makerworld:view"):
+            result = await session.execute(select(Group))
+            for group in result.scalars().all():
+                if not group.permissions:
+                    continue
+                perms = list(group.permissions)
+                changed = False
+                if "library:upload" in perms:
+                    for new_perm in ("makerworld:view", "makerworld:import"):
+                        if new_perm not in perms:
+                            perms.append(new_perm)
+                            changed = True
+                            logger.info("Added %s to group '%s' (has library:upload)", new_perm, group.name)
+                elif "library:read" in perms and "makerworld:view" not in perms:
+                    perms.append("makerworld:view")
+                    changed = True
+                    logger.info("Added makerworld:view to group '%s' (has library:read)", group.name)
+                if changed:
+                    group.permissions = perms
         await session.commit()
 
         # Manyfold (#1471) is a second model source beside MakerWorld, so a
@@ -6411,6 +6434,10 @@ async def seed_default_groups():
         # include it in the DEFAULT_GROUPS bootstrap, so this keeps upgrades
         # consistent. Viewers do NOT get orca_cloud:auth (read-only role,
         # not expected to author slicer presets / sync to Orca Cloud).
+        #
+        # Unlike the backfills around it, this one runs on every start on
+        # purpose: it only touches system groups, whose permissions no one can
+        # edit, so it can never undo an admin's choice and keeps them repaired.
         for non_admin_group_name in ("Operators", "Viewers"):
             grp = (await session.execute(select(Group).where(Group.name == non_admin_group_name))).scalar_one_or_none()
             if grp is None or grp.permissions is None:
@@ -6434,22 +6461,23 @@ async def seed_default_groups():
         # inventory:forecast_read was added after initial seeding, so groups
         # that already have inventory:read (or inventory:update) need it added.
         # inventory:forecast_write goes to any group with inventory:update.
-        result = await session.execute(select(Group))
-        for group in result.scalars().all():
-            if not group.permissions:
-                continue
-            perms = list(group.permissions)
-            changed = False
-            if "inventory:read" in perms and "inventory:forecast_read" not in perms:
-                perms.append("inventory:forecast_read")
-                changed = True
-                logger.info("Added inventory:forecast_read to group '%s' (backfill)", group.name)
-            if "inventory:update" in perms and "inventory:forecast_write" not in perms:
-                perms.append("inventory:forecast_write")
-                changed = True
-                logger.info("Added inventory:forecast_write to group '%s' (backfill)", group.name)
-            if changed:
-                group.permissions = perms
+        if await _backfill_due("_backfill_1184_forecast_permissions_done", "inventory:forecast_read"):
+            result = await session.execute(select(Group))
+            for group in result.scalars().all():
+                if not group.permissions:
+                    continue
+                perms = list(group.permissions)
+                changed = False
+                if "inventory:read" in perms and "inventory:forecast_read" not in perms:
+                    perms.append("inventory:forecast_read")
+                    changed = True
+                    logger.info("Added inventory:forecast_read to group '%s' (backfill)", group.name)
+                if "inventory:update" in perms and "inventory:forecast_write" not in perms:
+                    perms.append("inventory:forecast_write")
+                    changed = True
+                    logger.info("Added inventory:forecast_write to group '%s' (backfill)", group.name)
+                if changed:
+                    group.permissions = perms
         await session.commit()
 
         # Backfill pipeline permissions (#1425) for non-admin groups.
@@ -6457,33 +6485,32 @@ async def seed_default_groups():
         #   - Operators: all three (matches fresh-install DEFAULT_GROUPS)
         #   - Any other group with library:read_own or settings:read:
         #     pipelines:read only
-        result = await session.execute(select(Group))
-        for group in result.scalars().all():
-            if not group.permissions or group.name == "Administrators":
-                continue
-            perms = list(group.permissions)
-            changed = False
-            if group.name == "Operators":
-                for new_perm in ("pipelines:read", "pipelines:write", "pipelines:run"):
-                    if new_perm not in perms:
-                        perms.append(new_perm)
-                        changed = True
-                        logger.info("Added %s to Operators group (backfill)", new_perm)
-            elif "pipelines:read" not in perms and ("library:read_own" in perms or "settings:read" in perms):
-                perms.append("pipelines:read")
-                changed = True
-                logger.info("Added pipelines:read to group '%s' (backfill)", group.name)
-            if changed:
-                group.permissions = perms
+        if await _backfill_due("_backfill_1425_pipeline_permissions_done", "pipelines:read"):
+            result = await session.execute(select(Group))
+            for group in result.scalars().all():
+                if not group.permissions or group.name == "Administrators":
+                    continue
+                perms = list(group.permissions)
+                changed = False
+                if group.name == "Operators":
+                    for new_perm in ("pipelines:read", "pipelines:write", "pipelines:run"):
+                        if new_perm not in perms:
+                            perms.append(new_perm)
+                            changed = True
+                            logger.info("Added %s to Operators group (backfill)", new_perm)
+                elif "pipelines:read" not in perms and ("library:read_own" in perms or "settings:read" in perms):
+                    perms.append("pipelines:read")
+                    changed = True
+                    logger.info("Added pipelines:read to group '%s' (backfill)", group.name)
+                if changed:
+                    group.permissions = perms
         await session.commit()
 
         # queue:start_unreviewed (#1620): jobs of users without it wait for
         # someone to start them. Granted once to every group that could queue,
         # start or run jobs before it existed, so nothing changes on upgrade. Once
-        # only, unlike the backfills above: an admin removing it from a group is
-        # the whole point, and a per-boot backfill would hand it straight back.
-        from backend.app.models.settings import Settings
-
+        # only: an admin removing it from a group is the whole point, and a
+        # per-boot backfill would hand it straight back.
         review_flag = "_backfill_1620_queue_start_unreviewed_done"
         if (await session.execute(select(Settings).where(Settings.key == review_flag))).scalar_one_or_none() is None:
             result = await session.execute(select(Group))

backend/tests/integration/test_permission_backfills_once_3238.py

@@ -0,0 +1,155 @@
+"""Group permission backfills run once, not on every start (#3238).
+
+They used to add their permission to every matching group on each start, so
+an admin who took MakerWorld (or clear plate, forecasting, pipelines) away
+from a group got it back after a restart.
+"""
+
+import pytest
+from httpx import AsyncClient
+from sqlalchemy import select
+
+from backend.app.core import database as _database_module
+from backend.app.core.database import seed_default_groups
+from backend.app.models.group import Group
+from backend.app.models.settings import Settings
+
+# (flag, permission the Administrators group has once a version with the
+# backfill started, group permissions that earn it, permission it adds)
+BACKFILLS = [
+    pytest.param(
+        "_backfill_446_clear_plate_permission_done",
+        "printers:clear_plate",
+        ["printers:control"],
+        "printers:clear_plate",
+        id="clear_plate",
+    ),
+    pytest.param(
+        "_backfill_1099_makerworld_permissions_done",
+        "makerworld:view",
+        ["library:upload"],
+        "makerworld:import",
+        id="makerworld",
+    ),
+    pytest.param(
+        "_backfill_1184_forecast_permissions_done",
+        "inventory:forecast_read",
+        ["inventory:read"],
+        "inventory:forecast_read",
+        id="forecast",
+    ),
+    pytest.param(
+        "_backfill_1425_pipeline_permissions_done",
+        "pipelines:read",
+        ["settings:read"],
+        "pipelines:read",
+        id="pipelines",
+    ),
+]
+
+
+async def _set_group(name: str, permissions: list[str]) -> None:
+    async with _database_module.async_session() as session:
+        group = (await session.execute(select(Group).where(Group.name == name))).scalar_one_or_none()
+        if group is None:
+            session.add(Group(name=name, permissions=permissions, is_system=False))
+        else:
+            group.permissions = permissions
+        await session.commit()
+
+
+async def _perms(name: str) -> set[str]:
+    async with _database_module.async_session() as session:
+        group = (await session.execute(select(Group).where(Group.name == name))).scalar_one()
+        return set(group.permissions or [])
+
+
+async def _upgrade_from(flag: str, marker: str, *, knew_it: bool) -> None:
+    """Make the next start an upgrade from a version without the flag.
+
+    ``knew_it`` is whether that version already had the permission, which
+    leaves ``marker`` on Administrators.
+    """
+    async with _database_module.async_session() as session:
+        row = (await session.execute(select(Settings).where(Settings.key == flag))).scalar_one_or_none()
+        if row is not None:
+            await session.delete(row)
+        admin = (await session.execute(select(Group).where(Group.name == "Administrators"))).scalar_one()
+        perms = [p for p in admin.permissions if p != marker]
+        admin.permissions = [*perms, marker] if knew_it else perms
+        await session.commit()
+
+
+async def _flag_set(flag: str) -> bool:
+    async with _database_module.async_session() as session:
+        return (await session.execute(select(Settings).where(Settings.key == flag))).scalar_one_or_none() is not None
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_makerworld_stays_off_after_restart(async_client: AsyncClient):
+    """The report: MakerWorld turned off for a group with library:upload."""
+    await _set_group("student", ["library:read_own", "library:upload"])
+    await seed_default_groups()
+    await seed_default_groups()
+
+    assert "makerworld:view" not in await _perms("student")
+    assert "makerworld:import" not in await _perms("student")
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+@pytest.mark.parametrize(("flag", "marker", "earns", "added"), BACKFILLS)
+async def test_removed_permission_stays_removed(async_client: AsyncClient, flag, marker, earns, added):
+    await _set_group("custom", earns)
+    await seed_default_groups()
+    await seed_default_groups()
+
+    assert added not in await _perms("custom")
+    assert await _flag_set(flag)
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+@pytest.mark.parametrize(("flag", "marker", "earns", "added"), BACKFILLS)
+async def test_upgrade_from_a_version_that_already_ran_it(async_client: AsyncClient, flag, marker, earns, added):
+    """Before #3238 the backfill ran on every start; the start that adds the
+    flag must not hand back what an admin removed since."""
+    await _set_group("custom", earns)
+    await _upgrade_from(flag, marker, knew_it=True)
+
+    await seed_default_groups()
+
+    assert added not in await _perms("custom")
+    assert await _flag_set(flag)
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+@pytest.mark.parametrize(("flag", "marker", "earns", "added"), BACKFILLS)
+async def test_upgrade_from_before_the_permission(async_client: AsyncClient, flag, marker, earns, added):
+    """The backfill still runs on an install that never had the permission,
+    including those after the Administrators sync, which adds the permission
+    to Administrators on that same start."""
+    await _set_group("custom", earns)
+    await _upgrade_from(flag, marker, knew_it=False)
+
+    await seed_default_groups()
+    assert added in await _perms("custom")
+
+    # Taken away again, it stays away.
+    await _set_group("custom", earns)
+    await seed_default_groups()
+    assert added not in await _perms("custom")
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_administrators_still_get_every_permission(async_client: AsyncClient):
+    """Administrators are synced on every start; that is not a backfill."""
+    await seed_default_groups()
+    await _set_group("Administrators", ["settings:read"])
+
+    await seed_default_groups()
+
+    assert {"makerworld:view", "makerworld:import", "printers:clear_plate"} <= await _perms("Administrators")