syn2mas: migrate access tokens, refresh tokens and devices #3926
Conversation
reivilibre
force-pushed
the
rei/syn2mas_8_atrtdevs
branch
from
3cf7796 to
4db2158
Compare
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
7181cc8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41221c0f.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://rei-syn2mas-8-atrtdevs.matrix-authentication-service-docs.pages.dev |
reivilibre
force-pushed
the
rei/syn2mas_8_atrtdevs
branch
from
4db2158 to
d0e9ea2
Compare
| migrator = "MIGRATOR", | ||
| fixtures("user_alice", "access_token_alice_with_refresh_token") | ||
| )] | ||
| async fn test_read_access_and_refresh_tokens(pool: PgPool) { |
There was a problem hiding this comment.
it would be nice to have test for the annoying edge cases of like a token which was refreshed but not yet used, etc.
| @@ -204,9 +204,9 @@ impl CompatRefreshTokenRepository for PgCompatRefreshTokenRepository<'_> { | |||
| r#" | |||
| UPDATE compat_refresh_tokens | |||
| SET consumed_at = $2 | |||
| WHERE compat_refresh_token_id = $1 | |||
| WHERE compat_session_id = $1 | |||
There was a problem hiding this comment.
please add AND consumed_at IS NULL, so that we don't update all the refresh tokens consumed ts
Also, you need to change/remove the DatabaseError::ensure_affected_rows(&res, 1) bellow, else it will error out if there are more than one row affected
| Ok(SynapseRowCounts { users }) | ||
| let devices = sqlx::query_scalar( | ||
| " | ||
| SELECT COUNT(1) FROM devices |
There was a problem hiding this comment.
WHERE NOT hidden? else we'll count the cross signing devices as well
crates/syn2mas/src/migration.rs
Outdated
| created_at, | ||
| is_synapse_admin: synapse_admins.contains(&user_id), | ||
| last_active_at: last_seen.map(DateTime::from), | ||
| last_active_ip: ip.map(|ip| ip.parse()).transpose().ok().flatten(), |
There was a problem hiding this comment.
very minor, but maybe we should log a warning if for some reason that doesn't parse? That would help us in test runs we're not loosing a bunch of IPs here
| .to_owned(); | ||
| let Some(user_id) = user_localparts_to_uuid.get(username.as_str()).copied() else { | ||
| return Err(Error::MissingUserFromDependentTable { | ||
| table: "devices".to_owned(), |
There was a problem hiding this comment.
didn't notice before, but curious why this is a String and not a &'static str?
sandhose
force-pushed
the
rei/syn2mas_8_atrtdevs
branch
from
0efbff8 to
7181cc8
Compare
We also now support deviceless access tokens/
compat_sessions, human names oncompat_sessionsand the migration of guests.Commit-by-commit suggested! :)