letro/letro-authentication-service
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Allow compat session devices to have spaces

Browse Source
This commit is contained in:
Quentin Gliech
2025-02-19 17:55:18 +01:00
parent c779efef68
commit a3f22ae5f6
11 changed files with 132 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
crates/cli/src/commands/manage.rs
View File

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

66
crates/data-model/src/compat/device.rs
View File

@@ -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"

2
crates/data-model/src/compat/mod.rs
View File

@@ -12,7 +12,7 @@ mod session;
mod sso_login;
pub use self::{
device::Device,
device::{Device, ToScopeTokenError},
session::{CompatSession, CompatSessionState},
sso_login::{CompatSsoLogin, CompatSsoLoginState},
};

2
crates/data-model/src/lib.rs
View File

@@ -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,

4
crates/handlers/src/admin/model.rs
View File

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

13
crates/handlers/src/graphql/query/session.rs
View File

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

88
crates/handlers/src/oauth2/introspection.rs
View File

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

3
crates/oauth2-types/src/requests.rs
View File

@@ -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].

2
crates/storage-pg/src/app_session.rs
View File

@@ -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.

38
crates/storage-pg/src/compat/session.rs
View File

@@ -59,42 +59,28 @@ struct CompatSessionLookup {
last_active_ip: Option<IpAddr>,
}
impl TryFrom<CompatSessionLookup> for CompatSession {
type Error = DatabaseInconsistencyError;
fn try_from(value: CompatSessionLookup) -> Result<Self, Self::Error> {
impl From<CompatSessionLookup> 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<CompatSessionAndSsoLoginLookup> for (CompatSession, Option<CompatSs
fn try_from(value: CompatSessionAndSsoLoginLookup) -> Result<Self, Self::Error> {
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<CompatSessionAndSsoLoginLookup> for (CompatSession, Option<CompatSs
id,
state,
user_id: value.user_id.into(),
device,
device: value.device_id.map(Device::from),
human_name: value.human_name,
user_session_id: value.user_session_id.map(Ulid::from),
created_at: value.created_at,
@@ -310,7 +286,7 @@ impl CompatSessionRepository for PgCompatSessionRepository<'_> {
let Some(res) = res else { return Ok(None) };
Ok(Some(res.try_into()?))
Ok(Some(res.into()))
}
#[tracing::instrument(

13
crates/storage-pg/src/oauth2/session.rs
View File

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