From 64d8786d8fa46a25de733c0addd88530fb7d74f4 Mon Sep 17 00:00:00 2001 From: Michael Genson <71845777+michael-genson@users.noreply.github.com> Date: Tue, 23 Dec 2025 12:31:53 -0600 Subject: [PATCH] fix: Improve recipe bulk deletion (#6772) --- mealie/repos/repository_factory.py | 7 ++ mealie/repos/repository_recipes.py | 2 +- mealie/routes/recipe/bulk_actions.py | 19 +++- mealie/services/recipe/recipe_bulk_service.py | 8 -- mealie/services/recipe/recipe_service.py | 90 +++++++++++++------ .../user_recipe_tests/test_recipe_owner.py | 30 +++++++ 6 files changed, 119 insertions(+), 37 deletions(-) diff --git a/mealie/repos/repository_factory.py b/mealie/repos/repository_factory.py index 80db31c85..3fda48423 100644 --- a/mealie/repos/repository_factory.py +++ b/mealie/repos/repository_factory.py @@ -5,6 +5,7 @@ from pydantic import UUID4 from sqlalchemy import select from sqlalchemy.orm import Session +from mealie.db.models._model_utils.guid import GUID from mealie.db.models.group import Group, ReportEntryModel, ReportModel from mealie.db.models.group.exports import GroupDataExportsModel from mealie.db.models.group.preferences import GroupPreferencesModel @@ -117,6 +118,12 @@ class AllRepositories: self.group_id = group_id self.household_id = household_id + def uuid_to_str(self, val: UUID4) -> str: + """Helper method to convert a UUID into a database-safe string""" + + dialect = self.session.bind.dialect + return GUID.convert_value_to_guid(val, dialect) or "" + # ================================================================ # Recipe diff --git a/mealie/repos/repository_recipes.py b/mealie/repos/repository_recipes.py index a9982553d..4caadfdb9 100644 --- a/mealie/repos/repository_recipes.py +++ b/mealie/repos/repository_recipes.py @@ -130,7 +130,7 @@ class RepositoryRecipes(HouseholdRepositoryGeneric[Recipe, RecipeModel]): return self._delete_recipe(recipe_in_db) def delete_many(self, values: Iterable) -> list[Recipe]: - query = self._query().filter(self.model.id.in_(values)) + query = self._query().filter(self.model.slug.in_(values)).filter_by(**self._filter_builder()) recipes_in_db = self.session.execute(query).unique().scalars().all() results: list[Recipe] = [] diff --git a/mealie/routes/recipe/bulk_actions.py b/mealie/routes/recipe/bulk_actions.py index 4d34b40ea..0c1755bd0 100644 --- a/mealie/routes/recipe/bulk_actions.py +++ b/mealie/routes/recipe/bulk_actions.py @@ -1,10 +1,11 @@ from functools import cached_property from pathlib import Path -from fastapi import APIRouter, HTTPException +from fastapi import APIRouter, HTTPException, status from pydantic import UUID4 from mealie.core.dependencies.dependencies import get_temporary_zip_path +from mealie.core.exceptions import PermissionDenied from mealie.core.security import create_file_token from mealie.routes._base import BaseUserController, controller from mealie.schema.group.group_exports import GroupDataExport @@ -15,8 +16,9 @@ from mealie.schema.recipe.recipe_bulk_actions import ( DeleteRecipes, ExportRecipes, ) -from mealie.schema.response.responses import SuccessResponse +from mealie.schema.response.responses import ErrorResponse, SuccessResponse from mealie.services.recipe.recipe_bulk_service import RecipeBulkActionsService +from mealie.services.recipe.recipe_service import RecipeService router = APIRouter(prefix="/bulk-actions") @@ -27,6 +29,10 @@ class RecipeBulkActionsController(BaseUserController): def service(self) -> RecipeBulkActionsService: return RecipeBulkActionsService(self.repos, self.user, self.group) + @cached_property + def recipe_service(self) -> RecipeService: + return RecipeService(self.repos, self.user, self.household, self.translator) + # TODO Should these actions return some success response? @router.post("/tag") def bulk_tag_recipes(self, tag_data: AssignTags): @@ -42,7 +48,14 @@ class RecipeBulkActionsController(BaseUserController): @router.post("/delete") def bulk_delete_recipes(self, delete_recipes: DeleteRecipes): - self.service.delete_recipes(delete_recipes.recipes) + # TODO: this route should be migrated to the standard recipe controller + try: + self.recipe_service.delete_many(delete_recipes.recipes) + except PermissionDenied as e: + self.logger.error("Permission Denied on recipe controller action") + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail=ErrorResponse.respond(message="Permission Denied") + ) from e @router.post("/export", status_code=202) def bulk_export_recipes(self, export_recipes: ExportRecipes): diff --git a/mealie/services/recipe/recipe_bulk_service.py b/mealie/services/recipe/recipe_bulk_service.py index 32601af25..58e824224 100644 --- a/mealie/services/recipe/recipe_bulk_service.py +++ b/mealie/services/recipe/recipe_bulk_service.py @@ -110,11 +110,3 @@ class RecipeBulkActionsService(BaseService): except Exception as e: self.logger.error(f"Failed to categorize recipe {slug}") self.logger.error(e) - - def delete_recipes(self, recipes: list[str]) -> None: - for slug in recipes: - try: - self.repos.recipes.delete(slug) - except Exception as e: - self.logger.error(f"Failed to delete recipe {slug}") - self.logger.error(e) diff --git a/mealie/services/recipe/recipe_service.py b/mealie/services/recipe/recipe_service.py index 340738adc..bf5e45c1b 100644 --- a/mealie/services/recipe/recipe_service.py +++ b/mealie/services/recipe/recipe_service.py @@ -4,10 +4,12 @@ import shutil from datetime import UTC, datetime from pathlib import Path from shutil import copytree, rmtree +from textwrap import dedent from typing import Any from uuid import UUID, uuid4 from zipfile import ZipFile +import sqlalchemy as sa from fastapi import UploadFile from mealie.core import exceptions @@ -64,31 +66,57 @@ class RecipeService(RecipeServiceBase): raise exceptions.NoEntryFound("Recipe not found.") return recipe - def can_delete(self, recipe: Recipe) -> bool: + def can_delete(self, recipe_slugs: list[str]) -> bool: if self.user.admin: return True else: - return self.can_update(recipe) + return self.can_update(recipe_slugs) - def can_update(self, recipe: Recipe) -> bool: - if recipe.settings is None: - raise exceptions.UnexpectedNone("Recipe Settings is None") + def can_update(self, recipe_slugs: list[str]) -> bool: + sql = dedent( + """ + SELECT + CASE + WHEN COUNT(*) = SUM( + CASE + -- User owns the recipe + WHEN r.user_id = :user_id THEN 1 - # Check if this user owns the recipe - if self.user.id == recipe.user_id: - return True + -- Not owner: check if recipe is locked + WHEN COALESCE(rs.locked, TRUE) = TRUE THEN 0 - # Check if this user has permission to edit this recipe - if self.household.id != recipe.household_id: - other_household = self.repos.households.get_one(recipe.household_id) - if not (other_household and other_household.preferences): - return False - if other_household.preferences.lock_recipe_edits_from_other_households: - return False - if recipe.settings.locked: - return False + -- Different household: check household policy + WHEN + u.household_id != :household_id + AND COALESCE(hp.lock_recipe_edits_from_other_households, TRUE) = TRUE + THEN 0 - return True + -- All other cases: can update + ELSE 1 + END + ) THEN 1 + ELSE 0 + END AS all_can_update + FROM recipes r + LEFT JOIN recipe_settings rs ON rs.recipe_id = r.id + LEFT JOIN users u ON u.id = r.user_id + LEFT JOIN households h ON h.id = u.household_id + LEFT JOIN household_preferences hp ON hp.household_id = h.id + WHERE r.slug IN :recipe_slugs AND r.group_id = :group_id; + """ + ) + + result = self.repos.session.execute( + sa.text(sql).bindparams(sa.bindparam("recipe_slugs", expanding=True)), + params={ + "user_id": self.repos.uuid_to_str(self.user.id), + "household_id": self.repos.uuid_to_str(self.household.id), + "group_id": self.repos.uuid_to_str(self.user.group_id), + "recipe_slugs": recipe_slugs, + }, + ).scalar() + + return bool(result) def can_lock_unlock(self, recipe: Recipe) -> bool: return recipe.user_id == self.user.id @@ -423,7 +451,7 @@ class RecipeService(RecipeServiceBase): if recipe is None or recipe.settings is None: raise exceptions.NoEntryFound("Recipe not found.") - if not self.can_update(recipe): + if not self.can_update([recipe.slug]): raise exceptions.PermissionDenied("You do not have permission to edit this recipe.") setting_lock = new_data.settings is not None and recipe.settings.locked != new_data.settings.locked @@ -444,7 +472,7 @@ class RecipeService(RecipeServiceBase): def update_recipe_image(self, slug: str, image: bytes, extension: str): recipe = self.get_one(slug) - if not self.can_update(recipe): + if not self.can_update([recipe.slug]): raise exceptions.PermissionDenied("You do not have permission to edit this recipe.") data_service = RecipeDataService(recipe.id) @@ -454,7 +482,7 @@ class RecipeService(RecipeServiceBase): def delete_recipe_image(self, slug: str) -> None: recipe = self.get_one(slug) - if not self.can_update(recipe): + if not self.can_update([recipe.slug]): raise exceptions.PermissionDenied("You do not have permission to edit this recipe.") data_service = RecipeDataService(recipe.id) @@ -482,12 +510,24 @@ class RecipeService(RecipeServiceBase): def delete_one(self, slug_or_id: str | UUID) -> Recipe: recipe = self.get_one(slug_or_id) + resp = self.delete_many([recipe.slug]) + return resp[0] - if not self.can_delete(recipe): - raise exceptions.PermissionDenied("You do not have permission to delete this recipe.") + def delete_many(self, recipe_slugs: list[str]) -> list[Recipe]: + if not self.can_delete(recipe_slugs): + if len(recipe_slugs) == 1: + msg = "You do not have permission to delete this recipe." + else: + msg = "You do not have permission to delete all of these recipes." + raise exceptions.PermissionDenied(msg) + + data = self.group_recipes.delete_many(recipe_slugs) + for r in data: + try: + self.delete_assets(r) + except Exception: + self.logger.exception(f"Failed to delete recipe assets for {r.slug}") - data = self.group_recipes.delete(recipe.id, "id") - self.delete_assets(data) return data # ================================================================= diff --git a/tests/integration_tests/user_recipe_tests/test_recipe_owner.py b/tests/integration_tests/user_recipe_tests/test_recipe_owner.py index 0a355dac6..86397d61d 100644 --- a/tests/integration_tests/user_recipe_tests/test_recipe_owner.py +++ b/tests/integration_tests/user_recipe_tests/test_recipe_owner.py @@ -160,6 +160,36 @@ def test_other_user_cant_delete_recipe(api_client: TestClient, user_tuple: list[ assert response.status_code == 403 +def test_other_user_bulk_delete(api_client: TestClient, user_tuple: list[TestUser]): + slug_locked = random_string(10) + slug_unlocked = random_string(10) + unique_user, other_user = user_tuple + + unique_user.repos.recipes.create( + Recipe( + user_id=unique_user.user_id, + group_id=unique_user.group_id, + name=slug_locked, + settings=RecipeSettings(locked=True), + ) + ) + unique_user.repos.recipes.create( + Recipe( + user_id=unique_user.user_id, + group_id=unique_user.group_id, + name=slug_unlocked, + settings=RecipeSettings(locked=False), + ) + ) + + response = api_client.post( + api_routes.recipes_bulk_actions_delete, + json={"recipes": [slug_locked, slug_unlocked]}, + headers=other_user.token, + ) + assert response.status_code == 403 + + def test_admin_can_delete_locked_recipe_owned_by_another_user( api_client: TestClient, unfiltered_database: AllRepositories, unique_user: TestUser, admin_user: TestUser ):