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

NSE - Prevent playback on already read messages and enable on device … #6265

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

stefanceriu
Copy link
Member

@stefanceriu stefanceriu commented Jun 10, 2022

…silencing for unwanted push notifications

On device notification filtering is dependant on sygnal setting push_type = alert on notifications. An attempt at this was tried here but it's incomplete as voip pushes need another type/topic.

This PR now depends on matrix-org/matrix-ios-sdk#1500

@stefanceriu stefanceriu requested review from a team, SBiOSoftWhare and gileluard and removed request for a team June 10, 2022 11:41
@stefanceriu stefanceriu force-pushed the stefan/nse-filtering-and-playback branch from 0ffefe1 to 70b6e19 Compare June 10, 2022 11:43
@stefanceriu stefanceriu requested review from ismailgulek and removed request for SBiOSoftWhare June 10, 2022 11:43
@github-actions
Copy link

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/BRWEWr

Copy link
Contributor

@gileluard gileluard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ismailgulek ismailgulek left a comment

Choose a reason for hiding this comment

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

Had some comments after a static review. We'll need to test this after Sygnal task is done.

RiotNSE/NotificationService.swift Outdated Show resolved Hide resolved
RiotNSE/NotificationService.swift Outdated Show resolved Hide resolved
RiotNSE/NotificationService.swift Show resolved Hide resolved
RiotNSE/NotificationService.swift Outdated Show resolved Hide resolved
@stefanceriu stefanceriu force-pushed the stefan/nse-filtering-and-playback branch from 60d465d to ebb912a Compare June 14, 2022 11:42
@stefanceriu stefanceriu force-pushed the stefan/nse-filtering-and-playback branch from ebb912a to 22c4813 Compare June 14, 2022 13:25
Copy link
Contributor

@ismailgulek ismailgulek left a comment

Choose a reason for hiding this comment

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

LGTM!

@stefanceriu stefanceriu merged commit db149ef into develop Jun 15, 2022
@stefanceriu stefanceriu deleted the stefan/nse-filtering-and-playback branch June 15, 2022 12:18
@blau2
Copy link

blau2 commented Jul 15, 2022

Hi @stefanceriu, I was wondering if you would mind providing some additional context around why this change was made? Specifically, what does "playback" mean in this context? Beeper has forked the matrix-ios-sdk and we have gotten reports of users receiving notifications for very old messages when they login to their Matrix account on a new device. This PR looks like it would resolve this issue and I was wondering if it was a similar problem that prompted this fix. Thanks!

@stefanceriu
Copy link
Member Author

Hey! This was in response to a security concern in which if somebody would get access to your credentials and phisical device they would be able to decrypt and display any message in notification popups by sending a new push request for an old message identifier. The fix basically just checks if a mesages was previously read and it if it was ignores the push.

@blau2
Copy link

blau2 commented Jul 19, 2022

Thanks Stefan, sounds like a different problem than what we're encountering but good to know!

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.

4 participants