Skip to content
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

Port to libQuotient's keychain and account handling #838

Merged
merged 2 commits into from
Jan 5, 2023
Merged

Conversation

TobiasFella
Copy link
Member

  • At the moment, the "Stay logged in" option is ignored and the connection is always stored in the keychain. I don't think libQuotient currently allows us to have this option
  • When logging in, the "You're already logged in with this account; do you want to continue" [not the exact string] dialog was removed for mostly technical reasons; IMO this should be completely blocked before even logging in since there's no point in logging in multiple times with the same connection and libQuotient might explode from doing it :)

Also, I'm not familiar with the codebase, so this is done relatively blindely - please test well.

@KitsuneRal KitsuneRal added the enhancement A feature or change request for Quaternion label Dec 21, 2022
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Overall looks fairly reasonable, except a few comments and the fact that CI now fails when you build against an external (not a submodule) Quotient.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Thank you!

@KitsuneRal KitsuneRal merged commit 8794402 into dev Jan 5, 2023
@KitsuneRal
Copy link
Member

I forgot to address the "limitations":

  • At the moment, the "Stay logged in" option is ignored and the connection is always stored in the keychain. I don't think libQuotient currently allows us to have this option

At first, I thought that this option should just go because with the current E2EE state (without SSSS and soft logouts) it's all too easy to lose the database. On the other hand, I personally am using it for temporary logins into test accounts... When E2EE is off, I guess it's as easy as making saveAccessTokenToKeychain() call optional. With E2EE though, I guess I would opt into an in-memory database instead of storing anything on the disc... Maybe something for 0.8.

* When logging in, the "You're already logged in with this account; do you want to continue" [not the exact string] dialog was removed for mostly technical reasons; IMO this should be completely blocked before even logging in since there's no point in logging in multiple times with the same connection and libQuotient might explode from doing it :)

Well, I tested libQuotient to work in this case, before E2EE :) and I see that E2EE doesn't really support it, binding the database to the user account, not to the device. I agree that it's a very exotic case and don't mind dropping it; but we have to add some safeguards in the login procedure then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for Quaternion
Projects
Status: Version 0.0.96 - Done
Development

Successfully merging this pull request may close these issues.

2 participants