diff --git a/crates/didbot-serve/src/oauth/authorize.rs b/crates/didbot-serve/src/oauth/authorize.rs index fb354063..d0fa84f1 100644 --- a/crates/didbot-serve/src/oauth/authorize.rs +++ b/crates/didbot-serve/src/oauth/authorize.rs @@ -167,6 +167,11 @@ impl ScopePolicy for ConfiguredCeiling { /// What `authorize` hands back once a pending request has been redeemed: /// the decision [`crate::oauth::par`] minted for it. +/// +/// Only a decision that grants something reaches this type — see +/// [`finish_authorization`] — so `granted` and `cut` are read off the +/// verdict there, where the refusal case is already answered, and nothing +/// rendering one carries a branch for a record that grants nothing. #[derive(Debug, Clone)] pub struct AuthorizePending { /// The one-time reference an approval must carry. The same value the @@ -176,6 +181,13 @@ pub struct AuthorizePending { /// The decision itself — client origin, requested scope, verdict, and /// what the verdict cut. pub record: DecisionRecord, + /// What approving this covers. + pub granted: ScopeSet, + /// What the ceiling took out of the request. Empty when it took + /// nothing. + pub cut: ScopeSet, + /// What did the narrowing, when `cut` is not empty. + pub rule: String, } /// Why `authorize` refused before ever showing a decision. @@ -223,16 +235,26 @@ pub fn finish_authorization( decisions: &dyn DecisionStore, request_uri: &str, ) -> Result { + use crate::oauth::decision::Verdict; let record = decisions .get(request_uri) .ok_or(AuthorizeError::UnknownRequestUri)?; - let reference = record.token.clone().ok_or_else(|| match &record.verdict { - crate::oauth::decision::Verdict::Deny { reason, .. } => { - AuthorizeError::Refused(reason.clone()) - } - _ => AuthorizeError::UnknownRequestUri, - })?; - Ok(AuthorizePending { reference, record }) + let (granted, cut, rule) = match &record.verdict { + Verdict::Allow => (record.requested.clone(), ScopeSet::default(), String::new()), + Verdict::Narrow { granted, cut, rule } => (granted.clone(), cut.clone(), rule.clone()), + Verdict::Deny { reason, .. } => return Err(AuthorizeError::Refused(reason.clone())), + }; + let reference = record + .token + .clone() + .ok_or(AuthorizeError::UnknownRequestUri)?; + Ok(AuthorizePending { + reference, + record, + granted, + cut, + rule, + }) } #[cfg(test)] @@ -313,6 +335,8 @@ mod tests { let pending = finish_authorization(&decisions, &uri).expect("the decision is there"); assert_eq!(pending.reference.0, "reference"); assert_eq!(pending.record.account, "did:web:agent.example"); + assert_eq!(pending.granted.to_string(), "atproto"); + assert!(pending.cut.0.is_empty()); } /// A denied decision has no reference, so there is nothing for a page to diff --git a/crates/didbot-serve/src/routes.rs b/crates/didbot-serve/src/routes.rs index e0f7fad0..8e7d03ea 100644 --- a/crates/didbot-serve/src/routes.rs +++ b/crates/didbot-serve/src/routes.rs @@ -1485,11 +1485,17 @@ fn authorize_error_response(err: crate::oauth::authorize::AuthorizeError) -> Res /// There is no form. Approving is `bot.did.approveAuthorization`, which /// takes the account's own agent token as `Credential::AgentSelf` — a /// credential an HTML form cannot carry, so a form here could only post to -/// something that would refuse it. What the page offers instead is -/// `data-approval-token`: a browser-driven client reads it and makes the -/// XRPC call itself, with the credential it already holds. +/// something that would refuse it. A browser-driven client approving on the +/// agent's behalf reads the one-time reference from +/// `bot.did.getAuthorization`, which takes the same credential the approval +/// itself takes: this route takes no credential at all, so anything that can +/// follow the `request_uri` can read what this page says, and the reference +/// is not on it. +/// +/// Every element carries its value in an attribute for such a client and in +/// text for a person, which is why the account is rendered both ways rather +/// than sitting in markup a reader cannot see. fn authorize_page(pending: &crate::oauth::authorize::AuthorizePending) -> String { - use crate::oauth::decision::Verdict; let record = &pending.record; let escape = |value: &str| { value @@ -1498,13 +1504,6 @@ fn authorize_page(pending: &crate::oauth::authorize::AuthorizePending) -> String .replace('>', ">") .replace('"', """) }; - let (granted, cut, rule) = match &record.verdict { - Verdict::Allow => (record.requested.to_string(), String::new(), String::new()), - Verdict::Narrow { granted, cut, rule } => { - (granted.to_string(), cut.to_string(), rule.clone()) - } - Verdict::Deny { reason, rule } => (String::new(), reason.clone(), rule.clone()), - }; let items = |scopes: &crate::oauth::scope::ScopeSet| -> String { let items: String = scopes .0 @@ -1520,34 +1519,27 @@ fn authorize_page(pending: &crate::oauth::authorize::AuthorizePending) -> String // thing that decides this list; a token policy can still refuse the // exchange, and a write policy can still refuse a write a listed scope // covers, so a list labelled "policies" would promise their answers too. - let allowed = match &record.verdict { - Verdict::Allow => format!( - "The scope ceiling allows these scopes:{}", - items(&record.requested) - ), - Verdict::Narrow { granted, .. } => format!( - "The scope ceiling allows only these scopes:{}", - items(granted) - ), - Verdict::Deny { .. } => "No scopes are granted.".to_owned(), - }; - let approves = if record.verdict.is_deny() { + let only = if pending.cut.0.is_empty() { "" } else { - " data-approves=\"granted\" data-ceiling-checked=\"each-use\"" + "only " }; + let allowed = format!( + "The scope ceiling allows {only}these scopes:{}", + items(&pending.granted) + ); format!( "\

KRILLBRIDGE

\

{origin}

\ -

\ +

{account}

\
This application has requested these scopes:\ {requested_items}
\
\ + data-rule=\"{rule}\" data-approves=\"granted\" \ + data-ceiling-checked=\"each-use\">\ {allowed}
\ -

{reference}

\ ", origin = escape(&record.client_origin), key = escape(&record.client_key.0), @@ -1556,10 +1548,9 @@ fn authorize_page(pending: &crate::oauth::authorize::AuthorizePending) -> String requested = escape(&record.requested.to_string()), requested_items = items(&record.requested), verdict = record.verdict.kind(), - granted = escape(&granted), - cut = escape(&cut), - rule = escape(&rule), - reference = escape(&pending.reference.0), + granted = escape(&pending.granted.to_string()), + cut = escape(&pending.cut.to_string()), + rule = escape(&pending.rule), ) } diff --git a/crates/didbot-serve/tests/oauth_agent_flow.rs b/crates/didbot-serve/tests/oauth_agent_flow.rs index 8d1be035..06c6135b 100644 --- a/crates/didbot-serve/tests/oauth_agent_flow.rs +++ b/crates/didbot-serve/tests/oauth_agent_flow.rs @@ -19,9 +19,9 @@ //! //! `GET /oauth/authorize` still renders the decision for a person, and a //! few tests read it, but nothing has to: it carries no script, no form and -//! no credential, and `data-approval-token` is there for a browser-driven -//! client that would rather read the page than call -//! `bot.did.getAuthorization`. +//! no credential. A browser-driven client approving on the agent's behalf +//! reads the one-time reference from `bot.did.getAuthorization`, under the +//! credential the approval itself takes. //! //! The same router serves the grant a signed-in client then lives on. //! `oauth::token`'s own tests rotate a refresh token against the store @@ -889,7 +889,7 @@ async fn a_policy_loaded_after_par_leaves_the_record_and_refuses_the_token() { ) .await; assert_eq!(status, StatusCode::OK, "{page}"); - assert!(page.contains("data-approval-token")); + assert!(page.contains(r#"data-approves="granted""#), "{page}"); // The list is the scope ceiling's answer, and says so. The denial the // operator just loaded is about to refuse the exchange below, so a page @@ -1522,7 +1522,7 @@ async fn a_denied_request_lists_as_deny_and_mints_no_token() { ) .await; assert_eq!(status, StatusCode::FORBIDDEN); - assert!(!page.contains("data-approval-token")); + assert!(!page.contains(r#"data-approves="granted""#), "{page}"); assert!(fixture.code_store.redeem("anything").is_none()); } @@ -1719,7 +1719,7 @@ async fn a_stranger_filling_an_accounts_pending_bound_cannot_lock_it_out() { ) .await; assert_eq!(status, StatusCode::OK, "{page}"); - assert!(page.contains("data-approval-token"), "{page}"); + assert!(page.contains(r#"data-approves="granted""#), "{page}"); } /// **The e2e defect, over the wire.** A request naming a hard-blocked atom @@ -1894,6 +1894,51 @@ async fn a_shutdown_wakes_a_long_poll_instead_of_holding_its_connection() { ); } +/// **What the page stopped handing back.** `GET /oauth/authorize` takes no +/// credential, so whatever followed the `request_uri` — the requesting +/// client included — got the page and the one-time reference printed on it. +/// The reference leaves this server only through +/// `bot.did.getAuthorization`, under the same `Credential::AgentSelf` the +/// approval itself takes. +/// +/// And the account the page is about is readable by a person now, not only +/// by something parsing attributes. +#[tokio::test] +async fn the_consent_page_shows_the_account_and_not_the_one_time_reference() { + let fixture = build(); + let (_, challenge) = code_verifier_and_challenge(); + let request_uri = push(&fixture, &challenge).await; + let reference = token_for(&fixture, &request_uri); + + let (status, page) = get( + &fixture.app, + &format!("/oauth/authorize?client_id={CLIENT_ID}&request_uri={request_uri}"), + ) + .await; + assert_eq!(status, StatusCode::OK, "{page}"); + assert!( + !page.contains(&reference), + "the page printed the one-time reference: {page}" + ); + assert!( + page.contains(&format!( + r#"

{did}

"#, + did = fixture.agent_did + )), + "{page}" + ); + + // The agent reads it out of band, with the credential it approves with. + let (status, record) = get_xrpc( + &fixture.app, + &format!("bot.did.getAuthorization?requestUri={request_uri}"), + &fixture.agent_token, + ) + .await; + assert_eq!(status, StatusCode::OK, "{record}"); + assert_eq!(record["token"], reference); +} + /// The approval token as this file reads it: off the decision the router /// already minted, which is where a daemon reads it from too. fn token_for(fixture: &Fixture, request_uri: &str) -> String {