Skip to content

Properly ignore devices, threepids and access tokens from AS users #4122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 4, 2025
Merged

Properly ignore devices, threepids and access tokens from AS users #4122

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2385d09
Actualy read appservice users, but don't insert them
sandhose Feb 27, 2025
1e99a6b
syn2mas: use NonNilUuid for MAS user IDs
sandhose Mar 4, 2025
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
69 changes: 39 additions & 30 deletions crates/syn2mas/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ bitflags::bitflags! {
const IS_SYNAPSE_ADMIN = 0b0000_0001;
const IS_DEACTIVATED = 0b0000_0010;
const IS_GUEST = 0b0000_0100;
const IS_APPSERVICE = 0b0000_1000;
}
}

Expand All @@ -89,6 +90,10 @@ impl UserFlags {
const fn is_synapse_admin(self) -> bool {
self.contains(UserFlags::IS_SYNAPSE_ADMIN)
}

const fn is_appservice(self) -> bool {
self.contains(UserFlags::IS_APPSERVICE)
}
}

#[derive(Debug, Clone, Copy)]
Expand Down Expand Up @@ -177,6 +182,20 @@ async fn migrate_users(
if bool::from(user.is_guest) {
flags |= UserFlags::IS_GUEST;
}
if user.appservice_id.is_some() {
flags |= UserFlags::IS_APPSERVICE;

// Special case for appservice users: we don't insert them into the database
// We just record the user's information in the state and continue
state.users.insert(
CompactString::new(&mas_user.username),
UserInfo {
mas_user_id: Uuid::nil(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this field an Option<NonNilUuid> (that would take the same space in memory) if we wanted to make sure that we never use a nil one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea, even if it does mean you will probably need some unwraps or so, it feels more 'direct' to explicitly unwrap than potentially let some accident happen and find out later, probably in a harder to debug way.
That said, the current approach is defensible and not a big deal but avoiding sentinel values has been generally a pretty productive idea for me, so I prefer it as a maintenance safety improvement.

flags,
},
);
continue;
}

state.users.insert(
CompactString::new(&mas_user.username),
Expand Down Expand Up @@ -233,15 +252,16 @@ async fn migrate_threepids(
.into_extract_localpart(synapse_user_id.clone())?
.to_owned();
let Some(user_infos) = state.users.get(username.as_str()).copied() else {
if is_likely_appservice(&username) {
continue;
}
return Err(Error::MissingUserFromDependentTable {
table: "user_threepids".to_owned(),
user: synapse_user_id,
});
};

if user_infos.flags.is_appservice() {
continue;
}

if medium == "email" {
email_buffer
.write(
Expand Down Expand Up @@ -311,15 +331,16 @@ async fn migrate_external_ids(
.into_extract_localpart(synapse_user_id.clone())?
.to_owned();
let Some(user_infos) = state.users.get(username.as_str()).copied() else {
if is_likely_appservice(&username) {
continue;
}
return Err(Error::MissingUserFromDependentTable {
table: "user_external_ids".to_owned(),
user: synapse_user_id,
});
};

if user_infos.flags.is_appservice() {
continue;
}

let Some(&upstream_provider_id) = state.provider_id_mapping.get(&auth_provider) else {
return Err(Error::MissingAuthProviderMapping {
synapse_id: auth_provider,
Expand Down Expand Up @@ -389,16 +410,16 @@ async fn migrate_devices(
.into_extract_localpart(synapse_user_id.clone())?
.to_owned();
let Some(user_infos) = state.users.get(username.as_str()).copied() else {
if is_likely_appservice(&username) {
continue;
}
return Err(Error::MissingUserFromDependentTable {
table: "devices".to_owned(),
user: synapse_user_id,
});
};

if user_infos.flags.is_deactivated() || user_infos.flags.is_guest() {
if user_infos.flags.is_deactivated()
|| user_infos.flags.is_guest()
|| user_infos.flags.is_appservice()
{
continue;
}

Expand Down Expand Up @@ -483,16 +504,16 @@ async fn migrate_unrefreshable_access_tokens(
.into_extract_localpart(synapse_user_id.clone())?
.to_owned();
let Some(user_infos) = state.users.get(username.as_str()).copied() else {
if is_likely_appservice(&username) {
continue;
}
return Err(Error::MissingUserFromDependentTable {
table: "access_tokens".to_owned(),
user: synapse_user_id,
});
};

if user_infos.flags.is_deactivated() || user_infos.flags.is_guest() {
if user_infos.flags.is_deactivated()
|| user_infos.flags.is_guest()
|| user_infos.flags.is_appservice()
{
continue;
}

Expand Down Expand Up @@ -595,16 +616,16 @@ async fn migrate_refreshable_token_pairs(
.into_extract_localpart(synapse_user_id.clone())?
.to_owned();
let Some(user_infos) = state.users.get(username.as_str()).copied() else {
if is_likely_appservice(&username) {
continue;
}
return Err(Error::MissingUserFromDependentTable {
table: "refresh_tokens".to_owned(),
user: synapse_user_id,
});
};

if user_infos.flags.is_deactivated() || user_infos.flags.is_guest() {
if user_infos.flags.is_deactivated()
|| user_infos.flags.is_guest()
|| user_infos.flags.is_appservice()
{
continue;
}

Expand Down Expand Up @@ -701,15 +722,3 @@ fn transform_user(

Ok((new_user, mas_password))
}

/// Returns true if and only if the given localpart looks like it would belong
/// to an application service user.
/// The rule here is that it must start with an underscore.
/// Synapse reserves these by default, but there is no hard rule prohibiting
/// other namespaces from being reserved, so this is not a robust check.
// TODO replace with a more robust mechanism, if we even care about this sanity check
// e.g. read application service registration files.
#[inline]
fn is_likely_appservice(localpart: &str) -> bool {
localpart.starts_with('_')
}
5 changes: 3 additions & 2 deletions crates/syn2mas/src/synapse_reader/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ pub struct SynapseUser {
/// account!
pub is_guest: SynapseBool,
// TODO do we care about upgrade_ts (users who upgraded from guest accounts to real accounts)
/// The ID of the appservice that created this user, if any.
pub appservice_id: Option<String>,
}

/// Row of the `user_threepids` table in Synapse.
Expand Down Expand Up @@ -369,9 +371,8 @@ impl<'conn> SynapseReader<'conn> {
sqlx::query_as(
"
SELECT
name, password_hash, admin, deactivated, creation_ts, is_guest
name, password_hash, admin, deactivated, creation_ts, is_guest, appservice_id
FROM users
WHERE appservice_id IS NULL
",
)
.fetch(&mut *self.txn)
Expand Down
1 change: 1 addition & 0 deletions crates/syn2mas/src/synapse_reader/snapshots/syn2mas__synapse_reader__test__read_users.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ expression: users
is_guest: SynapseBool(
false,
),
appservice_id: None,
},
}
Loading