diff --git a/migrations/mysql/2026-05-05-120000_sso_auth_error/down.sql b/migrations/mysql/2026-05-05-120000_sso_auth_error/down.sql new file mode 100644 index 00000000..06974e60 --- /dev/null +++ b/migrations/mysql/2026-05-05-120000_sso_auth_error/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE sso_auth DROP COLUMN code_response_error; +DROP INDEX code_response_index; +ALTER TABLE sso_auth MODIFY COLUMN code_response TEXT; diff --git a/migrations/mysql/2026-05-05-120000_sso_auth_error/up.sql b/migrations/mysql/2026-05-05-120000_sso_auth_error/up.sql new file mode 100644 index 00000000..226e056c --- /dev/null +++ b/migrations/mysql/2026-05-05-120000_sso_auth_error/up.sql @@ -0,0 +1,4 @@ +DELETE FROM sso_auth; +ALTER TABLE sso_auth ADD COLUMN code_response_error TEXT; +ALTER TABLE sso_auth MODIFY COLUMN code_response VARCHAR(768); +CREATE INDEX code_response_index ON sso_auth(code_response); diff --git a/migrations/postgresql/2026-05-05-120000_sso_auth_error/down.sql b/migrations/postgresql/2026-05-05-120000_sso_auth_error/down.sql new file mode 100644 index 00000000..cb3b9731 --- /dev/null +++ b/migrations/postgresql/2026-05-05-120000_sso_auth_error/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE sso_auth DROP COLUMN IF EXISTS code_response_error; +DROP INDEX IF EXISTS code_response_index; diff --git a/migrations/postgresql/2026-05-05-120000_sso_auth_error/up.sql b/migrations/postgresql/2026-05-05-120000_sso_auth_error/up.sql new file mode 100644 index 00000000..e18c7454 --- /dev/null +++ b/migrations/postgresql/2026-05-05-120000_sso_auth_error/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE sso_auth ADD COLUMN IF NOT EXISTS code_response_error TEXT; +CREATE INDEX IF NOT EXISTS code_response_index ON sso_auth(code_response); diff --git a/migrations/sqlite/2026-05-05-120000_sso_auth_error/down.sql b/migrations/sqlite/2026-05-05-120000_sso_auth_error/down.sql new file mode 100644 index 00000000..289abe91 --- /dev/null +++ b/migrations/sqlite/2026-05-05-120000_sso_auth_error/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE sso_auth DROP COLUMN code_response_error; +DROP INDEX IF EXISTS code_response_index; diff --git a/migrations/sqlite/2026-05-05-120000_sso_auth_error/up.sql b/migrations/sqlite/2026-05-05-120000_sso_auth_error/up.sql new file mode 100644 index 00000000..bb49f11b --- /dev/null +++ b/migrations/sqlite/2026-05-05-120000_sso_auth_error/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE sso_auth ADD COLUMN code_response_error TEXT; +CREATE INDEX code_response_index ON sso_auth(code_response); diff --git a/src/api/identity.rs b/src/api/identity.rs index 569deaf9..99c27aa8 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -28,8 +28,9 @@ use crate::{ crypto, db::{ models::{ - AuthRequest, AuthRequestId, Device, DeviceId, EventType, Invitation, OIDCCodeWrapper, OrganizationApiKey, - OrganizationId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + AuthRequest, AuthRequestId, Device, DeviceId, EventType, Invitation, OIDCCodeResponseError, + OrganizationApiKey, OrganizationId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, + UserId, }, DbConn, }, @@ -186,7 +187,7 @@ async fn _sso_login( // Ratelimit the login crate::ratelimit::check_limit_login(&ip.ip)?; - let (state, code_verifier) = match (data.code.as_ref(), data.code_verifier.as_ref()) { + let (code, code_verifier) = match (data.code.as_ref(), data.code_verifier.as_ref()) { (None, _) => err!( "Got no code in OIDC data", ErrorEvent { @@ -202,7 +203,7 @@ async fn _sso_login( (Some(code), Some(code_verifier)) => (code, code_verifier.clone()), }; - let (sso_auth, user_infos) = sso::exchange_code(state, code_verifier, conn).await?; + let (sso_auth, user_infos) = sso::exchange_code(code, code_verifier, conn).await?; let user_with_sso = match SsoUser::find_by_identifier(&user_infos.identifier, conn).await { None => match SsoUser::find_by_mail(&user_infos.email, conn).await { None => None, @@ -1138,7 +1139,7 @@ struct ConnectData { // Needed for authorization code #[field(name = uncased("code"))] - code: Option, + code: Option, #[field(name = uncased("code_verifier"))] code_verifier: Option, } @@ -1165,19 +1166,12 @@ const SSO_BINDING_COOKIE: &str = "VW_SSO_BINDING"; #[get("/connect/oidc-signin?&", rank = 1)] async fn oidcsignin(code: OIDCCode, state: String, cookies: &CookieJar<'_>, mut conn: DbConn) -> ApiResult { - _oidcsignin_redirect( - state, - OIDCCodeWrapper::Ok { - code, - }, - cookies, - &mut conn, - ) - .await + _oidcsignin_redirect(state, code, None, cookies, &mut conn).await } -// Bitwarden client appear to only care for code and state so we pipe it through -// cf: https://github.com/bitwarden/clients/blob/80b74b3300e15b4ae414dc06044cc9b02b6c10a6/libs/auth/src/angular/sso/sso.component.ts#L141 +// Bitwarden client appear to only care for code and state +// We save the error in the database and set the encoded state as the code to be able to retrieve them later on +// cf: https://github.com/bitwarden/clients/blob/afd36d290ce18fb0048e0575e7d5a8f78b5dbffc/libs/auth/src/angular/sso/sso.component.ts#L156 #[get("/connect/oidc-signin?&&", rank = 2)] async fn oidcsignin_error( state: String, @@ -1187,11 +1181,12 @@ async fn oidcsignin_error( mut conn: DbConn, ) -> ApiResult { _oidcsignin_redirect( - state, - OIDCCodeWrapper::Error { + state.clone(), + state.into(), + Some(OIDCCodeResponseError { error, error_description, - }, + }), cookies, &mut conn, ) @@ -1200,10 +1195,10 @@ async fn oidcsignin_error( // The state was encoded using Base64 to ensure no issue with providers. // iss and scope parameters are needed for redirection to work on IOS. -// We pass the state as the code to get it back later on. async fn _oidcsignin_redirect( base64_state: String, - code_response: OIDCCodeWrapper, + code: OIDCCode, + error: Option, cookies: &CookieJar<'_>, conn: &mut DbConn, ) -> ApiResult { @@ -1224,7 +1219,8 @@ async fn _oidcsignin_redirect( } cookies.remove(Cookie::build(SSO_BINDING_COOKIE).path("/identity/connect/").build()); - sso_auth.code_response = Some(code_response); + sso_auth.code_response = Some(code.clone()); + sso_auth.code_response_error = error; sso_auth.updated_at = Utc::now().naive_utc(); sso_auth.save(conn).await?; @@ -1234,7 +1230,7 @@ async fn _oidcsignin_redirect( }; url.query_pairs_mut() - .append_pair("code", &state) + .append_pair("code", &code) .append_pair("state", &state) .append_pair("scope", &AuthMethod::Sso.scope()) .append_pair("iss", &CONFIG.domain()); diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index 2d31259c..7cc81852 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -38,7 +38,7 @@ pub use self::send::{ id::{SendFileId, SendId}, Send, SendType, }; -pub use self::sso_auth::{OIDCAuthenticatedUser, OIDCCodeWrapper, SsoAuth}; +pub use self::sso_auth::{OIDCAuthenticatedUser, OIDCCodeResponseError, SsoAuth}; pub use self::two_factor::{TwoFactor, TwoFactorType}; pub use self::two_factor_duo_context::TwoFactorDuoContext; pub use self::two_factor_incomplete::TwoFactorIncomplete; diff --git a/src/db/models/sso_auth.rs b/src/db/models/sso_auth.rs index 2c6eec6d..3ecaf9f7 100644 --- a/src/db/models/sso_auth.rs +++ b/src/db/models/sso_auth.rs @@ -15,17 +15,12 @@ use diesel::sql_types::Text; #[derive(AsExpression, Clone, Debug, Serialize, Deserialize, FromSqlRow)] #[diesel(sql_type = Text)] -pub enum OIDCCodeWrapper { - Ok { - code: OIDCCode, - }, - Error { - error: String, - error_description: Option, - }, +pub struct OIDCCodeResponseError { + pub error: String, + pub error_description: Option, } -impl_FromToSqlText!(OIDCCodeWrapper); +impl_FromToSqlText!(OIDCCodeResponseError); #[derive(AsExpression, Clone, Debug, Serialize, Deserialize, FromSqlRow)] #[diesel(sql_type = Text)] @@ -50,7 +45,8 @@ pub struct SsoAuth { pub client_challenge: OIDCCodeChallenge, pub nonce: String, pub redirect_uri: String, - pub code_response: Option, + pub code_response: Option, + pub code_response_error: Option, pub auth_response: Option, pub created_at: NaiveDateTime, pub updated_at: NaiveDateTime, @@ -76,6 +72,7 @@ impl SsoAuth { created_at: now, updated_at: now, code_response: None, + code_response_error: None, auth_response: None, binding_hash, } @@ -118,6 +115,17 @@ impl SsoAuth { }} } + pub async fn find_by_code(code: &OIDCCode, conn: &DbConn) -> Option { + let oldest = Utc::now().naive_utc() - *SSO_AUTH_EXPIRATION; + db_run! { conn: { + sso_auth::table + .filter(sso_auth::code_response.eq(code)) + .filter(sso_auth::created_at.ge(oldest)) + .first::(conn) + .ok() + }} + } + pub async fn delete(self, conn: &DbConn) -> EmptyResult { db_run! {conn: { diesel::delete(sso_auth::table.filter(sso_auth::state.eq(self.state))) diff --git a/src/db/schema.rs b/src/db/schema.rs index bf79ceac..af342186 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -262,6 +262,7 @@ table! { nonce -> Text, redirect_uri -> Text, code_response -> Nullable, + code_response_error -> Nullable, auth_response -> Nullable, created_at -> Timestamp, updated_at -> Timestamp, diff --git a/src/sso.rs b/src/sso.rs index 7505f84f..56e9a534 100644 --- a/src/sso.rs +++ b/src/sso.rs @@ -10,7 +10,7 @@ use crate::{ auth, auth::{AuthMethod, AuthTokens, TokenWrapper, BW_EXPIRATION, DEFAULT_REFRESH_VALIDITY}, db::{ - models::{Device, OIDCAuthenticatedUser, OIDCCodeWrapper, SsoAuth, SsoUser, User}, + models::{Device, OIDCAuthenticatedUser, SsoAuth, SsoUser, User}, DbConn, }, sso_client::Client, @@ -240,14 +240,14 @@ impl OIDCIdentifier { // - second time we will rely on `SsoAuth.auth_response` since the `code` has already been exchanged. // The `SsoAuth` will ensure that the user is authorized only once. pub async fn exchange_code( - state: &OIDCState, + code: &OIDCCode, client_verifier: OIDCCodeVerifier, conn: &DbConn, ) -> ApiResult<(SsoAuth, OIDCAuthenticatedUser)> { use openidconnect::OAuth2TokenResponse; - let mut sso_auth = match SsoAuth::find(state, conn).await { - None => err!(format!("Invalid state cannot retrieve sso auth")), + let mut sso_auth = match SsoAuth::find_by_code(code, conn).await { + None => err!(format!("Invalid code cannot retrieve sso auth")), Some(sso_auth) => sso_auth, }; @@ -255,18 +255,18 @@ pub async fn exchange_code( return Ok((sso_auth, authenticated_user)); } - let code = match sso_auth.code_response.clone() { - Some(OIDCCodeWrapper::Ok { - code, - }) => code.clone(), - Some(OIDCCodeWrapper::Error { - error, - error_description, - }) => { + let code = match (sso_auth.code_response.clone(), sso_auth.code_response_error.as_ref()) { + (Some(code), None) => code, + (_, Some(re)) => { + let error_msg = format!( + "SSO authorization failed: {}, {}", + re.error, + re.error_description.as_ref().unwrap_or(&String::new()) + ); sso_auth.delete(conn).await?; - err!(format!("SSO authorization failed: {error}, {}", error_description.as_ref().unwrap_or(&String::new()))) + err!(error_msg); } - None => { + (None, _) => { sso_auth.delete(conn).await?; err!("Missing authorization provider return"); }