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

Invisible crypto: display a warning when a verified user changes identity #2492

Open
Tracked by #2491
richvdh opened this issue Aug 2, 2024 · 19 comments
Open
Tracked by #2491

Comments

@richvdh
Copy link
Member

richvdh commented Aug 2, 2024

Part of #2491, itself part of Invisible crypto.

We need to display a warning when a user which we have previously verified changes their cryptographic identity.

See also #2500 and element-hq/element-web#27943, which concern showing an error if we get as far as sending a message despite this warning (eg, there is a race).

Note also: previously we discussed showing a notice if an unverified (tofu-trusted) user changes identity. We now believe that is unnecessary and that a message in the timeline (#2493) is sufficient EDIT 2024/08/28: we still believe this is the case in the long term, but since #2493 looks like a reasonable amount of work, we need a warning banner in the meantime. #2513 tracks this work.


Figma designs:

@richvdh
Copy link
Member Author

richvdh commented Aug 2, 2024

What happens if there are 2, or 5, or 50, affected users? (Likely if you reopen a session that has been closed for a while)

  • As a first pass, we just pick an arbitrary user. Then, as soon as you dismiss “Alice has reset their encryption”, it is replaced with “Bob has reset their encryption”.

  • Longer-term, maybe we should show a banner with “5 people have changed their encryption”, and have a link which opens a dialog box listing all the affected users.

@richvdh
Copy link
Member Author

richvdh commented Aug 5, 2024

XXX: not sure we actually need to lock out the composer, since the attempt to send the message will fail (matrix-org/matrix-rust-sdk#3793)?

It's better UX, rather than waiting for the user to send the message and then getting an error.

@richvdh

This comment was marked as resolved.

@richvdh
Copy link
Member Author

richvdh commented Aug 5, 2024

The banner needs to have been on screen for some seconds before it is considered that the user has seen it; if it's been on screen for more than N seconds, then the UI updates pinned key in the backend; so then there will be no error (and the banner will not be shown in future rooms).

@pmaier1 says we don't need to block sending at all in tofu mode -- if you care about who receives your messages, you need to verify.

@pmaier1
Copy link
Contributor

pmaier1 commented Aug 5, 2024

The TOFU identity change design needs another iteration, I think. At least we discussed to not block the user in this scenario.

@mxandreas
Copy link

mxandreas commented Aug 21, 2024

To make sure that there's no ambiguity in the overall model, and it is formally sound, I'd like to confirm the following before we go into the details of UI. Especially, as it can affect quite a bit some of the more specific scenarios.

a) My current understanding is that as soon as Alice's identity changes, Bob implicitly pins the new identity, regardless of what of what Bob sees or does on the UI. For example, if Bob has verified Alice and Alice changes their identity 2 times (without Bob doing anything on UI yet), then this counts as 2 identity changes - the first one is a verified identity change, and the second is a pinned identity change.

b) An alternative to this model is that there is an in-between state v-changed or p-changed which lasts until Bob takes some action. For example, if Bob has verified Alice and Alice changes their identity 2 times (without Bob doing anything on UI yet), then this counts as 1 identity change. Because after the first change, Alice landed in this v-changed stated and in this state we're ignoring all further identity changes until Bob takes action (e.g. withdraws/pins or verifies).

In my discussions with @pmaier1 I thought that a) is what we're using but some of the questions in here lead me to think that may be it is not the case.

Here's a model for a) in which case the Changed states generate a warning but are virtual in the sense that automatic/instant transition to the Pinned state follows without any user action.
image

@richvdh richvdh changed the title Invisible crypto: inform users about changes to other users' cryptographic identity Invisible crypto: display a warning when another user changes identity Aug 21, 2024
@richvdh
Copy link
Member Author

richvdh commented Aug 21, 2024

a) My current understanding is that as soon as Alice's identity changes, Bob implicitly pins the new identity, regardless of what of what Bob sees or does on the UI.

I don't think this is correct. First of all, per #2492 (comment), we try very hard to make sure that Bob gets a chance to see the warning before we update his "pinned" copy of Alice's identity. Secondly, if he had previously verified Alice, then he would actually have to go through the verification process again (or click "withdraw verification") before updating the pinned identity.

For example, if Bob has verified Alice and Alice changes their identity 2 times (without Bob doing anything on UI yet), then this counts as 2 identity changes - the first one is a verified identity change, and the second is a pinned identity change.

Worth noting that we don't store a separate "verified identity" -- we store (a) the most-recently-seen identity, and (b) the pinned identity, each of which may or may not be verified.

In this example, yes there are two identity changes, but they are both identity changes for a verified user, and therefore qualify for the stronger, blocking, "you must re-verify or withdraw verification before proceeding" warning.

@mxandreas
Copy link

Worth noting that we don't store a separate "verified identity" -- we store (a) the most-recently-seen identity, and (b) the pinned identity, each of which may or may not be verified.

Good to know. Also, good example why we should have an agreed model that guides implementation and UI. Very hard to tell which parts of this implementation logic (e.g. in terms of what gets stored) is just a way to realize the desired model, and which are the model we desire.

In this example, yes there are two identity changes, but they are both identity changes for a verified user

What qualifies as a verified user (given the storage mechanism that was mentioned)?

@mxandreas
Copy link

@richvdh I created a another state diagram based on what you said. I did not validate if it can be unambiguously mapped to the implementation/storage that you highlighted but perhaps we can agree first if that is what we want and need to drive the measures of trust and the UX.

image

@americanrefugee
Copy link

Identity change screens for all platforms:

@richvdh
Copy link
Member Author

richvdh commented Aug 21, 2024

Identity change screens for all platforms:

Just for reference: these designs span both this issue ("show a warning above the composer") and #2493.

@richvdh
Copy link
Member Author

richvdh commented Aug 21, 2024

See also my notes from the meeting earlier, at #2491 (comment)

@richvdh richvdh changed the title Invisible crypto: display a warning when another user changes identity Invisible crypto: display a warning when a verified user changes identity Aug 22, 2024
@richvdh
Copy link
Member Author

richvdh commented Aug 22, 2024

I think maybe the best way to implement this is for matrix-sdk-crypto to expose a list of "previously-verified users who have changed identity" (and a Stream for updates to the list), and then the UI can filter that list according to the list of encryption target members for the room.

@richvdh
Copy link
Member Author

richvdh commented Aug 22, 2024

currently, the designs for this include the wording "<Name>’s verified identity has changed and their messages are hidden". However, that won't actually be true until we do https://github.com/element-hq/crypto-internal/issues/353, https://github.com/element-hq/crypto-internal/issues/354 and https://github.com/element-hq/crypto-internal/issues/362

@richvdh
Copy link
Member Author

richvdh commented Aug 28, 2024

currently, the designs for this include the wording "’s verified identity has changed and their messages are hidden".

Per the updated designs above, this is no longer the case.

@andybalaam
Copy link
Member

andybalaam commented Sep 9, 2024

To clarify:

* for this task, the messages will appear, but with no content, just a placeholder error about the user's verified identity being changed.
* in the final design, messages are NOT hidden, but they are decorated with a shield. This is covered by #2523
* we don't yet know how to display messages in this state in a push notification

@richvdh
Copy link
Member Author

richvdh commented Sep 10, 2024

for this task, the messages will appear, but with no content, just a placeholder error about the user's verified identity being changed.

I think that's a separate task (https://github.com/element-hq/crypto-internal/issues/362)

@mxandreas
Copy link

@richvdh Leaving a note here (which could be incorporated into the description or a new ticket created as urgency may be different) regarding a situation when you send the message outside of the app - e.g. using the Share functionality. In such a case, from design perspective, it was proposed that a "local" push notification is shown that the message was not sent because the verified identity has changed.

@richvdh
Copy link
Member Author

richvdh commented Sep 18, 2024

@mxandreas could you make a separate issue? It sounds like a different situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants