mirror of
https://github.com/jhd3197/ServerKit.git
synced 2026-10-28 15:47:50 +09:00
fix(models): repo-wide delete-cascade audit for NOT NULL child FKs
Follow-up to 1704d60b, prompted by 'can this happen elsewhere?' — a
mapper-wide audit found 22 more one-to-many relationships whose child
FK is NOT NULL but which lacked a delete cascade, so hard-deleting the
parent (user/server/workspace/template/plugin delete, deployment
retention pruning, WP site teardown) raised the same IntegrityError as
the Recycle Bin purge.
20 get the cascade: plugin installs, deployment diffs, metric alerts,
template assignments (both sides), WP activities/sync/promotion jobs
(both sides), workspace members/keys, and the per-user detail rows
(passkeys, OAuth identities, notification prefs, sanitization
profiles, source connections, workflows, workspace memberships).
2 stay uncascaded on purpose, now enforced: User.applications (delete_
user refuses with 409 while the user owns apps — apps must go through
app delete + Recycle Bin purge so containers/volumes are cleaned) and
CloudProvider.servers (no delete path exists; dropping VM inventory
must stay a deliberate decision). The invariant sweep is now global
across every mapper with exactly that allowlist, which may only shrink.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
5bfb0f6747
commit
79a78c3c38
16 files changed
+139
-36
No files matched your search
@@ -216,6 +216,17 @@ def delete_user(user_id):
|
||||
if admin_count <= 1:
|
||||
return jsonify({'error': 'Cannot delete the last admin'}), 400
|
||||
|
||||
# Applications never ride a user delete: their user_id is NOT NULL, but the
|
||||
# rows own containers and volumes that must go through app delete + Recycle
|
||||
# Bin purge — never a silent cascade. Unfiltered on purpose: a tombstoned
|
||||
# app still holds the FK, so it blocks too.
|
||||
from app.models import Application
|
||||
owned = Application.query.filter_by(user_id=user.id).count()
|
||||
if owned:
|
||||
return jsonify({'error': f'User still owns {owned} application(s), '
|
||||
'including any in the Recycle Bin — delete or '
|
||||
'purge them first'}), 409
|
||||
|
||||
username = user.username
|
||||
user_id_deleted = user.id
|
||||
|
||||
|
||||
@@ -40,7 +40,10 @@ class AgentPlugin(JsonColumnMixin, TimestampMixin, db.Model):
|
||||
|
||||
|
||||
# Relationships
|
||||
installations = db.relationship('AgentPluginInstall', backref='plugin', lazy='dynamic')
|
||||
# cascade: install rows have a NOT NULL plugin_id — deleting the plugin must
|
||||
# delete them, or the default FK null-out IntegrityErrors.
|
||||
installations = db.relationship('AgentPluginInstall', backref='plugin',
|
||||
lazy='dynamic', cascade='all, delete-orphan')
|
||||
|
||||
@property
|
||||
def manifest(self):
|
||||
@@ -127,7 +130,9 @@ class AgentPluginInstall(JsonColumnMixin, db.Model):
|
||||
health_status = db.Column(db.String(32), default='unknown') # healthy, degraded, unhealthy, unknown
|
||||
metrics_json = db.Column(db.Text)
|
||||
|
||||
server = db.relationship('Server', backref=db.backref('plugin_installs', lazy='dynamic'))
|
||||
# cascade: server_id is NOT NULL — removing a server must take its installs.
|
||||
server = db.relationship('Server', backref=db.backref(
|
||||
'plugin_installs', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
@property
|
||||
def config(self):
|
||||
|
||||
@@ -194,7 +194,10 @@ class DeploymentDiff(JsonColumnMixin, db.Model):
|
||||
|
||||
created_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
|
||||
deployment = db.relationship('Deployment', foreign_keys=[deployment_id], backref='diff')
|
||||
# cascade: deployment_id is NOT NULL — retention pruning (and app purge)
|
||||
# deletes Deployment rows, which must take their diff along.
|
||||
deployment = db.relationship('Deployment', foreign_keys=[deployment_id],
|
||||
backref=db.backref('diff', cascade='all, delete-orphan'))
|
||||
|
||||
def to_dict(self):
|
||||
return {
|
||||
|
||||
@@ -33,7 +33,9 @@ class EnvironmentActivity(JsonColumnMixin, db.Model):
|
||||
created_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
|
||||
# Relationships
|
||||
site = db.relationship('WordPressSite', backref=db.backref('activities', lazy='dynamic'))
|
||||
# cascade: site_id is NOT NULL — a site's activity log dies with the site.
|
||||
site = db.relationship('WordPressSite', backref=db.backref(
|
||||
'activities', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
user = db.relationship('User', backref=db.backref('environment_activities', lazy='dynamic'))
|
||||
|
||||
def to_dict(self):
|
||||
|
||||
@@ -55,7 +55,9 @@ class MetricAlert(db.Model):
|
||||
created_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
resolved_at = db.Column(db.DateTime)
|
||||
|
||||
server = db.relationship('Server', backref='metric_alerts')
|
||||
# cascade: server_id is NOT NULL — alerts die with their server.
|
||||
server = db.relationship('Server', backref=db.backref(
|
||||
'metric_alerts', cascade='all, delete-orphan'))
|
||||
|
||||
def to_dict(self):
|
||||
return {
|
||||
|
||||
@@ -63,7 +63,9 @@ class NotificationPreferences(TimestampMixin, JsonColumnMixin, db.Model):
|
||||
|
||||
|
||||
# Relationship
|
||||
user = db.relationship('User', backref=db.backref('notification_preferences', uselist=False))
|
||||
# cascade: user_id is NOT NULL — preferences die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'notification_preferences', uselist=False, cascade='all, delete-orphan'))
|
||||
|
||||
def get_channels(self):
|
||||
"""Get the list of enabled channels."""
|
||||
|
||||
@@ -21,7 +21,9 @@ class OAuthIdentity(TimestampMixin, db.Model):
|
||||
token_expires_at = db.Column(db.DateTime, nullable=True)
|
||||
last_login_at = db.Column(db.DateTime, nullable=True)
|
||||
|
||||
user = db.relationship('User', backref=db.backref('oauth_identities', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — linked identities die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'oauth_identities', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
def to_dict(self):
|
||||
return {
|
||||
|
||||
@@ -29,7 +29,9 @@ class PasskeyCredential(JsonColumnMixin, db.Model):
|
||||
created_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
last_used_at = db.Column(db.DateTime, nullable=True)
|
||||
|
||||
user = db.relationship('User', backref=db.backref('passkeys', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — passkeys die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'passkeys', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
def get_transports(self):
|
||||
return self._json_read('transports', [])
|
||||
|
||||
@@ -40,10 +40,14 @@ class PromotionJob(JsonColumnMixin, db.Model):
|
||||
created_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
|
||||
# Relationships
|
||||
# cascade: both site FKs are NOT NULL — a promotion record cannot outlive
|
||||
# either endpoint, so deleting a site takes its job history along.
|
||||
source_site = db.relationship('WordPressSite', foreign_keys=[source_site_id],
|
||||
backref=db.backref('promotions_as_source', lazy='dynamic'))
|
||||
backref=db.backref('promotions_as_source', lazy='dynamic',
|
||||
cascade='all, delete-orphan'))
|
||||
target_site = db.relationship('WordPressSite', foreign_keys=[target_site_id],
|
||||
backref=db.backref('promotions_as_target', lazy='dynamic'))
|
||||
backref=db.backref('promotions_as_target', lazy='dynamic',
|
||||
cascade='all, delete-orphan'))
|
||||
user = db.relationship('User', backref=db.backref('promotion_jobs', lazy='dynamic'))
|
||||
pre_promotion_snapshot = db.relationship('DatabaseSnapshot', foreign_keys=[pre_promotion_snapshot_id])
|
||||
|
||||
|
||||
@@ -43,7 +43,9 @@ class SanitizationProfile(JsonColumnMixin, db.Model):
|
||||
updated_at = db.Column(db.DateTime, onupdate=datetime.utcnow)
|
||||
|
||||
# Relationships
|
||||
user = db.relationship('User', backref=db.backref('sanitization_profiles', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — personal profiles die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'sanitization_profiles', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
def get_config(self):
|
||||
"""Parse and return the config dict."""
|
||||
|
||||
@@ -35,7 +35,9 @@ class ServerTemplate(JsonColumnMixin, TimestampMixin, db.Model):
|
||||
|
||||
created_by = db.Column(db.Integer, db.ForeignKey('users.id'))
|
||||
|
||||
assignments = db.relationship('ServerTemplateAssignment', backref='template', lazy='dynamic')
|
||||
# cascade: template_id is NOT NULL — assignments die with the template.
|
||||
assignments = db.relationship('ServerTemplateAssignment', backref='template',
|
||||
lazy='dynamic', cascade='all, delete-orphan')
|
||||
|
||||
def _get_json(self, field):
|
||||
return self._json_read(field, [])
|
||||
@@ -172,7 +174,9 @@ class ServerTemplateAssignment(JsonColumnMixin, db.Model):
|
||||
applied_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
updated_at = db.Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow)
|
||||
|
||||
server = db.relationship('Server', backref=db.backref('template_assignments', lazy='dynamic'))
|
||||
# cascade: server_id is NOT NULL — assignments die with the server too.
|
||||
server = db.relationship('Server', backref=db.backref(
|
||||
'template_assignments', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
__table_args__ = (
|
||||
db.UniqueConstraint('template_id', 'server_id', name='uq_template_server'),
|
||||
|
||||
@@ -28,7 +28,9 @@ class SourceConnection(SerializableMixin, db.Model):
|
||||
updated_at = db.Column(db.DateTime, default=datetime.utcnow, onupdate=datetime.utcnow)
|
||||
last_used_at = db.Column(db.DateTime, nullable=True)
|
||||
|
||||
user = db.relationship('User', backref=db.backref('source_connections', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — a user's git credentials die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'source_connections', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
__table_args__ = (
|
||||
db.UniqueConstraint('user_id', 'provider', name='uq_source_connection_user_provider'),
|
||||
|
||||
@@ -113,17 +113,21 @@ class WordPressSite(JsonColumnMixin, TimestampMixin, db.Model):
|
||||
vulnerabilities = db.relationship('WordPressVulnerability', backref='site', lazy='dynamic', cascade='all, delete-orphan')
|
||||
update_runs = db.relationship('WordPressUpdateRun', backref='site', lazy='dynamic', cascade='all, delete-orphan')
|
||||
reports = db.relationship('WordPressReport', backref='site', lazy='dynamic', cascade='all, delete-orphan')
|
||||
# cascade on both: the site FKs are NOT NULL, so a sync job cannot outlive
|
||||
# either endpoint — deleting a site takes its job history along.
|
||||
sync_jobs_as_source = db.relationship(
|
||||
'SyncJob',
|
||||
foreign_keys='SyncJob.source_site_id',
|
||||
backref='source_site',
|
||||
lazy='dynamic'
|
||||
lazy='dynamic',
|
||||
cascade='all, delete-orphan'
|
||||
)
|
||||
sync_jobs_as_target = db.relationship(
|
||||
'SyncJob',
|
||||
foreign_keys='SyncJob.target_site_id',
|
||||
backref='target_site',
|
||||
lazy='dynamic'
|
||||
lazy='dynamic',
|
||||
cascade='all, delete-orphan'
|
||||
)
|
||||
|
||||
def to_dict(self, include_environments=False, include_snapshots=False):
|
||||
|
||||
@@ -29,7 +29,9 @@ class Workflow(JsonColumnMixin, TimestampMixin, db.Model):
|
||||
user_id = db.Column(db.Integer, db.ForeignKey('users.id'), nullable=False)
|
||||
|
||||
# Relationships
|
||||
user = db.relationship('User', backref=db.backref('workflows', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — a user's workflows die with the account.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'workflows', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
executions = db.relationship('WorkflowExecution', backref='workflow', lazy='dynamic', cascade='all, delete-orphan')
|
||||
|
||||
def to_dict(self):
|
||||
|
||||
@@ -36,8 +36,12 @@ class Workspace(JsonColumnMixin, TimestampMixin, db.Model):
|
||||
|
||||
created_by = db.Column(db.Integer, db.ForeignKey('users.id'))
|
||||
|
||||
members = db.relationship('WorkspaceMember', backref='workspace', lazy='dynamic')
|
||||
api_keys = db.relationship('WorkspaceApiKey', backref='workspace', lazy='dynamic')
|
||||
# cascade: both child FKs are NOT NULL — memberships and keys die with the
|
||||
# workspace, or the default FK null-out IntegrityErrors on delete.
|
||||
members = db.relationship('WorkspaceMember', backref='workspace',
|
||||
lazy='dynamic', cascade='all, delete-orphan')
|
||||
api_keys = db.relationship('WorkspaceApiKey', backref='workspace',
|
||||
lazy='dynamic', cascade='all, delete-orphan')
|
||||
|
||||
@property
|
||||
def settings(self):
|
||||
@@ -85,7 +89,9 @@ class WorkspaceMember(db.Model):
|
||||
|
||||
joined_at = db.Column(db.DateTime, default=datetime.utcnow)
|
||||
|
||||
user = db.relationship('User', backref=db.backref('workspace_memberships', lazy='dynamic'))
|
||||
# cascade: user_id is NOT NULL — memberships die with the account too.
|
||||
user = db.relationship('User', backref=db.backref(
|
||||
'workspace_memberships', lazy='dynamic', cascade='all, delete-orphan'))
|
||||
|
||||
__table_args__ = (
|
||||
db.UniqueConstraint('workspace_id', 'user_id', name='uq_workspace_user'),
|
||||
|
||||
@@ -190,26 +190,76 @@ def test_purge_takes_the_one_to_one_policy_rows_with_it(app, dead_app, monkeypat
|
||||
assert Deployment.query.filter_by(app_id=dead_app.id).first() is None
|
||||
|
||||
|
||||
def test_every_not_null_child_of_application_cascades_deletes(app):
|
||||
"""The invariant behind the purge crash, pinned for FUTURE models too: any
|
||||
one-to-many from Application whose child FK is NOT NULL must carry a delete
|
||||
cascade, because SQLAlchemy's default on parent delete is to NULL the child
|
||||
FK — which the NOT NULL constraint turns into an IntegrityError at purge."""
|
||||
# Relationships DELIBERATELY left without a delete cascade. Each entry must be
|
||||
# backed by something that keeps the parent delete from ever reaching the FK
|
||||
# null-out — a guard, or the absence of any delete path:
|
||||
# User.applications — delete_user REFUSES (409) while the user owns apps;
|
||||
# apps go through app delete + Recycle Bin purge so
|
||||
# containers/volumes are cleaned, never a cascade.
|
||||
# CloudProvider.servers — no code path deletes a CloudProvider row today;
|
||||
# silently dropping the inventory of provisioned
|
||||
# (billing!) VMs must stay a deliberate decision.
|
||||
DELIBERATELY_UNCASCADED = {
|
||||
'User.applications',
|
||||
'CloudProvider.servers',
|
||||
}
|
||||
|
||||
|
||||
def test_every_not_null_child_relationship_cascades_deletes(app):
|
||||
"""The invariant behind the purge crash, pinned for EVERY model: a
|
||||
one-to-many whose child FK is NOT NULL must carry a delete cascade, because
|
||||
SQLAlchemy's default on parent delete — dynamic relationships included — is
|
||||
to NULL the child FK, which the NOT NULL constraint turns into an
|
||||
IntegrityError the moment anything hard-deletes the parent (Recycle Bin
|
||||
purge, retention pruning, admin deletes). Exceptions are enumerated and
|
||||
justified in DELIBERATELY_UNCASCADED above."""
|
||||
from sqlalchemy import inspect as sa_inspect
|
||||
|
||||
with app.app_context():
|
||||
offenders = []
|
||||
for rel in sa_inspect(Application).relationships:
|
||||
if rel.direction.name != 'ONETOMANY' or rel.viewonly:
|
||||
offenders = set()
|
||||
seen = set()
|
||||
for mapper in db.Model.registry.mappers:
|
||||
cls = mapper.class_
|
||||
if cls in seen:
|
||||
continue
|
||||
fk_cols = [c for c in rel.remote_side if c.foreign_keys]
|
||||
if not fk_cols or all(c.nullable for c in fk_cols):
|
||||
continue
|
||||
if 'delete' not in rel.cascade:
|
||||
offenders.append(str(rel))
|
||||
assert not offenders, (
|
||||
'NOT NULL children of Application without a delete cascade '
|
||||
f'(purge would IntegrityError): {offenders}')
|
||||
seen.add(cls)
|
||||
for rel in mapper.relationships:
|
||||
if rel.direction.name != 'ONETOMANY' or rel.viewonly:
|
||||
continue
|
||||
fk_cols = [c for c in rel.remote_side if c.foreign_keys]
|
||||
if not fk_cols or all(c.nullable for c in fk_cols):
|
||||
continue
|
||||
if 'delete' not in rel.cascade:
|
||||
offenders.add(f'{cls.__name__}.{rel.key}')
|
||||
unexpected = offenders - DELIBERATELY_UNCASCADED
|
||||
assert not unexpected, (
|
||||
'NOT NULL child relationships without a delete cascade (a parent '
|
||||
f'hard-delete would IntegrityError): {sorted(unexpected)}')
|
||||
# The allowlist may only shrink: an entry that stops matching is stale.
|
||||
stale = DELIBERATELY_UNCASCADED - offenders
|
||||
assert not stale, f'stale DELIBERATELY_UNCASCADED entries: {sorted(stale)}'
|
||||
|
||||
|
||||
def test_deleting_a_user_who_owns_applications_is_refused(app, client, auth_headers, dead_app):
|
||||
"""The User.applications entry in DELIBERATELY_UNCASCADED is only honest
|
||||
while delete_user refuses instead of 500ing — even for a tombstoned app,
|
||||
which still holds the NOT NULL FK."""
|
||||
from werkzeug.security import generate_password_hash
|
||||
from app.models import User
|
||||
|
||||
with app.app_context():
|
||||
owner = User(email='app-owner@test.local', username='app-owner',
|
||||
password_hash=generate_password_hash('x'),
|
||||
role=User.ROLE_DEVELOPER, is_active=True)
|
||||
db.session.add(owner)
|
||||
db.session.commit()
|
||||
Application.query.get(dead_app.id).user_id = owner.id
|
||||
db.session.commit()
|
||||
owner_id = owner.id
|
||||
|
||||
res = client.delete(f'/api/v1/admin/users/{owner_id}', headers=auth_headers)
|
||||
assert res.status_code == 409, res.get_json()
|
||||
assert 'application' in res.get_json()['error']
|
||||
|
||||
|
||||
def test_purge_honours_an_explicit_remove_data_false(app, dead_app, monkeypatch):
|
||||
|
||||
Reference in new issue
Block a user