From 4e4c5c2c8ff15223786c829a3873662aa225d574 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 7 Sep 2026 16:52:00 +0200 Subject: [PATCH] fix: classify the renewal state once so the card cannot contradict itself Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01C1xHmkxuxYb1GYvth1BS75 --- backend/windmill-common/src/git_sync_oss.rs | 8 +- .../git_sync/GitSyncRepositoryCard.svelte | 74 +++++++++++++------ .../git_sync/ReplaceGitCredential.svelte | 16 ++-- 3 files changed, 66 insertions(+), 32 deletions(-) diff --git a/backend/windmill-common/src/git_sync_oss.rs b/backend/windmill-common/src/git_sync_oss.rs index 823b6f60d7..4c7de4fa7b 100644 --- a/backend/windmill-common/src/git_sync_oss.rs +++ b/backend/windmill-common/src/git_sync_oss.rs @@ -5,9 +5,11 @@ pub use crate::git_sync_ee::*; use sqlx::{Pool, Postgres}; use url::Url; -/// Gated on the pair for the same reason as [`with_stored_credential`] below: -/// `private` alone would leave this undefined, which only stays harmless while -/// every caller is itself `enterprise`-gated. +/// Gated on the pair to match [`with_stored_credential`] below, whose callers +/// reach it through this facade un-gated and so depend on it. Nothing routes +/// here today (the one caller imports the enterprise item directly), so this is +/// for uniformity: the next plain caller would otherwise find no definition +/// under `private` without `enterprise`. #[cfg(not(all(feature = "private", feature = "enterprise")))] pub async fn get_github_app_token_internal( _db: &Pool, diff --git a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte index bf5642d9c0..4cf1af1c7d 100644 --- a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte +++ b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte @@ -155,13 +155,34 @@ /** The host named by the resource's `managed_credential`, when Windmill holds * the repository's token rather than it being written into the URL. */ let managedCredential = $state(undefined) - /** Whether the token may rotate itself. Renewal also needs Windmill to hold it, - * which `managedCredential` stands in for; the card states both, and the alert - * and the quiet status line below must not disagree about either, so each is - * derived once here rather than recomputed per branch. */ let canSelfRotate = $derived( (repo?.credential?.scopes ?? []).some((s) => s === 'api' || s === 'self_rotate') ) + /** + * Why this repository's token is or is not being renewed. + * + * Renewal needs a scope that permits self-rotation and a credential the + * workspace holds, and the alert and the quiet status line below both describe + * that state at different day counts. Reading it independently in each has + * twice produced contradictory advice, so it is classified once here and both + * render the answer. + * + * `inherited` is reachable only on a fork: forking copies the resource's + * marker but not the credential, so the fork borrows its ancestor's and reports + * `rotatable: false` while the ancestor renews it on schedule. Any other + * workspace whose stored credential stopped matching its URL loses its recorded + * status entirely and never reaches here. + */ + let renewal = $derived.by(() => { + const credential = repo?.credential + if (!credential) return undefined + if (credential.rotatable) return $enterpriseLicense ? 'renewed' : 'needs-license' + // The marker comes from the resource, so until that load lands there is no + // answer yet and guessing one would state the opposite of the truth. + if (loadingResourceInfo) return 'unknown' + if (canSelfRotate) return managedCredential ? 'inherited' : 'not-held' + return managedCredential ? 'no-scope' : 'no-scope-and-not-held' + }) // Whether Windmill itself holds a credential for the repository, which is // what the managed features (webhooks, pull requests, commit checks) need. // A GitHub App installation qualifies, and so does a token the server keeps. @@ -226,23 +247,25 @@ } } if (days > 30) return undefined - // Renewal needs two things, and the advice differs by which is missing: a - // scope that permits self-rotation, and a credential this workspace holds. - // Scopes answer the first; `managedCredential` is the client's signal for the - // second, and naming the URL as the cause would be wrong for a fork, which - // borrows an ancestor's token and holds none of its own. - const remedy = managedCredential - ? // The remedy lives with the credential, which the resource owns; saying - // where stops the warning being a dead end. - ` Replace it on the ${repo?.git_repo_resource_path?.replace(/^\$res:/, '') ?? 'repository'} resource.` - : ' Connect the repository with the GitLab button to hand the token to Windmill, or replace it before it expires.' + // Nothing for this workspace to act on: the holder renews it, and telling a + // fork admin to replace it would split the credential in two. + if (renewal === 'inherited' || renewal === 'unknown') return undefined + const remedy = + renewal === 'no-scope' + ? // The remedy lives with the credential, which the resource owns; saying + // where stops the warning being a dead end. + ` Replace it on the ${repo?.git_repo_resource_path?.replace(/^\$res:/, '') ?? 'repository'} resource.` + : ' Connect the repository with the GitLab button to hand the token to Windmill, or replace it before it expires.' + const cause = + renewal === 'not-held' + ? 'Windmill is not the holder of this token, so it does not renew it.' + : renewal === 'no-scope' + ? 'Give the token the api or self_rotate scope so Windmill can renew it.' + : 'Windmill is not the holder of this token, and could not renew it anyway without the api or self_rotate scope.' return { type: days <= 7 ? ('error' as const) : days <= 14 ? ('warning' as const) : ('info' as const), title: `Repository token ${when}`, - body: - (canSelfRotate - ? 'Windmill is not the holder of this token, so it does not renew it.' - : 'Give the token the api or self_rotate scope so Windmill can renew it.') + remedy + body: cause + remedy } }) @@ -642,21 +665,26 @@
{#if credentialDaysLeft === undefined} Repository token does not expire. - {:else if repo.credential.rotatable && $enterpriseLicense} + {:else if renewal === 'renewed'} Repository token expires on {repo.credential.expires_at}, and Windmill renews it automatically. - {:else if repo.credential.rotatable} + {:else if renewal === 'needs-license'} Repository token expires on {repo.credential.expires_at}. Renewing it automatically requires an enterprise license. - {:else if canSelfRotate} + {:else if renewal === 'inherited'} + Repository token expires on {repo.credential.expires_at}, and the workspace that holds it + renews it. + {:else if renewal === 'not-held'} Repository token expires on {repo.credential.expires_at}. Windmill renews only a token it holds, and it does not hold this one. - {:else if managedCredential} + {:else if renewal === 'no-scope'} Repository token expires on {repo.credential.expires_at}. Give it the api or self_rotate scope so Windmill can renew it. - {:else} + {:else if renewal === 'no-scope-and-not-held'} Repository token expires on {repo.credential.expires_at}. Windmill renews only a token it holds, and this one would also need the api or self_rotate scope. + {:else} + Repository token expires on {repo.credential.expires_at}. {/if}
{/if} diff --git a/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte b/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte index 878214e0de..a9528fd8c1 100644 --- a/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte +++ b/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte @@ -40,18 +40,22 @@ if (!token || saving) return saving = true error = undefined - // Pinned before the first await: the field stays editable while the check - // runs, so re-reading it afterwards would store a token the check never saw. + // Pinned before the first await, all three: the field stays editable and the + // props follow the drawer's selected workspace and deployed URL, so + // re-reading any of them afterwards would store the token against something + // the check never validated. const candidate = token + const forRepo = repoUrl + const inWorkspace = workspace try { // Check the token before storing it. The server binds a credential to its // repository but only refuses it when something tries to use it, so a // wrong token would otherwise be accepted here and surface as a failed // sync later. - const parts = repoParts(repoUrl) + const parts = repoParts(forRepo) if (parts) { const projects = await GitSyncService.listGitlabProjects({ - workspace, + workspace: inWorkspace, // Searched by name rather than listed whole: the listing is one // capped page, so a token that reaches more projects than fit // would not show this one and a working token would be refused. @@ -67,8 +71,8 @@ } } await GitSyncService.setGitCredential({ - workspace, - requestBody: { repo_url: repoUrl, token: candidate } + workspace: inWorkspace, + requestBody: { repo_url: forRepo, token: candidate } }) token = '' sendUserToast('Token replaced')