feat(cli): revoke the session on logout instead of only deleting the file

crates/prick-auth/src/discovery.rs

@@ -235,6 +235,13 @@ pub struct AuthorizationServer {
     /// Where a client registers itself, when the server supports RFC 7591.
     #[serde(default)]
     pub registration_endpoint: Option<String>,
+    /// Where a token is revoked, when the server supports RFC 7009.
+    ///
+    /// Optional in the metadata and optional here. A server that advertises
+    /// none cannot be asked to forget a token, and `prk logout` says so rather
+    /// than implying it revoked something.
+    #[serde(default)]
+    pub revocation_endpoint: Option<String>,
     /// The PKCE methods on offer.
     #[serde(default)]
     pub code_challenge_methods_supported: Option<Vec<String>>,
@@ -695,6 +702,7 @@ mod tests {
             authorization_endpoint: "https://x/authorize".to_owned(),
             token_endpoint: "https://x/token".to_owned(),
             registration_endpoint: Some("https://x/register".to_owned()),
+            revocation_endpoint: Some("https://x/revoke".to_owned()),
             code_challenge_methods_supported: Some(vec!["S256".to_owned()]),
             scopes_supported: None,
         };

crates/prick-auth/src/oauth.rs

@@ -224,6 +224,75 @@ fn now() -> u64 {
     SystemTime::now().duration_since(UNIX_EPOCH).map_or(0, |elapsed| elapsed.as_secs())
 }
 
+/// The RFC 7009 hint for a refresh token.
+pub const HINT_REFRESH_TOKEN: &str = "refresh_token";
+
+/// The RFC 7009 hint for an access token.
+pub const HINT_ACCESS_TOKEN: &str = "access_token";
+
+/// Asks the authorization server to forget a token (RFC 7009).
+///
+/// # Which token to hand over
+///
+/// The refresh token, when there is one. RFC 7009 section 2.1 says a server
+/// SHOULD invalidate the access tokens issued from a refresh token it revokes,
+/// so revoking the renewable half is what actually ends the session -- and it
+/// is the half worth ending, because an access token expires on its own within
+/// minutes while a refresh token is what keeps a stolen credential alive.
+///
+/// # Why an unknown token is success
+///
+/// Section 2.2 requires a `200` both when the token was revoked and when the
+/// client submitted an invalid one, because the two are indistinguishable to a
+/// client and telling them apart would turn this endpoint into an oracle for
+/// whether a token is live. So a token the server has already forgotten is not
+/// an error here either: the desired state is "this token does not work", and
+/// it already holds.
+///
+/// # Errors
+///
+/// [`AuthError::Denied`] for an RFC 6749 error body, or a transport failure.
+/// Callers are expected to treat any of them as advisory: see
+/// `prk logout`, which discards the local credential either way.
+pub async fn revoke(
+    client: &Client,
+    revocation_endpoint: &str,
+    client_id: &str,
+    token: &SecretString,
+    token_type_hint: &str,
+) -> Result<(), AuthError> {
+    let form = [
+        ("token", token.expose_secret()),
+        ("token_type_hint", token_type_hint),
+        // A public client authenticates with nothing but its identity: the
+        // registration used `token_endpoint_auth_method: none`, and there is no
+        // secret to present here.
+        ("client_id", client_id),
+    ];
+
+    let received =
+        client.fetch(reqwest::Method::POST, revocation_endpoint, Body::Form(&form)).await?;
+    let facts = &received.facts;
+
+    if facts.status < 400 {
+        return Ok(());
+    }
+
+    if let Ok(body) = serde_json::from_slice::<OAuthErrorBody>(received.body()) {
+        return Err(AuthError::Denied { error: body.error });
+    }
+
+    Err(match prick_api::response::classify(facts) {
+        Some(classified) => {
+            AuthError::Api(prick_api::ApiError::from_response(facts.clone(), classified))
+        }
+        None => AuthError::Api(prick_api::ApiError::from_server(
+            facts.clone(),
+            format!("the revocation endpoint returned HTTP {}", facts.status),
+        )),
+    })
+}
+
 /// Posts to the token endpoint and interprets the result.
 ///
 /// The one place `invalid_grant` is turned into [`AuthError::AuthExpired`], so
@@ -521,6 +590,7 @@ where
             client_id: registration.client_id,
             token_endpoint: server.token_endpoint,
             resource,
+            revocation_endpoint: server.revocation_endpoint,
             tokens,
         },
         probe,
@@ -538,6 +608,7 @@ mod tests {
             authorization_endpoint: "https://example.cloudflareaccess.com/authorize".to_owned(),
             token_endpoint: "https://example.cloudflareaccess.com/token".to_owned(),
             registration_endpoint: Some("https://example.cloudflareaccess.com/register".to_owned()),
+            revocation_endpoint: Some("https://example.cloudflareaccess.com/revoke".to_owned()),
             code_challenge_methods_supported: Some(vec!["S256".to_owned()]),
             scopes_supported: None,
         }

crates/prick-auth/src/session.rs

@@ -157,6 +157,7 @@ mod tests {
             client_id: "client-1".to_owned(),
             token_endpoint: "https://example.cloudflareaccess.com/token".to_owned(),
             resource: Some("https://prick.example.com".to_owned()),
+            revocation_endpoint: Some("https://example.cloudflareaccess.com/revoke".to_owned()),
             tokens: Tokens {
                 access_token: SecretString::from("access-abc"),
                 refresh_token: refreshable.then(|| SecretString::from("refresh-xyz")),

crates/prick-auth/src/store.rs

@@ -125,6 +125,18 @@ pub struct StoredSession {
     /// every login did until Access started refusing it, so an old session
     /// refreshes as well as it ever did rather than failing to load.
     pub resource: Option<String>,
+    /// The RFC 7009 revocation endpoint, so `prk logout` can hand the token
+    /// back without repeating discovery.
+    ///
+    /// Stored rather than rediscovered because logout is the one command that
+    /// must work when the network is worse than usual -- a laptop being handed
+    /// on, a machine being decommissioned -- and a discovery round trip is one
+    /// more thing between the operator and a revoked token.
+    ///
+    /// `None` for a server that advertises no revocation endpoint, and for a
+    /// session written before this field existed. Both mean the same thing at
+    /// logout: fall back to discovery, and say so if that finds nothing.
+    pub revocation_endpoint: Option<String>,
     /// The tokens themselves.
     pub tokens: Tokens,
 }
@@ -161,6 +173,8 @@ struct Wire {
     token_endpoint: String,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     resource: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    revocation_endpoint: Option<String>,
     access_token: String,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     refresh_token: Option<String>,
@@ -183,6 +197,7 @@ impl Wire {
             client_id: session.client_id.clone(),
             token_endpoint: session.token_endpoint.clone(),
             resource: session.resource.clone(),
+            revocation_endpoint: session.revocation_endpoint.clone(),
             access_token: session.tokens.access_token.expose_secret().to_owned(),
             refresh_token: session
                 .tokens
@@ -200,6 +215,7 @@ impl Wire {
             client_id: std::mem::take(&mut self.client_id),
             token_endpoint: std::mem::take(&mut self.token_endpoint),
             resource: self.resource.take(),
+            revocation_endpoint: self.revocation_endpoint.take(),
             tokens: Tokens {
                 access_token: SecretString::from(std::mem::take(&mut self.access_token)),
                 refresh_token: self.refresh_token.take().map(SecretString::from),
@@ -621,6 +637,7 @@ mod tests {
             client_id: "client-123".to_owned(),
             token_endpoint: "https://example.cloudflareaccess.com/token".to_owned(),
             resource: Some("https://prick.example.com".to_owned()),
+            revocation_endpoint: Some("https://example.cloudflareaccess.com/revoke".to_owned()),
             tokens: Tokens {
                 access_token: SecretString::from("access-abc"),
                 refresh_token: Some(SecretString::from("refresh-xyz")),
@@ -782,6 +799,53 @@ mod tests {
         assert!(!loaded.is_refreshable());
     }
 
+    #[test]
+    fn the_revocation_endpoint_survives_a_round_trip() {
+        let (_dir, store) = store();
+        store.save(&session()).expect("save");
+
+        let loaded = store.load().expect("load").expect("a session");
+        assert_eq!(
+            loaded.revocation_endpoint.as_deref(),
+            Some("https://example.cloudflareaccess.com/revoke")
+        );
+    }
+
+    #[test]
+    fn a_credential_written_before_revocation_existed_still_loads() {
+        // The compatibility case that matters: everyone signed in today has a
+        // file with no `revocation_endpoint` in it, and a logout that refused to
+        // read it would leave them unable to sign out at all.
+        let (_dir, store) = store();
+        store.save(&session()).expect("save");
+
+        let raw = std::fs::read_to_string(store.path()).expect("read");
+        let older: serde_json::Value = serde_json::from_str(&raw).expect("JSON");
+        let mut older = older.as_object().expect("an object").clone();
+        older.remove("revocation_endpoint");
+        std::fs::write(store.path(), serde_json::to_string(&older).expect("JSON"))
+            .expect("write the older shape back");
+
+        let loaded = store.load().expect("an older file still loads").expect("a session");
+        assert!(loaded.revocation_endpoint.is_none());
+        // And the rest of it is intact, so the fallback is the only difference.
+        assert_eq!(loaded.client_id, session().client_id);
+        assert!(loaded.is_refreshable());
+    }
+
+    #[test]
+    fn a_session_with_nowhere_to_revoke_writes_no_such_field() {
+        // `skip_serializing_if`, so a server that advertises no revocation
+        // endpoint does not get a null recorded for one.
+        let (_dir, store) = store();
+        let mut without = session();
+        without.revocation_endpoint = None;
+        store.save(&without).expect("save");
+
+        let raw = std::fs::read_to_string(store.path()).expect("read");
+        assert!(!raw.contains("revocation_endpoint"), "{raw}");
+    }
+
     #[test]
     fn the_keyring_backend_says_so_rather_than_writing_a_file() {
         let dir = tempfile::tempdir().expect("a temporary directory");

crates/prick-auth/tests/login.rs

@@ -79,6 +79,7 @@ async fn mount_discovery(server: &MockServer) {
             "authorization_endpoint": format!("{}/authorize", server.uri()),
             "token_endpoint": format!("{}/token", server.uri()),
             "registration_endpoint": format!("{}/register", server.uri()),
+            "revocation_endpoint": format!("{}/revoke", server.uri()),
             "code_challenge_methods_supported": ["S256"],
             "scopes_supported": ["openid", "email", "profile", "offline_access"],
         })))
@@ -177,6 +178,13 @@ async fn a_full_login_produces_a_storable_session() {
         outcome.session.tokens.refresh_token.as_ref().map(SecretString::expose_secret),
         Some("refresh-1")
     );
+    // Recorded at login so that logout can revoke without repeating discovery,
+    // on the one command most likely to run somewhere with a worse network than
+    // the login had.
+    assert_eq!(
+        outcome.session.revocation_endpoint.as_deref(),
+        Some(format!("{}/revoke", server.uri()).as_str())
+    );
     assert!(
         outcome.session.tokens.expires_at.is_some(),
         "expires_in was not turned into a deadline"
@@ -779,6 +787,7 @@ async fn a_stale_session_is_renewed_before_the_request_that_needs_it() {
             client_id: "client-1".to_owned(),
             token_endpoint: format!("{}/token", server.uri()),
             resource: Some(server.uri()),
+            revocation_endpoint: Some(format!("{}/revoke", server.uri())),
             tokens: Tokens {
                 access_token: SecretString::from("access-stale"),
                 refresh_token: Some(SecretString::from("refresh-1")),
@@ -832,6 +841,7 @@ async fn a_revoked_session_surfaces_as_expired_rather_than_as_a_server_error() {
             client_id: "client-1".to_owned(),
             token_endpoint: format!("{}/token", server.uri()),
             resource: None,
+            revocation_endpoint: None,
             tokens: Tokens {
                 access_token: SecretString::from("stale"),
                 refresh_token: Some(SecretString::from("revoked")),
@@ -848,6 +858,101 @@ async fn a_revoked_session_surfaces_as_expired_rather_than_as_a_server_error() {
     assert_eq!(err.exit_code(), 3);
 }
 
+#[tokio::test]
+async fn a_revocation_hands_back_the_refresh_token_as_a_form_post() {
+    let server = MockServer::start().await;
+    Mock::given(method("POST"))
+        .and(path("/revoke"))
+        .respond_with(ResponseTemplate::new(200))
+        .mount(&server)
+        .await;
+
+    prick_auth::oauth::revoke(
+        &client_for(&server),
+        &format!("{}/revoke", server.uri()),
+        "client-1",
+        &SecretString::from("refresh-xyz"),
+        prick_auth::oauth::HINT_REFRESH_TOKEN,
+    )
+    .await
+    .expect("a 200 means the server has forgotten it");
+
+    let requests = server.received_requests().await.expect("the server recorded requests");
+    let revocation = requests
+        .iter()
+        .find(|request| request.url.path() == "/revoke")
+        .expect("the revocation was sent");
+
+    let body = String::from_utf8_lossy(&revocation.body);
+    let sent: std::collections::HashMap<String, String> =
+        url::form_urlencoded::parse(body.as_bytes())
+            .map(|(key, value)| (key.into_owned(), value.into_owned()))
+            .collect();
+
+    assert_eq!(sent.get("token").map(String::as_str), Some("refresh-xyz"));
+    assert_eq!(sent.get("token_type_hint").map(String::as_str), Some("refresh_token"));
+    // A public client authenticates with its identity alone, so the id has to be
+    // in the body -- without it the server cannot tell whose token this is.
+    assert_eq!(sent.get("client_id").map(String::as_str), Some("client-1"));
+
+    let content_type = revocation
+        .headers
+        .get("content-type")
+        .and_then(|value| value.to_str().ok())
+        .unwrap_or_default();
+    assert!(content_type.starts_with("application/x-www-form-urlencoded"), "{content_type}");
+}
+
+#[tokio::test]
+async fn a_token_the_server_never_knew_is_not_a_failure() {
+    // RFC 7009 section 2.2: a `200` covers both "revoked" and "that was not a
+    // token I recognise", because distinguishing them would make this endpoint
+    // an oracle for whether a token is live. The desired state already holds.
+    let server = MockServer::start().await;
+    Mock::given(method("POST"))
+        .and(path("/revoke"))
+        .respond_with(ResponseTemplate::new(200))
+        .mount(&server)
+        .await;
+
+    prick_auth::oauth::revoke(
+        &client_for(&server),
+        &format!("{}/revoke", server.uri()),
+        "client-1",
+        &SecretString::from("never-existed"),
+        prick_auth::oauth::HINT_ACCESS_TOKEN,
+    )
+    .await
+    .expect("an unknown token leaves nothing to do");
+}
+
+#[tokio::test]
+async fn a_refused_revocation_reports_the_servers_own_reason() {
+    let server = MockServer::start().await;
+    Mock::given(method("POST"))
+        .and(path("/revoke"))
+        .respond_with(ResponseTemplate::new(400).set_body_json(serde_json::json!({
+            "error": "unsupported_token_type"
+        })))
+        .mount(&server)
+        .await;
+
+    let err = prick_auth::oauth::revoke(
+        &client_for(&server),
+        &format!("{}/revoke", server.uri()),
+        "client-1",
+        &SecretString::from("refresh-xyz"),
+        prick_auth::oauth::HINT_REFRESH_TOKEN,
+    )
+    .await
+    .expect_err("a 400 is a refusal");
+
+    match err {
+        AuthError::Denied { error } => assert_eq!(error, "unsupported_token_type"),
+        other => panic!("expected the server's own error code, got {other:?}"),
+    }
+}
+
 fn now() -> u64 {
     std::time::SystemTime::now()
         .duration_since(std::time::UNIX_EPOCH)

crates/prk/src/cli.rs

@@ -174,8 +174,8 @@ pub enum Command {
     /// Authenticate against a prick server.
     Login(commands::auth::LoginArgs),
 
-    /// Discard stored credentials.
-    Logout,
+    /// Revoke the session and discard stored credentials.
+    Logout(commands::auth::LogoutArgs),
 
     /// Show the identity the server sees.
     Whoami,
@@ -218,7 +218,7 @@ impl Command {
     pub fn path(&self) -> &'static str {
         match self {
             Self::Login(_) => "login",
-            Self::Logout => "logout",
+            Self::Logout(_) => "logout",
             Self::Whoami => "whoami",
             Self::Doctor => "doctor",
             Self::Projects(sub) => sub.path(),