diff --git a/migrations/align_ownership_cascades.py b/migrations/align_ownership_cascades.py new file mode 100644 index 0000000..267f839 --- /dev/null +++ b/migrations/align_ownership_cascades.py @@ -0,0 +1,237 @@ +""" +Align ownership-style foreign keys with database-level delete cascades. + +Required when upgrading from <=1.0.1 to >1.0.1. if AccountEmail, +UserAvatar, or OrganizationResource tables were created without ON DELETE +CASCADE. SQLModel create_all() applies this for new databases but does not +alter existing constraints. + +Usage: + uv run python -m migrations.align_ownership_cascades .env + uv run python -m migrations.align_ownership_cascades .env --apply +""" + +from __future__ import annotations + +import argparse +from dataclasses import dataclass + +from dotenv import load_dotenv +from sqlalchemy import text +from sqlmodel import Session, create_engine + +from utils.core.db import get_connection_url + + +@dataclass(frozen=True) +class ForeignKeyTarget: + source_schema: str + source_table: str + source_column: str + target_schema: str + target_table: str + target_column: str + constraint_name: str + + @property + def label(self) -> str: + return f"{self.source_schema}.{self.source_table}.{self.source_column}" + + +@dataclass +class MigrationStats: + already_cascading: tuple[str, ...] = () + updated: tuple[str, ...] = () + skipped_missing_tables: tuple[str, ...] = () + + +TARGETS = ( + ForeignKeyTarget( + source_schema="private", + source_table="accountemail", + source_column="account_id", + target_schema="private", + target_table="account", + target_column="id", + constraint_name="fk_accountemail_account_id_account", + ), + ForeignKeyTarget( + source_schema="public", + source_table="useravatar", + source_column="user_id", + target_schema="public", + target_table="user", + target_column="id", + constraint_name="fk_useravatar_user_id_user", + ), + ForeignKeyTarget( + source_schema="public", + source_table="organizationresource", + source_column="organization_id", + target_schema="public", + target_table="organization", + target_column="id", + constraint_name="fk_organizationresource_organization_id_organization", + ), +) + + +def _quote_identifier(identifier: str) -> str: + return '"' + identifier.replace('"', '""') + '"' + + +def _qualified_table(schema: str, table: str) -> str: + return f"{_quote_identifier(schema)}.{_quote_identifier(table)}" + + +def _table_exists(session: Session, schema: str, table: str) -> bool: + result = session.connection().execute( + text( + """ + SELECT 1 + FROM information_schema.tables + WHERE table_schema = :schema + AND table_name = :table + """ + ), + {"schema": schema, "table": table}, + ) + return result.first() is not None + + +def _matching_foreign_keys( + session: Session, target: ForeignKeyTarget +) -> list[tuple[str, str]]: + result = session.connection().execute( + text( + """ + SELECT tc.constraint_name, rc.delete_rule + FROM information_schema.table_constraints AS tc + JOIN information_schema.key_column_usage AS kcu + ON kcu.constraint_schema = tc.constraint_schema + AND kcu.constraint_name = tc.constraint_name + JOIN information_schema.constraint_column_usage AS ccu + ON ccu.constraint_schema = tc.constraint_schema + AND ccu.constraint_name = tc.constraint_name + JOIN information_schema.referential_constraints AS rc + ON rc.constraint_schema = tc.constraint_schema + AND rc.constraint_name = tc.constraint_name + WHERE tc.constraint_type = 'FOREIGN KEY' + AND tc.table_schema = :source_schema + AND tc.table_name = :source_table + AND kcu.column_name = :source_column + AND ccu.table_schema = :target_schema + AND ccu.table_name = :target_table + AND ccu.column_name = :target_column + ORDER BY tc.constraint_name + """ + ), + { + "source_schema": target.source_schema, + "source_table": target.source_table, + "source_column": target.source_column, + "target_schema": target.target_schema, + "target_table": target.target_table, + "target_column": target.target_column, + }, + ) + return [(row.constraint_name, row.delete_rule) for row in result] + + +def _replace_foreign_key_with_cascade( + session: Session, target: ForeignKeyTarget, existing_names: list[str] +) -> None: + source_table = _qualified_table(target.source_schema, target.source_table) + target_table = _qualified_table(target.target_schema, target.target_table) + + for constraint_name in existing_names: + session.connection().execute( + text( + f"ALTER TABLE {source_table} " + f"DROP CONSTRAINT {_quote_identifier(constraint_name)}" + ) + ) + + session.connection().execute( + text( + f"ALTER TABLE {source_table} " + f"ADD CONSTRAINT {_quote_identifier(target.constraint_name)} " + f"FOREIGN KEY ({_quote_identifier(target.source_column)}) " + f"REFERENCES {target_table} ({_quote_identifier(target.target_column)}) " + "ON DELETE CASCADE" + ) + ) + + +def align_ownership_cascades(env_file: str, apply: bool) -> MigrationStats: + load_dotenv(env_file, override=True) + engine = create_engine(get_connection_url()) + already_cascading: list[str] = [] + updated: list[str] = [] + skipped_missing_tables: list[str] = [] + + try: + with Session(engine) as session: + for target in TARGETS: + if not _table_exists( + session, target.source_schema, target.source_table + ) or not _table_exists( + session, target.target_schema, target.target_table + ): + skipped_missing_tables.append(target.label) + continue + + existing = _matching_foreign_keys(session, target) + if len(existing) == 1 and existing[0][1] == "CASCADE": + already_cascading.append(target.label) + continue + + updated.append(target.label) + if apply: + _replace_foreign_key_with_cascade( + session, target, [name for name, _ in existing] + ) + + if apply: + session.commit() + else: + session.rollback() + finally: + engine.dispose() + + return MigrationStats( + already_cascading=tuple(already_cascading), + updated=tuple(updated), + skipped_missing_tables=tuple(skipped_missing_tables), + ) + + +def main() -> None: + parser = argparse.ArgumentParser( + description=( + "Recreate ownership-style foreign keys with ON DELETE CASCADE. " + "Without --apply, runs in dry-run mode." + ) + ) + parser.add_argument("env", help="Env file to use (e.g. .env)") + parser.add_argument( + "--apply", + action="store_true", + help="Apply the schema changes (default is dry-run).", + ) + args = parser.parse_args() + + stats = align_ownership_cascades(env_file=args.env, apply=args.apply) + mode = "APPLY" if args.apply else "DRY-RUN" + print(f"[{mode}] already_cascading={list(stats.already_cascading)}") + print(f"[{mode}] needs_update={list(stats.updated)}") + if stats.skipped_missing_tables: + print(f"[{mode}] skipped_missing_tables={list(stats.skipped_missing_tables)}") + if args.apply and stats.updated: + print(f"[{mode}] Foreign keys updated successfully.") + elif not args.apply and stats.updated: + print("Dry-run only. Re-run with --apply to recreate these foreign keys.") + + +if __name__ == "__main__": + main() diff --git a/routers/core/account.py b/routers/core/account.py index bef6c09..d6ddafb 100644 --- a/routers/core/account.py +++ b/routers/core/account.py @@ -8,13 +8,16 @@ from fastapi.templating import Jinja2Templates from starlette.datastructures import URLPath from pydantic import EmailStr -from sqlmodel import Session, select +from sqlmodel import Session, col, select from utils.core.models import ( User, DataIntegrityError, Account, AccountEmail, Invitation, + Organization, + Role, + UserRoleLink, ) from utils.core.dependencies import get_session from utils.core.models import RefreshToken @@ -90,6 +93,37 @@ # --- Route-specific dependencies --- +def _delete_organizations_where_user_is_only_member( + session: Session, user: User +) -> None: + """Delete organizations that would have no remaining users after user deletion.""" + if user.id is None: + return + + organization_ids = session.exec( + select(Role.organization_id) + .join(UserRoleLink, col(UserRoleLink.role_id) == col(Role.id)) + .where(UserRoleLink.user_id == user.id) + .distinct() + ).all() + + for organization_id in organization_ids: + user_ids = { + user_id + for user_id in session.exec( + select(UserRoleLink.user_id) + .join(Role, col(Role.id) == col(UserRoleLink.role_id)) + .where(Role.organization_id == organization_id) + .distinct() + ).all() + if user_id is not None + } + if user_ids == {user.id}: + organization = session.get(Organization, organization_id) + if organization is not None: + session.delete(organization) + + def validate_password_strength_and_match( password: str = Form(..., title="Password", description="Account password"), confirm_password: str = Form( @@ -264,8 +298,14 @@ async def delete_account( """ Delete a user account after verifying credentials. """ - # Delete the account and associated user - # Note: The user will be deleted automatically by cascade relationship + user = account.user + if user is None: + session.refresh(account, attribute_names=["user"]) + user = account.user + + if user is not None: + _delete_organizations_where_user_is_only_member(session, user) + session.delete(account) session.commit() diff --git a/tests/routers/core/test_account.py b/tests/routers/core/test_account.py index 63d263e..147982b 100644 --- a/tests/routers/core/test_account.py +++ b/tests/routers/core/test_account.py @@ -12,10 +12,15 @@ AccountEmail, AccountRecoveryToken, EmailVerificationToken, + Invitation, + Organization, PasswordResetToken, RefreshToken, Account, + Role, + UserRoleLink, ) +from utils.app.models import OrganizationResource from utils.core.auth import ( create_access_token, create_recovery_token, @@ -413,6 +418,122 @@ def test_logout_endpoint(auth_client: TestClient): ) +def test_delete_account_removes_sole_user_organization_and_owned_resources( + auth_client_owner: TestClient, + session: Session, + org_owner: User, + test_organization: Organization, + member_role: Role, +): + assert org_owner.id is not None + assert org_owner.account is not None + assert org_owner.account.id is not None + assert test_organization.id is not None + assert member_role.id is not None + + account_id = org_owner.account.id + user_id = org_owner.id + organization_id = test_organization.id + + resource = OrganizationResource( + organization_id=organization_id, + title="Owned resource", + ) + invitation = Invitation( + organization_id=organization_id, + role_id=member_role.id, + invitee_email="delete-org-invitee@example.com", + expires_at=datetime.now(UTC) + timedelta(days=7), + ) + session.add(resource) + session.add(invitation) + session.commit() + session.refresh(resource) + session.refresh(invitation) + assert resource.id is not None + assert invitation.id is not None + resource_id = resource.id + invitation_id = invitation.id + + response = auth_client_owner.post( + app.url_path_for("delete_account"), + data={"email": org_owner.account.email, "password": "Owner123!@#"}, + ) + + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("logout") + + session.expire_all() + assert session.get(Account, account_id) is None + assert session.get(User, user_id) is None + assert session.get(Organization, organization_id) is None + assert session.get(OrganizationResource, resource_id) is None + assert session.get(Invitation, invitation_id) is None + assert ( + session.exec(select(Role).where(Role.organization_id == organization_id)).all() + == [] + ) + + +def test_delete_account_preserves_shared_organization_and_owned_resources( + auth_client_owner: TestClient, + session: Session, + org_owner: User, + org_member_user: User, + test_organization: Organization, +): + assert org_owner.id is not None + assert org_owner.account is not None + assert org_owner.account.id is not None + assert org_member_user.id is not None + assert org_member_user.account is not None + assert org_member_user.account.id is not None + assert test_organization.id is not None + + deleted_account_id = org_owner.account.id + deleted_user_id = org_owner.id + remaining_user_id = org_member_user.id + remaining_account_id = org_member_user.account.id + organization_id = test_organization.id + + resource = OrganizationResource( + organization_id=organization_id, + title="Shared organization resource", + ) + session.add(resource) + session.commit() + session.refresh(resource) + assert resource.id is not None + resource_id = resource.id + + response = auth_client_owner.post( + app.url_path_for("delete_account"), + data={"email": org_owner.account.email, "password": "Owner123!@#"}, + ) + + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("logout") + + session.expire_all() + assert session.get(Account, deleted_account_id) is None + assert session.get(User, deleted_user_id) is None + assert session.get(Account, remaining_account_id) is not None + assert session.get(User, remaining_user_id) is not None + assert session.get(Organization, organization_id) is not None + assert session.get(OrganizationResource, resource_id) is not None + + organization_user_ids = { + user_id + for user_id in session.exec( + select(UserRoleLink.user_id) + .join(Role, Role.id == UserRoleLink.role_id) + .where(Role.organization_id == organization_id) + ).all() + } + assert deleted_user_id not in organization_user_ids + assert remaining_user_id in organization_user_ids + + # --- Error Case Tests --- diff --git a/tests/utils/test_models.py b/tests/utils/test_models.py index 1d57dd5..bd2d970 100644 --- a/tests/utils/test_models.py +++ b/tests/utils/test_models.py @@ -1,6 +1,7 @@ from datetime import timedelta, datetime, UTC from typing import Optional from sqlmodel import select, Session +from sqlalchemy import text from sqlalchemy.exc import IntegrityError import pytest from utils.core.models import ( @@ -14,6 +15,7 @@ Role, RolePermissionLink, User, + UserAvatar, UserRoleLink, ) from utils.core.enums import ValidPermissions @@ -474,6 +476,52 @@ def test_account_email_cascade_delete(session: Session, test_account: Account): assert len(remaining) == 0 +def test_account_email_database_cascade_delete(session: Session, test_account: Account): + """Test that AccountEmail rows cascade when the parent account is deleted in SQL.""" + email = AccountEmail( + account_id=test_account.id, + email="db-cascade@example.com", + is_primary=True, + verified=True, + ) + session.add(email) + session.commit() + session.refresh(email) + assert email.id is not None + email_id = email.id + + session.connection().execute( + text('DELETE FROM private."account" WHERE id = :account_id'), + {"account_id": test_account.id}, + ) + session.commit() + + assert session.get(AccountEmail, email_id) is None + + +def test_user_avatar_database_cascade_delete(session: Session, test_user: User): + """Test that UserAvatar rows cascade when the parent user is deleted in SQL.""" + assert test_user.id is not None + avatar = UserAvatar( + user_id=test_user.id, + avatar_data=b"avatar-bytes", + avatar_content_type="image/png", + ) + session.add(avatar) + session.commit() + session.refresh(avatar) + assert avatar.id is not None + avatar_id = avatar.id + + session.connection().execute( + text('DELETE FROM "user" WHERE id = :user_id'), + {"user_id": test_user.id}, + ) + session.commit() + + assert session.get(UserAvatar, avatar_id) is None + + # --- EmailVerificationToken model tests --- diff --git a/utils/app/models.py b/utils/app/models.py index f947e72..482dfbb 100644 --- a/utils/app/models.py +++ b/utils/app/models.py @@ -27,7 +27,9 @@ class OrganizationResource(SQLModel, table=True): """ id: Optional[int] = Field(default=None, primary_key=True) - organization_id: int = Field(foreign_key="organization.id", index=True) + organization_id: int = Field( + foreign_key="organization.id", ondelete="CASCADE", index=True + ) title: str description: Optional[str] = None created_at: datetime = Field(default_factory=utc_now) diff --git a/utils/core/models.py b/utils/core/models.py index a8b8f46..407511b 100644 --- a/utils/core/models.py +++ b/utils/core/models.py @@ -88,7 +88,9 @@ class AccountEmail(SQLModel, table=True): ) id: Optional[int] = Field(default=None, primary_key=True) - account_id: int = Field(foreign_key="private.account.id", index=True) + account_id: int = Field( + foreign_key="private.account.id", ondelete="CASCADE", index=True + ) email: str = Field(index=True) is_primary: bool = Field(default=False) verified: bool = Field(default=False) @@ -183,7 +185,9 @@ class UserAvatar(SQLModel, table=True): __tablename__ = "useravatar" id: Optional[int] = Field(default=None, primary_key=True) - user_id: int = Field(foreign_key="user.id", unique=True, index=True) + user_id: int = Field( + foreign_key="user.id", ondelete="CASCADE", unique=True, index=True + ) avatar_data: bytes = Field(sa_column=Column(LargeBinary, nullable=False)) avatar_content_type: str