fix(notifications): enforce can_delete and recipient access on DELETE

[ferdinand-van-butzelaar]

Closes #675.

The two missing checks

Both already exist in the codebase; the delete endpoint just never called them.

1. can_delete was not enforced server-side

NotificationsManager.delete_notification went straight to self._repo.remove(...). Reproduced against a session with a can_delete=False row present:

### development ###
before: can_delete=False
delete_notification returned: True
after: row DELETED

### this branch ###
before: can_delete=False
delete_notification raised: NotificationNotDeletableError: Notification … is marked as non-deletable
after: row still present

The four flows that rely on the flag — approval requests (workflow_executor.py:631), access grants (access_grants_manager.py:765), contract deploys (data_contracts_manager.py:5264), role access requests (user_routes.py:449) — were protected by the frontend alone.

2. No recipient access check

The endpoint was guarded only by PermissionChecker('notifications', FeatureAccessLevel.ADMIN). That permission says the caller may use the notifications feature, not that they may act on someone else's notification. can_user_access_notification() was already called by mark-as-read (notifications_routes.py:153) and the workflow approval handler (workflows_routes.py:1224) — the strictly more destructive operation had the weaker check.

The open question in the issue

Worth deciding deliberately whether "admins may delete anything" should extend to can_delete=False rows, since those exist specifically to force a response. The GET override implies yes; the flag's purpose implies no.

This PR lets admins override, and makes it explicit. Reasoning:

• get_notifications already sets can_delete = True on everything an admin can see, and the issue confirms that override is intentional. If DELETE refused unconditionally, an admin's UI would render a delete button that always 403s — the API would report one thing and enforce another.
• There is no other cleanup path. A notification orphaned by a workflow that died mid-flight has can_delete=False forever and no way to respond to it.
• The override is narrow and observable: it requires the Admin app role, is logged at INFO, and is recorded in the audit entry as admin_override.

Non-admins get the strict behaviour, and is_admin defaults to False so any caller that forgets to pass it fails safe. If you'd rather have no override at all, it's a two-line change and I'm happy to make it.

Changes

• delete_notification(db, notification_id, *, is_admin=False) loads the row first, raises NotificationNotFoundError when absent and the new NotificationNotDeletableError when can_delete is False and the caller is not an admin. A typed exception rather than a False return so the route can answer 403 and not 404 — the previous if not deleted: 404 could not tell the two apart.
• The route calls can_user_access_notification() before deleting, mirroring mark-as-read, and returns 403 on failure.
• New NotificationsManager.is_app_admin() plus a _role_maps() helper. The role-lookup-and-detect-admin block was duplicated verbatim in get_notifications and can_user_access_notification; both now share one implementation, so what the API reports as can_delete and what it enforces cannot drift apart. Net −40 lines there.

Backend only. The frontend already respects the flag it is served, so no UI change is needed.

Tests

18 new cases in src/backend/src/tests/unit/test_notification_delete_authz.py, against a real session rather than a mocked repository:

• can_delete enforcement, the admin override, and the default-to-non-admin contract
• is_app_admin — true for the Admin role's groups, false for a normal user, false with no user, false when the Admin role has no assigned groups, and false when the role lookup raises (fails closed rather than promoting on error)
• recipient access — own / other user's / role-scoped / broadcast
• route status mapping — 403 for a non-recipient, 403 for non-deletable, 404 for missing, 204 on success, and that delete_notification is not called at all on the two 403 paths

New suite	18 passed
Full backend suite (backend/src/tests/unit + backend/tests)	1503 passed, 1 skipped

The delete cases in the quarantined test_notifications_manager.py are updated to the new contract. That module stays quarantined — it assigns to manager._repo.get.return_value where _repo is the real repository singleton, so every case in it fails at setup, independently of this change.

Unrelated, spotted in passing

NotificationsManager.update_notification is defined twice (lines 416 and 470); the second shadows the first, and ruff flags it F811. Same failure mode as the tool-module duplicates in #677. Not touched here — happy to file it separately.

[CLAassistant]

All committers have signed the CLA.

[mvkonchits-db]

Verified every claim against development source rather than taking the description at face value. Clean, correctly-scoped authorization fix that closes the can_delete gap flagged as a follow-up in #674. Approving.

Diagnosis confirmed:

• delete_notification went straight to _repo.remove with no can_delete or recipient check (development L283–290).
• The DELETE route was guarded only by PermissionChecker('notifications', ADMIN) — a per-feature permission ("may use notifications"), not "may act on someone else's notification".
• mark-as-read already calls can_user_access_notification (routes L153); the strictly more destructive delete had the weaker check. This makes them consistent.
• NotificationDb.can_delete (column) and API Notification.can_delete (model, default True) both exist.

Fix verified:

• Typed exceptions (NotificationNotFoundError → 404, NotificationNotDeletableError → 403) let the route return correct status; the old if not deleted: 404 couldn't distinguish "missing" from "refused".
• Fails closed: is_admin defaults to False, and _role_maps() swallows lookup errors into empty maps so is_app_admin returns False on a role-service failure rather than promoting.
• The extracted is_app_admin is behaviorally identical to the old inline block in get_notifications (role_by_name.get('Admin') + assigned_groups + user_info.groups membership). Since get_notifications uses the same notion to report can_delete=True, what the API reports and what DELETE enforces can no longer drift apart — that's a real correctness argument, not just DRY tidiness.

One deliberate security decision worth a conscious maintainer sign-off: this lets admins override can_delete=False (the "you must respond" notifications). I agree with the reasoning — get_notifications already reports can_delete=True to admins, so refusing at DELETE would make the API contradict itself, and there's otherwise no cleanup path for a notification orphaned by a dead workflow. The override is narrow (Admin app-role only), logged at INFO, and audit-recorded as admin_override, and the author offered a two-line no-override alternative. Flagging so it's an intentional call, not a side effect.

Minor / non-blocking:

• Extra DB round-trip (route does get_notification_by_id + access check, then delete_notification re-gets). Negligible and mirrors mark-as-read.
• The update_notification F811 duplicate (L365 & L419) is real — correctly left out of scope and offered as a separate filing.

One note for whoever merges: CI hasn't run (fork PR from a first-time contributor needs workflow approval). My review is a static verification against source plus a read of the 18-case test suite; worth letting them actually execute before merge.