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
34 changes: 31 additions & 3 deletions apps/labrinth/src/auth/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ where
executor,
redis,
session_queue,
false,
)
.await?
else {
Expand Down Expand Up @@ -60,6 +61,7 @@ where
executor,
redis,
session_queue,
false,
)
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;
Expand Down Expand Up @@ -94,12 +96,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,
Expand All @@ -119,7 +147,7 @@ where
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;

if pat.expires < Utc::now() {
if !allow_expired && pat.expires < Utc::now() {
return Err(AuthenticationError::InvalidCredentials);
}

Expand All @@ -138,7 +166,7 @@ where
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;

if session.expires < Utc::now() {
if !allow_expired && session.expires < Utc::now() {
return Err(AuthenticationError::InvalidCredentials);
}

Expand Down Expand Up @@ -168,7 +196,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);
}

Expand Down
52 changes: 52 additions & 0 deletions apps/labrinth/src/database/models/session_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -58,6 +63,53 @@ impl SessionBuilder {
.execute(&mut *transaction)
.await?;

// If we put these Option values into the first query, and they are None,
// it will enter into the DB as NULL, when the desired behavior is database
// default.
match (self.expires, self.session_expires) {
(None, None) => {}
(Some(expires), Some(session_expires)) => {
sqlx::query!(
"
UPDATE sessions
SET expires = $1, refresh_expires = $2
WHERE id = $3
",
expires,
session_expires,
id as DBSessionId,
)
.execute(&mut *transaction)
.await?;
}
(Some(expires), None) => {
sqlx::query!(
"
UPDATE sessions
SET expires = $1
WHERE id = $2
",
expires,
id as DBSessionId,
)
.execute(&mut *transaction)
.await?;
}
(None, Some(session_expires)) => {
sqlx::query!(
"
UPDATE sessions
SET refresh_expires = $1
WHERE id = $2
",
session_expires,
id as DBSessionId,
)
.execute(&mut *transaction)
.await?;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would try to remove the UPDATE statements and create the session with just a single INSERT to avoid table bloat - can remove the database default & just define the default in code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting mixed signals here!! @aecsocket said to leave the db defaults?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion here - we discussed this in further detail, and came to the conclusion that the default shouldn't be specified in the DB, but in the code itself, since it's more important to combine the INSERT and UPDATE here.

My thought here is that SessionBuilder's insert and update operations should default to Utc::now() + (14 days) for expires, and + 60 days for refresh expires, like the DB already does, but in the Rust logic rather than in the DB. And that SessionBuilder will never omit inserting an expires or refresh_expires value - it will always be set to a value, or the default if not explicitly set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add a database migration to remove the defaults from the table on the db-level as to not cause unexpected behaviour or hide bugs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Ok(id)
}
}
Expand Down
1 change: 1 addition & 0 deletions apps/labrinth/src/routes/internal/admin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ pub async fn count_download(
&**pool,
&redis,
&session_queue,
false,
)
.await
.ok()
Expand Down
14 changes: 10 additions & 4 deletions apps/labrinth/src/routes/internal/flows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,6 +1082,7 @@ pub async fn init(
&**client,
&redis,
&session_queue,
false,
)
.await
.ok()
Expand Down Expand Up @@ -1116,6 +1117,7 @@ pub async fn init(
&**client,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;
Expand Down Expand Up @@ -1310,7 +1312,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!(
Expand Down Expand Up @@ -1535,7 +1538,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(|_| {
Expand Down Expand Up @@ -1629,7 +1633,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?;

Expand Down Expand Up @@ -1759,7 +1763,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?;

Expand Down Expand Up @@ -1947,6 +1951,7 @@ pub async fn remove_2fa(
&**pool,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;
Expand Down Expand Up @@ -2152,6 +2157,7 @@ pub async fn change_password(
&**pool,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| AuthenticationError::InvalidCredentials)?;
Expand Down
45 changes: 32 additions & 13 deletions apps/labrinth/src/routes/internal/session.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -12,7 +13,7 @@ use crate::util::env::parse_var;
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;
Expand Down Expand Up @@ -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?;

Expand All @@ -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?;
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -243,9 +257,14 @@ pub async fn refresh(
let mut transaction = pool.begin().await?;

DBSession::remove(session.id, &mut transaction).await?;
let new_session =
issue_session(req, session.user_id, &mut transaction, &redis)
.await?;
let new_session = issue_session(
req,
session.user_id,
&mut transaction,
&redis,
Some(session.refresh_expires),
)
.await?;
transaction.commit().await?;
DBSession::clear_cache(
vec![(
Expand Down
1 change: 1 addition & 0 deletions apps/labrinth/src/routes/internal/statuses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ pub async fn ws_init(
&**pool,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| {
Expand Down
2 changes: 2 additions & 0 deletions apps/labrinth/src/routes/v3/payouts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ pub async fn calculate_fees(
&**pool,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| {
Expand Down Expand Up @@ -471,6 +472,7 @@ pub async fn create_payout(
&**pool,
&redis,
&session_queue,
false,
)
.await?
.ok_or_else(|| {
Expand Down
Loading