Add clearing of shared secret on disconnected #247

marekoid · 2020-04-02T15:19:49Z

No description provided.

Fix IndexOutOfBounds when passing all 4 args in the example app. Make the example app honour variable length of arguments array. Fix warning. Improve formatting.

marekoid · 2020-04-02T15:22:18Z

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


-    private String authenticate() {
+    @Override
+    public String toSubscribeMessage() {


Just moved to be first as public (high level) method, the private (low level, detail) methods come after it. Also fixed warnings too about redundant generics at the definition side.

mdpye

It's a bit of a hack, but I think it will work for now :)

marekoid · 2020-04-02T16:01:20Z

It's a bit of a hack, but I think it will work for now :)

Why? It's using existing listener facility of connection which is being already provided to it and used also for other needs. The other needs will grow in the future when triggering of events is implemented (like in private channels).

marekoid · 2020-04-02T16:10:26Z

You mean probably needing to set listener and unset when called back as opposed to be called on a own method like it is the case of updateState? I was thinking about onConnectionStateChange in InternalChannel however most channel implementations won't need it so it didn't feel l like it would be justified to complicate the InternalChannel interface for one channel, especially as there is a more modular existing listener facility that can be used (that filtering on events you are interested in is kinda nice too).

Make the code more legible. That is more symmetrical when it comes to setting/removing the disconnect listener, and creating/disposing SecretBoxOpener. Make the example app exercise unsubscribe too and the two combinations (subscribed/unsubscribed) when disconnect is called.

marekoid · 2020-04-03T10:50:02Z

@mdpye thanks for your feedback that was thought provoking and pushed me to write better code: ddda077 let me know your thoughts, now it feels less hacky I hope?

mdpye · 2020-04-03T10:52:53Z

src/main/java/com/pusher/client/example/PrivateEncryptedChannelExampleApp.java

+            case 4: cluster = args[3];
+            case 3: eventName = args[2];
+            case 2: channelName = args[1];
+            case 0: apiKey = args[0];


0 args, or 1 arg?

Oh sorry a schoolboy error 🤦‍♂ Good catch! Fixed: 670609e

mdpye · 2020-04-03T10:55:16Z

Oops, sorry, didn't mean to hit "approve", though I generallly do, I think the final case fallthrough is incorrect.

Fix accepting a single arg. Thanks Mike for catching it: #247 (comment)

daniellevass · 2020-04-03T13:00:32Z

src/main/java/com/pusher/client/example/PrivateEncryptedChannelExampleApp.java

-            channelName = args[1];
-            eventName = args[2];
-            cluster = args[3];
+        switch (args.length) {


I've not seen this done before for setting a bunch of objects - is this going to go through each item setting it, or just to call it once and set only the last element?

Because the break; is missing from each case, they will all "fall though". It will start at the matching case and execute all the remaining ones. I've not seen it either, but it does seem quite neat.

daniellevass · 2020-04-03T13:03:48Z

src/main/java/com/pusher/client/example/PrivateEncryptedChannelExampleApp.java

+                pusher.connect(this);
+                Thread.sleep(5000);
+                pusher.unsubscribe(channelName); // to test clearing of shared secret (via tmp log)
+                if (i % 2 == 0) { // to test disconnect on both unsubscribed/subscribed


I don't think this Example app now models a real life use - it's trying to be a test and is not useful as the first example of Private Encrypted Channels people see. Either we need to refactor this to be clearer exactly what's happening and why we're disconnecting/reconnecting every 5 seconds, or maybe the easier solution is to make a new Private Encrypted Channel Example app that showcases how people handle disconnects - maybe this can be procedurally driven and not require a while loop.

thanks! as discussed reverted to not complicate the encrypted channel example, making it more of a test app: 95d5ca3

daniellevass · 2020-04-03T13:19:38Z

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

+
+        @Override
+        public void onConnectionStateChange(ConnectionStateChange change) {
+            disposeSecretBoxOpener();


Won't this get called a lot of times? e.g. you go through CONNECTING to CONNECTED - would this not be better if it was only if the connect state was DISCONNECTING?

It's a bit... "disconnected", but you'll find your answer here: https://github.com/pusher/pusher-websocket-java/pull/247/files#diff-48ff2981ea97376b5f1cedec4b59e059R114

Consistently tidy up PrivateEncryptedChannelImplTest

Following Danielle's feedback: https://github.com/pusher/pusher-websocket-java/pull/247/files#r402989472

codecov-io · 2020-04-03T16:36:05Z

Codecov Report

Merging #247 into decrypt-integration will increase coverage by 0.07%.
The diff coverage is 62.50%.

@@                    Coverage Diff                    @@
##             decrypt-integration     #247      +/-   ##
=========================================================
+ Coverage                  76.72%   76.79%   +0.07%     
- Complexity                   265      270       +5     
=========================================================
  Files                         33       33              
  Lines                       2337     2357      +20     
  Branches                     115      116       +1     
=========================================================
+ Hits                        1793     1810      +17     
- Misses                       492      495       +3     
  Partials                      52       52

Impacted Files	Coverage Δ	Complexity Δ
...ent/example/PrivateEncryptedChannelExampleApp.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ient/channel/impl/PrivateEncryptedChannelImpl.java	`98.11% <96.15%> (-1.89%)`	`17.00 <8.00> (+3.00)`	⬇️
...com/pusher/client/crypto/nacl/SecretBoxOpener.java	`82.60% <0.00%> (+6.52%)`	`6.00% <0.00%> (+1.00%)`
...com/pusher/client/util/internal/Preconditions.java	`40.00% <0.00%> (+20.00%)`	`3.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f16b2...95d5ca3. Read the comment docs.

Add clearing of shared secret on disconnected

8ea7340

Fix IndexOutOfBounds when passing all 4 args in the example app. Make the example app honour variable length of arguments array. Fix warning. Improve formatting.

marekoid commented Apr 2, 2020

View reviewed changes

marekoid requested review from daniellevass and mdpye April 2, 2020 15:22

mdpye reviewed Apr 2, 2020

View reviewed changes

marekoid requested a review from mdpye April 3, 2020 10:50

mdpye reviewed Apr 3, 2020

View reviewed changes

Add info about the need for tmp log for the semi-manual test

8ae2cb2

mdpye approved these changes Apr 3, 2020

View reviewed changes

Fix IndexOutOfBounds for no args in example app

670609e

Fix accepting a single arg. Thanks Mike for catching it: #247 (comment)

daniellevass reviewed Apr 3, 2020

View reviewed changes

Make naming consistent and more clear

b5059d3

daniellevass reviewed Apr 3, 2020

View reviewed changes

marekoid added 2 commits April 3, 2020 17:31

Add PrivateEncryptedChannelClearsKeyTest

9c8bf51

Consistently tidy up PrivateEncryptedChannelImplTest

Revert making example app unsubscribe/disconnect

95d5ca3

Following Danielle's feedback: https://github.com/pusher/pusher-websocket-java/pull/247/files#r402989472

Remove unused ArgumentCaptor

0250472

marekoid marked this pull request as ready for review April 3, 2020 16:41

marekoid requested review from daniellevass and mdpye April 3, 2020 16:41

daniellevass approved these changes Apr 6, 2020

View reviewed changes

daniellevass self-assigned this Apr 6, 2020

mdpye approved these changes Apr 6, 2020

View reviewed changes

daniellevass merged commit d346cea into decrypt-integration Apr 6, 2020

daniellevass deleted the clearSharedSecretOnDisconnected branch April 6, 2020 09:41

Add clearing of shared secret on disconnected #247

Add clearing of shared secret on disconnected #247

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye left a comment

Choose a reason for hiding this comment

Uh oh!

marekoid commented Apr 2, 2020

Uh oh!

marekoid commented Apr 2, 2020

Uh oh!

marekoid commented Apr 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdpye commented Apr 3, 2020

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!

codecov-io commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marekoid commented Apr 2, 2020 •

edited

Loading

codecov-io commented Apr 3, 2020 •

edited

Loading