From 4c33daaba09474ea6485e6bf384945966bb554ba Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 15 Apr 2023 23:03:16 +0200 Subject: [PATCH] rework ownership permissions --- backend/windmill-api/openapi.yaml | 15 ++ backend/windmill-api/src/apps.rs | 6 +- backend/windmill-api/src/flows.rs | 20 ++- backend/windmill-api/src/folders.rs | 74 +++----- backend/windmill-api/src/granular_acls.rs | 43 +++-- backend/windmill-api/src/jobs.rs | 17 +- backend/windmill-api/src/resources.rs | 5 +- backend/windmill-api/src/scripts.rs | 7 +- backend/windmill-api/src/variables.rs | 5 +- frontend/src/lib/components/AppConnect.svelte | 2 +- frontend/src/lib/components/ArgInput.svelte | 32 ++-- frontend/src/lib/components/Dropdown.svelte | 16 +- .../lib/components/FlowStatusViewer.svelte | 4 +- .../lib/components/LightweightArgInput.svelte | 32 ++-- frontend/src/lib/components/MoveDrawer.svelte | 6 +- frontend/src/lib/components/Path.svelte | 1 + .../src/lib/components/ResourceEditor.svelte | 4 +- frontend/src/lib/components/RunForm.svelte | 138 +++++++------- frontend/src/lib/components/ShareModal.svelte | 5 +- .../src/lib/components/SharedBadge.svelte | 62 ++++--- .../src/lib/components/VariableEditor.svelte | 4 +- .../common/drawer/DrawerContent.svelte | 2 +- .../components/common/table/FlowRow.svelte | 143 ++++++++------- .../components/common/table/ScriptRow.svelte | 169 ++++++++++-------- .../src/lib/components/home/ItemsList.svelte | 5 +- frontend/src/lib/stores.ts | 1 + frontend/src/lib/utils.ts | 40 +++-- .../(logged)/flows/get/[...path]/+page.svelte | 22 +-- .../(root)/(logged)/folders/+page.svelte | 2 +- .../scripts/get/[...hash]/+page.svelte | 77 +++++--- .../scripts/run/[...hash]/+page.svelte | 19 +- .../(root)/(logged)/variables/+page.svelte | 97 +++++----- frontend/tsconfig.json | 2 +- 33 files changed, 593 insertions(+), 484 deletions(-) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index d02afba444..6cf194ac1f 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -2757,6 +2757,16 @@ paths: parameters: - $ref: "#/components/parameters/WorkspaceId" - $ref: "#/components/parameters/ScriptPath" + requestBody: + description: archiveFlow + required: true + content: + application/json: + schema: + type: object + properties: + archived: + type: boolean responses: "200": description: flow archived @@ -5280,6 +5290,10 @@ components: type: array items: type: string + folders_owners: + type: array + items: + type: string usage: $ref: "#/components/schemas/Usage" required: @@ -5291,6 +5305,7 @@ components: - operator - disabled - folders + - folders_owners Usage: type: object diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 3a8f590e13..e59d1fde09 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -442,7 +442,6 @@ async fn update_app( authed: Authed, Extension(user_db): Extension, Extension(webhook): Extension, - Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Json(ns): Json, ) -> Result { @@ -459,10 +458,7 @@ async fn update_app( if let Some(npath) = &ns.path { if npath != path { - if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &path, &db) - .await?; - } + require_owner_of_path(&authed, path)?; } sqlb.set_str("path", npath); } diff --git a/backend/windmill-api/src/flows.rs b/backend/windmill-api/src/flows.rs index 65bbd4441b..795909419d 100644 --- a/backend/windmill-api/src/flows.rs +++ b/backend/windmill-api/src/flows.rs @@ -19,6 +19,7 @@ use axum::{ Json, Router, }; use hyper::StatusCode; +use serde::Deserialize; use sql_builder::prelude::*; use sql_builder::SqlBuilder; use sqlx::{Postgres, Transaction}; @@ -26,10 +27,11 @@ use windmill_audit::{audit_log, ActionKind}; use windmill_common::{ error::{self, to_anyhow, Error, JsonResult, Result}, flows::{Flow, ListFlowQuery, ListableFlow, NewFlow}, + jobs::JobPayload, schedule::Schedule, utils::{ http_get_from_hub, list_elems_from_hub, not_found_if_none, paginate, Pagination, StripPath, - }, jobs::JobPayload, + }, }; use windmill_queue::{push, schedule::push_scheduled_job, QueueTransaction}; @@ -85,9 +87,8 @@ async fn list_flows( .limit(per_page) .clone(); - if !lq.show_archived.unwrap_or(false) { - sqlb.and_where_eq("archived", false); - } + sqlb.and_where_eq("archived", lq.show_archived.unwrap_or(false)); + if let Some(ps) = &lq.path_start { sqlb.and_where_like_left("path", "?".bind(ps)); } @@ -325,7 +326,7 @@ async fn update_flow( check_schedule_conflict(tx.transaction_mut(), &w_id, &nf.path).await?; if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &flow_path, &db).await?; + require_owner_of_path(&authed, flow_path)?; } let mut schedulables: Vec = sqlx::query_as!( @@ -462,17 +463,24 @@ async fn exists_flow_by_path( Ok(Json(exists)) } +#[derive(Deserialize)] +struct Archived { + archived: Option, +} + async fn archive_flow_by_path( authed: Authed, Extension(user_db): Extension, Extension(webhook): Extension, Path((w_id, path)): Path<(String, StripPath)>, + Json(archived): Json, ) -> Result { let path = path.to_path(); let mut tx = user_db.begin(&authed).await?; sqlx::query!( - "UPDATE flow SET archived = true WHERE path = $1 AND workspace_id = $2", + "UPDATE flow SET archived = $1 WHERE path = $2 AND workspace_id = $3", + archived.archived.unwrap_or(true), path, &w_id ) diff --git a/backend/windmill-api/src/folders.rs b/backend/windmill-api/src/folders.rs index af3066ffda..bb6fcf86b8 100644 --- a/backend/windmill-api/src/folders.rs +++ b/backend/windmill-api/src/folders.rs @@ -22,13 +22,13 @@ use lazy_static::lazy_static; use regex::Regex; use windmill_audit::{audit_log, ActionKind}; use windmill_common::{ - error::{self, to_anyhow, Error, JsonResult, Result}, + error::{self, to_anyhow, JsonResult, Result}, users::username_to_permissioned_as, utils::{not_found_if_none, paginate, Pagination}, }; use serde::{Deserialize, Serialize}; -use sqlx::{query_scalar, FromRow, Postgres, Transaction}; +use sqlx::{FromRow, Postgres, Transaction}; pub fn workspaced_service() -> Router { Router::new() @@ -41,7 +41,7 @@ pub fn workspaced_service() -> Router { .route("/delete/:name", delete(delete_folder)) .route("/addowner/:name", post(add_owner)) .route("/removeowner/:name", post(remove_owner)) - .route("/is_owner/*path", get(is_owner)) + .route("/is_owner/*path", get(is_owner_api)) } #[derive(FromRow, Serialize, Deserialize, Clone)] @@ -219,47 +219,29 @@ async fn create_folder( Ok(format!("Created folder {}", ng.name)) } -pub async fn is_owner( - Authed { username, is_admin, groups, .. }: Authed, - Extension(db): Extension, - Path((w_id, name)): Path<(String, String)>, +pub async fn is_owner_api( + authed: Authed, + Path((_w_id, name)): Path<(String, String)>, ) -> JsonResult { - if is_admin { - Ok(Json(true)) + Ok(Json(is_owner(&authed, &name))) +} + +pub fn is_owner(Authed { is_admin, folders, .. }: &Authed, name: &str) -> bool { + if *is_admin { + true } else { - Ok(Json( - require_is_owner(&name, &username, &groups, &w_id, &db) - .await - .is_ok(), - )) + folders.into_iter().any(|x| x.0 == name && x.2) } } -pub async fn require_is_owner( - folder_name: &str, - username: &str, - groups: &Vec, - w_id: &str, - db: &DB, -) -> Result<()> { - let is_owner = query_scalar!( - "SELECT EXISTS(SELECT 1 FROM folder WHERE CONCAT('u/', $1::text) = ANY(owners) AND name = $2 AND workspace_id = $4) OR exists( - SELECT 1 FROM folder, unnest(folder.owners) as o - WHERE o = ANY($3::text[]) AND folder.name = $2 AND folder.workspace_id = $4)", - username, - folder_name, - groups, - w_id, - ).fetch_one(db) - .await? - .unwrap_or(false); - if !is_owner { - Err(Error::BadRequest(format!( - "{} is not an owner of {} and hence is not authorized to perform this operation", - username, folder_name - ))) - } else { +pub fn require_is_owner(authed: &Authed, name: &str) -> Result<()> { + if is_owner(authed, name) { Ok(()) + } else { + Err(windmill_common::error::Error::NotAuthorized(format!( + "You are not owner of the folder {}", + name + ))) } } @@ -491,7 +473,6 @@ async fn delete_folder( async fn add_owner( authed: Authed, - Extension(db): Extension, Extension(user_db): Extension, Extension(webhook): Extension, Path((w_id, name)): Path<(String, String)>, @@ -500,9 +481,7 @@ async fn add_owner( let mut tx = user_db.begin(&authed).await?; not_found_if_none(get_folderopt(&mut tx, &w_id, &name).await?, "Folder", &name)?; - if !authed.is_admin { - require_is_owner(&name, &authed.username, &authed.groups, &w_id, &db).await?; - } + require_is_owner(&authed, &name)?; sqlx::query!( "UPDATE folder SET owners = array_append(owners, $1) WHERE name = $2 AND workspace_id = $3 AND NOT $1 = ANY(owners) RETURNING name", @@ -538,14 +517,14 @@ pub async fn get_folders_for_user( username: &str, groups: &[String], db: &DB, -) -> Result> { +) -> Result> { let mut perms = groups .into_iter() .map(|x| format!("g/{}", x)) .collect::>(); perms.insert(0, format!("u/{}", username)); let folders = sqlx::query!( - "SELECT name, (EXISTS (SELECT 1 FROM (SELECT key, value FROM jsonb_each_text(extra_perms) WHERE key = ANY($1)) t WHERE value::boolean IS true)) as write FROM folder + "SELECT name, (EXISTS (SELECT 1 FROM (SELECT key, value FROM jsonb_each_text(extra_perms) WHERE key = ANY($1)) t WHERE value::boolean IS true)) as write, $1 && owners::text[] as owner FROM folder WHERE extra_perms ?| $1 AND workspace_id = $2", &perms[..], w_id, @@ -553,7 +532,7 @@ pub async fn get_folders_for_user( .fetch_all(db) .await? .into_iter() - .map(|x| (x.name, x.write.unwrap_or(false))) + .map(|x| (x.name, x.write.unwrap_or(false), x.owner.unwrap_or(false))) .collect(); Ok(folders) @@ -561,7 +540,6 @@ pub async fn get_folders_for_user( async fn remove_owner( authed: Authed, - Extension(db): Extension, Extension(user_db): Extension, Extension(webhook): Extension, Path((w_id, name)): Path<(String, String)>, @@ -570,9 +548,7 @@ async fn remove_owner( let mut tx = user_db.begin(&authed).await?; not_found_if_none(get_folderopt(&mut tx, &w_id, &name).await?, "Folder", &name)?; - if !authed.is_admin { - require_is_owner(&name, &authed.username, &authed.groups, &w_id, &db).await?; - } + require_is_owner(&authed, &name)?; sqlx::query!( "UPDATE folder SET owners = array_remove(owners, $1) WHERE name = $2 AND workspace_id = $3 RETURNING name", diff --git a/backend/windmill-api/src/granular_acls.rs b/backend/windmill-api/src/granular_acls.rs index e5da328b86..82e38fbd9f 100644 --- a/backend/windmill-api/src/granular_acls.rs +++ b/backend/windmill-api/src/granular_acls.rs @@ -43,25 +43,29 @@ async fn add_granular_acl( Json(GranularAcl { owner, write }): Json, ) -> Result { let path = path.to_path(); + let (kind, path) = path .split_once('/') .ok_or_else(|| Error::BadRequest("Invalid path or kind".to_string()))?; let mut tx = user_db.begin(&authed).await?; - if !authed.is_admin { - if kind == "folder" { - crate::folders::require_is_owner(&path, &authed.username, &authed.groups, &w_id, &db) - .await?; - } else if kind == "group_" { - } else { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &path, &db).await?; - } - } let identifier = if kind == "group_" || kind == "folder" { "name" } else { "path" }; + + if !authed.is_admin { + if kind == "folder" { + crate::folders::require_is_owner(&authed, path)?; + } else if kind == "group_" { + crate::groups::require_is_owner(path, &authed.username, &authed.groups, &w_id, &db) + .await?; + } else { + require_owner_of_path(&authed, path)?; + } + } + let obj_o = sqlx::query_scalar::<_, serde_json::Value>(&format!( "UPDATE {kind} SET extra_perms = jsonb_set(extra_perms, '{{\"{owner}\"}}', to_jsonb($1), \ true) WHERE {identifier} = $2 AND workspace_id = $3 RETURNING extra_perms" @@ -86,12 +90,22 @@ async fn remove_granular_acl( Json(GranularAcl { owner, write: _ }): Json, ) -> Result { let path = path.to_path(); - if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &path, &db).await?; - } + let (kind, path) = path .split_once('/') .ok_or_else(|| Error::BadRequest("Invalid path or kind".to_string()))?; + + if !authed.is_admin { + if kind == "folder" { + crate::folders::require_is_owner(&authed, path)?; + } else if kind == "group_" { + crate::groups::require_is_owner(path, &authed.username, &authed.groups, &w_id, &db) + .await?; + } else { + require_owner_of_path(&authed, path)?; + } + } + let mut tx = user_db.begin(&authed).await?; let identifier = if kind == "group_" || kind == "folder" { @@ -99,6 +113,11 @@ async fn remove_granular_acl( } else { "path" }; + + if identifier == "path" { + require_owner_of_path(&authed, path)?; + } + let obj_o = sqlx::query_scalar::<_, serde_json::Value>(&format!( "UPDATE {kind} SET extra_perms = extra_perms - $1 WHERE {identifier} = $2 AND \ workspace_id = $3 RETURNING extra_perms" diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 93f14bf933..e99d5447f7 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -626,7 +626,7 @@ async fn list_jobs( pub async fn resume_suspended_flow_as_owner( authed: Authed, Extension(db): Extension, - Path((w_id, flow_id)): Path<(String, Uuid)>, + Path((_w_id, flow_id)): Path<(String, Uuid)>, QueryOrBody(value): QueryOrBody, ) -> error::Result { let value = value.unwrap_or(serde_json::Value::Null); @@ -634,16 +634,11 @@ pub async fn resume_suspended_flow_as_owner( let (flow, job_id) = get_suspended_flow_info(flow_id, &mut tx).await?; - if !authed.is_admin { - require_owner_of_path( - &w_id, - &authed.username, - &authed.groups, - &flow.script_path.clone().unwrap_or_else(|| String::new()), - &db, - ) - .await?; - } + require_owner_of_path( + &authed, + &flow.script_path.clone().unwrap_or_else(|| String::new()), + )?; + insert_resume_job(0, job_id, &flow, value, Some(authed.username), &mut tx).await?; resume_immediately_if_relevant(flow, job_id, &mut tx).await?; diff --git a/backend/windmill-api/src/resources.rs b/backend/windmill-api/src/resources.rs index 30fc35921a..22c827935c 100644 --- a/backend/windmill-api/src/resources.rs +++ b/backend/windmill-api/src/resources.rs @@ -386,9 +386,8 @@ async fn update_resource( if npath != path { check_path_conflict(&mut tx, &w_id, &npath).await?; - if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &path, &db).await?; - } + require_owner_of_path(&authed, path)?; + sqlx::query!( "UPDATE variable SET path = $1 WHERE path = $2 AND workspace_id = $3", npath, diff --git a/backend/windmill-api/src/scripts.rs b/backend/windmill-api/src/scripts.rs index 9fb0ec193d..c6b0b88c1a 100644 --- a/backend/windmill-api/src/scripts.rs +++ b/backend/windmill-api/src/scripts.rs @@ -273,10 +273,7 @@ async fn create_script( let ps = get_script_by_hash_internal(tx.transaction_mut(), &w_id, p_hash).await?; if ps.path != ns.path { - if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &ps.path, &db) - .await?; - } + require_owner_of_path(&authed, &ps.path)?; } let ph = { @@ -637,6 +634,8 @@ async fn archive_script_by_path( let path = path.to_path(); let mut tx = user_db.begin(&authed).await?; + require_owner_of_path(&authed, path)?; + let hash: i64 = sqlx::query_scalar!( "UPDATE script SET archived = true WHERE path = $1 AND workspace_id = $2 RETURNING hash", path, diff --git a/backend/windmill-api/src/variables.rs b/backend/windmill-api/src/variables.rs index 1f19eff2b7..823c8bcb9c 100644 --- a/backend/windmill-api/src/variables.rs +++ b/backend/windmill-api/src/variables.rs @@ -380,9 +380,8 @@ async fn update_variable( if let Some(npath) = ns.path { if npath != path { check_path_conflict(&mut tx, &w_id, &npath).await?; - if !authed.is_admin { - require_owner_of_path(&w_id, &authed.username, &authed.groups, &path, &db).await?; - } + require_owner_of_path(&authed, path)?; + let mut v = sqlx::query_scalar!( "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", path, diff --git a/frontend/src/lib/components/AppConnect.svelte b/frontend/src/lib/components/AppConnect.svelte index f30ffbbd4b..c276a5e939 100644 --- a/frontend/src/lib/components/AppConnect.svelte +++ b/frontend/src/lib/components/AppConnect.svelte @@ -461,7 +461,7 @@ > {/if} {/if} -
+
{#if step > 1 && !no_back} {/if} diff --git a/frontend/src/lib/components/ArgInput.svelte b/frontend/src/lib/components/ArgInput.svelte index 8bffa2f1c2..59c4416418 100644 --- a/frontend/src/lib/components/ArgInput.svelte +++ b/frontend/src/lib/components/ArgInput.svelte @@ -311,21 +311,23 @@ {/if} {/key}
- +
+ +
{:else if inputCat == 'resource-object'} diff --git a/frontend/src/lib/components/Dropdown.svelte b/frontend/src/lib/components/Dropdown.svelte index 785aa34ce7..208c2cddfc 100644 --- a/frontend/src/lib/components/Dropdown.svelte +++ b/frontend/src/lib/components/Dropdown.svelte @@ -11,13 +11,21 @@ type Side = 'top' | 'bottom' type Placement = `${Side}-${Alignment}` - export let dropdownItems: DropdownItem[] + export let dropdownItems: DropdownItem[] | (() => DropdownItem[]) = [] export let name: string | undefined = undefined export let placement: Placement = 'bottom-start' export let btnClasses = '' $: buttonClass = twMerge('!border-0 bg-transparent !p-[6px]', btnClasses) const dispatch = createEventDispatcher() + + function computeDropdowns(): DropdownItem[] { + if (typeof dropdownItems === 'function') { + return dropdownItems() + } else { + return dropdownItems + } + } @@ -37,7 +45,7 @@ {/if} {#if dropdownItems} - {#each dropdownItems as item, i} + {#each computeDropdowns() as item, i} {#if item.action} +
+ +
{(value ?? []).length} item{(value ?? []).length > 1 ? 's' : ''} diff --git a/frontend/src/lib/components/MoveDrawer.svelte b/frontend/src/lib/components/MoveDrawer.svelte index 46de5b3948..754832f4a6 100644 --- a/frontend/src/lib/components/MoveDrawer.svelte +++ b/frontend/src/lib/components/MoveDrawer.svelte @@ -27,12 +27,12 @@ kind = kind_l initialPath = initialPath_l summary = summary_l - await loadOwner() + loadOwner() drawer.openDrawer() } - async function loadOwner() { - own = await isOwner(initialPath, $userStore!, $workspaceStore!) + function loadOwner() { + own = isOwner(initialPath, $userStore!, $workspaceStore!) } async function updatePath() { diff --git a/frontend/src/lib/components/Path.svelte b/frontend/src/lib/components/Path.svelte index 2c1aa6b900..77d01f3291 100644 --- a/frontend/src/lib/components/Path.svelte +++ b/frontend/src/lib/components/Path.svelte @@ -337,6 +337,7 @@ btnClasses="!p-1.5" variant="border" size="xs" + {disabled} on:click={newFolder.openDrawer} > import type { Schema } from '$lib/common' import { ResourceService, type Resource } from '$lib/gen' - import { canWrite, emptyString, sendUserToast } from '$lib/utils' + import { canWrite, emptyString, isOwner, sendUserToast } from '$lib/utils' import { createEventDispatcher } from 'svelte' import { Alert, Button, Drawer, Skeleton } from './common' import Path from './Path.svelte' @@ -141,7 +141,7 @@ {/if} No schema {/if} {#if schedulable} -
-
-
- -
- {#if viewOptions} -
-
-
-
-