Dev decrypt plus retry #249

daniellevass · 2020-04-06T09:51:22Z

What

Decrypts messages - we convert the received data into a EncryptedReceivedData object - we then replace the data part of the JSON with the decrypted message and pass it back to be packaged as a PusherEvent.
Retries only once if we can't decrypt it - if we can't decrypt it a second time we notify the listeners of an onDecryptionFailure - I have an open Q about what sort of Exception we should be returning here.
Added a prepareEvent method in the base Channel class which converts the json to the PusherEvent class, but this is where we can hook into decrypting it without having to copy all the other logic to handle messages.
Added a getInterestedListeners to get all the interested listeners for an event - we needed this in the ChannelImpl to notify when a message is received, I also needed a hook into these for notifying when failing to decrypt.

…n ChannelImpl

daniellevass · 2020-04-06T10:30:29Z

src/main/java/com/pusher/client/channel/impl/PrivateEncryptedChannelImpl.java

+        if (listeners != null) {
+            for (SubscriptionEventListener listener : listeners) {
+                ((PrivateEncryptedChannelEventListener)listener).onDecryptionFailure(
+                        new Exception("Failed to decrypt message"));


Not sure what kind of exception we should be returning here...

I'm not even sure if we should be passing an exception in a callback. A simple string describing the error would be sufficient.

It might be nice to pass some more details of the event as well. Perhaps the name of the event at least. I'm not sure the customer will have much use for the ciphertext or anything else though.

So if we made the method .onDecryptionFailure(String event, String reason) - you think that would be sufficient? People will understand it's a failure callback, so we don't need to give them a packaged Exception to handle.

daniellevass · 2020-04-06T13:12:29Z

src/main/java/com/pusher/client/channel/impl/PrivateEncryptedChannelImpl.java

+            // retry once only.
+            disposeSecretBoxOpener();
+            authenticate();
+
+            try {
+                Map receivedMessage = GSON.fromJson(message, Map.class);
+                final String decryptedMessage = decryptMessage((String) receivedMessage.get("data"));
+                receivedMessage.replace("data", decryptedMessage);
+
+                return GSON.fromJson(
+                        GSON.toJson(receivedMessage), PusherEvent.class);
+            } catch (AuthenticityException e2) {
+                disposeSecretBoxOpener();
+                notifyListenersOfDecryptFailure(event);
+            }
+        }


i'm not sure I like this nested try/catch but I'm not sure how to improve it... 🤔

Yes, it's not ideal, although it kind of works for a single retry. If we wanted multiple retries, we might prefer a loop:

for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { try { // decrypt it return decryptedMessage; } catch { // dispose of the secret // authenticate (possibly unless attempt == MAX_ATTEMPTS-1) } } // notify listeners of failure because we ran out of attempts before returning return null;

Alternatively, you can un-nest the second attempt by recognising that the try block ends with return, so the only way to reach code after the whole try/catch construct is via the catch.

try { // decrypt it return decryptedMessage; } catch { // dispose of the secret } // authenticate try { // decrypt it return decryptedMessage; } catch { // dispose of the secret } // notify listeners return null;

It might also benefit from a private function to encapsulated the repeated code in the decryption attempt.

None of them seem super elegant, but some food for thought.

Thanks for the helpful response. I'll have a think over this and see what I can do to refactor it to be a little bit better :)

I've refactored out Json parts to decryptMessage so it is now responsible for returning the PusherEvent. ~~I moved the retry logic into its own method so hopefully it's easier to follow what's happening instead of nested try's and catches.~~

mdpye

I haven't been able to look in to the tests yet, but I wanted to deliver this batch of feedback to you, because it contains some food for thought...

mdpye · 2020-04-06T14:24:10Z

src/main/java/com/pusher/client/channel/impl/ChannelImpl.java

+                listeners = null;
+            }
+        }
+        return listeners;


Because there's nothing else in this method other than the synchronized block, we don't really need the local variable, you can just replace the assignments of listeners with return statements.

Ah, that's very interesting. So Android Studio helped me simplify it even more. The if listeners = null is redundant, because if it's null we can just return... null - which then means we can simplify it down to just:

protected Set<SubscriptionEventListener> getInterestedListeners(String event) { synchronized (lock) { return eventNameToListenerMap.get(event); } }

I don't think that's equivalent. The old code returned a copy of the collection of listeners, not the actual collection from the map.

Because Java's collections are all mutable by default, it's important to copy the collection returned, so that if the caller manipulates it they aren't making unsynchronized changes to the contents of the eventNameToListenerMap.

Hmm. Good to know! I've updated this code to return a copy of the interested listeners - it's still a little bit simplified than what was there before.

mdpye · 2020-04-06T14:25:26Z

src/main/java/com/pusher/client/channel/impl/ChannelImpl.java

-                        }
-                    });
+                final PusherEvent pusherEvent = prepareEvent(event, message);
+                if (pusherEvent != null) {


Under what circumstances might this be null? If it's possible then it seems like an error we should handle, if it isn't then this check can be omitted...

The circumstance is that we failed to decrypt an encrypted message - we return null for the prepareEvent method and this then means it won't notify the listeners for onEvent instead we'll be notifying them through the PrivateChannelImpl that of an onDecryptionFailure.

mdpye · 2020-04-06T14:45:24Z

src/main/java/com/pusher/client/channel/impl/PrivateEncryptedChannelImpl.java

+            // retry once only.
+            disposeSecretBoxOpener();
+            authenticate();
+
+            try {
+                Map receivedMessage = GSON.fromJson(message, Map.class);
+                final String decryptedMessage = decryptMessage((String) receivedMessage.get("data"));
+                receivedMessage.replace("data", decryptedMessage);
+
+                return GSON.fromJson(
+                        GSON.toJson(receivedMessage), PusherEvent.class);
+            } catch (AuthenticityException e2) {
+                disposeSecretBoxOpener();
+                notifyListenersOfDecryptFailure(event);
+            }
+        }


Yes, it's not ideal, although it kind of works for a single retry. If we wanted multiple retries, we might prefer a loop:

for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { try { // decrypt it return decryptedMessage; } catch { // dispose of the secret // authenticate (possibly unless attempt == MAX_ATTEMPTS-1) } } // notify listeners of failure because we ran out of attempts before returning return null;

Alternatively, you can un-nest the second attempt by recognising that the try block ends with return, so the only way to reach code after the whole try/catch construct is via the catch.

try { // decrypt it return decryptedMessage; } catch { // dispose of the secret } // authenticate try { // decrypt it return decryptedMessage; } catch { // dispose of the secret } // notify listeners return null;

It might also benefit from a private function to encapsulated the repeated code in the decryption attempt.

None of them seem super elegant, but some food for thought.

mdpye · 2020-04-06T14:46:57Z

src/main/java/com/pusher/client/channel/impl/PrivateEncryptedChannelImpl.java

+                return GSON.fromJson(
+                        GSON.toJson(receivedMessage), PusherEvent.class);
+            } catch (AuthenticityException e2) {
+                disposeSecretBoxOpener();


What will happen to the next event if we dispose of the secret here on the final attempt?

That's a good question. It will probably crash with an exception as the secretbox will be null. What should we be doing in this situation? We can do a check to see if the secretbox is null at the start of the prepareEvent, and call the onDecryptionFailure with a different reason?

At any point, there should definitely be a test for this which i will add now.

Have added the test 4bfc70d#diff-adf187cd707fe9cbe8285aefab4bafbbR287 - and assumed i was okay to implement the logic as described. Please let me know what you think on this.

There are circumstances during the rotation of master key on the customer's backend where races can occur between events sent and keys received, so I think it's important that the SDK recover from a failed decryption and not fail all future events because of a period of instability when the keys are rotated.

Personally, I would leave the last fetched key in place and attempt the next event using it.

👍 Marek left feedback before that he expected it to be cleared after each failed decryption.

I think what you say makes sense - each subsequent message should be able to send 1 retry to the authorization endpoint to get a new shared_secret.

I've updated the code to do this.

I've also updated the tests: there is one test to ensure that two messages that have incorrect shared secrets after a retry, i also have a test for if a second message when it does the retry gets a correct key, it should succeed.

Does that make sense?

Yup, sounds great

mdpye · 2020-04-06T14:50:11Z

src/main/java/com/pusher/client/channel/impl/PrivateEncryptedChannelImpl.java

+            receivedMessage.replace("data", decryptedMessage);
+
+            return GSON.fromJson(
+                    GSON.toJson(receivedMessage), PusherEvent.class);


I'm not sure about this round-trip via GSON. If you look at the constructor for PusherEvent, it just wraps a map, so if we update the "data" key in our map, we should be able to construct a PusherEvent around it directly without needing to use serialisation.

Hmm. I'll try again to see what I can do. It was proving to be a little difficult to get this working.

Okay - so i'm passing in the Map<String, Object> into the PusherEvent and all the tests still pass. Thank you for the feedback!

…ructor

This reverts commit a87547f.

…ges get a retry to the authorization endpoint

mdpye

Just the one comment on accidentally removing the copy of the listeners Set, then I think we're good to go.

daniellevass added 4 commits April 6, 2020 10:28

Add. prepareEvent method to InternalChannel interface and implement i…

3a8a7bc

…n ChannelImpl

Refactor getInterestedListeners to it's own method in ChannelImpl

c8ea300

Decrypt PrivateEncrypted messages and retry once

f290cb6

Merge branch 'decrypt-integration' into dev-decrypt-plus-retry

8ee7b59

daniellevass self-assigned this Apr 6, 2020

daniellevass commented Apr 6, 2020

View reviewed changes

Add tests for retrying decrypting messages

0cf29b7

daniellevass commented Apr 6, 2020

View reviewed changes

daniellevass marked this pull request as ready for review April 6, 2020 13:19

daniellevass requested a review from mdpye April 6, 2020 13:19

mdpye suggested changes Apr 6, 2020

View reviewed changes

daniellevass added 7 commits April 6, 2020 16:09

Simplify the getInterestedListeners method in ChannelImpl

0e178e8

Handle multiple failed decryption calls better

4bfc70d

When decrypting a message, pass the json map to the PusherEvent const…

3a8f63e

…ructor

Refactor decryptMessage to package up as a PusherEvent

b03e466

Move the retry logic into it's own method

a87547f

Revert "Move the retry logic into it's own method"

2f5b7e2

This reverts commit a87547f.

Keep the shared_secret after the second retry so any subsequent messa…

0a62b93

…ges get a retry to the authorization endpoint

pusher deleted a comment from codecov-io Apr 7, 2020

daniellevass requested a review from mdpye April 7, 2020 09:05

mdpye suggested changes Apr 7, 2020

View reviewed changes

Return a copy of the interestedListeners

e694400

daniellevass requested a review from mdpye April 7, 2020 13:22

mdpye approved these changes Apr 7, 2020

View reviewed changes

daniellevass merged commit f4fa2dd into decrypt-integration Apr 7, 2020

daniellevass deleted the dev-decrypt-plus-retry branch April 7, 2020 13:35

Dev decrypt plus retry #249

Dev decrypt plus retry #249

Uh oh!

Conversation

daniellevass commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniellevass Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniellevass Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniellevass Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daniellevass commented Apr 6, 2020 •

edited

Loading

mdpye Apr 6, 2020 •

edited

Loading

daniellevass Apr 6, 2020 •

edited

Loading

daniellevass Apr 6, 2020 •

edited

Loading

mdpye Apr 6, 2020 •

edited

Loading

daniellevass Apr 6, 2020 •

edited

Loading