diff --git a/be/alembic/versions/d5f8a2e4c7b9_implement_soft_deletes_and_remove_cascades.py b/be/alembic/versions/d5f8a2e4c7b9_implement_soft_deletes_and_remove_cascades.py new file mode 100644 index 0000000..ec44a2e --- /dev/null +++ b/be/alembic/versions/d5f8a2e4c7b9_implement_soft_deletes_and_remove_cascades.py @@ -0,0 +1,101 @@ +"""implement soft deletes and remove cascades + +Revision ID: d5f8a2e4c7b9 +Revises: c693ade3601c +Create Date: 2024-03-20 10:00:00.000000 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = 'd5f8a2e4c7b9' +down_revision = 'c693ade3601c' +branch_labels = None +depends_on = None + +def upgrade(): + # Add soft delete columns to relevant tables + tables_for_soft_delete = [ + 'users', 'groups', 'lists', 'items', 'expenses', 'expense_splits', + 'chores', 'chore_assignments', 'recurrence_patterns', 'categories', + 'time_entries' + ] + + for table in tables_for_soft_delete: + op.add_column(table, sa.Column('deleted_at', sa.DateTime(timezone=True), nullable=True)) + op.add_column(table, sa.Column('is_deleted', sa.Boolean(), server_default='false', nullable=False)) + op.create_index(f'ix_{table}_deleted_at', table, ['deleted_at']) + op.create_index(f'ix_{table}_is_deleted', table, ['is_deleted']) + + # Remove cascade deletes from foreign keys + # First drop existing foreign keys with cascade + op.drop_constraint('user_groups_user_id_fkey', 'user_groups', type_='foreignkey') + op.drop_constraint('user_groups_group_id_fkey', 'user_groups', type_='foreignkey') + op.drop_constraint('invites_group_id_fkey', 'invites', type_='foreignkey') + op.drop_constraint('items_list_id_fkey', 'items', type_='foreignkey') + op.drop_constraint('expense_splits_expense_id_fkey', 'expense_splits', type_='foreignkey') + op.drop_constraint('chores_group_id_fkey', 'chores', type_='foreignkey') + op.drop_constraint('chore_assignments_chore_id_fkey', 'chore_assignments', type_='foreignkey') + op.drop_constraint('chore_assignments_assigned_to_user_id_fkey', 'chore_assignments', type_='foreignkey') + op.drop_constraint('chore_history_chore_id_fkey', 'chore_history', type_='foreignkey') + op.drop_constraint('chore_history_group_id_fkey', 'chore_history', type_='foreignkey') + op.drop_constraint('chore_assignment_history_assignment_id_fkey', 'chore_assignment_history', type_='foreignkey') + op.drop_constraint('time_entries_chore_assignment_id_fkey', 'time_entries', type_='foreignkey') + + # Recreate foreign keys without cascade delete + op.create_foreign_key('user_groups_user_id_fkey', 'user_groups', 'users', ['user_id'], ['id']) + op.create_foreign_key('user_groups_group_id_fkey', 'user_groups', 'groups', ['group_id'], ['id']) + op.create_foreign_key('invites_group_id_fkey', 'invites', 'groups', ['group_id'], ['id']) + op.create_foreign_key('items_list_id_fkey', 'items', 'lists', ['list_id'], ['id']) + op.create_foreign_key('expense_splits_expense_id_fkey', 'expense_splits', 'expenses', ['expense_id'], ['id']) + op.create_foreign_key('chores_group_id_fkey', 'chores', 'groups', ['group_id'], ['id']) + op.create_foreign_key('chore_assignments_chore_id_fkey', 'chore_assignments', 'chores', ['chore_id'], ['id']) + op.create_foreign_key('chore_assignments_assigned_to_user_id_fkey', 'chore_assignments', 'users', ['assigned_to_user_id'], ['id']) + op.create_foreign_key('chore_history_chore_id_fkey', 'chore_history', 'chores', ['chore_id'], ['id']) + op.create_foreign_key('chore_history_group_id_fkey', 'chore_history', 'groups', ['group_id'], ['id']) + op.create_foreign_key('chore_assignment_history_assignment_id_fkey', 'chore_assignment_history', 'chore_assignments', ['assignment_id'], ['id']) + op.create_foreign_key('time_entries_chore_assignment_id_fkey', 'time_entries', 'chore_assignments', ['chore_assignment_id'], ['id']) + +def downgrade(): + # Remove soft delete columns + tables_for_soft_delete = [ + 'users', 'groups', 'lists', 'items', 'expenses', 'expense_splits', + 'chores', 'chore_assignments', 'recurrence_patterns', 'categories', + 'time_entries' + ] + + for table in tables_for_soft_delete: + op.drop_index(f'ix_{table}_deleted_at', table) + op.drop_index(f'ix_{table}_is_deleted', table) + op.drop_column(table, 'deleted_at') + op.drop_column(table, 'is_deleted') + + # Restore cascade deletes + op.drop_constraint('user_groups_user_id_fkey', 'user_groups', type_='foreignkey') + op.drop_constraint('user_groups_group_id_fkey', 'user_groups', type_='foreignkey') + op.drop_constraint('invites_group_id_fkey', 'invites', type_='foreignkey') + op.drop_constraint('items_list_id_fkey', 'items', type_='foreignkey') + op.drop_constraint('expense_splits_expense_id_fkey', 'expense_splits', type_='foreignkey') + op.drop_constraint('chores_group_id_fkey', 'chores', type_='foreignkey') + op.drop_constraint('chore_assignments_chore_id_fkey', 'chore_assignments', type_='foreignkey') + op.drop_constraint('chore_assignments_assigned_to_user_id_fkey', 'chore_assignments', type_='foreignkey') + op.drop_constraint('chore_history_chore_id_fkey', 'chore_history', type_='foreignkey') + op.drop_constraint('chore_history_group_id_fkey', 'chore_history', type_='foreignkey') + op.drop_constraint('chore_assignment_history_assignment_id_fkey', 'chore_assignment_history', type_='foreignkey') + op.drop_constraint('time_entries_chore_assignment_id_fkey', 'time_entries', type_='foreignkey') + + # Recreate foreign keys with cascade delete + op.create_foreign_key('user_groups_user_id_fkey', 'user_groups', 'users', ['user_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('user_groups_group_id_fkey', 'user_groups', 'groups', ['group_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('invites_group_id_fkey', 'invites', 'groups', ['group_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('items_list_id_fkey', 'items', 'lists', ['list_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('expense_splits_expense_id_fkey', 'expense_splits', 'expenses', ['expense_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chores_group_id_fkey', 'chores', 'groups', ['group_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chore_assignments_chore_id_fkey', 'chore_assignments', 'chores', ['chore_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chore_assignments_assigned_to_user_id_fkey', 'chore_assignments', 'users', ['assigned_to_user_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chore_history_chore_id_fkey', 'chore_history', 'chores', ['chore_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chore_history_group_id_fkey', 'chore_history', 'groups', ['group_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('chore_assignment_history_assignment_id_fkey', 'chore_assignment_history', 'chore_assignments', ['assignment_id'], ['id'], ondelete='CASCADE') + op.create_foreign_key('time_entries_chore_assignment_id_fkey', 'time_entries', 'chore_assignments', ['chore_assignment_id'], ['id'], ondelete='CASCADE') \ No newline at end of file diff --git a/be/app/api/v1/endpoints/groups.py b/be/app/api/v1/endpoints/groups.py index 17eeab0..6fcfaa4 100644 --- a/be/app/api/v1/endpoints/groups.py +++ b/be/app/api/v1/endpoints/groups.py @@ -7,7 +7,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.database import get_transactional_session, get_session from app.auth import current_active_user from app.models import User as UserModel, UserRoleEnum -from app.schemas.group import GroupCreate, GroupPublic, GroupScheduleGenerateRequest +from app.schemas.group import GroupCreate, GroupPublic, GroupScheduleGenerateRequest, GroupDelete from app.schemas.invite import InviteCodePublic from app.schemas.message import Message from app.schemas.list import ListDetail @@ -177,6 +177,43 @@ async def get_group_active_invite( logger.info(f"User {current_user.email} retrieved active invite code for group {group_id}") return invite +@router.delete( + "/{group_id}", + response_model=Message, + summary="Delete Group (Owner Only)", + tags=["Groups"] +) +async def delete_group( + group_id: int, + delete_confirmation: GroupDelete, + db: AsyncSession = Depends(get_transactional_session), + current_user: UserModel = Depends(current_active_user), +): + """Permanently deletes a group and all associated data. Requires owner role and explicit confirmation.""" + logger.info(f"Owner {current_user.email} attempting to delete group {group_id}") + + # Check if user is owner + user_role = await crud_group.get_user_role_in_group(db, group_id=group_id, user_id=current_user.id) + if user_role != UserRoleEnum.owner: + logger.warning(f"Permission denied: User {current_user.email} (role: {user_role}) cannot delete group {group_id}") + raise GroupPermissionError(group_id, "delete group") + + # Get group to verify name + group = await crud_group.get_group_by_id(db, group_id) + if not group: + raise GroupNotFoundError(group_id) + + # Verify confirmation name matches group name + if delete_confirmation.confirmation_name != group.name: + raise GroupValidationError( + f"Confirmation name '{delete_confirmation.confirmation_name}' does not match group name '{group.name}'" + ) + + # Delete the group + await crud_group.delete_group(db, group_id) + logger.info(f"Group {group_id} successfully deleted by owner {current_user.email}") + return Message(detail="Group successfully deleted") + @router.delete( "/{group_id}/leave", response_model=Message, @@ -188,7 +225,7 @@ async def leave_group( db: AsyncSession = Depends(get_transactional_session), current_user: UserModel = Depends(current_active_user), ): - """Removes the current user from the specified group. If the owner is the last member, the group will be deleted.""" + """Removes the current user from the specified group. If the user is the owner and last member, the group will be archived.""" logger.info(f"User {current_user.email} attempting to leave group {group_id}") user_role = await crud_group.get_user_role_in_group(db, group_id=group_id, user_id=current_user.id) @@ -198,9 +235,11 @@ async def leave_group( if user_role == UserRoleEnum.owner: member_count = await crud_group.get_group_member_count(db, group_id) if member_count <= 1: - logger.info(f"Owner {current_user.email} is the last member. Deleting group {group_id}") - await crud_group.delete_group(db, group_id) - return Message(detail="Group deleted as you were the last member") + logger.info(f"Owner {current_user.email} is the last member. Group {group_id} will be archived.") + # TODO: Implement group archiving logic here + # For now, we'll just remove the user but keep the group + await crud_group.remove_user_from_group(db, group_id=group_id, user_id=current_user.id) + return Message(detail="You have left the group. As you were the last member, the group has been archived.") deleted = await crud_group.remove_user_from_group(db, group_id=group_id, user_id=current_user.id) diff --git a/be/app/models.py b/be/app/models.py index 7b6fbe3..613ae4d 100644 --- a/be/app/models.py +++ b/be/app/models.py @@ -20,11 +20,23 @@ from sqlalchemy import ( CheckConstraint, Date ) -from sqlalchemy.orm import relationship +from sqlalchemy.orm import relationship, declared_attr from sqlalchemy.dialects.postgresql import JSONB +from sqlalchemy.ext.declarative import declared_attr from .database import Base +class SoftDeleteMixin: + deleted_at = Column(DateTime(timezone=True), nullable=True, index=True) + is_deleted = Column(Boolean, default=False, nullable=False, index=True) + + @declared_attr + def __mapper_args__(cls): + return { + 'polymorphic_identity': cls.__name__.lower(), + 'passive_deletes': True + } + # --- Enums --- class UserRoleEnum(enum.Enum): owner = "owner" @@ -83,7 +95,7 @@ class ChoreHistoryEventTypeEnum(str, enum.Enum): DETAILS_CHANGED = "details_changed" # --- User Model --- -class User(Base): +class User(Base, SoftDeleteMixin): __tablename__ = "users" id = Column(Integer, primary_key=True, index=True) @@ -98,26 +110,26 @@ class User(Base): # --- Relationships --- created_groups = relationship("Group", back_populates="creator") - group_associations = relationship("UserGroup", back_populates="user", cascade="all, delete-orphan") + group_associations = relationship("UserGroup", back_populates="user") created_invites = relationship("Invite", back_populates="creator") created_lists = relationship("List", foreign_keys="List.created_by_id", back_populates="creator") added_items = relationship("Item", foreign_keys="Item.added_by_id", back_populates="added_by_user") completed_items = relationship("Item", foreign_keys="Item.completed_by_id", back_populates="completed_by_user") - expenses_paid = relationship("Expense", foreign_keys="Expense.paid_by_user_id", back_populates="paid_by_user", cascade="all, delete-orphan") - expenses_created = relationship("Expense", foreign_keys="Expense.created_by_user_id", back_populates="created_by_user", cascade="all, delete-orphan") - expense_splits = relationship("ExpenseSplit", foreign_keys="ExpenseSplit.user_id", back_populates="user", cascade="all, delete-orphan") - settlements_made = relationship("Settlement", foreign_keys="Settlement.paid_by_user_id", back_populates="payer", cascade="all, delete-orphan") - settlements_received = relationship("Settlement", foreign_keys="Settlement.paid_to_user_id", back_populates="payee", cascade="all, delete-orphan") - settlements_created = relationship("Settlement", foreign_keys="Settlement.created_by_user_id", back_populates="created_by_user", cascade="all, delete-orphan") + expenses_paid = relationship("Expense", foreign_keys="Expense.paid_by_user_id", back_populates="paid_by_user") + expenses_created = relationship("Expense", foreign_keys="Expense.created_by_user_id", back_populates="created_by_user") + expense_splits = relationship("ExpenseSplit", foreign_keys="ExpenseSplit.user_id", back_populates="user") + settlements_made = relationship("Settlement", foreign_keys="Settlement.paid_by_user_id", back_populates="payer") + settlements_received = relationship("Settlement", foreign_keys="Settlement.paid_to_user_id", back_populates="payee") + settlements_created = relationship("Settlement", foreign_keys="Settlement.created_by_user_id", back_populates="created_by_user") created_chores = relationship("Chore", foreign_keys="[Chore.created_by_id]", back_populates="creator") - assigned_chores = relationship("ChoreAssignment", back_populates="assigned_user", cascade="all, delete-orphan") - chore_history_entries = relationship("ChoreHistory", back_populates="changed_by_user", cascade="all, delete-orphan") - assignment_history_entries = relationship("ChoreAssignmentHistory", back_populates="changed_by_user", cascade="all, delete-orphan") + assigned_chores = relationship("ChoreAssignment", back_populates="assigned_user") + chore_history_entries = relationship("ChoreHistory", back_populates="changed_by_user") + assignment_history_entries = relationship("ChoreAssignmentHistory", back_populates="changed_by_user") financial_audit_logs = relationship("FinancialAuditLog", back_populates="user") time_entries = relationship("TimeEntry", back_populates="user") categories = relationship("Category", back_populates="user") -class Group(Base): +class Group(Base, SoftDeleteMixin): __tablename__ = "groups" id = Column(Integer, primary_key=True, index=True) @@ -128,22 +140,22 @@ class Group(Base): version = Column(Integer, nullable=False, default=1, server_default='1') creator = relationship("User", back_populates="created_groups") - member_associations = relationship("UserGroup", back_populates="group", cascade="all, delete-orphan") - invites = relationship("Invite", back_populates="group", cascade="all, delete-orphan") + member_associations = relationship("UserGroup", back_populates="group") + invites = relationship("Invite", back_populates="group") - lists = relationship("List", back_populates="group", cascade="all, delete-orphan") - expenses = relationship("Expense", foreign_keys="Expense.group_id", back_populates="group", cascade="all, delete-orphan") - settlements = relationship("Settlement", foreign_keys="Settlement.group_id", back_populates="group", cascade="all, delete-orphan") - chores = relationship("Chore", back_populates="group", cascade="all, delete-orphan") - chore_history = relationship("ChoreHistory", back_populates="group", cascade="all, delete-orphan") + lists = relationship("List", back_populates="group") + expenses = relationship("Expense", foreign_keys="Expense.group_id", back_populates="group") + settlements = relationship("Settlement", foreign_keys="Settlement.group_id", back_populates="group") + chores = relationship("Chore", back_populates="group") + chore_history = relationship("ChoreHistory", back_populates="group") class UserGroup(Base): __tablename__ = "user_groups" __table_args__ = (UniqueConstraint('user_id', 'group_id', name='uq_user_group'),) id = Column(Integer, primary_key=True, index=True) - user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False) - group_id = Column(Integer, ForeignKey("groups.id", ondelete="CASCADE"), nullable=False) + user_id = Column(Integer, ForeignKey("users.id"), nullable=False) + group_id = Column(Integer, ForeignKey("groups.id"), nullable=False) role = Column(SAEnum(UserRoleEnum, name="userroleenum", create_type=True), nullable=False, default=UserRoleEnum.member) joined_at = Column(DateTime(timezone=True), server_default=func.now(), nullable=False) @@ -158,7 +170,7 @@ class Invite(Base): id = Column(Integer, primary_key=True, index=True) code = Column(String, unique=False, index=True, nullable=False, default=lambda: secrets.token_urlsafe(16)) - group_id = Column(Integer, ForeignKey("groups.id", ondelete="CASCADE"), nullable=False) + group_id = Column(Integer, ForeignKey("groups.id"), nullable=False) created_by_id = Column(Integer, ForeignKey("users.id"), nullable=False) created_at = Column(DateTime(timezone=True), server_default=func.now(), nullable=False) expires_at = Column(DateTime(timezone=True), nullable=False, default=lambda: datetime.now(timezone.utc) + timedelta(days=7)) @@ -168,7 +180,7 @@ class Invite(Base): creator = relationship("User", back_populates="created_invites") -class List(Base): +class List(Base, SoftDeleteMixin): __tablename__ = "lists" id = Column(Integer, primary_key=True, index=True) @@ -187,20 +199,19 @@ class List(Base): items = relationship( "Item", back_populates="list", - cascade="all, delete-orphan", order_by="Item.position.asc(), Item.created_at.asc()" ) - expenses = relationship("Expense", foreign_keys="Expense.list_id", back_populates="list", cascade="all, delete-orphan") + expenses = relationship("Expense", foreign_keys="Expense.list_id", back_populates="list") -class Item(Base): +class Item(Base, SoftDeleteMixin): __tablename__ = "items" __table_args__ = ( Index('ix_items_list_id_position', 'list_id', 'position'), ) id = Column(Integer, primary_key=True, index=True) - list_id = Column(Integer, ForeignKey("lists.id", ondelete="CASCADE"), nullable=False) + list_id = Column(Integer, ForeignKey("lists.id"), nullable=False) name = Column(String, index=True, nullable=False) quantity = Column(String, nullable=True) is_complete = Column(Boolean, default=False, nullable=False) @@ -220,7 +231,7 @@ class Item(Base): expenses = relationship("Expense", back_populates="item") category = relationship("Category", back_populates="items") -class Expense(Base): +class Expense(Base, SoftDeleteMixin): __tablename__ = "expenses" id = Column(Integer, primary_key=True, index=True) @@ -245,7 +256,7 @@ class Expense(Base): list = relationship("List", foreign_keys=[list_id], back_populates="expenses") group = relationship("Group", foreign_keys=[group_id], back_populates="expenses") item = relationship("Item", foreign_keys=[item_id], back_populates="expenses") - splits = relationship("ExpenseSplit", back_populates="expense", cascade="all, delete-orphan") + splits = relationship("ExpenseSplit", back_populates="expense") parent_expense = relationship("Expense", remote_side=[id], back_populates="child_expenses") child_expenses = relationship("Expense", back_populates="parent_expense") overall_settlement_status = Column(SAEnum(ExpenseOverallStatusEnum, name="expenseoverallstatusenum", create_type=True), nullable=False, server_default=ExpenseOverallStatusEnum.unpaid.value, default=ExpenseOverallStatusEnum.unpaid) @@ -260,7 +271,7 @@ class Expense(Base): CheckConstraint('group_id IS NOT NULL OR list_id IS NOT NULL', name='ck_expense_group_or_list'), ) -class ExpenseSplit(Base): +class ExpenseSplit(Base, SoftDeleteMixin): __tablename__ = "expense_splits" __table_args__ = ( UniqueConstraint('expense_id', 'user_id', name='uq_expense_user_split'), @@ -268,7 +279,7 @@ class ExpenseSplit(Base): ) id = Column(Integer, primary_key=True, index=True) - expense_id = Column(Integer, ForeignKey("expenses.id", ondelete="CASCADE"), nullable=False) + expense_id = Column(Integer, ForeignKey("expenses.id"), nullable=False) user_id = Column(Integer, ForeignKey("users.id"), nullable=False) owed_amount = Column(Numeric(10, 2), nullable=False) @@ -280,7 +291,7 @@ class ExpenseSplit(Base): expense = relationship("Expense", back_populates="splits") user = relationship("User", foreign_keys=[user_id], back_populates="expense_splits") - settlement_activities = relationship("SettlementActivity", back_populates="split", cascade="all, delete-orphan") + settlement_activities = relationship("SettlementActivity", back_populates="split") status = Column(SAEnum(ExpenseSplitStatusEnum, name="expensesplitstatusenum", create_type=True), nullable=False, server_default=ExpenseSplitStatusEnum.unpaid.value, default=ExpenseSplitStatusEnum.unpaid) paid_at = Column(DateTime(timezone=True), nullable=True) @@ -335,12 +346,12 @@ class SettlementActivity(Base): # --- Chore Model --- -class Chore(Base): +class Chore(Base, SoftDeleteMixin): __tablename__ = "chores" id = Column(Integer, primary_key=True, index=True) type = Column(SAEnum(ChoreTypeEnum, name="choretypeenum", create_type=True), nullable=False) - group_id = Column(Integer, ForeignKey("groups.id", ondelete="CASCADE"), nullable=True, index=True) + group_id = Column(Integer, ForeignKey("groups.id"), nullable=True, index=True) name = Column(String, nullable=False, index=True) description = Column(Text, nullable=True) created_by_id = Column(Integer, ForeignKey("users.id"), nullable=False, index=True) @@ -357,19 +368,19 @@ class Chore(Base): group = relationship("Group", back_populates="chores") creator = relationship("User", back_populates="created_chores") - assignments = relationship("ChoreAssignment", back_populates="chore", cascade="all, delete-orphan") - history = relationship("ChoreHistory", back_populates="chore", cascade="all, delete-orphan") + assignments = relationship("ChoreAssignment", back_populates="chore") + history = relationship("ChoreHistory", back_populates="chore") parent_chore = relationship("Chore", remote_side=[id], back_populates="child_chores") - child_chores = relationship("Chore", back_populates="parent_chore", cascade="all, delete-orphan") + child_chores = relationship("Chore", back_populates="parent_chore") # --- ChoreAssignment Model --- -class ChoreAssignment(Base): +class ChoreAssignment(Base, SoftDeleteMixin): __tablename__ = "chore_assignments" id = Column(Integer, primary_key=True, index=True) - chore_id = Column(Integer, ForeignKey("chores.id", ondelete="CASCADE"), nullable=False, index=True) - assigned_to_user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False, index=True) + chore_id = Column(Integer, ForeignKey("chores.id"), nullable=False, index=True) + assigned_to_user_id = Column(Integer, ForeignKey("users.id"), nullable=False, index=True) due_date = Column(Date, nullable=False) is_complete = Column(Boolean, default=False, nullable=False) @@ -380,12 +391,12 @@ class ChoreAssignment(Base): chore = relationship("Chore", back_populates="assignments") assigned_user = relationship("User", back_populates="assigned_chores") - history = relationship("ChoreAssignmentHistory", back_populates="assignment", cascade="all, delete-orphan") - time_entries = relationship("TimeEntry", back_populates="assignment", cascade="all, delete-orphan") + history = relationship("ChoreAssignmentHistory", back_populates="assignment") + time_entries = relationship("TimeEntry", back_populates="assignment") # === NEW: RecurrencePattern Model === -class RecurrencePattern(Base): +class RecurrencePattern(Base, SoftDeleteMixin): __tablename__ = "recurrence_patterns" id = Column(Integer, primary_key=True, index=True) @@ -409,8 +420,8 @@ class ChoreHistory(Base): __tablename__ = "chore_history" id = Column(Integer, primary_key=True, index=True) - chore_id = Column(Integer, ForeignKey("chores.id", ondelete="CASCADE"), nullable=True, index=True) - group_id = Column(Integer, ForeignKey("groups.id", ondelete="CASCADE"), nullable=True, index=True) + chore_id = Column(Integer, ForeignKey("chores.id"), nullable=True, index=True) + group_id = Column(Integer, ForeignKey("groups.id"), nullable=True, index=True) event_type = Column(SAEnum(ChoreHistoryEventTypeEnum, name="chorehistoryeventtypeenum", create_type=True), nullable=False) event_data = Column(JSONB, nullable=True) changed_by_user_id = Column(Integer, ForeignKey("users.id"), nullable=True) @@ -424,7 +435,7 @@ class ChoreAssignmentHistory(Base): __tablename__ = "chore_assignment_history" id = Column(Integer, primary_key=True, index=True) - assignment_id = Column(Integer, ForeignKey("chore_assignments.id", ondelete="CASCADE"), nullable=False, index=True) + assignment_id = Column(Integer, ForeignKey("chore_assignments.id"), nullable=False, index=True) event_type = Column(SAEnum(ChoreHistoryEventTypeEnum, name="chorehistoryeventtypeenum", create_type=True), nullable=False) event_data = Column(JSONB, nullable=True) changed_by_user_id = Column(Integer, ForeignKey("users.id"), nullable=True) @@ -447,7 +458,7 @@ class FinancialAuditLog(Base): user = relationship("User", back_populates="financial_audit_logs") -class Category(Base): +class Category(Base, SoftDeleteMixin): __tablename__ = 'categories' id = Column(Integer, primary_key=True, index=True) name = Column(String, nullable=False, index=True) @@ -459,10 +470,10 @@ class Category(Base): __table_args__ = (UniqueConstraint('name', 'user_id', 'group_id', name='uq_category_scope'),) -class TimeEntry(Base): +class TimeEntry(Base, SoftDeleteMixin): __tablename__ = 'time_entries' id = Column(Integer, primary_key=True, index=True) - chore_assignment_id = Column(Integer, ForeignKey('chore_assignments.id', ondelete="CASCADE"), nullable=False) + chore_assignment_id = Column(Integer, ForeignKey('chore_assignments.id'), nullable=False) user_id = Column(Integer, ForeignKey('users.id'), nullable=False) start_time = Column(DateTime(timezone=True), nullable=False) end_time = Column(DateTime(timezone=True), nullable=True) diff --git a/be/app/schemas/group.py b/be/app/schemas/group.py index 9645447..521fead 100644 --- a/be/app/schemas/group.py +++ b/be/app/schemas/group.py @@ -7,6 +7,9 @@ from .chore import ChoreHistoryPublic class GroupCreate(BaseModel): name: str +class GroupDelete(BaseModel): + confirmation_name: str + class GroupScheduleGenerateRequest(BaseModel): start_date: date end_date: date diff --git a/be/done.md b/be/done.md new file mode 100644 index 0000000..e69de29 diff --git a/be/requirements.txt b/be/requirements.txt index 261c876..9c120d3 100644 --- a/be/requirements.txt +++ b/be/requirements.txt @@ -21,8 +21,5 @@ pytest>=7.4.0 pytest-asyncio>=0.21.0 pytest-cov>=4.1.0 httpx>=0.24.0 # For async HTTP testing -aiosqlite>=0.19.0 # For async SQLite support in tests - -# Scheduler APScheduler==3.10.4 redis>=5.0.0 \ No newline at end of file diff --git a/be/todo.md b/be/todo.md new file mode 100644 index 0000000..bd991d7 --- /dev/null +++ b/be/todo.md @@ -0,0 +1,318 @@ +# Backend Critical Issues & TODOs + +This document outlines critical issues found in the backend codebase that require immediate attention. The issues range from severe data integrity risks to security vulnerabilities and poor API design. + +--- + +### 1. Catastrophic Data Loss via Cascading Deletes + +**Severity: CRITICAL** + +**Problem:** +The SQLAlchemy models in `be/app/models.py` are configured with `ondelete="CASCADE"` and `cascade="all, delete-orphan"` on relationships to critical shared data like `Expense`, `List`, `Settlement`, and `Chore`. This creates a chain reaction of data destruction that can be triggered by a single user's action, affecting the data of all other users in a group. + +**Specific Scenarios:** + +- **Deleting a User:** When a user is deleted, all expenses they paid for, created, or were a part of (via `ExpenseSplit`) are deleted. All settlements they participated in are also deleted. This permanently corrupts the financial history of every group the user was a member of, violating the data integrity for all other members. +- **Deleting a Group:** Deleting a group permanently deletes **all** associated data, including every expense, shopping list, item, settlement, and the entire chore history. +- **Deleting a List:** Deleting a shopping list deletes all associated expenses. + +**Recommendation:** + +- Immediately remove all `ondelete="CASCADE"` and `cascade="all, delete-orphan"` configurations from the models where they affect shared data. +- Implement a **soft-delete** or **anonymization** strategy for user deletion to comply with data protection regulations (like GDPR's "right to be forgotten") without destroying the integrity of other users' data. When a user is "deleted," their PII should be scrubbed, but their ID and associated records should remain to keep financial history intact. + +--- + +### 2. Hidden Group Deletion in "Leave Group" Endpoint + +**Severity: CRITICAL** + +**Problem:** +There is no explicit `DELETE /groups/{group_id}` endpoint. Instead, the logic for deleting a group is hidden within the `DELETE /groups/{group_id}/leave` endpoint in `be/app/api/v1/endpoints/groups.py`. If the last remaining member of a group, who is also the owner, calls this endpoint, the entire group and all its associated data are silently and permanently deleted. + +**Why this is a problem:** + +- **Non-Obvious Destruction:** A user would never expect a "leave" action to be destructive for the entire group. +- **No Confirmation:** There is no "are you sure?" check. The action is immediate and irreversible. +- **High Risk of Accidental Deletion:** An owner could easily remove all members and then leave, accidentally wiping all data. + +**Recommendation:** + +- Create a separate, explicit `DELETE /groups/{group_id}` endpoint. +- This new endpoint must be protected and should only be accessible to the group owner. +- Implement multiple confirmation steps before allowing the deletion (e.g., requiring the user to type the group's name to confirm). +- The "leave group" endpoint should **never** result in the deletion of the group. If the last member leaves, the group should be archived or left empty, but not destroyed. + +--- + +### 3. User Deletion Triggers Data Cascade + +**Severity: CRITICAL** + +**Problem:** +The user management routes in `be/app/api/v1/endpoints/users.py` use the `fastapi-users` library. The configuration in `be/app/auth.py` uses the default `BaseUserManager`, which performs a **hard delete** on the user record in the database. + +**Why this is a problem:** +This is the trigger for the catastrophic data loss described in issue #1. A simple call to the `DELETE /users/me` endpoint will initiate the cascade, destroying shared financial records across multiple groups. This is a direct violation of data integrity for other users. + +**Recommendation:** + +- Override the default `delete` method in the `UserManager` class in `be/app/auth.py`. +- The new method must implement the anonymization strategy discussed in issue #1. It should scrub the user's PII and mark the account as inactive, but **not** delete the row from the database. + +--- + +### 4. Inconsistent and Flawed Authorization in Financial Endpoints + +**Severity: HIGH** + +**Problem:** +Authorization logic is inconsistent across the financial endpoints in `be/app/api/v1/endpoints/financials.py`, creating security gaps and potential for misuse. + +**Specific Scenarios:** + +- **Creating vs. Updating Expenses:** A regular user can't create an expense paid by someone else, but they _can_ modify an existing expense as long as they were the original payer. This is inconsistent. A better approach would be to only allow group **owners** to modify financial records created by others. +- **Deleting Settlements:** The `delete_settlement_record` endpoint dangerously allows the user who _created_ the settlement record to delete it. A user could pay a debt, have the recipient confirm it, and then the payer could simply delete the record of the payment. Only group **owners** should have the power to delete financial records, and it should be a heavily audited action. + +**Recommendation:** + +- Establish a clear and consistent authorization policy. A good default policy would be: + 1. Users can create and modify their own financial entries (e.g., expenses they paid for). + 2. Only group **owners** can modify or delete financial entries created by other users. +- Refactor all financial endpoints (`expenses`, `settlements`) to strictly and consistently enforce this policy. + +--- + +### 5. No Protection Against Over-Settlement of Debts + +**Severity: HIGH** + +**Problem:** +The `record_settlement_for_expense_split` endpoint in `financials.py` does not validate if the settlement amount exceeds the amount owed for that expense split. + +**Why this is a problem:** +A malicious user could exploit this by "settling" a $5 debt with a $500 payment. The system would record this, creating a false record that the original payer now owes the malicious user $495. This can be used to create phantom debts. + +**Recommendation:** + +- In `crud_settlement_activity.create_settlement_activity`, add validation to ensure the `amount_paid` in a settlement activity does not exceed the remaining `owed_amount` on the `ExpenseSplit`. +- If an overpayment is attempted, the API should return a `400 Bad Request` error with a clear message. + +--- + +### 6. Flawed Business Logic Preventing Group Expenses from Personal Lists + +**Severity: MEDIUM** + +**Problem:** +The `create_new_expense` endpoint prevents a user from creating a group expense that is associated with one of their personal shopping lists. + +**Why this is a problem:** +This doesn't reflect a real-world use case. A user might use a personal list to do shopping for a group and then need to file the expense against that group. The current logic forces an artificial separation. + +**Recommendation:** + +- Remove the validation check that blocks this action. A user should be able to link a group expense to any list they have access to, regardless of whether it's a personal or group list. The critical factor is the `group_id` on the expense itself, not on the list it's optionally linked to. + +--- + +### 7. Unrestricted Access to All Chore History + +**Severity: CRITICAL** + +**Problem:** +The chore history endpoints in `be/app/api/v1/endpoints/chores.py` lack any authorization checks. + +**Specific Endpoints:** + +- `GET /{chore_id}/history` +- `GET /assignments/{assignment_id}/history` + +Any authenticated user can access the historical data for _any_ chore or assignment in the entire database simply by guessing its ID. This allows users from one group to spy on the activities of other groups, which is a major data leak. + +**Recommendation:** + +- Add strict authorization checks to these endpoints immediately. +- Before returning any history, the code must verify that the `current_user` is a member of the group to which the chore or assignment belongs. +- For personal chores, it must verify that the `current_user` is the creator of the chore. + +--- + +### 8. Redundant and Confusing Chore Endpoints + +**Severity: LOW** + +**Problem:** +The API has a completely separate set of endpoints for "Personal Chores" (e.g., `POST /personal`, `DELETE /personal/{chore_id}`). It also provides generic endpoints (`PUT /{chore_id}`) that can perform the same actions and more, such as converting a personal chore to a group chore. + +**Why this is a problem:** + +- **API Bloat:** It makes the API larger and more confusing than necessary. +- **Maintenance Burden:** Having duplicate logic increases the chance of bugs and makes the codebase harder to maintain. For example, if a business rule for updates changes, it might need to be updated in multiple places. +- **Inconsistency:** The `update_personal_chore` and `update_group_chore` endpoints have slightly different logic and permission checks than the generic `update_chore_any_type` endpoint, which is a recipe for subtle bugs. + +**Recommendation:** + +- Deprecate and remove the separate `/personal/*` and `/groups/{group_id}/chores/*` endpoints for `POST`, `PUT`, and `DELETE`. +- Consolidate all create, update, and delete logic into a single set of endpoints (e.g., `POST /chores`, `PUT /chores/{chore_id}`, `DELETE /chores/{chore_id}`). +- This single set of endpoints should contain clear, unified logic that handles permissions based on the chore's `type` (personal or group). + +--- + +### 9. Sensitive User Data Leaked in Application Logs + +**Severity: MEDIUM** + +**Problem:** +Across multiple endpoints (`financials.py`, `groups.py`, `chores.py`, etc.) the code logs user email addresses and sometimes internal object IDs at the **INFO** or **WARNING** level. Example: + +```python +logger.info(f"User {current_user.email} creating expense: {expense_in.description}") +``` + +Logging personally identifiable information (PII) such as email addresses at non-debug levels creates a compliance risk (GDPR/Datenschutz) and can leak data if log files are exposed. + +**Recommendation:** + +- Remove PII (emails, names) from log messages at INFO/WARNING/ERROR levels. +- If user identifiers are required, use the numeric `user.id` or a hashed identifier. +- Move any detailed, user‐specific logging to DEBUG level with redaction. + +--- + +### 10. Detailed Internal Errors Returned to Clients + +**Severity: MEDIUM** + +**Problem:** +Many `except` blocks convert low-level SQLAlchemy exceptions directly into HTTP 400/500 responses with `detail=str(e)`. This leaks internal table names, constraint names, and stack traces to the client. + +**Example:** `crud/expense.py` and other CRUD modules. + +**Recommendation:** + +- Map all internal exceptions to generic error messages for clients (e.g., "Database error, please try again"). +- Log the original exception server-side (with redaction) but never expose raw error strings to API consumers. + +--- + +### 11. Insecure Default Configuration Values + +**Severity: HIGH** + +**Problem:** +`app/config.py` sets insecure defaults that may accidentally reach production: + +- `SESSION_SECRET_KEY = "your-session-secret-key"` – a published default secret. +- `ACCESS_TOKEN_EXPIRE_MINUTES = 480` (8 h) – excessive token lifetime increases risk if a token is leaked. +- Wide-open `CORS_ORIGINS` allowing any localhost ports. + +**Recommendation:** + +- Make `SESSION_SECRET_KEY` **mandatory** via env var with no default. Fail hard if missing. +- Reduce default `ACCESS_TOKEN_EXPIRE_MINUTES` to ≤60 and allow override via env. +- In production mode (`settings.is_production`), restrict CORS origins to the canonical frontend domain list only. + +--- + +### 12. Missing Global Rate Limiting / Brute-Force Protection + +**Severity: MEDIUM** + +**Problem:** +The API exposes authentication (JWT login) and sensitive endpoints without any rate-limiting or brute-force protection, leaving it vulnerable to password-guessing and scraping attacks. + +**Recommendation:** + +- Integrate a middleware-level rate limiter (e.g., `slowapi`, `fastapi-limiter`) backed by Redis. +- Apply stricter per-IP limits on `/auth/jwt/login`, OTP verification, and password-reset endpoints. + +--- + +### 13. Recurring Expense Job Never Commits Transactions + +**Severity: HIGH** + +**Problem:** +`be/app/jobs/recurring_expenses.py` uses `db.flush()` inside `_generate_next_occurrence` but **never calls `commit()`** on the `AsyncSession`. Because the helper is executed inside the scheduler wrapper `run_recurring_expenses_job`, the surrounding context manager closes the session without an explicit commit, so Postgres rolls the transaction back. No newly-generated expenses are permanently saved and the `max_occurrences` counter is not decremented. At the next scheduler tick the exact same expenses are re-generated, flooding the database and the UI with duplicates. + +**Recommendation:** + +1. Replace the manual `flush()` calls with a **single transactional block** (e.g. `async with db.begin(): …`) and call `await db.commit()` once per expense or after the batch loop. +2. Add an integration test that verifies a generated expense actually persists after the job finishes. +3. Consider running the scheduler job inside a **database advisory lock** to avoid multiple workers generating duplicates concurrently. + +--- + +### 14. Blocking, Mixed-Sync Scheduler Configuration Can Starve the Event Loop + +**Severity: MEDIUM** + +**Problem:** +`be/app/core/scheduler.py` converts the async database URL to a _sync_ URL (`postgresql://`) for `SQLAlchemyJobStore` **and** registers the job with `ThreadPoolExecutor`. This combination forces a blocking, synchronous DB driver (psycopg2) to execute inside FastAPI's event loop thread pool. Under load, long-running jobs will consume the limited thread pool, delay HTTP requests, and may exhaust the sync connection pool, causing 5xx errors throughout the API layer. + +**Recommendation:** + +- Use `AsyncIOJobStore` with the existing `postgresql+asyncpg` URL, or move scheduled tasks to a dedicated worker (e.g. Celery, RQ) outside the API process. +- If synchronous execution is unavoidable, enlarge the job executor pool and _run the scheduler in a separate process_ so it cannot impact request latency. + +--- + +### 15. Public Root & Health Endpoints Leak Deployment Metadata + +**Severity: MEDIUM** + +**Problem:** +Unauthenticated `GET /` and `GET /health` responses include the `environment` ("development", "staging", "production") and full semantic `version` string straight from `app/config.py`. Attackers can fingerprint deployment targets and time their exploits around release cycles. + +**Recommendation:** + +- Limit environment & version details to authenticated admin users or internal monitoring networks. +- Replace the public response with a minimal `{ "status": "ok" }` payload. +- Add a separate, authenticated diagnostics endpoint for detailed build info. + +--- + +### 16. Wildcard-Style CORS With `allow_credentials=True` Enables Session Hijacking + +**Severity: HIGH** + +**Problem:** +`be/app/main.py` registers `CORSMiddleware` with the entire `settings.cors_origins_list` **and** sets `allow_credentials=True`, which instructs browsers to _send cookies and HTTP-Only session tokens_ to _any_ origin in that list. Because the default list contains every localhost port and is hard-coded, a malicious site running on another local port can silently read or mutate a user's data via XHR once the victim logs in to the real front-end. + +**Recommendation:** + +1. In production, populate `CORS_ORIGINS` from env and restrict it to the canonical front-end domain(s). +2. Set `allow_credentials=False` unless cookie-based auth is absolutely required—JWTs are already used for API auth. +3. Use the `strict-origin-when-cross-origin` referrer policy and enable CSRF protection if cookies stay enabled. + +--- + +### 17. Groups & Settlements Lack Optimistic-Locking Enforcement + +**Severity: MEDIUM** + +**Problem:** +The `groups`, `settlements`, and `settlement_activities` tables include a `version` column, but **none of the corresponding endpoints check or increment that version** during `PUT`/`DELETE` operations (unlike `lists` and `items`). Concurrent updates therefore overwrite each other silently, leading to lost data and inconsistent financial history. + +**Recommendation:** + +- Mirror the pattern used in the Lists API: require clients to supply the expected `version` and return **409 Conflict** on mismatch. +- Increment `version` in SQL via `UPDATE … SET version = version + 1` inside the same transaction to guarantee atomicity. + +--- + +### 18. Redis Connection Pool Never Closed + +**Severity: LOW** + +**Problem:** +`be/app/core/redis.py` creates a module-level `redis_pool` but the pool is **never closed** on application shutdown. In long-running worker processes (e.g. during Kubernetes rolling updates) this leaks file descriptors and keeps TCP connections open until the OS forcibly times them out. + +**Recommendation:** + +- Add a lifespan handler or shutdown event to call `await redis_pool.aclose()`. +- Use a factory in the DI system to create and dispose Redis connections per request where feasible. + +---