librespot icon indicating copy to clipboard operation
librespot copied to clipboard

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

Open tedsteen opened this issue 1 year ago • 10 comments

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)

tedsteen avatar Oct 19 '24 17:10 tedsteen

@kingosticks makes sense?

roderickvd avatar Oct 19 '24 18:10 roderickvd

Not really, due to https://github.com/librespot-org/librespot/blob/d8e84238abf39878e382a4008f2672ddb4260e80/core/src/session.rs#L169 Does that trace message not come out when you're authing with your access token? Where's the requested log?

kingosticks avatar Oct 19 '24 18:10 kingosticks

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 avatar Oct 19 '24 19:10 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.

tedsteen avatar Oct 19 '24 19:10 tedsteen

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.

kingosticks avatar Oct 19 '24 19:10 kingosticks

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.

tedsteen avatar Oct 19 '24 20:10 tedsteen

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.

kingosticks avatar Oct 19 '24 21:10 kingosticks

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()))
}

tedsteen avatar Oct 20 '24 20:10 tedsteen

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.

kingosticks avatar Oct 20 '24 22:10 kingosticks

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..

tedsteen avatar Oct 20 '24 23:10 tedsteen

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.

kingosticks avatar Oct 21 '24 23:10 kingosticks

Assuming fixed in v0.6.0. Apparently, the tokens gotten through login5 live longer.

roderickvd avatar Nov 03 '24 19:11 roderickvd

I didn't want to open a new issue, because this seems quite adjacent. Also, possible user error? I wouldn't think so, but you never know. Here's a trimmed-down console log of an hour-of-activity from launch to failure.

  • OS: Windows 10 x64 22H2
  • Version: 0.6.0 (MSVC toolchain-compiled)
  • Use case: spotifyd-like, as a service with NSSM (logged in as user)
  • Login method: OAuth, set up with --enable-oauth option and stored to credentials.json in cache dir
  • Service's arguments: -n Imma-Think -O -c "%appdata%\librespot" -b 320 -d "VoiceMeeter VAIO3 Input (VB-Audio VoiceMeeter VAIO3)" -N -R 100

I first noticed weirdness when I was leaving the service unattended for a while- it wouldn't work. Restarting the service would fix it. Today, I was suspicious the timeframe was about an hour. Today, I just threw on some Bad Religion and the song The Island just kept repeating over and over again, and the log just started spinning its wheels. I had a look, and it's exactly at :58 on the next hour it manifests.

I feel like I'm being gaslit because this shouldn't be an issue? Perhaps one of my arguments is having an unintended (or intended?) effect?

instinctualjealousy avatar Feb 16 '25 22:02 instinctualjealousy

You could try installing the latest development version to test if it's fixed, as we made some bigger changes to the connect handling. I will take a look later into your log to see if there is anything suspicious.

cargo install --git https://github.com/librespot-org/librespot

photovoltex avatar Feb 17 '25 07:02 photovoltex

This would be better in a new issue. As I explained at https://github.com/librespot-org/librespot/issues/1377#issuecomment-2424163616, we simply do not hold/use the OAuth token for more than a few milliseconds. So while it does expire after an hour, we are finished with it long before then.

One of the other "tokens" may have expired while we are still using it. They should be being refreshed well before they expire, but it's possible there is a bug there.

kingosticks avatar Feb 17 '25 08:02 kingosticks

The dealer for example, refreshes the token when the websocket encounters an error, otherwise we wouldn't be able to restart it. Thats why dev might behave differently. But I agree with @kingosticks, it's probably better to open a separate issue, as we can always just close it if it's not relevant or a differnt issue^^.

photovoltex avatar Feb 17 '25 11:02 photovoltex

I'll open a new issue if it persists in this dev build- I'm very suspicious it'll work just fine though!

Edit: I've had no issues like the above mentioned on the dev build. It seems the issue has likely been fixed via those mentioned improvements in connectivity code.

instinctualjealousy avatar Feb 17 '25 16:02 instinctualjealousy