Feature : room settings - security and privacy #4212

ganfra · 2025-01-28T20:14:19Z

Content

This PR introduce the new screens of security and privacy, it includes the following change :

Room details reorganisation
New Security & Privacy screen
New Edit Room Address screen

It includes also the necessary changes from the sdk allowing to update and listen those settings.

Motivation and context

Figma : https://www.figma.com/design/7TqjqdMBaPpm3IavKsMYu6/Ask-to-join-(Knocking)?node-id=5147-58412&m=dev
Meta issue : element-hq/element-meta#2631

Screenshots / GIFs

Tests

Make sure knock requests are enabled
Open a non DM room where you have some moderator/admin rights
Open details/ security and privacy
Play with all the settings and hit save

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 24
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
You've made a self review of your PR

…tion

…rename alias to canonicalAlias

…clickable

github-actions · 2025-01-28T20:26: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/XNfE2U

codecov · 2025-01-28T20:58:59Z

Codecov Report

Attention: Patch coverage is 88.18061% with 89 lines in your changes missing coverage. Please review.

Project coverage is 83.40%. Comparing base (d00bc30) to head (c10da01).
Report is 126 commits behind head on develop.

Files with missing lines	Patch %	Lines
.../securityandprivacy/SecurityAndPrivacyPresenter.kt	78.01%	10 Missing and 21 partials ⚠️
.../impl/securityandprivacy/SecurityAndPrivacyView.kt	88.54%	9 Missing and 13 partials ⚠️
...atrix/ui/room/address/RoomAddressValidityEffect.kt	73.68%	2 Missing and 3 partials ⚠️
...droid/features/roomdetails/impl/RoomDetailsView.kt	90.47%	0 Missing and 4 partials ⚠️
.../securityandprivacy/SecurityAndPrivacyNavigator.kt	0.00%	4 Missing ⚠️
.../features/roomdetails/impl/RoomDetailsPresenter.kt	78.57%	0 Missing and 3 partials ⚠️
...rivacy/editroomaddress/EditRoomAddressPresenter.kt	95.00%	3 Missing ⚠️
...yandprivacy/editroomaddress/EditRoomAddressView.kt	93.61%	1 Missing and 2 partials ⚠️
...atures/createroom/impl/configureroom/RoomAccess.kt	0.00%	2 Missing ⚠️
...impl/securityandprivacy/SecurityAndPrivacyState.kt	94.59%	1 Missing and 1 partial ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4212      +/-   ##
===========================================
+ Coverage    83.31%   83.40%   +0.09%     
===========================================
  Files         1881     1896      +15     
  Lines        49101    49752     +651     
  Branches      5769     5856      +87     
===========================================
+ Hits         40909    41498     +589     
- Misses        6116     6139      +23     
- Partials      2076     2115      +39

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

jmartinesp

LGTM in general, although I have some worries about the error(...) being used while mapping some SDK data.

There are a few nits that I pointed out, but it's mostly naming and asking for more docs/comments, so you can ignore them if you don't agree.

Note I haven't tested the new features yet.

jmartinesp · 2025-01-29T07:38:57Z

...in/io/element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyView.kt

+            trailingContent = ListItemContent.RadioButton(selected = selected == SecurityAndPrivacyRoomAccess.Anyone),
+            onClick = { onSelected(SecurityAndPrivacyRoomAccess.Anyone) },
+        )
+        if (selected == SecurityAndPrivacyRoomAccess.SpaceMember) {


Maybe add some comments on why this is needed?

jmartinesp · 2025-01-29T07:40:11Z

...in/io/element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyView.kt

+        title = stringResource(CommonStrings.screen_security_and_privacy_room_history_section_header),
+        modifier = modifier,
+    ) {
+        Spacer(Modifier.height(16.dp))


Is this spacer needed? Also, it seems larger than spacers in other sections, at least it seems larger in the screenshots.

jmartinesp · 2025-01-29T07:41:21Z

...in/io/element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyView.kt

+            supportingContent = { Text(text = stringResource(CommonStrings.screen_security_and_privacy_encryption_section_footer)) },
+            trailingContent = ListItemContent.Switch(
+                checked = isEncryptionEnabled,
+                enabled = !isEncryptionEnabled,


Maybe we could add a comment here to to explain encryption can't be disabled once enabled.

jmartinesp · 2025-01-29T08:19:00Z

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

+     * [Public](`RoomVisibility::Public`) rooms are listed in the room
+     * directory and can be found using it.


🦀 spotted!

jmartinesp · 2025-01-29T08:25:55Z

.../element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyPresenter.kt

+        is JoinRule.Custom,
+        JoinRule.Invite,
+        JoinRule.Private,
+        null -> SecurityAndPrivacyRoomAccess.InviteOnly


Maybe explain why all these are mapped to InviteOnly?

jmartinesp · 2025-01-29T08:29:44Z

.../element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyPresenter.kt

+        SecurityAndPrivacyRoomAccess.Anyone -> JoinRule.Public
+        SecurityAndPrivacyRoomAccess.AskToJoin -> JoinRule.Knock
+        SecurityAndPrivacyRoomAccess.InviteOnly -> JoinRule.Private
+        SecurityAndPrivacyRoomAccess.SpaceMember -> error("Unsupported")


I think this is used in a safe context at the moment (a runCatching wrapped block), but maybe we should be more careful about it? If we add some other usage in the future we might be crashing the app.

Also, documenting why this is not supported would be nice.

Changed to nullable

jmartinesp · 2025-01-29T08:36:53Z

...oid/features/roomdetails/impl/securityandprivacy/editroomaddress/EditRoomAddressPresenter.kt

+    if (this.alias?.matchesServer(serverName) == true) {
+        return this.alias
+    }
+    return this.alternativeAliases.firstOrNull { it.value.contains(serverName) }


Can't this use matchesServer too?

jmartinesp · 2025-01-29T08:49:09Z

...oid/features/roomdetails/impl/securityandprivacy/editroomaddress/EditRoomAddressPresenter.kt

+                    val newAlternativeAliases = room.alternativeAliases.filter { it != savedAliasFromHomeserver }
+                    room.updateCanonicalAlias(newRoomAlias, newAlternativeAliases).getOrThrow()
                }
-                // Otherwise, update the alternative aliases and keep the current canonical alias
+                // Otherwise, only update the alternative aliases and keep the current canonical alias
                else -> {
-                    val newAlternativeAliases = listOf(newRoomAlias) + room.alternativeAliases
+                    val newAlternativeAliases = buildList {
+                        add(newRoomAlias)
+                        for (alias in room.alternativeAliases) {
+                            if (alias != savedAliasFromHomeserver) {
+                                add(alias)
+                            }
+                        }
+                    }


These seem super difficult to follow for me, could we document the different cases?

Have simplified and added comment

jmartinesp · 2025-01-29T08:49:58Z

.../element/android/features/roomdetails/impl/securityandprivacy/SecurityAndPrivacyPresenter.kt

@@ -71,6 +71,9 @@ class SecurityAndPrivacyPresenter @AssistedInject constructor(
        var currentIsEncrypted by remember(savedSettings.isEncrypted) {
            mutableStateOf(savedSettings.isEncrypted)
        }
+
+        var showEncryptionConfirmation by remember { mutableStateOf(false) }


showEnableEncryptionConfirmation maybe?

bmarty

Thanks for this huge PR.

I have a concerned about enabling encryption UX which could be improved, maybe discussed with product? I have attached a video of the problem.

bmarty · 2025-01-29T09:50:02Z

...ls/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/RoomDetailsPresenter.kt

+        val securityAndPrivacyPermissions = room.securityAndPrivacyPermissionsAsState(syncUpdateFlow.value)
+        val canShowSecurityAndPrivacy by remember {
+            derivedStateOf {
+                isKnockRequestsEnabled && roomType is RoomDetailsType.Room && securityAndPrivacyPermissions.value.hasAny


Strange to see this new screen behind isKnockRequestsEnabled since it also allow to for instance enable encryption IIUC.

Yes this has been discussed, and for now this is what we want...

bmarty · 2025-01-29T10:02:08Z

...hots/images/features.roomdetails.impl.securityandprivacy_SecurityAndPrivacyViewDark_6_en.png

Encryption will not be enabled until user press the "Save" button. So maybe show this popup when the user press the Save button?

This is really a problem because the user will strongly think that the encryption is enabled after this popup is closed, and this is enforced by the fact that there is no confirmation when leaving the screen without saving the change.

See the video below:

EnableEncryptionMisleading.mp4

Yes it's a bit misleading, but this is what product/design want.

@mxandreas can you see my video ^? And confirm that this is fine. The risk is that user may think they have enabled encryption and this will actually no be the case until the save button has been pressed. Thanks.

Maybe adding a confirmation dialog when user leaves without saving pending changes can mitigate this.

I agree, it can be misleading and mainly due to the popup.

Maybe adding a confirmation dialog when user leaves without saving pending changes can mitigate this.

This is the only consistent/scalable option, I see but not sure how easy it is to be done. Since @gaelledel has been the one driving such details about interaction, I think she needs to make call about this to make sure the bigger picture remains consistent.

IMO, we can proceed of releasing this "as is" and apply an update/fix sooner/later, though.

bmarty · 2025-01-29T10:11:46Z

...ls/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/RoomDetailsPresenter.kt

+        val securityAndPrivacyPermissions = room.securityAndPrivacyPermissionsAsState(syncUpdateFlow.value)
+        val canShowSecurityAndPrivacy by remember {
+            derivedStateOf {
+                isKnockRequestsEnabled && roomType is RoomDetailsType.Room && securityAndPrivacyPermissions.value.hasAny


Small neat, securityAndPrivacyPermissions.value.hasAny seems not to be live, I was on the room details screen when I became admin of the room and the item "Security & Privacy" did not appear. I had to go back to the previous screen and come back to see it.

Yes it's not live, because powerlevels are not live

bmarty

Thanks for the answers to my comments.

I hope we will implement a "Leaving without saving" alert that would be good for the UX and is not hard to do technically speaking.

sonarqubecloud · 2025-01-29T16:19:47Z

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 SonarQube Cloud

ganfra added 26 commits January 17, 2025 17:44

feat(security&privacy) : first implementation of ui

002325c

change(room details) : update room details with new sections organisa…

d354144

…tion

feat(security & privacy) : introduce EditRoomAddress screen

a781cc0

feat(security&privacy) : expose new sdk methods

b549400

feat(security&privacy) : get data from sdk

c391b5b

feat(security&privacy) : introduce navigator

129eb45

feat(room address) : extract some reusable code

9faa305

feat(security&privacy) : start handling edition of room address

f8cd8b3

feat(security&privacy) : expose methods from sdk to update alias and …

b7831f4

…rename alias to canonicalAlias

feat(security&privacy) : manage save action for edit room address

65e5447

feat(privacy&security) : extract some code for address management

0e6c86f

feat(security&privacy) : update the save address algorithm

392299d

feat(security&privacy) : manage encryption settings

7eda945

feat(security&privacy) : expose more methods from sdk

19d49a3

feat(security&privacy) : manage save action and some edge cases.

edee18a

feat(security&privacy) : introduce permissions and use in RoomDetails

75fef6b

feat(security&privacy) : use permissions and improve save

88fce64

feat(security&privacy) : update strings

ba0a857

feat(security&privacy) : make the whole RoomDirectoryVisibility item …

c07a7d9

…clickable

feat(security&privacy) : start writing tests

fdc4f1b

feat(security&privacy) : write View tests

c0c7d5b

feat(security&privacy) : add all tests for EditRoomAddress classes

876d06f

feat(security&privacy) : clean code

d992f38

feat(security&privacy) : update canShowSecurityAndPrivacy check

58918b1

feat(security&privacy) : some more clean up

d586bdc

feat(security&privacy) : fix tests after some rework landed

706b298

ganfra added PR-Wip For anything that isn't ready to ship and will be enabled at a later date Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. labels Jan 28, 2025

ganfra marked this pull request as ready for review January 28, 2025 20:14

ganfra requested a review from a team as a code owner January 28, 2025 20:14

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

ganfra requested review from bmarty and removed request for a team January 28, 2025 20:14

Update screenshots

82f5926

jmartinesp approved these changes Jan 29, 2025

View reviewed changes

bmarty requested changes Jan 29, 2025

View reviewed changes

bmarty approved these changes Jan 29, 2025

View reviewed changes

ganfra added 2 commits January 29, 2025 14:16

feat(security&privacy) : improve and document code after PR review.

8880eed

feat(security&privacy) : add more previews

15f2d0b

ganfra added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 29, 2025

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

ganfra enabled auto-merge January 29, 2025 15:48

Update screenshots

c10da01

ganfra merged commit 346e364 into develop Jan 29, 2025
26 checks passed

ganfra deleted the feature/fga/room_settings_security_privacy branch January 29, 2025 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature : room settings - security and privacy #4212

Feature : room settings - security and privacy #4212

ganfra commented Jan 28, 2025

github-actions bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

jmartinesp left a comment

jmartinesp Jan 29, 2025

jmartinesp Jan 29, 2025

ganfra Jan 29, 2025

jmartinesp Jan 29, 2025

jmartinesp Jan 29, 2025

jmartinesp Jan 29, 2025

jmartinesp Jan 29, 2025

ganfra Jan 29, 2025

jmartinesp Jan 29, 2025

jmartinesp Jan 29, 2025

ganfra Jan 29, 2025

jmartinesp Jan 29, 2025

bmarty left a comment

bmarty Jan 29, 2025

ganfra Jan 29, 2025

bmarty Jan 29, 2025 •

edited

Loading

ganfra Jan 29, 2025

bmarty Jan 29, 2025

bmarty Jan 29, 2025 •

edited

Loading

mxandreas Jan 29, 2025

mxandreas Jan 29, 2025 •

edited

Loading

bmarty Jan 29, 2025 •

edited

Loading

ganfra Jan 29, 2025

bmarty left a comment

sonarqubecloud bot commented Jan 29, 2025

		* [Public](`RoomVisibility::Public`) rooms are listed in the room
		* directory and can be found using it.

Feature : room settings - security and privacy #4212

Feature : room settings - security and privacy #4212

Conversation

ganfra commented Jan 28, 2025

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Jan 28, 2025 • edited Loading

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

jmartinesp left a 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

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxandreas Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

bmarty Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 29, 2025

Quality Gate passed

github-actions bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

bmarty Jan 29, 2025 •

edited

Loading

bmarty Jan 29, 2025 •

edited

Loading

mxandreas Jan 29, 2025 •

edited

Loading

bmarty Jan 29, 2025 •

edited

Loading