From 47359019702c6978ca4f1e333208f191afbdf557 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 7 Sep 2026 17:36:43 +0200 Subject: [PATCH] refactor: replace the managed-credential marker with a server answer Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01C1xHmkxuxYb1GYvth1BS75 --- backend/ee-repo-ref.txt | 2 +- backend/tests/git_sync_fork_credential.rs | 104 +++++++++++++++--- backend/windmill-api/openapi.yaml | 47 +++++++- backend/windmill-common/src/workspaces.rs | 10 -- .../lib/components/GitLabIntegration.svelte | 5 - .../src/lib/components/ResourceEditor.svelte | 27 ++++- .../git_sync/GitSyncRepositoryCard.svelte | 22 +++- .../components/git_sync/managedCredential.ts | 32 ------ 8 files changed, 172 insertions(+), 77 deletions(-) delete mode 100644 frontend/src/lib/components/git_sync/managedCredential.ts diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 0cd1b824a9..5b3eef469a 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -7b607d736cc23f0ff845599fd1ac9fad4c4fd307 \ No newline at end of file +746033ec5f4d77343d39f47a839ddbd12c0f5f54 \ No newline at end of file diff --git a/backend/tests/git_sync_fork_credential.rs b/backend/tests/git_sync_fork_credential.rs index 96d1897f5a..c1add83676 100644 --- a/backend/tests/git_sync_fork_credential.rs +++ b/backend/tests/git_sync_fork_credential.rs @@ -15,6 +15,7 @@ use windmill_common::git_sync_ee::{ git_credential_for_url, repo_provider, repo_supports_managed_git_features, set_git_credential, GitProvider, }; +use windmill_common::workspaces::GitCredentialProvider; const REPO: &str = "$res:u/admin/repo"; const URL: &str = "https://gitlab.com/grp/proj.git"; @@ -25,21 +26,11 @@ async fn credential_status_is_a_workspaces_own(db: Pool) -> anyhow::Re repo_supports_managed_git_features(&db, "parent-ws", REPO).await, "the workspace holding the recorded status qualifies" ); - assert_eq!( - repo_provider(&db, "parent-ws", REPO).await, - GitProvider::GitLab, - "and its provider comes from that record" - ); assert!( !repo_supports_managed_git_features(&db, "fork-ws", REPO).await, "a fork with no record of its own does not borrow the parent's: the status \ describes one repository, and this fork's resource could name another" ); - assert_eq!( - repo_provider(&db, "fork-ws", REPO).await, - GitProvider::GitHub, - "so it answers with the default provider until its own check runs" - ); assert!( !repo_supports_managed_git_features(&db, "errored-fork-ws", REPO).await, "a workspace whose own credential failed stays disqualified" @@ -47,6 +38,54 @@ async fn credential_status_is_a_workspaces_own(db: Pool) -> anyhow::Re Ok(()) } +/// The host a repository talks to is declared when its credential is stored, and +/// travels with the credential down the fork chain. +/// +/// Read from the recorded status instead, a fork answered with the default +/// provider until its own check ran, which is long enough to register a webhook +/// against the wrong receiver. +#[sqlx::test(fixtures("git_sync_fork_credential"))] +async fn the_provider_comes_from_the_credential_and_reaches_forks( + db: Pool, +) -> anyhow::Result<()> { + assert_eq!( + repo_provider(&db, "parent-ws", REPO).await, + GitProvider::GitHub, + "with nothing stored there is no declaration to read, so the default stands" + ); + + set_git_credential( + &db, + "parent-ws", + URL, + "glpat-secret", + GitCredentialProvider::Gitlab, + ) + .await?; + + assert_eq!( + repo_provider(&db, "parent-ws", REPO).await, + GitProvider::GitLab, + "the workspace that stored it reads its own declaration" + ); + assert_eq!( + repo_provider(&db, "fork-ws", REPO).await, + GitProvider::GitLab, + "and a fork resolving that credential reads it too, without a check of its own" + ); + assert_eq!( + repo_provider(&db, "deep-fork-ws", REPO).await, + GitProvider::GitLab, + "two levels down as well" + ); + assert_eq!( + repo_provider(&db, "orphan-ws", REPO).await, + GitProvider::GitHub, + "a workspace outside the chain resolves no credential and no declaration" + ); + Ok(()) +} + /// The stored credential is shared with forks and keyed by one repository. /// /// Both properties are the point of keeping it in `workspace_settings` under the @@ -57,7 +96,14 @@ async fn credential_status_is_a_workspaces_own(db: Pool) -> anyhow::Re async fn a_fork_reads_an_ancestors_credential_for_the_bound_repository_only( db: Pool, ) -> anyhow::Result<()> { - set_git_credential(&db, "parent-ws", URL, "glpat-secret").await?; + set_git_credential( + &db, + "parent-ws", + URL, + "glpat-secret", + GitCredentialProvider::Gitlab, + ) + .await?; assert_eq!( git_credential_for_url(&db, "parent-ws", URL) @@ -104,8 +150,22 @@ async fn a_fork_reads_an_ancestors_credential_for_the_bound_repository_only( async fn each_repository_keeps_its_own_credential(db: Pool) -> anyhow::Result<()> { const OTHER_URL: &str = "https://gitlab.com/grp/other.git"; - set_git_credential(&db, "parent-ws", URL, "glpat-first").await?; - set_git_credential(&db, "parent-ws", OTHER_URL, "glpat-second").await?; + set_git_credential( + &db, + "parent-ws", + URL, + "glpat-first", + GitCredentialProvider::Gitlab, + ) + .await?; + set_git_credential( + &db, + "parent-ws", + OTHER_URL, + "glpat-second", + GitCredentialProvider::Gitlab, + ) + .await?; assert_eq!( git_credential_for_url(&db, "parent-ws", URL) @@ -121,7 +181,14 @@ async fn each_repository_keeps_its_own_credential(db: Pool) -> anyhow: Some("glpat-second") ); - set_git_credential(&db, "parent-ws", URL, "glpat-replacement").await?; + set_git_credential( + &db, + "parent-ws", + URL, + "glpat-replacement", + GitCredentialProvider::Gitlab, + ) + .await?; assert_eq!( git_credential_for_url(&db, "parent-ws", URL) .await? @@ -147,7 +214,14 @@ async fn each_repository_keeps_its_own_credential(db: Pool) -> anyhow: async fn a_credential_is_not_served_over_a_downgraded_transport( db: Pool, ) -> anyhow::Result<()> { - set_git_credential(&db, "parent-ws", URL, "glpat-secret").await?; + set_git_credential( + &db, + "parent-ws", + URL, + "glpat-secret", + GitCredentialProvider::Gitlab, + ) + .await?; assert_eq!( git_credential_for_url(&db, "parent-ws", "http://gitlab.com/grp/proj.git").await?, None diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 2ae13fcd5e..e84de8a7a0 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -2947,6 +2947,47 @@ paths: items: $ref: "#/components/schemas/GitlabProject" + /w/{workspace}/git_sync/credential/origin: + get: + tags: + - Git Sync + summary: Where a repository's credential comes from + description: >- + Whether Windmill holds this repository's access token, and which host it + talks to. `held` means this workspace stores it, `borrowed` means an + ancestor does and it is not this workspace's to replace. Both absent + means the repository authenticates with whatever its URL carries. + Returns no secret. Requires workspace admin. + operationId: getCredentialOrigin + parameters: + - $ref: "#/components/parameters/WorkspaceId" + - name: path + in: query + required: true + description: >- + Path of the git repository resource, with or without the `$res:` + prefix. A path rather than a URL, because a resource URL may carry a + token and a URL in a query string lands in logs. + schema: + type: string + responses: + "200": + description: where the credential comes from + content: + application/json: + schema: + type: object + properties: + origin: + type: string + enum: + - held + - borrowed + provider: + type: string + enum: + - gitlab + /w/{workspace}/git_sync/credential: post: tags: @@ -34354,12 +34395,6 @@ components: rotatable: type: boolean description: whether this workspace renews the credential itself - renewed: - type: boolean - description: >- - whether the credential is renewed at all, by whichever workspace - holds it; true for a fork reading an ancestor's, which nothing local - renews but nothing needs to checked_at: type: integer format: int64 diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 6a92361176..d3d34d9fc4 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -446,16 +446,6 @@ pub struct GitCredentialStatus { /// a token carried in the repository URL is the operator's to manage, and one /// resolved from an ancestor is the ancestor's, so neither is renewed here. pub rotatable: bool, - /// Whether the credential gets renewed at all, by whichever workspace holds - /// it. Equals `rotatable` for the holder and is also true for a fork that - /// borrows an ancestor's, which nothing local renews but nothing needs to. - /// - /// The UI shows expiry warnings on exactly `!renewed`, so this is what keeps - /// a fork from nagging about a token its ancestor renews on schedule. Kept - /// server-side because only the server can tell a borrowed credential from an - /// unheld one, which the resource alone cannot express. - #[serde(default)] - pub renewed: bool, /// Unix timestamp (seconds) of the last check. pub checked_at: i64, /// Why the last check or rotation failed, cleared by the next success. diff --git a/frontend/src/lib/components/GitLabIntegration.svelte b/frontend/src/lib/components/GitLabIntegration.svelte index 8781195a34..80dcb0cc7a 100644 --- a/frontend/src/lib/components/GitLabIntegration.svelte +++ b/frontend/src/lib/components/GitLabIntegration.svelte @@ -116,11 +116,6 @@ ...args, url, is_github_app: false, - // The URL carries no credential, so without this the resource is - // indistinguishable from a public remote: it is what tells the rest of - // the UI the token is Windmill's to keep and renew, and which host it - // belongs to. - managed_credential: 'gitlab', branch: args.branch || chosen.default_branch || undefined }) token = '' diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 27cc768a6c..cc6f380083 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -1,13 +1,18 @@