Quesion: Client Credentials - am I doing it right?

[jasonterando]

Hi, I'm successfully using oauth2-rs to retrieve AWS Cognito client credentials from their token endpoint. My question concerns BasicClient::new. That function requires an Auth URL, which isn't needed for ClientCredential flow, and has the token URL optional, which is mandatory for Client Credential flow.

Am I doing the right thing in using BasicClient for Client Credential flow? I'm just putting the same URL in for both auth and token URLs, and it seems to work. Happy to keep doing it, I just want to make sure I'm using the library as intended.

Sample code...

// Build out a client and request using some parameters called client_id, client_secret and access_token_url
let client = BasicClient::new(
    ClientId::new(client_id),
    if client_secret.is_empty() {
        None
    } else {
        Some(ClientSecret::new(client_secret))
    },
    AuthUrl::new(access_token_url.clone()).unwrap(),
    Some(TokenUrl::new(access_token_url).unwrap()),
);

let mut token_request = client.exchange_client_credentials();
if scope.is_some() {
    token_request = token_request.add_scope(Scope::new(scope.unwrap()));
}
let token_result = token_request
    .request_async(async_http_client)
    .await;

match token_result {
    Ok(token) => {
        // Add the access token to the reqwest request builder...
        request_builder = request_builder.bearer_auth(token.access_token().secret());
    },
    Err(err) => {
        return Err(DispatchError::OAuth2(err));
    }
}

[ramosbugs]

Yup, the authorization endpoint isn't used for the client credentials flow, so you can set it to any syntactically-valid URL.

The Client API is optimized around the more common authorization code flow, and I wanted to avoid the possibility of users setting the authorization endpoint to None and then having that fail at runtime, which would require making authorize_url() fallible. Ideally the type system wouldn't let users call authorize_url() without setting an authorization endpoint, but that's difficult (though not impossible) to express in Rust. There's definitely room for improvement with this API, but none of the options seem perfect.

[jasonterando]

Thanks David. Still wrapping my head around Rust idioms and such. It may be that a "builder" pattern could work for this, but I'll reserve any formal opinion until I know more what the heck I'm talking about. Thanks for the crate and the response!

[ramosbugs]

The authorization endpoint is no longer required as of 1d1f4d17ecdf2a3feb565eb1789cc8649cac7705, which will be part of the upcoming 5.0 release

[jasonterando]

Nice!

[ramosbugs]

This is now released in 5.0.0-alpha.1. If you can, please try it out and let me know if you find any issues that should be fixed before stabilizing the 5.0 API. There's an Upgrade Guide to help.

[jasonterando]

Hi, took a stab at 5.0, and got it to work. A couple of implementation notes, not sure if they are worth calling out in the documentation or not...

Topic 1 It may be worth calling out in the docs that any of the "set_*" calls must be made inline with the initial BaseClient::new call. Doing something like the following won't work because the client.exchange_client_credentials will trigger a "borrow on moved" exception:

let client = BasicClient::new(ClientId::new(client_id.clone()))
    .set_token_uri(
        TokenUrl::new(token_url.to_string()).expect("Unable to parse OAuth token URL"),
    );

if ! client_secret.is_empty() {
    client.set_client_secret(ClientSecret::new(client_secret.to_string()));
}

let mut token_request = client.exchange_client_credentials();
//                      ^ borrow of moved value "client"

In this case, it's a hypothetical "what if there is no secret" which I'm willing to ignore because I can't think of any OAuth2 implementation for which I did not need both a client and secret. I don't think anything needs to be fixed here.

BTW - the conditional implementation for stuff like exchange_client_credentials is super clever - Rust continues to blow me away with stuff like this (once you figure it out)

Topic 2 It might be worth including an example that shows how to wrap non-async calls in spawn_blocking, because I think other people may run into similar issues that I did. Namely:

a. You have an application which is running under Tokio (in my case, it's a Tauri application) b. The exception types in oauth2 are not Copy, so that presented problems when calling from async_recursion functions. But even if we fixed that, I'm using a static variable to cache tokens, and lazy_static is not async friendly. So, I could only get stuff working with blocking. Admittedly, I'm still a very junior Rust dev, so there may be ways to do this.

Example of what appears to work for me:

//! oauth2_client_tokens
//!
//! This module implements OAuth2 client flow support, including support for caching tokens

use std::collections::HashMap;
use std::ops::Add;
use std::sync::Mutex;
use std::time::Instant;

use oauth2::basic::BasicClient;
use oauth2::reqwest::reqwest;
use oauth2::{ClientId, ClientSecret, Scope, TokenResponse, TokenUrl};
use tokio::task::spawn_blocking;

use crate::ExecutionError;

lazy_static! {
    /// Static cache of retrieved OAuth2 client tokens
    static ref OAUTH2_TOKEN_CACHE: Mutex<HashMap<String, (Instant, String)>> =
        Mutex::new(HashMap::new());
}

/// Return cached oauth2 token, with indicator of whether value was cached
pub async fn get_oauth2_client_credentials(
    id: &String,
    token_url: &String,
    client_id: &String,
    client_secret: &String,
    scope: &Option<String>,
) -> Result<(String, bool), ExecutionError> {
    let cloned_id = id.clone();
    let cloned_token_url = token_url.clone();
    let cloned_client_id = client_id.clone();
    let cloned_client_secret = client_secret.clone();
    let cloned_scope = scope.clone();

    // We have to create a blocked span because we are accesing a static cache of tokens,
    // and some error types in oauth2 library do not implement Copy
    match spawn_blocking(move || {
        let mut tokens = OAUTH2_TOKEN_CACHE.lock().unwrap();
        let valid_token = match tokens.get(&cloned_id) {
            Some(existing) => {
                if existing.0.gt(&Instant::now()) {
                    Some(existing.1.clone())
                } else {
                    None
                }
            }
            None => None,
        };

        // If cached token is valid not expired, return it
        if let Some(token) = valid_token {
            return Ok((token, true));
        }

        // Retrieve an access token
        let client = BasicClient::new(ClientId::new(cloned_client_id))
            .set_token_uri(
                TokenUrl::new(cloned_token_url).expect("Unable to parse OAuth token URL"),
            )
            .set_client_secret(ClientSecret::new(cloned_client_secret));

        let mut token_request = client.exchange_client_credentials();
        if let Some(scope_value) = cloned_scope {
            token_request = token_request.add_scope(Scope::new(scope_value.clone()));
        }

        let http_client = reqwest::blocking::ClientBuilder::new()
            .redirect(reqwest::redirect::Policy::none())
            .build()
            .expect("Unable to build OAuth HTTP client");

        match token_request.request(&http_client) {
            Ok(token_response) => {
                let expiration = match token_response.expires_in() {
                    Some(token_expires_in) => Instant::now().add(token_expires_in),
                    None => Instant::now(),
                };
                let token = token_response.access_token().secret().clone();

                tokens.insert(cloned_id, (expiration, token.clone()));
                Ok((token, false))
            }
            Err(err) => Err(ExecutionError::OAuth2(err)),
        }
    })
    .await
    {
        Ok(result) => result,
        Err(err) => Err(ExecutionError::Join(err)),
    }
}

/// Clear all cached OAuth2 tokens
pub fn clear_all_oauth2_tokens() {
    OAUTH2_TOKEN_CACHE.lock().unwrap().clear();
}

/// Clear specified cached OAuth2 credentials, returning true if value was cached
pub fn clear_oauth2_token(id: String) -> bool {
    let mut tokens = OAUTH2_TOKEN_CACHE.lock().unwrap();
    tokens.remove(&id).is_some()
}

[ramosbugs]

Thanks so much for taking the time to try this out and provide feedback!

Doing something like the following won't work because the client.exchange_client_credentials will trigger a "borrow on moved" exception:

let client = BasicClient::new(ClientId::new(client_id.clone()))
    .set_token_uri(
        TokenUrl::new(token_url.to_string()).expect("Unable to parse OAuth token URL"),
    );

if ! client_secret.is_empty() {
    client.set_client_secret(ClientSecret::new(client_secret.to_string()));
}

let mut token_request = client.exchange_client_credentials();
//                      ^ borrow of moved value "client"

In this case, I think you just need to reassign the client variable:

let mut client = BasicClient::new(ClientId::new(client_id.clone()))
    .set_token_uri(
        TokenUrl::new(token_url.to_string()).expect("Unable to parse OAuth token URL"),
    );

if ! client_secret.is_empty() {
    client = client.set_client_secret(ClientSecret::new(client_secret.to_string()));
}

Does that work?

Topic 2 It might be worth including an example that shows how to wrap non-async calls in spawn_blocking, because I think other people may run into similar issues that I did.

Using blocking code inside async is generally a recipe for trouble. I've added a note to the docs in 1fc8188b22eaa055d3eca748964fca5876a448e2 reminding users of this, but it's a general issue in Rust. I agree with the commenters in the issue you cited that it feels wrong for async code to have to know whether any code it transitively calls might block (without some kind of marker trait or similar approach to propagate that information to callers).

Namely:

a. You have an application which is running under Tokio (in my case, it's a Tauri application) b. The exception types in oauth2 are not Copy, so that presented problems when calling from async_recursion functions. But even if we fixed that, I'm using a static variable to cache tokens, and lazy_static is not async friendly. So, I could only get stuff working with blocking. Admittedly, I'm still a very junior Rust dev, so there may be ways to do this.

For globals accessed from async code, something like futures::lock::Mutex can be useful. Many of the error types that this crate embeds that come from other crates aren't Clone, let alone Copy, so that's not really something that can be addressed.

[jasonterando]

Hi. The reassignment worked for the optional set_client_secret, so good there. Per your guidance, I switched to the Tokio Mutex for the static cache and was able to move the cache access outside of the spawn_blocking block. Thanks again!