diff --git a/crates/fractal-github-base/src/github_app_read.rs b/crates/fractal-github-base/src/github_app_read.rs index 218af77236e..e3b104125a9 100644 --- a/crates/fractal-github-base/src/github_app_read.rs +++ b/crates/fractal-github-base/src/github_app_read.rs @@ -18,6 +18,15 @@ pub struct ReadSource { pub principal: String, pub token: Arc, pub used_fallback: Arc, + /// The installation token's permissions, as its sidecar records them. + pub permissions: Arc>, +} + +impl ReadSource { + /// Whether the installation token may read `permission`. + pub fn grants(&self, permission: &str) -> bool { + self.permissions.get(permission).is_some_and(|level| level == "read" || level == "write") + } } #[derive(Deserialize)] @@ -130,17 +139,29 @@ pub fn load(root: &Path, owner: &str, app: &str) -> std::result::Result, owner: &str, app: &str, pat: &GithubAuth) -> GithubAuth { let Some(root) = root else { fallback(owner, app, "missing"); return pat.clone(); }; - match load(root, owner, app) { + match load(root, owner, app).or_else(|reason| substitute(root, owner, app, reason)) { Ok(source) => GithubAuth::Read { source, pat: Box::new(pat.clone()) }, Err(reason) => { fallback(owner, app, reason); pat.clone() } } } +/// Issue reads use the gh-ci installation when no github-ledger installation is published +/// for the owner and gh-ci's token grants Issues read. Otherwise the original reason stands. +fn substitute(root: &Path, owner: &str, app: &str, reason: &'static str) -> std::result::Result { + if app != "github-ledger" || !matches!(reason, "missing" | "no-installation") { return Err(reason); } + load(root, owner, "gh-ci").ok().filter(|source| source.grants("issues")).ok_or(reason) +} + +/// The gh-ci installation for commit status reads, when its token grants Commit statuses read. +pub fn statuses_source(root: &Path, owner: &str) -> Option { + load(root, owner, "gh-ci").ok().filter(|source| source.grants("statuses")) +} + pub fn route(url: &reqwest::Url) -> Option<(&str, &'static str, bool)> { let mut segments = url.path().split_once("/repos/")?.1.split('/'); let owner = segments.next()?; @@ -263,7 +284,7 @@ pub(crate) mod tests { }).await } - async fn server_with_headers(handler: impl Fn(&reqwest::header::HeaderMap, &str, &str, &Value) -> (StatusCode, reqwest::header::HeaderMap, Value) + Send + Sync + 'static) -> Server { + pub(crate) async fn server_with_headers(handler: impl Fn(&reqwest::header::HeaderMap, &str, &str, &Value) -> (StatusCode, reqwest::header::HeaderMap, Value) + Send + Sync + 'static) -> Server { async fn handle(State(handler): State>, request: axum::extract::Request) -> axum::response::Response { let authorization = request.headers().get("authorization").unwrap().to_str().unwrap().to_owned(); let path = request.uri().to_string(); @@ -331,6 +352,52 @@ pub(crate) mod tests { assert_eq!(warnings.keys, 3); } + fn grant(root: &Path, owner: &str, app: &str, permissions: Value) { + let path = root.join(app).join(format!("{owner}.json")); + let mut sidecar: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + sidecar["permissions"] = permissions; + std::fs::write(path, serde_json::to_vec(&sidecar).unwrap()).unwrap(); + } + + #[test] + fn issue_reads_use_gh_ci_only_when_it_grants_issues_and_no_ledger_is_published() { + let root = tempfile::tempdir().unwrap(); + let owner = "issue-substitute"; + let pat = GithubAuth::test("fixture-pat"); + let expires = chrono::Utc::now() + chrono::Duration::minutes(60); + install(root.path(), owner, "gh-ci", "fixture-ci", expires); + assert_eq!(select(Some(root.path()), owner, "github-ledger", &pat).cache_principal(), "pat"); + assert_eq!(count(owner, "github-ledger", "no-installation"), 1); + grant(root.path(), owner, "gh-ci", json!({"metadata": "read", "issues": "read"})); + let substituted = select(Some(root.path()), owner, "github-ledger", &pat); + assert!(matches!(&substituted, GithubAuth::Read { source, .. } if source.app == "gh-ci")); + assert_eq!(substituted.local_token().as_str(), "fixture-ci"); + assert_eq!(count(owner, "github-ledger", "no-installation"), 1, "a substitute is not a fallback"); + install(root.path(), owner, "github-ledger", "fixture-ledger", expires); + assert_eq!(select(Some(root.path()), owner, "github-ledger", &pat).local_token().as_str(), "fixture-ledger"); + // A published but expired ledger installation is a fault, not a reason to substitute. + install(root.path(), owner, "github-ledger", "fixture-ledger", chrono::Utc::now() - chrono::Duration::seconds(1)); + assert_eq!(select(Some(root.path()), owner, "github-ledger", &pat).cache_principal(), "pat"); + assert_eq!(count(owner, "github-ledger", "expired"), 1); + } + + #[test] + fn status_reads_use_gh_ci_once_it_grants_statuses() { + let root = tempfile::tempdir().unwrap(); + let owner = "status-route"; + let pat = GithubAuth::test("fixture-pat"); + install(root.path(), owner, "gh-ci", "fixture-ci", chrono::Utc::now() + chrono::Duration::minutes(60)); + let auth = select(Some(root.path()), owner, "gh-ci", &pat); + let url = format!("https://api.github.com/repos/{owner}/repo/commits/abc/status"); + assert_eq!(crate::github_http::read_auth_for_url(&url, &auth).cache_principal(), "pat"); + assert_eq!(count(owner, "gh-ci", "statuses"), 1); + grant(root.path(), owner, "gh-ci", json!({"metadata": "read", "statuses": "read"})); + let selected = crate::github_http::read_auth_for_url(&url, &auth); + assert!(matches!(&selected, GithubAuth::Read { source, .. } if source.grants("statuses"))); + assert_eq!(selected.local_token().as_str(), "fixture-ci"); + assert_eq!(count(owner, "gh-ci", "statuses"), 1); + } + #[test] fn supplied_root_rotation_expiry_permissions_and_reason_counters() { let root = tempfile::tempdir().unwrap(); diff --git a/crates/fractal-github-base/src/github_http.rs b/crates/fractal-github-base/src/github_http.rs index 9bcf2c53efe..56fa9a33756 100644 --- a/crates/fractal-github-base/src/github_http.rs +++ b/crates/fractal-github-base/src/github_http.rs @@ -427,13 +427,22 @@ fn request_resource(path: &str) -> &'static str { } } -/// Select from the supplied root only. Token files are reopened for each read batch. -pub fn read_auth(owner: &str, app: &str, pat: &GithubAuth) -> GithubAuth { - let root = match pat { +fn token_root(pat: &GithubAuth) -> Option<&std::path::Path> { + match pat { GithubAuth::Read { source, .. } => Some(source.root.as_path()), _ => SOURCE.get().and_then(|config| config.github.app_token_root.as_deref()), - }; - app_read::select(root, owner, app, pat.pat()) + } +} + +/// Select from the supplied root only. Token files are reopened for each read batch. +pub fn read_auth(owner: &str, app: &str, pat: &GithubAuth) -> GithubAuth { + app_read::select(token_root(pat), owner, app, pat.pat()) +} + +/// The installation that may read `owner`'s commit statuses, if its token grants them. +pub fn statuses_auth(owner: &str, pat: &GithubAuth) -> Option { + let source = app_read::statuses_source(token_root(pat)?, owner)?; + Some(GithubAuth::Read { source, pat: Box::new(pat.pat().clone()) }) } impl GithubAuth { @@ -481,6 +490,9 @@ pub fn read_auth_for_url(url: &str, auth: &GithubAuth) -> GithubAuth { return auth.pat().clone(); }; if statuses { + if let Some(selected) = statuses_auth(owner, auth) { + return selected; + } if matches!(auth, GithubAuth::Read { .. }) || SOURCE.get().is_some_and(|config| config.github.app_token_root.is_some()) { app_read::fallback(owner, app, "statuses"); } diff --git a/crates/fractal-github-base/src/resource/github_repository.rs b/crates/fractal-github-base/src/resource/github_repository.rs index df6a7839be1..77f398f1040 100644 --- a/crates/fractal-github-base/src/resource/github_repository.rs +++ b/crates/fractal-github-base/src/resource/github_repository.rs @@ -1829,7 +1829,7 @@ async fn observe_poll( }) } -/// Neither installation has statuses:read. An app-only rollup is not complete. +/// An installation without statuses:read sees an app-only rollup, which is not complete. fn merge_legacy_statuses(node: &mut Value, combined: &Value) -> Result<()> { let statuses = combined.get("statuses").and_then(Value::as_array) .context("the GitHub combined status response has no statuses")?; @@ -2036,7 +2036,8 @@ async fn open_pull_requests( .min(PULL_REQUEST_PAGE_SIZE) .min(failed_size_ceiling - 1); } - if matches!(read_token, GithubAuth::Read { .. }) { + // With statuses:read the GraphQL rollup already carries StatusContext entries. + if matches!(&read_token, GithubAuth::Read { source, .. } if !source.grants("statuses")) { let mut statuses = BTreeMap::new(); for node in &mut nodes { let Some(head) = node.get("headRefOid").and_then(Value::as_str).map(str::to_owned) else { continue; }; @@ -2379,6 +2380,45 @@ mod tests { assert_eq!(requests.iter().filter(|(_, path, _)| path.contains("/status")).count(), 1); } + #[tokio::test] + async fn pat_legacy_status_reads_revalidate_with_their_etag() { + use crate::github_http::app_read::{select, tests::{install, server_with_headers}}; + use reqwest::StatusCode; + let root = tempfile::tempdir().unwrap(); + let owner = "status-etag-fixture"; + install(root.path(), owner, "gh-ci", "fixture-ci", chrono::Utc::now() + chrono::Duration::minutes(60)); + let auth = select(Some(root.path()), owner, "gh-ci", &GithubAuth::test("fixture-pat")); + let mut pull = open_pull(1, "status-head", "SUCCESS"); + pull.pointer_mut("/commits/nodes/0/commit/statusCheckRollup/contexts/nodes") + .unwrap().as_array_mut().unwrap().retain(|context| context["__typename"] == "CheckRun"); + let revalidations = Arc::new(parking_lot::Mutex::new(Vec::new())); + let recorded = revalidations.clone(); + let fixture = server_with_headers(move |headers, authorization, path, _| { + if !path.contains("/commits/") { + return (StatusCode::OK, Default::default(), json!({"data":{"repository":{"pullRequests":{ + "pageInfo":{"hasNextPage":false}, "nodes":[pull.clone()]}}}})); + } + assert_eq!(authorization, "Bearer fixture-pat"); + let validator = headers.get("if-none-match").map(|value| value.to_str().unwrap().to_owned()); + recorded.lock().push(validator.clone()); + let mut response = reqwest::header::HeaderMap::new(); + response.insert("etag", "\"status-etag\"".parse().unwrap()); + if validator.as_deref() == Some("\"status-etag\"") { + return (StatusCode::NOT_MODIFIED, response, Value::Null); + } + (StatusCode::OK, response, json!({"state":"success","total_count":1, + "statuses":[{"context":"legacy/build","state":"success"}]})) + }).await; + for _ in 0..2 { + let details = open_pull_requests(&reqwest::Client::new(), &fixture.base, &auth, + Duration::ZERO, "status-etag-fixture/repo", (owner, "repo"), true, true, + &BTreeSet::new(), true, &[]).await.unwrap(); + assert!(details.open[0]["checks"].as_array().unwrap().iter().any(|check| check["name"] == "legacy/build")); + } + assert_eq!(*revalidations.lock(), [None, Some("\"status-etag\"".to_owned())], + "the second status read must be conditional, so an unchanged head costs no PAT quota"); + } + #[test] fn legacy_status_merge_preserves_checks_and_refuses_truncation() { let mut pull = open_pull(1, "head", "SUCCESS"); diff --git a/docs/st3/github-observer-admission.md b/docs/st3/github-observer-admission.md index ace072685fc..926dc585c33 100644 --- a/docs/st3/github-observer-admission.md +++ b/docs/st3/github-observer-admission.md @@ -259,7 +259,11 @@ For a repository's owner, read `ROOT/gh-ci/OWNER.token` and `OWNER.json` for metadata, branches/contents, PRs/reviews, checks/actions and open-PR GraphQL. Use `ROOT/github-ledger/OWNER.token` and `OWNER.json` for issue lists, issue comments/excerpts, issue watch preflight and Issue identity GraphQL. PR watch -preflight and review excerpts use gh-ci. When either installation is selected, +preflight and review excerpts use gh-ci. When no github-ledger installation is +published for the owner (reason `missing` or `no-installation`) and the gh-ci +sidecar's `permissions` grant `issues` read, issue reads use the gh-ci token +instead and record no fallback; an expired or unreadable ledger installation +still falls back to the PAT. When either installation is selected, remembered opaque node IDs are partitioned by their recorded `ItemIdentity.pull`, never guessed from their bytes, and each partition gets its own minimal identity query and authorization. PAT-only batches retain the prior single mixed Issue/PR @@ -290,16 +294,17 @@ These count source-selection fallback events, not GitHub charges; conditional cache hits can still encounter a missing source. Remove transition fallback only after one week with zero unintentional fallback across the enrolled owners. -**Legacy status qualification:** neither app has statuses:read. An app GraphQL -`statusCheckRollup` returns check runs without legacy `StatusContext` data, so -repository PR detail reads retain a PAT-only combined commit-status GET, once -per distinct head SHA in a read, through the existing ETag cache. Its source +**Legacy status qualification:** an installation without statuses:read sees a +GraphQL `statusCheckRollup` of check runs without legacy `StatusContext` data, +so repository PR detail reads then add a PAT-only combined commit-status GET, +once per distinct head SHA in a read, through the existing ETag cache. Its source exception is observable as fallback reason `statuses`. Legacy status contexts are merged with app check runs before deriving check facts and required-check outcomes. Truncated status responses fail visibly rather than asserting missing -contexts passed. This intentional status exception must be excluded from the -one-week unintentional-fallback gate until installation permissions are separately -approved and qualified. No statuses permission is assumed or added here. +contexts passed. When the gh-ci sidecar grants `statuses` read, the rollup already +carries `StatusContext` entries, so the extra GET is skipped, and any commit-status +read routes to gh-ci without a fallback. Until then this intentional status +exception is excluded from the one-week unintentional-fallback gate. REST cache keys include the stable installation principal, permissions and repository-selection scope, not token bytes; rotation preserves cache identity. diff --git a/release-notes/gh-ci-issue-status-reads.json b/release-notes/gh-ci-issue-status-reads.json new file mode 100644 index 00000000000..7bfe04af234 --- /dev/null +++ b/release-notes/gh-ci-issue-status-reads.json @@ -0,0 +1,11 @@ +{ + "version": 1, + "summary": "GitHub issue and commit-status reads use the gh-ci installation once its token grants them", + "replay": {"status": "none", "detail": "No replay or projection changes."}, + "schema": {"status": "none", "detail": "No persistent schema changes."}, + "rules": {"status": "none", "detail": "No runtime or policy rule changes."}, + "compatibility": {"status": "none", "detail": "Behaviour is unchanged until an operator-published gh-ci sidecar grants issues or statuses read; forbidden installation reads still replay with the PAT."}, + "downtime": {"status": "none", "detail": "No runtime deployment or migration changes."}, + "manual": {"status": "none", "detail": "Granting Issues and Commit statuses read to the gh-ci App moves those reads off the PAT; no action is required otherwise."}, + "recovery": {"status": "none", "detail": "Revoking the permissions returns those reads to the PAT on the next token refresh."} +}