fix: session refresh works as intended now (#5330)

* fix: session refresh works as intended now

* use code-defined defaults for expires and session_expires

* fix sqlx

* database migration drop defaults

* run fmt

* remove comment in migration

Signed-off-by: Xander <xander@isxander.dev>

---------

Signed-off-by: Xander <xander@isxander.dev>

apps/labrinth/.sqlx/query-1adbd24d815107e13bc1440c7a8f4eeff66ab4165a9f4980032e114db4dc1286.json

@@ -1,26 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "\n                SELECT\n                    id,\n                    status AS \"status: PayoutStatus\"\n                FROM payouts\n                ORDER BY id\n                ",
-  "describe": {
-    "columns": [
-      {
-        "ordinal": 0,
-        "name": "id",
-        "type_info": "Int8"
-      },
-      {
-        "ordinal": 1,
-        "name": "status: PayoutStatus",
-        "type_info": "Varchar"
-      }
-    ],
-    "parameters": {
-      "Left": []
-    },
-    "nullable": [
-      false,
-      false
-    ]
-  },
-  "hash": "1adbd24d815107e13bc1440c7a8f4eeff66ab4165a9f4980032e114db4dc1286"
-}

apps/labrinth/.sqlx/query-914f186afcc80dcbaed00a2cb16d398ba57e653c1a2d896201ccc39b2912cae2.json

@@ -1,6 +1,6 @@
 {
   "db_name": "PostgreSQL",
-  "query": "\n            INSERT INTO sessions (\n                id, session, user_id, os, platform,\n                city, country, ip, user_agent\n            )\n            VALUES (\n                $1, $2, $3, $4, $5,\n                $6, $7, $8, $9\n            )\n            ",
+  "query": "\n            INSERT INTO sessions (\n                id, session, user_id, os, platform,\n                city, country, ip, user_agent,\n                expires, refresh_expires\n            )\n            VALUES (\n                $1, $2, $3, $4, $5,\n                $6, $7, $8, $9,\n                $10, $11\n            )\n            ",
   "describe": {
     "columns": [],
     "parameters": {
@@ -13,10 +13,12 @@
         "Varchar",
         "Varchar",
         "Varchar",
-        "Varchar"
+        "Varchar",
+        "Timestamptz",
+        "Timestamptz"
       ]
     },
     "nullable": []
   },
-  "hash": "f1525930830e17b5ee8feb796d9950dd3741131965f050840fa75423b5a54f01"
+  "hash": "914f186afcc80dcbaed00a2cb16d398ba57e653c1a2d896201ccc39b2912cae2"
 }

apps/labrinth/.sqlx/query-b92b5bb7d179c4fcdbc45600ccfd2402f52fea71e27b08e7926fcc2a9e62c0f3.json

@@ -1,20 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "SELECT status AS \"status: PayoutStatus\" FROM payouts WHERE id = 1",
-  "describe": {
-    "columns": [
-      {
-        "ordinal": 0,
-        "name": "status: PayoutStatus",
-        "type_info": "Varchar"
-      }
-    ],
-    "parameters": {
-      "Left": []
-    },
-    "nullable": [
-      false
-    ]
-  },
-  "hash": "b92b5bb7d179c4fcdbc45600ccfd2402f52fea71e27b08e7926fcc2a9e62c0f3"
-}

apps/labrinth/.sqlx/query-cd5ccd618fb3cc41646a6de86f9afedb074492b4ec7f2457c14113f5fd13aa02.json

@@ -1,18 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "\n                INSERT INTO payouts (id, method, platform_id, status, user_id, amount, created)\n                VALUES ($1, $2, $3, $4, $5, 10.0, NOW())\n                ",
-  "describe": {
-    "columns": [],
-    "parameters": {
-      "Left": [
-        "Int8",
-        "Text",
-        "Text",
-        "Varchar",
-        "Int8"
-      ]
-    },
-    "nullable": []
-  },
-  "hash": "cd5ccd618fb3cc41646a6de86f9afedb074492b4ec7f2457c14113f5fd13aa02"
-}

apps/labrinth/.sqlx/query-cec4240c7c848988b3dfd13e3f8e5c93783c7641b019fdb698a1ec0be1393606.json

@@ -1,17 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "\n                    INSERT INTO payouts (id, method, platform_id, status, user_id, amount, created)\n                    VALUES ($1, $2, NULL, $3, $4, 10.00, NOW())\n                    ",
-  "describe": {
-    "columns": [],
-    "parameters": {
-      "Left": [
-        "Int8",
-        "Text",
-        "Varchar",
-        "Int8"
-      ]
-    },
-    "nullable": []
-  },
-  "hash": "cec4240c7c848988b3dfd13e3f8e5c93783c7641b019fdb698a1ec0be1393606"
-}

apps/labrinth/migrations/20260220124226_remove_session_expiry_defaults.sql

@@ -0,0 +1,2 @@
+ALTER TABLE sessions ALTER COLUMN expires DROP DEFAULT;
+ALTER TABLE sessions ALTER COLUMN refresh_expires DROP DEFAULT;

apps/labrinth/src/auth/validate.rs

@@ -32,6 +32,7 @@ where
         executor,
         redis,
         session_queue,
+        false,
     )
     .await?
     else {
@@ -61,6 +62,7 @@ where
         executor,
         redis,
         session_queue,
+        false,
     )
     .await?
     .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
@@ -95,12 +97,38 @@ where
     Ok((scopes, User::from_full(db_user)))
 }
 
+pub async fn get_user_from_bearer_token<'a, E>(
+    req: &HttpRequest,
+    token: Option<&str>,
+    executor: E,
+    redis: &RedisPool,
+    session_queue: &AuthQueue,
+    allow_expired: bool,
+) -> Result<(Scopes, User), AuthenticationError>
+where
+    E: crate::database::Executor<'a, Database = sqlx::Postgres> + Copy,
+{
+    let (scopes, db_user) = get_user_record_from_bearer_token(
+        req,
+        token,
+        executor,
+        redis,
+        session_queue,
+        allow_expired,
+    )
+    .await?
+    .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
+
+    Ok((scopes, User::from_full(db_user)))
+}
+
 pub async fn get_user_record_from_bearer_token<'a, 'b, E>(
     req: &HttpRequest,
     token: Option<&str>,
     executor: E,
     redis: &RedisPool,
     session_queue: &AuthQueue,
+    allow_expired: bool,
 ) -> Result<Option<(Scopes, user_item::DBUser)>, AuthenticationError>
 where
     E: crate::database::Executor<'a, Database = sqlx::Postgres> + Copy,
@@ -120,7 +148,7 @@ where
                 .await?
                 .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
 
-            if pat.expires < Utc::now() {
+            if !allow_expired && pat.expires < Utc::now() {
                 return Err(AuthenticationError::InvalidCredentials);
             }
 
@@ -139,7 +167,7 @@ where
                 .await?
                 .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
 
-            if session.expires < Utc::now() {
+            if !allow_expired && session.expires < Utc::now() {
                 return Err(AuthenticationError::InvalidCredentials);
             }
 
@@ -169,7 +197,7 @@ where
                     .await?
                     .ok_or(AuthenticationError::InvalidCredentials)?;
 
-            if access_token.expires < Utc::now() {
+            if !allow_expired && access_token.expires < Utc::now() {
                 return Err(AuthenticationError::InvalidCredentials);
             }
 

apps/labrinth/src/database/models/session_item.rs

@@ -25,6 +25,11 @@ pub struct SessionBuilder {
 
     pub ip: String,
     pub user_agent: String,
+
+    // When None, database default of 14 days will be used
+    pub expires: Option<DateTime<Utc>>,
+    // When None, database default of 60 days will be used
+    pub session_expires: Option<DateTime<Utc>>,
 }
 
 impl SessionBuilder {
@@ -38,11 +43,13 @@ impl SessionBuilder {
             "
             INSERT INTO sessions (
                 id, session, user_id, os, platform,
-                city, country, ip, user_agent
+                city, country, ip, user_agent,
+                expires, refresh_expires
             )
             VALUES (
                 $1, $2, $3, $4, $5,
-                $6, $7, $8, $9
+                $6, $7, $8, $9,
+                $10, $11
             )
             ",
             id as DBSessionId,
@@ -54,6 +61,10 @@ impl SessionBuilder {
             self.country,
             self.ip,
             self.user_agent,
+            self.expires
+                .unwrap_or_else(|| Utc::now() + chrono::Duration::days(14)),
+            self.session_expires
+                .unwrap_or_else(|| Utc::now() + chrono::Duration::days(60)),
         )
         .execute(&mut *transaction)
         .await?;

apps/labrinth/src/routes/internal/admin.rs

@@ -57,6 +57,7 @@ pub async fn count_download(
         &**pool,
         &redis,
         &session_queue,
+        false,
     )
     .await
     .ok()

apps/labrinth/src/routes/internal/flows.rs

@@ -1079,6 +1079,7 @@ pub async fn init(
             &**client,
             &redis,
             &session_queue,
+            false,
         )
         .await
         .ok()
@@ -1114,6 +1115,7 @@ pub async fn init(
             &**client,
             &redis,
             &session_queue,
+            false,
         )
         .await?
         .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
@@ -1308,7 +1310,8 @@ pub async fn auth_callback(
             };
 
             let session =
-                issue_session(req, user_id, &mut transaction, &redis).await?;
+                issue_session(req, user_id, &mut transaction, &redis, None)
+                    .await?;
             transaction.commit().await?;
 
             let redirect_url = format!(
@@ -1533,7 +1536,8 @@ pub async fn create_account_with_password(
     .insert(&mut transaction)
     .await?;
 
-    let session = issue_session(req, user_id, &mut transaction, &redis).await?;
+    let session =
+        issue_session(req, user_id, &mut transaction, &redis, None).await?;
     let res = crate::models::sessions::Session::from(session, true, None);
 
     let mailbox: Mailbox = new_account.email.parse().map_err(|_| {
@@ -1627,7 +1631,7 @@ pub async fn login_password(
     } else {
         let mut transaction = pool.begin().await?;
         let session =
-            issue_session(req, user.id, &mut transaction, &redis).await?;
+            issue_session(req, user.id, &mut transaction, &redis, None).await?;
         let res = crate::models::sessions::Session::from(session, true, None);
         transaction.commit().await?;
 
@@ -1757,7 +1761,7 @@ pub async fn login_2fa(
         DBFlow::remove(&login.flow, &redis).await?;
 
         let session =
-            issue_session(req, user_id, &mut transaction, &redis).await?;
+            issue_session(req, user_id, &mut transaction, &redis, None).await?;
         let res = crate::models::sessions::Session::from(session, true, None);
         transaction.commit().await?;
 
@@ -1945,6 +1949,7 @@ pub async fn remove_2fa(
         &**pool,
         &redis,
         &session_queue,
+        false,
     )
     .await?
     .ok_or_else(|| AuthenticationError::InvalidCredentials)?;
@@ -2150,6 +2155,7 @@ pub async fn change_password(
             &**pool,
             &redis,
             &session_queue,
+            false,
         )
         .await?
         .ok_or_else(|| AuthenticationError::InvalidCredentials)?;

apps/labrinth/src/routes/internal/session.rs

@@ -1,3 +1,4 @@
+use crate::auth::validate::get_user_from_bearer_token;
 use crate::auth::{AuthenticationError, get_user_from_headers};
 use crate::database::models::DBUserId;
 use crate::database::models::session_item::DBSession;
@@ -12,7 +13,7 @@ use crate::routes::ApiError;
 use actix_web::http::header::AUTHORIZATION;
 use actix_web::web::{Data, ServiceConfig, scope};
 use actix_web::{HttpRequest, HttpResponse, delete, get, post, web};
-use chrono::Utc;
+use chrono::{DateTime, Utc};
 use rand::distributions::Alphanumeric;
 use rand::{Rng, SeedableRng};
 use rand_chacha::ChaCha20Rng;
@@ -88,6 +89,7 @@ pub async fn issue_session(
     user_id: DBUserId,
     transaction: &mut PgTransaction<'_>,
     redis: &RedisPool,
+    session_expires: Option<DateTime<Utc>>,
 ) -> Result<DBSession, AuthenticationError> {
     let metadata = get_session_metadata(&req).await?;
 
@@ -108,6 +110,8 @@ pub async fn issue_session(
         country: metadata.country,
         ip: metadata.ip,
         user_agent: metadata.user_agent,
+        expires: None,
+        session_expires,
     }
     .insert(transaction)
     .await?;
@@ -212,15 +216,6 @@ pub async fn refresh(
     redis: Data<RedisPool>,
     session_queue: Data<AuthQueue>,
 ) -> Result<HttpResponse, ApiError> {
-    let current_user = get_user_from_headers(
-        &req,
-        &**pool,
-        &redis,
-        &session_queue,
-        Scopes::empty(),
-    )
-    .await?
-    .1;
     let session = req
         .headers()
         .get(AUTHORIZATION)
@@ -229,6 +224,25 @@ pub async fn refresh(
             ApiError::Authentication(AuthenticationError::InvalidCredentials)
         })?;
 
+    // We should ensure that the authorization given is a session token, and not some other type of token (like a PAT), since this endpoint is only for refreshing sessions.
+    // This is done by checking the prefix of the token, which should be "mra_" for session tokens.
+    if !session.starts_with("mra_") {
+        return Err(ApiError::Authentication(
+            AuthenticationError::InvalidCredentials,
+        ));
+    }
+
+    let current_user = get_user_from_bearer_token(
+        &req,
+        Some(session),
+        &**pool,
+        &redis,
+        &session_queue,
+        true, // Allow expired sessions, since we want to allow refreshing expired sessions
+    )
+    .await?
+    .1;
+
     let session = DBSession::get(session, &**pool, &redis).await?;
 
     if let Some(session) = session {
@@ -243,9 +257,14 @@ pub async fn refresh(
         let mut transaction = pool.begin().await?;
 
         DBSession::remove(session.id, &mut transaction).await?;
-        let new_session =
+        let new_session = issue_session(
-            issue_session(req, session.user_id, &mut transaction, &redis)
+            req,
-                .await?;
+            session.user_id,
+            &mut transaction,
+            &redis,
+            Some(session.refresh_expires),
+        )
+        .await?;
         transaction.commit().await?;
         DBSession::clear_cache(
             vec![(

apps/labrinth/src/routes/internal/statuses.rs

@@ -58,6 +58,7 @@ pub async fn ws_init(
         &**pool,
         &redis,
         &session_queue,
+        false,
     )
     .await?
     .ok_or_else(|| {

apps/labrinth/src/routes/v3/payouts.rs

@@ -440,6 +440,7 @@ pub async fn calculate_fees(
         &**pool,
         &redis,
         &session_queue,
+        false,
     )
     .await?
     .ok_or_else(|| {
@@ -472,6 +473,7 @@ pub async fn create_payout(
         &**pool,
         &redis,
         &session_queue,
+        false,
     )
     .await?
     .ok_or_else(|| {

packages/sqlx-tracing/tests/postgres.rs

@@ -46,13 +46,21 @@ impl PostgresContainer {
 
 #[tokio::test]
 async fn execute() {
-    let observability = opentelemetry_testing::ObservabilityContainer::create().await;
+    let observability =
+        opentelemetry_testing::ObservabilityContainer::create().await;
     let provider = observability.install().await;
 
     let container = PostgresContainer::create().await;
     let pool = container.client().await;
 
-    common::should_trace("trace_pool", "postgresql", &observability, &provider, &pool).await;
+    common::should_trace(
+        "trace_pool",
+        "postgresql",
+        &observability,
+        &provider,
+        &pool,
+    )
+    .await;
 
     {
         let mut conn = pool.acquire().await.unwrap();
@@ -67,7 +75,8 @@ async fn execute() {
     }
 
     {
-        let mut tx: sqlx_tracing::Transaction<'_, Postgres> = pool.begin().await.unwrap();
+        let mut tx: sqlx_tracing::Transaction<'_, Postgres> =
+            pool.begin().await.unwrap();
         common::should_trace(
             "trace_tx",
             "postgresql",

packages/sqlx-tracing/tests/sqlite.rs

@@ -6,21 +6,37 @@ mod common;
 
 #[tokio::test]
 async fn execute() {
-    let observability = opentelemetry_testing::ObservabilityContainer::create().await;
+    let observability =
+        opentelemetry_testing::ObservabilityContainer::create().await;
     let provider = observability.install().await;
 
     let pool = sqlx::SqlitePool::connect(":memory:").await.unwrap();
     let pool = sqlx_tracing::Pool::from(pool);
 
-    common::should_trace("trace_pool", "sqlite", &observability, &provider, &pool).await;
+    common::should_trace(
+        "trace_pool",
+        "sqlite",
+        &observability,
+        &provider,
+        &pool,
+    )
+    .await;
 
     {
         let mut conn = pool.acquire().await.unwrap();
-        common::should_trace("trace_conn", "sqlite", &observability, &provider, &mut conn).await;
+        common::should_trace(
+            "trace_conn",
+            "sqlite",
+            &observability,
+            &provider,
+            &mut conn,
+        )
+        .await;
     }
 
     {
-        let mut tx: sqlx_tracing::Transaction<'_, Sqlite> = pool.begin().await.unwrap();
+        let mut tx: sqlx_tracing::Transaction<'_, Sqlite> =
+            pool.begin().await.unwrap();
         common::should_trace(
             "trace_tx",
             "sqlite",