[Device Manager] Unverified and inactive sessions list (PSG-698, PSG-696) #7171

onurays · 2022-09-19T13:53:16Z

Type of change

Feature
Bugfix
Technical
Other :

Content

From the main session management screen, clicking view all button of Security Recommendations section navigates to standalone session list screen with the correct filter applied by default.

Motivation and context

Fixes #7170.

Screenshots / GIFs

Unverified sessions	Inactive sessions

Tests

Enable VectorFeatures.isNewDeviceManagementEnabled
Navigate to Settings > Security & Privacy > Show All Sessions (V2, WIP)
Click on View All button under the Security Recommendations section
See the list of all sessions with the correct filter applied by default

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
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
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

mnaturel · 2022-09-20T14:03:46Z

...or/src/main/java/im/vector/app/features/settings/devices/v2/VectorSettingsDevicesFragment.kt

@@ -125,6 +128,29 @@ class VectorSettingsDevicesFragment :
        views.deviceListOtherSessions.callback = this
    }

+    private fun initSecurityRecommendationsView() {
+        views.deviceListUnverifiedSessionsRecommendation.callback = object : SecurityRecommendationView.Callback {


I think we should clean up the callback when the fragment is destroyed or when the SecurityRecommendationView is detached.

mnaturel · 2022-09-20T14:13:38Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/DevicesViewModel.kt

@@ -71,7 +71,7 @@ class DevicesViewModel @AssistedInject constructor(
                .execute { async ->
                    if (async is Success) {
                        val deviceFullInfoList = async.invoke()
-                        val unverifiedSessionsCount = deviceFullInfoList.count { !it.cryptoDeviceInfo?.isVerified.orFalse() }
+                        val unverifiedSessionsCount = deviceFullInfoList.count { !it.cryptoDeviceInfo?.trustLevel?.isCrossSigningVerified().orFalse() }


To be consistent, I am wondering if we should update the GetEncryptionTrustLevelForDeviceUseCase logic instead and rely on the RoomEncryptionTrustLevel of DeviceFullInfo for the verification status of a session. This field is used for example in SessionInfoView or in OtherSessionItem to render the verification status. What do you think?

We should definitely talk to crypto team (@BillCarsonFr) to be clear about trust levels.

mnaturel · 2022-09-20T14:14:47Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/filter/FilterDevicesUseCase.kt

@@ -31,8 +31,8 @@ class FilterDevicesUseCase @Inject constructor() {
                .filter {
                    when (filterType) {
                        DeviceManagerFilterType.ALL_SESSIONS -> true
-                        DeviceManagerFilterType.VERIFIED -> it.cryptoDeviceInfo?.isVerified.orFalse()
-                        DeviceManagerFilterType.UNVERIFIED -> !it.cryptoDeviceInfo?.isVerified.orFalse()
+                        DeviceManagerFilterType.VERIFIED -> it.cryptoDeviceInfo?.trustLevel?.isCrossSigningVerified().orFalse()


Same remark as a previous one, maybe we should rely on the RoomEncryptionTrustLevel field.

I will create a dedicated PR to refactor after talking to crypto team.

mnaturel · 2022-09-20T14:19:15Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/list/OtherSessionsController.kt

                    stringProvider([email protected])
                    clickListener { device.deviceInfo.deviceId?.let { host.callback?.onItemClicked(it) } }
                }
            }
        }
    }
+
+    private fun calculateDescription(device: DeviceFullInfo, formattedLastActivityDate: String): String {
+        return if (device.isInactive) {


I think it may be easier to read with a when {} pattern, what do you think?

Yep, better, done.

mnaturel · 2022-09-20T14:24:34Z

.../main/java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsActivity.kt

-            return Intent(context, OtherSessionsActivity::class.java)
+        fun newIntent(
+                context: Context,
+                titleResourceId: Int,


We may annotate this second argument with @StringRes.

mnaturel · 2022-09-20T14:25:21Z

.../src/main/java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsArgs.kt

+
+@Parcelize
+data class OtherSessionsArgs(
+        val titleResourceId: Int,


Same remark, @StringRes annotation can be added here.

mnaturel · 2022-09-20T14:28:08Z

.../main/java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsFragment.kt

@@ -57,6 +59,7 @@ class OtherSessionsFragment :

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
+        views.otherSessionsToolbar.title = getString(args.titleResourceId)
        setupToolbar(views.otherSessionsToolbar).allowBack()


I think you can call setTitle(args.titleResourceId) instead just right after setupToolbar() to configure the toolbar.

Cool, done.

mnaturel · 2022-09-20T14:31:22Z

...main/java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsViewModel.kt

@@ -55,7 +55,7 @@ class OtherSessionsViewModel @AssistedInject constructor(
        observeDevicesJob?.cancel()
        observeDevicesJob = getDeviceFullInfoListUseCase.execute(
                filterType = currentFilter,
-                excludeCurrentDevice = true
+                excludeCurrentDevice = !initialState.includeCurrentSession


Maybe we should always use the same pattern everywhere for this argument for consistency and to avoid errors? Either excludeCurrentDevice or includeCurrentDevice?

Make sense, done.

mnaturel · 2022-09-20T14:36:03Z

vector/src/test/java/im/vector/app/features/settings/devices/v2/DevicesViewModelTest.kt

@@ -57,6 +63,17 @@ class DevicesViewModelTest {
        )
    }

+    @Before


Do we really need this mock on SystemClock?

It is somehow failing on my machine. I didn't see a harm to mock it.

mnaturel · 2022-09-20T14:37:15Z

vector/src/test/java/im/vector/app/features/settings/devices/v2/DevicesViewModelTest.kt

@@ -164,20 +181,24 @@ class DevicesViewModelTest {
    private fun givenDeviceFullInfoList(): List<DeviceFullInfo> {
        val verifiedCryptoDeviceInfo = mockk<CryptoDeviceInfo>()
        every { verifiedCryptoDeviceInfo.isVerified } returns true


Maybe we should remove the isVerified mocks since we use trustLevel instead?

Right, done.

mnaturel

I have added some comments to improve readability and maintainability. I have tested it and it is working well.

Is it confirmed we can show the filter option when navigating from security recommendation views?

onurays · 2022-09-21T10:13:50Z

I have added some comments to improve readability and maintainability. I have tested it and it is working well.

Is it confirmed we can show the filter option when navigating from security recommendation views?

Thank you for your quick review. No answer yet about the filter icon.

sonarcloud · 2022-09-23T13:58:41Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

30.0% Coverage
0.0% Duplication

Onuray Sahin added 5 commits September 19, 2022 13:42

Make other sessions view dynamic.

740b69d

Render security recommendation sessions.

7db222a

Fix unverified session detection logic.

ec9843f

Fix unit tests.

2ad0cd4

Add changelog.

72cc0de

onurays requested review from a team, fedrunov and mnaturel and removed request for a team September 19, 2022 13:53

onurays marked this pull request as ready for review September 19, 2022 13:54

Fix unit test.

3ea054f

mnaturel reviewed Sep 20, 2022

View reviewed changes

Merge branch 'develop' into feature/ons/device_manager_security_sessions

45cf7dc

mnaturel reviewed Sep 20, 2022

View reviewed changes

mnaturel mentioned this pull request Sep 20, 2022

[Device management] Verify current session (PSG-722) #7139

Merged

15 tasks

mnaturel reviewed Sep 20, 2022

View reviewed changes

Onuray Sahin added 2 commits September 21, 2022 14:16

Code review fixes.

4ab798f

Code review fixes.

8de9ef8

mnaturel approved these changes Sep 22, 2022

View reviewed changes

Merge branch 'develop' into feature/ons/device_manager_security_sessions

7cd30a1

onurays merged commit 6c79aae into develop Sep 23, 2022

onurays deleted the feature/ons/device_manager_security_sessions branch September 23, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Device Manager] Unverified and inactive sessions list (PSG-698, PSG-696) #7171

[Device Manager] Unverified and inactive sessions list (PSG-698, PSG-696) #7171

onurays commented Sep 19, 2022 •

edited

Loading

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel Sep 20, 2022

onurays Sep 20, 2022

mnaturel Sep 20, 2022

onurays Sep 21, 2022

mnaturel left a comment

onurays commented Sep 21, 2022

sonarcloud bot commented Sep 23, 2022

@@ @@ -57,6 +63,17 @@ class DevicesViewModelTest { @@
                       )
                   }
+                  @Before

[Device Manager] Unverified and inactive sessions list (PSG-698, PSG-696) #7171

[Device Manager] Unverified and inactive sessions list (PSG-698, PSG-696) #7171

Conversation

onurays commented Sep 19, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

onurays commented Sep 21, 2022

sonarcloud bot commented Sep 23, 2022

onurays commented Sep 19, 2022 •

edited

Loading