From 0a9e1326fe20950c533ffa58419aa1e0d2cb87ea Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 16 Apr 2024 10:41:08 +0200 Subject: [PATCH] fix: tighten delete permissions --- ...144144_tighten_delete_permissions.down.sql | 1 + ...15144144_tighten_delete_permissions.up.sql | 228 ++++++++++++++++++ backend/windmill-api/src/folders.rs | 18 +- frontend/src/lib/utils.ts | 2 +- .../(root)/(logged)/folders/+page.svelte | 27 ++- 5 files changed, 262 insertions(+), 14 deletions(-) create mode 100644 backend/migrations/20240415144144_tighten_delete_permissions.down.sql create mode 100644 backend/migrations/20240415144144_tighten_delete_permissions.up.sql diff --git a/backend/migrations/20240415144144_tighten_delete_permissions.down.sql b/backend/migrations/20240415144144_tighten_delete_permissions.down.sql new file mode 100644 index 0000000000..d2f607c5b8 --- /dev/null +++ b/backend/migrations/20240415144144_tighten_delete_permissions.down.sql @@ -0,0 +1 @@ +-- Add down migration script here diff --git a/backend/migrations/20240415144144_tighten_delete_permissions.up.sql b/backend/migrations/20240415144144_tighten_delete_permissions.up.sql new file mode 100644 index 0000000000..4e64395e3a --- /dev/null +++ b/backend/migrations/20240415144144_tighten_delete_permissions.up.sql @@ -0,0 +1,228 @@ + +DO +$do$ + DECLARE + i text; + arr text[] := array['resource', 'script', 'variable', 'schedule', 'flow', 'app', 'raw_app']; + BEGIN + FOREACH i IN ARRAY arr + LOOP + EXECUTE FORMAT( + $$ + DROP POLICY IF EXISTS see_folder_extra_perms_user ON %1$I; + DROP POLICY IF EXISTS see_folder_extra_perms_user_delete ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_user ON %1$I; + DROP POLICY IF EXISTS see_member ON %1$I; + DROP POLICY IF EXISTS see_own ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_user_delete ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_groups ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_groups_delete ON %1$I; + + -- New policies for select, insert, update + DROP POLICY IF EXISTS see_folder_extra_perms_user_select ON %1$I; + DROP POLICY IF EXISTS see_folder_extra_perms_user_insert ON %1$I; + DROP POLICY IF EXISTS see_folder_extra_perms_user_update ON %1$I; + + DROP POLICY IF EXISTS see_own ON %1$I; + DROP POLICY IF EXISTS see_member ON %1$I; + + DROP POLICY IF EXISTS see_extra_perms_user_select ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_user_insert ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_user_update ON %1$I; + + DROP POLICY IF EXISTS see_extra_perms_groups_select ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_groups_insert ON %1$I; + DROP POLICY IF EXISTS see_extra_perms_groups_update ON %1$I; + + + -- Folder permissions split into select, insert, and update + CREATE POLICY see_folder_extra_perms_user_select ON %1$I FOR SELECT TO windmill_user + USING (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_read'), ',')::text[])); + + CREATE POLICY see_folder_extra_perms_user_insert ON %1$I FOR INSERT TO windmill_user + WITH CHECK (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_write'), ',')::text[])); + + CREATE POLICY see_folder_extra_perms_user_update ON %1$I FOR UPDATE TO windmill_user + USING (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_write'), ',')::text[])); + + CREATE POLICY see_folder_extra_perms_user_delete ON %1$I FOR UPDATE TO windmill_user + USING (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_write'), ',')::text[])); + + CREATE POLICY see_own ON %1$I FOR ALL TO windmill_user + USING (SPLIT_PART(%1$I.path, '/', 1) = 'u' AND SPLIT_PART(%1$I.path, '/', 2) = current_setting('session.user')); + + + CREATE POLICY see_member ON %1$I FOR ALL TO windmill_user + USING (SPLIT_PART(%1$I.path, '/', 1) = 'g' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.groups'), ',')::text[])); + + + + CREATE POLICY see_extra_perms_user_select ON %1$I FOR SELECT TO windmill_user + USING (extra_perms ? CONCAT('u/', current_setting('session.user'))); + + CREATE POLICY see_extra_perms_user_insert ON %1$I FOR INSERT TO windmill_user + WITH CHECK ((extra_perms ->> CONCAT('u/', current_setting('session.user')))::boolean); + + CREATE POLICY see_extra_perms_user_update ON %1$I FOR UPDATE TO windmill_user + USING ((extra_perms ->> CONCAT('u/', current_setting('session.user')))::boolean); + + CREATE POLICY see_extra_perms_user_delete ON %1$I FOR DELETE TO windmill_user + USING ((extra_perms ->> CONCAT('u/', current_setting('session.user')))::boolean); + + + + + CREATE POLICY see_extra_perms_groups_select ON %1$I FOR SELECT TO windmill_user + USING (extra_perms ?| regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]); + + CREATE POLICY see_extra_perms_groups_insert ON %1$I FOR INSERT TO windmill_user + WITH CHECK (exists( + SELECT key, value FROM jsonb_each_text(extra_perms) + WHERE SPLIT_PART(key, '/', 1) = 'g' AND key = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) + AND value::boolean)); + + CREATE POLICY see_extra_perms_groups_update ON %1$I FOR UPDATE TO windmill_user + USING (exists( + SELECT key, value FROM jsonb_each_text(extra_perms) + WHERE SPLIT_PART(key, '/', 1) = 'g' AND key = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) + AND value::boolean)); + + CREATE POLICY see_extra_perms_groups_delete ON %1$I FOR DELETE TO windmill_user + USING (exists( + SELECT key, value FROM jsonb_each_text(extra_perms) + WHERE SPLIT_PART(key, '/', 1) = 'g' AND key = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) + AND value::boolean)); + + $$, + i + ); + END LOOP; + END +$do$; + +DROP POLICY IF EXISTS see_extra_perms_user ON folder; +DROP POLICY IF EXISTS see_extra_perms_user_select ON folder; +DROP POLICY IF EXISTS see_extra_perms_user_insert ON folder; +DROP POLICY IF EXISTS see_extra_perms_user_update ON folder; +DROP POLICY IF EXISTS see_extra_perms_user_delete ON folder; + +DROP POLICY IF EXISTS see_extra_perms_groups ON folder; +DROP POLICY IF EXISTS see_extra_perms_groups_select ON folder; +DROP POLICY IF EXISTS see_extra_perms_groups_insert ON folder; +DROP POLICY IF EXISTS see_extra_perms_groups_update ON folder; +DROP POLICY IF EXISTS see_extra_perms_groups_delete ON folder; + + +-- Existing CREATE POLICY statements updated to reflect policy splitting for 'folder' table +CREATE POLICY see_extra_perms_user_select ON folder FOR SELECT TO windmill_user +USING (extra_perms ? CONCAT('u/', current_setting('session.user')) OR CONCAT('u/', current_setting('session.user')) = ANY(owners)); + +CREATE POLICY see_extra_perms_user_insert ON folder FOR INSERT TO windmill_user +WITH CHECK ((CONCAT('u/', current_setting('session.user')) = ANY(owners))); + +CREATE POLICY see_extra_perms_user_update ON folder FOR UPDATE TO windmill_user +USING ((CONCAT('u/', current_setting('session.user')) = ANY(owners))); + +CREATE POLICY see_extra_perms_user_delete ON folder FOR DELETE TO windmill_user +USING ((CONCAT('u/', current_setting('session.user')) = ANY(owners))); + +CREATE POLICY see_extra_perms_groups_select ON folder FOR SELECT TO windmill_user +USING (extra_perms ?| regexp_split_to_array(current_setting('session.pgroups'), ',')::text[] OR EXISTS ( + SELECT o FROM unnest(owners) AS o + WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); + +CREATE POLICY see_extra_perms_groups_insert ON folder FOR INSERT TO windmill_user +WITH CHECK (EXISTS ( + SELECT o FROM unnest(owners) AS o + WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); + + +CREATE POLICY see_extra_perms_groups_update ON folder FOR UPDATE TO windmill_user +USING (EXISTS ( + SELECT o FROM unnest(owners) AS o + WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); + + +CREATE POLICY see_extra_perms_groups_delete ON folder FOR DELETE TO windmill_user +USING (EXISTS ( + SELECT o FROM unnest(owners) AS o + WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); + + +-- DO +-- $do$ +-- DECLARE +-- i text; +-- arr text[] := array['resource', 'script', 'variable', 'schedule', 'flow', 'app', 'raw_app']; +-- BEGIN +-- FOREACH i IN ARRAY arr +-- LOOP +-- EXECUTE FORMAT( +-- $$ + + +-- CREATE POLICY see_folder_extra_perms_user ON %1$I FOR ALL TO windmill_user +-- USING (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_read'), ',')::text[])) +-- WITH CHECK (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_write'), ',')::text[])); + +-- CREATE POLICY see_folder_extra_perms_user_delete ON %1$I AS RESTRICTIVE FOR DELETE TO windmill_user +-- USING (SPLIT_PART(%1$I.path, '/', 1) = 'f' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.folders_write'), ',')::text[])); + +-- CREATE POLICY see_own ON %1$I FOR ALL TO windmill_user +-- USING (SPLIT_PART(%1$I.path, '/', 1) = 'u' AND SPLIT_PART(%1$I.path, '/', 2) = current_setting('session.user')); + +-- CREATE POLICY see_member ON %1$I FOR ALL TO windmill_user +-- USING (SPLIT_PART(%1$I.path, '/', 1) = 'g' AND SPLIT_PART(%1$I.path, '/', 2) = any(regexp_split_to_array(current_setting('session.groups'), ',')::text[])); + + +-- CREATE POLICY see_extra_perms_user ON %1$I FOR ALL TO windmill_user +-- USING (extra_perms ? CONCAT('u/', current_setting('session.user'))) +-- WITH CHECK ((extra_perms ->> CONCAT('u/', current_setting('session.user')))::boolean); + +-- CREATE POLICY see_extra_perms_user_delete ON %1$I FOR DELETE TO windmill_user +-- USING ((extra_perms ->> CONCAT('u/', current_setting('session.user')))::boolean); + +-- CREATE POLICY see_extra_perms_groups ON %1$I FOR ALL TO windmill_user +-- USING (extra_perms ?| regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) +-- WITH CHECK (exists( +-- SELECT key, value FROM jsonb_each_text(extra_perms) +-- WHERE SPLIT_PART(key, '/', 1) = 'g' AND key = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) +-- AND value::boolean)); + +-- CREATE POLICY see_extra_perms_groups_delete ON %1$I FOR DELETE TO windmill_user +-- USING (exists( +-- SELECT key, value FROM jsonb_each_text(extra_perms) +-- WHERE SPLIT_PART(key, '/', 1) = 'g' AND key = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]) +-- AND value::boolean)); +-- $$, +-- i +-- ); +-- END LOOP; +-- END +-- $do$; + + +-- DROP POLICY see_extra_perms_user ON folder; +-- DROP POLICY see_extra_perms_groups ON folder; + +-- CREATE POLICY see_extra_perms_user ON folder FOR ALL to windmill_user +-- USING (extra_perms ? CONCAT('u/', current_setting('session.user')) or (CONCAT('u/', current_setting('session.user')) = ANY(owners))) +-- WITH CHECK ((CONCAT('u/', current_setting('session.user')) = ANY(owners))); + +-- DROP POLICY IF EXISTS see_extra_perms_user_delete ON folder; +-- CREATE POLICY see_extra_perms_user_delete ON folder AS RESTRICTIVE FOR DELETE to windmill_user +-- USING ((CONCAT('u/', current_setting('session.user')) = ANY(owners))); + +-- CREATE POLICY see_extra_perms_groups ON folder FOR ALL to windmill_user +-- USING (extra_perms ?| regexp_split_to_array(current_setting('session.pgroups'), ',')::text[] or (exists( +-- SELECT o FROM unnest(owners) as o +-- WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[])))) +-- WITH CHECK (exists( +-- SELECT o FROM unnest(owners) as o +-- WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); + +-- DROP POLICY IF EXISTS see_extra_perms_groups_delete ON folder; +-- CREATE POLICY see_extra_perms_groups_delete ON folder AS RESTRICTIVE FOR DELETE to windmill_user +-- USING (exists( +-- SELECT o FROM unnest(owners) as o +-- WHERE o = ANY(regexp_split_to_array(current_setting('session.pgroups'), ',')::text[]))); \ No newline at end of file diff --git a/backend/windmill-api/src/folders.rs b/backend/windmill-api/src/folders.rs index 2d589a68a1..9c81031684 100644 --- a/backend/windmill-api/src/folders.rs +++ b/backend/windmill-api/src/folders.rs @@ -477,15 +477,23 @@ async fn delete_folder( let mut tx = user_db.begin(&authed).await?; not_found_if_none(get_folderopt(&mut tx, &w_id, &name).await?, "Folder", &name)?; - require_is_owner(&authed, &name)?; - sqlx::query!( - "DELETE FROM folder WHERE name = $1 AND workspace_id = $2", + let del = sqlx::query_scalar!( + "DELETE FROM folder WHERE name = $1 AND workspace_id = $2 RETURNING 1", name, w_id ) - .execute(&mut *tx) - .await?; + .fetch_optional(&mut *tx) + .await? + .flatten(); + + if del.is_none() { + return Err(windmill_common::error::Error::NotAuthorized(format!( + "Not authorized to delete folder {}", + name + ))); + } + audit_log( &mut *tx, &authed.username, diff --git a/frontend/src/lib/utils.ts b/frontend/src/lib/utils.ts index 77863b7901..ba12e62e6c 100644 --- a/frontend/src/lib/utils.ts +++ b/frontend/src/lib/utils.ts @@ -579,7 +579,7 @@ export function canWrite( if (user.pgroups.findIndex((x) => keys.includes(x) && extra_perms[x]) != -1) { return true } - if (user.folders.findIndex((x) => path.startsWith('f/' + x)) != -1) { + if (user.folders.findIndex((x) => path.startsWith('f/' + x + '/') && user.folders[x]) != -1) { return true } diff --git a/frontend/src/routes/(root)/(logged)/folders/+page.svelte b/frontend/src/routes/(root)/(logged)/folders/+page.svelte index 5f7de86352..b13d294b64 100644 --- a/frontend/src/routes/(root)/(logged)/folders/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/folders/+page.svelte @@ -10,7 +10,7 @@ import { Button, Drawer, DrawerContent, Popup, Skeleton } from '$lib/components/common' import FolderInfo from '$lib/components/FolderInfo.svelte' import FolderUsageInfo from '$lib/components/FolderUsageInfo.svelte' - import { canWrite } from '$lib/utils' + import { sendUserToast } from '$lib/utils' import DataTable from '$lib/components/table/DataTable.svelte' import Cell from '$lib/components/table/Cell.svelte' import { Pen, Trash, Plus } from 'lucide-svelte' @@ -25,7 +25,14 @@ async function loadFolders(): Promise { folders = (await FolderService.listFolders({ workspace: $workspaceStore! })).map((x) => { - return { canWrite: canWrite('f/' + x.name, x.extra_perms ?? {}, $userStore), ...x } + return { + canWrite: + $userStore != undefined && + ($userStore?.is_admin || + $userStore?.is_super_admin || + $userStore?.folders_owners.includes(x.name)), + ...x + } }) } @@ -170,16 +177,20 @@ } }, { - displayName: 'Delete', + displayName: `Delete${canWrite ? '' : ' (require owner permissions)'}`, icon: Trash, type: 'delete', disabled: !canWrite, action: async () => { - await FolderService.deleteFolder({ - workspace: $workspaceStore ?? '', - name - }) - loadFolders() + try { + await FolderService.deleteFolder({ + workspace: $workspaceStore ?? '', + name + }) + } catch (e) { + sendUserToast(e.body, true) + loadFolders() + } } } ]}