Warn the user when unverified user has changed their identity #3621

bmarty · 2024-10-08T07:47:48Z

Content

Using matrix-org/matrix-rust-sdk#4022 and matrix-org/matrix-rust-sdk#4068

Draft since the application is crashing for now when clicking on OK to pin the user. EDIT The crash has been fixed in matrix-org/matrix-rust-sdk#4089 so an SDK release will be neeeded, but this is not blocking this PR anymore (pinning identity cannot be tested, but the PR can be reviewed).

Motivation and context

element-hq/element-meta#2544

Screenshots / GIFs

Tests

Alice and Bob in a e2e room
Alice has not verified Bob
Messages can be sent
Bob reset their identity
Alice should see the banner, until they click on OK to pin the identity of Bob (but currently this is crashing)

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

…n the Url in any installed browser.

github-actions · 2024-10-08T08:34:07Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/TgzKg7

bmarty · 2024-10-08T09:51:45Z

...ures/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt

            }
            is PermalinkData.FallbackLink,
            is PermalinkData.RoomEmailInviteLink -> {
-                context.openUrlInExternalApp(url)
+                activity.openUrlInChromeCustomTab(null, darkTheme, url)


Note: at some point, direct usage of openUrlInExternalApp should be forbidden, but this is out of scope of this PR.

jmartinesp

LGTM, just a couple of suggestions.

jmartinesp · 2024-10-08T10:25:24Z

...in/io/element/android/features/messages/impl/crypto/identity/IdentityChangeStatePresenter.kt

+) : Presenter<IdentityChangeState> {
+    @Composable
+    override fun present(): IdentityChangeState {
+        val roomMemberIdentityStateChange = remember {


Couldn't this just use produceState?

Yes this would make more sense. Also use by delegate when possible?

jmartinesp · 2024-10-08T10:31:27Z

.../kotlin/io/element/android/features/messages/impl/crypto/identity/IdentityChangeStateView.kt

+    modifier: Modifier = Modifier,
+) {
+    // Pick the first identity change to PinViolation
+    val identityChange = state.roomMemberIdentityStateChanges.firstOrNull {


Maybe rename it to something like dangerousIdentityChange or pinViolationIdentityChange?

ganfra · 2024-10-08T11:36:10Z

...in/io/element/android/features/messages/impl/crypto/identity/IdentityChangeStatePresenter.kt

+
+        return IdentityChangeState(
+            roomMemberIdentityStateChanges = roomMemberIdentityStateChange.value
+                .filter { it.roomMember.userId !in ignoredUserIdChange.value }


I guess the list will never be large, but we are filtering on the main thread for every composition ^

This code does not exist anymore, are you reviewing an old version?

ganfra · 2024-10-08T11:38:57Z

...main/kotlin/io/element/android/features/messages/impl/crypto/identity/IdentityChangeState.kt

+)
+
+data class RoomMemberIdentityStateChange(
+    val roomMember: RoomMember,


Could we create a specific data class instead of passing RoomMember if we only need a few fields?

…the risk of useless recomposition. Also remove TypingNotificationStateForMessagesProvider which was not used anymore.

bmarty · 2024-10-08T13:04:36Z

@ganfra @jmartinesp thanks for the fast review. I think I have handled all your remarks.
I took the opportunity to update the code regarding typing notification as it had similar problems.

sonarqubecloud · 2024-10-08T13:11:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov · 2024-10-08T13:26:46Z

Codecov Report

Attention: Patch coverage is 90.73171% with 19 lines in your changes missing coverage. Please review.

Project coverage is 82.70%. Comparing base (409564a) to head (873d807).
Report is 27 commits behind head on develop.

Files with missing lines	Patch %	Lines
...pl/crypto/identity/IdentityChangeStatePresenter.kt	85.00%	2 Missing and 4 partials ⚠️
...es/impl/crypto/identity/IdentityChangeStateView.kt	87.50%	2 Missing and 2 partials ⚠️
...res/messages/impl/typing/TypingNotificationView.kt	60.00%	2 Missing ⚠️
...ment/android/features/messages/impl/utils/Emoji.kt	0.00%	1 Missing and 1 partial ⚠️
...ent/android/features/messages/impl/MessagesView.kt	83.33%	0 Missing and 1 partial ⚠️
...mpl/crypto/identity/IdentityChangeStateProvider.kt	95.65%	1 Missing ⚠️
...essages/impl/typing/TypingNotificationPresenter.kt	88.88%	0 Missing and 1 partial ⚠️
.../libraries/androidutils/browser/ChromeCustomTab.kt	0.00%	1 Missing ⚠️
...gnsystem/atomic/molecules/ComposerAlertMolecule.kt	97.22%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3621      +/-   ##
===========================================
+ Coverage    82.65%   82.70%   +0.04%     
===========================================
  Files         1735     1744       +9     
  Lines        41187    41334     +147     
  Branches      5000     5016      +16     
===========================================
+ Hits         34043    34185     +142     
+ Misses        5380     5376       -4     
- Partials      1764     1773       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty and others added 8 commits October 7, 2024 22:15

Add component ComposerAlertMolecule

81fc52d

Render PinViolation above the composer.

c69e5f4

Pin user identity.

d9fb0cf

Do not inject presenter directly.

9a85e92

Rename SecureBackupConfig to LearnMoreConfig

2d08c64

Avoid using application context.

3e5dd4b

Do what the doc says: if no CustomChrome tab is available, try to ope…

f998d5b

…n the Url in any installed browser.

Identity change: handle click on "learn more"

5baefd4

bmarty added the PR-Feature For a new feature label Oct 8, 2024

Fix compilation issues.

bc62f8c

bmarty added 3 commits October 8, 2024 11:09

Fix code quality.

0099777

Fix konsist test.

8c018c4

Improve code.

e49d689

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Oct 8, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Oct 8, 2024

bmarty added 2 commits October 8, 2024 11:48

Fix broken previews

33bfe1d

Fix preview of identity change banner in a timeline.

0fd275d

bmarty force-pushed the feature/bma/composerAlert branch from 9e01997 to 0fd275d Compare October 8, 2024 09:48

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Oct 8, 2024

bmarty marked this pull request as ready for review October 8, 2024 09:50

bmarty requested a review from a team as a code owner October 8, 2024 09:50

bmarty requested review from jmartinesp and removed request for a team October 8, 2024 09:50

bmarty commented Oct 8, 2024

View reviewed changes

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Oct 8, 2024

Update screenshots

84fd25c

jmartinesp approved these changes Oct 8, 2024

View reviewed changes

ganfra reviewed Oct 8, 2024

View reviewed changes

bmarty added 8 commits October 8, 2024 13:45

Use produceState

189fbe9

Rename val for clarity

fcee53b

tom

746a7fb

Fix Emoji test

4fefecd

Use produceState

8aa34d8

Create data classes TypingRoomMember and IdentityRoomMember to avoid …

41749ed

…the risk of useless recomposition. Also remove TypingNotificationStateForMessagesProvider which was not used anymore.

Fix regression.

f4b7443

Cleanup

873d807

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 8, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Oct 8, 2024

bmarty added the X-Release-Blocker label Oct 8, 2024

bmarty enabled auto-merge October 8, 2024 13:34

bmarty disabled auto-merge October 8, 2024 14:33

bmarty merged commit 0ffd787 into develop Oct 8, 2024
31 of 33 checks passed

bmarty deleted the feature/bma/composerAlert branch October 8, 2024 14:33

bmarty added Z-NextRelease For issues and PRs which should be included in the NextRelease. and removed X-Release-Blocker labels Oct 8, 2024

manuroe mentioned this pull request Oct 8, 2024

Invisible Crypto: EX Android: display a warning when an *unverified* user changes identity element-hq/element-meta#2524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn the user when unverified user has changed their identity #3621

Warn the user when unverified user has changed their identity #3621

bmarty commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024 •

edited

Loading

bmarty Oct 8, 2024

jmartinesp left a comment

jmartinesp Oct 8, 2024

ganfra Oct 8, 2024

jmartinesp Oct 8, 2024

ganfra Oct 8, 2024

bmarty Oct 8, 2024

ganfra Oct 8, 2024

bmarty commented Oct 8, 2024

sonarqubecloud bot commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

Warn the user when unverified user has changed their identity #3621

Warn the user when unverified user has changed their identity #3621

Conversation

bmarty commented Oct 8, 2024 • edited Loading

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Oct 8, 2024 • edited Loading

bmarty Oct 8, 2024

Choose a reason for hiding this comment

jmartinesp left a comment

Choose a reason for hiding this comment

jmartinesp Oct 8, 2024

Choose a reason for hiding this comment

ganfra Oct 8, 2024

Choose a reason for hiding this comment

jmartinesp Oct 8, 2024

Choose a reason for hiding this comment

ganfra Oct 8, 2024

Choose a reason for hiding this comment

bmarty Oct 8, 2024

Choose a reason for hiding this comment

ganfra Oct 8, 2024

Choose a reason for hiding this comment

bmarty commented Oct 8, 2024

sonarqubecloud bot commented Oct 8, 2024

Quality Gate passed

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

bmarty commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading