Type-state pattern and state management for credentials struct used with Spotify web API

Question

I am implementing authentication with the Spotify web API as part of a larger project. Yes I know there are already crates that can handle it for me but that is no fun. I am implementing the auth code flow. To spare you reading the documentation, you basically open a URL with the API credentials and some other info in the URL query for the user to authorize your app, and the user is redirected to the callback URL you specified with the auth code in the query, so I just listen on the localhost port to capture the redirect. Then you send a POST request to the token endpoint with the API credentials and the auth code to get your access token.

There are multiple things that can be improved about my auth module, but the main thing I am looking for feedback on is the type-state pattern of my Credentials struct. I attempted to implement it, but my implementation doesn't really achieve the goal of it.

The NoAuthCode and AuthCodePresent marker struts don't inherently have any meaning, and don't guarantee anything about the presence or type of the auth code. I still have to do a runtime check to ensure that the auth code is Some before using it even though the function only accepts Credentials<AuthCodePresent>. (Yes I know I am unwrap()ing it and I could just if let and return an Err but I want to solve the actual problem with my design).

How can I adjust my design to be able to guarantee the presence/absence of the auth code? Is the type-state pattern even the right tool for the job here, or is there another pattern I should be using?

I will gladly accept any feedback about the module, but I am specifically looking for feedback on the state management.

//! `src/auth.rs`

use base64::{engine::general_purpose::STANDARD as b64, Engine};
use rand::{distributions::Alphanumeric, Rng};
use reqwest::header::{HeaderMap, HeaderValue, AUTHORIZATION, CONTENT_TYPE};
use serde::{Deserialize, Serialize};
use std::{error::Error, marker::PhantomData};
use url::Url;

use crate::error::SfsError;

pub async fn authenticate(creds: &Credentials<NoAuthCode>) -> Result<AccessToken, Box<dyn Error>> {
    let creds = creds.clone();
    let creds = creds.get_auth_code()?;
    let token = creds.get_access_token().await?;
    Ok(token)
}

#[doc(hidden)]
#[derive(Clone)]
pub struct NoAuthCode;
#[doc(hidden)]
#[derive(Clone)]
pub struct AuthCodePresent;

#[derive(Clone)]
pub struct Credentials<State> {
    pub client_id: String,
    pub client_secret: String,
    authorization_code: Option<String>,
    state: String,
    auth_code_state: PhantomData<State>,
}

impl Credentials<NoAuthCode> {
    pub fn new(client_id: &str, client_secret: &str) -> Credentials<NoAuthCode> {
        Credentials {
            client_id: String::from(client_id),
            client_secret: String::from(client_secret),
            authorization_code: None,
            state: rand::thread_rng()
                .sample_iter(&Alphanumeric)
                .take(64)
                .map(char::from)
                .collect(),
            auth_code_state: PhantomData,
        }
    }

    pub fn get_auth_code(self) -> Result<Credentials<AuthCodePresent>, Box<dyn Error>> {
        let base_url = "https://accounts.spotify.com/authorize?";

        let auth_code_request = AuthCodeRequest::new(&self);
        let url_params = serde_urlencoded::to_string(&auth_code_request)?;

        let auth_code_request_url = Url::parse(&format!("{}{}", base_url, url_params))?;

        let server = tiny_http::Server::http("0.0.0.0:8888").unwrap();
        webbrowser::open(auth_code_request_url.as_str())?;

        let request = server.recv()?;
        let callback_url = request.url();

        if !callback_url.starts_with("/callback?") {
            return Err(SfsError::new_box("Error parsing callback url"));
        }

        let callback = serde_urlencoded::from_str::<AuthCodeCallback>(&callback_url[10..])?;

        if callback.error.is_some() {
            return Err(SfsError::new_box("Error during authentication."));
        }
        if &callback.state != &self.state {
            return Err(SfsError::new_box("Mismatched states"));
        }

        request.respond(
            tiny_http::Response::from_string(
                "<html><body><script>window.close();</script></body></html>",
            )
            .with_header(tiny_http::Header {
                field: "Content-Type".parse().unwrap(),
                value: "text/html".parse().unwrap(),
            }),
        )?;

        Ok(self.add_auth_code(&callback.code.unwrap()))
    }

    fn add_auth_code(self, auth_code: &str) -> Credentials<AuthCodePresent> {
        Credentials {
            client_id: self.client_id,
            client_secret: self.client_secret,
            authorization_code: Some(auth_code.to_string()),
            state: self.state,
            auth_code_state: PhantomData,
        }
    }
}

impl Credentials<AuthCodePresent> {
    pub async fn get_access_token(&self) -> Result<AccessToken, Box<dyn Error>> {
        let mut headers = HeaderMap::new();
        headers.insert(
            CONTENT_TYPE,
            HeaderValue::from_static("application/x-www-form-urlencoded"),
        );
        headers.insert(
            AUTHORIZATION,
            HeaderValue::from_str(&format!(
                "Basic {}",
                b64.encode(format!("{}:{}", self.client_id, self.client_secret))
            ))?,
        );

        let response = reqwest::Client::new()
            .post("https://accounts.spotify.com/api/token")
            .headers(headers)
            .form(&[
                ("code", self.authorization_code.clone().unwrap()),
                ("redirect_uri", "http://localhost:8888/callback".to_string()),
                ("grant_type", "authorization_code".to_string()),
            ])
            .send()
            .await?;

        let token: AccessToken = serde_json::from_str(&response.text().await?)?;

        Ok(token)
    }
}

#[derive(Deserialize, Debug)]
pub struct AccessToken {
    pub access_token: String,
    token_type: String,
    scope: String,
    expires_in: i32,
    refresh_token: String,
}

#[derive(Serialize)]
struct AuthCodeRequest {
    client_id: String,
    response_type: String,
    redirect_uri: String,
    state: String,
    scope: String,
}

impl AuthCodeRequest {
    fn new(creds: &Credentials<NoAuthCode>) -> AuthCodeRequest {
        AuthCodeRequest {
            client_id: creds.client_id.clone(),
            response_type: "code".to_string(),
            redirect_uri: "http://localhost:8888/callback".to_string(),
            state: creds.state.clone(),
            scope: format!(
                "{} {} {}",
                "playlist-read-private", "playlist-modify-public", "playlist-modify-private"
            ),
        }
    }
}

#[derive(Deserialize)]
struct AuthCodeCallback {
    code: Option<String>,
    error: Option<String>,
    state: String,
}

This module does depend on the error module in my project, but all it contains at the moment is a simple error struct implements the std::error::Error trait, so I didn't feel the need to include it.

Answer 1

Needless cloning and shadowed variables

Currently you're immediately shadowing the cloned creds in authenticate(). You can write authenticate() in one expression without cloning by taking the required ownership of the credentials. The user can then clone the to-be-passed credentials as needed:

pub async fn authenticate(creds: Credentials<NoAuthCode>) -> Result<AccessToken, Box<dyn Error>> {
    creds.get_auth_code()?.get_access_token().await
}

Prefer expect() over unwrap()

You currently unwrap the result of spawning the HTTP server. Consider using expecting it instead with an appropriate error message as to why you consider the respective socket to always be available (and maybe reconsider whether you can really assume this).

Avoid magic values

The HTML text and maybe even some other strings may be better of put into a constant and having some documentation. This is especially important if you use magic values repeatedly, which rely on each other, such as "http://localhost:8888/callback".

Separation of concerns

The Credentials struct, judging by its name, should represent credentials. It should not be concerned with spawning HTTP servers in its methods. Consider factoring out the contacting of the API into another struct, which produces the credentials. If you do that, you may also make the local HTTP server's socket address part of its configuration (see above).

Usage of types to identify state

If you use types to identify state, you can as well put the required data into the respective object:

#[doc(hidden)]
#[derive(Clone)]
pub struct NoAuthCode;
#[doc(hidden)]
#[derive(Clone)]
pub struct AuthCodePresent(String);

#[derive(Clone)]
pub struct Credentials<State> {
    pub client_id: String,
    pub client_secret: String,
    state: String,
    auth_code_state: State,
}

...


    fn add_auth_code(self, auth_code: &str) -> Credentials<AuthCodePresent> {
        Credentials {
            client_id: self.client_id,
            client_secret: self.client_secret,
            state: self.state,
            auth_code_state: AuthCodePresent(auth_code.to_string()),
        }
    }

... etc.

Addendum: You can additionally introduce a private trait to disallow users to pass in arbitrary types:

trait _State {}
impl _State for NoAuthCode {}
impl _State for AuthCodePresent {}

...

pub struct Credentials<State: _State> {
    ...
}