some more cleanup in usb classes

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java

@@ -93,7 +93,9 @@ public class CcidTransceiver {
         CcidDataBlock response = receiveDataBlock(sequenceNumber);
 
         long elapsedTime = SystemClock.elapsedRealtime() - startTime;
-        Log.d(Constants.TAG, "USB IccPowerOn call took " + elapsedTime + "ms");
+
+        Log.d(Constants.TAG, "Usb transport connected  T1/TPDU, took " + elapsedTime + "ms, ATR=" +
+                Hex.toHexString(response.getData()));
 
         return response;
     }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransportProtocol.java

@@ -20,5 +20,6 @@ package org.sufficientlysecure.keychain.securitytoken.usb;
 import android.support.annotation.NonNull;
 
 public interface CcidTransportProtocol {
+    void connect(@NonNull CcidTransceiver transceiver) throws UsbTransportException;
     byte[] transceive(@NonNull byte[] apdu) throws UsbTransportException;
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/T1ShortApduProtocol.java

@@ -24,18 +24,16 @@ import org.sufficientlysecure.keychain.securitytoken.usb.CcidTransceiver.CcidDat
 import org.sufficientlysecure.keychain.util.Log;
 
 class T1ShortApduProtocol implements CcidTransportProtocol {
-    private CcidTransceiver mTransceiver;
+    private CcidTransceiver ccidTransceiver;
 
-    T1ShortApduProtocol(CcidTransceiver transceiver) throws UsbTransportException {
+    public void connect(@NonNull CcidTransceiver transceiver) throws UsbTransportException {
-        mTransceiver = transceiver;
+        ccidTransceiver = transceiver;
-
+        ccidTransceiver.iccPowerOn();
-        CcidDataBlock atr = mTransceiver.iccPowerOn();
-        Log.d(Constants.TAG, "Usb transport connected T1/Short APDU, ATR=" + atr);
     }
 
     @Override
     public byte[] transceive(@NonNull final byte[] apdu) throws UsbTransportException {
-        CcidDataBlock response = mTransceiver.sendXfrBlock(apdu);
+        CcidDataBlock response = ccidTransceiver.sendXfrBlock(apdu);
         return response.getData();
     }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java

@@ -54,20 +54,154 @@ public class UsbTransport implements Transport {
     private static final int MASK_EXTENDED_APDU = 0x40000;
 
 
-    private final UsbManager mUsbManager;
+    private final UsbDevice usbDevice;
-    private final UsbDevice mUsbDevice;
+    private final UsbManager usbManager;
-    private UsbInterface mUsbInterface;
+
-    private UsbEndpoint mBulkIn;
+    private UsbDeviceConnection usbConnection;
-    private UsbEndpoint mBulkOut;
+    private UsbInterface usbInterface;
-    private UsbDeviceConnection mConnection;
+    private CcidTransportProtocol ccidTransportProtocol;
-    private CcidTransceiver mTransceiver;
-    private CcidTransportProtocol mProtocol;
 
     public UsbTransport(UsbDevice usbDevice, UsbManager usbManager) {
-        mUsbDevice = usbDevice;
+        this.usbDevice = usbDevice;
-        mUsbManager = usbManager;
+        this.usbManager = usbManager;
     }
 
+    @Override
+    public void release() {
+        if (usbConnection != null) {
+            usbConnection.releaseInterface(usbInterface);
+            usbConnection.close();
+            usbConnection = null;
+        }
+
+        Log.d(Constants.TAG, "Usb transport disconnected");
+    }
+
+    /**
+     * Check if device is was connected to and still is connected
+     * @return true if device is connected
+     */
+    @Override
+    public boolean isConnected() {
+        return usbConnection != null && usbManager.getDeviceList().containsValue(usbDevice) &&
+                usbConnection.getSerial() != null;
+    }
+
+    /**
+     * Check if Transport supports persistent connections e.g connections which can
+     * handle multiple operations in one session
+     * @return true if transport supports persistent connections
+     */
+    @Override
+    public boolean isPersistentConnectionAllowed() {
+        return true;
+    }
+
+    /**
+     * Connect to OTG device
+     */
+    @Override
+    public void connect() throws IOException {
+        usbInterface = getSmartCardInterface(usbDevice);
+        if (usbInterface == null) {
+            // Shouldn't happen as we whitelist only class 11 devices
+            throw new UsbTransportException("USB error - device doesn't have class 11 interface");
+        }
+
+        final Pair<UsbEndpoint, UsbEndpoint> ioEndpoints = getIoEndpoints(usbInterface);
+        UsbEndpoint usbBulkIn = ioEndpoints.first;
+        UsbEndpoint usbBulkOut = ioEndpoints.second;
+
+        if (usbBulkIn == null || usbBulkOut == null) {
+            throw new UsbTransportException("USB error - invalid class 11 interface");
+        }
+
+        usbConnection = usbManager.openDevice(usbDevice);
+        if (usbConnection == null) {
+            throw new UsbTransportException("USB error - failed to connect to device");
+        }
+
+        if (!usbConnection.claimInterface(usbInterface, true)) {
+            throw new UsbTransportException("USB error - failed to claim interface");
+        }
+
+        byte[] rawDescriptors = usbConnection.getRawDescriptors();
+        ccidTransportProtocol = getCcidTransportProtocolForRawDescriptors(rawDescriptors);
+
+        CcidTransceiver transceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut);
+        ccidTransportProtocol.connect(transceiver);
+    }
+
+    private CcidTransportProtocol getCcidTransportProtocolForRawDescriptors(byte[] desc) throws UsbTransportException {
+        int dwProtocols = 0, dwFeatures = 0;
+        boolean hasCcidDescriptor = false;
+
+        ByteBuffer byteBuffer = ByteBuffer.wrap(desc).order(ByteOrder.LITTLE_ENDIAN);
+
+        while (byteBuffer.hasRemaining()) {
+            byteBuffer.mark();
+            byte len = byteBuffer.get(), type = byteBuffer.get();
+
+            if (type == 0x21 && len == 0x36) {
+                byteBuffer.reset();
+
+                byteBuffer.position(byteBuffer.position() + PROTOCOLS_OFFSET);
+                dwProtocols = byteBuffer.getInt();
+
+                byteBuffer.reset();
+
+                byteBuffer.position(byteBuffer.position() + FEATURES_OFFSET);
+                dwFeatures = byteBuffer.getInt();
+                hasCcidDescriptor = true;
+                break;
+            } else {
+                byteBuffer.position(byteBuffer.position() + len - 2);
+            }
+        }
+
+        if (!hasCcidDescriptor) {
+            throw new UsbTransportException("CCID descriptor not found");
+        }
+
+        if ((dwProtocols & MASK_T1_PROTO) == 0) {
+            throw new UsbTransportException("T=0 protocol is not supported");
+        }
+
+        if ((dwFeatures & MASK_TPDU) != 0) {
+            return new T1TpduProtocol();
+        } else if (((dwFeatures & MASK_SHORT_APDU) != 0) || ((dwFeatures & MASK_EXTENDED_APDU) != 0)) {
+            return new T1ShortApduProtocol();
+        } else {
+            throw new UsbTransportException("Character level exchange is not supported");
+        }
+    }
+
+    /**
+     * Transmit and receive data
+     * @param data data to transmit
+     * @return received data
+     */
+    @Override
+    public ResponseAPDU transceive(CommandAPDU data) throws UsbTransportException {
+        return new ResponseAPDU(ccidTransportProtocol.transceive(data.getBytes()));
+    }
+
+    @Override
+    public boolean equals(final Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+
+        final UsbTransport that = (UsbTransport) o;
+
+        return usbDevice != null ? usbDevice.equals(that.usbDevice) : that.usbDevice == null;
+    }
+
+    @Override
+    public int hashCode() {
+        return usbDevice != null ? usbDevice.hashCode() : 0;
+    }
+
+
     /**
      * Get first class 11 (Chip/Smartcard) interface of the device
      *
@@ -108,149 +242,4 @@ public class UsbTransport implements Transport {
         }
         return new Pair<>(bulkIn, bulkOut);
     }
-
-    /**
-     * Release interface and disconnect
-     */
-    @Override
-    public void release() {
-        if (mConnection != null) {
-            mConnection.releaseInterface(mUsbInterface);
-            mConnection.close();
-            mConnection = null;
-        }
-
-        Log.d(Constants.TAG, "Usb transport disconnected");
-    }
-
-    /**
-     * Check if device is was connected to and still is connected
-     * @return true if device is connected
-     */
-    @Override
-    public boolean isConnected() {
-        return mConnection != null && mUsbManager.getDeviceList().containsValue(mUsbDevice) &&
-                mConnection.getSerial() != null;
-    }
-
-    /**
-     * Check if Transport supports persistent connections e.g connections which can
-     * handle multiple operations in one session
-     * @return true if transport supports persistent connections
-     */
-    @Override
-    public boolean isPersistentConnectionAllowed() {
-        return true;
-    }
-
-    /**
-     * Connect to OTG device
-     * @throws IOException
-     */
-    @Override
-    public void connect() throws IOException {
-        mUsbInterface = getSmartCardInterface(mUsbDevice);
-        if (mUsbInterface == null) {
-            // Shouldn't happen as we whitelist only class 11 devices
-            throw new UsbTransportException("USB error - device doesn't have class 11 interface");
-        }
-
-        final Pair<UsbEndpoint, UsbEndpoint> ioEndpoints = getIoEndpoints(mUsbInterface);
-        mBulkIn = ioEndpoints.first;
-        mBulkOut = ioEndpoints.second;
-
-        if (mBulkIn == null || mBulkOut == null) {
-            throw new UsbTransportException("USB error - invalid class 11 interface");
-        }
-
-        mConnection = mUsbManager.openDevice(mUsbDevice);
-        if (mConnection == null) {
-            throw new UsbTransportException("USB error - failed to connect to device");
-        }
-
-        if (!mConnection.claimInterface(mUsbInterface, true)) {
-            throw new UsbTransportException("USB error - failed to claim interface");
-        }
-
-        mTransceiver = new CcidTransceiver(mConnection, mBulkIn, mBulkOut);
-
-
-
-        configureProtocol();
-    }
-
-    private void configureProtocol() throws UsbTransportException {
-        byte[] desc = mConnection.getRawDescriptors();
-        int dwProtocols = 0, dwFeatures = 0;
-        boolean hasCcidDescriptor = false;
-
-        ByteBuffer byteBuffer = ByteBuffer.wrap(desc).order(ByteOrder.LITTLE_ENDIAN);
-
-        while (byteBuffer.hasRemaining()) {
-            byteBuffer.mark();
-            byte len = byteBuffer.get(), type = byteBuffer.get();
-
-            if (type == 0x21 && len == 0x36) {
-                byteBuffer.reset();
-
-                byteBuffer.position(byteBuffer.position() + PROTOCOLS_OFFSET);
-                dwProtocols = byteBuffer.getInt();
-
-                byteBuffer.reset();
-
-                byteBuffer.position(byteBuffer.position() + FEATURES_OFFSET);
-                dwFeatures = byteBuffer.getInt();
-                hasCcidDescriptor = true;
-                break;
-            } else {
-                byteBuffer.position(byteBuffer.position() + len - 2);
-            }
-        }
-
-        if (!hasCcidDescriptor) {
-            throw new UsbTransportException("CCID descriptor not found");
-        }
-
-        if ((dwProtocols & MASK_T1_PROTO) == 0) {
-            throw new UsbTransportException("T=0 protocol is not supported");
-        }
-
-        if ((dwFeatures & MASK_TPDU) != 0) {
-            mProtocol = new T1TpduProtocol(mTransceiver);
-        } else if (((dwFeatures & MASK_SHORT_APDU) != 0) || ((dwFeatures & MASK_EXTENDED_APDU) != 0)) {
-            mProtocol = new T1ShortApduProtocol(mTransceiver);
-        } else {
-            throw new UsbTransportException("Character level exchange is not supported");
-        }
-    }
-
-    /**
-     * Transmit and receive data
-     * @param data data to transmit
-     * @return received data
-     * @throws UsbTransportException
-     */
-    @Override
-    public ResponseAPDU transceive(CommandAPDU data) throws UsbTransportException {
-        return new ResponseAPDU(mProtocol.transceive(data.getBytes()));
-    }
-
-    @Override
-    public boolean equals(final Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-
-        final UsbTransport that = (UsbTransport) o;
-
-        return mUsbDevice != null ? mUsbDevice.equals(that.mUsbDevice) : that.mUsbDevice == null;
-    }
-
-    @Override
-    public int hashCode() {
-        return mUsbDevice != null ? mUsbDevice.hashCode() : 0;
-    }
-
-    public UsbDevice getUsbDevice() {
-        return mUsbDevice;
-    }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/tpdu/T1TpduProtocol.java

@@ -18,6 +18,7 @@
 package org.sufficientlysecure.keychain.securitytoken.usb.tpdu;
 
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 
 import org.bouncycastle.util.Arrays;
 import org.sufficientlysecure.keychain.Constants;
@@ -30,22 +31,28 @@ import org.sufficientlysecure.keychain.util.Log;
 public class T1TpduProtocol implements CcidTransportProtocol {
     private final static int MAX_FRAME_LEN = 254;
 
-    private byte mCounter = 0;
-    private final CcidTransceiver ccidTransceiver;
-    private final BlockChecksumType checksumType;
 
-    public T1TpduProtocol(CcidTransceiver transceiver) throws UsbTransportException {
+    private CcidTransceiver ccidTransceiver;
-        ccidTransceiver = transceiver;
+    private BlockChecksumType checksumType;
+
+    private byte mCounter = 0;
+
+
+    public void connect(@NonNull CcidTransceiver ccidTransceiver) throws UsbTransportException {
+        if (this.ccidTransceiver != null) {
+            throw new IllegalStateException("Protocol already connected!");
+        }
+        this.ccidTransceiver = ccidTransceiver;
 
         // Connect
-        CcidDataBlock response = ccidTransceiver.iccPowerOn();
+        CcidDataBlock response = this.ccidTransceiver.iccPowerOn();
-        Log.d(Constants.TAG, "Usb transport connected  T1/TPDU, ATR=" + response);
 
         // TODO: set checksum from atr
         checksumType = BlockChecksumType.LRC;
 
         // PPS all auto
         pps();
+
     }
 
     private void pps() throws UsbTransportException {
@@ -56,6 +63,10 @@ public class T1TpduProtocol implements CcidTransportProtocol {
     }
 
     public byte[] transceive(@NonNull byte[] apdu) throws UsbTransportException {
+        if (this.ccidTransceiver == null) {
+            throw new IllegalStateException("Protocol not connected!");
+        }
+
         int start = 0;
 
         if (apdu.length == 0) {
@@ -118,7 +129,7 @@ public class T1TpduProtocol implements CcidTransportProtocol {
     }
 
     // Factory methods
-    public Block getBlockFromResponse(CcidDataBlock dataBlock) throws UsbTransportException {
+    private Block getBlockFromResponse(CcidDataBlock dataBlock) throws UsbTransportException {
         final Block baseBlock = new Block(checksumType, dataBlock.getData());
 
         if ((baseBlock.getPcb() & IBlock.MASK_RBLOCK) == IBlock.MASK_VALUE_RBLOCK) {
@@ -132,11 +143,11 @@ public class T1TpduProtocol implements CcidTransportProtocol {
         throw new UsbTransportException("TPDU Unknown block type");
     }
 
-    public IBlock newIBlock(byte sequence, boolean chaining, byte[] apdu) throws UsbTransportException {
+    private IBlock newIBlock(byte sequence, boolean chaining, byte[] apdu) throws UsbTransportException {
         return new IBlock(checksumType, (byte) 0, sequence, chaining, apdu);
     }
 
-    public RBlock newRBlock(byte sequence) throws UsbTransportException {
+    private RBlock newRBlock(byte sequence) throws UsbTransportException {
         return new RBlock(checksumType, (byte) 0, sequence);
     }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/base/BaseSecurityTokenActivity.java

@@ -112,21 +112,26 @@ public abstract class BaseSecurityTokenActivity extends BaseActivity
         onSecurityTokenError(error);
     }
 
-    public void tagDiscovered(final Tag tag) {
+    public void tagDiscovered(Tag tag) {
         // Actual NFC operations are executed in doInBackground to not block the UI thread
-        if (!mTagHandlingEnabled)
+        if (!mTagHandlingEnabled) {
             return;
+        }
 
-        securityTokenDiscovered(new NfcTransport(tag));
+        NfcTransport nfcTransport = new NfcTransport(tag);
+        securityTokenDiscovered(nfcTransport);
     }
 
-    public void usbDeviceDiscovered(final UsbDevice usbDevice) {
+    public void usbDeviceDiscovered(UsbDevice usbDevice) {
         // Actual USB operations are executed in doInBackground to not block the UI thread
-        if (!mTagHandlingEnabled)
+        if (!mTagHandlingEnabled) {
             return;
+        }
 
         UsbManager usbManager = (UsbManager) getSystemService(Context.USB_SERVICE);
-        securityTokenDiscovered(new UsbTransport(usbDevice, usbManager));
+
+        UsbTransport usbTransport = new UsbTransport(usbDevice, usbManager);
+        securityTokenDiscovered(usbTransport);
     }
 
     public void securityTokenDiscovered(final Transport transport) {