fix: prevent ghost memberships from soft-deleted users
This commit is contained in:
@@ -306,10 +306,10 @@ def admin_delete_user(user_id):
|
|||||||
from gatehouse_app.models.user.user import User as _User
|
from gatehouse_app.models.user.user import User as _User
|
||||||
from gatehouse_app.models.ssh_ca.ssh_key import SSHKey
|
from gatehouse_app.models.ssh_ca.ssh_key import SSHKey
|
||||||
from gatehouse_app.models.ssh_ca.ssh_certificate import SSHCertificate
|
from gatehouse_app.models.ssh_ca.ssh_certificate import SSHCertificate
|
||||||
from gatehouse_app.models.auth.authentication_method import OAuthState
|
|
||||||
from gatehouse_app.extensions import db as _db
|
from gatehouse_app.extensions import db as _db
|
||||||
from gatehouse_app.utils.constants import AuditAction, OrganizationRole
|
from gatehouse_app.utils.constants import AuditAction, OrganizationRole
|
||||||
from gatehouse_app.services.audit_service import AuditService
|
from gatehouse_app.services.audit_service import AuditService
|
||||||
|
from gatehouse_app.services.user_service import UserService
|
||||||
|
|
||||||
caller = g.current_user
|
caller = g.current_user
|
||||||
data = request.get_json() or {}
|
data = request.get_json() or {}
|
||||||
@@ -372,20 +372,10 @@ def admin_delete_user(user_id):
|
|||||||
|
|
||||||
target_email = target.email
|
target_email = target.email
|
||||||
target_id_str = str(target.id)
|
target_id_str = str(target.id)
|
||||||
now = datetime.now(timezone.utc)
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
# Soft delete the user — set deleted_at timestamp.
|
UserService.delete_user(target, soft=True)
|
||||||
target.deleted_at = now
|
|
||||||
|
|
||||||
# Soft delete associated OAuthState records.
|
|
||||||
OAuthState.query.filter_by(user_id=target_id_str).filter(OAuthState.deleted_at == None).update(
|
|
||||||
{"deleted_at": now}, synchronize_session=False
|
|
||||||
)
|
|
||||||
|
|
||||||
_db.session.flush()
|
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
_db.session.rollback()
|
|
||||||
_logger.error(f"Soft delete failed for {target_id_str}: {exc}")
|
_logger.error(f"Soft delete failed for {target_id_str}: {exc}")
|
||||||
return api_response(success=False, message="Failed to delete user account. Please try again.", status=500, error_type="SERVER_ERROR")
|
return api_response(success=False, message="Failed to delete user account. Please try again.", status=500, error_type="SERVER_ERROR")
|
||||||
|
|
||||||
|
|||||||
@@ -68,11 +68,18 @@ class Organization(BaseModel):
|
|||||||
def is_member(self, user_id: str) -> bool:
|
def is_member(self, user_id: str) -> bool:
|
||||||
"""Check if a user is a member of the organization."""
|
"""Check if a user is a member of the organization."""
|
||||||
from gatehouse_app.models.organization.organization_member import OrganizationMember
|
from gatehouse_app.models.organization.organization_member import OrganizationMember
|
||||||
|
from gatehouse_app.models.user.user import User
|
||||||
|
|
||||||
return (
|
return (
|
||||||
OrganizationMember.query.filter_by(
|
db.session.query(OrganizationMember)
|
||||||
user_id=user_id, organization_id=self.id, deleted_at=None
|
.join(User, OrganizationMember.user_id == User.id)
|
||||||
).first()
|
.filter(
|
||||||
|
OrganizationMember.user_id == user_id,
|
||||||
|
OrganizationMember.organization_id == self.id,
|
||||||
|
OrganizationMember.deleted_at.is_(None),
|
||||||
|
User.deleted_at.is_(None),
|
||||||
|
)
|
||||||
|
.first()
|
||||||
is not None
|
is not None
|
||||||
)
|
)
|
||||||
def get_active_members(self):
|
def get_active_members(self):
|
||||||
|
|||||||
@@ -105,6 +105,7 @@ class UserService:
|
|||||||
- Session (all active sessions killed)
|
- Session (all active sessions killed)
|
||||||
- OIDCAuthCode (pending auth codes invalidated)
|
- OIDCAuthCode (pending auth codes invalidated)
|
||||||
- OIDCRefreshToken (refresh tokens invalidated)
|
- OIDCRefreshToken (refresh tokens invalidated)
|
||||||
|
- OAuthState (OAuth flow states invalidated)
|
||||||
- OIDCSession (OIDC sessions killed)
|
- OIDCSession (OIDC sessions killed)
|
||||||
- OIDCTokenMetadata (token metadata hidden)
|
- OIDCTokenMetadata (token metadata hidden)
|
||||||
|
|
||||||
@@ -120,6 +121,7 @@ class UserService:
|
|||||||
"""
|
"""
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from gatehouse_app.extensions import db as _db
|
from gatehouse_app.extensions import db as _db
|
||||||
|
from gatehouse_app.models.auth.authentication_method import OAuthState
|
||||||
|
|
||||||
if soft:
|
if soft:
|
||||||
now = datetime.now(timezone.utc)
|
now = datetime.now(timezone.utc)
|
||||||
@@ -169,6 +171,11 @@ class UserService:
|
|||||||
pass
|
pass
|
||||||
cert.deleted_at = now
|
cert.deleted_at = now
|
||||||
|
|
||||||
|
# --- OAuth states -----------------------------------------------
|
||||||
|
OAuthState.query.filter_by(user_id=user.id).filter(
|
||||||
|
OAuthState.deleted_at == None
|
||||||
|
).update({"deleted_at": now}, synchronize_session=False)
|
||||||
|
|
||||||
# --- Sessions ---------------------------------------------------
|
# --- Sessions ---------------------------------------------------
|
||||||
for session in user.sessions:
|
for session in user.sessions:
|
||||||
if session.deleted_at is None:
|
if session.deleted_at is None:
|
||||||
|
|||||||
@@ -212,6 +212,48 @@ class TestAdminUserManagement:
|
|||||||
integration_client.auth.login(email=victim["email"], password="VictimPass123!")
|
integration_client.auth.login(email=victim["email"], password="VictimPass123!")
|
||||||
assert exc_info.value.status_code in (400, 401)
|
assert exc_info.value.status_code in (400, 401)
|
||||||
|
|
||||||
|
def test_admin_soft_delete_cascades_org_membership(self, integration_app, integration_client, create_test_user, create_test_org, create_test_membership):
|
||||||
|
"""TEST: ADMIN-12 — Admin soft-delete cascades to org memberships.
|
||||||
|
|
||||||
|
WHAT: Admin soft-deletes a user in an org. The membership row
|
||||||
|
and the user row both get soft-deleted.
|
||||||
|
WHY: Ghost memberships (membership active but user deleted) would
|
||||||
|
make Organization.is_member() return True and break lookups.
|
||||||
|
EXPECTED: 200 OK. OrganizationMember.deleted_at and User.deleted_at
|
||||||
|
are set. Organization.is_member() returns False.
|
||||||
|
"""
|
||||||
|
from gatehouse_app.models.organization.organization_member import OrganizationMember
|
||||||
|
from gatehouse_app.models.organization.organization import Organization
|
||||||
|
from gatehouse_app.models.user.user import User
|
||||||
|
from gatehouse_app.extensions import db
|
||||||
|
|
||||||
|
admin = create_test_user(password="AdminPass123!")
|
||||||
|
victim = create_test_user(password="VictimPass123!")
|
||||||
|
org = create_test_org()
|
||||||
|
create_test_membership(admin["id"], org["id"], OrganizationRole.OWNER)
|
||||||
|
create_test_membership(victim["id"], org["id"], OrganizationRole.MEMBER)
|
||||||
|
|
||||||
|
integration_client.auth.login(email=admin["email"], password="AdminPass123!")
|
||||||
|
result = integration_client.admin.hard_delete_user(victim["id"], confirm=True)
|
||||||
|
assert_success(result)
|
||||||
|
|
||||||
|
with integration_app.app_context():
|
||||||
|
# Membership row still exists but is soft-deleted
|
||||||
|
membership = OrganizationMember.query.filter_by(
|
||||||
|
user_id=victim["id"], organization_id=org["id"]
|
||||||
|
).first()
|
||||||
|
assert membership is not None
|
||||||
|
assert membership.deleted_at is not None
|
||||||
|
|
||||||
|
# User row is soft-deleted
|
||||||
|
user = User.query.get(victim["id"])
|
||||||
|
assert user is not None
|
||||||
|
assert user.deleted_at is not None
|
||||||
|
|
||||||
|
# Organization.is_member() returns False (defense in depth)
|
||||||
|
org_obj = Organization.query.get(org["id"])
|
||||||
|
assert org_obj.is_member(victim["id"]) is False
|
||||||
|
|
||||||
|
|
||||||
class TestAdminSSHCertificates:
|
class TestAdminSSHCertificates:
|
||||||
"""Test admin SSH certificate listing endpoints."""
|
"""Test admin SSH certificate listing endpoints."""
|
||||||
|
|||||||
Reference in New Issue
Block a user