-
Notifications
You must be signed in to change notification settings - Fork 140
Add clearing of shared secret on disconnected #247
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
Changes from all commits
8ea7340
ddda077
8ae2cb2
670609e
b5059d3
9c8bf51
95d5ca3
0250472
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,9 @@ | |
| import com.pusher.client.channel.PrivateEncryptedChannel; | ||
| import com.pusher.client.channel.PrivateEncryptedChannelEventListener; | ||
| import com.pusher.client.channel.SubscriptionEventListener; | ||
| import com.pusher.client.connection.ConnectionEventListener; | ||
| import com.pusher.client.connection.ConnectionState; | ||
| import com.pusher.client.connection.ConnectionStateChange; | ||
| import com.pusher.client.connection.impl.InternalConnection; | ||
| import com.pusher.client.crypto.nacl.SecretBoxOpener; | ||
| import com.pusher.client.crypto.nacl.SecretBoxOpenerFactory; | ||
|
|
@@ -22,6 +25,23 @@ public class PrivateEncryptedChannelImpl extends ChannelImpl implements PrivateE | |
| private SecretBoxOpenerFactory secretBoxOpenerFactory; | ||
| private SecretBoxOpener secretBoxOpener; | ||
|
|
||
| // For not hanging on to shared secret past the Pusher.disconnect() call, | ||
| // i.e. when not necessary. Pusher.connect(...) call will trigger re-subscribe | ||
| // and hence re-authenticate which creates a new secretBoxOpener. | ||
| private ConnectionEventListener disposeSecretBoxOpenerOnDisconnectedListener = | ||
| new ConnectionEventListener() { | ||
|
|
||
| @Override | ||
| public void onConnectionStateChange(ConnectionStateChange change) { | ||
| disposeSecretBoxOpener(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(String message, String code, Exception e) { | ||
| // nop | ||
| } | ||
| }; | ||
|
|
||
| public PrivateEncryptedChannelImpl(final InternalConnection connection, | ||
| final String channelName, | ||
| final Authorizer authorizer, | ||
|
|
@@ -45,22 +65,38 @@ public void bind(final String eventName, final SubscriptionEventListener listene | |
| super.bind(eventName, listener); | ||
| } | ||
|
|
||
| private String authenticate() { | ||
| @Override | ||
| public String toSubscribeMessage() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| String authKey = authenticate(); | ||
|
|
||
| // create the data part | ||
| final Map<Object, Object> dataMap = new LinkedHashMap<>(); | ||
| dataMap.put("channel", name); | ||
| dataMap.put("auth", authKey); | ||
|
|
||
| // create the wrapper part | ||
| final Map<Object, Object> jsonObject = new LinkedHashMap<>(); | ||
| jsonObject.put("event", "pusher:subscribe"); | ||
| jsonObject.put("data", dataMap); | ||
|
|
||
| return GSON.toJson(jsonObject); | ||
| } | ||
|
|
||
| private String authenticate() { | ||
| try { | ||
| final Map authResponseMap = GSON.fromJson(getAuthResponse(), Map.class); | ||
| final String auth = (String) authResponseMap.get("auth"); | ||
| final String sharedSecret = (String) authResponseMap.get("shared_secret"); | ||
| @SuppressWarnings("rawtypes") // anything goes in JS | ||
| final Map authResponse = GSON.fromJson(getAuthResponse(), Map.class); | ||
|
|
||
| final String auth = (String) authResponse.get("auth"); | ||
| final String sharedSecret = (String) authResponse.get("shared_secret"); | ||
|
|
||
| if (auth == null || sharedSecret == null) { | ||
| throw new AuthorizationFailureException("Didn't receive all the fields expected " + | ||
| "from the Authorizer, expected an auth and shared_secret."); | ||
| } else { | ||
| secretBoxOpener = secretBoxOpenerFactory.create( | ||
| Base64.decode(sharedSecret)); | ||
| createSecretBoxOpener(Base64.decode(sharedSecret)); | ||
| return auth; | ||
| } | ||
|
|
||
| } catch (final AuthorizationFailureException e) { | ||
| throw e; // pass this upwards | ||
| } catch (final Exception e) { | ||
|
|
@@ -69,39 +105,38 @@ private String authenticate() { | |
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String toSubscribeMessage() { | ||
|
|
||
| String authKey = authenticate(); | ||
|
|
||
| // create the data part | ||
| final Map<Object, Object> dataMap = new LinkedHashMap<Object, Object>(); | ||
| dataMap.put("channel", name); | ||
| dataMap.put("auth", authKey); | ||
|
|
||
| // create the wrapper part | ||
| final Map<Object, Object> jsonObject = new LinkedHashMap<Object, Object>(); | ||
| jsonObject.put("event", "pusher:subscribe"); | ||
| jsonObject.put("data", dataMap); | ||
| private void createSecretBoxOpener(byte[] key) { | ||
| secretBoxOpener = secretBoxOpenerFactory.create(key); | ||
| setListenerToDisposeSecretBoxOpenerOnDisconnected(); | ||
| } | ||
|
|
||
| return GSON.toJson(jsonObject); | ||
| private void setListenerToDisposeSecretBoxOpenerOnDisconnected() { | ||
| connection.bind(ConnectionState.DISCONNECTED, | ||
| disposeSecretBoxOpenerOnDisconnectedListener); | ||
| } | ||
|
|
||
| @Override | ||
| public void updateState(ChannelState state) { | ||
| super.updateState(state); | ||
|
|
||
| if (state == ChannelState.UNSUBSCRIBED) { | ||
| tearDownChannel(); | ||
| disposeSecretBoxOpener(); | ||
| } | ||
| } | ||
|
|
||
| private void tearDownChannel() { | ||
| private void disposeSecretBoxOpener() { | ||
| if (secretBoxOpener != null) { | ||
| secretBoxOpener.clearKey(); | ||
| secretBoxOpener = null; | ||
| removeListenerToDisposeSecretBoxOpenerOnDisconnected(); | ||
| } | ||
| } | ||
|
|
||
| private void removeListenerToDisposeSecretBoxOpenerOnDisconnected() { | ||
| connection.unbind(ConnectionState.DISCONNECTED, | ||
| disposeSecretBoxOpenerOnDisconnectedListener); | ||
| } | ||
|
|
||
| private String getAuthResponse() { | ||
| final String socketId = connection.getSocketId(); | ||
| return authorizer.authorize(getName(), socketId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,24 +12,23 @@ | |
| public class PrivateEncryptedChannelExampleApp implements | ||
| ConnectionEventListener, PrivateEncryptedChannelEventListener { | ||
|
|
||
| private String apiKey = "FILL_ME_IN"; | ||
| private String apiKey = "FILL_ME_IN"; // "key" at https://dashboard.pusher.com | ||
| private String channelName = "private-encrypted-channel"; | ||
| private String eventName = "my-event"; | ||
| private String cluster = "eu"; | ||
|
|
||
| private final PrivateEncryptedChannel channel; | ||
| private PrivateEncryptedChannel channel; | ||
|
|
||
| public static void main(final String[] args) { | ||
| new PrivateEncryptedChannelExampleApp(args); | ||
| } | ||
|
|
||
| private PrivateEncryptedChannelExampleApp(final String[] args) { | ||
|
|
||
| if (args.length == 3) { | ||
| apiKey = args[0]; | ||
| channelName = args[1]; | ||
| eventName = args[2]; | ||
| cluster = args[3]; | ||
| switch (args.length) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the |
||
| case 4: cluster = args[3]; | ||
| case 3: eventName = args[2]; | ||
| case 2: channelName = args[1]; | ||
| case 1: apiKey = args[0]; | ||
| } | ||
|
|
||
| final HttpAuthorizer authorizer = new HttpAuthorizer( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package com.pusher.client.channel.impl; | ||
|
|
||
| import static org.mockito.Matchers.*; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| import com.pusher.client.Authorizer; | ||
| import com.pusher.client.channel.ChannelState; | ||
| import com.pusher.client.connection.ConnectionEventListener; | ||
| import com.pusher.client.connection.ConnectionState; | ||
| import com.pusher.client.connection.ConnectionStateChange; | ||
| import com.pusher.client.connection.impl.InternalConnection; | ||
| import com.pusher.client.crypto.nacl.SecretBoxOpener; | ||
| import com.pusher.client.crypto.nacl.SecretBoxOpenerFactory; | ||
| import com.pusher.client.util.Factory; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.mockito.Mock; | ||
| import org.mockito.runners.MockitoJUnitRunner; | ||
| import org.mockito.stubbing.Answer; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) | ||
| public class PrivateEncryptedChannelClearsKeyTest { | ||
|
|
||
| final String CHANNEL_NAME = "private-encrypted-unit-test-channel"; | ||
| final String AUTH_RESPONSE = "{\"auth\":\"636a81ba7e7b15725c00:3ee04892514e8a669dc5d30267221f16727596688894712cad305986e6fc0f3c\",\"shared_secret\":\"iBvNoPVYwByqSfg6anjPpEQ2j051b3rt1Vmnb+z5doo=\"}"; | ||
|
|
||
| @Mock | ||
| InternalConnection mockInternalConnection; | ||
| @Mock | ||
| Authorizer mockAuthorizer; | ||
| @Mock | ||
| Factory mockFactory; | ||
|
|
||
| @Mock | ||
| SecretBoxOpenerFactory mockSecretBoxOpenerFactory; | ||
| @Mock | ||
| SecretBoxOpener mockSecretBoxOpener; | ||
|
|
||
| PrivateEncryptedChannelImpl subject; | ||
|
|
||
| @Before | ||
| public void setUp() { | ||
| when(mockAuthorizer.authorize(eq(CHANNEL_NAME), anyString())).thenReturn(AUTH_RESPONSE); | ||
| when(mockSecretBoxOpenerFactory.create(any())).thenReturn(mockSecretBoxOpener); | ||
|
|
||
| subject = new PrivateEncryptedChannelImpl(mockInternalConnection, CHANNEL_NAME, | ||
| mockAuthorizer, mockFactory, mockSecretBoxOpenerFactory); | ||
| } | ||
|
|
||
| @Test | ||
| public void secretBoxOpenerIsClearedOnUnsubscribed() { | ||
| subject.toSubscribeMessage(); | ||
|
|
||
| subject.updateState(ChannelState.UNSUBSCRIBED); | ||
|
|
||
| verify(mockSecretBoxOpener).clearKey(); | ||
| } | ||
|
|
||
| @Test | ||
| public void secretBoxOpenerIsClearedOnDisconnected() { | ||
| doAnswer((Answer<Void>) invocation -> { | ||
| ConnectionEventListener l = (ConnectionEventListener) invocation.getArguments()[1]; | ||
| l.onConnectionStateChange(new ConnectionStateChange( | ||
| ConnectionState.DISCONNECTING, | ||
| ConnectionState.DISCONNECTED | ||
| )); | ||
| return null; | ||
| }).when(mockInternalConnection).bind(eq(ConnectionState.DISCONNECTED), any()); | ||
| subject.toSubscribeMessage(); | ||
|
|
||
| verify(mockSecretBoxOpener).clearKey(); | ||
| } | ||
|
|
||
| @Test | ||
| public void secretBoxOpenerIsClearedOnceOnUnsubscribedAndThenDisconnected() { | ||
| doAnswer((Answer<Void>) invocation -> { | ||
| subject.updateState(ChannelState.UNSUBSCRIBED); | ||
|
|
||
| ConnectionEventListener l = (ConnectionEventListener) invocation.getArguments()[1]; | ||
| l.onConnectionStateChange(new ConnectionStateChange( | ||
| ConnectionState.DISCONNECTING, | ||
| ConnectionState.DISCONNECTED | ||
| )); | ||
|
|
||
| return null; | ||
| }).when(mockInternalConnection).bind(eq(ConnectionState.DISCONNECTED), any()); | ||
| subject.toSubscribeMessage(); | ||
|
|
||
| verify(mockSecretBoxOpener).clearKey(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit... "disconnected", but you'll find your answer here: https://github.com/pusher/pusher-websocket-java/pull/247/files#diff-48ff2981ea97376b5f1cedec4b59e059R114