OAuth token has an expiry (1h) and will render the session unusable when it is expired

[tedsteen]

Description

When authenticating with an access token using OAuth we only pass the access token, but that will only be valid for 1h (expiry time was 3600s when I checked).
Since we only pass the access token the information about the expiry and refresh token is lost and we can't recover.

Would it make more sense to pass the entire OAuth token from the oauth2 crate and use the information in there to get a valid token?

Version

0.5.0

How to reproduce

Steps to reproduce the behavior in librespot e.g.

1. Start a session
2. Wait 1h
3. Try to do anything that requires an authenticated session
4. API throws Unauthenticated errors

Host:

• OS: MacOS (but probably applies to all platforms)

[roderickvd]

@kingosticks makes sense?

[kingosticks]

Not really, due to

librespot/core/src/session.rs

Line 169 in d8e8423

if credentials.auth_type == AuthenticationType::AUTHENTICATION_SPOTIFY_TOKEN {

Does that trace message not come out when you're authing with your access token? Where's the requested log?

[tedsteen]

Haven't looked at trace, but looking at the code it seems like there is nothing you can do once the token has expired, even if it is saved/cached somewhere.

To be able to continue the session you would need to exchange it for a refresh token: https://auth0.com/docs/secure/tokens/refresh-tokens/use-refresh-tokens#call-the-api

[tedsteen]

I guess I could keep track of the token expiry on my side and make sure to update the token in the librespot session, but I think it would be nicer if it was part of the library.

[kingosticks]

Sorry, I'm confused now. Do the provided steps actually produce a problem or is this a hypothetical problem based on code inspection ?

In a librespot season, we login only very briefly with the provided access token - only to obtain reusable credentials. We then forget all about that access token and associated session. We login again to a fresh session using the reusable credentials we just got. There should be no expiry issue with that second season, it didn't come from the original access token. You can use that second session to obtain a new access token to call other Spotify APIs. Or keep using your original login access token. All access tokens will expire at some point and so you must be aware of that in any of your code that might be reusing them.

Given the above, I don't think we need to concern ourselves with any more oauth stuff. Are you trying to integrate librespot with other code? Can you provide a concrete failing example so I can better understand.

[tedsteen]

The provided steps are correct at least from my machine and implementation, my last comment is based on assumptions from looking at the code.

I assumed that you used the access token I passed in to the session, but that assumption was apparently wrong..
So you create a long lived session internally then? That is not ideal from a security perspective, but that's a different issue.

I am integrating librespot with my code and I can see if I can create a simple reproducible failing example.. It's a bit messy tho because you have to wait an hour for it to happen.

In any case, since my assumptions where wrong this issue is wrong and should probably be closed. I could open a new one if I manage to create a reproducable.
Sorry for the noise.

[kingosticks]

I assumed that you used the access token I passed in to the session, but that assumption was apparently wrong.. So you create a long lived session internally then? That is not ideal from a security perspective, but that's a different issue.

We mimic how the desktop client works. As it stands, I don't see a problem but happy to try and better understand your view if you create a new issue focused on that.

[tedsteen]

Got back to my computer after being away for some time today and got this
Error { kind: Unauthenticated, error: StatusCode(401) }
when trying to call Track::get.

Since you described how the session works internally my guess is that I do something wrong.
Here is the relevant code for how I use the library (including the call to Track::get (SpotifyPlayer::get_track))

use std::sync::{Arc, Mutex};

use crate::oauth::{OAuthError, OAuthFlow};
use librespot::{
    core::{
        authentication::Credentials, cache::Cache, config::SessionConfig, session::Session,
        spotify_id::SpotifyId, Error,
    },
    metadata::{Metadata, Track},
    playback::{
        audio_backend,
        config::{AudioFormat, PlayerConfig},
        mixer::VolumeGetter,
        player::Player,
    },
};
use oauth2::TokenResponse;
use tauri::AppHandle;
use thiserror::Error;

use crate::settings::get_config_dir;

pub struct SpotifyPlayer {
    player: Arc<Player>,
    session: Session,
    volume: Arc<Mutex<u16>>,
    cache: Cache,
}

const DEFAULT_VOLUME: u16 = 80;
impl SpotifyPlayer {
    #[allow(clippy::new_without_default)]
    pub fn new() -> Self {
        let cache = get_config_dir()
            .and_then(|config_dir| {
                Cache::new(
                    Some(config_dir.clone()),
                    Some(config_dir.clone()),
                    Some(config_dir),
                    None,
                )
                .ok()
            })
            .expect("a cache to be created");

        let backend = audio_backend::find(None).unwrap();
        let player_config = PlayerConfig::default();

        struct SpotiampVolumeGetter {
            volume: Arc<Mutex<u16>>,
        }

        impl VolumeGetter for SpotiampVolumeGetter {
            fn attenuation_factor(&self) -> f64 {
                *self.volume.lock().unwrap() as f64 / 100.0
            }
        }
        let session = Session::new(SessionConfig::default(), Some(cache.clone()));
        let volume = Arc::new(Mutex::new(cache.volume().unwrap_or(DEFAULT_VOLUME)));
        let player = Player::new(
            player_config,
            session.clone(),
            Box::new(SpotiampVolumeGetter {
                volume: volume.clone(),
            }),
            move || {
                let audio_format = AudioFormat::default();
                backend(None, audio_format)
            },
        );
        Self {
            player,
            session,
            volume,
            cache,
        }
    }

    pub async fn login(&self, app: &AppHandle) -> Result<(), SessionError> {
        log::debug!("Getting credentials");
        let credentials = match self.cache.credentials() {
            Some(credentials) => credentials,
            None => {
                log::debug!("No credentials in cache, starting OAuth flow...");
                Self::get_credentials_from_oauth(app).await?
            }
        };

        self.session
            .connect(credentials, true)
            .await
            .map_err(|e| SessionError::ConnectError { e })?;
        log::debug!("Success! Using credentials from OAuth-flow and saving them for next time");
        Ok(())
    }

    async fn get_credentials_from_oauth(app: &AppHandle) -> Result<Credentials, SessionError> {
        let oauth_flow = OAuthFlow::new(
            "https://accounts.spotify.com/authorize",
            "https://accounts.spotify.com/api/token",
            "65b708073fc0480ea92a077233ca87bd",
        )
        .map_err(|e| SessionError::OauthError { e })?;

        let auth_url = oauth_flow.get_auth_url();
        log::debug!("Opening URL: {auth_url}");

        tauri::WebviewWindowBuilder::new(
            app,
            "login",
            tauri::WebviewUrl::External(auth_url.parse().expect("a valid auth URL")),
        )
        .title("Login")
        .inner_size(600.0, 800.0)
        .closable(false)
        .build()
        .map_err(|e| SessionError::OpenURLFailed { url: auth_url, e })?;

        let token = oauth_flow
            .start()
            .await
            .map_err(|e| SessionError::TokenExchangeFailure { e })?;

        Ok(Credentials::with_access_token(
            token.access_token().secret(),
        ))
    }

    pub async fn play(&mut self, uri: Option<&str>) -> Result<(), PlayError> {
        log::debug!("Play!");
        if let Some(uri) = uri {
            self.player.load(
                SpotifyId::from_uri(uri).map_err(|e| PlayError::TrackMetadataError { e })?,
                false,
                0,
            );
        }

        self.player.play();
        Ok(())
    }

    pub async fn pause(&mut self) -> Result<(), PlayError> {
        log::debug!("Pause!");
        self.player.pause();
        Ok(())
    }

    pub async fn stop(&mut self) -> Result<(), PlayError> {
        log::debug!("Stop!");
        self.player.stop();
        Ok(())
    }

    pub async fn get_track(&mut self, track: SpotifyId) -> Result<Track, PlayError> {
        log::debug!("Getting track data: {:?}", track);
        Track::get(&self.session, &track)
            .await
            .map_err(|e| PlayError::TrackMetadataError { e })
    }

    pub fn set_volume(&mut self, volume: u16) {
        *self.volume.lock().unwrap() = volume;
        self.cache.save_volume(volume);
    }

    pub fn get_volume(&self) -> u16 {
        *self.volume.lock().unwrap()
    }
}

#[derive(Debug, Error)]
pub enum SessionError {
    #[error("Failed to connect ({e:?}")]
    ConnectError { e: Error },

    #[error("OAuth error ({e:?}")]
    OauthError { e: OAuthError },

    #[error("Could not open URL {url} ({e:?})")]
    OpenURLFailed { url: String, e: tauri::Error },

    #[error("Could not get token ({e:?}")]
    TokenExchangeFailure { e: OAuthError },
}

#[derive(Debug, Error)]
pub enum PlayError {
    #[error("Failed to fetch metadata ({e:?})")]
    TrackMetadataError { e: Error },
}

This is how I store my SpotifyPlayer instance

pub fn player() -> &'static Mutex<SpotifyPlayer> {
    static MEM: OnceLock<Mutex<SpotifyPlayer>> = OnceLock::new();
    MEM.get_or_init(|| Mutex::new(SpotifyPlayer::new()))
}

[kingosticks]

And this code works if you call get_track straight away but reliably fails when you call it an hour after logging in? Can you please provide a debug log? And just to confirm, this is with 0.5.0, and not the very latest dev branch, right? Can you reproduce the error with the very latest code? I obviously don't have everything I need to run your code above but I'll try a similar example tomorrow.

[tedsteen]

And this code works if you call get_track straight away but reliably fails when you call it an hour after logging in?

Yes, it works fine until I leave it running for some time. I haven't timed it exactly, the one hour was an assumption based on the oauth timeout, but it happens every time I leave the app running and come back later (always more than 1h).

yes, this is 0.5.0, not latest dev branch.

I can try it out on the latest dev, but will have to get back to you when I have left it running for the bug to trigger..

[kingosticks]

I had a look at this earlier and I can't reproduce the issue on my Linux machine using what I hope is simplified equivalent code. I don't have a mac to try this on; it's possible the platforms behave differently, but I think if we did have a fundamental problem here then someone would have already hit it.

I tried with both 0.5.0 and latest dev, you can see from the logs they are different due to the recent addition of login5 support (#1344).

Maybe leaving it longer would have triggered it but then it would seem less likely to be related to oauth token expiry. A debug log showing the problem would really help.