session-management #50

Merged
rayyan merged 29 commits from session-management into master 2026-10-13 22:11:18 +02:00
Owner

Introduction

I started by working on key backup to fix the warning that has been plaguing us for so long:

image
[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

  • Fixed OAuth login to use the existing persistent SQLite-backed client.
  • Added OAuth session restoration and reused application registrations by issuer.
  • Restored the most recently signed-in account on startup.
  • Opened chat automatically after login and fixed initial room loading timing.
  • Added settings, logout, actual account details on bottom left and resizable side panels.
  • Added platform-specific device names, such as “Echelon on Linux.” (Also fixed #49)
  • Fixed the password visibility toggle (fixes #30).
  • Changed default logging to WARN/ERROR, with RUST_LOG available for development.
  • Updated shared IDE settings and included the Lucide license notices.
  • Reduced unnecessary token copies, wiped temporary/replaced token buffers, and avoided logging credential bytes from keyring errors.
  • Fixed #46 & #47

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:

  1. A refresh callback loads the snapshot.
  2. Logout deletes it.
  3. The callback commits its old copy, recreating removed credentials.

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)

  • Revoked sessions: local restoration does not prove the server still accepts the session. Clear reauthentication handling is still needed.

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)

# Introduction I started by working on key backup to fix the warning that has been plaguing us for so long: ![image](/attachments/37f24856-01ba-4b6a-8a36-df7940270555) [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 - Fixed OAuth login to use the existing persistent SQLite-backed client. - Added OAuth session restoration and reused application registrations by issuer. - Restored the most recently signed-in account on startup. - Opened chat automatically after login and fixed initial room loading timing. - Added settings, logout, actual account details on bottom left and resizable side panels. - Added platform-specific device names, such as “Echelon on Linux.” (Also fixed #49) - Fixed the password visibility toggle (fixes #30). - Changed default logging to WARN/ERROR, with `RUST_LOG` available for development. - Updated shared IDE settings and included the Lucide license notices. - Reduced unnecessary token copies, wiped temporary/replaced token buffers, and avoided logging credential bytes from keyring errors. - Fixed #46 & #47 --- ## 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: 1. A refresh callback loads the snapshot. 2. Logout deletes it. 3. The callback commits its old copy, recreating removed credentials. 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) - **Revoked sessions:** local restoration does not prove the server still accepts the session. Clear reauthentication handling is still needed. --- ## 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)
Cleaned workspace.xml and removed random whitespace in .gitignore
All checks were successful
test / test (push) Successful in 2m54s
ffa6080e86
Only tracking shared JetBrains settings
All checks were successful
test / test (push) Successful in 2m53s
4144801935
commit farming (jetbrains stuff)
All checks were successful
test / test (push) Successful in 2m54s
904a856b36
Updated tracing-subscriber to filter logs, so we can have different levels of logs
All checks were successful
test / test (push) Successful in 2m51s
33edcb2504
Also updated the README debugging section
Fixed OAuth registration reuse and session restoration
All checks were successful
test / test (push) Successful in 2m55s
9657c99151
- Reuse OAuth client registrations with portless loopback redirects
- Save OAuth client IDs in Stronghold and restore sessions from SQLite
- Keep password sessions compatible and remove the old OAuth register flow
- Update storage and restoration tests
Connected OAuth login to UI and added device display names
All checks were successful
test / test (push) Successful in 2m57s
97b00d3cac
(Also added some UI tests)
Made rooms bar and participants bar expandible and contractible
All checks were successful
test / test (push) Successful in 3m13s
2609c0f902
Added logout button and also protected session storage during cleanup, specifically by:
All checks were successful
test / test (push) Successful in 3m8s
9349337656
- Adding a mutex around Stronghold snapshot reads(more notes on this during PR), writes and deletion
- Checking that a stored session exists and its user, device and OAuth client IDs match before saving refreshed tokens
- Keeping the DB password incase deletion fails
- Removing account DB and credentials on logout
- Replacing the logged in handler with an unauthenticated one so we can login without restarting
- Added more tests :D
- Registered token callbacks before restoring sessions
- Removed unnecessary token copies and wiped replaced/deleted token buffers
- Protected secret decoding on error and cleared passwords after authentication
- Changed keyring error logging to avoid exposing credential bytes
- Blocked overlapping authentication through the UI and debug menu
- Added OS-specific names for password devices and a malformed-token regression test
Added separation between MIT and Lucide licenses
All checks were successful
build / build-windows (pull_request) Successful in 3m32s
build / build-android (pull_request) Successful in 4m35s
test / test (push) Successful in 3m23s
build / build-linux (pull_request) Successful in 3m2s
999549fdaa
@ -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/")?;
Owner

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.

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.
flaxeneel2 marked this conversation as resolved
Owner

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.

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_handle
Owner

another 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.

another 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.
flaxeneel2 marked this conversation as resolved
@ -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());
Owner

this technically makes another string with the password in it. i dont think the zeroing behaviour applies to this to_string()'d field.

this technically makes another string with the password in it. i dont think the zeroing behaviour applies to this `to_string()`'d field.
Owner

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

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
Owner

looked into it. zeroize behaviour still applies to clones but not to to_string()'d strings

looked into it. zeroize behaviour still applies to clones but not to `to_string()`'d strings
flaxeneel2 marked this conversation as resolved
@ -5,4 +5,0 @@
homeserver: String,
state: ClientState,
is_login: bool,
) -> Result<String, String> {
Owner

thank you for removing the is_login bool

thank you for removing the is_login bool
flaxeneel2 marked this conversation as resolved
@ -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).await
oauth_impl(homeserver, state).await
Owner

why not just put oauth_impl code in here? as far as i see it, i cant find any other calls on it

why not just put `oauth_impl` code in here? as far as i see it, i cant find any other calls on it
flaxeneel2 marked this conversation as resolved
@ -372,2 +376,4 @@
}
/// Clear account-owned UI state after logout while retaining layout preferences.
fn reset_account_ui(ui: &AppWindow) {
Owner

spooky scary

spooky scary
Owner

i feel like i'm reading undertale code right now

i feel like i'm reading undertale code right now
flaxeneel2 marked this conversation as resolved
src/lib.rs Outdated
@ -1052,0 +1207,4 @@
.trim_end_matches('/')
.trim_start_matches("https://")
.trim_start_matches("http://")
.to_string();
Owner

2 to_string() calls here, 2 allocs. can be reduced to just one. also dont really need 3 trim calls for this. can do let homeserver = client.homeserver().host_str().unwrap_or_default().to_owned(); or

use url::Position;

let url = client.homeserver();
let homeserver = url[Position::BeforeHost..Position::BeforePath].to_owned();

if we want to keep port

2 `to_string()` calls here, 2 allocs. can be reduced to just one. also dont really need 3 trim calls for this. can do `let homeserver = client.homeserver().host_str().unwrap_or_default().to_owned();` or ```rs use url::Position; let url = client.homeserver(); let homeserver = url[Position::BeforeHost..Position::BeforePath].to_owned(); ``` if we want to keep port
flaxeneel2 marked this conversation as resolved
@ -1052,0 +1241,4 @@
});
}
});
hierarchy_task = Some(handle.spawn(async move {
Owner

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?

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?
Owner

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

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
Owner

right now the name lookup task is not tracked either, so reopening the chat leaves it running in the background (abort() doesnt catch it).

right now the name lookup task is not tracked either, so reopening the chat leaves it running in the background (`abort()` doesnt catch it).
flaxeneel2 marked this conversation as resolved
@ -1570,3 +1806,4 @@
}
});
match app_state.echelon_store.get_last() {
Owner

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.

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.
flaxeneel2 marked this conversation as resolved
@ -99,6 +99,7 @@ impl SyncManager {
if let Some(handle) = sync_guard.take() {
debug!("Stopping sync task...");
handle.abort();
let _ = handle.await;
Owner

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.

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.
flaxeneel2 marked this conversation as resolved
@ -113,3 +149,3 @@
None,
)?;
store.insert(
wipe_store_value(store.insert(
Owner

very nice

very nice
flaxeneel2 marked this conversation as resolved
@ -162,1 +148,3 @@
}
let revoke_error = match tokio::time::timeout(
std::time::Duration::from_secs(10),
handler.revoke_session(),
Owner

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

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
flaxeneel2 marked this conversation as resolved
@ -40,0 +52,4 @@
return;
}
};
let Some(current_device) = devices.iter().find(|device| device.device_id == device_id)
Owner

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.

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.
flaxeneel2 marked this conversation as resolved
@ -29,2 +24,2 @@
oauth.server_metadata().await?;
let metadata = oauth.server_metadata().await?;
let issuer = metadata.issuer.as_str();
Owner

you do not need 2 variables here if metadata is not used elsewhere except for issuer

you do not need 2 variables here if metadata is not used elsewhere except for issuer
Author
Owner

It just looks cleaner as is, compiler handles it anyway, so it's not really necessary imo.

It just looks cleaner as is, compiler handles it anyway, so it's not really necessary imo.
flaxeneel2 marked this conversation as resolved
@ -40,0 +90,4 @@
} else {
"Echelon"
}
}
Owner

could you do this using match instead?
something like this:

match std::env::consts::OS {
    "android" => {}
    ...
}
could you do this using match instead? something like this: ```rust match std::env::consts::OS { "android" => {} ... } ```
Author
Owner

done

done
flaxeneel2 marked this conversation as resolved
Author
Owner

I changed the URL to echelon.sh instead of git.flaxeneel2.net (old clients, I can't change)

"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."

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.

"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"

  • Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the Zeroizing wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later.

"why not just put oauth_impl code in here? as far as i see it, i cant find any other calls on it"

I changed it, it's an artifact from back when we had the registration and login bool, so we don't need it anymore.

"we perhaps allow the user to choose which account they restore in? it can be a different PR though for now this is fine."

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.

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
flaxeneel2 commented 7 hours ago
Owner

right now the name lookup task is not tracked either, so reopening the chat leaves it running in the background (abort() doesnt catch it).

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.

I changed the URL to echelon.sh instead of git.flaxeneel2.net (old clients, I can't change) > "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." 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. > "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" - Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the `Zeroizing` wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later. > "why not just put oauth_impl code in here? as far as i see it, i cant find any other calls on it" I changed it, it's an artifact from back when we had the registration and login bool, so we don't need it anymore. > "we perhaps allow the user to choose which account they restore in? it can be a different PR though for now this is fine." 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. > 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 flaxeneel2 commented 7 hours ago Owner > right now the name lookup task is not tracked either, so reopening the chat leaves it running in the background (abort() doesnt catch it). 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.
Owner

good work!

Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the Zeroizing wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later.

Here, I feel thats what Zeroizing's Clone Impl does no? for any Zeroizing<Z>, if the underlying Z implements Clone, 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

good work! > Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the Zeroizing wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later. Here, I feel thats what [Zeroizing's Clone Impl](https://docs.rs/zeroize/latest/zeroize/struct.Zeroizing.html#impl-Clone-for-Zeroizing%3CZ%3E) does no? for any `Zeroizing<Z>`, if the underlying `Z` implements `Clone`, 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
replaced if statements within device display name with a match
All checks were successful
test / test (push) Successful in 3m12s
build / build-windows (pull_request) Successful in 3m14s
build / build-linux (pull_request) Successful in 2m47s
build / build-android (pull_request) Successful in 4m24s
fdf49add08
Author
Owner

@flaxeneel2 wrote in #50 (comment):

good work!

Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the Zeroizing wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later.

Here, I feel thats what Zeroizing's Clone Impl does no? for any Zeroizing<Z>, if the underlying Z implements Clone, 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

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.

@flaxeneel2 wrote in https://git.flaxeneel2.net/streigen/echelon/pulls/50#issuecomment-299: > good work! > > > Correct, cloning the plain string doesn't really solve it either, since the zeroisation only applies to the value owned by the Zeroizing wrapper, not every copy of the underlying password. We could also open an issue to be psychotic and try to fix this somehow anyway later. > > Here, I feel thats what [Zeroizing's Clone Impl](https://docs.rs/zeroize/latest/zeroize/struct.Zeroizing.html#impl-Clone-for-Zeroizing%3CZ%3E) does no? for any `Zeroizing<Z>`, if the underlying `Z` implements `Clone`, 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 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.
human489 requested changes 2026-10-13 18:27:07 +02:00
Dismissed
@ -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();
Owner

you don't need to clone user_id if it is never consumed

you don't need to clone `user_id` if it is never consumed
Author
Owner

Fixed 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

Fixed 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
human489 marked this conversation as resolved
Fixed minor issues with ownership of user_id and redirect_uri
Some checks failed
test / test (push) Successful in 3m10s
build / build-windows (pull_request) Successful in 3m10s
build / build-linux (pull_request) Successful in 2m43s
build / build-android (pull_request) Has been cancelled
5da3803c21
update version to 0.2.0
All checks were successful
test / test (push) Successful in 3m15s
build / build-windows (pull_request) Successful in 3m21s
build / build-linux (pull_request) Successful in 3m5s
build / build-android (pull_request) Successful in 13m2s
5e2ab36f01
human489 approved these changes 2026-10-13 21:53:15 +02:00
flaxeneel2 approved these changes 2026-10-13 21:56:24 +02:00
flaxeneel2 left a comment

looks good to me

looks good to me
rayyan merged commit 86b047bdde into master 2026-10-13 22:11:18 +02:00
rayyan deleted branch session-management 2026-10-13 22:11:19 +02:00
Sign in to join this conversation.
No description provided.