From e1f387914eb31676b9bc6b55ded5b1c52e72ea35 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 7 Sep 2026 07:49:29 +0200 Subject: [PATCH] refactor(datatables): role permissions belong to the database, keyed by it and owned by the workspace that turned them on --- ...14e04b4a13774d442a8edb6c9cc642140cf45.json | 23 - ...b0ba654f21458fb4d2d0a55e5c3eab29bb279.json | 34 + ...041bf4019929e5911cf4f144f24dd6916b106.json | 22 + ...f8e315e3e078b3d130d0db48277306b189ea9.json | 15 + ...125c1d0a76781c8340e65a72804f469d0f6c8.json | 15 + ...61b6e7b9d35a41594681e2a92a87359e6a018.json | 23 + ...b136783bc820ed96a9ebe84e4c79a47cc5df4.json | 30 - ...59d6b5e01e21890ef53ccb600c2f3506d84a3.json | 15 - ...41f357a09e2c94792542287cbbc810451c47a.json | 34 + ...ba1ea98aae07a9c2e30de79b3db411e1ef3e9.json | 34 + ...b35534824748db021100a95eb69078d6e0f74.json | 34 + ...6dd9c68dc10deedd93cb04a8c6636d736f30b.json | 14 - ...baeb468330b51edfb690921c67828c4c0bdf3.json | 14 - ...d1deb08846598ac67edb454c85b7000a2bd0b.json | 14 + ...01d1d28e19e3658b333e3248c203eb4895b68.json | 28 - ...f7100f39a7a7704a8ad31dba82042f6fd0b63.json | 47 - ...d92b1aaa9a043706fbcb85e39f1dd94fe91ab.json | 22 + ...25d51261c12c016218df01b2897abe94096fe.json | 16 + ...318a1d1a61af5ec7032353acd5e89027ae8dd.json | 14 - backend/ee-repo-ref.txt | 2 +- ...23_datatable_database_permissions.down.sql | 1 + ...5123_datatable_database_permissions.up.sql | 18 + .../tests/datatable_tenants.rs | 898 ++++------- backend/windmill-api-users/src/users.rs | 8 +- .../src/datatable_acl.rs | 57 +- .../src/datatable_permissions.rs | 753 ++++------ .../src/datatable_permissions_oss.rs | 3 +- .../windmill-api-workspaces/src/workspaces.rs | 269 +--- .../src/workspaces_extra.rs | 199 +-- backend/windmill-api/openapi.yaml | 8 +- backend/windmill-api/src/offboarding.rs | 14 +- backend/windmill-api/src/users.rs | 29 +- backend/windmill-api/src/workspaces_export.rs | 5 +- backend/windmill-common/src/workspaces.rs | 1332 +++++++---------- backend/windmill-store/src/resources.rs | 104 -- backend/windmill-store/src/variables.rs | 14 - .../DataTablePermissionsButton.svelte | 32 +- .../ForkDatatableSection.svelte | 52 +- 38 files changed, 1550 insertions(+), 2696 deletions(-) delete mode 100644 backend/.sqlx/query-01b0ebb2d85a8e59b04dd42275d14e04b4a13774d442a8edb6c9cc642140cf45.json create mode 100644 backend/.sqlx/query-3abcd060b4e1865f17e73ae7e17b0ba654f21458fb4d2d0a55e5c3eab29bb279.json create mode 100644 backend/.sqlx/query-3cb0fdbb787328c61696a3f4657041bf4019929e5911cf4f144f24dd6916b106.json create mode 100644 backend/.sqlx/query-3f11db5b3f8af739cb287f45071f8e315e3e078b3d130d0db48277306b189ea9.json create mode 100644 backend/.sqlx/query-47fc4d3ffd3ace0beb29cd3c4c3125c1d0a76781c8340e65a72804f469d0f6c8.json create mode 100644 backend/.sqlx/query-58e5cfe9eb87bda9f7de87c403861b6e7b9d35a41594681e2a92a87359e6a018.json delete mode 100644 backend/.sqlx/query-6b6360caca5569643c74c5a4033b136783bc820ed96a9ebe84e4c79a47cc5df4.json delete mode 100644 backend/.sqlx/query-77841f6a74d13f92b095a5fdeca59d6b5e01e21890ef53ccb600c2f3506d84a3.json create mode 100644 backend/.sqlx/query-82f7f52f60868a3aa659f2bee6b41f357a09e2c94792542287cbbc810451c47a.json create mode 100644 backend/.sqlx/query-852c7619569dd676b24319267c3ba1ea98aae07a9c2e30de79b3db411e1ef3e9.json create mode 100644 backend/.sqlx/query-95671af1034d05d58fb6e03e555b35534824748db021100a95eb69078d6e0f74.json delete mode 100644 backend/.sqlx/query-a673eb72797bbdab342b86fe9156dd9c68dc10deedd93cb04a8c6636d736f30b.json delete mode 100644 backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json create mode 100644 backend/.sqlx/query-d51b957f116e7292b3c5bb955e5d1deb08846598ac67edb454c85b7000a2bd0b.json delete mode 100644 backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json delete mode 100644 backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json create mode 100644 backend/.sqlx/query-ec849eb2e6bf2d5d13566721ea8d92b1aaa9a043706fbcb85e39f1dd94fe91ab.json create mode 100644 backend/.sqlx/query-f17b405bb47b4633675b75caffe25d51261c12c016218df01b2897abe94096fe.json delete mode 100644 backend/.sqlx/query-f42f40af69449bdd47103c63139318a1d1a61af5ec7032353acd5e89027ae8dd.json create mode 100644 backend/migrations/20260906215123_datatable_database_permissions.down.sql create mode 100644 backend/migrations/20260906215123_datatable_database_permissions.up.sql diff --git a/backend/.sqlx/query-01b0ebb2d85a8e59b04dd42275d14e04b4a13774d442a8edb6c9cc642140cf45.json b/backend/.sqlx/query-01b0ebb2d85a8e59b04dd42275d14e04b4a13774d442a8edb6c9cc642140cf45.json deleted file mode 100644 index 836a4477f9..0000000000 --- a/backend/.sqlx/query-01b0ebb2d85a8e59b04dd42275d14e04b4a13774d442a8edb6c9cc642140cf45.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT COALESCE((datatable->'datatables'->$2->'permissions'->>'enabled')::boolean, false)\n FROM workspace_settings WHERE workspace_id = $1", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "coalesce", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "01b0ebb2d85a8e59b04dd42275d14e04b4a13774d442a8edb6c9cc642140cf45" -} diff --git a/backend/.sqlx/query-3abcd060b4e1865f17e73ae7e17b0ba654f21458fb4d2d0a55e5c3eab29bb279.json b/backend/.sqlx/query-3abcd060b4e1865f17e73ae7e17b0ba654f21458fb4d2d0a55e5c3eab29bb279.json new file mode 100644 index 0000000000..07607d9e98 --- /dev/null +++ b/backend/.sqlx/query-3abcd060b4e1865f17e73ae7e17b0ba654f21458fb4d2d0a55e5c3eab29bb279.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT database_key, owner_workspace_id, permissions\n FROM datatable_database_permissions WHERE database_key = $1 FOR UPDATE", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "database_key", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "owner_workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "permissions", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "3abcd060b4e1865f17e73ae7e17b0ba654f21458fb4d2d0a55e5c3eab29bb279" +} diff --git a/backend/.sqlx/query-3cb0fdbb787328c61696a3f4657041bf4019929e5911cf4f144f24dd6916b106.json b/backend/.sqlx/query-3cb0fdbb787328c61696a3f4657041bf4019929e5911cf4f144f24dd6916b106.json new file mode 100644 index 0000000000..59828f908d --- /dev/null +++ b/backend/.sqlx/query-3cb0fdbb787328c61696a3f4657041bf4019929e5911cf4f144f24dd6916b106.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT pg_advisory_xact_lock(hashtext('datatable_database_permissions:' || $1))", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "pg_advisory_xact_lock", + "type_info": "Void" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "3cb0fdbb787328c61696a3f4657041bf4019929e5911cf4f144f24dd6916b106" +} diff --git a/backend/.sqlx/query-3f11db5b3f8af739cb287f45071f8e315e3e078b3d130d0db48277306b189ea9.json b/backend/.sqlx/query-3f11db5b3f8af739cb287f45071f8e315e3e078b3d130d0db48277306b189ea9.json new file mode 100644 index 0000000000..cd749da42f --- /dev/null +++ b/backend/.sqlx/query-3f11db5b3f8af739cb287f45071f8e315e3e078b3d130d0db48277306b189ea9.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE datatable_database_permissions SET owner_workspace_id = $1 WHERE owner_workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text" + ] + }, + "nullable": [] + }, + "hash": "3f11db5b3f8af739cb287f45071f8e315e3e078b3d130d0db48277306b189ea9" +} diff --git a/backend/.sqlx/query-47fc4d3ffd3ace0beb29cd3c4c3125c1d0a76781c8340e65a72804f469d0f6c8.json b/backend/.sqlx/query-47fc4d3ffd3ace0beb29cd3c4c3125c1d0a76781c8340e65a72804f469d0f6c8.json new file mode 100644 index 0000000000..30e1a9e354 --- /dev/null +++ b/backend/.sqlx/query-47fc4d3ffd3ace0beb29cd3c4c3125c1d0a76781c8340e65a72804f469d0f6c8.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', (\n SELECT COALESCE(jsonb_object_agg(\n key,\n CASE WHEN key = ANY($2) THEN value ELSE value - 'forked_from' END\n ), '{}'::jsonb)\n FROM jsonb_each(datatable->'datatables')\n ))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "47fc4d3ffd3ace0beb29cd3c4c3125c1d0a76781c8340e65a72804f469d0f6c8" +} diff --git a/backend/.sqlx/query-58e5cfe9eb87bda9f7de87c403861b6e7b9d35a41594681e2a92a87359e6a018.json b/backend/.sqlx/query-58e5cfe9eb87bda9f7de87c403861b6e7b9d35a41594681e2a92a87359e6a018.json new file mode 100644 index 0000000000..f4f7acd0b7 --- /dev/null +++ b/backend/.sqlx/query-58e5cfe9eb87bda9f7de87c403861b6e7b9d35a41594681e2a92a87359e6a018.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT username FROM usr WHERE workspace_id = $1 AND email = $2 AND disabled = false", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "username", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "58e5cfe9eb87bda9f7de87c403861b6e7b9d35a41594681e2a92a87359e6a018" +} diff --git a/backend/.sqlx/query-6b6360caca5569643c74c5a4033b136783bc820ed96a9ebe84e4c79a47cc5df4.json b/backend/.sqlx/query-6b6360caca5569643c74c5a4033b136783bc820ed96a9ebe84e4c79a47cc5df4.json deleted file mode 100644 index f7ff750b55..0000000000 --- a/backend/.sqlx/query-6b6360caca5569643c74c5a4033b136783bc820ed96a9ebe84e4c79a47cc5df4.json +++ /dev/null @@ -1,30 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n WHERE NOT (ws.workspace_id = $1 AND dt.key = $2)\n AND COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n AND dt.value->'database' = $3\n ORDER BY ws.workspace_id, dt.key", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "workspace_id!", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "name!", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Text", - "Text", - "Jsonb" - ] - }, - "nullable": [ - false, - null - ] - }, - "hash": "6b6360caca5569643c74c5a4033b136783bc820ed96a9ebe84e4c79a47cc5df4" -} diff --git a/backend/.sqlx/query-77841f6a74d13f92b095a5fdeca59d6b5e01e21890ef53ccb600c2f3506d84a3.json b/backend/.sqlx/query-77841f6a74d13f92b095a5fdeca59d6b5e01e21890ef53ccb600c2f3506d84a3.json deleted file mode 100644 index bc09acefed..0000000000 --- a/backend/.sqlx/query-77841f6a74d13f92b095a5fdeca59d6b5e01e21890ef53ccb600c2f3506d84a3.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', (\n SELECT COALESCE(jsonb_object_agg(\n key,\n CASE WHEN key = ANY($2) THEN value - 'permissions' ELSE value - 'forked_from' END\n ), '{}'::jsonb)\n FROM jsonb_each(datatable->'datatables')\n WHERE key = ANY($2)\n OR COALESCE((value->'permissions'->>'enabled')::boolean, false) = false\n ))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Text", - "TextArray" - ] - }, - "nullable": [] - }, - "hash": "77841f6a74d13f92b095a5fdeca59d6b5e01e21890ef53ccb600c2f3506d84a3" -} diff --git a/backend/.sqlx/query-82f7f52f60868a3aa659f2bee6b41f357a09e2c94792542287cbbc810451c47a.json b/backend/.sqlx/query-82f7f52f60868a3aa659f2bee6b41f357a09e2c94792542287cbbc810451c47a.json new file mode 100644 index 0000000000..65ef531d15 --- /dev/null +++ b/backend/.sqlx/query-82f7f52f60868a3aa659f2bee6b41f357a09e2c94792542287cbbc810451c47a.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT database_key, owner_workspace_id, permissions\n FROM datatable_database_permissions WHERE owner_workspace_id = $1\n ORDER BY database_key FOR UPDATE", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "database_key", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "owner_workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "permissions", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "82f7f52f60868a3aa659f2bee6b41f357a09e2c94792542287cbbc810451c47a" +} diff --git a/backend/.sqlx/query-852c7619569dd676b24319267c3ba1ea98aae07a9c2e30de79b3db411e1ef3e9.json b/backend/.sqlx/query-852c7619569dd676b24319267c3ba1ea98aae07a9c2e30de79b3db411e1ef3e9.json new file mode 100644 index 0000000000..6633f65b17 --- /dev/null +++ b/backend/.sqlx/query-852c7619569dd676b24319267c3ba1ea98aae07a9c2e30de79b3db411e1ef3e9.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT database_key, owner_workspace_id, permissions\n FROM datatable_database_permissions WHERE owner_workspace_id = $1 ORDER BY database_key", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "database_key", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "owner_workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "permissions", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "852c7619569dd676b24319267c3ba1ea98aae07a9c2e30de79b3db411e1ef3e9" +} diff --git a/backend/.sqlx/query-95671af1034d05d58fb6e03e555b35534824748db021100a95eb69078d6e0f74.json b/backend/.sqlx/query-95671af1034d05d58fb6e03e555b35534824748db021100a95eb69078d6e0f74.json new file mode 100644 index 0000000000..dd53a7ba28 --- /dev/null +++ b/backend/.sqlx/query-95671af1034d05d58fb6e03e555b35534824748db021100a95eb69078d6e0f74.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT database_key, owner_workspace_id, permissions\n FROM datatable_database_permissions WHERE database_key = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "database_key", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "owner_workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "permissions", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "95671af1034d05d58fb6e03e555b35534824748db021100a95eb69078d6e0f74" +} diff --git a/backend/.sqlx/query-a673eb72797bbdab342b86fe9156dd9c68dc10deedd93cb04a8c6636d736f30b.json b/backend/.sqlx/query-a673eb72797bbdab342b86fe9156dd9c68dc10deedd93cb04a8c6636d736f30b.json deleted file mode 100644 index 73358ff2d6..0000000000 --- a/backend/.sqlx/query-a673eb72797bbdab342b86fe9156dd9c68dc10deedd93cb04a8c6636d736f30b.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', '{}'::jsonb)\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Text" - ] - }, - "nullable": [] - }, - "hash": "a673eb72797bbdab342b86fe9156dd9c68dc10deedd93cb04a8c6636d736f30b" -} diff --git a/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json b/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json deleted file mode 100644 index 5e4cb7ca90..0000000000 --- a/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "DELETE FROM datatable_migrations m\n WHERE m.workspace_id = $1\n AND NOT EXISTS (\n SELECT 1 FROM workspace_settings ws\n WHERE ws.workspace_id = $1 AND ws.datatable->'datatables' ? m.datatable\n )", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Text" - ] - }, - "nullable": [] - }, - "hash": "bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3" -} diff --git a/backend/.sqlx/query-d51b957f116e7292b3c5bb955e5d1deb08846598ac67edb454c85b7000a2bd0b.json b/backend/.sqlx/query-d51b957f116e7292b3c5bb955e5d1deb08846598ac67edb454c85b7000a2bd0b.json new file mode 100644 index 0000000000..38b76ba618 --- /dev/null +++ b/backend/.sqlx/query-d51b957f116e7292b3c5bb955e5d1deb08846598ac67edb454c85b7000a2bd0b.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM datatable_database_permissions WHERE database_key = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "d51b957f116e7292b3c5bb955e5d1deb08846598ac67edb454c85b7000a2bd0b" +} diff --git a/backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json b/backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json deleted file mode 100644 index 2799bc59a2..0000000000 --- a/backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n AND dt.value->'permissions'->>'physical_identity' = $1\n ORDER BY ws.workspace_id, dt.key", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "workspace_id!", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "name!", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Text" - ] - }, - "nullable": [ - false, - null - ] - }, - "hash": "e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68" -} diff --git a/backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json b/backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json deleted file mode 100644 index dee4138908..0000000000 --- a/backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json +++ /dev/null @@ -1,47 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\", w.deleted AS \"deleted!\",\n dt.value->'database' AS \"database!\", r.value AS \"resource?\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id\n CROSS JOIN LATERAL jsonb_each(ws.datatable->'datatables') dt\n LEFT JOIN resource r ON r.workspace_id = ws.workspace_id\n AND dt.value->'database'->>'resource_type' <> 'instance'\n AND r.path = dt.value->'database'->>'resource_path'\n WHERE NOT (ws.workspace_id = $1 AND dt.key = $2)\n AND jsonb_typeof(dt.value->'database') = 'object'\n ORDER BY ws.workspace_id, dt.key", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "workspace_id!", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "name!", - "type_info": "Text" - }, - { - "ordinal": 2, - "name": "deleted!", - "type_info": "Bool" - }, - { - "ordinal": 3, - "name": "database!", - "type_info": "Jsonb" - }, - { - "ordinal": 4, - "name": "resource?", - "type_info": "Jsonb" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - false, - null, - false, - null, - true - ] - }, - "hash": "e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63" -} diff --git a/backend/.sqlx/query-ec849eb2e6bf2d5d13566721ea8d92b1aaa9a043706fbcb85e39f1dd94fe91ab.json b/backend/.sqlx/query-ec849eb2e6bf2d5d13566721ea8d92b1aaa9a043706fbcb85e39f1dd94fe91ab.json new file mode 100644 index 0000000000..36fae84deb --- /dev/null +++ b/backend/.sqlx/query-ec849eb2e6bf2d5d13566721ea8d92b1aaa9a043706fbcb85e39f1dd94fe91ab.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "?column?", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "ec849eb2e6bf2d5d13566721ea8d92b1aaa9a043706fbcb85e39f1dd94fe91ab" +} diff --git a/backend/.sqlx/query-f17b405bb47b4633675b75caffe25d51261c12c016218df01b2897abe94096fe.json b/backend/.sqlx/query-f17b405bb47b4633675b75caffe25d51261c12c016218df01b2897abe94096fe.json new file mode 100644 index 0000000000..865484adde --- /dev/null +++ b/backend/.sqlx/query-f17b405bb47b4633675b75caffe25d51261c12c016218df01b2897abe94096fe.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO datatable_database_permissions (database_key, owner_workspace_id, permissions)\n VALUES ($1, $2, $3)\n ON CONFLICT (database_key) DO UPDATE SET permissions = EXCLUDED.permissions, updated_at = now()", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Varchar", + "Jsonb" + ] + }, + "nullable": [] + }, + "hash": "f17b405bb47b4633675b75caffe25d51261c12c016218df01b2897abe94096fe" +} diff --git a/backend/.sqlx/query-f42f40af69449bdd47103c63139318a1d1a61af5ec7032353acd5e89027ae8dd.json b/backend/.sqlx/query-f42f40af69449bdd47103c63139318a1d1a61af5ec7032353acd5e89027ae8dd.json deleted file mode 100644 index 6b4f7f930c..0000000000 --- a/backend/.sqlx/query-f42f40af69449bdd47103c63139318a1d1a61af5ec7032353acd5e89027ae8dd.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', COALESCE((\n SELECT jsonb_object_agg(key, value - 'permissions')\n FROM jsonb_each(datatable->'datatables')\n ), '{}'::jsonb))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Text" - ] - }, - "nullable": [] - }, - "hash": "f42f40af69449bdd47103c63139318a1d1a61af5ec7032353acd5e89027ae8dd" -} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 1266c658d3..d33a667e0d 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -0c220600e7ceece78a7623fd4185adfb0054d1c9 +3579765f3b36a339b9de2a2e8186bcfb681732d9 diff --git a/backend/migrations/20260906215123_datatable_database_permissions.down.sql b/backend/migrations/20260906215123_datatable_database_permissions.down.sql new file mode 100644 index 0000000000..a9fb307cb4 --- /dev/null +++ b/backend/migrations/20260906215123_datatable_database_permissions.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS datatable_database_permissions; diff --git a/backend/migrations/20260906215123_datatable_database_permissions.up.sql b/backend/migrations/20260906215123_datatable_database_permissions.up.sql new file mode 100644 index 0000000000..866ac6ed32 --- /dev/null +++ b/backend/migrations/20260906215123_datatable_database_permissions.up.sql @@ -0,0 +1,18 @@ +-- Role-based access to a data table's database is a property of the Postgres +-- database the roles are created in, not of the config entry that points at it: +-- every entry reaching the same database, in any workspace, resolves to this row. +-- database_key: 'instance:' for an instance database, 'pg:' for a resource-backed one. The tenants named in +-- `permissions` are principals of owner_workspace_id, and only its admins manage +-- the row. +CREATE TABLE datatable_database_permissions ( + database_key TEXT PRIMARY KEY, + owner_workspace_id VARCHAR(50) NOT NULL REFERENCES workspace(id) ON DELETE CASCADE, + permissions JSONB NOT NULL, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE INDEX datatable_database_permissions_owner ON datatable_database_permissions (owner_workspace_id); + +GRANT ALL ON datatable_database_permissions TO windmill_user; +GRANT ALL ON datatable_database_permissions TO windmill_admin; diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index 25dcf862e2..bc4baa3d1f 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -11,16 +11,67 @@ fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuil builder.header("Authorization", format!("Bearer {token}")) } -/// Who the data table's one role currently lets run as it. -async fn tenants(db: &Pool) -> Vec { - let value: Option = sqlx::query_scalar( - "SELECT datatable->'datatables'->'main'->'permissions'->'roles'->'analyst'->'tenants' - FROM workspace_settings WHERE workspace_id = 'test-workspace'", +const MAIN_KEY: &str = "instance:dt_main"; + +/// A data table `main` on the instance database `dt_main`, in `w`. +async fn plant_main(db: &Pool, w: &str) { + sqlx::query( + "INSERT INTO workspace_settings (workspace_id, datatable) VALUES ($1, $2) + ON CONFLICT (workspace_id) DO UPDATE SET datatable = EXCLUDED.datatable", ) + .bind(w) + .bind(json!({ + "datatables": { + "main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } + } + })) + .execute(db) + .await + .unwrap(); +} + +/// Permissions on a database, owned by `owner`, with one `analyst` role. +async fn plant_permissions(db: &Pool, key: &str, owner: &str, analyst_tenants: &[&str]) { + sqlx::query( + "INSERT INTO datatable_database_permissions (database_key, owner_workspace_id, permissions) + VALUES ($1, $2, $3)", + ) + .bind(key) + .bind(owner) + .bind(json!({ "enabled": true, "roles": { + "admin": { "tenants": [] }, + "analyst": { "tenants": analyst_tenants, "pg_rolename": "wm_analyst_x", "pg_password": "pw" } + }})) + .execute(db) + .await + .unwrap(); +} + +/// Who the `analyst` role of `key` currently lets run as it. +async fn tenants(db: &Pool, key: &str) -> Vec { + let value: serde_json::Value = sqlx::query_scalar( + "SELECT permissions->'roles'->'analyst'->'tenants' + FROM datatable_database_permissions WHERE database_key = $1", + ) + .bind(key) .fetch_one(db) .await .unwrap(); - serde_json::from_value(value.unwrap()).unwrap() + serde_json::from_value(value).unwrap() +} + +async fn usable_roles(port: u16, w: &str, datatable: &str, token: &str) -> serde_json::Value { + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/{w}/workspaces/datatable_usable_roles/{datatable}" + )), + token, + ) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "{}", resp.text().await.unwrap()); + resp.json().await.unwrap() } /// A tenant is a name, and a name outlives the principal that held it: whoever @@ -34,24 +85,20 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); - sqlx::query( - r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, + plant_main(&db, "test-workspace").await; + plant_permissions( + &db, + MAIN_KEY, + "test-workspace", + &[ + "*", + "u/test-user-2", + "u/test-user-3", + "g/leaving_group", + "f/leaving_folder", + ], ) - .bind(json!({ - "datatables": { - "main": { - "database": { "resource_type": "instance", "resource_path": "dt_main" }, - "permissions": { "enabled": true, "roles": { - "admin": { "tenants": [] }, - "analyst": { "tenants": [ - "*", "u/test-user-2", "u/test-user-3", "g/leaving_group", "f/leaving_folder" - ]} - }} - } - } - })) - .execute(&db) - .await?; + .await; for (endpoint, body) in [ ("groups/create", json!({ "name": "leaving_group" })), @@ -71,7 +118,9 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a .send() .await?; assert_eq!(resp.status(), 200, "delete group: {}", resp.text().await?); - assert!(!tenants(&db).await.contains(&"g/leaving_group".to_string())); + assert!(!tenants(&db, MAIN_KEY) + .await + .contains(&"g/leaving_group".to_string())); let resp = authed( client().delete(format!("{ws}/folders/delete/leaving_folder")), @@ -80,14 +129,18 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a .send() .await?; assert_eq!(resp.status(), 200, "delete folder: {}", resp.text().await?); - assert!(!tenants(&db).await.contains(&"f/leaving_folder".to_string())); + assert!(!tenants(&db, MAIN_KEY) + .await + .contains(&"f/leaving_folder".to_string())); // Leaving frees the username as surely as an admin removing the member does. let resp = authed(client().post(format!("{ws}/users/leave")), "SECRET_TOKEN_2") .send() .await?; assert_eq!(resp.status(), 200, "leave: {}", resp.text().await?); - assert!(!tenants(&db).await.contains(&"u/test-user-2".to_string())); + assert!(!tenants(&db, MAIN_KEY) + .await + .contains(&"u/test-user-2".to_string())); let resp = authed( client().delete(format!( @@ -98,275 +151,188 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a .send() .await?; assert_eq!(resp.status(), 200, "global delete: {}", resp.text().await?); - assert!(!tenants(&db).await.contains(&"u/test-user-3".to_string())); + assert!(!tenants(&db, MAIN_KEY) + .await + .contains(&"u/test-user-3".to_string())); // What no deletion named is left alone — the wildcard above all, which is // not a principal and cannot be freed. - assert_eq!(tenants(&db).await, vec!["*".to_string()]); + assert_eq!(tenants(&db, MAIN_KEY).await, vec!["*".to_string()]); Ok(()) } -/// A fork made while the data table was unpermissioned carries a copy of it that -/// points at the same database; opting in would leave every member of the fork -/// reaching that database through the copy's own connection. Pinned on the save -/// and on the preview, since a plan the save refuses to run must not be offered. +/// The permissions are the database's, so a fork's copy of the data table reaches +/// the same roles — evaluated as a member of the workspace that owns them. Being +/// admin of the fork, which any member is of a fork they made, counts for +/// nothing; a superadmin reaches every role from anywhere; and the roles are +/// managed from the owning workspace alone. #[sqlx::test(migrations = "../migrations", fixtures("base"))] -async fn enabling_permissions_is_refused_while_a_fork_exists( +async fn a_fork_copy_is_evaluated_as_a_member_of_the_owning_workspace( db: Pool, ) -> anyhow::Result<()> { initialize_tracing().await; let server = ApiServer::start(db.clone()).await?; let port = server.addr.port(); - let ws = format!("http://localhost:{port}/api/w/test-workspace"); - sqlx::query( - r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, - ) - .bind(json!({ - "datatables": { - "main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }, - "byo": { "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" } } - } - })) - .execute(&db) - .await?; + plant_main(&db, "test-workspace").await; + plant_permissions(&db, MAIN_KEY, "test-workspace", &["u/test-user-3"]).await; sqlx::query( "INSERT INTO workspace (id, name, owner, parent_workspace_id) - VALUES ('wm-fork-t', 'wm-fork-t', 'test-user', 'test-workspace')", + VALUES ('wm-fork-t', 'wm-fork-t', 'test2@windmill.dev', 'test-workspace')", + ) + .execute(&db) + .await?; + plant_main(&db, "wm-fork-t").await; + // test-user-2 made the fork and is admin of it, and is no member of the parent. + sqlx::query( + "INSERT INTO usr (workspace_id, email, username, is_admin, role) VALUES + ('wm-fork-t', 'test2@windmill.dev', 'test-user-2', true, 'Admin'), + ('wm-fork-t', 'test3@windmill.dev', 'test-user-3', false, 'User')", ) .execute(&db) .await?; sqlx::query( - "INSERT INTO workspace_settings (workspace_id, datatable) VALUES ('wm-fork-t', $1)", - ) - .bind(json!({ - "datatables": { - "main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }, - "byo": { "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" } } - } - })) - .execute(&db) - .await?; - - let body = json!({ "enabled": true, "roles": [] }); - for endpoint in [ - "workspaces/datatable_permissions/main/preview", - "workspaces/datatable_permissions/main", - ] { - let resp = authed(client().post(format!("{ws}/{endpoint}")), "SECRET_TOKEN") - .json(&body) - .send() - .await?; - let status = resp.status(); - let text = resp.text().await?; - assert_eq!(status, 400, "{endpoint}: {text}"); - assert!( - text.contains("wm-fork-t (data table 'main')"), - "{endpoint}: {text}" - ); - } - - // Only the opt-in is gated: once permissions are on, forks made afterwards - // never receive the data table, and editing the roles — revoking a tenant - // above all — has to keep working while they exist. The preview then gets as - // far as the database, which this test does not have. - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables,main,permissions}', - '{"enabled": true, "roles": {"admin": {"tenants": []}}}') - WHERE workspace_id = 'test-workspace'"#, + "DELETE FROM usr WHERE workspace_id = 'test-workspace' AND email = 'test2@windmill.dev'", ) .execute(&db) .await?; - let edit = json!({ "enabled": true, "roles": [ - { "name": "admin", "tenants": [] }, { "name": "analyst", "tenants": ["u/test-user"] } - ]}); + + let fork_admin = usable_roles(port, "wm-fork-t", "main", "SECRET_TOKEN_2").await; + assert_eq!(fork_admin["enabled"], json!(true)); + assert_eq!(fork_admin["roles"], json!([]), "{fork_admin}"); + + let tenant = usable_roles(port, "wm-fork-t", "main", "SECRET_TOKEN_3").await; + assert_eq!(tenant["roles"], json!(["analyst"]), "{tenant}"); + + let superadmin = usable_roles(port, "wm-fork-t", "main", "SECRET_TOKEN").await; + assert_eq!( + superadmin["roles"], + json!(["admin", "analyst"]), + "{superadmin}" + ); + + // Managed from the owning workspace: the fork's admin reads them, changes nothing. let resp = authed( - client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" + client().get(format!( + "http://localhost:{port}/api/w/wm-fork-t/workspaces/datatable_permissions/main" )), - "SECRET_TOKEN", + "SECRET_TOKEN_2", ) - .json(&edit) .send() .await?; - let text = resp.text().await?; - assert!(!text.contains("cannot be enabled"), "{text}"); - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = datatable #- '{datatables,main,permissions}' - WHERE workspace_id = 'test-workspace'"#, - ) - .execute(&db) - .await?; - - // A resource-backed copy keeps the parent's pointer when cloned — the cloned - // resource is what changes — so `forked_from` is what tells the two apart. - let byo = format!("{ws}/workspaces/datatable_permissions/byo/preview"); - let resp = authed(client().post(&byo), "SECRET_TOKEN") - .json(&body) - .send() - .await?; - assert_eq!(resp.status(), 400); - let text = resp.text().await?; - assert!(text.contains("wm-fork-t (data table 'byo')"), "{text}"); - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables,byo,forked_from}', '{"schema": {}}') - WHERE workspace_id = 'wm-fork-t'"#, - ) - .execute(&db) - .await?; - // Past the refusal the preview fails on the resource, which this test does - // not have; the refusal is what is pinned. - let resp = authed(client().post(&byo), "SECRET_TOKEN") - .json(&body) - .send() - .await?; - let text = resp.text().await?; - assert!(!text.contains("cannot be enabled"), "{text}"); - - // Archiving keeps the fork's members and its copy, so it still counts; a fork - // whose copy is a clone of its own does not. - sqlx::query("UPDATE workspace SET deleted = true WHERE id = 'wm-fork-t'") - .execute(&db) - .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let info: serde_json::Value = resp.json().await?; + assert_eq!(info["owner_workspace_id"], json!("test-workspace")); + assert_eq!(info["editable"], json!(false)); let resp = authed( client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" + "http://localhost:{port}/api/w/wm-fork-t/workspaces/datatable_permissions/main" )), - "SECRET_TOKEN", + "SECRET_TOKEN_2", ) - .json(&body) + .json(&json!({ "enabled": true, "roles": [ + { "name": "admin", "tenants": [] }, { "name": "analyst", "tenants": ["u/test-user-2"] } + ]})) .send() .await?; - assert_eq!(resp.status(), 400); + let status = resp.status().as_u16(); let text = resp.text().await?; + assert_eq!(status, 401, "{text}"); assert!( - text.contains("wm-fork-t, archived (data table 'main')"), + text.contains("managed from workspace 'test-workspace'"), "{text}" ); - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set( - jsonb_set(datatable, '{datatables,main,forked_from}', '{"schema": {}}'), - '{datatables,main,database,resource_path}', '"wm_fork_dt_main"') - WHERE workspace_id = 'wm-fork-t'"#, - ) - .execute(&db) - .await?; - let resp = authed( - client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" - )), - "SECRET_TOKEN", - ) - .json(&body) - .send() - .await?; - let text = resp.text().await?; - assert!(!text.contains("cannot be enabled"), "{text}"); - - // A workspace that is no longer a fork — a detached dev workspace — keeps its - // copy of the data table, pointing at the same instance database. - sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'wm-fork-t'") - .execute(&db) - .await?; - sqlx::query("DELETE FROM workspace WHERE id = 'wm-fork-t'") - .execute(&db) - .await?; - sqlx::query( - "INSERT INTO workspace (id, name, owner) VALUES ('detached', 'detached', 'test-user')", - ) - .execute(&db) - .await?; - sqlx::query("INSERT INTO workspace_settings (workspace_id, datatable) VALUES ('detached', $1)") - .bind(json!({ - "datatables": { - "copy": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } - } - })) - .execute(&db) - .await?; - let resp = authed( - client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" - )), - "SECRET_TOKEN", - ) - .json(&body) - .send() - .await?; - assert_eq!(resp.status(), 400); - let text = resp.text().await?; - assert!(text.contains("detached (data table 'copy')"), "{text}"); - - // Archived, it still counts; with the copy gone the refusal lifts, and the - // preview then gets as far as the database, which this test does not have. - sqlx::query("UPDATE workspace SET deleted = true WHERE id = 'detached'") - .execute(&db) - .await?; - let resp = authed( - client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" - )), - "SECRET_TOKEN", - ) - .json(&body) - .send() - .await?; - let text = resp.text().await?; - assert!( - text.contains("detached, archived (data table 'copy')"), - "{text}" - ); - sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'detached'") - .execute(&db) - .await?; - let resp = authed( - client().post(format!( - "{ws}/workspaces/datatable_permissions/main/preview" - )), - "SECRET_TOKEN", - ) - .json(&body) - .send() - .await?; - let text = resp.text().await?; - assert!(!text.contains("cannot be enabled"), "{text}"); Ok(()) } -/// The rename keeps a copy of the settings under the archived id, and commits it -/// before the old id is archived. No data table may be in that copy: a -/// permissioned one without its `permissions` block would resolve, for anyone -/// still using the old id, to the owner connection, and any one naming the same -/// instance database would keep the renamed workspace from opting in. +/// A resource-backed database is the host, port and database its resource +/// resolves to: entries naming it under other paths and other logins, in other +/// workspaces, reach the same permissions; a resource pointed at another database +/// reaches none. #[sqlx::test(migrations = "../migrations", fixtures("base"))] -async fn a_rename_leaves_no_datatable_under_the_old_id(db: Pool) -> anyhow::Result<()> { +async fn entries_reaching_one_database_share_its_permissions( + db: Pool, +) -> anyhow::Result<()> { initialize_tracing().await; let server = ApiServer::start(db.clone()).await?; let port = server.addr.port(); + sqlx::query("INSERT INTO workspace (id, name, owner) VALUES ('other', 'other', 'test-user')") + .execute(&db) + .await?; sqlx::query( - r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, + "INSERT INTO usr (workspace_id, email, username, is_admin, role) + VALUES ('other', 'test3@windmill.dev', 'user-three', false, 'User')", ) - .bind(json!({ - "datatables": { - "open": { "database": { "resource_type": "instance", "resource_path": "dt_open" } }, - "main": { - "database": { "resource_type": "instance", "resource_path": "dt_main" }, - "permissions": { "enabled": true, "roles": { - "admin": { "tenants": [] }, - "analyst": { "tenants": ["*"], "pg_rolename": "wm_x", "pg_password": "s3cret" } - }} - } - } - })) .execute(&db) .await?; + let plant = |w: &'static str, path: &'static str, user: &'static str, dbname: &'static str| { + let db = db.clone(); + async move { + sqlx::query( + "INSERT INTO resource (workspace_id, path, value, resource_type, created_by, edited_at) + VALUES ($1, $2, $3, 'postgresql', 'test-user', now())", + ) + .bind(w) + .bind(path) + .bind(json!({ "host": "db.example", "port": 5432, "dbname": dbname, "user": user, "password": "pw" })) + .execute(&db) + .await + .unwrap(); + sqlx::query( + "INSERT INTO workspace_settings (workspace_id, datatable) VALUES ($1, $2) + ON CONFLICT (workspace_id) DO UPDATE SET datatable = EXCLUDED.datatable", + ) + .bind(w) + .bind(json!({ "datatables": { + "byo": { "database": { "resource_type": "postgresql", "resource_path": path } } + }})) + .execute(&db) + .await + .unwrap(); + } + }; + plant("test-workspace", "u/test-user/pg", "app", "prod").await; + plant("other", "f/moved/pg", "postgres", "prod").await; + + let key = windmill_common::workspaces::datatable_database_key( + &windmill_common::workspaces::DataTableDatabase { + resource_type: windmill_common::workspaces::DataTableCatalogResourceType::Postgresql, + resource_path: "u/test-user/pg".to_string(), + }, + &json!({ "host": "db.example", "port": 5432, "dbname": "prod" }), + ); + plant_permissions(&db, &key, "test-workspace", &["u/test-user-3"]).await; + + // test-user-3 is `test-user-3` in the owning workspace and `user-three` in + // `other`: the tenant is matched where it was written. + let from_other = usable_roles(port, "other", "byo", "SECRET_TOKEN_3").await; + assert_eq!(from_other["enabled"], json!(true)); + assert_eq!(from_other["roles"], json!(["analyst"]), "{from_other}"); + + sqlx::query( + "UPDATE resource SET value = jsonb_set(value, '{dbname}', '\"staging\"') + WHERE workspace_id = 'other' AND path = 'f/moved/pg'", + ) + .execute(&db) + .await?; + let elsewhere = usable_roles(port, "other", "byo", "SECRET_TOKEN_3").await; + assert_eq!(elsewhere["enabled"], json!(false), "{elsewhere}"); + + Ok(()) +} + +/// The permissions a workspace owns follow it through a change of its id. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn a_rename_moves_the_permissions_it_owns(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + plant_main(&db, "test-workspace").await; + plant_permissions(&db, MAIN_KEY, "test-workspace", &["u/test-user-3"]).await; let resp = authed( client().post(format!( @@ -379,38 +345,24 @@ async fn a_rename_leaves_no_datatable_under_the_old_id(db: Pool) -> an .await?; assert_eq!(resp.status(), 200, "{}", resp.text().await?); - let names = |w: &'static str| { - let db = db.clone(); - async move { - let value: serde_json::Value = sqlx::query_scalar( - "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = $1", - ) - .bind(w) - .fetch_one(&db) - .await - .unwrap(); - let mut keys: Vec = value.as_object().unwrap().keys().cloned().collect(); - keys.sort(); - (keys, value) - } - }; - let (old_names, _) = names("test-workspace").await; - assert!(old_names.is_empty(), "{old_names:?}"); - let (new_names, new_value) = names("renamed-ws").await; - assert_eq!(new_names, vec!["main".to_string(), "open".to_string()]); - assert_eq!(new_value["main"]["permissions"]["enabled"], json!(true)); - assert_eq!( - new_value["main"]["permissions"]["roles"]["analyst"]["pg_password"], - json!("s3cret") - ); + let owner: String = sqlx::query_scalar( + "SELECT owner_workspace_id FROM datatable_database_permissions WHERE database_key = $1", + ) + .bind(MAIN_KEY) + .fetch_one(&db) + .await?; + assert_eq!(owner, "renamed-ws"); + let roles = usable_roles(port, "renamed-ws", "main", "SECRET_TOKEN_3").await; + assert_eq!(roles["roles"], json!(["analyst"]), "{roles}"); Ok(()) } /// A fork that keeps the original copies the parent's entry verbatim, `forked_from` -/// included when the parent's own entry is a clone (a detached dev workspace keeps -/// its clones). The stamp means "cloned into this workspace's own database", and -/// the opt-in trusts it, so a copy must not carry one. +/// included when the parent's own entry is a clone. The stamp means "cloned into +/// this workspace's own database" — it is what lets the fork's deletion drop that +/// database — so a copy must not carry one, and the config form may update the +/// schema snapshot inside an existing stamp but never add or remove one. #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn a_kept_original_does_not_inherit_the_clone_stamp( db: Pool, @@ -428,22 +380,11 @@ async fn a_kept_original_does_not_inherit_the_clone_stamp( "byo": { "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" }, "forked_from": { "schema": {} } - }, - "governed": { - "database": { "resource_type": "instance", "resource_path": "dt_governed" }, - "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] } } } } } })) .execute(&db) .await?; - sqlx::query( - "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up) - VALUES ('test-workspace', 'governed', 1, 'init', 'SELECT 1'), - ('test-workspace', 'byo', 1, 'init', 'SELECT 1')", - ) - .execute(&db) - .await?; let resp = authed( client().post(format!("{ws}/workspaces/create_fork")), @@ -461,88 +402,56 @@ async fn a_kept_original_does_not_inherit_the_clone_stamp( .await?; assert_eq!(copy["database"]["resource_path"], json!("u/test-user/pg")); assert!(copy.get("forked_from").is_none(), "{copy}"); - // The permissioned data table stayed out of the fork, its migrations with it. - let migrated: Vec = sqlx::query_scalar( - "SELECT datatable FROM datatable_migrations WHERE workspace_id = 'wm-fork-kept' ORDER BY 1", - ) - .fetch_all(&db) - .await?; - assert_eq!(migrated, vec!["byo".to_string()]); - let resp = authed( - client().post(format!("{ws}/workspaces/datatable_permissions/byo/preview")), - "SECRET_TOKEN", - ) - .json(&json!({ "enabled": true, "roles": [] })) - .send() - .await?; - assert_eq!(resp.status(), 400); - let text = resp.text().await?; - assert!(text.contains("wm-fork-kept (data table 'byo')"), "{text}"); - - // The form cannot stamp the copy either. - let resp = authed( - client().post(format!( - "http://localhost:{port}/api/w/wm-fork-kept/workspaces/edit_datatable_config" - )), - "SECRET_TOKEN", - ) - .json(&json!({ - "settings": { "datatables": { - "byo": { - "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" }, - "forked_from": { "schema": {} } - } - }} - })) - .send() - .await?; - assert_eq!(resp.status(), 200, "{}", resp.text().await?); - let copy: serde_json::Value = sqlx::query_scalar( - "SELECT datatable->'datatables'->'byo' FROM workspace_settings WHERE workspace_id = 'wm-fork-kept'", - ) - .fetch_one(&db) - .await?; - assert!(copy.get("forked_from").is_none(), "{copy}"); - - // Where the stamp exists it survives a save that omits it — the form - // round-trips configs the client trimmed — and its schema snapshot follows a - // save that carries one, which is how the schema diff records its baseline. - let save = |datatables: serde_json::Value| { - let ws = ws.clone(); - async move { - let resp = authed( - client().post(format!("{ws}/workspaces/edit_datatable_config")), - "SECRET_TOKEN", - ) - .json(&json!({ "settings": { "datatables": datatables } })) - .send() - .await - .unwrap(); - assert_eq!(resp.status(), 200, "{}", resp.text().await.unwrap()); - } - }; - let stamp = |db: Pool| async move { - let entry: serde_json::Value = sqlx::query_scalar( - "SELECT datatable->'datatables'->'byo' FROM workspace_settings WHERE workspace_id = 'test-workspace'", + let save = |w: &'static str, datatables: serde_json::Value| async move { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{w}/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", ) - .fetch_one(&db) + .json(&json!({ "settings": { "datatables": datatables } })) + .send() .await .unwrap(); - entry["forked_from"].clone() + assert_eq!(resp.status(), 200, "{}", resp.text().await.unwrap()); }; - save(json!({ "byo": { - "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" } - }})) + let stamp = |w: &'static str| { + let db = db.clone(); + async move { + let entry: serde_json::Value = sqlx::query_scalar( + "SELECT datatable->'datatables'->'byo' FROM workspace_settings WHERE workspace_id = $1", + ) + .bind(w) + .fetch_one(&db) + .await + .unwrap(); + entry["forked_from"].clone() + } + }; + let byo = + json!({ "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" } }); + // The form cannot stamp the copy... + save( + "wm-fork-kept", + json!({ "byo": { + "database": byo["database"], "forked_from": { "schema": {} } + }}), + ) .await; - assert_eq!(stamp(db.clone()).await, json!({ "schema": {} })); - save(json!({ "byo": { - "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" }, - "forked_from": { "schema": { "t": ["id"] } } - }})) + assert_eq!(stamp("wm-fork-kept").await, serde_json::Value::Null); + // ...nor take the parent's stamp away, and it may update the snapshot inside it. + save("test-workspace", json!({ "byo": byo })).await; + assert_eq!(stamp("test-workspace").await, json!({ "schema": {} })); + save( + "test-workspace", + json!({ "byo": { + "database": byo["database"], "forked_from": { "schema": { "t": ["id"] } } + }}), + ) .await; assert_eq!( - stamp(db.clone()).await, + stamp("test-workspace").await, json!({ "schema": { "t": ["id"] } }) ); @@ -550,9 +459,9 @@ async fn a_kept_original_does_not_inherit_the_clone_stamp( } /// A save carries the whole role list the drawer loaded, so it can name a tenant -/// another admin's deletion took off the role in between, and a rename can name -/// a role that stored migrations still carry in their `-- role` annotation. Both -/// are refused rather than written back. +/// another admin's deletion took off the role in between, and it can leave a +/// role stored migrations still carry in their `-- role` annotation undefined. +/// Both are refused rather than written; turning permissions off is not. #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn a_save_names_only_what_exists(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; @@ -560,22 +469,8 @@ async fn a_save_names_only_what_exists(db: Pool) -> anyhow::Result<()> let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); - sqlx::query( - r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, - ) - .bind(json!({ - "datatables": { - "main": { - "database": { "resource_type": "instance", "resource_path": "dt_main" }, - "permissions": { "enabled": true, "roles": { - "admin": { "tenants": [] }, - "analyst": { "tenants": ["u/test-user"] } - }} - } - } - })) - .execute(&db) - .await?; + plant_main(&db, "test-workspace").await; + plant_permissions(&db, MAIN_KEY, "test-workspace", &["u/test-user"]).await; sqlx::query( "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up, code_down) VALUES ('test-workspace', 'main', 1, 'add_orders', '-- role analyst\nCREATE TABLE orders ()', NULL)", @@ -616,7 +511,6 @@ async fn a_save_names_only_what_exists(db: Pool) -> anyhow::Result<()> assert_eq!(status, 400, "{text}"); assert!(text.contains("'add_orders' (role 'analyst')"), "{text}"); - // Removing the role strands the migration the same way. let (status, text) = preview(json!({ "enabled": true, "roles": [{ "name": "admin", "tenants": [] }] })) @@ -650,255 +544,3 @@ async fn a_save_names_only_what_exists(db: Pool) -> anyhow::Result<()> Ok(()) } - -/// Another workspace naming the same resource path holds a resource of its own, -/// which counts only when it resolves to the same database — a detached dev -/// workspace's cloned resource does, an unrelated workspace's same-named one -/// does not. -#[sqlx::test(migrations = "../migrations", fixtures("base"))] -async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database( - db: Pool, -) -> anyhow::Result<()> { - initialize_tracing().await; - let server = ApiServer::start(db.clone()).await?; - let port = server.addr.port(); - - sqlx::query( - "INSERT INTO workspace (id, name, owner) VALUES ('detached', 'detached', 'test-user')", - ) - .execute(&db) - .await?; - sqlx::query( - "INSERT INTO usr (workspace_id, email, username, is_admin, role) - VALUES ('detached', 'test@windmill.dev', 'test-user', true, 'Admin')", - ) - .execute(&db) - .await?; - let byo = - json!({ "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" } }); - for w in ["test-workspace", "detached"] { - sqlx::query( - "INSERT INTO workspace_settings (workspace_id, datatable) VALUES ($1, $2) - ON CONFLICT (workspace_id) DO UPDATE SET datatable = EXCLUDED.datatable", - ) - .bind(w) - .bind(json!({ "datatables": { "byo": byo } })) - .execute(&db) - .await?; - } - let create_pg = |w: &'static str, dbname: &'static str| async move { - let resp = authed( - client().post(format!("http://localhost:{port}/api/w/{w}/resources/create")), - "SECRET_TOKEN", - ) - .json(&json!({ - "path": "u/test-user/pg", - "resource_type": "postgresql", - "value": { "host": "db.example", "port": 5432, "dbname": dbname, "user": "app", "password": "pw", "sslmode": "disable" } - })) - .send() - .await - .unwrap(); - assert_eq!(resp.status(), 201, "{w}: {}", resp.text().await.unwrap()); - }; - create_pg("test-workspace", "prod").await; - create_pg("detached", "prod").await; - - let preview = || async { - let resp = authed( - client().post(format!( - "http://localhost:{port}/api/w/test-workspace/workspaces/datatable_permissions/byo/preview" - )), - "SECRET_TOKEN", - ) - .json(&json!({ "enabled": true, "roles": [] })) - .send() - .await - .unwrap(); - resp.text().await.unwrap() - }; - let text = preview().await; - assert!( - text.contains("same database: detached (data table 'byo')"), - "{text}" - ); - - // Same path, another database: not a copy. - sqlx::query( - "UPDATE resource SET value = jsonb_set(value, '{dbname}', '\"other\"') - WHERE workspace_id = 'detached' AND path = 'u/test-user/pg'", - ) - .execute(&db) - .await?; - let text = preview().await; - assert!(!text.contains("cannot be enabled"), "{text}"); - - // Another path and another login, the same database: a copy, wherever the - // resource was moved and whoever it connects as. - sqlx::query( - "UPDATE resource SET path = 'f/moved/pg', - value = jsonb_set(jsonb_set(value, '{dbname}', '\"prod\"'), '{user}', '\"postgres\"') - WHERE workspace_id = 'detached' AND path = 'u/test-user/pg'", - ) - .execute(&db) - .await?; - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables,byo,database,resource_path}', '"f/moved/pg"') - WHERE workspace_id = 'detached'"#, - ) - .execute(&db) - .await?; - let text = preview().await; - assert!( - text.contains("same database: detached (data table 'byo')"), - "{text}" - ); - - // The same rule from the other side: once `byo` is governed, the settings - // form cannot add an entry that reaches its database, in this workspace or - // another, and the pointer alone does not decide it. - let identity: String = { - // Stamped by the opt-in in the real flow; here from the same resource. - let resp = authed( - client().get(format!( - "http://localhost:{port}/api/w/test-workspace/resources/get_value_interpolated/u/test-user/pg" - )), - "SECRET_TOKEN", - ) - .send() - .await?; - assert_eq!(resp.status(), 200); - let value: serde_json::Value = resp.json().await?; - windmill_common::workspaces::physical_database_identity(&value) - }; - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables,byo,permissions}', - jsonb_build_object('enabled', true, 'roles', '{"admin": {"tenants": []}}'::jsonb, - 'physical_identity', $1::text)) - WHERE workspace_id = 'test-workspace'"#, - ) - .bind(&identity) - .execute(&db) - .await?; - let save = |w: &'static str, path: &'static str| async move { - let resp = authed( - client().post(format!( - "http://localhost:{port}/api/w/{w}/workspaces/edit_datatable_config" - )), - "SECRET_TOKEN", - ) - .json(&json!({ "settings": { "datatables": { - "door": { "database": { "resource_type": "postgresql", "resource_path": path } } - }}})) - .send() - .await - .unwrap(); - (resp.status().as_u16(), resp.text().await.unwrap()) - }; - let (status, text) = save("detached", "f/moved/pg").await; - assert_eq!(status, 400, "{text}"); - assert!( - text.contains("data table 'byo' of workspace test-workspace"), - "{text}" - ); - sqlx::query( - "UPDATE resource SET value = jsonb_set(value, '{dbname}', '\"other\"') - WHERE workspace_id = 'detached' AND path = 'f/moved/pg'", - ) - .execute(&db) - .await?; - let (status, text) = save("detached", "f/moved/pg").await; - assert_eq!(status, 200, "{text}"); - // And the resource behind that entry cannot be pointed back at it either. - let resp = authed( - client().post(format!( - "http://localhost:{port}/api/w/detached/resources/update/f/moved/pg" - )), - "SECRET_TOKEN", - ) - .json(&json!({ "value": { "host": "db.example", "port": 5432, "dbname": "prod", "user": "app", "password": "pw", "sslmode": "disable" } })) - .send() - .await?; - let status = resp.status().as_u16(); - let text = resp.text().await?; - assert_eq!(status, 400, "{text}"); - assert!(text.contains("a data table of another workspace"), "{text}"); - // Nor deleted and recreated at the path the entry still names. - let resp = authed( - client().delete(format!( - "http://localhost:{port}/api/w/detached/resources/delete/f/moved/pg" - )), - "SECRET_TOKEN", - ) - .send() - .await?; - assert_eq!(resp.status(), 200, "{}", resp.text().await?); - let resp = authed( - client().post(format!("http://localhost:{port}/api/w/detached/resources/create")), - "SECRET_TOKEN", - ) - .json(&json!({ - "path": "f/moved/pg", - "resource_type": "postgresql", - "value": { "host": "db.example", "port": 5432, "dbname": "prod", "user": "app", "password": "pw", "sslmode": "disable" } - })) - .send() - .await?; - let status = resp.status().as_u16(); - let text = resp.text().await?; - assert_eq!(status, 400, "{text}"); - assert!(text.contains("a data table of another workspace"), "{text}"); - // A second door that got past every write-time guard — a `$var:` changed - // under the resource, say — is refused where it is used. - sqlx::query( - "INSERT INTO resource (workspace_id, path, value, resource_type, created_by, edited_at) - VALUES ('detached', 'f/moved/pg', $1, 'postgresql', 'test-user', now())", - ) - .bind(json!({ "host": "db.example", "port": 5432, "dbname": "prod", "user": "postgres", "password": "pw" })) - .execute(&db) - .await?; - let resp = authed( - client().get(format!( - "http://localhost:{port}/api/w/detached/workspaces/get_datatable_table_schema?datatable_name=door&schema_name=public&table_name=t" - )), - "SECRET_TOKEN", - ) - .send() - .await?; - let status = resp.status().as_u16(); - let text = resp.text().await?; - assert_eq!(status, 401, "{text}"); - assert!( - text.contains("whose role permissions are enabled"), - "{text}" - ); - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = datatable #- '{datatables,byo,permissions}' - WHERE workspace_id = 'test-workspace'"#, - ) - .execute(&db) - .await?; - - // A second entry of this workspace on the same resource is a second door. - sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'detached'") - .execute(&db) - .await?; - sqlx::query( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables,byo2}', $1) - WHERE workspace_id = 'test-workspace'"#, - ) - .bind(&byo) - .execute(&db) - .await?; - let text = preview().await; - assert!( - text.contains("same database: test-workspace (data table 'byo2')"), - "{text}" - ); - - Ok(()) -} diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 8903bbb4ce..773e9c061c 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -1684,7 +1684,7 @@ async fn delete_user( // A username is scoped to one workspace, and so are the tenants naming it, so // the memberships are read before anything is deleted. Ordered, and before // every other write of this transaction — see - // `lock_workspace_settings_unchecked`. + // `lock_datatable_permissions_unchecked`. let memberships = sqlx::query!( "SELECT workspace_id, username FROM usr WHERE email = $1 ORDER BY workspace_id", &email_to_delete @@ -2445,7 +2445,7 @@ pub async fn delete_workspace_user_internal( // The username is free once this user's rows are gone, so a tenant left // behind would hand every role it names to whoever is invited into it next. - // First in the transaction — see `lock_workspace_settings_unchecked`. + // First in the transaction — see `lock_datatable_permissions_unchecked`. windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( w_id, &format!("u/{username_to_delete}"), @@ -2610,10 +2610,10 @@ async fn delete_workspace_user( ) -> Result { let mut tx = db.begin().await?; - // Before the `usr` row below — see `lock_workspace_settings_unchecked`. The + // Before the `usr` row below — see `lock_datatable_permissions_unchecked`. The // removal itself takes this row too; re-acquiring it inside a transaction // costs nothing. - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id).await?; + windmill_common::workspaces::lock_datatable_permissions_unchecked(&mut tx, &w_id).await?; // Locked so that the authorization below and the delete it guards see the same row. let target = sqlx::query!( diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 10d97a41e3..075d82e1ed 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -30,12 +30,16 @@ use windmill_audit::audit_oss::audit_log; use windmill_audit::ActionKind; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{ - can_use_datatable_role, get_datatable_resource_from_db, - get_datatable_resource_from_db_unchecked, DatatableAccess, ADMIN_DATATABLE_ROLE, + get_datatable_resource_from_db, get_datatable_resource_from_db_unchecked, DatatableAccess, + ADMIN_DATATABLE_ROLE, }; use windmill_common::{PgDatabase, DB}; -use crate::datatable_permissions::{connect_as_admin_unchecked, read_datatable_unchecked}; +use crate::datatable_permissions::{connect_as_admin_unchecked, usable_roles}; +use windmill_common::workspaces::{ + database_permissions_by_key, lock_database_permissions, resolve_datatable_database_unchecked, + DatabasePermissions, +}; pub(crate) fn routes() -> Router { Router::new() @@ -254,12 +258,7 @@ async fn connect_as_caller( // lists are what say who reaches which. Without permissions every member // resolves to the data table's own connection, which owns everything — so // there it is the workspace admins' to change, as the roles themselves are. - if !authed.is_admin - && !read_datatable_unchecked(db, w_id, datatable_name) - .await? - .permissions - .is_some_and(|p| p.enabled) - { + if !authed.is_admin && database_record(db, w_id, datatable_name).await?.is_none() { return Err(Error::NotAuthorized(format!( "Only an admin can manage access on data table '{datatable_name}', which has no roles" ))); @@ -406,6 +405,18 @@ async fn first_unmanageable_object( Ok(row.map(|row| format!("{schema}.{}", row.get::<_, String>(0)))) } +/// The permissions of the database the data table reaches, when they are on. +async fn database_record( + db: &DB, + w_id: &str, + datatable_name: &str, +) -> Result> { + let (_, _, key) = resolve_datatable_database_unchecked(db, w_id, datatable_name).await?; + Ok(database_permissions_by_key(db, &key) + .await? + .filter(|r| r.permissions.enabled)) +} + /// The data table roles `authed` may themselves run as. async fn usable_role_names( db: &DB, @@ -413,15 +424,16 @@ async fn usable_role_names( datatable_name: &str, authed: &ApiAuthed, ) -> Result> { - let authed_ref = authed.to_authed_ref(); - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; - Ok(match datatable.permissions.filter(|p| p.enabled) { - Some(p) => p - .roles - .iter() - .filter(|(_, role)| can_use_datatable_role(role, &authed_ref)) - .map(|(name, _)| name.clone()) - .collect(), + Ok(match database_record(db, w_id, datatable_name).await? { + Some(record) => { + usable_roles( + db, + w_id, + &record, + &DatatableAccess::Authed(authed.to_authed_ref()), + ) + .await? + } None => vec![ADMIN_DATATABLE_ROLE.to_string()], }) } @@ -470,11 +482,10 @@ async fn role_map( datatable_name: &str, admin_pg_role: &str, ) -> Result> { - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; let mut map = BTreeMap::new(); map.insert(ADMIN_DATATABLE_ROLE.to_string(), admin_pg_role.to_string()); - if let Some(permissions) = datatable.permissions.filter(|p| p.enabled) { - for (name, role) in permissions.roles { + if let Some(record) = database_record(db, w_id, datatable_name).await? { + for (name, role) in record.permissions.roles { if let Some(pg_rolename) = role.pg_rolename { map.insert(name, pg_rolename); } @@ -1045,7 +1056,9 @@ async fn apply_datatable_acl( // off the catalog and the config, and a role save running at the same time // is what changes both under it. Held to the end of this handler. let mut lock_tx = db.begin().await?; - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut lock_tx, &w_id).await?; + let (_, _, database_key) = + resolve_datatable_database_unchecked(&db, &w_id, &datatable_name).await?; + lock_database_permissions(&mut lock_tx, &database_key).await?; // Authorization first: the repair below opens a connection as the instance's // own Postgres user, which is not something a request that is about to be diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index a592fdcd57..eee9fadd1c 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -33,9 +33,10 @@ use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::utils::require_admin; use windmill_common::worker::SqlAnnotations; use windmill_common::workspaces::{ - can_use_datatable_role, datatable_database_identity, get_datatable_resource_from_db_unchecked, - physical_database_identity, DataTable, DataTableCatalogResourceType, DataTablePermissions, - ADMIN_DATATABLE_ROLE, + can_use_datatable_role_in_owner_workspace, database_permissions_by_key, + delete_database_permissions, lock_database_permissions, resolve_datatable_database_unchecked, + upsert_database_permissions, DataTable, DataTableCatalogResourceType, DataTablePermissions, + DatabasePermissions, DatatableAccess, ADMIN_DATATABLE_ROLE, }; use windmill_common::{PgDatabase, DB}; @@ -74,6 +75,13 @@ pub struct DatatablePermissionsInfo { pub roles: Vec, /// The role a script gets when it names none. pub default_role: String, + /// The workspace whose admins manage these permissions and whose principals + /// the tenants are: the one that turned them on. Absent while they are off. + #[serde(skip_serializing_if = "Option::is_none")] + pub owner_workspace_id: Option, + /// Whether the caller may change them from here: an admin of the owning + /// workspace, or a superadmin. + pub editable: bool, } #[derive(Deserialize, Debug)] @@ -175,10 +183,8 @@ pub(crate) fn quote_ident(ident: &str) -> String { /// Read a data table's config, whatever the caller is. /// -/// Authorization: performs none. What it returns is the config as stored, -/// generated role passwords included, so callers MUST have authorized the read, -/// and MUST NOT pass the value outward without -/// [`windmill_common::workspaces::redact_datatable_settings_for_export`]. +/// Authorization: performs none, for any workspace it is handed, so callers +/// MUST have authorized the read. pub(crate) async fn read_datatable_unchecked( db: &DB, w_id: &str, @@ -231,13 +237,9 @@ pub(crate) async fn ensure_instance_db_can_delegate(db: &DB, w_id: &str, datatab /// database rather than assumed from its config. pub(crate) struct AdminConnection { pub(crate) dbname: String, - /// The database this resolved to, as the config records it so a later - /// resolution can tell it has not moved. `None` for an instance database, - /// which no workspace edit can repoint. - pub(crate) database_identity: Option, - /// The database without the login, instance databases included: what every - /// other data table entry is held apart from. - pub(crate) physical_identity: String, + /// The key of the database this connection reaches, which is what its + /// permissions are stored under. + pub(crate) database_key: String, pub(crate) admin_pg_role: String, pub(crate) pg_roles: PgRoleInventory, /// Whether `PUBLIC` holds CREATE on schema `public`, i.e. every role in this @@ -349,14 +351,8 @@ pub(crate) async fn connect_as_admin_unchecked( w_id: &str, datatable_name: &str, ) -> Result<(tokio_postgres::Client, AdminConnection)> { - let db_resource = get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?; - let database_identity = (read_datatable_unchecked(db, w_id, datatable_name) - .await? - .database - .resource_type - == DataTableCatalogResourceType::Postgresql) - .then(|| datatable_database_identity(&db_resource)); - let physical_identity = physical_database_identity(&db_resource); + let (_, db_resource, database_key) = + resolve_datatable_database_unchecked(db, w_id, datatable_name).await?; let pg_db: PgDatabase = serde_json::from_value(db_resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; let dbname = pg_db.dbname.clone(); @@ -441,9 +437,9 @@ pub(crate) async fn connect_as_admin_unchecked( // own, plus the connection they were all created from. let mut own_pg_roles = vec![admin_pg_role.clone()]; own_pg_roles.extend( - read_datatable_unchecked(db, w_id, datatable_name) + database_permissions_by_key(db, &database_key) .await? - .permissions + .map(|r| r.permissions) .filter(|p| p.enabled) .into_iter() .flat_map(|p| p.roles.into_values()) @@ -455,8 +451,7 @@ pub(crate) async fn connect_as_admin_unchecked( client, AdminConnection { dbname, - database_identity, - physical_identity, + database_key, admin_pg_role, pg_roles: PgRoleInventory { existing, adoptable }, public_schema_is_open, @@ -465,44 +460,31 @@ pub(crate) async fn connect_as_admin_unchecked( )) } +/// Plan `req` against the database the data table reaches, whose permissions +/// are `old` (read under the caller's lock, or absent when none are on yet). async fn build_plan( db: &DB, w_id: &str, datatable_name: &str, + old: Option<&DataTablePermissions>, req: &SetDatatablePermissions, -) -> Result<(tokio_postgres::Client, RolePlan)> { +) -> Result<(tokio_postgres::Client, AdminConnection, RolePlan)> { require_datatable_permissions_license().await?; - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; - ensure_save_names_what_exists( - db, - w_id, - datatable_name, - datatable.permissions.as_ref(), - req, - ) - .await?; + ensure_save_names_what_exists(db, w_id, datatable_name, old, req).await?; let (client, conn) = connect_as_admin_unchecked(db, w_id, datatable_name).await?; - let plan = crate::datatable_permissions_oss::plan_role_changes( - w_id, - datatable_name, + let mut plan = crate::datatable_permissions_oss::plan_role_changes( + &conn.database_key, &conn.dbname, &conn.admin_pg_role, - datatable.permissions.as_ref(), + old, req, &conn.pg_roles, conn.public_schema_is_open, &conn.default_acl_rules, )?; - let mut plan = plan; - // Stamped from the connection the roles are about to be created through, so - // a resolution that lands anywhere else later can refuse. - plan.permissions.database_identity = conn.database_identity; - plan.permissions.physical_identity = Some(conn.physical_identity); if !req.enabled { // Opting out is never refused; what it strands is said out loud. - let roles: HashSet<&str> = datatable - .permissions - .as_ref() + let roles: HashSet<&str> = old .map(|p| p.roles.keys().map(String::as_str)) .into_iter() .flatten() @@ -517,24 +499,26 @@ async fn build_plan( )); } } - Ok((client, plan)) + Ok((client, conn, plan)) } -/// The connection and statements that drop a deleted data table's roles, giving -/// its objects back to admin first. +/// The connection and statements that drop every role of the database a data +/// table reaches, giving their objects back to admin first, with the key of that +/// database. /// /// Resolved separately from being run: resolving needs the data table's config, -/// which the save is about to remove, while running is irreversible and must not -/// happen until that save has committed. -pub(crate) type PlannedRoleDrop = (tokio_postgres::Client, RolePlan); +/// which a deletion may be about to remove, while running is irreversible and +/// must not happen until that deletion has committed. +pub(crate) type PlannedRoleDrop = (tokio_postgres::Client, RolePlan, String); -/// Plan the removal of every Postgres role of a data table that is being -/// deleted, against the config as it still stands. +/// Plan the removal of every Postgres role of the database a data table reaches, +/// for a database that is going away with the data table — a workspace being +/// deleted, a fork's clone being dropped. /// -/// A data table whose database is already unreachable must still be removable -/// from the config, so a failure here is logged and the deletion goes ahead -/// without a plan — leaving roles that only a `DROP ROLE` by hand will clear. -pub(crate) async fn plan_drop_of_deleted_datatable( +/// A database that is already unreachable must not block the deletion, so a +/// failure here is logged and the deletion goes ahead without a plan — leaving +/// roles that only a `DROP ROLE` by hand will clear. +pub(crate) async fn plan_drop_of_datatable_roles( db: &DB, w_id: &str, datatable_name: &str, @@ -546,66 +530,54 @@ pub(crate) async fn plan_drop_of_deleted_datatable( renames: vec![], }; let res = async { - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; - if !datatable - .permissions - .as_ref() - .is_some_and(|p| p.enabled && p.roles.len() > 1) - { - return Ok(None); - } - build_plan(db, w_id, datatable_name, &req).await.map(Some) + let (_, _, key) = resolve_datatable_database_unchecked(db, w_id, datatable_name).await?; + let Some(record) = database_permissions_by_key(db, &key) + .await? + .filter(|r| r.permissions.enabled && r.permissions.roles.len() > 1) + else { + return Ok::<_, Error>(None); + }; + let (client, _, plan) = + build_plan(db, w_id, datatable_name, Some(&record.permissions), &req).await?; + Ok(Some((client, plan, key))) } .await; match res { Ok(planned) => planned, Err(e) => { tracing::error!( - "Could not plan dropping the Postgres roles of deleted data table {datatable_name} in {w_id}: {e:#}" + "Could not plan dropping the Postgres roles behind data table {datatable_name} in {w_id}: {e:#}" ); None } } } -/// The Postgres logins the workspace's config currently names. -fn pg_rolenames_in_use(settings: Option<&serde_json::Value>) -> HashSet { - settings - .and_then(|s| s.get("datatables")) - .and_then(|d| d.as_object()) - .map(|datatables| { - datatables - .values() - .filter_map(|dt| dt.pointer("/permissions/roles")?.as_object()) - .flat_map(|roles| roles.values()) - .filter_map(|role| role.get("pg_rolename")?.as_str()) - .map(str::to_string) - .collect() - }) - .unwrap_or_default() -} - -/// Destroy the roles a committed config stopped naming. +/// Destroy the roles a committed save stopped naming. /// -/// Asked under the settings row, one role at a time: a role the config names -/// again — a data table recreated under the same name, or a save that put the -/// role back — is left alone, and one it does not name is dropped whatever else -/// has changed in the meantime. Nobody else can be planning against these -/// between the question and the answer, since that row is what every save takes -/// first. +/// Asked under the database's permissions row, one role at a time: a role the +/// row names again — a save that put the role back — is left alone, and one it +/// does not name is dropped whatever else has changed in the meantime. Nobody +/// else can be planning against these between the question and the answer, +/// since that row is what every save takes first. /// -/// Best-effort: a role outliving its config is recoverable, dropping one a live -/// data table depends on is not. -async fn drop_roles_the_config_no_longer_names( +/// Best-effort: a role outliving its row is recoverable, dropping one a live +/// role depends on is not. +async fn drop_roles_the_record_no_longer_names( db: &DB, - w_id: &str, + database_key: &str, client: &mut tokio_postgres::Client, statements: &[&PlannedStatement], ) -> Result<()> { let mut tx = db.begin().await?; - let settings = - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, w_id).await?; - let in_use = pg_rolenames_in_use(settings.as_ref()); + let in_use: HashSet = lock_database_permissions(&mut tx, database_key) + .await? + .map(|r| r.permissions) + .filter(|p| p.enabled) + .into_iter() + .flat_map(|p| p.roles.into_values()) + .filter_map(|role| role.pg_rolename) + .collect(); let to_run: Vec<&PlannedStatement> = statements .iter() .filter(|s| { @@ -623,10 +595,9 @@ async fn drop_roles_the_config_no_longer_names( attempted.sort(); attempted.dedup(); let ran = async { - // The settings row is held for as long as these run, and they run on a - // database this workspace does not control — a lock held there, or a - // role with a great deal to reassign, would otherwise stall every save - // of every data table in the workspace behind it. + // The row is held for as long as these run, and they run on a database + // Windmill does not control — a lock held there, or a role with a great + // deal to reassign, would otherwise stall every save behind it. client .batch_execute("SET statement_timeout = '60s'") .await @@ -640,9 +611,9 @@ async fn drop_roles_the_config_no_longer_names( } .await; // Named here rather than by the caller, and on every way out: which of - // them were skipped because the config names them again is only known - // under the lock above, and whatever failed, the config that stopped - // naming these has committed and nothing comes back for them. + // them were skipped because the row names them again is only known under + // the lock above, and whatever failed, the save that stopped naming these + // has committed and nothing comes back for them. ran.map_err(|e| { Error::ExecutionErr(format!("{e}. Roles left behind: {}", attempted.join(", "))) })?; @@ -651,20 +622,54 @@ async fn drop_roles_the_config_no_longer_names( Ok(()) } -/// Drop the roles of a data table that was deleted, once the config saying so -/// has committed. +/// Drop the roles of a database that is going away, once the deletion saying so +/// has committed, and forget its permissions. pub(crate) async fn run_planned_drop( db: &DB, w_id: &str, datatable_name: &str, - (mut client, plan): PlannedRoleDrop, + planned: PlannedRoleDrop, ) { + let key = planned.2.clone(); + if run_planned_drop_keeping_record(db, w_id, datatable_name, planned).await { + forget_database_permissions(db, &key).await; + } +} + +/// Drop the roles a plan names, leaving the database's permissions row in place: +/// with its logins gone every role is refused and `admin` stays the owning +/// workspace's alone, which is the safe state for a database that was meant to go +/// and did not. Returns whether the roles were dropped. +pub(crate) async fn run_planned_drop_keeping_record( + db: &DB, + w_id: &str, + datatable_name: &str, + (mut client, plan, database_key): PlannedRoleDrop, +) -> bool { let statements: Vec<&PlannedStatement> = plan.statements.iter().collect(); - if let Err(e) = drop_roles_the_config_no_longer_names(db, w_id, &mut client, &statements).await - { - tracing::error!( - "Could not drop the Postgres roles of deleted data table {datatable_name} in {w_id}: {e:#}" - ); + match drop_roles_the_record_no_longer_names(db, &database_key, &mut client, &statements).await { + Ok(()) => true, + Err(e) => { + tracing::error!( + "Could not drop the Postgres roles behind data table {datatable_name} in {w_id}: {e:#}" + ); + false + } + } +} + +/// Forget a database's permissions: for a database that is gone, roles and all. +pub(crate) async fn forget_database_permissions(db: &DB, database_key: &str) { + let forgotten = async { + let mut tx = db.begin().await?; + lock_database_permissions(&mut tx, database_key).await?; + delete_database_permissions(&mut tx, database_key).await?; + tx.commit().await?; + Ok::<(), Error>(()) + } + .await; + if let Err(e) = forgotten { + tracing::error!("Could not forget the permissions of {database_key}: {e:#}"); } } @@ -697,18 +702,52 @@ async fn run_statements( }) } -async fn get_datatable_permissions( - authed: ApiAuthed, - Extension(db): Extension, - Path((w_id, datatable_name)): Path<(String, String)>, -) -> JsonResult { +/// The permissions the caller may manage from `w_id` for the database a data +/// table reaches: an admin of the owning workspace, or a superadmin. A record +/// that does not exist yet is created by the workspace that opts in — not from a +/// fork, whose data table is either a copy of a database another workspace owns +/// or a clone the fork can drop, roles and all. +async fn ensure_can_manage_permissions( + db: &DB, + authed: &ApiAuthed, + w_id: &str, + record: Option<&DatabasePermissions>, + enabling: bool, +) -> Result<()> { require_admin(authed.is_admin, &authed.username)?; - let datatable = read_datatable_unchecked(&db, &w_id, &datatable_name).await?; - let permissions = datatable.permissions.unwrap_or_default(); - let default_role = permissions.default_role().to_string(); - Ok(Json(DatatablePermissionsInfo { + match record { + Some(record) if record.owner_workspace_id != w_id => { + if !windmill_common::auth::is_super_admin_email(db, &authed.email).await? { + return Err(Error::NotAuthorized(format!( + "The permissions of this database are managed from workspace '{}', \ + which turned them on.", + record.owner_workspace_id + ))); + } + } + Some(_) => {} + None => { + if enabling && crate::workspaces_extra::workspace_is_fork(db, w_id).await? { + return Err(Error::BadRequest( + "Data table permissions cannot be enabled from a fork workspace: its data \ + table points either at the database of the workspace it was forked from, \ + which is where to set them, or at a copy the fork can drop." + .to_string(), + )); + } + } + } + Ok(()) +} + +fn permissions_info( + record: Option<&DatabasePermissions>, + editable: bool, +) -> DatatablePermissionsInfo { + let permissions = record.map(|r| r.permissions.clone()).unwrap_or_default(); + DatatablePermissionsInfo { enabled: permissions.enabled, - default_role, + default_role: permissions.default_role().to_string(), roles: permissions .roles .into_iter() @@ -718,7 +757,23 @@ async fn get_datatable_permissions( pg_rolename: role.pg_rolename, }) .collect(), - })) + owner_workspace_id: record.map(|r| r.owner_workspace_id.clone()), + editable, + } +} + +async fn get_datatable_permissions( + authed: ApiAuthed, + Extension(db): Extension, + Path((w_id, datatable_name)): Path<(String, String)>, +) -> JsonResult { + require_admin(authed.is_admin, &authed.username)?; + let (_, _, key) = resolve_datatable_database_unchecked(&db, &w_id, &datatable_name).await?; + let record = database_permissions_by_key(&db, &key).await?; + let editable = ensure_can_manage_permissions(&db, &authed, &w_id, record.as_ref(), false) + .await + .is_ok(); + Ok(Json(permissions_info(record.as_ref(), editable))) } /// Refuse a data table operation that would run as a role `authed` may not use. @@ -734,8 +789,11 @@ pub(crate) async fn ensure_can_use_datatable_role( authed: &ApiAuthed, context: &str, ) -> Result<()> { - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; - let Some(permissions) = datatable.permissions.filter(|p| p.enabled) else { + let (_, _, key) = resolve_datatable_database_unchecked(db, w_id, datatable_name).await?; + let Some(record) = database_permissions_by_key(db, &key) + .await? + .filter(|r| r.permissions.enabled) + else { // Unpermissioned: only the built-in role exists, and everyone reaches it. return match role { Some(role) if role != ADMIN_DATATABLE_ROLE => Err(Error::BadRequest(format!( @@ -744,13 +802,21 @@ pub(crate) async fn ensure_can_use_datatable_role( _ => Ok(()), }; }; - let role_name = role.unwrap_or_else(|| permissions.default_role()); - let entry = permissions.roles.get(role_name).ok_or_else(|| { + let role_name = role.unwrap_or_else(|| record.permissions.default_role()); + let entry = record.permissions.roles.get(role_name).ok_or_else(|| { Error::NotFound(format!( "{context} names role '{role_name}', which is not defined on data table '{datatable_name}'" )) })?; - if !can_use_datatable_role(entry, &authed.to_authed_ref()) { + let allowed = can_use_datatable_role_in_owner_workspace( + db, + &record.owner_workspace_id, + w_id, + entry, + &DatatableAccess::Authed(authed.to_authed_ref()), + ) + .await?; + if !allowed { return Err(Error::NotAuthorized(format!( "{context} runs as role '{role_name}' of data table '{datatable_name}', which you are not allowed to use" ))); @@ -758,298 +824,28 @@ pub(crate) async fn ensure_can_use_datatable_role( Ok(()) } -/// Permissions are turned on where this data table is the only one reaching the -/// database: its workspace is not a fork, no fork below holds a copy of it, and -/// no other data table entry, in any workspace, reaches the same database. -/// -/// A fork's data table is either a copy pointing at the database of the workspace -/// it was forked from, where roles created in the fork would hold grants that -/// workspace's own config does not name, or a clone whose whole database the fork -/// can drop, taking the roles with it. In the other direction, a fork made while -/// the data table was unpermissioned carries a verbatim copy of it, and every -/// member of that fork — including members this workspace does not have — would -/// keep reaching the database through the copy's own connection, which owns -/// everything in it. A dev workspace detached from this one keeps such a copy -/// without being a fork any more, which is what the last check is for. Another -/// entry naming the same instance database is exact; a resource-backed entry -/// holds a resource of its own, under whatever path, so it counts when that -/// resource resolves to the same host, port, database and user — which is what -/// a detached copy's cloned resource does, wherever it was moved since. -/// -/// Archived workspaces count. Archiving keeps the members, their session tokens -/// and the settings, and nothing on the job path checks the flag, so an archived -/// fork reaches the database exactly as a live one does. Only a permanent -/// deletion, or removing the copy, takes that away. The shell a rename archives -/// is left with no data tables at all, so it never counts. -/// -/// All three are properties of the opt-in, so only the save that turns -/// permissions on is checked. Forks made afterwards never receive a permissioned -/// data table (see the strip in the fork creation), and a save that edits the -/// roles of a live config must keep working while they exist — revoking a tenant -/// above all. Turning permissions off is never refused. -/// -/// The save calls this under the settings row lock, which fork creation takes on -/// the parent before copying its settings: a fork mid-creation has either -/// committed, and is listed here, or copies the config after the opt-in landed. -async fn refuse_enabling_permissions_over_shared_access( +/// The roles of a database `access`, made from `w_id`, may run as. +pub(crate) async fn usable_roles( db: &DB, w_id: &str, - datatable_name: &str, - enabled: bool, -) -> Result<()> { - if !enabled { - return Ok(()); - } - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; - if datatable.permissions.as_ref().is_some_and(|p| p.enabled) { - return Ok(()); - } - if crate::workspaces_extra::workspace_is_fork(db, w_id).await? { - return Err(Error::BadRequest( - "Data table permissions cannot be enabled from a fork workspace: a fork's data \ - table points either at the database of the workspace it was forked from, where \ - roles created here would be invisible to that workspace's own configuration, or \ - at a copy the fork can drop. Set them where the data table belongs. Disabling \ - them here is allowed." - .to_string(), - )); - } - let database = serde_json::to_value(&datatable.database) - .map_err(|e| Error::internal_err(format!("Failed to serialize the database: {e}")))?; - let describe = |workspace_id: &str, name: &str, deleted: bool| { - format!( - "{workspace_id}{} (data table '{name}')", - if deleted { ", archived" } else { "" } - ) - }; - let forks = windmill_common::workspaces::list_fork_descendants(db, w_id).await?; - if !forks.is_empty() { - // A clone (`forked_from`) points at a database of its own and does not - // count. It is not told apart by the pointer alone: cloning a - // resource-backed data table rewrites the cloned resource, not the path - // the entry names, so the pointer still equals the parent's. - let copies = sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!", w.deleted AS "deleted!" - FROM workspace_settings ws - JOIN workspace w ON w.id = ws.workspace_id, - jsonb_each(ws.datatable->'datatables') dt - WHERE ws.workspace_id = ANY($1) - AND dt.value->'database' = $2 - AND dt.value->'forked_from' IS NULL - ORDER BY ws.workspace_id, dt.key"#, - &forks[..], - database, - ) - .fetch_all(db) - .await?; - if !copies.is_empty() { - let copies: Vec = copies - .into_iter() - .map(|c| describe(&c.workspace_id, &c.name, c.deleted)) - .collect(); - return Err(Error::BadRequest(format!( - "Data table permissions cannot be enabled while a fork of this workspace holds a \ - copy of the data table pointing at the same database: {}. Its members would \ - keep reaching it through the copy's own connection, as every role at once — an \ - archived fork included, since archiving keeps its members. Remove the data \ - table from the fork, or delete the fork permanently, first.", - copies.join(", ") - ))); - } - } - let others = entries_reaching(db, w_id, datatable_name, &datatable.database).await?; - if !others.is_empty() { - return Err(Error::BadRequest(format!( - "Data table permissions cannot be enabled while another data table reaches the \ - same database: {}. Its users would keep reaching it through that data table's \ - own connection, as every role at once — an archived workspace included, since \ - archiving keeps its members. Remove that data table, or delete the workspace \ - permanently, first.", - others.join(", ") - ))); - } - Ok(()) -} - -/// `[, archived] (data table '')` for every data table entry on -/// the instance, other than `(w_id, datatable_name)`, that reaches `database` — -/// this workspace's other entries included: a second entry on the same -/// resource is a second door. -/// -/// An instance database is matched by name. A resource-backed entry reaches -/// wherever its resource points, whatever path it names and whichever login it -/// carries, so it is matched by the resource's host, port and database. Read -/// from the stored resource rows -/// in one query: a field that is a `$var:` / `$res:` reference is only resolved -/// — a per-row read, and a secret backend call where the workspace uses one — -/// when every plain field already agrees, so the loop that decrypts runs for -/// candidates that can match and not for every data table on the instance. A -/// resource that no longer exists reaches nothing. -async fn entries_reaching( - db: &DB, - w_id: &str, - datatable_name: &str, - database: &windmill_common::workspaces::DataTableDatabase, + record: &DatabasePermissions, + access: &DatatableAccess<'_>, ) -> Result> { - let pointer = serde_json::to_value(database) - .map_err(|e| Error::internal_err(format!("Failed to serialize the database: {e}")))?; - let describe = |workspace_id: &str, name: &str, deleted: bool| { - format!( - "{workspace_id}{} (data table '{name}')", - if deleted { ", archived" } else { "" } + let mut usable = Vec::new(); + for (name, role) in record.permissions.roles.iter() { + if can_use_datatable_role_in_owner_workspace( + db, + &record.owner_workspace_id, + w_id, + role, + access, ) - }; - let entries = sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!", w.deleted AS "deleted!", - dt.value->'database' AS "database!", r.value AS "resource?" - FROM workspace_settings ws - JOIN workspace w ON w.id = ws.workspace_id - CROSS JOIN LATERAL jsonb_each(ws.datatable->'datatables') dt - LEFT JOIN resource r ON r.workspace_id = ws.workspace_id - AND dt.value->'database'->>'resource_type' <> 'instance' - AND r.path = dt.value->'database'->>'resource_path' - WHERE NOT (ws.workspace_id = $1 AND dt.key = $2) - AND jsonb_typeof(dt.value->'database') = 'object' - ORDER BY ws.workspace_id, dt.key"#, - w_id, - datatable_name, - ) - .fetch_all(db) - .await?; - let mut reaching = Vec::new(); - match database.resource_type { - DataTableCatalogResourceType::Instance => { - reaching.extend( - entries - .iter() - .filter(|o| o.database == pointer) - .map(|o| describe(&o.workspace_id, &o.name, o.deleted)), - ); - } - DataTableCatalogResourceType::Postgresql => { - let ours = get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?; - let identity = physical_database_identity(&ours); - let is_reference = |v: &serde_json::Value| { - v.as_str() - .is_some_and(|s| s.starts_with("$var:") || s.starts_with("$res:")) - }; - for o in entries.iter() { - let Some(raw) = o.resource.as_ref() else { - continue; - }; - // A value that is not an object — a `$res:` string, say — only - // says what it reaches once resolved. - let mut references = !raw.is_object(); - let mut plain_fields_agree = true; - for field in PHYSICAL_DATABASE_FIELDS { - let theirs = raw.get(field).unwrap_or(&serde_json::Value::Null); - if is_reference(theirs) { - references = true; - } else if !references - && theirs != ours.get(field).unwrap_or(&serde_json::Value::Null) - { - plain_fields_agree = false; - } - } - if !plain_fields_agree { - continue; - } - if !references { - reaching.push(describe(&o.workspace_id, &o.name, o.deleted)); - continue; - } - let resolved = - match get_datatable_resource_from_db_unchecked(db, &o.workspace_id, &o.name) - .await - { - Ok(resolved) => resolved, - Err(Error::NotFound(_)) => continue, - // Not resolving is not proof of not reaching: refused, and - // named. - Err(e) => { - return Err(Error::BadRequest(format!( - "Whether {} reaches the same database could not be checked \ - ({e}). Remove that data table first.", - describe(&o.workspace_id, &o.name, o.deleted) - ))) - } - }; - if physical_database_identity(&resolved) == identity { - reaching.push(describe(&o.workspace_id, &o.name, o.deleted)); - } - } + .await? + { + usable.push(name.clone()); } } - Ok(reaching) -} - -/// The fields [`physical_database_identity`] hashes. -const PHYSICAL_DATABASE_FIELDS: [&str; 3] = ["host", "port", "dbname"]; - -/// Refuse a data table entry that reaches a database another data table governs. -/// -/// The opt-in refuses while any other entry reaches the database; this is the -/// same rule from the other side, for the settings form that adds an entry or -/// points one elsewhere. Without it a second entry on a governed database is a -/// second door, open to every member as the owning connection. Governed entries -/// are matched by the identity their opt-in stamped, so nothing of theirs is -/// resolved; the entry being saved is resolved once, and must resolve. -pub(crate) async fn refuse_reaching_a_governed_database( - db: &DB, - w_id: &str, - datatable_name: &str, - database: &windmill_common::workspaces::DataTableDatabase, -) -> Result<()> { - let hit = match database.resource_type { - DataTableCatalogResourceType::Instance => { - let pointer = serde_json::to_value(database).map_err(|e| { - Error::internal_err(format!("Failed to serialize the database: {e}")) - })?; - sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!" - FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt - WHERE NOT (ws.workspace_id = $1 AND dt.key = $2) - AND COALESCE((dt.value->'permissions'->>'enabled')::boolean, false) - AND dt.value->'database' = $3 - ORDER BY ws.workspace_id, dt.key"#, - w_id, - datatable_name, - pointer, - ) - .fetch_optional(db) - .await? - .map(|g| (g.workspace_id, g.name)) - } - DataTableCatalogResourceType::Postgresql => { - // The stored config is what the form is about to replace, so the entry - // is resolved from the resource it names rather than from the config. - let resource = windmill_common::workspaces::transform_json_value_unchecked( - &serde_json::Value::String(format!("$res:{}", database.resource_path)), - w_id, - db, - ) - .await?; - windmill_common::workspaces::governed_datatable_reaching( - db, - &physical_database_identity(&resource), - &[(w_id, datatable_name)], - ) - .await? - } - }; - if let Some((gw, gname)) = hit { - return Err(Error::BadRequest(format!( - "Data table '{datatable_name}' would reach the database of {}, whose role \ - permissions are enabled: every member would reach it through this data table's \ - own connection, as every role at once.", - if gw == w_id { - format!("data table '{gname}'") - } else { - format!("data table '{gname}' of workspace {gw}") - } - ))); - } - Ok(()) + Ok(usable) } /// Refuse a save that names something that no longer exists: a tenant whose @@ -1209,24 +1005,28 @@ async fn list_usable_datatable_roles( Extension(db): Extension, Path((w_id, datatable_name)): Path<(String, String)>, ) -> JsonResult { - let datatable = read_datatable_unchecked(&db, &w_id, &datatable_name).await?; - let Some(permissions) = datatable.permissions.filter(|p| p.enabled) else { + let (_, _, key) = resolve_datatable_database_unchecked(&db, &w_id, &datatable_name).await?; + let Some(record) = database_permissions_by_key(&db, &key) + .await? + .filter(|r| r.permissions.enabled) + else { return Ok(Json(UsableDatatableRoles { enabled: false, roles: vec![], default_role: ADMIN_DATATABLE_ROLE.to_string(), })); }; - let authed_ref = authed.to_authed_ref(); + let roles = usable_roles( + &db, + &w_id, + &record, + &DatatableAccess::Authed(authed.to_authed_ref()), + ) + .await?; Ok(Json(UsableDatatableRoles { enabled: true, - default_role: permissions.default_role().to_string(), - roles: permissions - .roles - .iter() - .filter(|(_, role)| can_use_datatable_role(role, &authed_ref)) - .map(|(name, _)| name.clone()) - .collect(), + default_role: record.permissions.default_role().to_string(), + roles, })) } @@ -1236,12 +1036,19 @@ async fn preview_datatable_permissions( Path((w_id, datatable_name)): Path<(String, String)>, Json(req): Json, ) -> JsonResult { - require_admin(authed.is_admin, &authed.username)?; - // Refused here too: the preview connects to the database and reads its roles, - // and offering a plan that the save will not run is its own kind of wrong. - refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled) - .await?; - let (_client, plan) = build_plan(&db, &w_id, &datatable_name, &req).await?; + let (_, _, key) = resolve_datatable_database_unchecked(&db, &w_id, &datatable_name).await?; + let record = database_permissions_by_key(&db, &key).await?; + // Refused here too: offering a plan that the save will not run is its own + // kind of wrong. + ensure_can_manage_permissions(&db, &authed, &w_id, record.as_ref(), req.enabled).await?; + let (_client, _, plan) = build_plan( + &db, + &w_id, + &datatable_name, + record.as_ref().map(|r| &r.permissions), + &req, + ) + .await?; Ok(Json(DatatablePermissionsPreview { statements: plan.statements.into_iter().map(|s| s.display).collect(), warnings: plan.warnings, @@ -1255,16 +1062,21 @@ async fn set_datatable_permissions( Json(req): Json, ) -> Result { require_admin(authed.is_admin, &authed.username)?; + let (_, _, key) = resolve_datatable_database_unchecked(&db, &w_id, &datatable_name).await?; - // Reading the config, planning against it, running the plan and persisting - // it are one operation: interleaved with another save, or with the removal - // of a principal some role names as a tenant, this would store a block it - // computed before the other committed. The settings row is what everything - // touching that config takes, so taking it here is what serializes them. + // Reading the permissions, planning against them, running the plan and + // persisting them are one operation: interleaved with another save, or with + // the removal of a principal some role names as a tenant, this would store + // roles it computed before the other committed. The database's row is what + // everything touching its permissions takes, so taking it here is what + // serializes them — whether or not the row exists yet. let mut tx = db.begin().await?; - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id).await?; - refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled) - .await?; + let record = lock_database_permissions(&mut tx, &key).await?; + ensure_can_manage_permissions(&db, &authed, &w_id, record.as_ref(), req.enabled).await?; + let owner_workspace_id = record + .as_ref() + .map(|r| r.owner_workspace_id.clone()) + .unwrap_or_else(|| w_id.clone()); // The roles about to be created are handed privileges by this connection, // which cannot pass on what it holds without the grant option. @@ -1272,12 +1084,19 @@ async fn set_datatable_permissions( // The plan is rebuilt here rather than trusted from the preview: the client // never gets to choose what runs against the database. - let (mut client, plan) = build_plan(&db, &w_id, &datatable_name, &req).await?; + let (mut client, _, plan) = build_plan( + &db, + &w_id, + &datatable_name, + record.as_ref().map(|r| &r.permissions), + &req, + ) + .await?; - // Creating and renaming roles is committed before the config: a Windmill-side - // failure after this point leaves roles the config does not know about, which + // Creating and renaming roles is committed before the row: a Windmill-side + // failure after this point leaves roles the row does not know about, which // the next plan adopts (it reads `pg_roles`), whereas the reverse order would - // leave the config naming roles that were never created. Dropping one has no + // leave the row naming roles that were never created. Dropping one has no // such way back, so those wait below — except where this save gives the freed // name to another role, which only works in one order. let keeps_the_name = |statement: &PlannedStatement| { @@ -1295,26 +1114,10 @@ async fn set_datatable_permissions( .partition(|s| s.drops_role.is_some() && !keeps_the_name(s)); run_statements(&mut client, &immediate).await?; - let permissions = serde_json::to_value(&plan.permissions) - .map_err(|e| Error::internal_err(format!("Failed to serialize permissions: {e}")))?; - - // Written at the permissions path only, so a concurrent edit of the data - // table's own settings is not clobbered. - let updated = sqlx::query_scalar!( - "UPDATE workspace_settings - SET datatable = jsonb_set(datatable, ARRAY['datatables', $2, 'permissions'], $3) - WHERE workspace_id = $1 AND datatable->'datatables' ? $2 - RETURNING workspace_id", - &w_id, - &datatable_name, - permissions, - ) - .fetch_optional(&mut *tx) - .await?; - if updated.is_none() { - return Err(Error::NotFound(format!( - "Data table '{datatable_name}' not found" - ))); + if req.enabled { + upsert_database_permissions(&mut tx, &key, &owner_workspace_id, &plan.permissions).await?; + } else { + delete_database_permissions(&mut tx, &key).await?; } audit_log( @@ -1327,6 +1130,7 @@ async fn set_datatable_permissions( Some( [ ("datatable", datatable_name.as_str()), + ("database", key.as_str()), ("enabled", if req.enabled { "true" } else { "false" }), ] .into(), @@ -1336,24 +1140,21 @@ async fn set_datatable_permissions( tx.commit().await?; - // What the config no longer names, now that it says so. A failure here is the - // end of the line for these logins: the config that named them has committed, - // so no later plan diffs against them and nothing will try again. Say which - // ones, since dropping them is now a database administrator's job. + // What the row no longer names, now that it says so. A failure here is the + // end of the line for these logins: the save that stopped naming them has + // committed, so no later plan diffs against them and nothing will try again. + // Say which ones, since dropping them is now a database administrator's job. if !deferred.is_empty() { - drop_roles_the_config_no_longer_names(&db, &w_id, &mut client, &deferred) + drop_roles_the_record_no_longer_names(&db, &key, &mut client, &deferred) .await .map_err(|e| { Error::ExecutionErr(format!( - "Permissions of data table {datatable_name} were saved, but the Postgres \ - logins they no longer name could not be removed: {e}. Saving again will not \ - retry them — the config no longer names them, so they have to be dropped by \ - hand." + "The permissions were saved, but some roles could not be dropped: {e}" )) })?; } Ok(format!( - "Updated permissions of data table {datatable_name}" + "Permissions of data table {datatable_name} updated" )) } diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs index b3ad1ee253..fc3ac96c03 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs @@ -28,8 +28,7 @@ use { #[cfg(not(all(feature = "private", feature = "enterprise")))] pub(crate) fn plan_role_changes( - _w_id: &str, - _datatable: &str, + _database_key: &str, _dbname: &str, _admin_pg_role: &str, _old: Option<&DataTablePermissions>, diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 437a6c9bc1..a105cec815 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -45,12 +45,11 @@ use windmill_common::workspaces::GitRepositorySettings; #[cfg(feature = "enterprise")] use windmill_common::workspaces::WorkspaceDeploymentUISettings; use windmill_common::workspaces::{ - can_use_datatable_role, check_deploy_rules, check_user_against_rule, - get_datatable_resource_from_db, get_datatable_resource_from_db_unchecked, - redact_datatable_settings_for_export, validate_dev_workspace_id, validate_fork_workspace_id, - validate_workspace_name, DataTable, DataTableCatalogResourceType, DataTableForkBehavior, - DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, RuleCheckResult, - WorkspaceGitSyncSettings, ADMIN_DATATABLE_ROLE, DEV_WORKSPACE_LOCK_RULE_NAME, + check_deploy_rules, check_user_against_rule, get_datatable_resource_from_db, + get_datatable_resource_from_db_unchecked, validate_dev_workspace_id, + validate_fork_workspace_id, validate_workspace_name, DataTable, DataTableCatalogResourceType, + DataTableForkBehavior, DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, + RuleCheckResult, WorkspaceGitSyncSettings, ADMIN_DATATABLE_ROLE, DEV_WORKSPACE_LOCK_RULE_NAME, }; use windmill_common::workspaces::{Ducklake, DucklakeCatalogResourceType}; use windmill_common::PgDatabase; @@ -364,8 +363,6 @@ pub struct WorkspacePublicSettings { pub deploy_ui: Option, #[serde(skip_serializing_if = "Option::is_none")] pub large_file_storage: Option, - /// Carries each data table role's generated login as stored, so it only - /// leaves the server through `redact_datatable_settings_for_export`. #[serde(skip_serializing_if = "Option::is_none")] pub datatable: Option, } @@ -1117,7 +1114,6 @@ async fn get_settings( if let Some(git_sync) = settings.git_sync.as_mut() { redact_git_sync_webhook_secrets(git_sync); } - settings.datatable = redact_datatable_settings_for_export(settings.datatable); Ok(Json(settings)) } @@ -1154,12 +1150,11 @@ async fn get_public_settings( .await .map_err(|e| Error::internal_err(format!("getting public settings: {e:#}")))?; - let mut settings = not_found_if_none(settings, "workspace settings", &w_id)?; + let settings = not_found_if_none(settings, "workspace settings", &w_id)?; tx.commit().await?; // Every workspace member reads this one, so the generated role logins go // through the same redaction as the admin settings and the tarball. - settings.datatable = redact_datatable_settings_for_export(settings.datatable); Ok(Json(settings)) } @@ -2452,42 +2447,38 @@ async fn list_datatable_tables( } /// Which roles the caller may use on each data table of the workspace, and the -/// one they get by default. Read in one go: the tree lists every data table, and -/// this is config only, so it costs a single query rather than one per table. +/// one they get by default. The tree lists every data table, so each one is +/// resolved to its database and its permissions read; a data table that does not +/// resolve reports the default alone. async fn list_datatable_roles( db: &DB, authed: &ApiAuthed, w_id: &str, ) -> Result, String)>> { - let Some(datatables) = sqlx::query_scalar!( - "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", - w_id - ) - .fetch_optional(db) - .await? - .flatten() - .and_then(|v| serde_json::from_value::>(v).ok()) else { - return Ok(HashMap::new()); - }; - - let authed_ref = authed.to_authed_ref(); - Ok(datatables - .into_iter() - .map(|(name, dt)| { - let info = match dt.permissions.filter(|p| p.enabled) { - Some(p) => ( - p.roles - .iter() - .filter(|(_, role)| can_use_datatable_role(role, &authed_ref)) - .map(|(role_name, _)| role_name.clone()) - .collect(), - p.default_role().to_string(), - ), - None => (vec![], ADMIN_DATATABLE_ROLE.to_string()), - }; - (name, info) - }) - .collect()) + let names = list_datatable_names(db, w_id).await?; + let access = DatatableAccess::Authed(authed.to_authed_ref()); + let mut roles = HashMap::new(); + for name in names { + let record = match windmill_common::workspaces::resolve_datatable_database_unchecked( + db, w_id, &name, + ) + .await + { + Ok((_, _, key)) => windmill_common::workspaces::database_permissions_by_key(db, &key) + .await? + .filter(|r| r.permissions.enabled), + Err(_) => None, + }; + let info = match record { + Some(record) => ( + crate::datatable_permissions::usable_roles(db, w_id, &record, &access).await?, + record.permissions.default_role().to_string(), + ), + None => (vec![], ADMIN_DATATABLE_ROLE.to_string()), + }; + roles.insert(name, info); + } + Ok(roles) } async fn get_datatable_table_schema( @@ -3138,46 +3129,6 @@ pub(crate) async fn is_instance_datatable(db: &DB, w_id: &str, name: &str) -> Re .unwrap_or(false)) } -/// Refuse to clone a data table whose role permissions are enabled. -/// -/// A clone lands in a brand-new database where none of the roles exist, and the -/// fork's copy of the config is stripped of its permissions — so every member of -/// the fork resolves to the copy's own owner connection and reads, in full, the -/// data the roles existed to divide. Reproducing the roles in the copy is a -/// separate piece of work; until it exists, a fork goes without the data table -/// (the fork creation leaves a permissioned one out of the fork's config). -/// -/// Authorization: performs none. It reads whether `w_id`'s data table is -/// permissioned, for any `w_id` it is handed, so callers MUST already have -/// authorized the caller for that workspace. -pub(crate) async fn refuse_clone_of_permissioned_datatable( - db: &DB, - w_id: &str, - source: &str, -) -> Result<()> { - let Some(name) = source.strip_prefix("datatable://") else { - return Ok(()); - }; - let enabled = sqlx::query_scalar!( - "SELECT COALESCE((datatable->'datatables'->$2->'permissions'->>'enabled')::boolean, false) - FROM workspace_settings WHERE workspace_id = $1", - w_id, - name, - ) - .fetch_optional(db) - .await? - .flatten() - .unwrap_or(false); - if enabled { - return Err(Error::BadRequest(format!( - "Data table '{name}' has role permissions enabled and cannot be cloned into a fork: \ - the copy cannot carry its roles, so it would be readable in full by every member of \ - the fork. The fork goes without it; disable its permissions first to clone it." - ))); - } - Ok(()) -} - /// Same as [`is_instance_datatable`], for the `datatable://` / `$res:` form the /// import endpoints take. async fn is_instance_datatable_source(db: &DB, w_id: &str, source: &str) -> Result { @@ -3506,7 +3457,6 @@ async fn create_pg_database( Json(req): Json, ) -> Result { windmill_common::validate_dbname(&req.target_dbname)?; - refuse_clone_of_permissioned_datatable(&db, &w_id, &req.source).await?; // Non-superadmin: restrict dbname to wm_fork_ prefix if !windmill_api_auth::is_super_admin_authed(&db, &authed).await? { @@ -3598,13 +3548,6 @@ async fn import_pg_database( } } - // Only the fork clone flow overrides the target database name; a plain - // database-to-database import is an admin moving data between databases they - // already reach, and lands nowhere that strips permissions. - if req.target_dbname_override.is_some() { - refuse_clone_of_permissioned_datatable(&db, &w_id, &req.source).await?; - } - let schema_only = req.fork_behavior == DataTableForkBehavior::SchemaOnly; let source_pg = resolve_pg_source_for_copy(&db, &user_db, &authed, &w_id, &req.source).await?; let mut target_pg = @@ -3800,15 +3743,15 @@ async fn edit_datatable_config( let is_superadmin = require_super_admin(&db, &authed).await.is_ok(); let mut tx = db.begin().await?; - // This form carries the whole config forward — permissions restored from the - // old value included — so it reads and writes the settings under the same - // lock as the role save and the principal cleanups, or it puts back what one - // of them just took away. let old_datatables: HashMap = serde_json::from_value( - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id) - .await? - .and_then(|d| d.get("datatables").cloned()) - .unwrap_or(serde_json::Value::Null), + sqlx::query_scalar!( + "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + &w_id + ) + .fetch_optional(&mut *tx) + .await? + .flatten() + .unwrap_or(serde_json::Value::Null), ) .unwrap_or_default(); @@ -3860,12 +3803,10 @@ async fn edit_datatable_config( Some(true) } }; - // Same for permissions, owned by the datatable_permissions endpoints. + // `forked_from` is stamped by the fork clone and read by the fork's + // database drop as "this entry has a database of its own": the form may + // update the schema snapshot inside it, never add or remove the stamp. let old = old_datatables.get(lookup); - dt.permissions = old.and_then(|old| old.permissions.clone()); - // `forked_from` is stamped by the fork clone and read by the permissions - // opt-in as "this entry has a database of its own": the form may update the - // schema snapshot inside it, never add or remove the stamp. dt.forked_from = match ( old.and_then(|old| old.forked_from.as_ref()), dt.forked_from.take(), @@ -3876,55 +3817,9 @@ async fn edit_datatable_config( schema: old.schema.clone(), }), }; - // An entry that is new, or points somewhere new, must not reach a database - // another data table governs. - let points_elsewhere = old.is_none_or(|old| { - old.database.resource_path != dt.database.resource_path - || old.database.resource_type != dt.database.resource_type - }); - if points_elsewhere { - crate::datatable_permissions::refuse_reaching_a_governed_database( - &db, - &w_id, - name, - &dt.database, - ) - .await?; - } - // The roles live in the database this data table points at: their logins - // were created there and every grant they hold is recorded there. Carried - // onto another database they authenticate against a cluster that never - // heard of the grants, so the switch has to go through opting out first — - // which is also what drops the roles from the database they belong to. - if let Some(old) = old.filter(|_| dt.permissions.as_ref().is_some_and(|p| p.enabled)) { - if old.database.resource_path != dt.database.resource_path - || old.database.resource_type != dt.database.resource_type - { - return Err(Error::BadRequest(format!( - "Data table '{name}' has permissions enabled, so it cannot be pointed at \ - another database: disable them first, which drops its roles from the \ - database they were created in." - ))); - } - // A generated login is named from the data table's name, so a renamed - // one keeps logins named for the name it left — and a data table - // created under that name next generates those same names, adopts - // those logins and resets their passwords. Renaming is rare; sharing - // a login between two data tables is not something to leave open. - if lookup != name.as_str() { - return Err(Error::BadRequest(format!( - "Data table '{lookup}' has permissions enabled, so it cannot be renamed to \ - '{name}': its Postgres logins are named after '{lookup}' and a data table \ - created under that name would take them over. Disable its permissions \ - first, which drops those logins, then rename and enable them again." - ))); - } - } } - // The settings carry each role's generated login password. - let args_for_audit = - windmill_common::workspaces::datatable_settings_for_audit(&new_config.settings); + let args_for_audit = serde_json::to_string(&new_config.settings).unwrap_or_default(); audit_log( &mut *tx, &authed, @@ -3955,27 +3850,6 @@ async fn edit_datatable_config( } } - // Planned before the config is overwritten, while the data tables about to - // disappear can still be resolved to a connection, and run once it has - // committed: `DROP OWNED` discards the roles' grants for good, so a save that - // rolls back after this point must not have destroyed anything. - // - // What is disappearing is read from the two configs rather than from the - // request's `deleted_datatables`: a save that drops a name and adds another - // declares no rename and no deletion, and the logins of the name it dropped - // would be left for whatever data table is created under it next. - let mut planned_role_drops = Vec::new(); - for gone in old_datatables - .keys() - .filter(|name| !new_config.settings.datatables.contains_key(*name)) - { - if let Some(planned) = - crate::datatable_permissions::plan_drop_of_deleted_datatable(&db, &w_id, gone).await - { - planned_role_drops.push((gone.clone(), planned)); - } - } - let config: serde_json::Value = serde_json::to_value(new_config.settings) .map_err(|err| Error::internal_err(err.to_string()))?; @@ -3999,10 +3873,6 @@ async fn edit_datatable_config( tx.commit().await?; - for (deleted, planned) in planned_role_drops { - crate::datatable_permissions::run_planned_drop(&db, &w_id, &deleted, planned).await; - } - for substrate in created_substrates { windmill_common::feature_usage::log_feature_usage("datatable", "created", substrate); } @@ -7851,10 +7721,6 @@ async fn apply_forked_datatable( fdt: &ForkedDatatableInfo, ) -> Result<()> { windmill_common::validate_dbname(&fdt.new_dbname)?; - // The clone endpoints refuse this too; this is the one a caller cannot go - // around, since it is what wires the fork's config to the copied database. - refuse_clone_of_permissioned_datatable(db, parent_w_id, &format!("datatable://{}", fdt.name)) - .await?; if !fdt.new_dbname.starts_with("wm_fork_") { return Err(Error::BadRequest(format!( "Forked datatable database name '{}' must start with 'wm_fork_'", @@ -8288,14 +8154,6 @@ async fn create_workspace_fork( .await?; } - // Enabling a data table's permissions is refused while a fork holds a copy of it, and - // that check runs under this same row: a fork still being created has either committed, - // and is found, or copies the parent's settings only after the opt-in landed, so the - // permissioned data table is stripped from it below. Pairing lock first, as - // `lock_workspace_settings_unchecked` states. - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &parent_workspace_id) - .await?; - let forked_id = nw.id; sqlx::query!( @@ -8388,21 +8246,10 @@ async fn create_workspace_fork( apply_forked_datatable(&db, &mut tx, &parent_workspace_id, &forked_id, fdt).await?; } - // A forked data table now points at a fresh database where the parent's roles hold - // nothing, so its cloned `permissions` block is meaningless and is dropped — the fork - // opts in on its own. - // - // A data table that was NOT forked still points at the parent's database, where those - // roles do hold grants, and neither answer is safe: dropping the block would let a - // fork (which any member may create) reach the parent's data as root, while keeping it - // freezes who may run as what at the moment of the fork — the parent revoking a tenant - // would never reach the copy, and the fork would keep running as the role it named. So - // a permissioned data table is not shared into a fork at all; the fork can fork it, or - // go without it. - // - // A copy that was not forked here also loses any `forked_from` it inherited: the stamp - // means "cloned into this workspace's own database", which is what the permissions - // opt-in reads it as, and a copy of the parent's clone points where the parent points. + // A copy that was not forked here loses any `forked_from` it inherited: the stamp + // means "cloned into this workspace's own database" — it is what lets the fork's + // deletion drop that database — and a copy of the parent's clone points where the + // parent points. let forked_datatable_names: Vec = nw .forked_datatables .iter() @@ -8413,11 +8260,9 @@ async fn create_workspace_fork( SET datatable = jsonb_set(datatable, '{datatables}', ( SELECT COALESCE(jsonb_object_agg( key, - CASE WHEN key = ANY($2) THEN value - 'permissions' ELSE value - 'forked_from' END + CASE WHEN key = ANY($2) THEN value ELSE value - 'forked_from' END ), '{}'::jsonb) FROM jsonb_each(datatable->'datatables') - WHERE key = ANY($2) - OR COALESCE((value->'permissions'->>'enabled')::boolean, false) = false )) WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'"#, &forked_id, @@ -8426,20 +8271,6 @@ async fn create_workspace_fork( .execute(&mut *tx) .await?; - // The migrations were cloned for every data table; the ones left out above - // would otherwise keep a history for a data table the fork does not have. - sqlx::query!( - r#"DELETE FROM datatable_migrations m - WHERE m.workspace_id = $1 - AND NOT EXISTS ( - SELECT 1 FROM workspace_settings ws - WHERE ws.workspace_id = $1 AND ws.datatable->'datatables' ? m.datatable - )"#, - &forked_id, - ) - .execute(&mut *tx) - .await?; - // The settings clone copies the source's ducklake config verbatim — including a parent // fork's own `fork_behavior` stamps. Sharing is a per-fork-creation choice, never // inherited: reset any cloned stamps first, then apply this fork's requested list. diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 64108568fe..a936a1a839 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -62,11 +62,10 @@ pub(crate) async fn change_workspace_id( // rename moves the chain from one to the other. crate::workspaces::lock_dev_pairing(&mut tx, &[&old_id, &rw.new_id]).await?; - // The settings are copied below, and a permissions save holds this row while it changes - // the roles in the database and then the config: copied without it, the new workspace - // could carry the config from before that save while the database has the roles from - // after it. Pairing lock first, as `lock_workspace_settings_unchecked` states. - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &old_id).await?; + // The permissions this workspace owns move to the new id below, so a save of them + // holds still meanwhile. Pairing lock first, as `lock_datatable_permissions_unchecked` + // states. + windmill_common::workspaces::lock_datatable_permissions_unchecked(&mut tx, &old_id).await?; check_w_id_conflict(&mut tx, &rw.new_id).await?; @@ -126,24 +125,11 @@ pub(crate) async fn change_workspace_id( .execute(&mut *tx) .await?; - // The archived copy keeps no data tables. Two configs would otherwise name the same - // databases and, for a permissioned one, the same Postgres logins, which only the renamed - // workspace owns: deleting the archived id would plan drops for logins still in use, and - // — since this transaction commits before the old id is archived — a copy without its - // `permissions` block would hand every caller of the old id the owner connection in - // between. An archived shell that still named an instance database would also keep the - // renamed workspace from ever opting in, as another workspace reaching the same database. - // A missing data table fails closed. - // - // The renamed workspace keeps the roles and keeps working: a role's `pg_rolename` is - // what resolution uses, and the generated name only decides what a *new* role is called. - // Its next permissions save finds the stored name no longer matches the one this - // workspace id generates and renames the login to match, under the ownership proof that - // rename already carries. + // The permissions of every database this workspace turned on follow it: their tenants + // are its principals, which the rows below move too. sqlx::query!( - "UPDATE workspace_settings - SET datatable = jsonb_set(datatable, '{datatables}', '{}'::jsonb) - WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", + "UPDATE datatable_database_permissions SET owner_workspace_id = $1 WHERE owner_workspace_id = $2", + &rw.new_id, &old_id, ) .execute(&mut *tx) @@ -1007,17 +993,21 @@ pub(crate) async fn delete_workspace( vec![] }); - // Same shape, same reason: a permissioned data table's logins live in a - // database whose only record is the settings row below, so what to drop is - // resolved while that row is here and the drop itself runs after the commit. - // Nothing is dropped here. - // The first row this transaction locks — see `lock_workspace_settings_unchecked`. - // Without it a permissions save that commits between this read and the row's - // deletion adds a login nothing then drops, and the config that named it is - // gone. + // Same shape, same reason: the logins behind the workspace's data tables live in + // databases that go with it, so what to drop is resolved while the config is here + // and the drop itself runs after the commit. Nothing is dropped here. + // The first rows this transaction locks — see `lock_datatable_permissions_unchecked`. + // Without it a permissions save that commits between this read and the rows' + // deletion adds a login nothing then drops. let mut planned_role_drops = Vec::new(); - let datatable_config = - windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id).await?; + windmill_common::workspaces::lock_datatable_permissions_unchecked(&mut tx, &w_id).await?; + let datatable_config = sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1", + &w_id + ) + .fetch_optional(&mut *tx) + .await? + .flatten(); for name in datatable_config .as_ref() .and_then(|c| c.get("datatables")) @@ -1026,7 +1016,7 @@ pub(crate) async fn delete_workspace( .unwrap_or_default() { if let Some(planned) = - crate::datatable_permissions::plan_drop_of_deleted_datatable(&db, &w_id, &name).await + crate::datatable_permissions::plan_drop_of_datatable_roles(&db, &w_id, &name).await { planned_role_drops.push((name, planned)); } @@ -1358,94 +1348,23 @@ pub struct DropForkedDatatableDatabasesRequest { datatable_names: Vec, } -/// Take a data table's generated logins away before its database goes. -/// -/// A login is cluster-wide, so it outlives the database it was created in and -/// stays adoptable by whatever takes this workspace's id and this data table's -/// name next. The config stops naming them first: a data table whose database is -/// being dropped has no business claiming roles in it, and that is also what the -/// drop reads to know they are nobody's. -/// -/// Returns the block it removed, which the caller must put back if the drop it -/// was clearing the way for does not happen: a data table that keeps its -/// database and loses its permissions leaves every member of the workspace -/// resolving to the data table's own connection, which owns everything in it. +/// Take the generated logins of a data table's database away before that +/// database goes: a login is cluster-wide, so it would outlive the database it +/// was created in. The permissions row stays until the drop succeeds — with its +/// logins gone every role is refused meanwhile — and is forgotten by the caller +/// once the database is. Returns the key of that database's permissions when +/// roles were dropped. async fn drop_datatable_roles_before_its_database( db: &DB, w_id: &str, dt_name: &str, - errors: &mut Vec, -) -> Option { +) -> Option { let planned = - crate::datatable_permissions::plan_drop_of_deleted_datatable(db, w_id, dt_name).await?; - // Read and clear under the settings lock, so what comes back is what was - // taken away and is what putting it back would restore. - let cleared = async { - let mut tx = db.begin().await?; - let removed = windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, w_id) - .await? - .and_then(|c| { - c.get("datatables") - .and_then(|d| d.get(dt_name)) - .and_then(|d| d.get("permissions")) - .cloned() - }); - sqlx::query!( - "UPDATE workspace_settings - SET datatable = datatable #- ARRAY['datatables', $2, 'permissions'] - WHERE workspace_id = $1", - w_id, - dt_name, - ) - .execute(&mut *tx) - .await?; - tx.commit().await?; - Ok::<_, windmill_common::error::Error>(removed) - } - .await; - match cleared { - Ok(removed) => { - crate::datatable_permissions::run_planned_drop(db, w_id, dt_name, planned).await; - removed - } - Err(e) => { - errors.push(format!( - "Could not clear the permissions of datatable://{}: {}", - dt_name, e - )); - None - } - } -} - -/// Put back the block a drop that did not happen took away. The logins it names -/// are already gone, so every role now resolves to nothing and is refused — the -/// safe answer, and the one a re-save of the data table's permissions repairs by -/// recreating them. -async fn restore_datatable_permissions( - db: &DB, - w_id: &str, - dt_name: &str, - permissions: serde_json::Value, - errors: &mut Vec, -) { - if let Err(e) = sqlx::query!( - "UPDATE workspace_settings - SET datatable = jsonb_set(datatable, ARRAY['datatables', $2, 'permissions'], $3) - WHERE workspace_id = $1 - AND datatable->'datatables' ? $2", - w_id, - dt_name, - permissions, - ) - .execute(db) - .await - { - errors.push(format!( - "Could not restore the permissions of datatable://{}, which is now open to every member of the workspace: {}", - dt_name, e - )); - } + crate::datatable_permissions::plan_drop_of_datatable_roles(db, w_id, dt_name).await?; + let key = planned.2.clone(); + crate::datatable_permissions::run_planned_drop_keeping_record(db, w_id, dt_name, planned) + .await + .then_some(key) } /// Drop forked datatable databases. Returns errors per datatable that failed. @@ -1510,17 +1429,18 @@ pub async fn drop_forked_datatable_databases( )); continue; } - let removed = - drop_datatable_roles_before_its_database(&db, &w_id, dt_name, &mut errors).await; - if let Err(e) = windmill_common::drop_custom_instance_database(&db, db_to_drop).await { - errors.push(format!( + let permissions_key = + drop_datatable_roles_before_its_database(&db, &w_id, dt_name).await; + match windmill_common::drop_custom_instance_database(&db, db_to_drop).await { + Ok(()) => { + if let Some(key) = permissions_key { + crate::datatable_permissions::forget_database_permissions(&db, &key).await; + } + } + Err(e) => errors.push(format!( "Could not drop instance database '{}' for datatable://{}: {}", db_to_drop, dt_name, e - )); - if let Some(permissions) = removed { - restore_datatable_permissions(&db, &w_id, dt_name, permissions, &mut errors) - .await; - } + )), } } else { let fork_pg = match crate::workspaces::resolve_pg_source_checked( @@ -1581,27 +1501,24 @@ pub async fn drop_forked_datatable_databases( match parent_pg.connect(Some(&db)).await { Ok((client, connection)) => { let join_handle = tokio::spawn(async move { connection.await }); - let removed = - drop_datatable_roles_before_its_database(&db, &w_id, dt_name, &mut errors) - .await; - if let Err(e) = client + let permissions_key = + drop_datatable_roles_before_its_database(&db, &w_id, dt_name).await; + match client .execute(&format!("DROP DATABASE \"{}\"", db_to_drop), &[]) .await { - errors.push(format!( + Ok(_) => { + if let Some(key) = permissions_key { + crate::datatable_permissions::forget_database_permissions( + &db, &key, + ) + .await; + } + } + Err(e) => errors.push(format!( "Could not drop database '{}' for datatable://{}: {}", db_to_drop, dt_name, e - )); - if let Some(permissions) = removed { - restore_datatable_permissions( - &db, - &w_id, - dt_name, - permissions, - &mut errors, - ) - .await; - } + )), } drop(client); let _ = windmill_common::shutdown_pg_connection(join_handle).await; diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 9621389b3a..9238c717b6 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -34220,7 +34220,7 @@ components: type: string DatatablePermissions: type: object - required: [enabled, roles, default_role] + required: [enabled, roles, default_role, editable] properties: enabled: type: boolean @@ -34231,6 +34231,12 @@ components: default_role: description: the role a script gets when it names none type: string + owner_workspace_id: + description: the workspace that turned the permissions on, whose admins manage them and whose principals the tenants are; absent while they are off + type: string + editable: + description: whether the caller may change them from this workspace (an admin of the owning workspace, or a superadmin) + type: boolean SetDatatablePermissions: type: object required: [enabled, roles] diff --git a/backend/windmill-api/src/offboarding.rs b/backend/windmill-api/src/offboarding.rs index 556f7fdeb1..14a122cb9f 100644 --- a/backend/windmill-api/src/offboarding.rs +++ b/backend/windmill-api/src/offboarding.rs @@ -833,10 +833,16 @@ async fn offboard_user_from_workspace<'c>( new_permissioned_as: &str, ) -> Result { // Before this transaction locks anything else — see - // `lock_workspace_settings_unchecked`. Everything below reassigns rows a - // rename or a deletion writes while holding this row. - let datatable_settings = - windmill_common::workspaces::lock_workspace_settings_unchecked(tx, w_id).await?; + // `lock_datatable_permissions_unchecked`. Everything below reassigns rows a + // rename or a deletion writes while holding those. + windmill_common::workspaces::lock_datatable_permissions_unchecked(tx, w_id).await?; + let datatable_settings = sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + w_id + ) + .fetch_optional(&mut **tx) + .await? + .flatten(); let new_prefix = reassign_to.to_string(); let departing = windmill_common::users::username_to_permissioned_as(username); diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index cf730bcfd5..d81efba577 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -321,11 +321,17 @@ async fn update_username_in_workpsace<'c>( w_id: &str, ) -> error::Result<()> { // Before anything else in the transaction — see - // `lock_workspace_settings_unchecked`. `rename_user` walks memberships in + // `lock_datatable_permissions_unchecked`. `rename_user` walks memberships in // `workspace_id` order, so a rename spanning workspaces takes their rows in // that order too. - let datatable_settings = - windmill_common::workspaces::lock_workspace_settings_unchecked(tx, w_id).await?; + windmill_common::workspaces::lock_datatable_permissions_unchecked(tx, w_id).await?; + let datatable_settings = sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + w_id + ) + .fetch_optional(&mut **tx) + .await? + .flatten(); // ---- instance and workspace users ---- @@ -944,21 +950,22 @@ async fn update_username_in_workpsace<'c>( // executor compares it against the caller's name. Left behind, the rename // takes the role away from the user it followed and hands it to whoever // takes the old name next. + windmill_common::workspaces::rename_datatable_tenant_in_workspace_unchecked( + w_id, + &format!("u/{old_username}"), + &format!("u/{new_username}"), + tx, + ) + .await?; if let Some(mut settings) = datatable_settings { - let mut renamed = windmill_common::workspaces::rename_datatable_tenant( - &mut settings, - &format!("u/{old_username}"), - &format!("u/{new_username}"), - ); // The resource rewrite above moves a data table's own postgres resource // with everything else the user owns; the config names it by path, so it // has to travel too. - renamed |= windmill_common::workspaces::move_datatable_resource_paths( + if windmill_common::workspaces::move_datatable_resource_paths( &mut settings, &format!("u/{old_username}/"), &format!("u/{new_username}/"), - ); - if renamed { + ) { sqlx::query!( "UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = $2", settings, diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 58deadfca1..05cd354cd4 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -47,7 +47,6 @@ use windmill_common::runnable_settings::{ConcurrencySettings, DebouncingSettings use windmill_common::scripts::ScriptRunnableSettingsHandle; use windmill_common::utils::require_admin; use windmill_common::variables::decrypt; -use windmill_common::workspaces::redact_datatable_settings_for_export; use windmill_common::worker::WINDMILL_DIR; use windmill_common::{ db::UserDB, @@ -1632,7 +1631,7 @@ pub(crate) async fn tarball_workspace( mute_critical_alerts: row.mute_critical_alerts, color: row.color.clone(), operator_settings: row.operator_settings.clone(), - datatable: redact_datatable_settings_for_export(row.datatable.clone()), + datatable: row.datatable.clone(), slack_team_id: row.slack_team_id.clone(), slack_name: row.slack_name.clone(), slack_command_script: row.slack_command_script.clone(), @@ -1696,7 +1695,7 @@ pub(crate) async fn tarball_workspace( mute_critical_alerts: row.mute_critical_alerts, color: row.color, operator_settings: row.operator_settings, - datatable: redact_datatable_settings_for_export(row.datatable), + datatable: row.datatable, slack_team_id: row.slack_team_id, slack_name: row.slack_name, slack_command_script: row.slack_command_script, diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index fd2935dd43..b0bfb0b9bb 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1214,11 +1214,6 @@ pub struct DataTable { /// when migrations already exist (see `datatable_migrations_enabled`). #[serde(default, skip_serializing_if = "Option::is_none")] pub migrations_enabled: Option, - /// Role-based access control, opt-in per data table. Absent or - /// `enabled: false` means every workspace member reaches the database - /// through the single connection resolved below. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub permissions: Option, } /// The role every permissioned data table has: it is the connection the data @@ -1241,19 +1236,6 @@ pub struct DataTablePermissions { /// The role a script gets when it names none. Absent means `admin`. #[serde(default, skip_serializing_if = "Option::is_none")] pub default_role: Option, - /// The database the roles below were created in, as - /// [`datatable_database_identity`] fingerprints it. Absent for a data table - /// on an instance database, whose connection is Windmill's own and cannot be - /// repointed by editing a resource. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub database_identity: Option, - /// The database itself, as [`physical_database_identity`] fingerprints it — - /// the login left out. Stamped at the opt-in, instance databases included, - /// and what every other data table entry is held apart from: one reaching - /// this database would hand its users the owning connection, whichever login - /// its own resource carries. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub physical_identity: Option, } impl DataTablePermissions { @@ -1276,111 +1258,235 @@ pub struct DataTableRole { pub tenants: Vec, } -/// Every role's tenant list in a `workspace_settings.datatable` value. -fn datatable_tenant_lists( - datatable: &mut serde_json::Value, -) -> impl Iterator> { - datatable - .get_mut("datatables") - .and_then(|d| d.as_object_mut()) - .into_iter() - .flat_map(|datatables| datatables.values_mut()) - .filter_map(|dt| dt.get_mut("permissions")) - .filter_map(|p| p.get_mut("roles")) - .filter_map(|r| r.as_object_mut()) - .flat_map(|roles| roles.values_mut()) - .filter_map(|role| role.get_mut("tenants")) - .filter_map(|t| t.as_array_mut()) +/// Role-based access to one Postgres database, keyed by the database itself. +/// +/// The roles are created in the database, so every data table entry that reaches +/// it — in any workspace, a fork's copy, a detached workspace, an archived one — +/// resolves to this one row rather than carrying a copy of its own. The tenants +/// in `permissions` are principals of `owner_workspace_id`, and a caller from +/// another workspace is evaluated as a member of that one (see +/// [`can_use_datatable_role_in_owner_workspace`]). +#[derive(Debug, Clone)] +pub struct DatabasePermissions { + pub database_key: String, + pub owner_workspace_id: String, + pub permissions: DataTablePermissions, } -/// Follow one tenant across a rename, or drop it where the principal is gone. +/// The key of the database a data table entry reaches. /// -/// A tenant is matched whole (`u/alice`, `g/devs`, `f/team`), so the `*` -/// wildcard and every other principal are left alone. Returns whether anything -/// changed, so a caller can skip the write. -/// -/// Which of the two a flow needs follows the principal: a rename keeps the role -/// with the same person, while a deletion has to take it away — the name is free -/// afterwards, and whoever takes it next would otherwise inherit every role the -/// old one was a tenant of. -fn update_datatable_tenant( - datatable: &mut serde_json::Value, - tenant: &str, - replacement: Option<&str>, -) -> bool { - let mut changed = false; - for tenants in datatable_tenant_lists(datatable) { - match replacement { - Some(new) => { - for entry in tenants.iter_mut() { - if entry.as_str() == Some(tenant) { - *entry = serde_json::Value::String(new.to_string()); - changed = true; - } - } - } - None => { - let before = tenants.len(); - tenants.retain(|entry| entry.as_str() != Some(tenant)); - changed |= tenants.len() != before; +/// An instance database is named by its `dbname`, unique on the instance's +/// Postgres. A resource-backed one is the host, port and database its resource +/// resolves to — the login left out: the same database reached as another user +/// is the same database, and whoever can make a resource resolve there either +/// holds its credentials, and has that access already, or does not, and gets a +/// connection that fails. +pub fn datatable_database_key( + database: &DataTableDatabase, + resolved: &serde_json::Value, +) -> String { + match database.resource_type { + DataTableCatalogResourceType::Instance => { + format!("instance:{}", database.resource_path) + } + DataTableCatalogResourceType::Postgresql => { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + // NUL-joined so a value cannot be replayed by moving characters across + // the field boundaries. + for field in ["host", "port", "dbname"] { + let value = resolved + .get(field) + .map(|v| v.to_string()) + .unwrap_or_default(); + hasher.update(value.as_bytes()); + hasher.update([0u8]); } + format!("pg:{}", hex::encode(hasher.finalize())) } } - changed } -/// `u/` becomes `u/` on every role that named it. -pub fn rename_datatable_tenant( - datatable: &mut serde_json::Value, - old_tenant: &str, - new_tenant: &str, -) -> bool { - update_datatable_tenant(datatable, old_tenant, Some(new_tenant)) +fn database_permissions_row( + database_key: String, + owner_workspace_id: String, + permissions: serde_json::Value, +) -> Result { + Ok(DatabasePermissions { + database_key, + owner_workspace_id, + permissions: serde_json::from_value(permissions).map_err(|e| { + Error::internal_err(format!("Invalid stored database permissions: {e}")) + })?, + }) } -/// Take a deleted user, group or folder off every role it was a tenant of. -pub fn remove_datatable_tenant(datatable: &mut serde_json::Value, tenant: &str) -> bool { - update_datatable_tenant(datatable, tenant, None) -} - -/// Take the workspace's settings row, and read the data table config under it, -/// for the length of the caller's transaction. +/// The permissions of the database `database_key` names, if any workspace has +/// turned them on. /// -/// This row is the one lock over everything a data table's permissions depend -/// on: the config itself, and the users, groups and folders its roles name as -/// tenants. Every path that touches either — a role save, an ACL change, the -/// settings form, a rename, a deletion — takes it, which is what stops one of -/// them persisting a block it computed before another committed: a save that -/// planned with `g/devs` would otherwise put the tenant back after the group's -/// deletion took it away. +/// Authorization: performs none. What it returns carries the roles' login +/// passwords, so callers MUST NOT pass it outward. +pub async fn database_permissions_by_key<'e, E: sqlx::PgExecutor<'e>>( + db: E, + database_key: &str, +) -> Result> { + let row = sqlx::query!( + r#"SELECT database_key, owner_workspace_id, permissions + FROM datatable_database_permissions WHERE database_key = $1"#, + database_key + ) + .fetch_optional(db) + .await?; + row.map(|r| database_permissions_row(r.database_key, r.owner_workspace_id, r.permissions)) + .transpose() +} + +/// Take the database's permissions row for the length of the caller's +/// transaction, whether or not it exists yet: an advisory lock on the key +/// serializes two opt-ins racing to create it, and the row lock serializes a +/// save with the principal deletions that strip tenants from it. +/// +/// **Take it before the transaction locks anything else.** One lock, always +/// acquired first, cannot deadlock. A caller that also needs a workspace's rows +/// (a rename, a deletion) takes those through +/// [`lock_datatable_permissions_unchecked`] and nothing else first. +/// +/// Authorization: performs none; the row carries login passwords, so callers +/// MUST have authorized the read and MUST NOT pass the value outward. +pub async fn lock_database_permissions( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + database_key: &str, +) -> Result> { + sqlx::query!( + "SELECT pg_advisory_xact_lock(hashtext('datatable_database_permissions:' || $1))", + database_key + ) + .execute(&mut **tx) + .await?; + let row = sqlx::query!( + r#"SELECT database_key, owner_workspace_id, permissions + FROM datatable_database_permissions WHERE database_key = $1 FOR UPDATE"#, + database_key + ) + .fetch_optional(&mut **tx) + .await?; + row.map(|r| database_permissions_row(r.database_key, r.owner_workspace_id, r.permissions)) + .transpose() +} + +/// Write the database's permissions, creating the row for `owner_workspace_id` +/// when none exists. An existing row keeps its owner. +pub async fn upsert_database_permissions( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + database_key: &str, + owner_workspace_id: &str, + permissions: &DataTablePermissions, +) -> Result<()> { + let permissions = serde_json::to_value(permissions) + .map_err(|e| Error::internal_err(format!("Failed to serialize permissions: {e}")))?; + sqlx::query!( + r#"INSERT INTO datatable_database_permissions (database_key, owner_workspace_id, permissions) + VALUES ($1, $2, $3) + ON CONFLICT (database_key) DO UPDATE SET permissions = EXCLUDED.permissions, updated_at = now()"#, + database_key, + owner_workspace_id, + permissions + ) + .execute(&mut **tx) + .await?; + Ok(()) +} + +/// Forget the database's permissions: what the opt-out does once the roles are +/// dropped. +pub async fn delete_database_permissions( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + database_key: &str, +) -> Result<()> { + sqlx::query!( + "DELETE FROM datatable_database_permissions WHERE database_key = $1", + database_key + ) + .execute(&mut **tx) + .await?; + Ok(()) +} + +/// The databases whose permissions `w_id` owns. +/// +/// Authorization: performs none. The rows carry login passwords; callers MUST +/// have authorized the read and MUST NOT pass them outward. +pub async fn database_permissions_owned_by<'e, E: sqlx::PgExecutor<'e>>( + db: E, + w_id: &str, +) -> Result> { + let rows = sqlx::query!( + r#"SELECT database_key, owner_workspace_id, permissions + FROM datatable_database_permissions WHERE owner_workspace_id = $1 ORDER BY database_key"#, + w_id + ) + .fetch_all(db) + .await?; + rows.into_iter() + .map(|r| database_permissions_row(r.database_key, r.owner_workspace_id, r.permissions)) + .collect() +} + +/// Take every permissions row `w_id` owns for the length of the caller's +/// transaction, and read them under the lock. +/// +/// The rows are the one lock over what a database's permissions depend on: the +/// roles themselves, and the users, groups and folders they name as tenants. +/// Every path that touches either — a role save, an ACL change, a principal's +/// rename or deletion — takes it, which is what stops one of them persisting a +/// tenant list it computed before another committed: a save that planned with +/// `g/devs` would otherwise put the tenant back after the group's deletion took +/// it away. /// /// **Take it before the transaction locks anything else**, with one exception: /// the dev-pairing advisory lock (`lock_dev_pairing`) comes first where a path -/// needs both — fork creation and the workspace rename — and nothing takes this -/// row and then reaches for that one. One order, held everywhere, cannot -/// deadlock; a caller that writes `usr` or `group_` and then reaches for this -/// one holds two in an order some other path holds the other way round. A -/// transaction spanning workspaces takes them in `workspace_id` order, for the -/// same reason. That is the whole ordering rule: what a handler writes after -/// taking it, and in what order, does not matter. +/// needs both, and nothing takes these rows and then reaches for that one. One +/// order, held everywhere, cannot deadlock. A transaction spanning workspaces +/// takes them in `workspace_id` order, for the same reason. /// -/// Authorization: performs none, for any workspace it is handed. What it returns -/// is the config as stored, generated role passwords included, so callers MUST -/// have authorized the read, and MUST NOT pass the value outward without -/// [`redact_datatable_settings_for_export`]. Taking the lock is not itself a -/// read of anything a caller must be admin for — a path that only serializes -/// against other writers may take it and ignore the value. -pub async fn lock_workspace_settings_unchecked( +/// Authorization: performs none, for any workspace it is handed. The rows carry +/// login passwords, so callers MUST have authorized the read and MUST NOT pass +/// them outward. A path that only serializes against other writers may take the +/// lock and ignore the value. +pub async fn lock_datatable_permissions_unchecked( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, -) -> Result> { - Ok(sqlx::query_scalar!( - "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", +) -> Result> { + let rows = sqlx::query!( + r#"SELECT database_key, owner_workspace_id, permissions + FROM datatable_database_permissions WHERE owner_workspace_id = $1 + ORDER BY database_key FOR UPDATE"#, w_id ) - .fetch_optional(&mut **tx) - .await? - .flatten()) + .fetch_all(&mut **tx) + .await?; + rows.into_iter() + .map(|r| database_permissions_row(r.database_key, r.owner_workspace_id, r.permissions)) + .collect() +} + +/// Apply `change` to every tenant list of every database `w_id` owns, in the +/// caller's transaction, writing back the rows it changed. +async fn update_datatable_tenants_unchecked( + w_id: &str, + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + change: impl Fn(&mut Vec) -> bool, +) -> Result<()> { + for mut row in lock_datatable_permissions_unchecked(tx, w_id).await? { + let mut changed = false; + for role in row.permissions.roles.values_mut() { + changed |= change(&mut role.tenants); + } + if changed { + upsert_database_permissions(tx, &row.database_key, w_id, &row.permissions).await?; + } + } + Ok(()) } /// Take a principal off every data table role of one workspace, in the caller's @@ -1401,19 +1507,40 @@ pub async fn remove_datatable_tenant_in_workspace_unchecked( tenant: &str, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, ) -> Result<()> { - let Some(mut settings) = lock_workspace_settings_unchecked(tx, w_id).await? else { - return Ok(()); - }; - if remove_datatable_tenant(&mut settings, tenant) { - sqlx::query!( - "UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = $2", - settings, - w_id - ) - .execute(&mut **tx) - .await?; - } - Ok(()) + update_datatable_tenants_unchecked(w_id, tx, |tenants| { + let before = tenants.len(); + tenants.retain(|t| t != tenant); + tenants.len() != before + }) + .await +} + +/// Follow a principal's rename through every data table role of one workspace, +/// in the caller's own transaction: the tenant is stored by name, and left +/// behind it would leave the principal without the role and hand it to whoever +/// takes the old name next. +/// +/// Authorization: performs none; callers MUST have authorized the rename itself. +pub async fn rename_datatable_tenant_in_workspace_unchecked( + w_id: &str, + old: &str, + new: &str, + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, +) -> Result<()> { + update_datatable_tenants_unchecked(w_id, tx, |tenants| { + let mut changed = false; + for tenant in tenants.iter_mut() { + if tenant == old { + *tenant = new.to_string(); + changed = true; + } + } + if changed { + tenants.dedup(); + } + changed + }) + .await } /// Follow the backing postgres resource of every data table under a path that @@ -1450,355 +1577,32 @@ pub fn move_datatable_resource_paths( changed } -/// Strip the generated role passwords out of a `workspace_settings.datatable` -/// value. -/// -/// Those passwords are direct database logins: anything that hands the settings -/// blob outside the server — the settings endpoint, the workspace tarball, a -/// git-synced `settings.yaml` — must go through this first, or the credential -/// lands somewhere far weaker than the role it protects (a repository's history, -/// a non-admin's export). -pub fn redact_datatable_settings_for_export( - datatable: Option, -) -> Option { - let mut datatable = datatable?; - let Some(datatables) = datatable - .get_mut("datatables") - .and_then(|d| d.as_object_mut()) - else { - return Some(datatable); - }; - for (_, dt) in datatables.iter_mut() { - let Some(roles) = dt - .get_mut("permissions") - .and_then(|p| p.get_mut("roles")) - .and_then(|r| r.as_object_mut()) - else { - continue; - }; - for (_, role) in roles.iter_mut() { - if let Some(role) = role.as_object_mut() { - role.remove("pg_password"); - } - } - } - Some(datatable) -} - -/// The data tables of the workspace that reach their database through this -/// resource, each with whether its permissions are enabled. -async fn datatables_on_resource(db: &DB, w_id: &str, path: &str) -> Result> { - let datatables: std::collections::HashMap = sqlx::query_scalar!( - "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", - w_id - ) - .fetch_optional(db) - .await? - .flatten() - .and_then(|v| serde_json::from_value(v).ok()) - .unwrap_or_default(); - - let mut names: Vec<(String, bool)> = datatables - .into_iter() - .filter(|(_, dt)| { - dt.database.resource_type == DataTableCatalogResourceType::Postgresql - && dt.database.resource_path == path - }) - .map(|(name, dt)| (name, dt.permissions.as_ref().is_some_and(|p| p.enabled))) - .collect(); - names.sort(); - Ok(names) -} - -/// The data tables with permissions enabled that reach their database through -/// this resource. -async fn datatables_permissioned_on_resource( - db: &DB, - w_id: &str, - path: &str, -) -> Result> { - Ok(datatables_on_resource(db, w_id, path) - .await? - .into_iter() - .filter(|(_, permissioned)| *permissioned) - .map(|(name, _)| name) - .collect()) -} - -fn resource_backs_permissioned_datatable(names: &[String]) -> Error { - Error::BadRequest(format!( - "This resource is how data table {} reaches its database, and it has permissions \ - enabled: its roles were created in that database and every grant they hold is \ - recorded there. Disable them first, which drops the roles from it.", - names.join(", ") - )) -} - -/// Refuse a resource edit that would move a permissioned data table onto another -/// database. -/// -/// The roles of such a data table live in the database it points at: their logins -/// were created there and every grant they hold is recorded there. The path in -/// the config staying the same says nothing — the resource behind it can be -/// edited — so the identity the connection resolves to is what has to hold -/// still. A password rotation is not an identity change and stays allowed. -/// -/// This is where the edit is refused, not where the invariant is kept: it reads -/// the config through the pool, so an edit and a save that enables permissions -/// can each read the other's before-state, and a resource can be repointed -/// through a `$res:` or `$var:` this never expands. What holds either way is -/// [`ensure_datatable_database_unchanged`], which asks the same question of the -/// resolved connection at the moment a role is used. -/// -/// Authorization: performs none, and needs none — it only ever refuses. Callers -/// authorize the edit itself. -/// -/// `new_value` absent means the value is being cleared, which is a change like -/// any other: the next write would land on a resource with no identity to -/// compare against. -/// -/// The other direction holds too: a resource behind an *unpermissioned* data -/// table may not be pointed at a database another data table governs — that -/// entry would be a second door onto it, open to every member as the owning -/// connection. The settings form refuses such an entry; this refuses the -/// resource write — an edit, a create at a path an entry still names, a rename -/// onto one — that would turn an existing entry into one. The new value is -/// resolved for it, `$var:` references included, since that is what the entry -/// would reach. A resource owner need not be an admin anywhere, so a governed -/// data table of another workspace is not named to them. -pub async fn ensure_resource_identity_change_allowed( - db: &DB, - w_id: &str, - path: &str, - old_value: Option<&serde_json::Value>, - new_value: Option<&serde_json::Value>, -) -> Result<()> { - let backing = datatables_on_resource(db, w_id, path).await?; - if backing.is_empty() { - return Ok(()); - } - let identity = |v: &serde_json::Value| { - ( - v.get("host").cloned(), - v.get("port").cloned(), - v.get("dbname").cloned(), - v.get("user").cloned(), - ) - }; - let unchanged = match (old_value, new_value) { - (Some(old), Some(new)) => identity(old) == identity(new), - _ => false, - }; - // Nothing to move away from until there is a previous identity. - if old_value.is_some() && !unchanged { - let names: Vec = backing - .iter() - .filter(|(_, permissioned)| *permissioned) - .map(|(name, _)| name.clone()) - .collect(); - if !names.is_empty() { - return Err(resource_backs_permissioned_datatable(&names)); - } - } - let Some(new_value) = new_value.filter(|_| !unchanged) else { - return Ok(()); - }; - let resolved = transform_json_unchecked(new_value, w_id, db).await?; - let new_identity = physical_database_identity(&resolved); - let own: Vec<(&str, &str)> = backing - .iter() - .map(|(name, _)| (w_id, name.as_str())) - .collect(); - if let Some((gw, gname)) = governed_datatable_reaching(db, &new_identity, &own).await? { - let mut names: Vec<&str> = backing.iter().map(|(n, _)| n.as_str()).collect(); - names.sort(); - return Err(Error::BadRequest(format!( - "This resource is how data table {} reaches its database; pointed there, it would \ - reach the database of {}, whose role permissions are enabled, and open it to \ - every member as the owning connection.", - names.join(", "), - if gw == w_id { - format!("data table '{gname}'") - } else { - "a data table of another workspace".to_string() - } - ))); - } - Ok(()) -} - -/// The permissioned data table, in any workspace, whose opt-in stamped this -/// [`physical_database_identity`] — other than the `except` entries — if there -/// is one. -/// -/// Authorization: performs none, and needs none: it answers with a name, for a -/// caller that only ever refuses on it. -pub async fn governed_datatable_reaching( - db: &DB, - physical_identity: &str, - except: &[(&str, &str)], -) -> Result> { - let governed = sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!" - FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt - WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false) - AND dt.value->'permissions'->>'physical_identity' = $1 - ORDER BY ws.workspace_id, dt.key"#, - physical_identity, - ) - .fetch_all(db) - .await?; - Ok(governed - .into_iter() - .find(|g| { - !except - .iter() - .any(|(w, n)| *w == g.workspace_id && *n == g.name) - }) - .map(|g| (g.workspace_id, g.name))) -} - -/// Fingerprint the database a connection resolves to, over the fields that make -/// it a different database rather than the same one reached differently — a -/// password rotation is not an identity change. -/// -/// `user` is one of those fields, so this is the resource as it resolves, before -/// a role's login is swapped in: recording one and comparing the other never -/// matches. -pub fn datatable_database_identity(resolved: &serde_json::Value) -> String { - use sha2::{Digest, Sha256}; - let mut hasher = Sha256::new(); - // NUL-joined so a value cannot be replayed by moving characters across the - // field boundaries, and hashed so the config does not carry the host around. - for field in ["host", "port", "dbname", "user"] { - let value = resolved - .get(field) - .map(|v| v.to_string()) - .unwrap_or_default(); - hasher.update(value.as_bytes()); - hasher.update([0u8]); - } - hex::encode(hasher.finalize()) -} - -/// Fingerprint the database a connection resolves to, without the login: the -/// same host, port and database reached as another user is the same database, -/// and for the question of who else reaches it, the login does not matter. -pub fn physical_database_identity(resolved: &serde_json::Value) -> String { - use sha2::{Digest, Sha256}; - let mut hasher = Sha256::new(); - for field in ["host", "port", "dbname"] { - let value = resolved - .get(field) - .map(|v| v.to_string()) - .unwrap_or_default(); - hasher.update(value.as_bytes()); - hasher.update([0u8]); - } - hex::encode(hasher.finalize()) -} - -/// Whether a resolution has to prove the data table still points where its roles -/// live. -/// -/// Every role of a permissioned data table, `admin` included: it has no login to -/// swap in — it is the data table's own connection — but following that -/// connection to another database is what the record exists to stop, so the -/// question cannot be gated on there being credentials to override. -/// -/// `Unchecked` is exempt because it is not a caller with an identity to answer -/// for: it is the machinery that repairs a data table (role DDL, migrations, -/// fork snapshots) and would otherwise be unable to repair a moved one, plus the -/// admin diagnostic and the postgres trigger's replication connection, which -/// reach the data table's own connection the way they did before roles existed. -fn resolution_proves_database_identity(internal: bool, datatable: &DataTable) -> bool { - !internal && datatable.permissions.as_ref().is_some_and(|p| p.enabled) -} - -/// Refuse a role whose data table no longer resolves to the database that role -/// was created in. -/// -/// The guards below refuse the edits that would move it, but they read the -/// config and the resource separately from the save that enables permissions and -/// from each other, and a resource can be repointed through a `$res:` or `$var:` -/// they never look at. This is the same question asked where it is answerable: -/// against the fully expanded connection, at the moment it is used. -fn ensure_datatable_database_unchanged( - name: &str, - datatable: &DataTable, - resolved: &serde_json::Value, -) -> Result<()> { - // An instance database is named by the config rather than by a resource, so - // there is nothing a workspace can edit to move it. - if datatable.database.resource_type != DataTableCatalogResourceType::Postgresql { - return Ok(()); - } - let recorded = datatable - .permissions - .as_ref() - .and_then(|p| p.database_identity.as_deref()); - if recorded.is_some_and(|recorded| recorded == datatable_database_identity(resolved)) { - return Ok(()); - } - Err(Error::NotAuthorized(format!( - "Data table '{name}' no longer resolves to the database its roles were created in, so \ - they cannot be used: their logins and grants are in the previous one. Save the data \ - table's permissions again to recreate them where it points now." - ))) -} - -/// Refuse to take a resource out from under a permissioned data table: deleting -/// it, or moving it to another path, leaves the config naming something that is -/// not there — and the next resource created at that path answers for roles it -/// never had. -/// -/// Same standing as [`ensure_resource_identity_change_allowed`]: an early, -/// unauthorized refusal, backed by the check at resolution. -pub async fn ensure_resource_removal_allowed(db: &DB, w_id: &str, path: &str) -> Result<()> { - let names = datatables_permissioned_on_resource(db, w_id, path).await?; - if names.is_empty() { - return Ok(()); - } - Err(resource_backs_permissioned_datatable(&names)) -} - -/// The data table settings as one audit parameter, which is stored and traced -/// in the clear — so it goes through the same redaction as any other export. -pub fn datatable_settings_for_audit(settings: &impl Serialize) -> String { - serde_json::to_value(settings) - .ok() - .and_then(|v| redact_datatable_settings_for_export(Some(v))) - .map(|v| v.to_string()) - .unwrap_or_default() -} - /// Postgres caps identifiers at 63 bytes (NAMEDATALEN - 1) and silently /// truncates past it, which would collapse two distinct roles onto one. const PG_IDENTIFIER_MAX_LEN: usize = 63; -/// The postgres role backing `role` on `w_id`'s `datatable`. +/// The postgres role backing `role` in the database `database_key` names. /// -/// Postgres roles are cluster-wide while data table and role names are scoped to a -/// workspace, so uniqueness comes from a hash of the whole `(workspace, data -/// table, role)` triple. The readable `wm_` prefix is only there to make the -/// role recognizable in `\du` and in grant statements written by hand — it is -/// sanitized and truncated, so it identifies nothing on its own and two data -/// tables may well share it. -pub fn datatable_pg_role_name(w_id: &str, datatable: &str, role: &str) -> String { +/// Postgres roles are cluster-wide while role names are chosen per database, so +/// uniqueness comes from a hash of the `(database, role)` pair. The readable +/// `wm_` prefix is only there to make the role recognizable in `\du` and +/// in grant statements written by hand — it is sanitized and truncated, so it +/// identifies nothing on its own and two databases may well share it. +pub fn datatable_pg_role_name(database_key: &str, role: &str) -> String { use sha2::{Digest, Sha256}; // NUL-joined so the digest cannot be replayed by moving characters across the // field boundaries. let mut hasher = Sha256::new(); - for part in [w_id, datatable, role] { + for part in [database_key, role] { hasher.update(part.as_bytes()); hasher.update([0u8]); } let digest = hasher.finalize(); - // The whole separation between two data tables' logins is these bytes: a - // name is generated, and a generated name that lands on an existing role is - // a role two data tables share. Sixteen of them put that out of reach of a - // search as well as of chance — four did not, and `pg_roles` is a cluster - // catalog, so the search space is every workspace on the instance. + // The whole separation between two databases' logins is these bytes: a name + // is generated, and a generated name that lands on an existing role is a role + // two databases share. Sixteen of them put that out of reach of a search as + // well as of chance — four did not, and `pg_roles` is a cluster catalog, so + // the search space is every database on the instance. let discriminator = hex::encode(&digest[..16]); let readable: String = format!("wm_{role}") @@ -1995,17 +1799,17 @@ pub async fn get_datatable_replication_resource_from_db_unchecked( /// Look up the role a resolution asks for, without authorizing it. /// -/// `Ok(None)` means the data table is unpermissioned and resolves through its own -/// connection. On a permissioned one, naming no role selects the configured -/// default and an unknown role is an error; on an unpermissioned one, naming any -/// role other than `admin` is an error too — silently ignoring it would run the -/// script with more privileges than it asked for. +/// `Ok(None)` means the database is unpermissioned and the data table resolves +/// through its own connection. On a permissioned one, naming no role selects the +/// configured default and an unknown role is an error; on an unpermissioned one, +/// naming any role other than `admin` is an error too — silently ignoring it +/// would run the script with more privileges than it asked for. fn datatable_role_entry<'a>( - datatable: &'a DataTable, + record: Option<&'a DatabasePermissions>, name: &str, role: Option<&str>, ) -> Result> { - let Some(permissions) = datatable.permissions.as_ref().filter(|p| p.enabled) else { + let Some(permissions) = record.map(|r| &r.permissions).filter(|p| p.enabled) else { return match role { Some(role) if role != ADMIN_DATATABLE_ROLE => Err(Error::BadRequest(format!( "Cannot use role '{role}': permissions are not enabled on data table '{name}'. \ @@ -2030,33 +1834,38 @@ fn datatable_role_entry<'a>( Ok(Some((role_name.as_str(), role_entry))) } -/// Resolve which postgres login the data table should be reached through, and -/// authorize it. +/// Whether `access`, made from workspace `w_id`, may run as `role` of a database +/// whose permissions `owner_w_id` owns. /// -/// Returns the `(user, password)` to swap into the connection, or `None` when -/// the data table's own credentials are to be used — which is every -/// unpermissioned data table, and the `admin` role of a permissioned one. -async fn resolve_datatable_role( +/// Tenants are principals of the owning workspace, and so is the admin bypass: +/// a caller from another workspace — a fork's copy of the data table, a +/// detached workspace — is looked up in the owning workspace by email, and is +/// evaluated as that member, or refused when they are none. Being admin of the +/// calling workspace, which any member is of a fork they made, counts for +/// nothing there. Superadmins reach every role from anywhere. A job permissioned +/// as a group or folder of another workspace is refused: those names mean +/// nothing outside the workspace that defined them. +/// +/// Authorization: this is the check. `DatatableAccess::Unchecked` is allowed +/// through, for callers that have authorized already. +pub async fn can_use_datatable_role_in_owner_workspace( db: &DB, + owner_w_id: &str, w_id: &str, - name: &str, - datatable: &DataTable, - role: Option<&str>, - access: DatatableAccess<'_>, -) -> Result> { - let Some((role_name, role_entry)) = datatable_role_entry(datatable, name, role)? else { - return Ok(None); - }; - - let allowed = match access { - DatatableAccess::Unchecked => true, - DatatableAccess::NoIdentity => false, - DatatableAccess::Authed(ref authed) => can_use_datatable_role(role_entry, authed), + role: &DataTableRole, + access: &DatatableAccess<'_>, +) -> Result { + let (permissioned_as, email): (String, String) = match access { + DatatableAccess::Unchecked => return Ok(true), + DatatableAccess::NoIdentity => return Ok(false), + DatatableAccess::Authed(authed) => { + if w_id == owner_w_id { + return Ok(can_use_datatable_role(role, authed)); + } + (format!("u/{}", authed.username), authed.email.to_string()) + } DatatableAccess::PermissionedAs { permissioned_as, email } => { - let authed = - crate::auth::fetch_authed_from_permissioned_as(permissioned_as, email, w_id, db) - .await?; - can_use_datatable_role(role_entry, &authed.to_authed_ref()) + (permissioned_as.to_string(), email.to_string()) } DatatableAccess::Job(job_id) => { let job = sqlx::query!( @@ -2067,17 +1876,64 @@ async fn resolve_datatable_role( .fetch_optional(db) .await? .ok_or_else(|| Error::NotFound(format!("job {job_id} not found in {w_id}")))?; - let authed = crate::auth::fetch_authed_from_permissioned_as( - &job.permissioned_as, - &job.permissioned_as_email, - w_id, - db, - ) - .await?; - can_use_datatable_role(role_entry, &authed.to_authed_ref()) + (job.permissioned_as, job.permissioned_as_email) } }; - if !allowed { + if w_id == owner_w_id { + let authed = + crate::auth::fetch_authed_from_permissioned_as(&permissioned_as, &email, w_id, db) + .await?; + return Ok(can_use_datatable_role(role, &authed.to_authed_ref())); + } + if crate::auth::is_super_admin_email(db, &email).await? { + return Ok(true); + } + if !permissioned_as.starts_with("u/") { + return Ok(false); + } + // The caller as a member of the owning workspace: their username there, not + // the one they carry in `w_id`. + let Some(username) = sqlx::query_scalar!( + "SELECT username FROM usr WHERE workspace_id = $1 AND email = $2 AND disabled = false", + owner_w_id, + &email + ) + .fetch_optional(db) + .await? + else { + return Ok(false); + }; + let authed = crate::auth::fetch_authed_from_permissioned_as( + &format!("u/{username}"), + &email, + owner_w_id, + db, + ) + .await?; + Ok(can_use_datatable_role(role, &authed.to_authed_ref())) +} + +/// Resolve which postgres login the data table should be reached through, and +/// authorize it. +/// +/// Returns the `(user, password)` to swap into the connection, or `None` when +/// the data table's own credentials are to be used — which is every +/// unpermissioned data table, and the `admin` role of a permissioned one. +async fn resolve_datatable_role( + db: &DB, + w_id: &str, + name: &str, + record: Option<&DatabasePermissions>, + role: Option<&str>, + access: &DatatableAccess<'_>, +) -> Result> { + let Some((role_name, role_entry)) = datatable_role_entry(record, name, role)? else { + return Ok(None); + }; + let owner_w_id = &record + .expect("a role entry comes from a record") + .owner_workspace_id; + if !can_use_datatable_role_in_owner_workspace(db, owner_w_id, w_id, role_entry, access).await? { return Err(Error::NotAuthorized(format!( "Not allowed to use role '{role_name}' of data table '{name}'" ))); @@ -2085,8 +1941,7 @@ async fn resolve_datatable_role( // A role named without a stored credential is not a reason to fall back to // the data table's own connection: that one owns everything, so the caller - // would silently get more than the role they asked for. Exports and - // git-synced settings redact the password, so a restored config lands here. + // would silently get more than the role they asked for. match ( role_entry.pg_rolename.clone(), role_entry.pg_password.clone(), @@ -2099,14 +1954,65 @@ async fn resolve_datatable_role( } } -async fn get_datatable_resource_inner( +/// The connection a data table entry resolves to, before any role is swapped in: +/// the instance's own user for an instance database, the resource as it resolves +/// for a resource-backed one. +/// +/// Authorization: performs none, and returns credentials — callers MUST have +/// authorized the access. +pub async fn resolve_datatable_connection_unchecked( db: &DB, w_id: &str, name: &str, + datatable: &DataTable, replication: bool, - role: Option<&str>, - access: DatatableAccess<'_>, ) -> Result { + if datatable.database.resource_type == DataTableCatalogResourceType::Instance { + let mut pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; + pg_creds.dbname = datatable.database.resource_path.clone(); + if replication { + pg_creds.user = Some("custom_instance_replication_user".to_string()); + pg_creds.password = Some(get_custom_pg_instance_replication_password(&db).await?); + } else { + pg_creds.user = Some("custom_instance_user".to_string()); + pg_creds.password = Some(get_custom_pg_instance_password(&db).await?); + } + serde_json::to_value(&pg_creds) + .map_err(|e| Error::internal_err(format!("Error serializing pg creds: {}", e))) + } else { + // Name the data table too: the caller asked for one by name, and a bare + // "resource f/x/y does not exist" leaves them to work out which one points at it. + transform_json_unchecked( + &serde_json::Value::String(format!("$res:{}", datatable.database.resource_path)), + w_id, + db, + ) + .await + .map_err(|e| match e { + Error::NotFound(m) => Error::NotFound(format!("data table {name}: {m}")), + e => e, + }) + } +} + +/// A data table entry's config, its resolved connection and the key of the +/// database it reaches. +/// +/// Authorization: performs none, and returns credentials — callers MUST have +/// authorized the access. +pub async fn resolve_datatable_database_unchecked( + db: &DB, + w_id: &str, + name: &str, +) -> Result<(DataTable, serde_json::Value, String)> { + let datatable = read_datatable_config(db, w_id, name).await?; + let resolved = + resolve_datatable_connection_unchecked(db, w_id, name, &datatable, false).await?; + let key = datatable_database_key(&datatable.database, &resolved); + Ok((datatable, resolved, key)) +} + +async fn read_datatable_config(db: &DB, w_id: &str, name: &str) -> Result { let datatables = sqlx::query_scalar!( r#" SELECT ws.datatable->'datatables' AS datatables @@ -2124,72 +2030,28 @@ async fn get_datatable_resource_inner( .and_then(|d| d.get(name)) .filter(|v| !v.is_null()) .ok_or_else(|| datatable_not_found_error(name, datatables.as_ref()))?; - let datatable = serde_json::from_value::(datatable.clone())?; - - let internal = matches!(access, DatatableAccess::Unchecked); - let role_override = resolve_datatable_role(db, w_id, name, &datatable, role, access).await?; + Ok(serde_json::from_value::(datatable.clone())?) +} +async fn get_datatable_resource_inner( + db: &DB, + w_id: &str, + name: &str, + replication: bool, + role: Option<&str>, + access: DatatableAccess<'_>, +) -> Result { + let datatable = read_datatable_config(db, w_id, name).await?; let mut db_resource = - if datatable.database.resource_type == DataTableCatalogResourceType::Instance { - let mut pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; - pg_creds.dbname = datatable.database.resource_path.clone(); - if replication { - pg_creds.user = Some("custom_instance_replication_user".to_string()); - pg_creds.password = Some(get_custom_pg_instance_replication_password(&db).await?); - } else { - pg_creds.user = Some("custom_instance_user".to_string()); - pg_creds.password = Some(get_custom_pg_instance_password(&db).await?); - } - serde_json::to_value(&pg_creds) - .map_err(|e| Error::internal_err(format!("Error serializing pg creds: {}", e)))? - } else { - // Name the data table too: the caller asked for one by name, and a bare - // "resource f/x/y does not exist" leaves them to work out which one points at it. - transform_json_unchecked( - &serde_json::Value::String(format!("$res:{}", datatable.database.resource_path)), - w_id, - db, - ) - .await - .map_err(|e| match e { - Error::NotFound(m) => Error::NotFound(format!("data table {name}: {m}")), - e => e, - })? - }; + resolve_datatable_connection_unchecked(db, w_id, name, &datatable, replication).await?; - // Before the swap below, not merely outside it: the recorded identity is of the - // connection as the resource resolves it, and swapping a role's login into - // `user` would make every comparison fail. - if resolution_proves_database_identity(internal, &datatable) { - ensure_datatable_database_unchanged(name, &datatable, &db_resource)?; - } - - // An unpermissioned data table must not be a second door onto a database - // another data table governs. The writes that could make it one — the - // settings form, a resource write — refuse, but none of them is transactional - // with the opt-in, and a `$var:` a resource references changes under it - // through no write of the resource at all; here, on the resolved connection, - // is where the answer is authoritative. Internal callers are the guards - // themselves and the data table's own administration. - if !internal && !datatable.permissions.as_ref().is_some_and(|p| p.enabled) { - if let Some((gw, gname)) = governed_datatable_reaching( - db, - &physical_database_identity(&db_resource), - &[(w_id, name)], - ) - .await? - { - return Err(Error::NotAuthorized(format!( - "Data table '{name}' reaches the database of {}, whose role permissions are \ - enabled: its own connection would bypass them. Use that data table instead.", - if gw == w_id { - format!("data table '{gname}'") - } else { - "a data table of another workspace".to_string() - } - ))); - } - } + // The permissions are the database's, looked up by what the entry resolved + // to: a copy of the entry anywhere reaches the same row, and an entry pointed + // somewhere else reaches that database's. + let key = datatable_database_key(&datatable.database, &db_resource); + let record = database_permissions_by_key(db, &key).await?; + let role_override = + resolve_datatable_role(db, w_id, name, record.as_ref(), role, &access).await?; // The role logs in as itself rather than through `SET ROLE`, which a script // could `RESET ROLE` its way back out of and regain admin's privileges. @@ -3433,331 +3295,155 @@ mod tests { #[test] fn datatable_pg_role_names_are_readable_and_never_collide() { - assert!(datatable_pg_role_name("acme", "main", "analyst").starts_with("wm_analyst_")); - - // The readable half identifies nothing — every pair below shares it, or - // could — so only the hash keeps them apart. They must stay apart: two - // Windmill roles sharing one Postgres login would share its grants. - let collide = [ - // different workspaces - (("acme", "main", "analyst"), ("globex", "main", "analyst")), - // '-' and '_' both sanitize to '_' - (("acme", "main", "analyst-1"), ("acme", "main", "analyst_1")), - // the field separator is itself '_', so the boundary can shift - (("acme", "sales_ro", "x"), ("acme", "sales", "ro_x")), - // case is folded - (("acme", "main", "Analyst"), ("acme", "main", "analyst")), - ]; - for ((w1, d1, r1), (w2, d2, r2)) in collide { - assert_ne!( - datatable_pg_role_name(w1, d1, r1), - datatable_pg_role_name(w2, d2, r2), - "{w1}/{d1}/{r1} vs {w2}/{d2}/{r2}" - ); - } - - // The readable half is a prefix anyone can reproduce, so the discriminator - // is the whole of it — and a short one is searchable, not merely unlucky: - // these two triples shared a name when it was four bytes wide. + assert!(datatable_pg_role_name("instance:dt_main", "analyst").starts_with("wm_analyst_")); + // Two databases, or two roles, never share a login, however the readable + // part collides. assert_ne!( - datatable_pg_role_name("acme", "dt34415", "analyst"), - datatable_pg_role_name("acme", "dt50535", "analyst") + datatable_pg_role_name("instance:dt_main", "analyst"), + datatable_pg_role_name("instance:dt_other", "analyst") ); - let (_, discriminator) = datatable_pg_role_name("acme", "main", "analyst") - .rsplit_once('_') - .map(|(a, b)| (a.to_string(), b.to_string())) - .unwrap(); - assert_eq!(discriminator.len(), 32, "16 bytes of hash, hex-encoded"); - - // Postgres silently truncates past 63 bytes, which would undo the above. - for name in [ - datatable_pg_role_name(&"w".repeat(60), "main", "analyst"), - datatable_pg_role_name("acme", &"d".repeat(200), &"r".repeat(60)), - ] { - assert!(name.len() <= PG_IDENTIFIER_MAX_LEN, "{name}"); - } assert_ne!( - datatable_pg_role_name(&"w".repeat(60), "main", "analyst"), - datatable_pg_role_name(&"w".repeat(61), "main", "analyst") + datatable_pg_role_name("instance:dt_main", "analyst-1"), + datatable_pg_role_name("instance:dt_main", "analyst_1") + ); + // A long role name is truncated to what Postgres keeps, discriminator intact. + let long = datatable_pg_role_name("instance:dt_main", &"r".repeat(200)); + assert!(long.len() <= PG_IDENTIFIER_MAX_LEN); + assert!(long.ends_with( + &datatable_pg_role_name("instance:dt_main", &"r".repeat(200))[long.len() - 32..] + )); + } + + fn resolved(host: &str, dbname: &str, user: &str) -> serde_json::Value { + serde_json::json!({ "host": host, "port": 5432, "dbname": dbname, "user": user, "password": "x" }) + } + + fn pg(path: &str) -> DataTableDatabase { + DataTableDatabase { + resource_type: DataTableCatalogResourceType::Postgresql, + resource_path: path.to_string(), + } + } + + /// The key is the database, not the entry: the login and the resource path + /// play no part, the host, port and database name do. + #[test] + fn a_database_key_is_the_database_whatever_reaches_it() { + let instance = DataTableDatabase { + resource_type: DataTableCatalogResourceType::Instance, + resource_path: "dt_main".to_string(), + }; + assert_eq!( + datatable_database_key(&instance, &serde_json::Value::Null), + "instance:dt_main" + ); + assert_eq!( + datatable_database_key(&pg("u/a/pg"), &resolved("db", "prod", "app")), + datatable_database_key(&pg("f/other/pg"), &resolved("db", "prod", "postgres")) + ); + assert_ne!( + datatable_database_key(&pg("u/a/pg"), &resolved("db", "prod", "app")), + datatable_database_key(&pg("u/a/pg"), &resolved("db", "staging", "app")) ); } - fn permissioned(roles: &[(&str, &[&str])]) -> DataTable { + fn record(roles: &[(&str, &[&str])], default_role: Option<&str>) -> DatabasePermissions { let mut map = std::collections::BTreeMap::new(); + map.insert(ADMIN_DATATABLE_ROLE.to_string(), DataTableRole::default()); for (name, tenants) in roles { map.insert( name.to_string(), DataTableRole { - pg_rolename: (*name != ADMIN_DATATABLE_ROLE).then(|| format!("wm_{name}")), - pg_password: (*name != ADMIN_DATATABLE_ROLE).then(|| "pwd".to_string()), + pg_rolename: Some(format!("wm_{name}")), + pg_password: Some("pw".to_string()), tenants: tenants.iter().map(|t| t.to_string()).collect(), }, ); } - DataTable { - database: DataTableDatabase { - resource_type: DataTableCatalogResourceType::Instance, - resource_path: "db".to_string(), - }, - forked_from: None, - migrations_enabled: None, - permissions: Some(DataTablePermissions { + DatabasePermissions { + database_key: "instance:dt_main".to_string(), + owner_workspace_id: "acme".to_string(), + permissions: DataTablePermissions { enabled: true, roles: map, - default_role: None, - database_identity: None, - physical_identity: None, - }), + default_role: default_role.map(|r| r.to_string()), + }, } } #[test] fn datatable_role_lookup_defaults_to_admin_and_rejects_unknown_roles() { - let dt = permissioned(&[(ADMIN_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); - - // No role named -> admin, which reuses the data table's own connection. - let (name, entry) = datatable_role_entry(&dt, "main", None).unwrap().unwrap(); + let record = record(&[("analyst", &["u/alice"])], None); + let (name, _) = datatable_role_entry(Some(&record), "main", None) + .unwrap() + .unwrap(); assert_eq!(name, ADMIN_DATATABLE_ROLE); - assert!(entry.pg_rolename.is_none()); - - let (name, entry) = datatable_role_entry(&dt, "main", Some("analyst")) + let (name, _) = datatable_role_entry(Some(&record), "main", Some("analyst")) .unwrap() .unwrap(); assert_eq!(name, "analyst"); - assert_eq!(entry.pg_rolename.as_deref(), Some("wm_analyst")); - - assert!(datatable_role_entry(&dt, "main", Some("nope")).is_err()); + assert!(datatable_role_entry(Some(&record), "main", Some("nope")).is_err()); } #[test] fn naming_no_role_selects_the_configured_default() { - let mut dt = permissioned(&[(ADMIN_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); - dt.permissions.as_mut().unwrap().default_role = Some("analyst".to_string()); - - // The point of a default: a script that names nothing gets the role the - // workspace chose, not admin. - let (name, entry) = datatable_role_entry(&dt, "main", None).unwrap().unwrap(); + let record = record(&[("analyst", &["*"])], Some("analyst")); + let (name, _) = datatable_role_entry(Some(&record), "main", None) + .unwrap() + .unwrap(); assert_eq!(name, "analyst"); - assert_eq!(entry.pg_rolename.as_deref(), Some("wm_analyst")); - - // Naming admin explicitly still reaches admin. - let (name, _) = datatable_role_entry(&dt, "main", Some(ADMIN_DATATABLE_ROLE)) - .unwrap() - .unwrap(); - assert_eq!(name, ADMIN_DATATABLE_ROLE); } - /// The internal machinery — role DDL, migration bookkeeping, fork snapshots — - /// is built on the unchecked resolution and needs admin's privileges, so a - /// configured default role must not divert it. + /// Without permissions on the database only `admin` may be named: a role + /// silently ignored would run with more privileges than it asked for. #[test] - fn the_unchecked_resolution_is_admin_even_when_another_role_is_default() { - let mut dt = permissioned(&[(ADMIN_DATATABLE_ROLE, &[]), ("analyst", &[])]); - dt.permissions.as_mut().unwrap().default_role = Some("analyst".to_string()); - - let (name, entry) = datatable_role_entry(&dt, "main", Some(ADMIN_DATATABLE_ROLE)) - .unwrap() - .unwrap(); - assert_eq!(name, ADMIN_DATATABLE_ROLE); - // admin reuses the data table's own connection rather than a created login. - assert!(entry.pg_rolename.is_none()); - } - - /// Which resolutions have to prove it — the question the presence of a login - /// to swap in cannot answer, since `admin` has none and still resolves through - /// the data table's own connection. - #[test] - fn every_identified_caller_of_a_permissioned_data_table_proves_it() { - let permissioned_dt = - permissioned(&[(ADMIN_DATATABLE_ROLE, &["u/alice"]), ("analyst", &[])]); - let mut unpermissioned = - permissioned(&[(ADMIN_DATATABLE_ROLE, &["u/alice"]), ("analyst", &[])]); - unpermissioned.permissions.as_mut().unwrap().enabled = false; - - // Whatever role the caller lands on, including the one with no login. - assert!(resolution_proves_database_identity(false, &permissioned_dt)); - // The machinery that repairs a data table resolves as admin to do it, and - // a moved one is exactly what it is repairing. - assert!(!resolution_proves_database_identity(true, &permissioned_dt)); - // Nothing was created anywhere, so there is nothing to have moved away from. - assert!(!resolution_proves_database_identity(false, &unpermissioned)); - } - - /// The roles of a data table live in one database, so a config that points - /// somewhere else must not be able to use them there. - #[test] - fn a_role_is_refused_once_its_data_table_points_at_another_database() { - let resolved = |host: &str, password: &str| { - serde_json::json!({ - "host": host, "port": 5432, "dbname": "app", "user": "owner", - "password": password - }) - }; - // A password rotation is not a move; the host changing is. - assert_eq!( - datatable_database_identity(&resolved("db.internal", "one")), - datatable_database_identity(&resolved("db.internal", "two")) - ); - assert_ne!( - datatable_database_identity(&resolved("db.internal", "one")), - datatable_database_identity(&resolved("elsewhere.internal", "one")) - ); - - let mut dt = permissioned(&[(ADMIN_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); - dt.database.resource_type = DataTableCatalogResourceType::Postgresql; - dt.permissions.as_mut().unwrap().database_identity = - Some(datatable_database_identity(&resolved("db.internal", "one"))); - - // Reached where the roles were created, through a rotated password. - ensure_datatable_database_unchanged("main", &dt, &resolved("db.internal", "two")).unwrap(); - // Repointed — however the resource got there, including through a `$var:` - // no guard on the resource itself would see. + fn naming_a_role_on_an_unpermissioned_database_is_refused() { + assert!(datatable_role_entry(None, "main", None).unwrap().is_none()); assert!( - ensure_datatable_database_unchanged("main", &dt, &resolved("elsewhere", "one")) - .is_err() - ); - // A config that never recorded one cannot claim to match: the roles it - // names were created against a database nobody wrote down. - dt.permissions.as_mut().unwrap().database_identity = None; - assert!( - ensure_datatable_database_unchanged("main", &dt, &resolved("db.internal", "one")) - .is_err() - ); - } - - #[test] - fn naming_a_role_on_an_unpermissioned_datatable_is_refused() { - let mut dt = permissioned(&[(ADMIN_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); - dt.permissions.as_mut().unwrap().enabled = false; - - // Silently ignoring the role would run the script as the data table's own - // connection — more privilege than it asked for. - assert!(datatable_role_entry(&dt, "main", Some("analyst")).is_err()); - // admin and "no role" both mean the existing connection, so they are fine. - assert!(datatable_role_entry(&dt, "main", None).unwrap().is_none()); - assert!( - datatable_role_entry(&dt, "main", Some(ADMIN_DATATABLE_ROLE)) + datatable_role_entry(None, "main", Some(ADMIN_DATATABLE_ROLE)) .unwrap() .is_none() ); + assert!(datatable_role_entry(None, "main", Some("analyst")).is_err()); + let mut off = record(&[("analyst", &["*"])], None); + off.permissions.enabled = false; + assert!(datatable_role_entry(Some(&off), "main", Some("analyst")).is_err()); } - /// The tenant walk is what every flow that renames or deletes a principal - /// has to run, so it answers for the shapes those flows produce. - #[test] - fn a_tenant_follows_a_rename_and_leaves_with_a_deletion() { - let sample = || { - serde_json::json!({ - "datatables": { - "main": { "permissions": { "roles": { - "admin": { "tenants": [] }, - "analyst": { "tenants": ["u/alice", "g/devs", "*"] } - }}}, - "other": { "permissions": { "enabled": false, "roles": { - "reader": { "tenants": ["f/team", "u/alice"] } - }}} - } - }) - }; - - // A rename matches the whole tenant, so a group or folder of the same - // name — and the wildcard — are not touched. - let mut renamed = sample(); - assert!(rename_datatable_tenant(&mut renamed, "u/alice", "u/bob")); - assert_eq!( - renamed["datatables"]["main"]["permissions"]["roles"]["analyst"]["tenants"], - serde_json::json!(["u/bob", "g/devs", "*"]) - ); - // Every data table, including one whose permissions are off: a later - // re-enable would otherwise bring the stale name back. - assert_eq!( - renamed["datatables"]["other"]["permissions"]["roles"]["reader"]["tenants"], - serde_json::json!(["f/team", "u/bob"]) - ); - - // A deletion takes the tenant away instead: the name is free afterwards. - let mut removed = sample(); - assert!(remove_datatable_tenant(&mut removed, "g/devs")); - assert_eq!( - removed["datatables"]["main"]["permissions"]["roles"]["analyst"]["tenants"], - serde_json::json!(["u/alice", "*"]) - ); - - // Nothing to do reports nothing to write. - let mut untouched = sample(); - assert!(!rename_datatable_tenant( - &mut untouched, - "u/carol", - "u/dave" - )); - assert!(!remove_datatable_tenant(&mut untouched, "f/nope")); - assert_eq!(untouched, sample()); - } - - /// A data table names its database by resource path, so a flow that moves - /// the resource has to move the name with it. #[test] fn a_data_table_follows_its_resource_across_a_path_move() { let mut settings = serde_json::json!({ "datatables": { - "main": { "database": { "resource_type": "postgresql", "resource_path": "u/alice/mypg" } }, - "managed": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }, - "elsewhere": { "database": { "resource_type": "postgresql", "resource_path": "f/team/pg" } } + "main": { "database": { "resource_type": "postgresql", "resource_path": "u/alice/pg" } }, + "other": { "database": { "resource_type": "postgresql", "resource_path": "f/team/pg" } }, + "instance": { "database": { "resource_type": "instance", "resource_path": "u/alice/pg" } } } }); assert!(move_datatable_resource_paths( &mut settings, "u/alice/", - "f/team/" + "u/alicia/" )); assert_eq!( settings["datatables"]["main"]["database"]["resource_path"], - "f/team/mypg" - ); - // A path that does not start with what moved is left as it is. - assert_eq!( - settings["datatables"]["managed"]["database"]["resource_path"], - "dt_main" + "u/alicia/pg" ); assert_eq!( - settings["datatables"]["elsewhere"]["database"]["resource_path"], + settings["datatables"]["other"]["database"]["resource_path"], "f/team/pg" ); + // An instance database name is not a path, whatever it looks like. + assert_eq!( + settings["datatables"]["instance"]["database"]["resource_path"], + "u/alicia/pg" + ); assert!(!move_datatable_resource_paths( &mut settings, "u/nobody/", - "u/somebody/" + "u/x/" )); } - #[test] - fn datatable_settings_export_drops_role_passwords() { - let settings = serde_json::json!({ - "datatables": { - "main": { - "database": { "resource_type": "instance", "resource_path": "db" }, - "permissions": { "enabled": true, "roles": { - "admin": { "tenants": [] }, - "analyst": { "pg_rolename": "wm_x", "pg_password": "s3cret", "tenants": ["u/alice"] } - }} - }, - "other": { "database": { "resource_type": "instance", "resource_path": "db2" } } - } - }); - // The audit parameter is that same redaction, not a Debug of the settings. - assert!(!datatable_settings_for_audit(&settings).contains("s3cret")); - let redacted = redact_datatable_settings_for_export(Some(settings)).unwrap(); - let analyst = &redacted["datatables"]["main"]["permissions"]["roles"]["analyst"]; - assert!(analyst.get("pg_password").is_none()); - // Everything else survives: the export is still a usable settings file. - assert_eq!(analyst["pg_rolename"], "wm_x"); - assert_eq!(analyst["tenants"][0], "u/alice"); - assert_eq!( - redacted["datatables"]["other"]["database"]["resource_path"], - "db2" - ); - } - #[test] fn test_parse_fork_branch() { // Generated fork (`wm-fork-abc`) and dev workspace (`staging`) forms. diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index a90c5b2b83..4a9aab623f 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -1196,27 +1196,6 @@ async fn create_resource( .await?; } if update_if_exists { - // An upsert over an existing row is an edit, so it answers to the same - // rule: a permissioned data table's roles live in the database its - // resource names, and that is not free to move while they exist. - let previous = sqlx::query_scalar!( - "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", - resource.path, - w_id - ) - .fetch_optional(&db) - .await? - .flatten(); - let nvalue: serde_json::Value = serde_json::from_str(raw_json.0.get()) - .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?; - windmill_common::workspaces::ensure_resource_identity_change_allowed( - &db, - &w_id, - &resource.path, - previous.as_ref(), - Some(&nvalue), - ) - .await?; sqlx::query!( "INSERT INTO resource (workspace_id, path, value, description, resource_type, created_by, edited_at, labels) @@ -1234,17 +1213,6 @@ async fn create_resource( .await .map_err(sanitize_db_error)?; } else { - // A data table entry may already name this path, its resource deleted since. - let nvalue: serde_json::Value = serde_json::from_str(raw_json.0.get()) - .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?; - windmill_common::workspaces::ensure_resource_identity_change_allowed( - &db, - &w_id, - &resource.path, - None, - Some(&nvalue), - ) - .await?; // Create-only (the default): DO NOTHING + a row-count guard, so a path that appears between // check_path_conflict above and this insert is rejected rather than overwritten. A plain // DO UPDATE here would clobber a concurrently-created resource, breaking create-only callers @@ -1355,7 +1323,6 @@ async fn delete_resource( { return Err(Error::PermissionDenied(msg)); } - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; let mut tx = user_db.begin(&authed).await?; // Capture resource data for trashbin before deleting @@ -1662,10 +1629,6 @@ async fn delete_resources_bulk( { return Err(Error::PermissionDenied(msg)); } - for path in &request.paths { - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; - } - let mut tx = user_db.begin(&authed).await?; // Capture resources for trashbin per path before bulk delete, and @@ -1856,56 +1819,6 @@ async fn update_resource( return Err(Error::PermissionDenied(msg)); } - // A rename takes the resource out from under whatever names its path — and - // puts it under whatever names the new one. - if let Some(npath) = ns.path.as_deref().filter(|npath| *npath != path) { - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; - let arriving: Option = match ns.value.as_ref() { - Some(v) => Some( - serde_json::from_str(v.get()) - .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?, - ), - None => sqlx::query_scalar!( - "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", - path, - &w_id - ) - .fetch_optional(&db) - .await? - .flatten(), - }; - windmill_common::workspaces::ensure_resource_identity_change_allowed( - &db, - &w_id, - npath, - None, - arriving.as_ref(), - ) - .await?; - } - // Same as `set_resource_value`: the identity a permissioned data table's - // roles were created against is not free to move underneath them. - if let Some(nvalue) = ns.value.as_ref() { - let previous = sqlx::query_scalar!( - "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", - path, - &w_id - ) - .fetch_optional(&db) - .await? - .flatten(); - let nvalue: serde_json::Value = serde_json::from_str(nvalue.get()) - .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?; - windmill_common::workspaces::ensure_resource_identity_change_allowed( - &db, - &w_id, - path, - previous.as_ref(), - Some(&nvalue), - ) - .await?; - } - let mut sqlb = SqlBuilder::update_table("resource"); sqlb.and_where_eq("path", "?".bind(&path)); sqlb.and_where_eq("workspace_id", "?".bind(&w_id)); @@ -2220,23 +2133,6 @@ async fn set_resource_value( // A data table's roles live in the database its resource points at, so the // identity behind that path is not free to move while they exist. - let previous = sqlx::query_scalar!( - "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", - path, - w_id - ) - .fetch_optional(db) - .await? - .flatten(); - windmill_common::workspaces::ensure_resource_identity_change_allowed( - db, - w_id, - path, - previous.as_ref(), - value.as_ref(), - ) - .await?; - let mut tx = user_db.clone().begin(authed).await?; // `RETURNING resource_type` rather than a second lookup: the advisory below has to know the diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 8ef64e3fd4..8f288dc85e 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -744,10 +744,6 @@ async fn delete_variable( return Err(Error::PermissionDenied(msg)); } - // This takes the resource linked at the same path with it, so it answers to - // the same rule as deleting that resource directly. - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; - // Check if variable is a secret before deleting (for Vault cleanup) let is_secret = sqlx::query_scalar!( "SELECT is_secret FROM variable WHERE path = $1 AND workspace_id = $2", @@ -922,11 +918,6 @@ async fn delete_variables_bulk( return Err(Error::PermissionDenied(msg)); } - // Each of these takes the resource linked at the same path with it. - for path in &request.paths { - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; - } - // Query which paths are secrets before deletion (for Vault cleanup) let secret_paths: Vec = sqlx::query_scalar!( "SELECT path FROM variable WHERE path = ANY($1) AND workspace_id = $2 AND is_secret = true", @@ -1108,11 +1099,6 @@ async fn update_variable( // source path. if let Some(npath) = ns.path.as_deref() { check_scopes(&authed, || format!("variables:write:{}", npath))?; - // It moves the resource linked at the same path too, which is how a - // permissioned data table finds its database. - if npath != path { - windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?; - } check_proper_path(npath)?; } let authed = maybe_refresh_folders(&path, &w_id, authed, &db).await; diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index bb68c9fad4..c9452a5495 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -49,14 +49,17 @@ onSaved?: () => void } = $props() - // The server refuses to enable permissions in a fork: its data table points - // either at the database of the workspace it was forked from, where the roles - // would be invisible to that workspace's own config, or at a copy the fork can - // drop. Turning them off stays available, so one that already has them can be - // rid of them. + // The server refuses to turn permissions on from a fork: its data table points + // either at the database of the workspace it was forked from, which is where + // to set them, or at a copy the fork can drop. const isFork = $derived( !!$userWorkspaces.find((w) => w.id === workspace)?.parent_workspace_id ) + // Permissions belong to the database, and are managed from the workspace that + // turned them on; from any other workspace reaching the same database they are + // read-only here. + let editable = $state(true) + let ownerWorkspace = $state(undefined) // Matches every workspace member, unlike the `all` group whose membership is // bookkeeping that can drift. @@ -127,6 +130,8 @@ loaded.unshift({ id: randomUUID(), name: ADMIN_DATATABLE_ROLE, tenants: [] }) } enabled = res.enabled + editable = res.editable + ownerWorkspace = res.owner_workspace_id roles = loaded defaultRoleId = loaded.find((r) => r.name === res.default_role)?.id ?? loaded[0]?.id saved = { @@ -263,7 +268,7 @@
- {#if isFork && !enabled} + {#if !editable && ownerWorkspace} + + This data table reaches a database whose permissions were turned on in workspace + {ownerWorkspace}. Its roles and tenants are that workspace's, and its admins change them + there. + + {:else if isFork && !enabled} - A fork's data table points either at that workspace's database, where roles created - here would be invisible to its own configuration, or at a copy this fork can drop. - Enable permissions there instead, once this fork is deleted. + A fork's data table points either at that workspace's database, which is where to + enable permissions, or at a copy this fork can drop. {/if} @@ -382,7 +392,7 @@