From 9290c46ea85070a71478c3c7b9fa433ba033a7bc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 13 May 2026 16:39:33 -0500 Subject: [PATCH 1/8] Add reasoning for why upsert device immediately when logging in From https://github.com/element-hq/matrix-authentication-service/pull/5607#discussion_r3232971115 --- crates/handlers/src/compat/login.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index e36e57a5b..7b13c6552 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -457,6 +457,12 @@ pub(crate) async fn post( // Now we can create the device on the homeserver, without holding the // transaction + // + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we want + // the device to be created synchronously on the homeserver, so that when we + // respond, the device already exists (TODO: Why important?). We're using an upsert + // so if the device already exists for some reason (like when we're replacing it, or + // a concurrent device sync happening) it won't have any effect. if let Err(err) = homeserver .upsert_device( &user.username, From 25f6b219901dd212e4f677cddcccc2e60be18ee2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 13 May 2026 16:54:44 -0500 Subject: [PATCH 2/8] Schedule `SyncDevicesJob` after `dangerous_hard_limit_eviction` --- crates/handlers/src/compat/login.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 7b13c6552..854b82279 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -508,6 +508,7 @@ pub(crate) async fn post( /// Given the violations from [`Policy::evaluate_compat_login`], return the /// appropriate `RouteError` response. async fn process_violations_for_compat_login( + rng: &mut (dyn RngCore + Send), clock: &dyn Clock, repo: &mut BoxRepository, session_limit_config: Option<&SessionLimitConfig>, @@ -602,6 +603,11 @@ async fn process_violations_for_compat_login( .finish(clock, compat_session.to_owned()) .await?; } + + // Schedule a device sync with the homeserver + repo.queue_job() + .schedule_job(rng, clock, SyncDevicesJob::new_for_id(user.id)) + .await?; } else { // Tell the user about the limit return Err(RouteError::PolicyHardSessionLimitReached); @@ -846,6 +852,7 @@ async fn token_login( }) .await?; process_violations_for_compat_login( + rng, clock, repo, session_limit_config, @@ -972,8 +979,15 @@ async fn user_password_login( requester: policy_requester, }) .await?; - process_violations_for_compat_login(clock, repo, session_limit_config, &user, res.violations) - .await?; + process_violations_for_compat_login( + rng, + clock, + repo, + session_limit_config, + &user, + res.violations, + ) + .await?; let session = repo .compat_session() From 09139721c071a3edf3cb24c4907346c7f53bee73 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 13 May 2026 17:10:02 -0500 Subject: [PATCH 3/8] Add logout reasoning --- crates/handlers/src/compat/logout.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/handlers/src/compat/logout.rs b/crates/handlers/src/compat/logout.rs index 4642cc54b..68f8f5d96 100644 --- a/crates/handlers/src/compat/logout.rs +++ b/crates/handlers/src/compat/logout.rs @@ -117,13 +117,19 @@ pub(crate) async fn post( // XXX: this is probably not the right error .ok_or(RouteError::InvalidAuthorization)?; + // This will make the access token invalid + repo.compat_session().finish(&clock, session).await?; + // Schedule a job to sync the devices of the user with the homeserver + // + // Doing this in a background job is ok as the access token will be invalid right + // away (from the session being finished above) and we do actually want to do a full + // device list sync, because we're not sure whether we want to delete the device (if + // there is for example a concurrent logout and login with the same device ID). repo.queue_job() .schedule_job(&mut rng, &clock, SyncDevicesJob::new(&user)) .await?; - repo.compat_session().finish(&clock, session).await?; - repo.save().await?; LOGOUT_COUNTER.add(1, &[KeyValue::new(RESULT, "success")]); From 5eadefa729bdb357ef077eee8e62b44346a88213 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 13 May 2026 17:15:38 -0500 Subject: [PATCH 4/8] Fix lints --- crates/handlers/src/compat/login.rs | 11 ++++++----- crates/handlers/src/compat/logout.rs | 9 +++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 854b82279..7a7d85f28 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -458,11 +458,12 @@ pub(crate) async fn post( // Now we can create the device on the homeserver, without holding the // transaction // - // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we want - // the device to be created synchronously on the homeserver, so that when we - // respond, the device already exists (TODO: Why important?). We're using an upsert - // so if the device already exists for some reason (like when we're replacing it, or - // a concurrent device sync happening) it won't have any effect. + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we + // want the device to be created synchronously on the homeserver, so that + // when we respond, the device already exists (TODO: Why important?). We're + // using an upsert so if the device already exists for some reason (like + // when we're replacing it, or a concurrent device sync happening) it won't + // have any effect. if let Err(err) = homeserver .upsert_device( &user.username, diff --git a/crates/handlers/src/compat/logout.rs b/crates/handlers/src/compat/logout.rs index 68f8f5d96..b73e3b268 100644 --- a/crates/handlers/src/compat/logout.rs +++ b/crates/handlers/src/compat/logout.rs @@ -122,10 +122,11 @@ pub(crate) async fn post( // Schedule a job to sync the devices of the user with the homeserver // - // Doing this in a background job is ok as the access token will be invalid right - // away (from the session being finished above) and we do actually want to do a full - // device list sync, because we're not sure whether we want to delete the device (if - // there is for example a concurrent logout and login with the same device ID). + // Doing this in a background job is ok as the access token will be invalid + // right away (from the session being finished above) and we do actually + // want to do a full device list sync, because we're not sure whether we + // want to delete the device (if there is for example a concurrent logout + // and login with the same device ID). repo.queue_job() .schedule_job(&mut rng, &clock, SyncDevicesJob::new(&user)) .await?; From f21e59b50d37a4c94eefabb87cc3689ff7f91538 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 13 May 2026 17:27:07 -0500 Subject: [PATCH 5/8] Explain as opposed to --- crates/handlers/src/compat/logout.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/handlers/src/compat/logout.rs b/crates/handlers/src/compat/logout.rs index b73e3b268..8800fcf44 100644 --- a/crates/handlers/src/compat/logout.rs +++ b/crates/handlers/src/compat/logout.rs @@ -124,9 +124,10 @@ pub(crate) async fn post( // // Doing this in a background job is ok as the access token will be invalid // right away (from the session being finished above) and we do actually - // want to do a full device list sync, because we're not sure whether we - // want to delete the device (if there is for example a concurrent logout - // and login with the same device ID). + // want to do a full device list sync (as opposed to + // `homeserver.delete_device(...)`), because we're not sure whether we want + // to delete the device (if there is for example a concurrent logout and + // login with the same device ID). repo.queue_job() .schedule_job(&mut rng, &clock, SyncDevicesJob::new(&user)) .await?; From 2aba54c4ba11c5aa5dc38213695f1c228c65d11f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 20 May 2026 11:47:18 -0500 Subject: [PATCH 6/8] Update reason for why synchronous device creation See https://github.com/element-hq/matrix-authentication-service/pull/5679#discussion_r3237767718 --- crates/handlers/src/compat/login.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 61eee7f4a..858b7afcd 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -458,12 +458,13 @@ pub(crate) async fn post( // Now we can create the device on the homeserver, without holding the // transaction // - // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we - // want the device to be created synchronously on the homeserver, so that - // when we respond, the device already exists (TODO: Why important?). We're - // using an upsert so if the device already exists for some reason (like - // when we're replacing it, or a concurrent device sync happening) it won't - // have any effect. + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we want + // the device to be created synchronously on the homeserver, so that when we + // respond, the device already exists. If the device doesn't exist on Synapse, token + // introspection from Synapse to MAS will work, but then the device won't exist, so + // Synapse will return a 401. We're using an upsert so if the device already exists + // for some reason (like when we're replacing it, or a concurrent device sync + // happening) it won't have any effect. if let Err(err) = homeserver .upsert_device( &user.username, From 7764e9e2961c4eefe892583f7b8276065ae626f5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 20 May 2026 11:48:37 -0500 Subject: [PATCH 7/8] Formatting --- crates/handlers/src/compat/login.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 858b7afcd..01ae4192f 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -458,13 +458,14 @@ pub(crate) async fn post( // Now we can create the device on the homeserver, without holding the // transaction // - // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we want - // the device to be created synchronously on the homeserver, so that when we - // respond, the device already exists. If the device doesn't exist on Synapse, token - // introspection from Synapse to MAS will work, but then the device won't exist, so - // Synapse will return a 401. We're using an upsert so if the device already exists - // for some reason (like when we're replacing it, or a concurrent device sync - // happening) it won't have any effect. + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we + // want the device to be created synchronously on the homeserver, so that + // when we respond, the device already exists. If the device doesn't exist + // on Synapse, token introspection from Synapse to MAS will work, but then + // the device won't exist, so Synapse will return a 401. We're using an + // upsert so if the device already exists for some reason (like when we're + // replacing it, or a concurrent device sync happening) it won't have any + // effect. if let Err(err) = homeserver .upsert_device( &user.username, From b73e06aded47c63c3baf88f52f66c53ddd1f6608 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 20 May 2026 11:58:36 -0500 Subject: [PATCH 8/8] Update comment langauge and add it to other login spots --- crates/handlers/src/compat/login.rs | 14 ++++++++------ crates/handlers/src/oauth2/token.rs | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 01ae4192f..37bcaa0c6 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -460,12 +460,14 @@ pub(crate) async fn post( // // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we // want the device to be created synchronously on the homeserver, so that - // when we respond, the device already exists. If the device doesn't exist - // on Synapse, token introspection from Synapse to MAS will work, but then - // the device won't exist, so Synapse will return a 401. We're using an - // upsert so if the device already exists for some reason (like when we're - // replacing it, or a concurrent device sync happening) it won't have any - // effect. + // when we respond, the access token works completely. If the device doesn't + // exist on the homeserver side, token introspection from Synapse to MAS + // will work but Synapse will return a 401 because it doesn't see the + // device. + // + // We're using an upsert so if the device already exists for some reason (like + // when we're replacing it, or a concurrent device sync happening) it won't + // have any effect. if let Err(err) = homeserver .upsert_device( &user.username, diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index e070b640b..925f4209d 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -573,6 +573,15 @@ async fn authorization_code_grant( // Look for device to provision for scope in &*session.scope { if let Some(device) = Device::from_scope_token(scope) { + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we + // want the device to be created synchronously on the homeserver, so + // that when we respond, the access token works completely. If the + // device doesn't exist on the homeserver side, token introspection + // from Synapse to MAS will work but Synapse will return a 401 + // because it doesn't see the device. + // + // We're using an upsert so if the device already exists for some reason + // (like when a concurrent device sync happening) it won't have any effect. homeserver .upsert_device( &browser_session.user.username, @@ -977,6 +986,15 @@ async fn device_code_grant( // Look for device to provision for scope in &*session.scope { if let Some(device) = Device::from_scope_token(scope) { + // Normally, devices get synced to the homeserver in a `SyncDevicesJob` but we + // want the device to be created synchronously on the homeserver, so + // that when we respond, the access token works completely. If the + // device doesn't exist on the homeserver side, token introspection + // from Synapse to MAS will work but Synapse will return a 401 + // because it doesn't see the device. + // + // We're using an upsert so if the device already exists for some reason + // (like when a concurrent device sync happening) it won't have any effect. homeserver .upsert_device(&browser_session.user.username, device.as_str(), None) .await