Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. #3461

ganfra · 2024-09-13T14:54:13Z

Content

This PR makes the following changes:

Build clients with the requirements to fail when sending to once verified users who aren't (fully) verified any more
Show a description of the send failures for verified users in the ActionListView
Add a bottom sheet to resolve failures for verified users by tapping on the description above, or by tapping on the failure in the timeline.
If there is more than one user ID in the failure, the sheet iterates through them 1 by 1, resolving each one and then dismissing itself at the end.

Motivation and context

Closes #3459
Closes #3460

Screenshots / GIFs

Tests

Makes sure to verify a user with another client
Reset the security or login with a new device for this verified user
Send a message with EX in an encrypted room with this user
See the message failing to send
Click on the error icon or long click on the item
See the bottomsheet and play with the options

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

…ise as you want.

github-actions · 2024-09-13T15:03:52Z

📱 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/zKE1En

codecov · 2024-09-13T16:29:53Z

Codecov Report

Attention: Patch coverage is 88.72832% with 39 lines in your changes missing coverage. Please review.

Project coverage is 82.67%. Comparing base (da3f5e0) to head (be3ead0).
Report is 16 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ypto/sendfailure/VerifiedUserSendFailureFactory.kt	50.00%	2 Missing and 6 partials ⚠️
...lure/resolve/ResolveVerifiedUserSendFailureView.kt	89.06%	1 Missing and 6 partials ⚠️
...failure/resolve/VerifiedUserSendFailureResolver.kt	82.75%	1 Missing and 4 partials ⚠️
...resolve/ResolveVerifiedUserSendFailurePresenter.kt	87.87%	1 Missing and 3 partials ⚠️
...failure/resolve/VerifiedUserSendFailureIterator.kt	80.95%	2 Missing and 2 partials ⚠️
...eatures/messages/impl/actionlist/ActionListView.kt	90.90%	0 Missing and 3 partials ⚠️
...essages/impl/pinned/list/PinnedMessagesListView.kt	50.00%	2 Missing ⚠️
...atures/messages/impl/timeline/TimelinePresenter.kt	60.00%	2 Missing ⚠️
...es/messages/impl/actionlist/ActionListPresenter.kt	80.00%	0 Missing and 1 partial ⚠️
.../impl/timeline/components/ATimelineItemEventRow.kt	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3461      +/-   ##
===========================================
+ Coverage    82.62%   82.67%   +0.05%     
===========================================
  Files         1701     1710       +9     
  Lines        40015    40303     +288     
  Branches      4868     4913      +45     
===========================================
+ Hits         33061    33322     +261     
- Misses        5233     5237       +4     
- Partials      1721     1744      +23

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

jmartinesp

Thanks for the code, it works pretty well! I have a few comments, but none are either really important or urgent.

jmartinesp · 2024-09-16T08:25:52Z

.../src/main/kotlin/io/element/android/features/messages/impl/actionlist/ActionListPresenter.kt

+            is LocalEventSendState.Failed.VerifiedUserHasUnsignedDevice -> {
+                val userId = sendState.devices.keys.firstOrNull()
+                if (userId == null) {
+                    ActionListState.VerifiedUserSendFailure.None


Is this OK? I guess it shouldn't ever happen except if there is some bug in the SDK, but maybe we should still display the UI with some placeholder value and log the issue?

jmartinesp · 2024-09-16T08:27:04Z

.../src/main/kotlin/io/element/android/features/messages/impl/actionlist/ActionListPresenter.kt

+            is LocalEventSendState.Failed.VerifiedUserChangedIdentity -> {
+                val userId = sendState.users.firstOrNull()
+                if (userId == null) {
+                    ActionListState.VerifiedUserSendFailure.None


Same here, whether we decide to treat this as 'the SDK failed to provide some user but the status is right' or 'the status is wrong since there is no user' I think we should at least log these issues.

jmartinesp · 2024-09-16T08:29:57Z

libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt

+    suspend fun ignoreDeviceTrustAndResend(devices: Map<UserId, List<DeviceId>>, transactionId: TransactionId): Result<Unit>
+    suspend fun withdrawVerificationAndResend(userIds: List<UserId>, transactionId: TransactionId): Result<Unit>


Could we add some docs here? I'm not really sure what 'withdraw verification' means in this context.

jmartinesp · 2024-09-16T08:39:23Z

...roid/features/messages/impl/crypto/sendfailure/resolve/ResolveVerifiedUserSendFailureView.kt

+            id = CommonStrings.screen_resolve_send_failure_changed_identity_title,
+            userDisplayName
+        )
+        VerifiedUserSendFailure.None -> ""


Maybe these should be error("This method should never be called for this state")? Up to you, it would allow us to catch any regressions faster (and maybe make some users angry 😅 ).

jmartinesp · 2024-09-16T08:40:22Z

...android/features/messages/impl/crypto/sendfailure/resolve/VerifiedUserSendFailureResolver.kt

+import io.element.android.libraries.matrix.api.timeline.item.event.LocalEventSendState
+import timber.log.Timber
+
+class VerifiedUserSendFailureResolver(


Some docs, maybe?

sonarcloud · 2024-09-16T13:30:06Z

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

ElementBot · 2024-09-16T13:48:27Z

	Messages
📖	This pull request seems relatively large. Please consider splitting it into multiple smaller ones.
📖	Sign-off not required, allow-list

Generated by 🚫 dangerJS against be3ead0

ganfra added 8 commits September 13, 2024 16:40

Send failure verified user : set the room key recipient strategy

d22d9f0

Design : allow passing ListItemColor to ListItem composable to custom…

b2c7ea0

…ise as you want.

Send failure verified user : display in ActionListView

e0bc026

Send failure verified user : expose new methods on MatrixRoom.

de933b1

Send state : introduce Failed.VerifiedUser interface.

416810a

Send failure verified user : resolve ui and logic

ff368b4

Send failure verified user : handle click on failed send icon.

e16adb9

Send failure verified user : set the room key recipient strategy

b245c69

ganfra added PR-Change For updates to an existing feature Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. labels Sep 13, 2024

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

Update screenshots

16bf8f0

ganfra marked this pull request as ready for review September 13, 2024 15:07

ganfra requested a review from a team as a code owner September 13, 2024 15:07

ganfra requested review from jmartinesp and removed request for a team September 13, 2024 15:07

Send failure verified user : fix tests.

aeea991

jmartinesp approved these changes Sep 16, 2024

View reviewed changes

Send failure verified user : add some comments and logs.

be3ead0

ganfra changed the title ~~Feature/fga/send failure identity changes~~ Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. Sep 16, 2024

ganfra added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Sep 16, 2024

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

ganfra merged commit 47d0c50 into develop Sep 16, 2024
30 checks passed

ganfra deleted the feature/fga/send_failure_identity_changes branch September 16, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. #3461

Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. #3461

ganfra commented Sep 13, 2024

github-actions bot commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading

jmartinesp left a comment

jmartinesp Sep 16, 2024

jmartinesp Sep 16, 2024

jmartinesp Sep 16, 2024

jmartinesp Sep 16, 2024

jmartinesp Sep 16, 2024

sonarcloud bot commented Sep 16, 2024

ElementBot commented Sep 16, 2024

		suspend fun ignoreDeviceTrustAndResend(devices: Map<UserId, List<DeviceId>>, transactionId: TransactionId): Result<Unit>
		suspend fun withdrawVerificationAndResend(userIds: List<UserId>, transactionId: TransactionId): Result<Unit>

Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. #3461

Require acknowledgement to send to a verified user if their identity changed or if a device is unverified. #3461

Conversation

ganfra commented Sep 13, 2024

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Sep 13, 2024 • edited Loading

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

jmartinesp left a comment

Choose a reason for hiding this comment

jmartinesp Sep 16, 2024

Choose a reason for hiding this comment

jmartinesp Sep 16, 2024

Choose a reason for hiding this comment

jmartinesp Sep 16, 2024

Choose a reason for hiding this comment

jmartinesp Sep 16, 2024

Choose a reason for hiding this comment

jmartinesp Sep 16, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Sep 16, 2024

Quality Gate passed

ElementBot commented Sep 16, 2024

github-actions bot commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading