session-management #50
No reviewers
Labels
No labels
bug
core
documentation
duplicate
enhancement
experiment
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
streigen/echelon!50
Loading…
Reference in a new issue
No description provided.
Delete branch "session-management"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Introduction
I started by working on key backup to fix the warning that has been plaguing us for so long:
[my beloved]
However, while researching how to solve it, I realised that there were several issues that needed to be fixed before I could work on session verification, mainly with OAuth, session restoration and logout. This PR addresses those foundations before session verification and recovery are added, along with the settings menu prototype.
This should also make account switching fairly easy for @human489 to implement.
Summary of changes
RUST_LOGavailable for development.Authentication and Storage
Persistent OAuth sessions
OAuth previously retained the temporary authentication client. It now saves the session and restores it into a SQLite-backed client through the existing factory, matching the same flow as password logins.
OAuth also needs an additional entry within the store, called an oauth_client_id. Existing password snapshots still work, but older OAuth snapshots do not.
Token persistence callbacks are installed before SDK restoration. Restoration starts background encryption requests that can trigger a refresh; registering the callback first allows those early token rotations to be saved.
Snapshot Operation protections during logout
Added a mutex around Stronghold snapshot reads, writes and deletion, including SQLite-password operations. Each operation loads its own snapshot, so its complete load/check/commit sequence must be coordinated.
Without this protection:
Refresh writes now require an existing session with matching user, device and OAuth client IDs. This also prevents an old device’s callback from overwriting a replacement login for the same account.
Reads use the same mutex because retrieving the keyring key and loading its snapshot must remain consistent with replacement/deletion.
Logout cleanup
Logout attempts server revocation, stops and awaits sync, clears the room subscription and removes the current account’s database and credentials. It then leaves an unauthenticated handler available so another login works without restarting.
If database deletion fails, credentials are cleared while its encryption password/key are retained, and incomplete cleanup is reported. Deleting the password while leaving encrypted files would make those files inaccessible.
Other accounts and the shared OAuth application registration are still kept.
Authentication UI guards
Applied existing busy state across login, startup restoration, logout and debug authentication, including navigation that could bypass disabled controls.
For example, starting debug logout during startup restoration could otherwise allow the older restore to finish afterward and reinstall a signed-in handler.
Known limits and follow-up
Failed persistent-client setup: a fresh login can save device B’s session before discovering that the existing crypto store belongs to device A. Restoration then fails and can repeat on startup. Successful logout removes the old database; leftover data and failed cleanup still need an explicit reauthentication policy.
Suggested improvement: If saving refreshed tokens fails, the running app still has them in memory, but restarting may load outdated credentials and require another login.
So, I think we should retry failed local token saves in the background, with increasing delays, and warn the user while saving remains unsuccessful. Retries should use the latest tokens, stop after logout or session replacement, and never overwrite a newer saved pair. The retry task should avoid keeping the SDK client/database open during logout. (Wanted to discuss this before implementation since it's a major change, and I would be happy to talk about it further)
Final Notes
Added new test cases for the new features.
Clippy also passes without any warnings and manual testing was done on all aforementioned features.
I also recommend bumping the version number to 0.2.0 (Has not been done yet)
@ -48,0 +35,4 @@.set_port(None).map_err(|_| anyhow::anyhow!("Invalid OAuth redirect URI"))?;let url = Url::parse("https://git.flaxeneel2.net/streigen/echelon/")?;i think we can remove my name from here and instead use https://echelon.sh (ideally) or https://streigen.com instead. I have added redirect so they redirect user to this git instance.
one prior bug we had that we should maybe fix in this PR is that logging in again while an old account folder still existing on disk breaks that account for good.
both password auth and oauth auth save the new session and add account before they open the user's local db. if that db holds the older encryption state, matrix sdk will reject it with
MismatchedAccount. By then the old tokens have already been overwritten without properly being revoked so the old device is left orphaned on the server, and every future bootup will attempt to restore session and fail the same way as well.Only fix for this bug so far is deleting the data folder by hand. we might as well fix this right now. hmmm could add a few tests for this behaviour too so we can catch future regressions.
@ -55,3 +58,4 @@webbrowser::open(auth_data.url.as_str()).map_err(|e| anyhow::anyhow!("Failed to open URL in browser: {}", e))?;let query = redirect_handleanother bug that used to exist before that could be fixed in this PR. starting an oauth flow and aborting it by (for ex) closing the browser tab kinda leaves the app in limbo. the app has no timeout or no way to cancel an oauth, with the only option being restarting the app entirely.
@ -28,3 +29,3 @@let mut request = RegistrationRequest::new();request.username = Some(username.clone());request.password = Some(password.clone());request.password = Some(password.to_string());this technically makes another string with the password in it. i dont think the zeroing behaviour applies to this
to_string()'d field.also i just realised we cant even fully zeroize passwords since it lives as a slint shared string somewhere as well. its only a best effort zeroize atp
looked into it. zeroize behaviour still applies to clones but not to
to_string()'d strings@ -5,4 +5,0 @@homeserver: String,state: ClientState,is_login: bool,) -> Result<String, String> {thank you for removing the is_login bool
@ -50,3 +38,1 @@pub async fn oauth_register(homeserver: String, state: ClientState) -> Result<String, String> {trace!("Starting OAuth register for homeserver: {}", homeserver);oauth_impl(homeserver, state, false).awaitoauth_impl(homeserver, state).awaitwhy not just put
oauth_implcode in here? as far as i see it, i cant find any other calls on it@ -372,2 +376,4 @@}/// Clear account-owned UI state after logout while retaining layout preferences.fn reset_account_ui(ui: &AppWindow) {spooky scary
i feel like i'm reading undertale code right now
@ -1052,0 +1207,4 @@.trim_end_matches('/').trim_start_matches("https://").trim_start_matches("http://").to_string();2
to_string()calls here, 2 allocs. can be reduced to just one. also dont really need 3 trim calls for this. can dolet homeserver = client.homeserver().host_str().unwrap_or_default().to_owned();orif we want to keep port
@ -1052,0 +1241,4 @@});}});hierarchy_task = Some(handle.spawn(async move {ah so you moved hierarchy task to its own thread, nice. just curious, did it help with the load times? what was the difference between spawning a task for it and not spawning one?
as in, wouldnt the 2 seperate running tasks both need a hold on client? making one wait for the other to finish with their lock anyways
right now the name lookup task is not tracked either, so reopening the chat leaves it running in the background (
abort()doesnt catch it).@ -1570,3 +1806,4 @@}});match app_state.echelon_store.get_last() {hmm about this, i noticed that on logout and starting of app again it restores the last non-logged-out account. this could work, but, should we perhaps allow the user to choose which account they restore in? it can be a different PR though for now this is fine.
@ -99,6 +99,7 @@ impl SyncManager {if let Some(handle) = sync_guard.take() {debug!("Stopping sync task...");handle.abort();let _ = handle.await;i think it would be beneficial to not discard the handle result here as we could use this to log panics in the sync task.
@ -113,3 +149,3 @@None,)?;store.insert(wipe_store_value(store.insert(very nice
@ -162,1 +148,3 @@}let revoke_error = match tokio::time::timeout(std::time::Duration::from_secs(10),handler.revoke_session(),revoke is called before sync is stopped. if a sync happens while this code is running itll get a 401. in case for oauth it would try refreshing with refresh token that just got revoked
@ -40,0 +52,4 @@return;}};let Some(current_device) = devices.iter().find(|device| device.device_id == device_id)downloading the entire device list just to check for one device is a bit longer than needed. could use
client.send(get_device::v3::Request::new(device_id));to get just the device id in question.@ -29,2 +24,2 @@oauth.server_metadata().await?;let metadata = oauth.server_metadata().await?;let issuer = metadata.issuer.as_str();you do not need 2 variables here if metadata is not used elsewhere except for issuer
It just looks cleaner as is, compiler handles it anyway, so it's not really necessary imo.
@ -40,0 +90,4 @@} else {"Echelon"}}could you do this using match instead?
something like this:
done
I changed the URL to echelon.sh instead of git.flaxeneel2.net (old clients, I can't change)
I did notice this, but I thought it would be better handled as part of account switching. Under the intended login flow, users shouldn't normally be able to log into an account that already exists locally, so I opened #51 for @human489 to address when implementing account switching. I also mentioned adding tests within the issue.
As suggested, I added a button to cancel OAuth and a five-minute timeout while waiting for the browser. At the moment, there doesn’t seem to be a practical way for Echelon to close the browser window it opens, so I may look into that separately in the future. For now, I don’t think it’s necessary to include within the scope of this PR.
Zeroizingwrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later.I changed it, it's an artifact from back when we had the registration and login bool, so we don't need it anymore.
Yeah, this can simply be a part of account switching whenever @human489 decides to implement it.
I added the homeserver parsing that was recommended and I logged the panic in the sync
Added remaining minor optimisations suggested.
Measured five runs with room data already available. The sidebar averaged 51.20ms on this branch versus 53.28ms on master, so there’s no significant difference. The display-name update averaged 86.03ms, finishing after the sidebar in every run.
Both tasks briefly take a read lock to clone the client, then release it before doing their work, so that lock doesn’t serialize them. The separate name task lets the sidebar finish independently of the profile request.
Fixed the untracked name task: reopening chat now cancels both previous tasks, and generation checks reject stale queued UI updates.
good work!
Here, I feel thats what Zeroizing's Clone Impl does no? for any
Zeroizing<Z>, if the underlyingZimplementsClone, Zeroizing implements it for that datatype as well. I think its no point right now though since slint's SharedString would be hard to zero out anyways, so its okay@flaxeneel2 wrote in #50 (comment):
Like we discussed, it's a non issue once we implement the homeserver login anyway. So not really any point in worrying about it anyway, like you said.
@ -81,0 +70,4 @@.session().ok_or_else(|| anyhow::anyhow!("Missing OAuth session after login"))?;let session = session_of(&new_client)?;let user_id = session.user_id.clone();you don't need to clone
user_idif it is never consumedFixed it, and also updated the remaining references to it, so I don't have an &&string for no reason, and I also consumed redirect_uri when I last used it, since I spotted that while fixing what you asked :D
looks good to me