Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ use teloxide::dispatching::dialogue::RedisStorage as TeloxideRedisStorage;
use teloxide::dispatching::dialogue::serializer::Bincode;
use teloxide::requests::RequesterExt as _;

use crate::entity::prelude::UserStatus;
use crate::metrics::influx::InfluxClient;
use crate::metrics::prometheus::PrometheusClient;
use crate::queue::QueueManager;
use crate::services::{AISlopDetectionService, SongLinkService, UserService};
use crate::spotify::TokenState;
use crate::user::UserState;
use crate::{lyrics, profanity, spotify};
use crate::{lyrics, profanity, spotify, telegram};

pub struct App {
spotify_manager: spotify::Manager,
Expand Down Expand Up @@ -372,8 +374,21 @@ impl App {
}

pub async fn user_state(&'static self, user_id: &str) -> anyhow::Result<UserState> {
let spotify = self.spotify_manager.for_user(&self.db, user_id).await?;
let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?;

if matches!(token_state, TokenState::Invalid)
&& user.status != UserStatus::SpotifyTokenInvalid
{
UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?;

// NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way
// with error_handler module
if let Err(err) = telegram::notify_token_invalid(self, &user).await {
tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token");
}
}

Comment on lines 377 to +391

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Stale user status in UserState.

When the token is determined to be invalid, the user status is updated in the database via UserService::set_status, but the in-memory user variable is not updated. Consequently, the UserState created on line 392 will carry a stale status. Any downstream logic expecting the status to be SpotifyTokenInvalid during this execution flow will behave incorrectly.

Update the user model's status in memory before constructing UserState.

♻️ Proposed fix
-        let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
+        let (mut user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
         let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?;

         if matches!(token_state, TokenState::Invalid)
             && user.status != UserStatus::SpotifyTokenInvalid
         {
             UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?;
+            user.status = UserStatus::SpotifyTokenInvalid;

             // NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?;
if matches!(token_state, TokenState::Invalid)
&& user.status != UserStatus::SpotifyTokenInvalid
{
UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?;
// NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way
// with error_handler module
if let Err(err) = telegram::notify_token_invalid(self, &user).await {
tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token");
}
}
let (mut user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?;
if matches!(token_state, TokenState::Invalid)
&& user.status != UserStatus::SpotifyTokenInvalid
{
UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?;
user.status = UserStatus::SpotifyTokenInvalid;
// NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way
// with error_handler module
if let Err(err) = telegram::notify_token_invalid(self, &user).await {
tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token");
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app.rs` around lines 377 - 391, Update the invalid-token branch in the
flow around UserService::set_status to also set the in-memory user.status to
UserStatus::SpotifyTokenInvalid before constructing UserState, while preserving
the existing database update and notification behavior.

let state = UserState::new(user, newly_created, spotify);

Ok(state)
Expand Down
52 changes: 33 additions & 19 deletions src/spotify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::borrow::Cow;
use std::ops::Deref;
use std::sync::Arc;

use anyhow::{Context as _, anyhow};
use anyhow::Context as _;
use auth::SpotifyAuthService;
use chrono::{Duration, NaiveDate};
use deadpool_redis::redis::AsyncCommands as _;
Expand All @@ -24,11 +24,9 @@ use rspotify::model::{
TrackId,
};
use rspotify::{AuthCodeSpotify, ClientError, ClientResult, Token, scopes};
use sea_orm::{DbConn, TransactionTrait as _};
use sea_orm::DbConn;
use teloxide::utils::html;

use crate::entity::prelude::*;
use crate::services::UserService;
use crate::user::UserState;

pub struct ShortPlaylist {
Expand Down Expand Up @@ -300,6 +298,11 @@ impl From<ClientError> for CurrentlyPlaying {
}
}

pub enum TokenState {
Valid,
Invalid,
}

pub struct Manager {
spotify: AuthCodeSpotify,
}
Expand Down Expand Up @@ -354,7 +357,7 @@ impl Manager {
db: &DbConn,
user_id: &str,
instance: &AuthCodeSpotify,
) -> anyhow::Result<()> {
) -> anyhow::Result<TokenState> {
let should_reauth = instance
.get_token()
.lock()
Expand All @@ -364,21 +367,27 @@ impl Manager {
.is_some_and(Token::is_expired);

if !should_reauth {
return Ok(());
return Ok(TokenState::Valid);
}

let res = instance.refresh_token().await;

if !Self::is_token_valid(res).await? {
{
let txn = db.begin().await?;

UserService::set_status(&txn, user_id, UserStatus::SpotifyTokenInvalid).await?;

txn.commit().await?;
}

return Err(anyhow!("Token is invalid"));
// Keep the spotify_auth row in the database, but drop the in-memory
// token so the state behaves as unauthenticated and handlers prompt
// the user to /login instead of failing before any reply is sent
*instance
.get_token()
.lock()
.await
.expect("Cannot acquire lock") = None;

tracing::debug!(
%user_id,
"Spotify refresh token is invalid, continuing with unauthenticated client"
);

return Ok(TokenState::Invalid);
}

let token = instance
Expand All @@ -392,7 +401,7 @@ impl Manager {
SpotifyAuthService::set_token(db, user_id, token).await?;
}

Ok(())
Ok(TokenState::Valid)
}

async fn is_token_valid(mut res: ClientResult<()>) -> anyhow::Result<bool> {
Expand All @@ -410,7 +419,12 @@ impl Manager {
}
}

pub async fn for_user(&self, db: &DbConn, user_id: &str) -> anyhow::Result<AuthCodeSpotify> {
#[tracing::instrument(skip_all, fields(%user_id))]
pub async fn for_user(
&self,
db: &DbConn,
user_id: &str,
) -> anyhow::Result<(AuthCodeSpotify, TokenState)> {
let mut instance = self.spotify.clone();
instance.token = Arc::default();
let token = SpotifyAuthService::get_token(db, user_id).await?;
Expand All @@ -421,9 +435,9 @@ impl Manager {
.await
.expect("Cannot acquire lock") = token;

Self::token_refresh(db, user_id, &instance).await?;
let token_state = Self::token_refresh(db, user_id, &instance).await?;

Ok(instance)
Ok((instance, token_state))
}

pub async fn get_authorize_url(&self, state: &UserState) -> anyhow::Result<String> {
Expand Down
27 changes: 27 additions & 0 deletions src/telegram/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,31 @@ pub mod inline_buttons_admin;
pub mod keyboards;
pub mod utils;

use teloxide::payloads::SendMessageSetters as _;
use teloxide::prelude::Requester as _;
use teloxide::types::ChatId;

use crate::app::App;
use crate::entity::prelude::UserModel;
use crate::telegram::commands::UserCommandDisplay;
use crate::telegram::keyboards::StartKeyboard;

pub const MESSAGE_MAX_LEN: usize = 4096;

// TODO: Find a better place for this function
#[tracing::instrument(skip_all, fields(user_id = %user.id))]
pub async fn notify_token_invalid(app: &App, user: &UserModel) -> anyhow::Result<()> {
app.bot()
.send_message(
ChatId(user.id.parse()?),
t!(
"error.spotify-invalid-token",
locale = user.locale.as_ref(),
command = UserCommandDisplay::Login,
),
)
.reply_markup(StartKeyboard::markup(user.locale.as_ref()))
.await?;

Ok(())
}
4 changes: 4 additions & 0 deletions src/tick/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ pub async fn check(app: &'static App, user_id: &str) -> anyhow::Result<CheckUser
Ok(state) => state,
};

if !state.is_spotify_authed().await {
return Ok(CheckUserResult::Complete);
}

let playing = state.spotify().await.current_playing_wrapped().await;

let (track, context) = match playing {
Expand Down
Loading