diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 003841a35..da5cb595d 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -292,7 +292,7 @@ impl Options { .context("User not found")?; let device = if let Some(device_id) = device_id { - device_id.try_into()? + device_id.into() } else { Device::generate(&mut rng) }; diff --git a/crates/data-model/src/compat/device.rs b/crates/data-model/src/compat/device.rs index 85c6051d3..ca34ff2ac 100644 --- a/crates/data-model/src/compat/device.rs +++ b/crates/data-model/src/compat/device.rs @@ -22,21 +22,22 @@ pub struct Device { } #[derive(Debug, Error)] -pub enum InvalidDeviceID { - #[error("Device ID contains invalid characters")] +pub enum ToScopeTokenError { + #[error("Device ID contains characters that can't be encoded in a scope")] InvalidCharacters, } impl Device { /// Get the corresponding [`ScopeToken`] for that device - #[must_use] - pub fn to_scope_token(&self) -> ScopeToken { - // SAFETY: the inner id should only have valid scope characters - let Ok(scope_token) = format!("{DEVICE_SCOPE_PREFIX}{}", self.id).parse() else { - unreachable!() - }; - - scope_token + /// + /// # Errors + /// + /// Returns an error if the device ID contains characters that can't be + /// encoded in a scope + pub fn to_scope_token(&self) -> Result { + format!("{DEVICE_SCOPE_PREFIX}{}", self.id) + .parse() + .map_err(|_| ToScopeTokenError::InvalidCharacters) } /// Get the corresponding [`Device`] from a [`ScopeToken`] @@ -45,8 +46,7 @@ impl Device { #[must_use] pub fn from_scope_token(token: &ScopeToken) -> Option { let id = token.as_str().strip_prefix(DEVICE_SCOPE_PREFIX)?; - // XXX: we might be silently ignoring errors here, but it's probably fine? - Device::try_from(id.to_owned()).ok() + Some(Device::from(id.to_owned())) } /// Generate a random device ID @@ -62,39 +62,15 @@ impl Device { } } -const fn valid_device_chars(c: char) -> bool { - // This matches the regex in the policy - c.is_ascii_alphanumeric() - || c == '.' - || c == '_' - || c == '~' - || c == '!' - || c == '$' - || c == '&' - || c == '\'' - || c == '(' - || c == ')' - || c == '*' - || c == '+' - || c == ',' - || c == ';' - || c == '=' - || c == ':' - || c == '@' - || c == '/' - || c == '-' +impl From for Device { + fn from(id: String) -> Self { + Self { id } + } } -impl TryFrom for Device { - type Error = InvalidDeviceID; - - /// Create a [`Device`] out of an ID, validating the ID has the right shape - fn try_from(id: String) -> Result { - if !id.chars().all(valid_device_chars) { - return Err(InvalidDeviceID::InvalidCharacters); - } - - Ok(Self { id }) +impl From for String { + fn from(device: Device) -> Self { + device.id } } @@ -112,8 +88,8 @@ mod test { #[test] fn test_device_id_to_from_scope_token() { - let device = Device::try_from("AABBCCDDEE".to_owned()).unwrap(); - let scope_token = device.to_scope_token(); + let device = Device::from("AABBCCDDEE".to_owned()); + let scope_token = device.to_scope_token().unwrap(); assert_eq!( scope_token.as_str(), "urn:matrix:org.matrix.msc2967.client:device:AABBCCDDEE" diff --git a/crates/data-model/src/compat/mod.rs b/crates/data-model/src/compat/mod.rs index c3f7142d1..c50d74261 100644 --- a/crates/data-model/src/compat/mod.rs +++ b/crates/data-model/src/compat/mod.rs @@ -12,7 +12,7 @@ mod session; mod sso_login; pub use self::{ - device::Device, + device::{Device, ToScopeTokenError}, session::{CompatSession, CompatSessionState}, sso_login::{CompatSsoLogin, CompatSsoLoginState}, }; diff --git a/crates/data-model/src/lib.rs b/crates/data-model/src/lib.rs index b26f74f1b..46c04410c 100644 --- a/crates/data-model/src/lib.rs +++ b/crates/data-model/src/lib.rs @@ -26,7 +26,7 @@ pub use ulid::Ulid; pub use self::{ compat::{ CompatAccessToken, CompatRefreshToken, CompatRefreshTokenState, CompatSession, - CompatSessionState, CompatSsoLogin, CompatSsoLoginState, Device, + CompatSessionState, CompatSsoLogin, CompatSsoLoginState, Device, ToScopeTokenError, }, oauth2::{ AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Client, DeviceCodeGrant, diff --git a/crates/handlers/src/admin/model.rs b/crates/handlers/src/admin/model.rs index b98770379..467e6e835 100644 --- a/crates/handlers/src/admin/model.rs +++ b/crates/handlers/src/admin/model.rs @@ -229,7 +229,7 @@ impl CompatSession { Self { id: Ulid::from_bytes([0x01; 16]), user_id: Ulid::from_bytes([0x01; 16]), - device_id: Some("AABBCCDDEE".to_owned().try_into().unwrap()), + device_id: Some("AABBCCDDEE".to_owned().into()), user_session_id: Some(Ulid::from_bytes([0x11; 16])), redirect_uri: Some("https://example.com/redirect".parse().unwrap()), created_at: DateTime::default(), @@ -241,7 +241,7 @@ impl CompatSession { Self { id: Ulid::from_bytes([0x02; 16]), user_id: Ulid::from_bytes([0x01; 16]), - device_id: Some("FFGGHHIIJJ".to_owned().try_into().unwrap()), + device_id: Some("FFGGHHIIJJ".to_owned().into()), user_session_id: Some(Ulid::from_bytes([0x12; 16])), redirect_uri: None, created_at: DateTime::default(), diff --git a/crates/handlers/src/graphql/query/session.rs b/crates/handlers/src/graphql/query/session.rs index db1e7eed9..1115bed00 100644 --- a/crates/handlers/src/graphql/query/session.rs +++ b/crates/handlers/src/graphql/query/session.rs @@ -44,9 +44,7 @@ impl SessionQuery { return Ok(None); } - let Ok(device) = Device::try_from(device_id) else { - return Ok(None); - }; + let device = Device::from(device_id); let state = ctx.state(); let mut repo = state.repository().await?; @@ -81,7 +79,14 @@ impl SessionQuery { // Then, try to find an OAuth 2.0 session. Because we don't have any dedicated // device column, we're looking up using the device scope. - let scope = Scope::from_iter([device.to_scope_token()]); + // All device IDs can't necessarily be encoded as a scope. If it's not the case, + // we'll skip looking for OAuth 2.0 sessions. + let Ok(scope_token) = device.to_scope_token() else { + repo.cancel().await?; + + return Ok(None); + }; + let scope = Scope::from_iter([scope_token]); let filter = OAuth2SessionFilter::new() .for_user(&user) .active_only() diff --git a/crates/handlers/src/oauth2/introspection.rs b/crates/handlers/src/oauth2/introspection.rs index e44f28617..421e10cbb 100644 --- a/crates/handlers/src/oauth2/introspection.rs +++ b/crates/handlers/src/oauth2/introspection.rs @@ -4,8 +4,8 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. -use axum::{Json, extract::State, response::IntoResponse}; -use hyper::StatusCode; +use axum::{Json, extract::State, http::HeaderValue, response::IntoResponse}; +use hyper::{HeaderMap, StatusCode}; use mas_axum_utils::{ client_authorization::{ClientAuthorization, CredentialsVerificationError}, sentry::SentryEventID, @@ -74,6 +74,10 @@ pub enum RouteError { #[error("unknown compat session")] CantLoadCompatSession, + /// The Device ID in the compat session can't be encoded as a scope + #[error("device ID contains characters that are not allowed in a scope")] + CantEncodeDeviceID(#[from] mas_data_model::ToScopeTokenError), + #[error("invalid user")] InvalidUser, @@ -120,7 +124,8 @@ impl IntoResponse for RouteError { | Self::InvalidUser | Self::InvalidCompatSession | Self::InvalidOAuthSession - | Self::InvalidTokenFormat(_) => Json(INACTIVE).into_response(), + | Self::InvalidTokenFormat(_) + | Self::CantEncodeDeviceID(_) => Json(INACTIVE).into_response(), Self::NotAllowed => ( StatusCode::UNAUTHORIZED, Json(ClientError::from(ClientErrorCode::AccessDenied)), @@ -152,6 +157,7 @@ const INACTIVE: IntrospectionResponse = IntrospectionResponse { aud: None, iss: None, jti: None, + device_id: None, }; const API_SCOPE: ScopeToken = ScopeToken::from_static("urn:matrix:org.matrix.msc2967.client:api:*"); @@ -170,6 +176,7 @@ pub(crate) async fn post( mut repo: BoxRepository, activity_tracker: ActivityTracker, State(encrypter): State, + headers: HeaderMap, client_authorization: ClientAuthorization, ) -> Result { let client = client_authorization @@ -202,6 +209,16 @@ pub(crate) async fn post( } } + // Not all device IDs can be encoded as scope. On OAuth 2.0 sessions, we + // don't have this problem, as the device ID *is* already encoded as a scope. + // But on compatibility sessions, it's possible to have device IDs with + // spaces in them, or other weird characters. + // In those cases, we prefer explicitly giving out the device ID as a separate + // field. The client introspecting tells us whether it supports having the + // device ID as a separate field through this header. + let supports_explicit_device_id = + headers.get("X-MAS-Supports-Device-Id") == Some(&HeaderValue::from_static("1")); + // XXX: we should get the IP from the client introspecting the token let ip = None; @@ -270,6 +287,7 @@ pub(crate) async fn post( aud: None, iss: None, jti: Some(access_token.jti()), + device_id: None, } } @@ -329,6 +347,7 @@ pub(crate) async fn post( aud: None, iss: None, jti: Some(refresh_token.jti()), + device_id: None, } } @@ -365,7 +384,19 @@ pub(crate) async fn post( // Grant the synapse admin scope if the session has the admin flag set. let synapse_admin_scope_opt = session.is_synapse_admin.then_some(SYNAPSE_ADMIN_SCOPE); - let device_scope_opt = session.device.as_ref().map(Device::to_scope_token); + + // If the client supports explicitly giving the device ID in the response, skip + // encoding it in the scope + let device_scope_opt = if supports_explicit_device_id { + None + } else { + session + .device + .as_ref() + .map(Device::to_scope_token) + .transpose()? + }; + let scope = [API_SCOPE] .into_iter() .chain(device_scope_opt) @@ -389,6 +420,7 @@ pub(crate) async fn post( aud: None, iss: None, jti: None, + device_id: session.device.map(Device::into), } } @@ -425,7 +457,19 @@ pub(crate) async fn post( // Grant the synapse admin scope if the session has the admin flag set. let synapse_admin_scope_opt = session.is_synapse_admin.then_some(SYNAPSE_ADMIN_SCOPE); - let device_scope_opt = session.device.as_ref().map(Device::to_scope_token); + + // If the client supports explicitly giving the device ID in the response, skip + // encoding it in the scope + let device_scope_opt = if supports_explicit_device_id { + None + } else { + session + .device + .as_ref() + .map(Device::to_scope_token) + .transpose()? + }; + let scope = [API_SCOPE] .into_iter() .chain(device_scope_opt) @@ -449,6 +493,7 @@ pub(crate) async fn post( aud: None, iss: None, jti: None, + device_id: session.device.map(Device::into), } } }; @@ -777,10 +822,30 @@ mod tests { response.assert_status(StatusCode::OK); let response: IntrospectionResponse = response.json(); assert!(response.active); - assert_eq!(response.username, Some("alice".to_owned())); - assert_eq!(response.client_id, Some("legacy".to_owned())); + assert_eq!(response.username.as_deref(), Some("alice")); + assert_eq!(response.client_id.as_deref(), Some("legacy")); assert_eq!(response.token_type, Some(OAuthTokenTypeHint::AccessToken)); - assert_eq!(response.scope, Some(expected_scope.clone())); + assert_eq!(response.scope.as_ref(), Some(&expected_scope)); + assert_eq!(response.device_id.as_deref(), Some(device_id)); + + // Check that requesting with X-MAS-Supports-Device-Id removes the device ID + // from the scope but not from the explicit device_id field + let request = Request::post(OAuth2Introspection::PATH) + .basic_auth(&introspecting_client_id, &introspecting_client_secret) + .header("X-MAS-Supports-Device-Id", "1") + .form(json!({ "token": access_token })); + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + let response: IntrospectionResponse = response.json(); + assert!(response.active); + assert_eq!(response.username.as_deref(), Some("alice")); + assert_eq!(response.client_id.as_deref(), Some("legacy")); + assert_eq!(response.token_type, Some(OAuthTokenTypeHint::AccessToken)); + assert_eq!( + response.scope.map(|s| s.to_string()), + Some("urn:matrix:org.matrix.msc2967.client:api:*".to_owned()) + ); + assert_eq!(response.device_id.as_deref(), Some(device_id)); // Do the same request, but with a token_type_hint let request = Request::post(OAuth2Introspection::PATH) @@ -808,10 +873,11 @@ mod tests { response.assert_status(StatusCode::OK); let response: IntrospectionResponse = response.json(); assert!(response.active); - assert_eq!(response.username, Some("alice".to_owned())); - assert_eq!(response.client_id, Some("legacy".to_owned())); + assert_eq!(response.username.as_deref(), Some("alice")); + assert_eq!(response.client_id.as_deref(), Some("legacy")); assert_eq!(response.token_type, Some(OAuthTokenTypeHint::RefreshToken)); - assert_eq!(response.scope, Some(expected_scope.clone())); + assert_eq!(response.scope.as_ref(), Some(&expected_scope)); + assert_eq!(response.device_id.as_deref(), Some(device_id)); // Do the same request, but with a token_type_hint let request = Request::post(OAuth2Introspection::PATH) diff --git a/crates/oauth2-types/src/requests.rs b/crates/oauth2-types/src/requests.rs index 0201b5613..0b452ff72 100644 --- a/crates/oauth2-types/src/requests.rs +++ b/crates/oauth2-types/src/requests.rs @@ -786,6 +786,9 @@ pub struct IntrospectionResponse { /// String identifier for the token. pub jti: Option, + + /// MAS extension: explicit device ID + pub device_id: Option, } /// A request to the [Revocation Endpoint]. diff --git a/crates/storage-pg/src/app_session.rs b/crates/storage-pg/src/app_session.rs index f7b6a0ac6..ea97ec2d4 100644 --- a/crates/storage-pg/src/app_session.rs +++ b/crates/storage-pg/src/app_session.rs @@ -588,7 +588,7 @@ mod tests { .unwrap(); let device2 = Device::generate(&mut rng); - let scope = Scope::from_iter([OPENID, device2.to_scope_token()]); + let scope = Scope::from_iter([OPENID, device2.to_scope_token().unwrap()]); // We're moving the clock forward by 1 minute between each session to ensure // we're getting consistent ordering in lists. diff --git a/crates/storage-pg/src/compat/session.rs b/crates/storage-pg/src/compat/session.rs index e3bb48861..fe8c35b3c 100644 --- a/crates/storage-pg/src/compat/session.rs +++ b/crates/storage-pg/src/compat/session.rs @@ -59,42 +59,28 @@ struct CompatSessionLookup { last_active_ip: Option, } -impl TryFrom for CompatSession { - type Error = DatabaseInconsistencyError; - - fn try_from(value: CompatSessionLookup) -> Result { +impl From for CompatSession { + fn from(value: CompatSessionLookup) -> Self { let id = value.compat_session_id.into(); - let device = value - .device_id - .map(Device::try_from) - .transpose() - .map_err(|e| { - DatabaseInconsistencyError::on("compat_sessions") - .column("device_id") - .row(id) - .source(e) - })?; let state = match value.finished_at { None => CompatSessionState::Valid, Some(finished_at) => CompatSessionState::Finished { finished_at }, }; - let session = CompatSession { + CompatSession { id, state, user_id: value.user_id.into(), user_session_id: value.user_session_id.map(Ulid::from), - device, + device: value.device_id.map(Device::from), human_name: value.human_name, created_at: value.created_at, is_synapse_admin: value.is_synapse_admin, user_agent: value.user_agent.map(UserAgent::parse), last_active_at: value.last_active_at, last_active_ip: value.last_active_ip, - }; - - Ok(session) + } } } @@ -125,16 +111,6 @@ impl TryFrom for (CompatSession, Option Result { let id = value.compat_session_id.into(); - let device = value - .device_id - .map(Device::try_from) - .transpose() - .map_err(|e| { - DatabaseInconsistencyError::on("compat_sessions") - .column("device_id") - .row(id) - .source(e) - })?; let state = match value.finished_at { None => CompatSessionState::Valid, @@ -145,7 +121,7 @@ impl TryFrom for (CompatSession, Option { let Some(res) = res else { return Ok(None) }; - Ok(Some(res.try_into()?)) + Ok(Some(res.into())) } #[tracing::instrument( diff --git a/crates/storage-pg/src/oauth2/session.rs b/crates/storage-pg/src/oauth2/session.rs index 0517f6d98..6b753e17d 100644 --- a/crates/storage-pg/src/oauth2/session.rs +++ b/crates/storage-pg/src/oauth2/session.rs @@ -125,10 +125,15 @@ impl Filter for OAuth2SessionFilter<'_> { } })) .add_option(self.device().map(|device| { - Expr::val(device.to_scope_token().to_string()).eq(PgFunc::any(Expr::col(( - OAuth2Sessions::Table, - OAuth2Sessions::ScopeList, - )))) + if let Ok(scope_token) = device.to_scope_token() { + Expr::val(scope_token.to_string()).eq(PgFunc::any(Expr::col(( + OAuth2Sessions::Table, + OAuth2Sessions::ScopeList, + )))) + } else { + // If the device ID can't be encoded as a scope token, match no rows + Expr::val(false).into() + } })) .add_option(self.browser_session().map(|browser_session| { Expr::col((OAuth2Sessions::Table, OAuth2Sessions::UserSessionId))