From ea1bacc79424a62ab664f0807f564eeca50edadd Mon Sep 17 00:00:00 2001 From: James Bhattarai Date: Tue, 3 Mar 2026 23:22:50 +0545 Subject: [PATCH] Fix: Deletion Deadlocks (Owner, User) --- gatehouse_app/api/v1/organizations.py | 65 ++++++++---- gatehouse_app/api/v1/users.py | 40 +++++--- .../services/organization_service.py | 46 +++++++++ gatehouse_app/services/user_service.py | 99 ++++++++++++++++++- 4 files changed, 215 insertions(+), 35 deletions(-) diff --git a/gatehouse_app/api/v1/organizations.py b/gatehouse_app/api/v1/organizations.py index 7d8f4d3..447ac0d 100644 --- a/gatehouse_app/api/v1/organizations.py +++ b/gatehouse_app/api/v1/organizations.py @@ -220,48 +220,73 @@ def update_organization(org_id): @api_v1_bp.route("/organizations/", methods=["DELETE"]) @login_required -@require_owner @full_access_required def delete_organization(org_id): """ Delete organization (soft delete). - The owner may only delete the organization if they are the *sole* remaining - member. If other active members exist they must first transfer ownership - (or remove all other members) before deleting the organization. + Only the OWNER of the organization may call this endpoint. + + When the organization has other active members the caller must explicitly + confirm the deletion by sending ``{"confirm": true}`` in the request body. + All members (and their memberships) are soft-deleted together with the org + in a single atomic transaction so no orphaned data is left behind. Args: org_id: Organization ID + Request body (JSON, optional): + confirm (bool): Required when the org has other active members. + Returns: 200: Organization deleted successfully + 400: Organization has other members but confirm was not true 401: Not authenticated 403: Not the owner 404: Organization not found - 409: Organization still has other members — transfer ownership first """ + from gatehouse_app.models.organization.organization_member import OrganizationMember as _OrgMember + from gatehouse_app.utils.constants import OrganizationRole as _OrgRole + + caller = g.current_user + org = OrganizationService.get_organization_by_id(org_id) - # Guard: block deletion while non-owner members still exist so ownership - # can be transferred rather than silently orphaning them. - active_member_count = org.get_member_count() - if active_member_count > 1: + # Only the owner may delete the organization. + caller_membership = _OrgMember.query.filter_by( + user_id=caller.id, + organization_id=org.id, + deleted_at=None, + ).first() + + if not caller_membership or caller_membership.role != _OrgRole.OWNER: return api_response( success=False, - message=( - "This organization still has other members. " - "Please transfer ownership to another member or remove all " - "other members before deleting the organization." - ), - status=409, - error_type="ORG_HAS_MEMBERS", - error_details={"member_count": active_member_count}, + message="Only the organization owner can delete the organization.", + status=403, + error_type="AUTHORIZATION_ERROR", ) - OrganizationService.delete_organization( + # If other members exist, require explicit confirmation to avoid accidents. + active_member_count = org.get_member_count() + if active_member_count > 1: + data = request.get_json(silent=True) or {} + if not data.get("confirm"): + return api_response( + success=False, + message=( + f"This organization has {active_member_count} active members. " + "Deleting it will remove all members and their data. " + 'Send {"confirm": true} to confirm.' + ), + status=400, + error_type="CONFIRMATION_REQUIRED", + error_details={"member_count": active_member_count}, + ) + + OrganizationService.force_delete_organization( org=org, - user_id=g.current_user.id, - soft=True, + user_id=caller.id, ) return api_response( diff --git a/gatehouse_app/api/v1/users.py b/gatehouse_app/api/v1/users.py index 65c42c3..112ea1f 100644 --- a/gatehouse_app/api/v1/users.py +++ b/gatehouse_app/api/v1/users.py @@ -73,50 +73,62 @@ def delete_me(): """ Delete current user account (soft delete). - Blocked if the user is the sole owner of any organization that has other - active members — they must transfer ownership or dissolve those organizations - first. + Behaviour for owned organizations: + - If the org has other active members → blocked; user must transfer ownership first. + - If they are the sole member → org is automatically cascade-deleted (no orphan risk). Returns: - 200: Account deleted successfully + 200: Account deleted successfully (sole-member orgs auto-deleted) 401: Not authenticated - 409: User is sole owner of one or more organizations with other members + 409: USER_IS_SOLE_OWNER — user owns orgs that still have other members """ from gatehouse_app.models.organization.organization_member import OrganizationMember from gatehouse_app.utils.constants import OrganizationRole + from gatehouse_app.services.organization_service import OrganizationService user = g.current_user - # Find orgs where this user is the sole owner AND other members exist. + # Find all orgs where this user is the owner. owned_memberships = OrganizationMember.query.filter_by( user_id=user.id, role=OrganizationRole.OWNER, deleted_at=None, ).all() - blocked_orgs = [] + # Separate into two buckets depending on whether other members exist. + transfer_needed = [] # org has other members → must transfer ownership first + auto_delete = [] # user is sole member → safe to cascade-delete automatically + for membership in owned_memberships: org = membership.organization if org.deleted_at is not None: continue member_count = org.get_member_count() if member_count > 1: - blocked_orgs.append(org.name) + transfer_needed.append(org.name) + else: + auto_delete.append(org) - if blocked_orgs: - names = ", ".join(f'"{n}"' for n in blocked_orgs) + # Hard block: user owns orgs with other members — must transfer first. + if transfer_needed: + names = ", ".join(f'"{n}"' for n in transfer_needed) return api_response( success=False, message=( - f"You are the sole owner of {len(blocked_orgs)} organization" - f"{'s' if len(blocked_orgs) > 1 else ''}: {names}. " - "Transfer ownership or delete those organizations before deleting your account." + f"You are the owner of {len(transfer_needed)} organization" + f"{'s' if len(transfer_needed) > 1 else ''} that still " + f"{'have' if len(transfer_needed) > 1 else 'has'} other members " + f"({names}). Transfer ownership to another member first." ), status=409, error_type="USER_IS_SOLE_OWNER", - error_details={"organizations": blocked_orgs}, + error_details={"transfer_ownership": transfer_needed}, ) + # Auto-delete any sole-member orgs so no orphaned org rows can ever be left behind. + for org in auto_delete: + OrganizationService.force_delete_organization(org, user_id=user.id) + UserService.delete_user(user, soft=True) return api_response( diff --git a/gatehouse_app/services/organization_service.py b/gatehouse_app/services/organization_service.py index 27ec7f4..c071a9d 100644 --- a/gatehouse_app/services/organization_service.py +++ b/gatehouse_app/services/organization_service.py @@ -171,6 +171,52 @@ class OrganizationService: return org + @staticmethod + def force_delete_organization(org, user_id): + """ + Force-delete an organization and all its members in a single atomic operation. + + All active memberships are soft-deleted before the organization itself + is soft-deleted, preventing orphaned membership rows and avoiding any + cascade deadlocks. + + Args: + org: Organization instance + user_id: ID of the owner performing the delete + + Returns: + Deleted Organization instance + """ + from datetime import datetime, timezone + + now = datetime.now(timezone.utc) + member_count = 0 + + # Soft-delete all active memberships first. + for member in org.members: + if member.deleted_at is None: + member.deleted_at = now + member_count += 1 + + # Now soft-delete the organization itself. + org.delete(soft=True) + + # Log with member count for audit trail. + AuditService.log_action( + action=AuditAction.ORG_DELETE, + user_id=user_id, + organization_id=org.id, + resource_type="organization", + resource_id=org.id, + metadata={"members_removed": member_count}, + description=( + f"Organization deleted by owner; " + f"{member_count} membership(s) removed." + ), + ) + + return org + @staticmethod def add_member(org, user_id, role, inviter_id): """ diff --git a/gatehouse_app/services/user_service.py b/gatehouse_app/services/user_service.py index b8b5fc9..d98a404 100644 --- a/gatehouse_app/services/user_service.py +++ b/gatehouse_app/services/user_service.py @@ -91,6 +91,26 @@ class UserService: """ Delete user account. + For a soft delete this method also soft-deletes every related row that + has its own ``deleted_at`` column so that those records stop appearing + in active queries immediately: + - OrganizationMember (user no longer shows in member lists) + - DepartmentMembership + - PrincipalMembership + - CAPermission (CA access revoked) + - MfaPolicyCompliance (compliance record hidden) + - AuthenticationMethod (login methods hidden) + - SSHKey (keys are hidden) + - SSHCertificate (certs are revoked + hidden) + - Session (all active sessions killed) + - OIDCAuthCode (pending auth codes invalidated) + - OIDCRefreshToken (refresh tokens invalidated) + - OIDCSession (OIDC sessions killed) + - OIDCTokenMetadata (token metadata hidden) + + All changes are committed in a single transaction after the user row + itself is marked deleted, preventing any partial-delete state. + Args: user: User instance soft: If True, performs soft delete @@ -98,7 +118,84 @@ class UserService: Returns: Deleted User instance """ - user.delete(soft=soft) + from datetime import datetime, timezone + from gatehouse_app.extensions import db as _db + + if soft: + now = datetime.now(timezone.utc) + + # --- Org memberships ------------------------------------------- + for m in user.organization_memberships: + if m.deleted_at is None: + m.deleted_at = now + + # --- Department memberships ------------------------------------- + for m in user.department_memberships: + if m.deleted_at is None: + m.deleted_at = now + + # --- Principal memberships -------------------------------------- + for m in user.principal_memberships: + if m.deleted_at is None: + m.deleted_at = now + + # --- CA permissions -------------------------------------------- + for p in user.ca_permissions: + if p.deleted_at is None: + p.deleted_at = now + + # --- MFA compliance records ------------------------------------ + for c in user.mfa_compliance: + if c.deleted_at is None: + c.deleted_at = now + + # --- Authentication methods ------------------------------------ + for m in user.authentication_methods: + if m.deleted_at is None: + m.deleted_at = now + + # --- SSH keys --------------------------------------------------- + for key in user.ssh_keys: + if key.deleted_at is None: + key.deleted_at = now + + # --- SSH certificates: revoke then soft-delete ------------------ + for cert in user.ssh_certificates: + if cert.deleted_at is None: + try: + if not getattr(cert, "revoked", False): + cert.revoke("account_deleted") + except Exception: + pass + cert.deleted_at = now + + # --- Sessions --------------------------------------------------- + for session in user.sessions: + if session.deleted_at is None: + session.deleted_at = now + + # --- OIDC tokens / sessions ------------------------------------ + for code in user.oidc_auth_codes: + if code.deleted_at is None: + code.deleted_at = now + + for token in user.oidc_refresh_tokens: + if token.deleted_at is None: + token.deleted_at = now + + for oidc_session in user.oidc_sessions: + if oidc_session.deleted_at is None: + oidc_session.deleted_at = now + + for meta in user.oidc_token_metadata: + if meta.deleted_at is None: + meta.deleted_at = now + + # --- Mark the user row itself ----------------------------------- + user.deleted_at = now + _db.session.commit() + else: + user.delete(soft=False) # Log user deletion AuditService.log_action(