Changeset View
Standalone View
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
Show All 15 Lines | |||||||||||
import androidx.localbroadcastmanager.content.LocalBroadcastManager; | import androidx.localbroadcastmanager.content.LocalBroadcastManager; | ||||||||||
import app.comm.android.ExpoUtils; | import app.comm.android.ExpoUtils; | ||||||||||
import app.comm.android.MainActivity; | import app.comm.android.MainActivity; | ||||||||||
import app.comm.android.R; | import app.comm.android.R; | ||||||||||
import app.comm.android.fbjni.CommSecureStore; | import app.comm.android.fbjni.CommSecureStore; | ||||||||||
import app.comm.android.fbjni.GlobalDBSingleton; | import app.comm.android.fbjni.GlobalDBSingleton; | ||||||||||
import app.comm.android.fbjni.MessageOperationsUtilities; | import app.comm.android.fbjni.MessageOperationsUtilities; | ||||||||||
import app.comm.android.fbjni.NetworkModule; | import app.comm.android.fbjni.NetworkModule; | ||||||||||
import app.comm.android.fbjni.NotificationsCryptoModule; | |||||||||||
import app.comm.android.fbjni.ThreadOperations; | import app.comm.android.fbjni.ThreadOperations; | ||||||||||
import com.google.firebase.messaging.FirebaseMessagingService; | import com.google.firebase.messaging.FirebaseMessagingService; | ||||||||||
import com.google.firebase.messaging.RemoteMessage; | import com.google.firebase.messaging.RemoteMessage; | ||||||||||
import java.io.File; | import java.io.File; | ||||||||||
import java.util.ArrayList; | |||||||||||
import java.util.List; | |||||||||||
import java.util.Set; | |||||||||||
import me.leolin.shortcutbadger.ShortcutBadger; | import me.leolin.shortcutbadger.ShortcutBadger; | ||||||||||
public class CommNotificationsHandler extends FirebaseMessagingService { | public class CommNotificationsHandler extends FirebaseMessagingService { | ||||||||||
private static final String BADGE_KEY = "badge"; | private static final String BADGE_KEY = "badge"; | ||||||||||
private static final String BADGE_ONLY_KEY = "badgeOnly"; | private static final String BADGE_ONLY_KEY = "badgeOnly"; | ||||||||||
private static final String BACKGROUND_NOTIF_TYPE_KEY = "backgroundNotifType"; | private static final String BACKGROUND_NOTIF_TYPE_KEY = "backgroundNotifType"; | ||||||||||
private static final String SET_UNREAD_STATUS_KEY = "setUnreadStatus"; | private static final String SET_UNREAD_STATUS_KEY = "setUnreadStatus"; | ||||||||||
private static final String NOTIF_ID_KEY = "id"; | private static final String NOTIF_ID_KEY = "id"; | ||||||||||
private static final String ENCRYPTED_KEY = "encrypted"; | |||||||||||
private static final Set<String> UNENCRYPTED_FIELDS = | |||||||||||
Set.of(NOTIF_ID_KEY, BADGE_ONLY_KEY, ENCRYPTED_KEY); | |||||||||||
private static final String CHANNEL_ID = "default"; | private static final String CHANNEL_ID = "default"; | ||||||||||
private static final long[] VIBRATION_SPEC = {500, 500}; | private static final long[] VIBRATION_SPEC = {500, 500}; | ||||||||||
private Bitmap displayableNotificationLargeIcon; | private Bitmap displayableNotificationLargeIcon; | ||||||||||
private NotificationManager notificationManager; | private NotificationManager notificationManager; | ||||||||||
private LocalBroadcastManager localBroadcastManager; | private LocalBroadcastManager localBroadcastManager; | ||||||||||
public static final String RESCIND_KEY = "rescind"; | public static final String RESCIND_KEY = "rescind"; | ||||||||||
Show All 29 Lines | public class CommNotificationsHandler extends FirebaseMessagingService { | ||||||||||
@Override | @Override | ||||||||||
public void onMessageReceived(RemoteMessage message) { | public void onMessageReceived(RemoteMessage message) { | ||||||||||
String rescind = message.getData().get(RESCIND_KEY); | String rescind = message.getData().get(RESCIND_KEY); | ||||||||||
if ("true".equals(rescind) && | if ("true".equals(rescind) && | ||||||||||
android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.M) { | android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.M) { | ||||||||||
handleNotificationRescind(message); | handleNotificationRescind(message); | ||||||||||
} | } | ||||||||||
String encrypted = message.getData().get(ENCRYPTED_KEY); | |||||||||||
if (encrypted != null && "1".equals(encrypted)) { | |||||||||||
try { | |||||||||||
message = this.decryptRemoteMessage(message); | |||||||||||
} catch (Exception e) { | |||||||||||
Log.w("COMM", "Failed to decrypt encrypted notification", e); | |||||||||||
return; | |||||||||||
} | |||||||||||
} else if (encrypted != null) { | |||||||||||
tomek: What do you think about being more explicit in conditions when it comes to `encrypted` values? | |||||||||||
marcinAuthorUnsubmitted Done Inline ActionsIntroducing this change might lead us into NullPointerException since we don't assign encrypted field to every notification, even after landing this entire stack. This field is used as follows:
marcin: Introducing this change might lead us into `NullPointerException` since we don't assign… | |||||||||||
tomekUnsubmitted Not Done Inline ActionsThere shouldn't be an NPE - to avoid that you're using an inverted syntax (notice: encrypted.equals("0") which NPEs vs "0".equals(encrypted) which doesn't). This is the exact reason behind using this strange syntax. tomek: There shouldn't be an NPE - to avoid that you're using an inverted syntax (notice: `encrypted. | |||||||||||
Log.w( | |||||||||||
"COMM", | |||||||||||
"Received unencrypted notification for client with existing olm session for notifications"); | |||||||||||
} | |||||||||||
String badge = message.getData().get(BADGE_KEY); | String badge = message.getData().get(BADGE_KEY); | ||||||||||
if (badge != null) { | if (badge != null) { | ||||||||||
try { | try { | ||||||||||
int badgeCount = Integer.parseInt(badge); | int badgeCount = Integer.parseInt(badge); | ||||||||||
if (badgeCount > 0) { | if (badgeCount > 0) { | ||||||||||
ShortcutBadger.applyCount(this, badgeCount); | ShortcutBadger.applyCount(this, badgeCount); | ||||||||||
} else { | } else { | ||||||||||
ShortcutBadger.removeCount(this); | ShortcutBadger.removeCount(this); | ||||||||||
▲ Show 20 Lines • Show All 111 Lines • ▼ Show 20 Lines | private PendingIntent createStartMainActivityAction(RemoteMessage message) { | ||||||||||
intent.putExtra("message", message); | intent.putExtra("message", message); | ||||||||||
return PendingIntent.getActivity( | return PendingIntent.getActivity( | ||||||||||
this.getApplicationContext(), | this.getApplicationContext(), | ||||||||||
message.getData().get(NOTIF_ID_KEY).hashCode(), | message.getData().get(NOTIF_ID_KEY).hashCode(), | ||||||||||
intent, | intent, | ||||||||||
PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_MUTABLE); | PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_MUTABLE); | ||||||||||
} | } | ||||||||||
private RemoteMessage decryptRemoteMessage(RemoteMessage message) { | |||||||||||
List<String> payloadFieldNames = | |||||||||||
new ArrayList<>(message.getData().keySet()); | |||||||||||
for (String payloadFieldName : payloadFieldNames) { | |||||||||||
tomekUnsubmitted Not Done Inline Actions
I don't think it is beneficial to create an ArrayList just to iterate on it. We can directly iterate Set. tomek: I don't think it is beneficial to create an `ArrayList` just to iterate on it. We can directly… | |||||||||||
marcinAuthorUnsubmitted Done Inline ActionsWe don't modify which keys are present and which are not, but we modify values that are stored under those keys. Therefore according to the docs: https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#keySet-- iterating directly over keySet output might not be safe. That is why I created the copy of keys present in the map first. marcin: We don't modify which keys are present and which are not, but we modify values that are stored… | |||||||||||
tomekUnsubmitted Not Done Inline ActionsOk, makes sense. There's a more functional approach using Map.replaceAll, but using an intermediate array may also stay. tomek: Ok, makes sense. There's a more functional approach using `Map.replaceAll`, but using an… | |||||||||||
if (UNENCRYPTED_FIELDS.contains(payloadFieldName)) { | |||||||||||
continue; | |||||||||||
} | |||||||||||
String encryptedPayloadFieldValue = | |||||||||||
message.getData().get(payloadFieldName); | |||||||||||
if (encryptedPayloadFieldValue == null) { | |||||||||||
continue; | |||||||||||
} | |||||||||||
String decryptedPayloadFieldValue = NotificationsCryptoModule.decrypt( | |||||||||||
encryptedPayloadFieldValue, | |||||||||||
NotificationsCryptoModule.olmEncryptedTypeMessage(), | |||||||||||
"CommNotificationsHandler"); | |||||||||||
message.getData().put(payloadFieldName, decryptedPayloadFieldValue); | |||||||||||
} | |||||||||||
return message; | |||||||||||
tomekUnsubmitted Not Done Inline ActionsWhy do we encrypt/decrypt field-by-field? Aren't we leaking some info by doing so? Won't it be safer to encrypt the whole e.g. JSON with all the fields? tomek: Why do we encrypt/decrypt field-by-field? Aren't we leaking some info by doing so? Won't it be… | |||||||||||
marcinAuthorUnsubmitted Done Inline ActionsThere are a couple of reasons here:
In conclusion:
cc @ashoat marcin: There are a couple of reasons here:
1. Not every field is encrypted, but I understand we could… | |||||||||||
tomekUnsubmitted Not Done Inline ActionsMy intuition tells me that encrypting field-by-field is more vulnerable, because an attacker has a couple of independent encrypted values instead of one, bigger one (but intuition isn't a good tool when it comes to cryptography). From size perspective it also feels like encrypting all at once is more efficient - if e.g. a padding is added, it is added once instead of once per field. So for me it seems like encrypting all at once should be better. But not sure if the difference is significant enough that we should change the solution. Also, I might be wrong and field-by -field is better overall. tomek: My intuition tells me that encrypting field-by-field is more vulnerable, because an attacker… | |||||||||||
} | |||||||||||
} | } |
What do you think about being more explicit in conditions when it comes to encrypted values?