Conversation
There was a problem hiding this comment.
Pull request overview
Updates ACL Alias/Destination UX and backend endpoints to prevent deletion when the resource is referenced by ACL rules, matching the intended server behavior (issue #2022).
Changes:
- Add
parent_idto alias/destination API types and update apply-destinations client API to use/acl/destination/apply. - Introduce UI modals for “deletion blocked” (showing referencing rules) and delete confirmation; gate tables on rules availability.
- Refactor/extend backend ACL handlers (kind-specific apply/delete for aliases vs destinations) and reorganize ACL integration tests into modules.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/shared/api/types.ts | Adds parent_id to AclAlias/AclDestination and updates request types to omit it. |
| web/src/shared/api/api.ts | Fixes destinations “apply” endpoint and payload (/acl/destination/apply, { destinations }). |
| web/src/pages/DestinationsPage/tabs/DestinationPendingTab/DestinationPendingTab.tsx | Delays rendering pending destinations table until rules are loaded; passes disableBlockedModal. |
| web/src/pages/DestinationsPage/tabs/DestinationDeployedTab/DestinationDeployedTab.tsx | Delays rendering deployed destinations table until rules are loaded. |
| web/src/pages/DestinationsPage/components/DestinationsTable.tsx | Adds blocked/delete modals and computes “Used in rules”; blocks delete when rules not ready. |
| web/src/pages/AliasesPage/tabs/AliasesPendingTab.tsx | Delays rendering pending aliases table until rules are loaded; passes disableBlockedModal. |
| web/src/pages/AliasesPage/tabs/AliasesDeployedTab.tsx | Delays rendering deployed aliases table until rules are loaded. |
| web/src/pages/AliasesPage/AliasTable.tsx | Adds blocked/delete modals; updates “Used in rules” computation using parent_id; blocks delete when rules not ready. |
| web/src/pages/Acl/components/DeletionBlockedModal/style.scss | Styles for the new “deletion blocked” modal content. |
| web/src/pages/Acl/components/DeletionBlockedModal/DeletionBlockedModal.tsx | New modal that lists rules preventing deletion. |
| web/src/pages/Acl/components/DeleteConfirmModal/DeleteConfirmModal.tsx | New reusable delete confirmation modal. |
| web/pnpm-lock.yaml | Lockfile normalization/update (removes libc fields for several packages). |
| flake.lock | Updates pinned Nix inputs (nixpkgs, rust-overlay). |
| crates/defguard_core/tests/integration/api/acl/rules.rs | Moves shared helpers out; keeps rule-focused tests and updates destination update test data. |
| crates/defguard_core/tests/integration/api/acl/mod.rs | New shared test module with common helpers and submodules for aliases/destinations/rules. |
| crates/defguard_core/tests/integration/api/acl/destinations.rs | New destinations integration test module (CRUD, apply, delete blocking, validation). |
| crates/defguard_core/tests/integration/api/acl/aliases.rs | New aliases integration test module (CRUD, apply, delete blocking, validation). |
| crates/defguard_core/src/openapi.rs | Updates OpenAPI paths to new handler locations and adds destination apply endpoint. |
| crates/defguard_core/src/lib.rs | Wires /api/v1/acl/destination/apply route and updates handler imports. |
| crates/defguard_core/src/handlers/mod.rs | Adds API responses for new destination-specific ACL errors. |
| crates/defguard_core/src/enterprise/handlers/acl/destination.rs | Adds destination apply endpoint and kind-specific destination delete; exports destination API structs. |
| crates/defguard_core/src/enterprise/handlers/acl/alias.rs | Adds alias validation; adds alias apply endpoint; kind-specific alias delete. |
| crates/defguard_core/src/enterprise/handlers/acl.rs | Makes destination module public and removes alias-apply handler (moved into alias module). |
| crates/defguard_core/src/enterprise/db/models/acl.rs | Introduces destination-specific ACL errors; adds delete_by_kind and apply_by_kind; adjusts apply error mapping by kind. |
Files not reviewed (1)
- web/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <DeleteConfirmModal | ||
| isOpen={deleteModal !== null} | ||
| title={deleteModal?.title ?? ''} | ||
| description={deleteModal?.description ?? ''} | ||
| onConfirm={() => { | ||
| if (!deleteModal) return; | ||
| deleteAlias(deleteModal.aliasId); | ||
| setDeleteModal(null); | ||
| }} | ||
| onClose={() => setDeleteModal(null)} |
There was a problem hiding this comment.
The confirm dialog triggers deleteAlias(...) and immediately closes without wiring mutation pending state into DeleteConfirmModal. This allows repeated confirmations (multiple requests) and hides failures. Consider using isPending from the mutation and only closing after success (or handling errors).
web/src/pages/Acl/components/DeletionBlockedModal/DeletionBlockedModal.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/DestinationsPage/components/DestinationsTable.tsx
Outdated
Show resolved
Hide resolved
| if (row.rules.length > 0) { | ||
| if (disableBlockedModal) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
disableBlockedModal makes the delete action a silent no-op when row.rules.length > 0 (returns without any UI feedback). If deletion is intentionally blocked in this context, consider disabling/hiding the delete menu item or still showing the blocked modal so the user understands why deletion can't proceed.
| <DeleteConfirmModal | ||
| isOpen={deleteModal !== null} | ||
| title={deleteModal?.title ?? ''} | ||
| description={deleteModal?.description ?? ''} | ||
| onConfirm={() => { | ||
| if (!deleteModal) return; | ||
| deleteDestination(deleteModal.destinationId); | ||
| setDeleteModal(null); | ||
| }} |
There was a problem hiding this comment.
The confirm dialog calls deleteDestination(...) and immediately closes without passing mutation pending state into DeleteConfirmModal. This can allow duplicate submissions and makes it hard to surface errors. Consider using isPending from the mutation and only closing after success (or handling errors).
| if (row.rules.length > 0) { | ||
| if (disableBlockedModal) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
disableBlockedModal causes the delete handler to return when row.rules.length > 0, resulting in a no-op click (no modal, no feedback). If the intent is to prevent deletion here, consider disabling/hiding the delete action (or still showing the blocked modal) so users understand why nothing happens.
web/src/pages/Acl/components/DeleteConfirmModal/DeleteConfirmModal.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Acl/components/DeletionBlockedModal/DeletionBlockedModal.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Acl/components/DeletionBlockedModal/DeletionBlockedModal.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Acl/components/DeleteConfirmModal/DeleteConfirmModal.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/DestinationsPage/tabs/DestinationPendingTab/DestinationPendingTab.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/DestinationsPage/tabs/DestinationDeployedTab/DestinationDeployedTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- web/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -54,15 +54,15 @@ | |||
| "@biomejs/biome": "2.4.5", | |||
| "@inlang/paraglide-js": "2.13.1", | |||
There was a problem hiding this comment.
@inlang/paraglide-js is now ^2.13.2 in dependencies, but it is still pinned to 2.13.1 in devDependencies, while pnpm-lock.yaml no longer contains 2.13.1. This mismatch can break installs (e.g. pnpm install --frozen-lockfile). Align the devDependency version with the dependency (or remove one of the duplicate entries) and regenerate the lockfile accordingly.
| "@inlang/paraglide-js": "2.13.1", | |
| "@inlang/paraglide-js": "^2.13.2", |
| pub(crate) async fn apply_by_kind( | ||
| aliases: &[Id], | ||
| kind: AliasKind, | ||
| appstate: &AppState, | ||
| ) -> Result<(), AclError> { | ||
| debug!( | ||
| "Applying {} ACL aliases of kind {kind:?}: {aliases:?}", | ||
| aliases.len(), | ||
| ); |
There was a problem hiding this comment.
In apply_by_kind, the parameter and log message still use the name aliases, but the function is also used for destinations. Renaming the parameter (and related log strings) to something neutral like ids/items would make call sites (e.g. destinations) clearer and avoid confusion.
This updated the UI to reflect the current behavior - if an alias/destination is used by some rules, we don't allow users to delete them. Instead they are shown a modal listing all the rules that use this alias/destination.
It also fixes some API and UI inconsistencies:
Test suite is also extended to cover the destinations API & verify alias/destination validation.
Closes #2022