feat(git-sync): sync extra_perms for variables (#11004)

* feat(git-sync): sync extra_perms for variables

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hvv5B8VP5Di4dbcCiVyZyE

* refactor: trim the variable ACL-sync comment to the 4-line limit

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hvv5B8VP5Di4dbcCiVyZyE

* test: cover the revoke direction of variable extra_perms sync

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hvv5B8VP5Di4dbcCiVyZyE

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/windmill-api-groups/src/granular_acls.rs

@@ -318,6 +318,19 @@ async fn add_granular_acl(
             )
             .await?
         }
+        "variable" => {
+            handle_deployment_metadata(
+                &authed.email,
+                &authed.username,
+                &db,
+                &w_id,
+                DeployedObject::Variable { path: path.to_string(), parent_path: None },
+                Some(format!("Variable '{}' changed permissions", path)),
+                true,
+                None,
+            )
+            .await?
+        }
         _ => (),
     }
 
@@ -528,6 +541,19 @@ async fn remove_granular_acl(
                 )
                 .await?
             }
+            "variable" => {
+                handle_deployment_metadata(
+                    &authed.email,
+                    &authed.username,
+                    &db,
+                    &w_id,
+                    DeployedObject::Variable { path: path.to_string(), parent_path: None },
+                    Some(format!("Variable '{}' changed permissions", path)),
+                    true,
+                    None,
+                )
+                .await?
+            }
             _ => (),
         }
     }

backend/windmill-api/src/workspaces_export.rs

@@ -346,7 +346,7 @@ pub(crate) struct ArchiveQueryParams {
     default_ts: Option<String>,
     /// Settings format version: "v1" (default) returns legacy flat format, "v2" returns grouped format
     settings_version: Option<String>,
-    /// Opt-in: include `extra_perms` on flow / script / app rows. Default `false`
+    /// Opt-in: include `extra_perms` on script / flow / app / variable rows. Default `false`
     /// so cross-workspace tarball imports do not carry over ACLs referring to
     /// identities that may not exist in the target workspace. `wmill sync pull`
     /// passes `true` to surface ACLs in the git-tracked yaml.
@@ -365,8 +365,8 @@ pub(crate) struct ArchiveQueryParams {
 ///                      pre-existing serialization for folders and groups so
 ///                      no customer sees a one-time noisy diff on upgrade.
 /// * `KeepIfNonEmpty` — keep when there is at least one entry, drop when `{}`
-///                      or null. New surface for flow / script / app, which
-///                      never carried ACLs in source before this change.
+///                      or null. New surface for script / flow / app / variable,
+///                      which never carried ACLs in source before this change.
 #[derive(Clone, Copy)]
 pub enum ExtraPermsBehavior {
     Drop,
@@ -665,7 +665,7 @@ pub(crate) async fn tarball_workspace(
         check_scopes(&authed, || "variables:read".to_string())?;
     }
 
-    // Opt-in behavior for surfacing per-resource ACLs on flow/app rows.
+    // Opt-in behavior for surfacing per-resource ACLs on script/flow/app/variable rows.
     // Folder and group rows have always carried `extra_perms` in source and
     // continue to do so unconditionally (`KeepEvenEmpty`) so existing
     // customer git repos see no one-time noisy diff.
@@ -1002,8 +1002,7 @@ pub(crate) async fn tarball_workspace(
                     Error::internal_err(format!("Error decrypting variable {}: {}", var.path, e))
                 })?);
             }
-            let var_str =
-                &to_string_without_metadata(&var, ExtraPermsBehavior::Drop, None).unwrap();
+            let var_str = &to_string_without_metadata(&var, new_kinds_extra_perms, None).unwrap();
             archive
                 .write_to_archive(&var_str, &format!("{}.variable.json", var.path))
                 .await?;

cli/src/commands/sync/pull.ts

@@ -104,7 +104,7 @@ export async function downloadZip(
   // from v1 the on-behalf-of address is stripped below, so the tarball sends the
   // `has_on_behalf_of` marker instead and never resolves an address.
   // `preserve_extra_perms=true` opts the tarball into surfacing granular ACLs
-  // on flow / script / app rows. Default-off on the server protects cross-
+  // on script / flow / app / variable rows. Default-off on the server protects cross-
   // workspace tarball imports from carrying ACLs that reference identities
   // missing in the target workspace; the CLI sync flow explicitly wants them.
   const baseParams = `&plain_secret=${plainSecrets ?? false

cli/src/commands/variable/variable.ts

@@ -19,6 +19,7 @@ import { sep as SEP } from "node:path";
 
 import * as wmill from "../../../gen/services.gen.ts";
 import { ListableVariable } from "../../../gen/types.gen.ts";
+import { applyExtraPermsDiff } from "../../core/extra_perms.ts";
 
 async function list(opts: GlobalOptions & { json?: boolean }) {
   if (opts.json) log.setSilent(true);
@@ -97,6 +98,7 @@ export interface VariableFile {
   description: string;
   account?: number;
   is_oauth?: boolean;
+  extra_perms?: Record<string, boolean>;
 }
 
 /**
@@ -152,37 +154,42 @@ export async function pushVariable(
     log.debug(`Variable ${remotePath} does not exist on remote`);
   }
 
+  // extra_perms is synced independently via /acls/* (see applyExtraPermsDiff)
+  // so a perm-only edit never rewrites the variable value. Strip the field from
+  // the body that goes to update_variable / create_variable and treat it as a
+  // separate step both for the up-to-date short-circuit and after the write.
+  const { extra_perms: localPerms, ...localVariableBody } = localVariable;
+
   if (variable) {
-    if (isSuperset(localVariable, variable)) {
+    if (isSuperset(localVariableBody, variable)) {
       log.debug(`Variable ${remotePath} is up-to-date`);
-      return;
-    }
+    } else {
+      log.debug(`Variable ${remotePath} is not up-to-date, updating`);
 
-    log.debug(`Variable ${remotePath} is not up-to-date, updating`);
-
-    // Apply is_secret only when it differs from the remote (the value is always
-    // sent, so the server allows the flag change). Upgrades (non-secret->secret)
-    // always apply; downgrades only when explicitly allowed (single-file push) —
-    // see allowSecretDowngrade. `undefined` leaves the flag untouched.
-    let nextIsSecret: boolean | undefined = undefined;
-    if (localVariable.is_secret !== variable.is_secret) {
-      if (localVariable.is_secret) {
-        nextIsSecret = true;
-      } else if (allowSecretDowngrade) {
-        nextIsSecret = false;
+      // Apply is_secret only when it differs from the remote (the value is always
+      // sent, so the server allows the flag change). Upgrades (non-secret->secret)
+      // always apply; downgrades only when explicitly allowed (single-file push) —
+      // see allowSecretDowngrade. `undefined` leaves the flag untouched.
+      let nextIsSecret: boolean | undefined = undefined;
+      if (localVariableBody.is_secret !== variable.is_secret) {
+        if (localVariableBody.is_secret) {
+          nextIsSecret = true;
+        } else if (allowSecretDowngrade) {
+          nextIsSecret = false;
+        }
       }
-    }
 
-    await wmill.updateVariable({
-      workspace,
-      path: remotePath.replaceAll(SEP, "/"),
-      alreadyEncrypted: !plainSecrets,
-      requestBody: {
-        ...localVariable,
-        is_secret: nextIsSecret,
-        ...(wsSpecific !== undefined ? { ws_specific: wsSpecific } : {}),
-      },
-    });
+      await wmill.updateVariable({
+        workspace,
+        path: remotePath.replaceAll(SEP, "/"),
+        alreadyEncrypted: !plainSecrets,
+        requestBody: {
+          ...localVariableBody,
+          is_secret: nextIsSecret,
+          ...(wsSpecific !== undefined ? { ws_specific: wsSpecific } : {}),
+        },
+      });
+    }
   } else {
     log.info(colors.yellow.bold(`Creating new variable ${remotePath}...`));
     await wmill.createVariable({
@@ -190,11 +197,22 @@ export async function pushVariable(
       alreadyEncrypted: !plainSecrets,
       requestBody: {
         path: remotePath.replaceAll(SEP, "/"),
-        ...localVariable,
+        ...localVariableBody,
         ...(wsSpecific !== undefined ? { ws_specific: wsSpecific } : {}),
       },
     });
   }
+
+  // Synced whether or not the body changed. No refetch: folder perms are never
+  // merged onto item.extra_perms, and the update/create body carries no
+  // extra_perms, so the value getVariable read above is still the remote one.
+  await applyExtraPermsDiff(
+    workspace,
+    "variable",
+    remotePath.replaceAll(SEP, "/"),
+    localPerms,
+    (variable as any)?.extra_perms,
+  );
 }
 
 async function push(

cli/test/variable_resource_push.test.ts

@@ -361,6 +361,126 @@ describe("variable", () => {
       expect(content).toContain("is_secret: false");
     });
   });
+
+  test("extra_perms round-trips and pushes via /acls/* without rewriting the variable", async () => {
+    await withTestBackend(async (backend, tempDir) => {
+      await setupWorkspaceProfile(backend);
+
+      const uniqueId = Date.now();
+      const varPath = `f/test/perms_var_${uniqueId}`;
+
+      const createResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/variables/create`,
+        {
+          method: "POST",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify({
+            path: varPath,
+            value: "perms_test_value",
+            is_secret: false,
+            description: "Variable for extra_perms test",
+          }),
+        }
+      );
+      expect(createResp.status).toBeLessThan(300);
+      await createResp.text();
+
+      const aclResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/acls/add/variable/${varPath}`,
+        {
+          method: "POST",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify({ owner: "g/all", write: true }),
+        }
+      );
+      expect(aclResp.status).toBeLessThan(300);
+      await aclResp.text();
+
+      await writeFile(
+        join(tempDir, "wmill.yaml"),
+        `defaultTs: bun\nincludes:\n  - "${varPath}**"\nexcludes: []\n`,
+        "utf-8"
+      );
+
+      const pullResult = await backend.runCLICommand(
+        ["sync", "pull", "--yes"],
+        tempDir
+      );
+      expect(pullResult.code).toEqual(0);
+
+      const localPath = join(tempDir, `${varPath}.variable.yaml`);
+      const pulled = await readFile(localPath, "utf-8");
+      expect(pulled).toContain("extra_perms:");
+      expect(pulled).toContain("g/all: true");
+
+      const beforeResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/variables/get/${varPath}`
+      );
+      const before = await beforeResp.json();
+
+      // Perm-only edit: downgrade the grant to read.
+      await writeFile(
+        localPath,
+        pulled.replace("g/all: true", "g/all: false"),
+        "utf-8"
+      );
+
+      const pushResult = await backend.runCLICommand(
+        ["sync", "push", "--yes"],
+        tempDir
+      );
+      expect(pushResult.code).toEqual(0);
+
+      const afterResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/variables/get/${varPath}`
+      );
+      const after = await afterResp.json();
+      expect(after.extra_perms).toEqual({ "g/all": false });
+      // Routed through /acls/* rather than update_variable, so the row itself
+      // is untouched.
+      expect(after.edited_at).toEqual(before.edited_at);
+      expect(after.value).toEqual("perms_test_value");
+
+      // A yaml with no extra_perms field at all is "no opinion": a checkout
+      // that predates ACL sync must never revoke UI-managed grants.
+      await writeFile(
+        localPath,
+        `description: "Variable for extra_perms test"\nvalue: perms_test_value\nis_secret: false\n`,
+        "utf-8"
+      );
+      const noOpinionResult = await backend.runCLICommand(
+        ["sync", "push", "--yes"],
+        tempDir
+      );
+      expect(noOpinionResult.code).toEqual(0);
+
+      const noOpinionApiResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/variables/get/${varPath}`
+      );
+      expect((await noOpinionApiResp.json()).extra_perms).toEqual({
+        "g/all": false,
+      });
+
+      // An owner present remotely but absent from a *present* map is revoked —
+      // the one direction that can destroy a grant.
+      await writeFile(
+        localPath,
+        `description: "Variable for extra_perms test"\nvalue: perms_test_value\nis_secret: false\nextra_perms: {}\n`,
+        "utf-8"
+      );
+      const revokeResult = await backend.runCLICommand(
+        ["sync", "push", "--yes"],
+        tempDir
+      );
+      expect(revokeResult.code).toEqual(0);
+
+      const finalResp = await backend.apiRequest!(
+        `/api/w/${backend.workspace}/variables/get/${varPath}`
+      );
+      const final = await finalResp.json();
+      expect(final.extra_perms).toEqual({});
+    });
+  });
 });
 
 // =============================================================================