Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion crates/cli/src/commands/manage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,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)
};
Expand Down
66 changes: 21 additions & 45 deletions crates/data-model/src/compat/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`]
Expand All @@ -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
Expand All @@ -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
}
}

Expand All @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion crates/data-model/src/compat/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ mod session;
mod sso_login;

pub use self::{
device::Device,
device::{Device, ToScopeTokenError},
session::{CompatSession, CompatSessionState},
sso_login::{CompatSsoLogin, CompatSsoLoginState},
};
Expand Down
2 changes: 1 addition & 1 deletion crates/data-model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions crates/handlers/src/admin/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down
13 changes: 9 additions & 4 deletions crates/handlers/src/graphql/query/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;

Expand Down Expand Up @@ -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()
Expand Down
88 changes: 77 additions & 11 deletions crates/handlers/src/oauth2/introspection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
// SPDX-License-Identifier: AGPL-3.0-only
// Please see LICENSE in the repository root for full details.

use axum::{extract::State, response::IntoResponse, Json};
use hyper::StatusCode;
use axum::{extract::State, http::HeaderValue, response::IntoResponse, Json};
use hyper::{HeaderMap, StatusCode};
use mas_axum_utils::{
client_authorization::{ClientAuthorization, CredentialsVerificationError},
sentry::SentryEventID,
Expand Down Expand Up @@ -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,

Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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:*");
Expand All @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -270,6 +287,7 @@ pub(crate) async fn post(
aud: None,
iss: None,
jti: Some(access_token.jti()),
device_id: None,
}
}

Expand Down Expand Up @@ -329,6 +347,7 @@ pub(crate) async fn post(
aud: None,
iss: None,
jti: Some(refresh_token.jti()),
device_id: None,
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -389,6 +420,7 @@ pub(crate) async fn post(
aud: None,
iss: None,
jti: None,
device_id: session.device.map(Device::into),
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -449,6 +493,7 @@ pub(crate) async fn post(
aud: None,
iss: None,
jti: None,
device_id: session.device.map(Device::into),
}
}
};
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions crates/oauth2-types/src/requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand Down
2 changes: 1 addition & 1 deletion crates/storage-pg/src/app_session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading