Fix MM 56723 #7883
Fix MM 56723 #7883
Conversation
larkox
added
2: Dev Review
ios/Gekidou/Sources/Gekidou/PushNotification/PushNotification+Signature.swift
Show resolved
Hide resolved
ios/Gekidou/Sources/Gekidou/PushNotification/PushNotification+Signature.swift
Show resolved
Hide resolved
| if #available(iOSApplicationExtension 15.0, *) { | ||
| os_log(OSLogType.default, "Mattermost Notifications: creating invalid intent") | ||
|
|
||
| bestAttemptContent?.body = "We could not verify this notification with the server" |
There was a problem hiding this comment.
This could be translated.
Also move out of the if statement and assign to the declared notification variable
There was a problem hiding this comment.
I added the translation, but couldn't make it work. Not sure if a problem of working locally (I ran the replace_assets lane and even though didn't seem to work), but since I am following the same pattern as in other places in the code, I don't think is a problem exactly on this app.
| } | ||
| } | ||
| } else { | ||
| self.contentHandler?(notification) |
There was a problem hiding this comment.
This will display the original msg in the notification for OS lower than 15, we ok with that? That if of course unless you address the previous comment
android/app/src/main/java/com/mattermost/helpers/database_extension/General.kt
Show resolved
Hide resolved
android/app/src/main/java/com/mattermost/helpers/CustomPushNotificationHelper.java
Show resolved
Hide resolved
| @@ -227,6 +238,153 @@ public static void createNotificationChannels(Context context) { | |||
| } | |||
| } | |||
|
|
|||
| public static boolean verifySignature(final Context context, String signature, String serverUrl, String ackId) { | |||
| if (signature == null) { | |||
| Log.i("Mattermost Notifications Signature verification", "No signature in the notification"); | |||
There was a problem hiding this comment.
Is this for backward compatibility with older servers? If so, can we add a comment and ticket to make signature verification mandatory after, say, next two ESRs?
There was a problem hiding this comment.
This is for compatibility with old push proxies. I can add a comment about that.
| break; | ||
| } | ||
|
|
||
| if (major < majorTarget) { |
There was a problem hiding this comment.
Should equal value case also be included here?
There was a problem hiding this comment.
We only check the minor version if the version is exactly the same. So... imagine we have the targets 9.8.0 and 7.5.0.
If we are on version 8.0.0, when comparing with 9.8.0, since the major version is lower, we can continue to the next target. But if we are on version 9.9.0, even if the major version is the same, we want to check the minor version.
I hope it makes sense
larkox
added
4: Reviews Complete
* Fix MM 56723 (iOS) * Add android * Android fixes and version checking * Add version check to ios * Address feedback * Add all versions to android * Check all versions on iOS * Fix unhandled version case * Add comments * Add final version numbers
Summary
Fix MM 56723
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-56723
Related PRs:
Push Proxy: mattermost/mattermost-push-proxy#119
Server: mattermost/mattermost#26643
Release Note