-
Notifications
You must be signed in to change notification settings - Fork 96
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
Securejoin: store bobstate in database instead of context #2920
Conversation
d275162
to
eecf121
Compare
347fc2a
to
5cb4889
Compare
51a124c
to
1e8f6e0
Compare
src/securejoin.rs
Outdated
// TODO: Check if Alice has a hidden 1:1 chat in which there is an info message | ||
// saying bob is verified. However currently this is purposefully sent to | ||
// Alice's group chat by commit 3b7b8ea0f1f8b9c9d300388768eed069be59c8be? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r10s would you like to check which way is really desired here? I think the current implementation shows the "contact verified" message in a group chat for alice. So if she invites 5 people to a group with QR code she'd get 5 verified messages in the group while the 1:1 chat with those 5 people is still hidden and empty. Was that the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And likewise, how should it behave for bob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, this is fine.
the intention for both, Alice and Bob, is to avoid creation of currently unneeded chats.
since #2508, for Bob, directly after scanning the group chat is opened and in focus of the user. it makes sense to show all messages there. creating a second one-to-one chat when scanning a group is confusing an not needed.
for Alice, it makes sense to have a similar behavior; also as otherwise Alice's chat list is easily spammed with one-to-one chats of all joiners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got round to fixing up this bit. I'll rebase and merge later this week.
7750215
to
13159a6
Compare
The state bob needs to maintain during a secure-join process when exchanging messages used to be stored on the context. This means if the process was killed this state was lost and the securejoin process would fail. Moving this state into the database should help this. This still only allows a single securejoin process at a time, this may be relaxed in the future. For now any previous securejoin process that was running is killed if a new one is started (this was already the case). This can remove some of the complexity around BobState handling: since the state is in the database we can already make state interactions transactional and correct. We no longer need the mutex around the state handling. This means the BobStateHandle construct that was handling the interactions between always having a valid state and handling the mutex is no longer needed, resulting in some nice simplifications. Part of #2777.
13159a6
to
6365418
Compare
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context. This means if
the process was killed this state was lost and the securejoin process
would fail. Moving this state into the database should help this.
This still only allows a single securejoin process at a time, this may
be relaxed in the future. For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).
This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct. We no longer need the mutex around the
state handling. This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.
Part of #2777.
Note that the base of this PR is #2901 to make debugging nicer.