Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Improve error display for messages sent from insecure devices #93

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Sep 27, 2024

#92 provides a mode in which we will reject:

  • messages sent from devices which their owners have not cross-signed
  • messages sent from users who we had previously verified, but who have now rotated their identity.

This PR improves the display of both kinds of message.

The former is shown like this:

image

The latter is shown like this:

image

This is based on the designs at element-hq/element-meta#2523, but we don't yet show the content of the event (that being the point of element-hq/element-meta#2523).

Fixes https://github.com/element-hq/crypto-internal/issues/362.

Add a labs option which will, when set, switch into the "invisible crypto"
mode of refusing to send keys to, or decrypt messages from, devices that have
not been signed by their owner.
Copy link
Member

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

Looks good!

  • The key icon at the left of the new error is not aligned with the new error. Can we align it vertically?
  • Instead of using the old pcss variable, we can use directly the compound tokens
  • Same for all the spacing value

@@ -377,3 +377,14 @@ export async function awaitVerifier(
return verificationRequest.verifier;
});
}

/** Log in a second device for the given bot user */
export async function createSecondBotDevice(page: Page, homeserver: HomeserverInstance, bob: Bot) {
Copy link
Member

Choose a reason for hiding this comment

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

Playwright allow to add fixture which allows to encapsulate and easily share behaviour across the tests.
You can take a look at pinned-messages.spec.ts to see an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Frankly, I don't find our use of fixtures very helpful, and think we should use them less rather than more. They are too "magical", and not flexible enough, whilst a separate function is very explicit, and not much more code.

In this instance: there is a bunch of preparation which has to happen before the second device is created. I don't think I can do that with a fixture?

/* Formatting for the "Verified identity has changed" error */
.mx_DecryptionFailureVerifiedIdentityChanged > span {
/* Show it in red */
color: $e2e-warning-color;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use instead directly a compound token: Light/color/border/critical/subtle

Copy link
Member Author

Choose a reason for hiding this comment

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

done. Thanks for the tip!

res/css/views/messages/_DecryptionFailureBody.pcss Outdated Show resolved Hide resolved

/* With a red border */
border: 1px solid $e2e-warning-color;
border-radius: $font-16px;
Copy link
Member

Choose a reason for hiding this comment

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

Compound spacing token!

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

I this one needs to scale with the font size, otherwise we get a different shape at large font sizes:

image

@richvdh
Copy link
Member Author

richvdh commented Sep 30, 2024

The key icon at the left of the new error is not aligned with the new error. Can we align it vertically?

Given this is a stop-gap while we wait for element-hq/element-meta#2523 to land, I'm not inclined to spend too much time finessing this.

I can use the compound tokens, where they exist, though.

Use compound colour tokens, and add a background colour.
@richvdh richvdh added this pull request to the merge queue Sep 30, 2024
Merged via the queue into develop with commit f28f1d9 Sep 30, 2024
27 checks passed
@richvdh richvdh deleted the rav/insecure_devices_error_messages branch September 30, 2024 12:58
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Oct 9, 2024
https://build.opensuse.org/request/show/1206523
by user dheidler + anag+factory
- Version 1.11.80
  ## ✨ Features
  * enable Element Call on desktop nightly (element-hq/element-desktop#1873). Contributed by @fkwp.
  * Add doc for 'force\_verification config option (element-hq/element-web#28035). Contributed by @dbkr.
  * Roll back change to device isolation mode (element-hq/matrix-react-sdk#104). Contributed by @richvdh.
  * Remove right panel toggling behaviour on room header buttons (element-hq/matrix-react-sdk#100). Contributed by @t3chguy.
  * Improve error display for messages sent from insecure devices (element-hq/matrix-react-sdk#93). Contributed by @richvdh.
  * Add labs option to exclude unverified devices (https://github.com/element-hq/matri
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Oct 9, 2024
https://build.opensuse.org/request/show/1206522
by user dheidler + anag+factory
- Version 1.11.80
  ## ✨ Features
  * Add doc for 'force\_verification config option (element-hq/element-web#28035). Contributed by @dbkr.
  * Roll back change to device isolation mode (element-hq/matrix-react-sdk#104). Contributed by @richvdh.
  * Remove right panel toggling behaviour on room header buttons (element-hq/matrix-react-sdk#100). Contributed by @t3chguy.
  * Improve error display for messages sent from insecure devices (element-hq/matrix-react-sdk#93). Contributed by @richvdh.
  * Add labs option to exclude unverified devices (element-hq/matrix-react-sdk#92). Contributed by @richvdh.
  * Improve contrast for timestamps, date separators \& spotlight trigger (ht
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants