Fix for #1067 (#1071)

* Add suberror support (internal only)

* Bump MSAL version of development envs to 3

* Fix for #1067 - FRT fail silently only on client_mismatch

src/Directory.Build.props

@@ -22,7 +22,7 @@
 
   <PropertyGroup Label="NuGet and AssemblyInfo metadata">
     <!--This should be passed from the VSTS build-->
-    <MsalClientSemVer Condition="'$(MsalClientSemVer)' == ''">1.0.0-localbuild</MsalClientSemVer>
+    <MsalClientSemVer Condition="'$(MsalClientSemVer)' == ''">3.0.4-localbuild</MsalClientSemVer>
     <!--This will generate AssemblyVersion, AssemblyFileVersion and AssemblyInformationVersion-->
     <Version>$(MsalClientSemVer)</Version>
 

src/Microsoft.Identity.Client/Internal/Requests/SilentRequest.cs

@@ -42,6 +42,7 @@ internal class SilentRequest : RequestBase
     {
         private readonly AcquireTokenSilentParameters _silentParameters;
         private const string TheOnlyFamilyId = "1";
+        private const string FociClientMismatchSubError = "client_mismatch";
 
         public SilentRequest(
             IServiceBundle serviceBundle,
@@ -173,13 +174,29 @@ private async Task<MsalTokenResponse> TryGetTokenUsingFociAsync(CancellationToke
                     MsalTokenResponse frtTokenResponse = await RefreshAccessTokenAsync(familyRefreshToken, cancellationToken)
                         .ConfigureAwait(false);
 
-                    logger.Verbose("[FOCI] FRT exchanged succeeded");
+                    logger.Verbose("[FOCI] FRT refresh succeeded");
                     return frtTokenResponse;
                 }
-                catch (MsalServiceException)
+                catch (MsalServiceException ex)
                 {
-                    logger.Error("[FOCI] FRT exchanged failed " + (familyRefreshToken != null));
+
+                    // Hack: STS does not yet send back the suberror on these platforms because they are not in an allowed list,
+                    // so the best thing we can do is to consider all errors as client_mismatch.
+#if NETSTANDARD || UAP || MAC
                     return null;
+#endif
+                    if (MsalError.InvalidGrantError.Equals(ex?.ErrorCode, StringComparison.OrdinalIgnoreCase) &&
+                        FociClientMismatchSubError.Equals(ex?.SubError, StringComparison.OrdinalIgnoreCase))
+                    {
+                        logger.Error("[FOCI] FRT refresh failed - client mismatch");
+                        return null;
+                    }
+
+                    // Rethrow failures to refresh the FRT, other than client_mismatch, because 
+                    // apps need to handle them in the same way they handle exceptions from refreshing the RT.
+                    // For example, some apps have special handling for MFA errors.
+                    logger.Error("[FOCI] FRT refresh failed - other error");
+                    throw;
                 }
             }
 

src/Microsoft.Identity.Client/MsalServiceException.cs

@@ -174,12 +174,13 @@ internal OAuth2ResponseBase OAuth2Response
                 _oauth2ResponseBase = value;
                 Claims = _oauth2ResponseBase?.Claims;
                 CorrelationId = _oauth2ResponseBase?.CorrelationId;
+                SubError = _oauth2ResponseBase?.SubError;
             }
         }
 
         /// <summary>
         /// Gets the status code returned from http layer. This status code is either the <c>HttpStatusCode</c> in the inner
-        /// <see cref="T:System.Net.Http.HttpRequestException"/> response or the the NavigateError Event Status Code in a browser based flow (See
+        /// <see cref="System.Net.Http.HttpRequestException"/> response or the the NavigateError Event Status Code in a browser based flow (See
         /// http://msdn.microsoft.com/en-us/library/bb268233(v=vs.85).aspx).
         /// You can use this code for purposes such as implementing retry logic or error investigation.
         /// </summary>
@@ -209,6 +210,12 @@ internal OAuth2ResponseBase OAuth2Response
         /// </summary>
         public string ResponseBody { get; internal set; }
 
+        /// <remarks>
+        /// The suberror should not be exposed for public consumption yet, as STS needs to do some work
+        /// first.
+        /// </remarks>
+        internal string SubError { get; set; }
+
         /// <summary>
         /// Contains the http headers from the server response that indicated an error. 
         /// </summary>
@@ -240,6 +247,7 @@ public override string ToString()
         private const string ClaimsKey = "claims";
         private const string ResponseBodyKey = "response_body";
         private const string CorrelationIdKey = "correlation_id";
+        private const string SubErrorKey = "sub_error";
 
         internal override void PopulateJson(JObject jobj)
         {
@@ -248,6 +256,7 @@ internal override void PopulateJson(JObject jobj)
             jobj[ClaimsKey] = Claims;
             jobj[ResponseBodyKey] = ResponseBody;
             jobj[CorrelationIdKey] = CorrelationId;
+            jobj[SubErrorKey] = SubError;
         }
 
         internal override void PopulateObjectFromJson(JObject jobj)
@@ -257,6 +266,7 @@ internal override void PopulateObjectFromJson(JObject jobj)
             Claims = JsonUtils.GetExistingOrEmptyString(jobj, ClaimsKey);
             ResponseBody = JsonUtils.GetExistingOrEmptyString(jobj, ResponseBodyKey);
             CorrelationId = JsonUtils.GetExistingOrEmptyString(jobj, CorrelationIdKey);
+            SubError = JsonUtils.GetExistingOrEmptyString(jobj, SubErrorKey);
         }
     }
 }

tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHelpers.cs

@@ -140,15 +140,17 @@ public static HttpResponseMessage CreateSuccessTokenResponseMessage(bool foci =
                 foci ? FociTokenResponse : DefaultTokenResponse);
         }
 
-        public static HttpResponseMessage CreateInvalidGrantTokenResponseMessage()
+        public static HttpResponseMessage CreateInvalidGrantTokenResponseMessage(string subError = null)
         {
             return CreateFailureMessage(HttpStatusCode.BadRequest,
                 "{\"error\":\"invalid_grant\",\"error_description\":\"AADSTS70002: Error " +
                 "validating credentials.AADSTS70008: The provided access grant is expired " +
                 "or revoked.Trace ID: f7ec686c-9196-4220-a754-cd9197de44e9Correlation ID: " +
                 "04bb0cae-580b-49ac-9a10-b6c3316b1eaaTimestamp: 2015-09-16 07:24:55Z\"," +
                 "\"error_codes\":[70002,70008],\"timestamp\":\"2015-09-16 07:24:55Z\"," +
-                "\"trace_id\":\"f7ec686c-9196-4220-a754-cd9197de44e9\",\"correlation_id\":" +
+                "\"trace_id\":\"f7ec686c-9196-4220-a754-cd9197de44e9\"," +
+                (subError != null ? ("\"suberror\":" + "\"" + subError + "\",") : "") +
+                "\"correlation_id\":" +
                 "\"04bb0cae-580b-49ac-9a10-b6c3316b1eaa\"}");
         }
 

tests/Microsoft.Identity.Test.Unit.net45/ExceptionTests/MsalExceptionFactoryTests.cs

@@ -45,7 +45,7 @@ public class MsalExceptionFactoryTests
         private const string ExCode = "exCode";
         private const string ExMessage = "exMessage";
 
-        private const string jsonError = @"{ ""error"":""invalid_tenant"", ""suberror"":""suberror"",
+        private const string jsonError = @"{ ""error"":""invalid_tenant"", ""suberror"":""some_suberror"",
             ""claims"":""some_claims"",
             ""error_description"":""AADSTS90002: Tenant 'x' not found. "", ""error_codes"":[90002],""timestamp"":""2019-01-28 14:16:04Z"",
             ""trace_id"":""43f14373-8d7d-466e-a5f1-6e3889291e00"",
@@ -70,7 +70,7 @@ public void ParamValidation()
         }
 
         [TestMethod]
-        public void MsalClientException_FromCoreException()
+        public void MsalClientException_FromMessageAndCode()
         {
             // Act
             var msalException = new MsalClientException(ExCode, ExMessage);
@@ -118,6 +118,7 @@ public void MsalServiceException_Oauth2Response_Only()
             Assert.AreEqual(ExMessage, msalServiceException.Message);
             Assert.AreEqual("some_claims", msalServiceException.Claims);
             Assert.AreEqual("6347d33d-941a-4c35-9912-a9cf54fb1b3e", msalServiceException.CorrelationId);
+            Assert.AreEqual("some_suberror", msalServiceException.SubError);
         }
 
         [TestMethod]
@@ -150,6 +151,7 @@ public void MsalServiceException_HttpResponse_OAuthResponse()
 
             Assert.AreEqual("some_claims", msalServiceException.Claims);
             Assert.AreEqual("6347d33d-941a-4c35-9912-a9cf54fb1b3e", msalServiceException.CorrelationId);
+            Assert.AreEqual("some_suberror", msalServiceException.SubError);
 
             // Act
             string piiMessage = MsalLogger.GetPiiScrubbedExceptionDetails(msalException);
@@ -236,6 +238,7 @@ public void MsalServiceException_FromHttpResponse()
             Assert.AreEqual(responseBody, msalServiceException.ResponseBody);
             Assert.AreEqual(ExMessage, msalServiceException.Message);
             Assert.AreEqual((int)statusCode, msalServiceException.StatusCode);
+            Assert.AreEqual("some_suberror", msalServiceException.SubError);
 
             Assert.AreEqual(retryAfterSpan, msalServiceException.Headers.RetryAfter.Delta);
         }

tests/Microsoft.Identity.Test.Unit.net45/ExceptionTests/MsalExceptionSerializationTests.cs

@@ -40,6 +40,8 @@ public class MsalExceptionSerializationTests
         private const string SomeClaims = "here are some claims";
         private const string SomeCorrelationId = "the correlation id";
         private const string SomeResponseBody = "the response body";
+        private const string SomeSubError = "the_sub_error";
+
 
         private void SerializeDeserializeAndValidate(MsalException ex, Type expectedType, bool isServiceExceptionDerived)
         {
@@ -58,6 +60,7 @@ private void SerializeDeserializeAndValidate(MsalException ex, Type expectedType
                 Assert.AreEqual(SomeClaims, svcEx.Claims);
                 Assert.AreEqual(SomeResponseBody, svcEx.ResponseBody);
                 Assert.AreEqual(SomeCorrelationId, svcEx.CorrelationId);
+                Assert.AreEqual(SomeSubError, svcEx.SubError);
             }
         }
 
@@ -75,7 +78,8 @@ public void MsalServiceExceptionCanSerializeAndDeserializeRoundTrip()
             {
                 Claims = SomeClaims,
                 CorrelationId = SomeCorrelationId,
-                ResponseBody = SomeResponseBody
+                ResponseBody = SomeResponseBody,
+                SubError = SomeSubError
             };
 
             SerializeDeserializeAndValidate(ex, typeof(MsalServiceException), true);
@@ -95,7 +99,8 @@ public void MsalUiRequiredExceptionCanSerializeAndDeserializeRoundTrip()
             {
                 Claims = SomeClaims,
                 CorrelationId = SomeCorrelationId,
-                ResponseBody = SomeResponseBody
+                ResponseBody = SomeResponseBody,
+                SubError = SomeSubError
             };
 
             SerializeDeserializeAndValidate(ex, typeof(MsalUiRequiredException), true);

tests/Microsoft.Identity.Test.Unit.net45/RequestsTests/FociTests.cs

@@ -49,7 +49,8 @@ private enum ServerTokenResponse
         {
             NonFociToken,
             FociToken,
-            Error
+            ErrorClientMismatch,
+            OtherError
         }
 
         private string _inMemoryCache;
@@ -105,7 +106,8 @@ public async Task FociAndNonFociAppsCoexistAsync()
                 await InteractiveAsync(_appA, ServerTokenResponse.FociToken).ConfigureAwait(false);
 
                 // B cannot acquire a token interactivelty, but will try to use FRT
-                AssertException.TaskThrows<MsalUiRequiredException>(() => SilentAsync(_appB, ServerTokenResponse.Error));
+                var ex = AssertException.TaskThrows<MsalUiRequiredException>(() => SilentAsync(_appB, ServerTokenResponse.ErrorClientMismatch));
+                Assert.AreEqual(MsalError.NoTokensFoundError, ex.ErrorCode);
 
                 // B can resume acquiring tokens silently via the normal RT, after a non
                 await InteractiveAsync(_appB, ServerTokenResponse.NonFociToken).ConfigureAwait(false);
@@ -115,6 +117,37 @@ public async Task FociAndNonFociAppsCoexistAsync()
                 await AssertAccountsAsync().ConfigureAwait(false);
                 AssertAppHasRT(_appB);
                 AssertFRTExists();
+            }
+        }
+
+        /// <summary>
+        /// A and B are in the family. When B tries to refresh the FRT, an error occurs (e.g. MFA required).
+        /// This error has to be surfaced. This is a different scenario than receiving "client_mismatch" error
+        /// </summary>
+        [TestMethod]
+        [WorkItem(1067)] // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/1067
+        public async Task FociDoesNotHideRTRefreshErrorsAsync()
+        {
+            using (_harness = new MockHttpAndServiceBundle())
+            {
+                InitApps();
+
+                // Act
+                await InteractiveAsync(_appA, ServerTokenResponse.FociToken).ConfigureAwait(false);
+
+                // B cannot acquire a token interactivelty, but will try to use FRT
+                var ex = AssertException.TaskThrows<MsalUiRequiredException>(() => SilentAsync(_appB, ServerTokenResponse.OtherError));
+                Assert.AreEqual(MsalError.InvalidGrantError, ex.ErrorCode);
+                Assert.IsTrue(!String.IsNullOrEmpty(ex.CorrelationId));
+
+                // B performs interactive auth and everything goes back to normal - both A and B can silently sing in
+                await InteractiveAsync(_appB, ServerTokenResponse.FociToken).ConfigureAwait(false);
+                await SilentAsync(_appB, ServerTokenResponse.FociToken).ConfigureAwait(false);
+                await SilentAsync(_appA, ServerTokenResponse.FociToken).ConfigureAwait(false);
+
+                // Assert
+                await AssertAccountsAsync().ConfigureAwait(false);
+                AssertFRTExists();
 
             }
         }
@@ -165,7 +198,7 @@ public async Task FociAppLeavesFamilyAsync()
                 await SilentAsync(_appB, ServerTokenResponse.FociToken).ConfigureAwait(false);
 
                 // B leaves the family -> STS will not refresh its token based on the FRT
-                AssertException.TaskThrows<MsalUiRequiredException>(() => SilentAsync(_appB, ServerTokenResponse.Error));
+                AssertException.TaskThrows<MsalUiRequiredException>(() => SilentAsync(_appB, ServerTokenResponse.ErrorClientMismatch));
 
                 // B can resume acquiring tokens silently via the normal RT, after an interactive flow
                 await InteractiveAsync(_appB, ServerTokenResponse.NonFociToken).ConfigureAwait(false);
@@ -208,8 +241,8 @@ private async Task SilentAsync(
                 {
                     ExpectedMethod = HttpMethod.Post,
                     ResponseMessage =
-                    (serverTokenResponse == ServerTokenResponse.Error) ?
-                        MockHelpers.CreateInvalidGrantTokenResponseMessage() :
+                    IsError(serverTokenResponse) ?
+                        MockHelpers.CreateInvalidGrantTokenResponseMessage(GetSubError(serverTokenResponse)) :
                         MockHelpers.CreateSuccessTokenResponseMessage(
                             MsalTestConstants.UniqueId,
                             MsalTestConstants.DisplayableId,
@@ -227,9 +260,30 @@ private async Task SilentAsync(
 
         }
 
+        private static string GetSubError(ServerTokenResponse response)
+        {
+            if (response == ServerTokenResponse.ErrorClientMismatch)
+            {
+                return "client_mismatch";
+            }
+
+            if (response == ServerTokenResponse.OtherError)
+            {
+                return null;
+            }
+
+            throw new InvalidOperationException("Test error: response type is not an error");
+        }
+
+        private static bool IsError(ServerTokenResponse response)
+        {
+            return response == ServerTokenResponse.ErrorClientMismatch ||
+                response == ServerTokenResponse.OtherError;
+        }
+
         private async Task InteractiveAsync(PublicClientApplication app, ServerTokenResponse serverTokenResponse)
         {
-            if (serverTokenResponse == ServerTokenResponse.Error)
+            if (serverTokenResponse == ServerTokenResponse.ErrorClientMismatch)
             {
                 throw new NotImplementedException("test error");
             }