Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 70 additions & 3 deletions crates/fractal-github-base/src/github_app_read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ pub struct ReadSource {
pub principal: String,
pub token: Arc<String>,
pub used_fallback: Arc<AtomicBool>,
/// The installation token's permissions, as its sidecar records them.
pub permissions: Arc<BTreeMap<String, String>>,
}

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)]
Expand Down Expand Up @@ -130,17 +139,29 @@ pub fn load(root: &Path, owner: &str, app: &str) -> std::result::Result<ReadSour
if token.is_empty() || token.bytes().any(|byte| byte.is_ascii_whitespace() || byte.is_ascii_control()) { return Err("unreadable"); }
let principal = format!("app:{app}:{owner}:{}:{}:{}", metadata.installation_id, metadata.repository_selection,
serde_json::to_string(&metadata.permissions).map_err(|_| "unreadable")?);
Ok(ReadSource { root: root.into(), owner: owner.into(), app: app.into(), principal, token: Arc::new(token), used_fallback: Arc::new(AtomicBool::new(false)) })
Ok(ReadSource { root: root.into(), owner: owner.into(), app: app.into(), principal, token: Arc::new(token), used_fallback: Arc::new(AtomicBool::new(false)), permissions: Arc::new(metadata.permissions) })
}

pub fn select(root: Option<&Path>, 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<ReadSource, &'static str> {
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<ReadSource> {
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()?;
Expand Down Expand Up @@ -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<Arc<Handler>>, 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();
Expand Down Expand Up @@ -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();
Expand Down
22 changes: 17 additions & 5 deletions crates/fractal-github-base/src/github_http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<GithubAuth> {
let source = app_read::statuses_source(token_root(pat)?, owner)?;
Some(GithubAuth::Read { source, pat: Box::new(pat.pat().clone()) })
}

impl GithubAuth {
Expand Down Expand Up @@ -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");
}
Expand Down
44 changes: 42 additions & 2 deletions crates/fractal-github-base/src/resource/github_repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")?;
Expand Down Expand Up @@ -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; };
Expand Down Expand Up @@ -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");
Expand Down
21 changes: 13 additions & 8 deletions docs/st3/github-observer-admission.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
Loading