fix: create Cipher instance in place, do not store it in ChannelOptions

lib/src/main/java/io/ably/lib/rest/DeviceDetails.java

@@ -142,6 +142,9 @@ public boolean equals(Object o) {
         thisJson.remove("deviceSecret");
         otherJson.remove("deviceSecret");
 
+        normalizeRecipientField(thisJson);
+        normalizeRecipientField(otherJson);
+
         if ((this.metadata == null || this.metadata.entrySet().isEmpty()) && (other.metadata == null || other.metadata.entrySet().isEmpty())) {
             // Empty metadata == null metadata.
             thisJson.remove("metadata");
@@ -170,4 +173,20 @@ public DeviceDetails fromJsonElement(JsonElement e) {
     public static HttpCore.ResponseHandler<DeviceDetails> httpResponseHandler = new Serialisation.HttpResponseHandler<DeviceDetails>(DeviceDetails.class, fromJsonElement);
 
     public static HttpCore.BodyHandler<DeviceDetails> httpBodyHandler = new Serialisation.HttpBodyHandler<DeviceDetails>(DeviceDetails[].class, fromJsonElement);
+
+    /**
+     * Push recipient can contain some additional field, but `transportType`, `deviceToken`, `registrationToken` only matters for equals
+     */
+    private static void normalizeRecipientField(JsonObject deviceDetailsJson) {
+        JsonElement push = deviceDetailsJson.get("push");
+        if (push == null) return;
+        JsonElement recipient = push.getAsJsonObject().get("recipient");
+        if (recipient == null) return;
+        JsonObject normalizedRecipient = JsonUtils.object()
+                .add("transportType", recipient.getAsJsonObject().get("transportType"))
+                .add("deviceToken", recipient.getAsJsonObject().get("deviceToken"))
+                .add("registrationToken", recipient.getAsJsonObject().get("registrationToken"))
+                .toJson();
+        push.getAsJsonObject().add("recipient", normalizedRecipient);
+    }
 }

lib/src/main/java/io/ably/lib/types/BaseMessage.java

@@ -8,7 +8,9 @@
 import com.google.gson.JsonParseException;
 import com.google.gson.JsonPrimitive;
 import io.ably.lib.util.Base64Coder;
+import io.ably.lib.util.Crypto;
 import io.ably.lib.util.Crypto.EncryptingChannelCipher;
+import io.ably.lib.util.Crypto.DecryptingChannelCipher;
 import io.ably.lib.util.Log;
 import io.ably.lib.util.Serialisation;
 import org.msgpack.core.MessageFormat;
@@ -145,7 +147,8 @@ public void decode(ChannelOptions opts,  DecodingContext context) throws Message
                         case "cipher":
                             if(opts != null && opts.encrypted) {
                                 try {
-                                    data = opts.getCipherSet().getDecipher().decrypt((byte[]) data);
+                                    DecryptingChannelCipher cipher = Crypto.createChannelDecipher(opts.getCipherParamsOrDefault());
+                                    data = cipher.decrypt((byte[]) data);
                                 } catch(AblyException e) {
                                     throw MessageDecodeException.fromDescription(e.errorInfo.message);
                                 }
@@ -193,7 +196,7 @@ public void encode(ChannelOptions opts) throws AblyException {
             }
         }
         if (opts != null && opts.encrypted) {
-            EncryptingChannelCipher cipher = opts.getCipherSet().getEncipher();
+            EncryptingChannelCipher cipher = Crypto.createChannelEncipher(opts.getCipherParamsOrDefault());
             data = cipher.encrypt((byte[]) data);
             encoding = ((encoding == null) ? "" : encoding + "/") + "cipher+" + cipher.getAlgorithm();
         }

lib/src/main/java/io/ably/lib/types/ChannelOptions.java

@@ -4,8 +4,7 @@
 
 import io.ably.lib.util.Base64Coder;
 import io.ably.lib.util.Crypto;
-import io.ably.lib.util.Crypto.ChannelCipher;
-import io.ably.lib.util.Crypto.ChannelCipherSet;
+import io.ably.lib.util.Crypto.CipherParams;
 
 /**
  * Passes additional properties to a {@link io.ably.lib.rest.Channel} or {@link io.ably.lib.realtime.Channel} object,
@@ -27,8 +26,6 @@ public class ChannelOptions {
      */
     public ChannelMode[] modes;
 
-    private ChannelCipherSet cipherSet;
-
     /**
      * Requests encryption for this channel when not null,
      * and specifies encryption-related parameters (such as algorithm, chaining mode, key length and key).
@@ -59,58 +56,6 @@ public int getModeFlags() {
         return flags;
     }
 
-    /**
-     * Returns a wrapper around the cipher set to be used for this channel. This wrapper is only available in this API
-     * to support customers who may have been using it in their applications with version 1.2.10 or before.
-     *
-     * @deprecated Since version 1.2.11, this method (which was only ever intended for internal use within this library
-     * has been replaced by {@link #getCipherSet()}. It will be removed in the future.
-     */
-    @Deprecated
-    public ChannelCipher getCipher() throws AblyException {
-        return new ChannelCipher() {
-            @Override
-            public byte[] encrypt(byte[] plaintext) throws AblyException {
-                return getCipherSet().getEncipher().encrypt(plaintext);
-            }
-
-            @Override
-            public byte[] decrypt(byte[] ciphertext) throws AblyException {
-                return getCipherSet().getDecipher().decrypt(ciphertext);
-            }
-
-            @Override
-            public String getAlgorithm() {
-                try {
-                    return getCipherSet().getEncipher().getAlgorithm();
-                } catch (final AblyException e) {
-                    throw new IllegalStateException("Unexpected exception when using legacy crypto cipher interface.", e);
-                }
-            }
-        };
-    }
-
-    /**
-     * Internal; this method is not intended for use by application developers. It may be changed or removed in future.
-     *
-     * Returns the cipher set to be used for encrypting and decrypting data on a channel, given the current state of
-     * this instance. On the first call to this method a new cipher set instance is created, with subsequent callers to
-     * this method being returned that same cipher set instance. This method is safe to be called from any thread.
-     *
-     * @apiNote Once this method has been called then the cipher set is fixed based on the value of the
-     * {@link #cipherParams} field at that time. If that field is then mutated, the cipher set will not be updated.
-     * This is not great API design and we should fix this under https://github.com/ably/ably-java/issues/745
-     */
-    public synchronized ChannelCipherSet getCipherSet() throws AblyException {
-        if (!encrypted) {
-            throw new IllegalStateException("ChannelOptions encrypted field value is false.");
-        }
-        if (null == cipherSet) {
-            cipherSet = Crypto.createChannelCipherSet(cipherParams);
-        }
-        return cipherSet;
-    }
-
     /**
      * <b>Deprecated. Use withCipherKey(byte[]) instead.</b><br><br>
      * Create ChannelOptions from the given cipher key.
@@ -161,4 +106,15 @@ public static ChannelOptions withCipherKey(byte[] key) throws AblyException {
     public static ChannelOptions withCipherKey(String base64Key) throws AblyException {
         return withCipherKey(Base64Coder.decode(base64Key));
     }
+
+    /**
+     * Internal; returns cipher params or generate default
+     */
+    public synchronized CipherParams getCipherParamsOrDefault() throws AblyException {
+        CipherParams params = Crypto.checkCipherParams(this.cipherParams);
+        if (this.cipherParams == null) {
+            this.cipherParams = params;
+        }
+        return params;
+    }
 }

lib/src/main/java/io/ably/lib/util/Crypto.java

@@ -6,7 +6,6 @@
 import java.security.SecureRandom;
 import java.util.ConcurrentModificationException;
 import java.util.Locale;
-import java.util.concurrent.Semaphore;
 
 import javax.crypto.BadPaddingException;
 import javax.crypto.Cipher;
@@ -176,22 +175,6 @@ public static byte[] generateRandomKey() {
         return generateRandomKey(DEFAULT_KEYLENGTH);
     }
 
-    /**
-     * Interface for a ChannelCipher instance that may be associated with a Channel.
-     *
-     * The operational methods implemented by channel cipher instances (encrypt and decrypt) are not designed to be
-     * safe to be called from any thread.
-     *
-     * @deprecated Since version 1.2.11, this interface (which was only ever intended for internal use within this
-     * library) has been replaced by {@link ChannelCipherSet}. It will be removed in the future.
-     */
-    @Deprecated
-    public interface ChannelCipher {
-        byte[] encrypt(byte[] plaintext) throws AblyException;
-        byte[] decrypt(byte[] ciphertext) throws AblyException;
-        String getAlgorithm();
-    }
-
     /**
      * Internal; a cipher used to encrypt plaintext to ciphertext, for a channel.
      */
@@ -227,40 +210,30 @@ public interface DecryptingChannelCipher {
     }
 
     /**
-     * Internal; a matching encipher and decipher pair, where both are guaranteed to have been configured with the same
-     * {@link CipherParams} as each other.
+     * Internal; get an encrypting cipher instance based on the given channel options.
      */
-    public interface ChannelCipherSet {
-        EncryptingChannelCipher getEncipher();
-        DecryptingChannelCipher getDecipher();
+    public static EncryptingChannelCipher createChannelEncipher(final CipherParams cipherParams) throws AblyException {
+        return new EncryptingCBCCipher(cipherParams);
     }
 
     /**
-     * Internal; get an encrypting cipher instance based on the given channel options.
+     * Internal; get a decrypting cipher instance based on the given channel options.
      */
-    public static ChannelCipherSet createChannelCipherSet(final Object cipherParams) throws AblyException {
-        final CipherParams nonNullParams;
-        if (null == cipherParams)
-            nonNullParams = Crypto.getDefaultParams();
-        else if (cipherParams instanceof CipherParams)
-            nonNullParams = (CipherParams)cipherParams;
-        else
-            throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));
-
-        return new ChannelCipherSet() {
-            private final EncryptingChannelCipher encipher = new EncryptingCBCCipher(nonNullParams);
-            private final DecryptingChannelCipher decipher = new DecryptingCBCCipher(nonNullParams);
-
-            @Override
-            public EncryptingChannelCipher getEncipher() {
-                return encipher;
-            }
+    public static DecryptingChannelCipher createChannelDecipher(final CipherParams cipherParams) throws AblyException {
+        return new DecryptingCBCCipher(cipherParams);
+    }
 
-            @Override
-            public DecryptingChannelCipher getDecipher() {
-                return decipher;
-            }
-        };
+    /**
+     * Internal; if `cipherParams` is null returns default params otherwise check if params valid and returns them
+     */
+    public static CipherParams checkCipherParams(final Object cipherParams) throws AblyException {
+        if (null == cipherParams) {
+            return Crypto.getDefaultParams();
+        } else if (cipherParams instanceof CipherParams) {
+            return (CipherParams) cipherParams;
+        } else {
+            throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));
+        }
     }
 
     /**
@@ -277,7 +250,6 @@ private static class CBCCipher {
         protected final Cipher cipher;
         protected final int blockLength;
         protected final String algorithm;
-        private final Semaphore semaphore = new Semaphore(1);
 
         protected CBCCipher(final CipherParams params) throws AblyException {
             final String cipherAlgorithm = params.getAlgorithm();
@@ -293,28 +265,6 @@ protected CBCCipher(final CipherParams params) throws AblyException {
                 throw AblyException.fromThrowable(e);
             }
         }
-
-        /**
-         * Subclasses must call this method before performing any work that uses the {@link #cipher} or otherwise
-         * mutates the state of this instance.
-         *
-         * TODO: under https://github.com/ably/ably-java/issues/747 we can then:
-         * - remove the need for the {@link #releaseOperationalPermit()} method, and
-         * - make this method return an AutoCloseable implementation that releases the semaphore.
-         */
-        protected void acquireOperationalPermit() {
-            if (!semaphore.tryAcquire()) {
-                throw new ConcurrentModificationException("ChannelCipher instances are not designed to be operated from multiple threads simultaneously.");
-            }
-        }
-
-        /**
-         * Subclasses must call this method after performing any work that uses the {@link #cipher} or otherwise
-         * mutates the state of this instance.
-         */
-        protected void releaseOperationalPermit() {
-            semaphore.release();
-        }
     }
 
     private static class EncryptingCBCCipher extends CBCCipher implements EncryptingChannelCipher {
@@ -390,23 +340,17 @@ private byte[] getNextIv() {
         public byte[] encrypt(byte[] plaintext) {
             if (plaintext == null) return null;
 
-            acquireOperationalPermit();
-            try {
-                final int plaintextLength = plaintext.length;
-                final int paddedLength = getPaddedLength(plaintextLength);
-                final byte[] cipherIn = new byte[paddedLength];
-                final byte[] ciphertext = new byte[paddedLength + blockLength];
-                final int padding = paddedLength - plaintextLength;
-                System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
-                System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
-                System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
-                final byte[] cipherOut = cipher.update(cipherIn);
-                System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
-                return ciphertext;
-            } finally {
-                // TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
-                releaseOperationalPermit();
-            }
+            final int plaintextLength = plaintext.length;
+            final int paddedLength = getPaddedLength(plaintextLength);
+            final byte[] cipherIn = new byte[paddedLength];
+            final byte[] ciphertext = new byte[paddedLength + blockLength];
+            final int padding = paddedLength - plaintextLength;
+            System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
+            System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
+            System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
+            final byte[] cipherOut = cipher.update(cipherIn);
+            System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
+            return ciphertext;
         }
     }
 
@@ -417,17 +361,13 @@ private static class DecryptingCBCCipher extends CBCCipher implements Decrypting
 
         @Override
         public byte[] decrypt(byte[] ciphertext) throws AblyException {
-            if(ciphertext == null) return null;
+            if (ciphertext == null) return null;
 
-            acquireOperationalPermit();
             try {
                 cipher.init(Cipher.DECRYPT_MODE, keySpec, new IvParameterSpec(ciphertext, 0, blockLength));
                 return cipher.doFinal(ciphertext, blockLength, ciphertext.length - blockLength);
             } catch (InvalidAlgorithmParameterException | IllegalBlockSizeException | BadPaddingException | InvalidKeyException e) {
                 throw AblyException.fromThrowable(e);
-            } finally {
-                // TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
-                releaseOperationalPermit();
             }
         }
     }

lib/src/test/java/io/ably/lib/rest/DeviceDetailsTest.java

@@ -0,0 +1,37 @@
+package io.ably.lib.rest;
+
+import io.ably.lib.util.JsonUtils;
+import org.junit.Test;
+
+import static org.junit.Assert.assertTrue;
+
+public class DeviceDetailsTest {
+
+    @Test
+    public void shouldIgnoreUnrelatedRecipientFields() {
+        DeviceDetails details = DeviceDetails.fromJsonObject(JsonUtils.object()
+                .add("id", "testDeviceDetails")
+                .add("platform", "ios")
+                .add("formFactor", "phone")
+                .add("metadata", JsonUtils.object())
+                .add("push", JsonUtils.object()
+                        .add("recipient", JsonUtils.object()
+                                .add("transportType", "apns")
+                                .add("deviceToken", "foo")
+                                .add("apnsDeviceTokens", JsonUtils.object().add("default", "foo"))))
+                .toJson());
+
+        DeviceDetails otherDetails = DeviceDetails.fromJsonObject(JsonUtils.object()
+                .add("id", "testDeviceDetails")
+                .add("platform", "ios")
+                .add("formFactor", "phone")
+                .add("metadata", JsonUtils.object())
+                .add("push", JsonUtils.object()
+                        .add("recipient", JsonUtils.object()
+                                .add("transportType", "apns")
+                                .add("deviceToken", "foo")))
+                .toJson());
+
+        assertTrue("Should ignore `apnsDeviceTokens` field",  details.equals(otherDetails));
+    }
+}