[ee] [do not merge yet] refactor: replace email with permissioned_as for triggers/schedules#8439
[ee] [do not merge yet] refactor: replace email with permissioned_as for triggers/schedules#8439
Conversation
Add a new `permissioned_as` column (format: `u/{username}`, `g/{group}`,
or raw email) to all trigger tables and schedule. This value is used
directly for job permission checks, removing the need for email lookups
when creating/updating triggers.
- Migration: add permissioned_as to all 9 trigger tables + schedule,
drop email from trigger tables (schedule keeps it for backwards compat)
- Backend: resolve_email() (async, DB) -> resolve_permissioned_as() (sync)
- Email cache: get_email_from_permissioned_as() with quick_cache for
places that still need email (fetch_api_authed, schedule backwards compat)
- Frontend: rename email/preserve_email -> permissioned_as/preserve_permissioned_as
in deploy data and OpenAPI schemas
- Tests updated for new field names and u/{username} format
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deploying windmill with
|
| Latest commit: |
b4da03d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e878a416.windmill.pages.dev |
| Branch Preview URL: | https://refactor-triggers-permission.windmill.pages.dev |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests that call TriggerCrud and Listener trait methods directly to verify dynamic SQL correctly references the permissioned_as column. Covers get_trigger_by_path, list_triggers, set_trigger_mode, and fetch_enabled_unlistened_triggers for all trigger types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… groups - Schedule: permissioned_as only set on create, not on edit/set_enabled - Schedule: stop reading email column, use get_email_from_permissioned_as - Triggers: use fetch_api_authed_from_permissioned_as instead of edited_by - Triggers: rename listener fields for clarity (username -> edited_by) - Fix audit author username for group permissioned_as (g/test -> group-test) to match session.user, preventing RLS policy violations on audit_partitioned - OpenAPI: remove permissioned_as/preserve_permissioned_as from EditSchedule - Add backwards-compat comments for schedule email writes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @hugocasa's task in 6m 37s —— View job Code Review: Replace
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| @@ -513,7 +509,7 @@ | |||
| es.cron_version, | |||
There was a problem hiding this comment.
Bug: edit_schedule does not update permissioned_as in the database.
The UPDATE query updates email (via COALESCE($24, email)) but never sets permissioned_as. After a schedule edit, the permissioned_as column retains its old value while email and edited_by are updated to the editing user's values.
When push_scheduled_job runs, it reads schedule.permissioned_as (the stale value) and resolves email from it — so the schedule still runs as the original creator, not the editing user. This is inconsistent with create_schedule (which sets permissioned_as) and with all trigger types (which update permissioned_as on edit and set_mode).
You should add permissioned_as = $N to the UPDATE SET clause and bind resolve_permissioned_as(...) similar to create_schedule.
| ), | ||
| ) | ||
| .await?; | ||
| if let Some(on_behalf_of) = windmill_common::check_on_behalf_of_preservation( | ||
| es.email.as_deref(), | ||
| es.preserve_email.unwrap_or(false), | ||
| &authed, | ||
| &authed.username, | ||
| ) { | ||
| audit_log( | ||
| &mut *tx, | ||
| &authed, | ||
| "schedule.on_behalf_of", | ||
| ActionKind::Update, | ||
| &w_id, | ||
| Some(path), | ||
| Some( | ||
| [ | ||
| ("on_behalf_of", on_behalf_of.as_str()), | ||
| ("action", "update"), | ||
| ] | ||
| .into(), | ||
| ), | ||
| ) | ||
| .await?; | ||
| } | ||
|
|
||
| if schedule.enabled { | ||
| tx = push_scheduled_job(&db, tx, &schedule, None, None).await?; |
There was a problem hiding this comment.
Bug: on_behalf_of audit log removed from edit_schedule but kept in create_schedule.
The edit_schedule path previously had an on_behalf_of audit log (parallel to create_schedule), which was removed in this PR. However, the EditSchedule struct also had its email/preserve_email fields removed — so the audit log removal is a logical consequence.
But this means that when a schedule is deployed to another workspace via the deploy UI, the edit path will silently lose the permissioned_as preservation (since EditSchedule no longer accepts permissioned_as/preserve_permissioned_as). Is this intentional? The frontend still sends permissioned_as for schedule deploy data (see utils_deployable.ts:245), but EditSchedule won't deserialize it.
backend/windmill-common/src/lib.rs
Outdated
| pub fn resolve_on_behalf_of_permissioned_as( | ||
| on_behalf_of_permissioned_as: Option<&str>, | ||
| preserve: bool, | ||
| authed: &impl db::Authable, | ||
| ) -> Option<String> { | ||
| if on_behalf_of_permissioned_as.is_some() { | ||
| if preserve && can_preserve_on_behalf_of(authed) { | ||
| on_behalf_of_permissioned_as.map(|s| s.to_string()) | ||
| } else { | ||
| Some(users::username_to_permissioned_as(authed.username())) | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: resolve_on_behalf_of_permissioned_as appears unused.
This function was added but I can't find any callers. grep shows it only exists in this file. If it's meant for future use, consider removing it until needed to keep the code clean.
| @@ -808,6 +782,7 @@ | |||
| args AS "args: _", | |||
There was a problem hiding this comment.
Question: set_enabled updates email but not permissioned_as.
Same issue pattern as edit_schedule: the set_enabled path updates email = $2 (to authed.email) but doesn't touch permissioned_as. This means toggling enabled/disabled changes who the schedule's email points to, but permissioned_as (which is now the canonical authority) stays stale.
Since push_scheduled_job now reads permissioned_as instead of email, the email update here is purely for backwards compat — which is fine. But should permissioned_as also be updated to username_to_permissioned_as(&authed.username) to match the behavior of trigger set_mode?
| @@ -62,7 +69,7 @@ export async function deployItem(params: DeployItemParams): Promise<DeployResult | |||
| requestBody: { | |||
There was a problem hiding this comment.
Naming inconsistency: on_behalf_of_email still sent for flows/scripts but now carries a permissioned_as value.
The onBehalfOfPermissionedAs variable is passed as on_behalf_of_email in the request body for flows (line 69, 78) and scripts (line 101). This means the backend on_behalf_of_email field now receives permissioned_as-formatted values like u/admin instead of actual emails. Is the backend already handling this correctly, or should this field also be renamed to avoid confusion?
| if let Some(username) = permissioned_as.strip_prefix("u/") { | ||
| let key = (workspace_id.to_string(), username.to_string()); | ||
| if let Some((email, cached_at)) = EMAIL_CACHE.get(&key) { | ||
| if cached_at.elapsed().as_secs() < EMAIL_CACHE_TTL_SECS { | ||
| return Ok(email); | ||
| } | ||
| } | ||
| let email = sqlx::query_scalar!( |
There was a problem hiding this comment.
Minor: Cache key allocates two Strings on every call, even on cache hit.
The EMAIL_CACHE.get(&key) call requires constructing the key (workspace_id.to_string(), username.to_string()) before checking the cache. Since this function is called on every trigger/schedule execution, consider using a borrowed-key approach or a hash-based lookup to avoid the allocation on hot paths. Not critical given the 60s TTL, but worth noting for a frequently-called function.
| ALTER TABLE http_trigger ADD COLUMN permissioned_as VARCHAR(255) NOT NULL DEFAULT ''; | ||
| UPDATE http_trigger SET permissioned_as = CASE WHEN edited_by LIKE '%@%' THEN edited_by ELSE 'u/' || edited_by END; | ||
| ALTER TABLE http_trigger DROP COLUMN email; |
There was a problem hiding this comment.
Consideration: Migration drops email column without a transaction wrapper.
The pattern ADD COLUMN → UPDATE → DROP COLUMN is repeated 9 times without an explicit BEGIN/TRANSACTION. If the migration fails partway through (e.g., after dropping email from http_trigger but before updating websocket_trigger), you'd end up in a partially-migrated state.
SQLx migrations typically run each file in a transaction, so this may be fine depending on your migration runner config — just worth verifying.
Also: the DEFAULT '' on permissioned_as means any rows inserted between the ADD COLUMN and UPDATE (unlikely but possible during a rolling deploy) would get an empty permissioned_as. The fallback in get_email_from_permissioned_as would treat '' as a raw email and return it as-is, which would be an empty string email — potentially problematic.
| try { | ||
| if (kind === 'sqs') { | ||
| const trigger = await SqsTriggerService.getSqsTrigger({ workspace, path }) | ||
| return trigger.edited_by | ||
| return trigger.permissioned_as | ||
| } else if (kind === 'kafka') { | ||
| const trigger = await KafkaTriggerService.getKafkaTrigger({ workspace, path }) | ||
| return trigger.edited_by | ||
| return trigger.permissioned_as |
There was a problem hiding this comment.
Good change: getTriggerPermissionedAs now returns permissioned_as directly.
Previously getTriggerEmail was returning edited_by (not email!) which was already a misnomer. This rename and change to return permissioned_as is more correct and consistent.
| @@ -467,7 +474,7 @@ export async function getOnBehalfOfEmail( | |||
| const app = await AppService.getAppByPath({ workspace, path }) | |||
There was a problem hiding this comment.
Naming: Function is still called getOnBehalfOfEmail but now returns permissioned_as values.
Consider renaming to getOnBehalfOfPermissionedAs for consistency with the rest of the refactor. The callers (CompareWorkspaces.svelte, DeployWorkspace.svelte) were already renamed to getOnBehalfOfPermissionedAsForDeploy.
…hedule - Frontend: rename onBehalfOfPermissionedAs -> onBehalfOf with comments explaining it carries emails for flows/scripts and permissioned_as for triggers/schedules - Frontend: rename getOnBehalfOfEmail -> getOnBehalfOf, getOnBehalfOfPermissionedAsForDeploy -> getOnBehalfOfForDeploy, customOnBehalfOfEmails -> customOnBehalfOf - Backend: add optional permissioned_as/preserve_permissioned_as to EditSchedule with COALESCE (only updates when provided) - Backend: add on_behalf_of audit log for schedule edit - Backend: remove unused resolve_on_behalf_of_permissioned_as - Tests: remove email assertions from schedule update test (email is just backwards compat, only permissioned_as matters) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ule edit Derive email from the preserved permissioned_as via cache lookup instead of always writing authed.email. This keeps the email column consistent with the old behavior for backwards compat with old workers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Triggers now use permissioned_as (not edited_by) for permissions, so update the deploy UI wording to reflect this. Also update wm_deployers group description to mention schedules and permissioned_as. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When picking a custom user for trigger/schedule deployment, store
u/${username} (permissioned_as format) instead of the email. Flows/scripts
continue to use email format for on_behalf_of_email.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…kind OnBehalfOfSelector now handles the email vs permissioned_as format internally based on kind: - triggers: returns u/username, displays u/username in all options - flows/scripts/apps: returns email, displays username The onSelect callback now takes (choice, value?) where value is already in the correct format. Parent components just store it directly without needing to know about the format difference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Display is now consistent: all kinds show u/username in the selector. The returned value still differs (email for flows/scripts, u/username for triggers) since the backend APIs expect different formats. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The email column was dropped from trigger tables in the migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Migration: remove DEFAULT '', use nullable → populate → SET NOT NULL
- App policy: set both on_behalf_of and on_behalf_of_email for all choices
- OnBehalfOfSelector: return OnBehalfOfDetails {email, permissionedAs} instead of ambiguous value
- Remove unused email field from Capture struct and query
- Rename getSourceEmail/getTargetEmail → getSourceOnBehalfOf/getTargetOnBehalfOf
- Rename test functions from preserve_email to preserve_permissioned_as
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Since the migration no longer uses DEFAULT '', all INSERTs must explicitly provide permissioned_as. Updated test fixtures and schedule_push tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add permissioned_as to workspace export strip list (like edited_by) - Add permissioned_as to CLI TriggerFile Omit list - Fix TriggerExtraProperty.required: email → permissioned_as - Regenerate frontend and CLI types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These directories are gitignored and should not be tracked. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Already stripped in workspace export, no need to also omit from the type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use single concatenated string for cache key instead of (String, String) tuple - Remove permissioned_as from CLI TriggerFile Omit (already stripped in export) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use a borrowed EmailCacheKey(&str, &str) for cache lookups via quick_cache's Equivalent support. Only allocates (String, String) on cache miss for insert. This is called on every trigger fire and schedule push. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The backend always returns permissioned_as (non-optional String), so the schema should reflect that. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
edited_by can be 'group-{name}' for group-owned triggers/schedules.
The migration now correctly maps these to 'g/{name}' format instead
of incorrectly producing 'u/group-{name}'.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 0971392.
Summary
permissioned_ascolumn (u/{username},g/{group}, or raw email) to all 9 trigger tables + schedule tableemailcolumn from trigger tables; schedule keeps it for backwards compat with old workersresolve_permissioned_as()is now sync (no DB lookup needed), replacing asyncresolve_email()get_email_from_permissioned_as()with TTL cache for places that still need email (e.g.fetch_api_authed)email/preserve_email→permissioned_as/preserve_permissioned_asTest plan
cargo check --all-targetspassesnpx svelte-check --threshold errorpasses (0 errors)emailandpermissioned_aspermissioned_as, no longer writesemailpermissioned_asvia cachepermissioned_asfor admin/deployer usersu/{edited_by}EE companion: windmill-labs/windmill-ee-private#refactor-triggers-permission
🤖 Generated with Claude Code