Skip to content

Generate/load pickle key on SSO #29568

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

Merged
merged 6 commits into from
Mar 21, 2025
Merged

Generate/load pickle key on SSO #29568

merged 6 commits into from
Mar 21, 2025

Conversation

Jujure
Copy link
Contributor

@Jujure Jujure commented Mar 21, 2025

In lifecycle, no pickle key seems to be ever created when logging in through SSO since the pickle key creation is done in setLoggedIn which is not called after attemptDelegatedAuthLogin. Thus when using SSO login, the indexed DB will be stored in plaintext on disk.

This aims to try to generate and load a pickle key in onSuccessfulDelegatedAuthLogin.

Testing strategy:

  • On an element-desktop build with keytar
  • Sign out/Sign in through SSO
  • Check the indexed DB in the dev tools, mx_access_token should be encrypted and not in plaintext

I am still unsure on how to write unit tests about this as the OIDC native flow tests all seems to call setLoggedIn manually but I do not think that this code is actually reachable in the actual app when doing SSO, only doSetLoggedIn is.

I would like to take feedbacks on this before making significant changes to tests I did not write and may not understand completely.

@Jujure Jujure requested a review from a team as a code owner March 21, 2025 14:49
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Mar 21, 2025
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Would be good to reuse code rather than duplicate it, especially as the other codepath has helpful logging. Please extract the code from setLoggedIn into a method which both code paths can share.

@Jujure
Copy link
Contributor Author

Jujure commented Mar 21, 2025

Just done that, to avoid different behaviours, setLoggedIn now also tries to load an existing pickle key given the device ID/MXID instead of creating a new one everytime. I don't think this causes any issues but if so, let me know.

@Jujure Jujure requested a review from t3chguy March 21, 2025 16:17
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane to me other than the prettier fail

Signed-off-by: Julien CLEMENT <[email protected]>
@t3chguy t3chguy added this pull request to the merge queue Mar 21, 2025
Merged via the queue into element-hq:develop with commit fba5938 Mar 21, 2025
29 checks passed
@Jujure Jujure deleted the pickle_sso branch March 24, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants