fix(iot-dev): Fix bug where routine MQTT SAS token expired disconnects didn't provide the correct reason

device/iot-device-client/src/main/java/com/microsoft/azure/sdk/iot/device/auth/IotHubSasTokenAuthenticationProvider.java

@@ -88,6 +88,12 @@ public boolean isAuthenticationProviderRenewalNecessary()
         return (this.sasToken != null && this.sasToken.isExpired());
     }
 
+    public boolean isSasTokenExpired()
+    {
+        //similar to isAuthenticationProviderRenewalNecessary, but this won't be overriden by most classes that inherit from this class
+        return (this.sasToken != null && this.sasToken.isExpired());
+    }
+
     /**
      * Returns true if the saved token should be refreshed
      *

device/iot-device-client/src/main/java/com/microsoft/azure/sdk/iot/device/auth/IotHubSasTokenProvidedAuthenticationProvider.java

@@ -74,4 +74,9 @@ public int getMillisecondsBeforeProactiveRenewal()
         double timeBufferMultiplier = this.timeBufferPercentage / 100.0; //Convert 85 to .85, for example. Percentage multipliers are in decimal
         return (int) (tokenValidSeconds * 1000 * timeBufferMultiplier);
     }
+
+    public boolean isSasTokenExpired()
+    {
+        return (lastSasToken != null && IotHubSasToken.isExpired(new String(lastSasToken)));
+    }
 }

device/iot-device-client/src/main/java/com/microsoft/azure/sdk/iot/device/transport/IotHubTransport.java

@@ -422,7 +422,7 @@ public void open(boolean withRetry) throws TransportException
         // registering any devices to it. No need to check for SAS token expiry if no devices are registered yet.
         if (this.getDefaultConfig() != null)
         {
-            if (this.isSasTokenExpired())
+            if (this.isAuthenticationProviderExpired())
             {
                 throw new SecurityException("Your sas token has expired");
             }
@@ -1066,12 +1066,7 @@ private IotHubConnectionStatusChangeReason exceptionToStatusChangeReason(Throwab
         if (e instanceof TransportException)
         {
             TransportException transportException = (TransportException) e;
-            if (transportException.isRetryable())
-            {
-                log.debug("Mapping throwable to NO_NETWORK because it was a retryable exception", e);
-                return IotHubConnectionStatusChangeReason.NO_NETWORK;
-            }
-            else if (isSasTokenExpired())
+            if (isSasTokenExpired())
             {
                 log.debug("Mapping throwable to EXPIRED_SAS_TOKEN because it was a non-retryable exception and the saved sas token has expired", e);
                 return IotHubConnectionStatusChangeReason.EXPIRED_SAS_TOKEN;
@@ -1081,6 +1076,11 @@ else if (e instanceof UnauthorizedException || e instanceof MqttUnauthorizedExce
                 log.debug("Mapping throwable to BAD_CREDENTIAL because it was a non-retryable exception authorization exception but the saved sas token has not expired yet", e);
                 return IotHubConnectionStatusChangeReason.BAD_CREDENTIAL;
             }
+            else if (transportException.isRetryable())
+            {
+                log.debug("Mapping throwable to NO_NETWORK because it was a retryable exception", e);
+                return IotHubConnectionStatusChangeReason.NO_NETWORK;
+            }
         }
 
         log.debug("Mapping exception throwable to COMMUNICATION_ERROR because the sdk was unable to classify the thrown exception to anything other category", e);
@@ -1520,7 +1520,7 @@ private boolean isMessageValid(IotHubTransportPacket packet)
             return false;
         }
 
-        if (isSasTokenExpired())
+        if (isAuthenticationProviderExpired())
         {
             log.debug("Creating a callback for the message with expired sas token with UNAUTHORIZED status");
             packet.setStatus(IotHubStatusCode.UNAUTHORIZED);
@@ -1657,7 +1657,7 @@ else if (this.connectionStatusChangeCallbacks.containsKey(deviceId))
     // check SAS token expiry when using SAS based auth, and there is always a SAS token authentication provider
     // when using SAS based auth.
     @SuppressWarnings("ConstantConditions")
-    private boolean isSasTokenExpired()
+    private boolean isAuthenticationProviderExpired()
     {
         if (this.getDefaultConfig() == null)
         {
@@ -1668,6 +1668,21 @@ private boolean isSasTokenExpired()
                 && this.getDefaultConfig().getSasTokenAuthentication().isAuthenticationProviderRenewalNecessary();
     }
 
+    // warning is about how getSasTokenAuthentication() may return null. In this case, it never will since we only
+    // check SAS token expiry when using SAS based auth, and there is always a SAS token authentication provider
+    // when using SAS based auth.
+    @SuppressWarnings("ConstantConditions")
+    private boolean isSasTokenExpired()
+    {
+        if (this.getDefaultConfig() == null)
+        {
+            return false;
+        }
+
+        return this.getDefaultConfig().getAuthenticationType() == DeviceClientConfig.AuthType.SAS_TOKEN
+            && this.getDefaultConfig().getSasTokenAuthentication().isSasTokenExpired();
+    }
+
     /**
      * Returns if the provided packet has lasted longer than the device operation timeout
      *
@@ -1832,7 +1847,7 @@ private static void sleepUninterruptibly(long sleepFor, TimeUnit unit)
      */
     private void checkForUnauthorizedException(TransportException transportException)
     {
-        if (!this.isSasTokenExpired() && (transportException instanceof MqttUnauthorizedException
+        if (!this.isAuthenticationProviderExpired() && (transportException instanceof MqttUnauthorizedException
                 || transportException instanceof UnauthorizedException
                 || transportException instanceof AmqpUnauthorizedAccessException))
         {

device/iot-device-client/src/test/java/com/microsoft/azure/sdk/iot/device/transport/IotHubTransportTest.java

@@ -523,7 +523,7 @@ public void openThrowsIfSasTokenExpired() throws DeviceClientException
         //arrange
         new MockUp<IotHubTransport>()
         {
-            @Mock boolean isSasTokenExpired()
+            @Mock boolean isAuthenticationProviderExpired()
             {
                 return true;
             }
@@ -781,14 +781,6 @@ public void exceptionToStatusChangeReasonSasTokenExpired()
         };
         final IotHubTransport transport = new IotHubTransport(mockedConfig, mockedIotHubConnectionStatusChangeCallback, false);
 
-        new Expectations()
-        {
-            {
-                mockedTransportException.isRetryable();
-                result = false;
-            }
-        };
-
         //act
         IotHubConnectionStatusChangeReason reason = Deencapsulation.invoke(transport, "exceptionToStatusChangeReason", new Class[] {Throwable.class}, mockedTransportException);
 
@@ -2453,7 +2445,7 @@ public void isMessageValidWithMessageNotExpiredSasTokenExpired()
                 methodsCalled.append("addToCallbackQueue");
             }
 
-            @Mock boolean isSasTokenExpired()
+            @Mock boolean isAuthenticationProviderExpired()
             {
                 return true;
             }
@@ -2698,7 +2690,7 @@ public void isSasTokenExpiredAuthenticationTypeIsSasTokenAndExpired()
             {
                 mockedConfig.getAuthenticationType();
                 result = DeviceClientConfig.AuthType.SAS_TOKEN;
-                mockedConfig.getSasTokenAuthentication().isAuthenticationProviderRenewalNecessary();
+                mockedConfig.getSasTokenAuthentication().isSasTokenExpired();
                 result = true;
             }
         };
@@ -2952,7 +2944,7 @@ public void checkForUnauthorizedExceptionWithExpiredSASToken()
         //arrange
         new MockUp<IotHubTransport>()
         {
-            @Mock boolean isSasTokenExpired()
+            @Mock boolean isAuthenticationProviderExpired()
             {
                 return true;
             }

iot-e2e-tests/common/src/test/java/tests/integration/com/microsoft/azure/sdk/iot/iothub/TokenRenewalTests.java

@@ -150,17 +150,27 @@ public void tokenRenewalWorks() throws Exception
         Success[] amqpDisconnectDidNotHappenSuccesses = new Success[clients.size()];
         Success[] mqttDisconnectDidHappenSuccesses = new Success[clients.size()];
         Success[] shutdownWasGracefulSuccesses = new Success[clients.size()];
+        Success[] mqttDisconnectHadTokenExpiredReasonSuccesses = new Success[clients.size()];
         for (int clientIndex = 0; clientIndex < clients.size(); clientIndex++)
         {
             amqpDisconnectDidNotHappenSuccesses[clientIndex] = new Success();
             mqttDisconnectDidHappenSuccesses[clientIndex] = new Success();
             shutdownWasGracefulSuccesses[clientIndex] = new Success();
-
+            mqttDisconnectHadTokenExpiredReasonSuccesses[clientIndex] = new Success();
+
             amqpDisconnectDidNotHappenSuccesses[clientIndex].setResult(true); //assume success until unexpected DISCONNECTED_RETRYING
             mqttDisconnectDidHappenSuccesses[clientIndex].setResult(false); //assume failure until DISCONNECTED_RETRYING is triggered by token expiring
             shutdownWasGracefulSuccesses[clientIndex].setResult(true); //assume success until DISCONNECTED callback without CLIENT_CLOSE
-
-            clients.get(clientIndex).registerConnectionStatusChangeCallback(new IotHubConnectionStatusChangeTokenRenewalCallbackVerifier(clients.get(clientIndex).getConfig().getProtocol(), amqpDisconnectDidNotHappenSuccesses[clientIndex], mqttDisconnectDidHappenSuccesses[clientIndex], shutdownWasGracefulSuccesses[clientIndex]), clients.get(clientIndex));
+            mqttDisconnectHadTokenExpiredReasonSuccesses[clientIndex].setResult(false); //assume failure until first disconnected_retrying executes with reason EXPIRED_SAS_TOKEN
+
+            clients.get(clientIndex).registerConnectionStatusChangeCallback(
+                new IotHubConnectionStatusChangeTokenRenewalCallbackVerifier(
+                    clients.get(clientIndex).getConfig().getProtocol(),
+                    amqpDisconnectDidNotHappenSuccesses[clientIndex],
+                    mqttDisconnectDidHappenSuccesses[clientIndex],
+                    shutdownWasGracefulSuccesses[clientIndex],
+                    mqttDisconnectHadTokenExpiredReasonSuccesses[clientIndex]),
+                clients.get(clientIndex));
         }
 
         openEachClient(clients);
@@ -182,7 +192,7 @@ public void tokenRenewalWorks() throws Exception
 
         testIdentities.clear();
 
-        verifyClientsConnectivityBehavedCorrectly(clients, amqpDisconnectDidNotHappenSuccesses, mqttDisconnectDidHappenSuccesses, shutdownWasGracefulSuccesses);
+        verifyClientsConnectivityBehavedCorrectly(clients, amqpDisconnectDidNotHappenSuccesses, mqttDisconnectDidHappenSuccesses, shutdownWasGracefulSuccesses, mqttDisconnectHadTokenExpiredReasonSuccesses);
     }
 
     private void closeClients(List<InternalClient> clients) throws IOException
@@ -201,7 +211,12 @@ private void closeClients(List<InternalClient> clients) throws IOException
         }
     }
 
-    private void verifyClientsConnectivityBehavedCorrectly(List<InternalClient> clients, Success[] amqpDisconnectDidNotHappenSuccesses, Success[] mqttDisconnectDidHappenSuccesses, Success[] shutdownWasGracefulSuccesses)
+    private void verifyClientsConnectivityBehavedCorrectly(
+        List<InternalClient> clients,
+        Success[] amqpDisconnectDidNotHappenSuccesses,
+        Success[] mqttDisconnectDidHappenSuccesses,
+        Success[] shutdownWasGracefulSuccesses,
+        Success[] mqttDisconnectHadTokenExpiredReasonSuccesses)
     {
         for (int clientIndex = 0; clientIndex < clients.size(); clientIndex++)
         {
@@ -212,6 +227,7 @@ private void verifyClientsConnectivityBehavedCorrectly(List<InternalClient> clie
             {
                 assertTrue(CorrelationDetailsLoggingAssert.buildExceptionMessage("MQTT connection was never lost, token renewal was not tested", client), mqttDisconnectDidHappenSuccesses[clientIndex].result);
                 assertTrue(CorrelationDetailsLoggingAssert.buildExceptionMessage("Connection was lost during test, or shutdown was not graceful", client), shutdownWasGracefulSuccesses[clientIndex].result);
+                assertTrue(CorrelationDetailsLoggingAssert.buildExceptionMessage("MQTT connection was lost, but for an unexpected reason", client), mqttDisconnectHadTokenExpiredReasonSuccesses[clientIndex].result);
             }
             else if (protocol == AMQPS || protocol == AMQPS_WS)
             {
@@ -304,13 +320,15 @@ private static class IotHubConnectionStatusChangeTokenRenewalCallbackVerifier im
         Success amqpDisconnectDidNotHappen;
         Success mqttDisconnectDidHappen;
         Success shutdownWasGraceful;
+        Success mqttDisconnectHadTokenExpiredReason;
 
-        public IotHubConnectionStatusChangeTokenRenewalCallbackVerifier(IotHubClientProtocol protocol, Success amqpDisconnectDidNotHappen, Success mqttDisconnectDidHappen, Success shutdownWasGraceful)
+        public IotHubConnectionStatusChangeTokenRenewalCallbackVerifier(IotHubClientProtocol protocol, Success amqpDisconnectDidNotHappen, Success mqttDisconnectDidHappen, Success shutdownWasGraceful, Success mqttDisconnectHadTokenExpiredReason)
         {
             this.protocol = protocol;
             this.mqttDisconnectDidHappen = mqttDisconnectDidHappen;
             this.amqpDisconnectDidNotHappen = amqpDisconnectDidNotHappen;
             this.shutdownWasGraceful = shutdownWasGraceful;
+            this.mqttDisconnectHadTokenExpiredReason = mqttDisconnectHadTokenExpiredReason;
         }
 
         @Override
@@ -345,6 +363,12 @@ else if (status == IotHubConnectionStatus.DISCONNECTED_RETRYING)
                 {
                     amqpDisconnectDidNotHappen.setResult(false);
                 }
+
+                if (protocol == MQTT || protocol == MQTT_WS)
+                {
+                    boolean reasonIsSasTokenExpired = statusChangeReason == IotHubConnectionStatusChangeReason.EXPIRED_SAS_TOKEN;
+                    mqttDisconnectHadTokenExpiredReason.setResult(reasonIsSasTokenExpired);
+                }
             }
         }
     }