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: Web: display a warning when an *unverified* user changes identity #2513

Closed
Tracked by #2491
richvdh opened this issue Aug 28, 2024 · 12 comments · Fixed by element-hq/element-web#28211
Assignees

Comments

@richvdh
Copy link
Member

richvdh commented Aug 28, 2024

Followup to #2492. Part of #2491, itself part of Invisible crypto.

When an unverified user changes their identity, we need to make our user aware of this. In the long term, the intention is just to show a notice in the timeline (#2493); however, that is difficult to implement and we need a stop-gap.

This task is for Element Web.

The proposal is to show a warning above the composer, in much the same way as we would for verified users (#2491), but without locking the composer.

Figma designs:

@richvdh
Copy link
Member Author

richvdh commented Sep 10, 2024

Implementation notes:

  • in matrix-sdk-crypto, we need to extend UserIdentities to expose identity_needs_user_approval not sure this is needed
  • in matrix-sdk-crypto-wasm, extend UserIdentity and OwnUserIdentity to expose identity_needs_user_approval
  • in matrix-js-sdk, extend RustCrypto.getUserVerificationStatus and UserVerificationStatus to include identityNeedsUserApproval
  • in matrix-react-sdk, create a component which will:
    • on start, call Room.getEncryptionTargetMembers to get a list of users of interest, and then call getUserVerificationStatus on each, to build a list of users with pin violations
    • register callback for room membership updates; when we get one, check if the user is on the target encryption list, and check for pin violations (or remove from list if they have left)
    • register callback for trust updates; when we get one:
      • if the user is not in our list and there is a pin violation, check if they are on the target encryption list, and update the list if so
      • if the user is on our list and there is not a pin violation, remove them immediately

@andybalaam andybalaam assigned andybalaam and unassigned andybalaam Sep 16, 2024
@richvdh richvdh assigned richvdh and unassigned richvdh Sep 18, 2024
@andybalaam
Copy link
Member

andybalaam commented Sep 20, 2024

It would be great if we can re-use the logic I am writing here matrix-org/matrix-rust-sdk@main...andybalaam/room-identity-stream .

The core logic is inside the crypto crate, so there is some chance we can re-use it.

@uhoreg
Copy link
Member

uhoreg commented Oct 2, 2024

Is there a particular reason we want the banner to say "... appears to have changed" instead of simply "... has changed"? ( @mxandreas ?)

@mxandreas
Copy link

Is there a particular reason we want the banner to say "... appears to have changed" instead of simply "... has changed"?

Great question :) I was told that there is one, but I do not know what it exactly is. I also challenged it a while ago, and the response was that there had been a long debate on it already, thus I left it as there were more important topics. FWIW - I think it helps to distinguish between the pinned vs verified identity change, so supports debugging with users, etc.

@andybalaam
Copy link
Member

Yeah, the wording is from https://github.com/matrix-org/matrix-spec-proposals/blob/andybalaam/crypto-terminology/proposals/4161-crypto-terminology.md#identity and it was crafted over quite a lot of time.

The point is that there are 2 possible cases:

  • EITHER 1. the person is the same, and their cryptographic identity has changed,
  • OR 2. the actual physical identity of the person is different - i.e. their account has been hijacked.

The wording is saying "it looks like 2 happened".

Make sense?

@uhoreg uhoreg self-assigned this Oct 3, 2024
@uhoreg
Copy link
Member

uhoreg commented Oct 3, 2024

It would be great if we can re-use the logic I am writing here matrix-org/[email protected]/room-identity-stream .

The core logic is inside the crypto crate, so there is some chance we can re-use it.

For reference, this is also matrix-org/matrix-rust-sdk#4022

@uhoreg
Copy link
Member

uhoreg commented Oct 3, 2024

If there are multiple users whose identity has changed, as a first cut, I'll just show one user at a time. It may be slightly annoying, but hopefully there won't be too many users that need acknowledging at a time.

@mxandreas
Copy link

If there are multiple users whose identity has changed, as a first cut, I'll just show one user at a time. It may be slightly annoying, but hopefully there won't be too many users that need acknowledging at a time.

Yes, that was the plan for now, also for EX.

@mxandreas
Copy link

For visibility: #2525 (comment)

@uhoreg
Copy link
Member

uhoreg commented Oct 9, 2024

image

@mxandreas
Copy link

mxandreas commented Oct 14, 2024

@uhoreg Andy already mentioned but just in case - if you implement this, then please do it so that in clear rooms the banner is not shown: #2560

@mxandreas
Copy link

@uhoreg And another similar follow-up to me mentioned is #2565

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