From bbb871d3231a9a0627f4d18b2a1c5fe0727b4a9a Mon Sep 17 00:00:00 2001 From: Kuba <78603704+jakub-tldr@users.noreply.github.com> Date: Mon, 11 May 2026 14:58:25 +0200 Subject: [PATCH 1/5] Migrate empty allowed groups from 1.6.x (#2902) --- ....0]_migrate_location_with_empty_allowed_groups.up.sql | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 migrations/20260511085206_[2.0.0]_migrate_location_with_empty_allowed_groups.up.sql diff --git a/migrations/20260511085206_[2.0.0]_migrate_location_with_empty_allowed_groups.up.sql b/migrations/20260511085206_[2.0.0]_migrate_location_with_empty_allowed_groups.up.sql new file mode 100644 index 000000000..761c58737 --- /dev/null +++ b/migrations/20260511085206_[2.0.0]_migrate_location_with_empty_allowed_groups.up.sql @@ -0,0 +1,9 @@ +-- In 1.6.x an empty allowed groups list meant all groups have access to location. +-- Restore that meaning for locations that were migrated without any explicit group assigned. +UPDATE wireguard_network AS location +SET allow_all_groups = true +WHERE NOT EXISTS ( + SELECT 1 + FROM wireguard_network_allowed_group AS allowed_group + WHERE allowed_group.network_id = location.id +); From 3c581630531e32a700e411659a026de480b326d5 Mon Sep 17 00:00:00 2001 From: Kuba <78603704+jakub-tldr@users.noreply.github.com> Date: Tue, 12 May 2026 10:55:37 +0200 Subject: [PATCH 2/5] Prevent deleting groups used in locations (#2908) * allow for empty submitProps * add locations to groups page * show blocked/affected groups modal on deletion * better text * add query, block removing groups which are only allowed group in location * adjust tests * pnpm fix --- crates/defguard_common/src/db/models/group.rs | 20 ++++++ crates/defguard_core/src/handlers/group.rs | 12 ++++ .../api/wireguard_network_allowed_groups.rs | 70 +++++++++++++++++-- web/messages/en/modal.json | 2 + web/src/pages/GroupsPage/GroupsPage.tsx | 4 +- .../components/GroupsTable/GroupsTable.tsx | 33 +++++++-- .../routes/_authorized/_default/groups.tsx | 2 + .../ConfirmActionModal/ConfirmActionModal.tsx | 19 ++--- web/src/shared/validators.ts | 2 +- 9 files changed, 144 insertions(+), 20 deletions(-) diff --git a/crates/defguard_common/src/db/models/group.rs b/crates/defguard_common/src/db/models/group.rs index eb81daad3..b05fe0148 100644 --- a/crates/defguard_common/src/db/models/group.rs +++ b/crates/defguard_common/src/db/models/group.rs @@ -108,6 +108,26 @@ impl Group { .await } + pub async fn locations_with_one_allowed_group<'e, E>(&self, executor: E) -> sqlx::Result> + where + E: PgExecutor<'e>, + { + query_scalar( + "SELECT wn.name FROM wireguard_network wn \ + JOIN wireguard_network_allowed_group target_group \ + ON target_group.network_id = wn.id AND target_group.group_id = $1 \ + JOIN wireguard_network_allowed_group allowed_group \ + ON allowed_group.network_id = wn.id \ + WHERE NOT wn.allow_all_groups \ + GROUP BY wn.id, wn.name \ + HAVING COUNT(allowed_group.group_id) = 1 \ + ORDER BY wn.name", + ) + .bind(self.id) + .fetch_all(executor) + .await + } + pub async fn find_by_permission<'e, E>( executor: E, permission: Permission, diff --git a/crates/defguard_core/src/handlers/group.rs b/crates/defguard_core/src/handlers/group.rs index 09006a46b..fbe40ab42 100644 --- a/crates/defguard_core/src/handlers/group.rs +++ b/crates/defguard_core/src/handlers/group.rs @@ -591,6 +591,18 @@ pub(crate) async fn delete_group( return Ok(ApiResponse::with_status(StatusCode::BAD_REQUEST)); } } + let blocked_locations = group + .locations_with_one_allowed_group(&appstate.pool) + .await?; + if !blocked_locations.is_empty() { + let msg = format!( + "Cannot delete group {} because it is the only allowed group in locations: {}", + group.name, + blocked_locations.join(", ") + ); + error!("{msg}"); + return Ok(ApiResponse::with_status(StatusCode::BAD_REQUEST)); + } group.clone().delete(&appstate.pool).await?; ldap_delete_group(&group.name, &appstate.pool).await; diff --git a/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs b/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs index 85d9400ea..e4b90edef 100644 --- a/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs +++ b/crates/defguard_core/tests/integration/api/wireguard_network_allowed_groups.rs @@ -919,7 +919,7 @@ DNS = 10.0.0.2 } #[sqlx::test] -async fn test_delete_only_allowed_group(_: PgPoolOptions, options: PgConnectOptions) { +async fn test_delete_only_allowed_group_rejected(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; let (client, client_state) = make_test_client(pool).await; @@ -977,14 +977,76 @@ async fn test_delete_only_allowed_group(_: PgPoolOptions, options: PgConnectOpti .delete(format!("/api/v1/group/{allowed_group_id}")) .send() .await; - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); - // network configuration was created only for the admin device + // network configuration remains unchanged let peers = get_location_allowed_peers(&network, &client_state.pool) .await .unwrap(); - assert_eq!(peers.len(), 1); + assert_eq!(peers.len(), 2); assert_eq!(peers[0].pubkey, devices[0].wireguard_pubkey); + assert_eq!(peers[1].pubkey, devices[1].wireguard_pubkey); +} + +#[sqlx::test] +async fn test_delete_allowed_group_when_location_keeps_other_groups( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + + let (client, client_state) = make_test_client(pool).await; + setup_test_users(&client_state.pool).await; + + let mut wg_rx = client_state.wireguard_rx; + + let auth = Auth::new("admin", "pass123"); + let response = &client.post("/api/v1/auth").json(&auth).send().await; + assert_eq!(response.status(), StatusCode::OK); + + let response = client + .post("/api/v1/network") + .json(&json!({ + "name": "network", + "address": "10.1.1.1/24", + "port": 55555, + "endpoint": "192.168.4.14", + "allowed_ips": "10.1.1.0/24", + "dns": "1.1.1.1", + "mtu": 1420, + "fwmark": 0, + "allow_all_groups": false, + "allowed_groups": ["allowed group", "not allowed group"], + "keepalive_interval": 25, + "peer_disconnect_threshold": 300, + "acl_enabled": false, + "acl_default_allow": false, + "location_mfa_mode": "disabled", + "service_location_mode": "disabled" + })) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let network: WireguardNetwork = response.json().await; + let event = wg_rx.try_recv().unwrap(); + assert_matches!(event, GatewayEvent::NetworkCreated(..)); + + let allowed_group_id = Group::find_by_name(&client_state.pool, "allowed group") + .await + .unwrap() + .unwrap() + .id; + let response = client + .delete(format!("/api/v1/group/{allowed_group_id}")) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + let allowed_groups = network + .fetch_allowed_groups(&client_state.pool) + .await + .unwrap(); + assert_eq!(allowed_groups, vec!["not allowed group"]); } #[sqlx::test] diff --git a/web/messages/en/modal.json b/web/messages/en/modal.json index 9f6f6a674..03db4c73f 100644 --- a/web/messages/en/modal.json +++ b/web/messages/en/modal.json @@ -32,6 +32,8 @@ "modal_delete_edge_error": "Failed to delete edge", "modal_delete_group_title": "Delete group", "modal_delete_group_body": "Are you sure you want to delete group {name}? This action cannot be undone.", + "modal_delete_group_affected_locations_body": "Group {name} is used in location. Deleting it will also revoke location access for users in this group.", + "modal_delete_group_blocked_body": "Group {name} is the only allowed group in location. Deleting it would leave those locations with an empty Allowed groups list. Update locations manually before deleting this group.", "modal_delete_group_success": "Group deleted", "modal_delete_group_error": "Failed to delete group", "modal_delete_user_device_title": "Delete device", diff --git a/web/src/pages/GroupsPage/GroupsPage.tsx b/web/src/pages/GroupsPage/GroupsPage.tsx index 0eeb4dc73..fc63ac999 100644 --- a/web/src/pages/GroupsPage/GroupsPage.tsx +++ b/web/src/pages/GroupsPage/GroupsPage.tsx @@ -6,6 +6,7 @@ import { ThemeSpacing } from '../../shared/defguard-ui/types'; import { TablePageLayout } from '../../shared/layout/TablePageLayout/TablePageLayout'; import { getGroupsInfoQueryOptions, + getLocationsQueryOptions, getUsersOverviewQueryOptions, } from '../../shared/query'; import { GroupsTable } from './components/GroupsTable/GroupsTable'; @@ -13,13 +14,14 @@ import { CEGroupModal } from './modals/CEGroupModal/CEGroupModal'; export const GroupsPage = () => { const { data: groups } = useSuspenseQuery(getGroupsInfoQueryOptions); + const { data: locations } = useSuspenseQuery(getLocationsQueryOptions); const { data: users } = useSuspenseQuery(getUsersOverviewQueryOptions); return ( <> - + diff --git a/web/src/pages/GroupsPage/components/GroupsTable/GroupsTable.tsx b/web/src/pages/GroupsPage/components/GroupsTable/GroupsTable.tsx index 3fd2acb44..69c250917 100644 --- a/web/src/pages/GroupsPage/components/GroupsTable/GroupsTable.tsx +++ b/web/src/pages/GroupsPage/components/GroupsTable/GroupsTable.tsx @@ -7,7 +7,7 @@ import { import { useMemo, useState } from 'react'; import { m } from '../../../../paraglide/messages'; import api from '../../../../shared/api/api'; -import type { GroupInfo, User } from '../../../../shared/api/types'; +import type { GroupInfo, NetworkLocation, User } from '../../../../shared/api/types'; import { Badge } from '../../../../shared/defguard-ui/components/Badge/Badge'; import { Button } from '../../../../shared/defguard-ui/components/Button/Button'; import type { MenuItemProps } from '../../../../shared/defguard-ui/components/Menu/types'; @@ -23,6 +23,7 @@ import { ModalName } from '../../../../shared/hooks/modalControls/modalTypes'; type Props = { groups: GroupInfo[]; + locations: NetworkLocation[]; users: User[]; }; @@ -30,7 +31,7 @@ type RowData = GroupInfo; const columnHelper = createColumnHelper(); -export const GroupsTable = ({ groups, users }: Props) => { +export const GroupsTable = ({ groups, locations, users }: Props) => { const [search, setSearch] = useState(''); const reservedNames = useMemo(() => groups.map((g) => g.name), [groups]); @@ -117,11 +118,33 @@ export const GroupsTable = ({ groups, users }: Props) => { icon: 'delete', variant: 'danger', onClick: () => { + const affectedLocations = locations.filter( + (location) => + !location.allow_all_groups && + location.allowed_groups.includes(rowData.name), + ); + const blockedLocations = affectedLocations.filter( + (location) => location.allowed_groups.length === 1, + ); + if (blockedLocations.length > 0) { + openModal(ModalName.ConfirmAction, { + title: m.modal_delete_group_title(), + contentMd: m.modal_delete_group_blocked_body({ name: rowData.name }), + actionPromise: () => api.group.deleteGroup(rowData.id), + cancelProps: { text: m.controls_close() }, + }); + return; + } openModal(ModalName.ConfirmAction, { title: m.modal_delete_group_title(), - contentMd: m.modal_delete_group_body({ name: rowData.name }), + contentMd: + affectedLocations.length > 0 + ? m.modal_delete_group_affected_locations_body({ + name: rowData.name, + }) + : m.modal_delete_group_body({ name: rowData.name }), actionPromise: () => api.group.deleteGroup(rowData.id), - invalidateKeys: [['group'], ['group-info']], + invalidateKeys: [['group'], ['group-info'], ['network']], submitProps: { text: m.controls_delete(), variant: 'critical' }, onSuccess: () => Snackbar.default(m.modal_delete_group_success()), onError: () => Snackbar.error(m.modal_delete_group_error()), @@ -133,7 +156,7 @@ export const GroupsTable = ({ groups, users }: Props) => { }, }), ], - [reservedNames, users], + [locations, reservedNames, users], ); const table = useReactTable({ diff --git a/web/src/routes/_authorized/_default/groups.tsx b/web/src/routes/_authorized/_default/groups.tsx index 25c163fca..258d66d89 100644 --- a/web/src/routes/_authorized/_default/groups.tsx +++ b/web/src/routes/_authorized/_default/groups.tsx @@ -2,6 +2,7 @@ import { createFileRoute } from '@tanstack/react-router'; import { GroupsPage } from '../../../pages/GroupsPage/GroupsPage'; import { getGroupsInfoQueryOptions, + getLocationsQueryOptions, getUsersOverviewQueryOptions, } from '../../../shared/query'; @@ -10,6 +11,7 @@ export const Route = createFileRoute('/_authorized/_default/groups')({ loader: ({ context }) => { return Promise.all([ context.queryClient.fetchQuery(getGroupsInfoQueryOptions), + context.queryClient.fetchQuery(getLocationsQueryOptions), context.queryClient.fetchQuery(getUsersOverviewQueryOptions), ]); }, diff --git a/web/src/shared/components/modals/ConfirmActionModal/ConfirmActionModal.tsx b/web/src/shared/components/modals/ConfirmActionModal/ConfirmActionModal.tsx index eb3aaeaf7..491247e05 100644 --- a/web/src/shared/components/modals/ConfirmActionModal/ConfirmActionModal.tsx +++ b/web/src/shared/components/modals/ConfirmActionModal/ConfirmActionModal.tsx @@ -85,15 +85,16 @@ const ModalContent = ({ data }: { data: ModalData }) => { closeModal(modalNameValue); }} /> -