-
Notifications
You must be signed in to change notification settings - Fork 58
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.
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
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
1e99a6b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://921d1ba7.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://quenting-syn2mas-ignore-as-u.matrix-authentication-service-docs.pages.dev |
crates/syn2mas/src/migration.rs
Outdated
| state.users.insert( | ||
| CompactString::new(&mas_user.username), | ||
| UserInfo { | ||
| mas_user_id: Uuid::nil(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
crates/syn2mas/src/migration.rs
Outdated
| state.users.insert( | ||
| CompactString::new(&mas_user.username), | ||
| UserInfo { | ||
| mas_user_id: Uuid::nil(), |
There was a problem hiding this comment.
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.
|
@reivilibre I did the thing of using NonNilUuids for MAS user IDs in 1e99a6b. wdyt? Should I extend that to all MAS ULIDs? |
This reads the AS users from the Synapse database, doesn't import them, but keep them in memory, so that we can properly ignore threepids, external ids, devices and access tokens belonging to them.