Reapply push rules on the decrypted event source (PSG-1146) #8170

Florian14 · 2023-02-24T08:48:47Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Reapply the local push rules on the decrypted event content.

Motivation and context

When the notifications were enabled for encrypted events, we did not reapply the push rules on the decrypted content, so all the encrypted events were triggering a notification even if the notifications for Direct Messages or Group Message were enabled.

Screenshots / GIFs

Tests

Enable all the default notifications from the app settings (DM, Group Messages, encrypted DM, encrypted Group Messages)
Put the app in bg
Send a message in encrypted DM & encrypted room (> 2 members) from another account
Verify that you receive both notifications
Disable DM, go in bg & send a message from the other account in both rooms
Verify there is a single notification for group messages
Enable DM & disable encrypted DM
Test again and verify that you have the same result
Do the same tests for group messages (you should only receive notifications in DM)

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()

Florian14 · 2023-02-24T09:00:08Z

Assigned to @BillCarsonFr manually, since you added the initial TODO and worked on this feature

BillCarsonFr

I think there is an issue with that code

BillCarsonFr · 2023-03-01T08:21:31Z

...dk-android/src/main/java/org/matrix/android/sdk/api/session/pushrules/EventMatchCondition.kt

@@ -39,8 +39,8 @@ class EventMatchCondition(
    override fun technicalDescription() = "'$key' matches '$pattern'"

    fun isSatisfied(event: Event): Boolean {
-        // TODO encrypted events?
-        val rawJson = MoshiProvider.providesMoshi().adapter(Event::class.java).toJsonValue(event) as? Map<*, *>
+        val rawJson: Map<*, *> = event.mxDecryptionResult?.payload


This doesn't looks correct to me. The key "path" is based on the full event content.
If you only take the payload it won't work as expected.
Eg the key content.body won't work because your raw json will be

{ "body": "xxxx" }

Instead of

{ "content" : {{ "body": "xxxx" }} "

What you would need to do is to take the rawJson, then in case of encrypted event (with a decryption result) you replace the typeand content with what is in event.mxDecryptionResult.

There is an existing test class for this condition you can add test to check that it's working as expected

I updated with your suggestion, I replace all the entries from the encrypted raw json with the decrypted one. Also, I had to ensure the event is decrypted before applying the push rules. It was not necessary when I tested last time but it seems that the event is not always decrypted at this moment (it was not the case currently).

…hing condition

BillCarsonFr · 2023-03-01T11:02:50Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/EventDecryptor.kt

@@ -94,6 +94,7 @@ internal class EventDecryptor @Inject constructor(
     * @param timeline the id of the timeline where the event is decrypted. It is used to prevent replay attack.
     */
    suspend fun decryptEventAndSaveResult(event: Event, timeline: String) {
+        if (event.type == EventType.ENCRYPTED && event.mxDecryptionResult != null) return


Usually to detect failed to decrypt we do getClearType() == m.room.encrypted

BillCarsonFr · 2023-03-01T11:18:25Z

...d/src/main/java/org/matrix/android/sdk/internal/session/pushrules/ProcessEventForPushTask.kt

@@ -73,6 +75,7 @@ internal class DefaultProcessEventForPushTask @Inject constructor(
                        " to check for push rules with ${params.rules.size} rules"
        )
        val matchedEvents = allEvents.mapNotNull { event ->
+            eventDecryptor.decryptEventAndSaveResult(event, "")


This might do unnecessary redecryption. Should be a decrypt if needed (test if amready decrypted before retrying).
Also at this point if it's not decrypted, I don't think that the second attempt will work. We don't have the key.

decryptEventAndSaveResult just returns if the event is already decrypted.
I just tested again with some logs and I confirm that the event is successfully decrypted at this moment:

13:03:51.024 V [PushRules] --> checkPushRules 13:03:51.043 V Execute transaction in 20 millis 13:03:51.063 V [PushRules] Found 1 out of 1 to check for push rules with 27 rules 13:03:51.064 D decryptEventAndSaveResult | event.getClearType()=m.room.encrypted 13:03:51.065 V decryptEvent $i8Dx2Ri7FIr-meu********, requestKeysOnFail:true 13:03:51.065 V ## Inbound: getInboundGroupSession 2Ge4kbC0P******** in cache true 13:03:51.069 V ########################################################## 13:03:51.070 V ## decryptGroupMessage() timeline: 13:03:51.070 V ## decryptGroupMessage() senderKey: AvjFBpOE************ 13:03:51.071 V ## decryptGroupMessage() sessionId: 2Ge4k*************** 13:03:51.071 V ## decryptGroupMessage() roomId: !fngDRO******** 13:03:51.072 V ## decryptGroupMessage() eventId: $i8Dx2Ri7FIr-meu******** 13:03:51.072 V ## decryptGroupMessage() mIndex: 17 13:03:51.073 V ## dispatchLiveEventDecrypted $i8Dx2Ri7FIr-meu****** 13:03:51.075 D decryptEventAndSaveResult | update event with decryption result: MXEventDecryptionResult(clearEvent={room_id=!fngDRO******, type=m.room.message, content={msgtype=m.text, body=test event in encrypted room}}, senderCurve25519Key=AvjFBpOEsqVexQ9F********, claimedEd25519Key=R5fd2pJs9XH******, forwardingCurve25519KeyChain=[], isSafe=true) 13:03:51.105 D Skipping notification update due to event list not changing 13:03:51.139 V [PushRules] Rule PushRule(actions=[notify, {set_tweak=sound, value=default}], default=false, enabled=true, ruleId=.m.rule.message, conditions=[PushCondition(kind=event_match, key=type, pattern=m.room.message, iz=null)], pattern=null) match for event $i8Dx2Ri7FIr-meu******* 13:03:51.140 D [PushRules] matched 1 out of 1 13:03:51.141 V [PushRules] Found 0 redacted events 13:03:51.143 D Push rule match for event $i8Dx2Ri7FIr-meu****** 13:03:51.148 V [PushRules] <-- Push task scheduled

I agree that this is probably not the best place to try to decrypt but it seems relevant to do the decryption. But I don't really know where it'd be better to do the decryption tbh

I think I see the problem.
In DefaultProcessEventForPushTask there is that:

it.takeIf { ... }?.copy(roomId = key)

This is doing a copy to add the roomId to the event, but the normal copy is not copying @Transiant fields. So it removed the decryption result.

So I think it should use copyAll()instead.

BillCarsonFr · 2023-03-01T11:21:23Z

...android/src/test/java/org/matrix/android/sdk/api/session/pushrules/PushRulesConditionTest.kt

@@ -155,6 +172,33 @@ class PushRulesConditionTest : MatrixTest {
        }
    }

+    @Test
+    fun test_decrypted_eventmatch_type_condition() {


add some test for content match rules:

m.rules.contains_user_name: content.body matches "alice"

m.rules.roomnotif: content.body matches "@room"

And an additional test to proove that a decrypted event won't match anymore the .m.rule.encrypted(type matches "m.room.encrypted")

As the decryption is done on a copy of the event (containing the roomId), we have to update the initial event with the decryption result

…push_rules_after_decryption

sonarqubecloud · 2023-03-06T08:47:18Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

34.7% Coverage
0.0% Duplication

BillCarsonFr

LGTM

Florian Renaud added 3 commits February 24, 2023 09:13

Apply push rules on encrypted events

b6ac915

Reuse event.supportsNotification extension

9d7758c

Add changelog file

b20d74a

Florian14 requested a review from BillCarsonFr February 24, 2023 08:54

Florian14 changed the title ~~Reapply push rules on the decrypted event source~~ Reapply push rules on the decrypted event source (PSG-1146) Feb 24, 2023

BillCarsonFr requested changes Mar 1, 2023

View reviewed changes

Florian Renaud added 2 commits March 1, 2023 10:42

Update encrypted raw json with decrypted event content for event matc…

ccf971a

…hing condition

Ensure the event is decrypted before applying push rules

75559fb

Florian14 requested a review from BillCarsonFr March 1, 2023 10:50

BillCarsonFr requested changes Mar 1, 2023

View reviewed changes

Florian Renaud added 2 commits March 1, 2023 13:50

Add additional tests

891897f

Improve encryption check condition

245ca13

Florian14 requested a review from BillCarsonFr March 1, 2023 12:53

Florian Renaud added 3 commits March 2, 2023 14:40

Update decryption result on the events from the sync response

084bd44

As the decryption is done on a copy of the event (containing the roomId), we have to update the initial event with the decryption result

Merge develop into feature/fre/apply_push_rules_after_decryption

4146b55

Merge remote-tracking branch 'origin/develop' into feature/fre/apply_…

21c7fc3

…push_rules_after_decryption

BillCarsonFr approved these changes Mar 7, 2023

View reviewed changes

Florian14 merged commit 39c702f into develop Mar 7, 2023

Florian14 deleted the feature/fre/apply_push_rules_after_decryption branch March 7, 2023 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reapply push rules on the decrypted event source (PSG-1146) #8170

Reapply push rules on the decrypted event source (PSG-1146) #8170

Florian14 commented Feb 24, 2023

Florian14 commented Feb 24, 2023

BillCarsonFr left a comment

BillCarsonFr Mar 1, 2023

Florian14 Mar 1, 2023

BillCarsonFr Mar 1, 2023

BillCarsonFr Mar 1, 2023

Florian14 Mar 1, 2023

BillCarsonFr Mar 1, 2023

BillCarsonFr Mar 1, 2023

Florian14 Mar 1, 2023

sonarqubecloud bot commented Mar 6, 2023

BillCarsonFr left a comment

Reapply push rules on the decrypted event source (PSG-1146) #8170

Reapply push rules on the decrypted event source (PSG-1146) #8170

Conversation

Florian14 commented Feb 24, 2023

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Florian14 commented Feb 24, 2023

BillCarsonFr 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

sonarqubecloud bot commented Mar 6, 2023

BillCarsonFr left a comment

Choose a reason for hiding this comment