element-meta icon indicating copy to clipboard operation
element-meta copied to clipboard

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

Open richvdh opened this issue 1 year ago • 19 comments

Part of https://github.com/element-hq/element-meta/issues/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 https://github.com/element-hq/element-meta/issues/2500 and https://github.com/element-hq/element-web/issues/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 avatar Aug 02 '24 12:08 richvdh

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 avatar Aug 02 '24 12:08 richvdh

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

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

richvdh avatar Aug 05 '24 13:08 richvdh

XXX: isn't the fact that it doesn't lock out the composer counter to https://github.com/matrix-org/matrix-rust-sdk/issues/3565?

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

richvdh avatar Aug 05 '24 13:08 richvdh

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.

richvdh avatar Aug 05 '24 14:08 richvdh

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

pmaier1 avatar Aug 05 '24 14:08 pmaier1

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

mxandreas avatar Aug 21 '24 06:08 mxandreas

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 https://github.com/element-hq/element-meta/issues/2492#issuecomment-2269146688, 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.

richvdh avatar Aug 21 '24 09:08 richvdh

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 avatar Aug 21 '24 10:08 mxandreas

@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

mxandreas avatar Aug 21 '24 10:08 mxandreas

Identity change screens for all platforms:

americanrefugee avatar Aug 21 '24 14:08 americanrefugee

Identity change screens for all platforms:

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

richvdh avatar Aug 21 '24 14:08 richvdh

See also my notes from the meeting earlier, at https://github.com/element-hq/element-meta/issues/2491#issuecomment-2301947967

richvdh avatar Aug 21 '24 14:08 richvdh

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 avatar Aug 22 '24 15:08 richvdh

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 avatar Aug 22 '24 15:08 richvdh

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

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

richvdh avatar Aug 28 '24 16:08 richvdh

~~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 https://github.com/element-hq/element-meta/issues/2523~~ ~~* we don't yet know how to display messages in this state in a push notification~~

andybalaam avatar Sep 09 '24 13:09 andybalaam

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)

richvdh avatar Sep 10 '24 13:09 richvdh

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

mxandreas avatar Sep 18 '24 07:09 mxandreas

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

richvdh avatar Sep 18 '24 08:09 richvdh

This is currently in a rather odd state.

  • On EX: if a verified user changes identity, no warning is shown. (If an unverified [TOFU] user changes identity, we show a warning banner, per element-meta#2524.)
  • On EW: if a verified user changes identity, we show the same warning as if an unverified user changes identity (see element-meta#2513); however we also show a red warning shield in the composer.
    • If you try to send the message, it is sent as normal (including to the affected user's devices???), unless the “exclude insecure devices” feature flag is turned ON, in which case it provides only a generic error message.

See also comments from @mxandreas at https://element-io.atlassian.net/browse/ER-122?focusedCommentId=37229.

richvdh avatar Jan 06 '25 16:01 richvdh

@BillCarsonFr will create a new ticket to cover the fact that he has done part of this,

AND he will update this issue to reflect the information in the comments, and the work that he has already done.

andybalaam avatar Jan 16 '25 15:01 andybalaam

@BillCarsonFr will create a new ticket to cover the fact that he has done part of this,

https://github.com/element-hq/element-meta/issues/2715

BillCarsonFr avatar Feb 03 '25 08:02 BillCarsonFr

Closing this in favor of https://github.com/element-hq/element-meta/issues/2570 - all the sub-stories / tasks were either linked to that story or to the epic https://github.com/element-hq/element-meta/issues/2729.

Cleaning it up so we are using the structure by function t keep track of the verified-identities work.

mxandreas avatar Feb 18 '25 07:02 mxandreas