mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
feat(github): point install errors at the configured app and require creds for docker
GitHubAppCredentials now carries the configured app slug, so the "not installed" error from the installation lookup links to the specific app's install page (https://github.com/organizations/{owner}/settings/apps/{slug}/installations) when known, instead of the generic org installations page. Threaded through the server, workflow pipeline, and CLI runner. Also treat docker like daytona for GitHub credential gating: both are clone-based providers that need an installation token to fetch the repo, so a docker run now requires credentials when daytona would. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7d7931e56a
commit
6cb185b858
10 changed files with 149 additions and 14 deletions
|
|
@ -501,7 +501,7 @@ fn maybe_build_github_credentials(
|
|||
let resolved_run = &settings.run;
|
||||
let resolved_server = ServerSettingsBuilder::load_default().ok();
|
||||
let required_github_credentials = (resolved_run.execution.mode != RunMode::DryRun
|
||||
&& resolved_run.sandbox.provider == "daytona")
|
||||
&& clone_sandbox_requires_github_credentials(&resolved_run.sandbox.provider))
|
||||
|| resolved_server
|
||||
.as_ref()
|
||||
.is_some_and(|settings| !settings.server.integrations.github.permissions.is_empty());
|
||||
|
|
@ -515,20 +515,33 @@ fn maybe_build_github_credentials(
|
|||
.as_ref()
|
||||
.and_then(|settings| settings.server.integrations.github.app_id.as_ref())
|
||||
.map(InterpString::as_source);
|
||||
let app_slug = resolved_server
|
||||
.as_ref()
|
||||
.and_then(|settings| settings.server.integrations.github.slug.as_ref())
|
||||
.map(InterpString::as_source);
|
||||
|
||||
if required_github_credentials {
|
||||
return build_github_credentials(strategy, app_id.as_deref(), vault);
|
||||
return build_github_credentials(strategy, app_id.as_deref(), app_slug.as_deref(), vault);
|
||||
}
|
||||
|
||||
if pull_request_enabled {
|
||||
return Ok(build_github_credentials(strategy, app_id.as_deref(), vault)
|
||||
.ok()
|
||||
.flatten());
|
||||
return Ok(build_github_credentials(
|
||||
strategy,
|
||||
app_id.as_deref(),
|
||||
app_slug.as_deref(),
|
||||
vault,
|
||||
)
|
||||
.ok()
|
||||
.flatten());
|
||||
}
|
||||
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
fn clone_sandbox_requires_github_credentials(provider: &str) -> bool {
|
||||
matches!(provider, "docker" | "daytona")
|
||||
}
|
||||
|
||||
fn install_signal_handlers(
|
||||
run_control: Arc<RunControlState>,
|
||||
cancel_token: Arc<AtomicBool>,
|
||||
|
|
@ -599,6 +612,13 @@ mod tests {
|
|||
};
|
||||
use crate::args::RunWorkerMode;
|
||||
|
||||
#[test]
|
||||
fn clone_sandbox_credentials_are_required_for_clone_based_providers() {
|
||||
assert!(super::clone_sandbox_requires_github_credentials("docker"));
|
||||
assert!(super::clone_sandbox_requires_github_credentials("daytona"));
|
||||
assert!(!super::clone_sandbox_requires_github_credentials("local"));
|
||||
}
|
||||
|
||||
fn running_event(actor: Option<ActorRef>) -> fabro_types::RunEvent {
|
||||
fabro_types::RunEvent {
|
||||
id: "evt_1".to_string(),
|
||||
|
|
|
|||
|
|
@ -7,11 +7,12 @@ use fabro_vault::Vault;
|
|||
pub(crate) fn build_github_credentials(
|
||||
strategy: GithubIntegrationStrategy,
|
||||
app_id: Option<&str>,
|
||||
app_slug: Option<&str>,
|
||||
vault: Option<&Vault>,
|
||||
) -> anyhow::Result<Option<GitHubCredentials>> {
|
||||
match strategy {
|
||||
GithubIntegrationStrategy::App => {
|
||||
GitHubCredentials::from_env(app_id).map_err(|err| anyhow!(err))
|
||||
GitHubCredentials::from_env_with_slug(app_id, app_slug).map_err(|err| anyhow!(err))
|
||||
}
|
||||
GithubIntegrationStrategy::Token => {
|
||||
let token = lookup_github_token(vault);
|
||||
|
|
|
|||
|
|
@ -97,6 +97,7 @@ pub struct AppInfo {
|
|||
pub struct GitHubAppCredentials {
|
||||
pub app_id: String,
|
||||
pub private_key_pem: String,
|
||||
pub slug: Option<String>,
|
||||
}
|
||||
|
||||
impl GitHubAppCredentials {
|
||||
|
|
@ -112,6 +113,13 @@ impl GitHubAppCredentials {
|
|||
}
|
||||
|
||||
pub fn from_env(app_id: Option<&str>) -> Result<Option<Self>, String> {
|
||||
Self::from_env_with_slug(app_id, None)
|
||||
}
|
||||
|
||||
pub fn from_env_with_slug(
|
||||
app_id: Option<&str>,
|
||||
slug: Option<&str>,
|
||||
) -> Result<Option<Self>, String> {
|
||||
let Some(app_id) = app_id else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
|
@ -121,8 +129,18 @@ impl GitHubAppCredentials {
|
|||
Ok(Some(Self {
|
||||
app_id: app_id.to_string(),
|
||||
private_key_pem,
|
||||
slug: slug
|
||||
.map(str::trim)
|
||||
.filter(|slug| !slug.is_empty())
|
||||
.map(str::to_string),
|
||||
}))
|
||||
}
|
||||
|
||||
pub fn installation_url(&self, owner: &str) -> Option<String> {
|
||||
self.slug.as_ref().map(|slug| {
|
||||
format!("https://github.com/organizations/{owner}/settings/apps/{slug}/installations")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug)]
|
||||
|
|
@ -136,6 +154,13 @@ impl GitHubCredentials {
|
|||
Ok(GitHubAppCredentials::from_env(app_id)?.map(Self::App))
|
||||
}
|
||||
|
||||
pub fn from_env_with_slug(
|
||||
app_id: Option<&str>,
|
||||
slug: Option<&str>,
|
||||
) -> Result<Option<Self>, String> {
|
||||
Ok(GitHubAppCredentials::from_env_with_slug(app_id, slug)?.map(Self::App))
|
||||
}
|
||||
|
||||
async fn resolve_bearer_token(
|
||||
&self,
|
||||
client: &impl HttpClient,
|
||||
|
|
@ -147,13 +172,15 @@ impl GitHubCredentials {
|
|||
match self {
|
||||
Self::App(creds) => {
|
||||
let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?;
|
||||
create_installation_access_token_with_permissions(
|
||||
let install_url = creds.installation_url(owner);
|
||||
create_installation_access_token_with_permissions_and_install_url(
|
||||
client,
|
||||
&jwt,
|
||||
owner,
|
||||
repo,
|
||||
base_url,
|
||||
permissions,
|
||||
install_url.as_deref(),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
|
@ -357,6 +384,27 @@ pub async fn create_installation_access_token_with_permissions(
|
|||
repo: &str,
|
||||
base_url: &str,
|
||||
permissions: serde_json::Value,
|
||||
) -> Result<String, String> {
|
||||
create_installation_access_token_with_permissions_and_install_url(
|
||||
client,
|
||||
jwt,
|
||||
owner,
|
||||
repo,
|
||||
base_url,
|
||||
permissions,
|
||||
None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn create_installation_access_token_with_permissions_and_install_url(
|
||||
client: &impl HttpClient,
|
||||
jwt: &str,
|
||||
owner: &str,
|
||||
repo: &str,
|
||||
base_url: &str,
|
||||
permissions: serde_json::Value,
|
||||
install_url: Option<&str>,
|
||||
) -> Result<String, String> {
|
||||
#[derive(Deserialize)]
|
||||
struct Installation {
|
||||
|
|
@ -369,19 +417,27 @@ pub async fn create_installation_access_token_with_permissions(
|
|||
}
|
||||
|
||||
// Step 1: Find the installation for this repo
|
||||
let install_url = format!("{base_url}/repos/{owner}/{repo}/installation");
|
||||
let installation_endpoint = format!("{base_url}/repos/{owner}/{repo}/installation");
|
||||
let auth = format!("Bearer {jwt}");
|
||||
let resp = client
|
||||
.request(HttpMethod::Get, &install_url, &github_headers(&auth), None)
|
||||
.request(
|
||||
HttpMethod::Get,
|
||||
&installation_endpoint,
|
||||
&github_headers(&auth),
|
||||
None,
|
||||
)
|
||||
.await
|
||||
.map_err(|e| format!("Failed to look up GitHub App installation: {e}"))?;
|
||||
|
||||
match resp.status {
|
||||
200 => {}
|
||||
404 => {
|
||||
let install_url = install_url.map(str::to_string).unwrap_or_else(|| {
|
||||
format!("https://github.com/organizations/{owner}/settings/installations")
|
||||
});
|
||||
return Err(format!(
|
||||
"GitHub App is not installed for {owner}. \
|
||||
Install it at https://github.com/organizations/{owner}/settings/installations"
|
||||
Install it at {install_url}"
|
||||
));
|
||||
}
|
||||
403 => {
|
||||
|
|
@ -1600,6 +1656,33 @@ mod tests {
|
|||
assert!(err.contains("owner"), "got: {err}");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_iat_not_installed_uses_app_specific_install_url() {
|
||||
let mock =
|
||||
MockHttpClient::new().on(HttpMethod::Get, "/repos/owner/repo/installation", 404, "");
|
||||
let install_url =
|
||||
"https://github.com/organizations/owner/settings/apps/fabro-test/installations";
|
||||
|
||||
let err = create_installation_access_token_with_permissions_and_install_url(
|
||||
&mock,
|
||||
"jwt",
|
||||
"owner",
|
||||
"repo",
|
||||
"",
|
||||
serde_json::json!({ "contents": "write" }),
|
||||
Some(install_url),
|
||||
)
|
||||
.await
|
||||
.unwrap_err();
|
||||
|
||||
assert!(err.contains("not installed"), "got: {err}");
|
||||
assert!(err.contains(install_url), "got: {err}");
|
||||
assert!(
|
||||
!err.contains("https://github.com/organizations/owner/settings/installations"),
|
||||
"got: {err}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_iat_suspended() {
|
||||
let mock =
|
||||
|
|
@ -1706,6 +1789,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let result = branch_exists_with_client(
|
||||
&mock,
|
||||
|
|
@ -1744,6 +1828,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let result = branch_exists_with_client(
|
||||
&mock,
|
||||
|
|
@ -1782,6 +1867,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let result = branch_exists_with_client(
|
||||
&mock,
|
||||
|
|
@ -1943,6 +2029,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let detail = get_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
@ -1988,6 +2075,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let err = get_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
@ -2084,6 +2172,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
merge_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
@ -2118,6 +2207,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let err = merge_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
@ -2153,6 +2243,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let err = merge_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
@ -2197,6 +2288,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
close_pull_request_with_client(&mock, &GitHubContext::new(&creds, ""), "owner", "repo", 42)
|
||||
.await
|
||||
|
|
@ -2224,6 +2316,7 @@ mod tests {
|
|||
let creds = GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test".to_string(),
|
||||
private_key_pem: pem.to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let err = close_pull_request_with_client(
|
||||
&mock,
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ fn github_credentials() -> GitHubCredentials {
|
|||
GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "42".to_string(),
|
||||
private_key_pem: TEST_RSA_KEY.to_string(),
|
||||
slug: Some("test-app".to_string()),
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -969,13 +969,15 @@ async fn mint_github_token(
|
|||
.map_err(|err| anyhow!("{err}"))?;
|
||||
let client = fabro_http::http_client()?;
|
||||
let perms_json = serde_json::to_value(permissions)?;
|
||||
fabro_github::create_installation_access_token_with_permissions(
|
||||
let install_url = creds.installation_url(&owner);
|
||||
fabro_github::create_installation_access_token_with_permissions_and_install_url(
|
||||
&client,
|
||||
&jwt,
|
||||
&owner,
|
||||
&repo,
|
||||
&fabro_github::github_api_base_url(),
|
||||
perms_json,
|
||||
install_url.as_deref(),
|
||||
)
|
||||
.await
|
||||
.map_err(|err| anyhow!("{err}"))
|
||||
|
|
|
|||
|
|
@ -801,6 +801,7 @@ impl AppState {
|
|||
fabro_github::GitHubAppCredentials {
|
||||
app_id,
|
||||
private_key_pem,
|
||||
slug: settings.slug.as_ref().map(InterpString::as_source),
|
||||
},
|
||||
)))
|
||||
}
|
||||
|
|
@ -1743,6 +1744,10 @@ fn system_sandbox_provider(
|
|||
)
|
||||
}
|
||||
|
||||
fn clone_sandbox_requires_github_credentials(provider: &str) -> bool {
|
||||
matches!(provider, "docker" | "daytona")
|
||||
}
|
||||
|
||||
fn parse_system_duration(raw: &str) -> anyhow::Result<chrono::Duration> {
|
||||
let raw = raw.trim();
|
||||
anyhow::ensure!(!raw.is_empty(), "empty duration string");
|
||||
|
|
@ -2033,13 +2038,14 @@ async fn get_github_repo(
|
|||
.into_response();
|
||||
}
|
||||
|
||||
match fabro_github::create_installation_access_token_with_permissions(
|
||||
match fabro_github::create_installation_access_token_with_permissions_and_install_url(
|
||||
client_ref,
|
||||
&jwt,
|
||||
&owner,
|
||||
&name,
|
||||
&base_url,
|
||||
serde_json::json!({ "contents": "write", "pull_requests": "write" }),
|
||||
Some(&install_url),
|
||||
)
|
||||
.await
|
||||
{
|
||||
|
|
@ -4632,7 +4638,7 @@ async fn execute_run_in_process(state: Arc<AppState>, run_id: RunId) {
|
|||
let github_app_result = {
|
||||
let settings = &persisted.run_spec().settings.run;
|
||||
let required_github_credentials = (settings.execution.mode != RunMode::DryRun
|
||||
&& settings.sandbox.provider == "daytona")
|
||||
&& clone_sandbox_requires_github_credentials(&settings.sandbox.provider))
|
||||
|| !github_settings.permissions.is_empty();
|
||||
if required_github_credentials {
|
||||
state.github_credentials(github_settings)
|
||||
|
|
@ -8667,6 +8673,13 @@ provider = "invalid-provider"
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clone_sandbox_credentials_are_required_for_clone_based_providers() {
|
||||
assert!(clone_sandbox_requires_github_credentials("docker"));
|
||||
assert!(clone_sandbox_requires_github_credentials("daytona"));
|
||||
assert!(!clone_sandbox_requires_github_credentials("local"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_secret_stores_file_secret_and_excludes_it_from_snapshot() {
|
||||
let state = create_app_state();
|
||||
|
|
|
|||
|
|
@ -634,6 +634,7 @@ mod tests {
|
|||
GitHubCredentials::App(GitHubAppCredentials {
|
||||
app_id: "test-app".to_string(),
|
||||
private_key_pem: pem,
|
||||
slug: None,
|
||||
}),
|
||||
test_http_client(),
|
||||
"owner".to_string(),
|
||||
|
|
|
|||
|
|
@ -229,13 +229,15 @@ async fn mint_github_token(
|
|||
.map_err(|e| Error::engine(e.clone()))?;
|
||||
let client = fabro_http::http_client().map_err(|e| Error::engine(e.to_string()))?;
|
||||
let perms_json = serde_json::to_value(permissions).map_err(|e| Error::engine(e.to_string()))?;
|
||||
fabro_github::create_installation_access_token_with_permissions(
|
||||
let install_url = creds.installation_url(&owner);
|
||||
fabro_github::create_installation_access_token_with_permissions_and_install_url(
|
||||
&client,
|
||||
&jwt,
|
||||
&owner,
|
||||
&repo,
|
||||
&fabro_github::github_api_base_url(),
|
||||
perms_json,
|
||||
install_url.as_deref(),
|
||||
)
|
||||
.await
|
||||
.map_err(|e| Error::engine(e.clone()))
|
||||
|
|
|
|||
|
|
@ -1510,6 +1510,7 @@ mod tests {
|
|||
let creds = fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials {
|
||||
app_id: "123".to_string(),
|
||||
private_key_pem: "unused".to_string(),
|
||||
slug: None,
|
||||
});
|
||||
let base_url = github_app::github_api_base_url();
|
||||
let result = maybe_open_pull_request(OpenPullRequestRequest {
|
||||
|
|
|
|||
|
|
@ -207,6 +207,7 @@ fn load_github_app_credentials() -> fabro_github::GitHubCredentials {
|
|||
fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials {
|
||||
app_id,
|
||||
private_key_pem,
|
||||
slug: None,
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue