Allow compat session devices to have spaces (#4067)

Author: sandhose

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)
                 };

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<ScopeToken, ToScopeTokenError> {
+        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<Self> {
         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<String> for Device {
+    fn from(id: String) -> Self {
+        Self { id }
+    }
 }
 
-impl TryFrom<String> 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<Self, Self::Error> {
-        if !id.chars().all(valid_device_chars) {
-            return Err(InvalidDeviceID::InvalidCharacters);
-        }
-
-        Ok(Self { id })
+impl From<Device> 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"

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},
 };

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,

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(),

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()

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<Encrypter>,
+    headers: HeaderMap,
     client_authorization: ClientAuthorization<IntrospectionRequest>,
 ) -> Result<impl IntoResponse, RouteError> {
     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)

crates/oauth2-types/src/requests.rs

@@ -786,6 +786,9 @@ pub struct IntrospectionResponse {
 
     /// String identifier for the token.
     pub jti: Option<String>,
+
+    /// MAS extension: explicit device ID
+    pub device_id: Option<String>,
 }
 
 /// A request to the [Revocation Endpoint].

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.