Add invisible decryption config flag and show invisible crypto decryption errors #50

uhoreg · 2024-09-18T04:35:55Z

Depends on matrix-org/matrix-js-sdk#4407

Adds an "invisible crypto" setting. When enabled, sets the crypto mode in the client crypto API. Shows the appropriate error message when returned from the crypto API.

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

uhoreg · 2024-09-18T05:05:41Z

Ah. Forgot that SonarCloud doesn't take into account the end-to-end tests, so I'll have to add jest tests.

uhoreg · 2024-09-18T05:11:10Z

src/components/views/messages/DecryptionFailureBody.tsx

+            return _t("timeline|decryption_failure|sender_identity_previously_verified");
+
+        case DecryptionFailureCode.UNSIGNED_SENDER_DEVICE:
+            // TODO: event should be hidden instead of showing this error (only


I think this needs to be done by checking the decryption failure code in EventTile.tsx, and if it is this code, then render an empty tile?

Probably, but I'd like us to kick this down the road for a bit. Let's display the error for now and think about hiding them later.

(it's the same as HISTORICAL_MESSAGE_USER_NOT_JOINED)

richvdh · 2024-09-18T10:59:28Z

res/css/views/messages/_DecryptionFailureBody.pcss

@@ -10,3 +10,7 @@ Please see LICENSE files in the repository root for full details.
    color: $secondary-content;
    font-style: italic;
 }
+
+.mx_DecryptionFailureVerifiedIdentityChanged {
+    color: red;


I feel like we shouldn't be hardcoding colors like this

richvdh · 2024-09-18T12:08:26Z

src/components/views/messages/DecryptionFailureBody.tsx

 // A placeholder element for messages that could not be decrypted
 export const DecryptionFailureBody = forwardRef<HTMLDivElement, IBodyProps>(({ mxEvent }, ref): React.JSX.Element => {
    const verificationState = useContext(LocalDeviceVerificationStateContext);
    return (
-        <div className="mx_DecryptionFailureBody mx_EventTile_content" ref={ref}>
+        <div className={"mx_DecryptionFailureBody mx_EventTile_content" + getErrorExtraClass(mxEvent)} ref={ref}>


we should use classnames here rather than doing string manipulation.

richvdh · 2024-09-18T12:18:18Z

src/settings/Settings.tsx

+    "feature_invisible_crypto": {
+        isFeature: true,
+        labsGroup: LabGroup.Encryption,
+        controller: new InvisibleCryptoController(),
+        displayName: _td("labs|invisible_crypto"),
+        description: _td("labs|invisible_crypto_description"),
+        supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED,
+        supportedLevelsAreOrdered: true,
+        default: false,
+    },


I think we probably need two settings -- one for "invisible crypto", and one for "transition mode" ?

Yeah, I wasn't sure how best to represent this. Having two separate toggles for three levels seems wrong (e.g. what do we do if both "invisible" and "transition" are enabled). I think that ideally, this would be a dropdown or something that would allow us to select one of the three levels, but I don't how to do that in settings, or if it's even possible with feature flags. I think that if it isn't possible for feature flags, my second preference would be for the feature flag to be to enable "new cryptography modes" or some such, which would enable a selector somewhere else to select the mode. But, basically, I didn't have time to sufficiently investigate.

Yeah, I suspect probably a custom labs flag that's actually a drop down 9and maybe not actually a labs flag but manually put into the labs section?) That or a labs flag to enable another setting in the crypto section, although that risks people not realising they've turned the labs flag on.

Either way, would you want this PR is as it is with no option for transition mode or do we want to wait?

richvdh · 2024-09-18T15:11:02Z

src/components/views/messages/DecryptionFailureBody.tsx

could we maybe move the changes to DecryptionFailureBody into a separate PR?

So, one PR for the config setting, and one PR for the display changes? Possibly, but I suspect that the config setting PR will have a lack of code worth testing, which would cause SonarCloud to complain. I'll rewrite this PR so that it has two commits - one for the config setting and one for the display changes, so that it can be split into two PRs.

I discovered some tests for settings, and added one for invisible crypto. So maybe SonarCloud would be happy with a PR for just the setting. But I've run out of time to split it into a separate PR. But it is now in two commits.

uhoreg · 2024-09-18T23:02:15Z

A couple of issue with this PR:

we don't retry decryption after we've verified a user. I've created [invisible cryprto] re-try decrypting messages after we've verified a user (or withdraw verification) #58 to track the issue.
we don't have any special handling for the display in the roomlist message preview, or the thread preview in the main timeline. So we end up with things like

I've asked for designs for these at Invisible crypto: display message content instead of an error, when the sender's verified identity changed element-meta#2523 (comment) so we may want to just wait until we implement Invisible crypto: display message content instead of an error, when the sender's verified identity changed element-meta#2523 to address this.

dbkr

Seems plausible otherwise

dbkr · 2024-09-19T10:28:50Z

src/settings/Settings.tsx

+    "feature_invisible_crypto": {
+        isFeature: true,
+        labsGroup: LabGroup.Encryption,
+        controller: new InvisibleCryptoController(),
+        displayName: _td("labs|invisible_crypto"),
+        description: _td("labs|invisible_crypto_description"),
+        supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED,
+        supportedLevelsAreOrdered: true,
+        default: false,
+    },


Yeah, I suspect probably a custom labs flag that's actually a drop down 9and maybe not actually a labs flag but manually put into the labs section?) That or a labs flag to enable another setting in the crypto section, although that risks people not realising they've turned the labs flag on.

Either way, would you want this PR is as it is with no option for transition mode or do we want to wait?

uhoreg added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Sep 18, 2024

github-actions bot requested a deployment to Netlify September 18, 2024 04:38 In progress

uhoreg force-pushed the invisible_crypto_decryption branch 2 times, most recently from 034cedc to e1081e4 Compare September 18, 2024 04:56

uhoreg force-pushed the invisible_crypto_decryption branch from e1081e4 to 9afdc75 Compare September 18, 2024 05:10

uhoreg commented Sep 18, 2024

View reviewed changes

github-actions bot deployed to Netlify September 18, 2024 05:26 View deployment

github-actions bot deployed to EndToEndTests September 18, 2024 05:26 View deployment

richvdh reviewed Sep 18, 2024

View reviewed changes

richvdh requested changes Sep 18, 2024

View reviewed changes

github-actions bot deployed to Netlify September 18, 2024 22:38 View deployment

github-actions bot deployed to EndToEndTests September 18, 2024 22:38 View deployment

uhoreg added 2 commits September 18, 2024 18:39

add setting to enable invisible crypto mode in js-sdk

9420138

display invisible crypto decryption errors

c2fc124

uhoreg force-pushed the invisible_crypto_decryption branch from de8c5fb to c2fc124 Compare September 18, 2024 22:43

uhoreg requested a review from richvdh September 18, 2024 22:50

uhoreg mentioned this pull request Sep 18, 2024

[invisible cryprto] re-try decrypting messages after we've verified a user (or withdraw verification) #58

Open

github-actions bot deployed to Netlify September 18, 2024 22:58 View deployment

github-actions bot deployed to EndToEndTests September 18, 2024 22:59 View deployment

uhoreg marked this pull request as ready for review September 18, 2024 23:02

uhoreg requested a review from a team as a code owner September 18, 2024 23:02

uhoreg requested a review from dbkr September 18, 2024 23:02

dbkr reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add invisible decryption config flag and show invisible crypto decryption errors #50

Add invisible decryption config flag and show invisible crypto decryption errors #50

uhoreg commented Sep 18, 2024 •

edited

Loading

uhoreg commented Sep 18, 2024

uhoreg Sep 18, 2024

richvdh Sep 18, 2024 •

edited

Loading

richvdh Sep 18, 2024

richvdh Sep 18, 2024

richvdh Sep 18, 2024

uhoreg Sep 18, 2024

dbkr Sep 19, 2024

richvdh Sep 18, 2024

uhoreg Sep 18, 2024

uhoreg Sep 18, 2024

uhoreg commented Sep 18, 2024

dbkr left a comment

dbkr Sep 19, 2024

Add invisible decryption config flag and show invisible crypto decryption errors #50

Are you sure you want to change the base?

Add invisible decryption config flag and show invisible crypto decryption errors #50

Conversation

uhoreg commented Sep 18, 2024 • edited Loading

Checklist

uhoreg commented Sep 18, 2024

Choose a reason for hiding this comment

richvdh Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Sep 18, 2024

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Sep 18, 2024 •

edited

Loading

richvdh Sep 18, 2024 •

edited

Loading