Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two sync loop experimental solution to decrypt push notifications #1082

Merged
merged 29 commits into from
Jun 22, 2023

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented Jun 14, 2023

This is using an external matrix rust sdk experimental branch, not merged yet in main, so it will likely fail to compile on CI for now.

The idea is to use the notification sync (which I feel it should be renamed to encryption or decryption sync) to actually update the keys.

The sync runs both in the main app and in the NSE, we can now wait for the notification to be fully decrypted (we wait for about 10 seconds at most for now but it takes way less) before showing it to the user.

This requires to not use all common extensions but remove e2ee and toDeviceEvents from the normal sync, since the notification sync does that job instead.

Updated to work with RoomListAPI

Adding also an ff, to enable or not this, since could require some testing for quite some in the nightly time before we know if is stable or not.

@github-actions
Copy link

github-actions bot commented Jun 14, 2023

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 53dc155

@Velin92 Velin92 linked an issue Jun 14, 2023 that may be closed by this pull request
@Velin92 Velin92 force-pushed the mauroromito/notification_encryption_sync branch 2 times, most recently from 30a383b to 7407ad7 Compare June 19, 2023 09:11
@Velin92 Velin92 requested a review from stefanceriu June 19, 2023 09:28
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments inline but it looks good to me 👍

NSE/Sources/Other/NSEUserSession.swift Outdated Show resolved Hide resolved
NSE/Sources/NotificationServiceExtension.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
@Velin92 Velin92 force-pushed the mauroromito/notification_encryption_sync branch from acd9933 to 2fba9cc Compare June 20, 2023 15:17
@Velin92 Velin92 force-pushed the mauroromito/notification_encryption_sync branch from 4394ff9 to 83f063a Compare June 21, 2023 08:02
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about isSyncing and merging together the 2 loops, otherwise it looks good to me 👍

ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
ElementX/Sources/Services/Client/ClientProxy.swift Outdated Show resolved Hide resolved
@stefanceriu stefanceriu self-requested a review June 22, 2023 09:24
@Velin92 Velin92 force-pushed the mauroromito/notification_encryption_sync branch from 83f063a to bbff3d1 Compare June 22, 2023 15:07
@Velin92 Velin92 marked this pull request as ready for review June 22, 2023 15:08
@Velin92 Velin92 requested a review from a team as a code owner June 22, 2023 15:08
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 9.85% and project coverage change: -0.54 ⚠️

Comparison is base (bbcfec3) 42.37% compared to head (068eba4) 41.84%.

❗ Current head 068eba4 differs from pull request most recent head 53dc155. Consider uploading reports for the commit 53dc155 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1082      +/-   ##
===========================================
- Coverage    42.37%   41.84%   -0.54%     
===========================================
  Files          393      396       +3     
  Lines        24830    24983     +153     
  Branches     12992    13063      +71     
===========================================
- Hits         10522    10454      -68     
- Misses       14029    14248     +219     
- Partials       279      281       +2     
Flag Coverage Δ
unittests 23.83% <9.85%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
DesignKit/Sources/Colors/ElementColors.swift 81.25% <0.00%> (-2.75%) ⬇️
ElementX/Sources/Application/AppCoordinator.swift 48.78% <0.00%> (-2.11%) ⬇️
ElementX/Sources/Application/AppSettings.swift 96.66% <ø> (-0.06%) ⬇️
...Sources/FlowCoordinators/RoomFlowCoordinator.swift 45.24% <0.00%> (+0.43%) ⬆️
...urces/Other/Extensions/UNNotificationContent.swift 0.00% <ø> (ø)
...s/Other/SwiftUI/Form Styles/FormButtonStyles.swift 23.92% <0.00%> (-0.62%) ⬇️
...tX/Sources/Other/UserIndicator/UserIndicator.swift 35.13% <0.00%> (-2.71%) ⬇️
...s/Other/UserIndicator/UserIndicatorModalView.swift 43.33% <0.00%> (-0.74%) ⬇️
...s/BugReportScreen/BugReportScreenCoordinator.swift 37.25% <0.00%> (ø)
...ces/Screens/CreateRoom/View/CreateRoomScreen.swift 0.00% <0.00%> (ø)
... and 34 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Velin92 Velin92 enabled auto-merge (squash) June 22, 2023 16:44
@Velin92 Velin92 force-pushed the mauroromito/notification_encryption_sync branch from cecc567 to 99cd55d Compare June 22, 2023 17:02
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Velin92 Velin92 merged commit afc4332 into develop Jun 22, 2023
@Velin92 Velin92 deleted the mauroromito/notification_encryption_sync branch June 22, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two sync loops for push notifications
3 participants