Skip to content
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.

Sign up for GitHub

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

Jump to bottom

process android broker error codes which should result in UI required… #2200

Merged
merged 3 commits into from
Nov 18, 2020
Merged

process android broker error codes which should result in UI required… #2200

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bc2456
process android broker error codes which should result in UI required…
jennyf19 Nov 18, 2020
1d7a573
prefix code w/"android"
jennyf19 Nov 18, 2020
40617e2
Merge branch 'master' into jennyf/UiExcept
jennyf19 Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/client/Microsoft.Identity.Client/Internal/Broker/BrokerResponseConst.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ internal static class BrokerResponseConst

public const string TokenType = "token_type";

//Error codes returned from broker
public const string NoTokenFound = "no_tokens_found";
//Error codes returned from Android broker
public const string AndroidNoTokenFound = "no_tokens_found";
public const string AndroidNoAccountFound = "no_account_found";
public const string AndroidInvalidRefreshToken = "Broker refresh token is invalid";
}
}
1 change: 1 addition & 0 deletions src/client/Microsoft.Identity.Client/Internal/Requests/Interactive/InteractiveRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ private async Task<MsalTokenResponse> FetchTokensFromBrokerAsync(string brokerIn
return await brokerInteractiveRequest.FetchTokensAsync(cancellationToken)
.ConfigureAwait(false);
}

private async Task<MsalTokenResponse> GetTokenResponseAsync(CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
Expand Down
6 changes: 4 additions & 2 deletions src/client/Microsoft.Identity.Client/Internal/Requests/Silent/BrokerSilentStrategy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private async Task<MsalTokenResponse> SendTokenRequestToBrokerAsync()
_logger.Info(LogMessages.CheckMsalTokenResponseReturnedFromBroker);
if (msalTokenResponse.AccessToken != null)
{
_logger.Info("Success. Response contains an access token");
_logger.Info("Success. Response contains an access token. ");
return;
}

Expand All @@ -79,7 +79,9 @@ private async Task<MsalTokenResponse> SendTokenRequestToBrokerAsync()
_logger.Info(
LogMessages.ErrorReturnedInBrokerResponse(msalTokenResponse.Error));

if (msalTokenResponse.Error == BrokerResponseConst.NoTokenFound)
if (msalTokenResponse.Error == BrokerResponseConst.AndroidNoTokenFound ||
msalTokenResponse.Error == BrokerResponseConst.AndroidNoAccountFound ||
msalTokenResponse.Error == BrokerResponseConst.AndroidInvalidRefreshToken)
{
throw new MsalUiRequiredException(msalTokenResponse.Error, msalTokenResponse.ErrorDescription);
}
Expand Down
46 changes: 24 additions & 22 deletions src/client/Microsoft.Identity.Client/Platforms/Android/Broker/AndroidBroker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@ public bool IsBrokerInstalledAndInvokable()
}
catch (Exception ex)
{
_logger.ErrorPiiWithPrefix(ex, "Broker interactive invocation failed.");
_logger.ErrorPiiWithPrefix(ex, "Android broker interactive invocation failed. ");
HandleBrokerOperationError(ex);
}

using (_logger.LogBlockDuration("waiting for broker response"))
using (_logger.LogBlockDuration("Waiting for Android broker response. "))
{
await s_readyForResponse.WaitAsync().ConfigureAwait(false);
return s_androidBrokerTokenResponse;
Expand All @@ -103,7 +103,7 @@ public bool IsBrokerInstalledAndInvokable()
}
catch (Exception ex)
{
_logger.ErrorPiiWithPrefix(ex, "Broker silent invocation failed.");
_logger.ErrorPiiWithPrefix(ex, "Android broker silent invocation failed. ");
HandleBrokerOperationError(ex);
throw;
}
Expand All @@ -114,9 +114,9 @@ private async Task AcquireTokenInteractiveViaBrokerAsync(BrokerRequest brokerReq
using (_logger.LogMethodDuration())
{
// onActivityResult will receive the response for this activity.
// Lauching this activity will switch to the broker app.
// Launching this activity will switch to the broker app.

_logger.Verbose("Starting Android Broker interactive authentication");
_logger.Verbose("Starting Android Broker interactive authentication. ");
Intent brokerIntent = await _brokerHelper
.GetIntentForInteractiveBrokerRequestAsync(brokerRequest, _parentActivity)
.ConfigureAwait(false);
Expand All @@ -134,7 +134,7 @@ private async Task AcquireTokenInteractiveViaBrokerAsync(BrokerRequest brokerReq
}
catch (ActivityNotFoundException e)
{
_logger.ErrorPiiWithPrefix(e, "Unable to get android activity during interactive broker request");
_logger.ErrorPiiWithPrefix(e, "Unable to get Android activity during interactive broker request. ");
throw;
}
}
Expand All @@ -147,7 +147,7 @@ private async Task<MsalTokenResponse> AcquireTokenSilentViaBrokerAsync(BrokerReq

using (_logger.LogMethodDuration())
{
_logger.Verbose("User is specified for silent token request. Starting silent broker request.");
_logger.Verbose("User is specified for silent token request. Starting silent Android broker request. ");
string silentResult = await _brokerHelper.GetBrokerAuthTokenSilentlyAsync(brokerRequest, _parentActivity).ConfigureAwait(false);
if (!string.IsNullOrEmpty(silentResult))
{
Expand All @@ -157,7 +157,7 @@ private async Task<MsalTokenResponse> AcquireTokenSilentViaBrokerAsync(BrokerReq
return new MsalTokenResponse
{
Error = MsalError.BrokerResponseReturnedError,
ErrorDescription = "Unknown broker error. Failed to acquire token silently from the broker. " + MsalErrorMessage.AndroidBrokerCannotBeInvoked,
ErrorDescription = "Unknown Android broker error. Failed to acquire token silently from the broker. " + MsalErrorMessage.AndroidBrokerCannotBeInvoked,
};
}
}
Expand All @@ -178,22 +178,22 @@ internal static void SetBrokerResult(Intent data, int resultCode, ICoreLogger un
{
if (data == null)
{
unreliableLogger?.Info("Data is null, stopping.");
unreliableLogger?.Info("Data is null, stopping. ");
return;
}

switch (resultCode)
{
case (int)BrokerResponseCode.ResponseReceived:
unreliableLogger?.Info("Response received, decoding...");
unreliableLogger?.Info("Response received, decoding... ");

s_androidBrokerTokenResponse =
MsalTokenResponse.CreateFromAndroidBrokerResponse(
data.GetStringExtra(BrokerConstants.BrokerResultV2),
s_correlationId);
break;
case (int)BrokerResponseCode.UserCancelled:
unreliableLogger?.Info("Response received - user cancelled");
unreliableLogger?.Info("Response received - user cancelled. ");

s_androidBrokerTokenResponse = new MsalTokenResponse
{
Expand All @@ -202,7 +202,7 @@ internal static void SetBrokerResult(Intent data, int resultCode, ICoreLogger un
};
break;
case (int)BrokerResponseCode.BrowserCodeError:
unreliableLogger?.Info("Response received - error ");
unreliableLogger?.Info("Response received - error. ");

dynamic errorResult = JObject.Parse(data.GetStringExtra(BrokerConstants.BrokerResultV2));
string error = null;
Expand All @@ -213,13 +213,13 @@ internal static void SetBrokerResult(Intent data, int resultCode, ICoreLogger un
error = errorResult[BrokerResponseConst.BrokerErrorCode]?.ToString();
errorDescription = errorResult[BrokerResponseConst.BrokerErrorMessage]?.ToString();

unreliableLogger?.Error($"error: {error} errorDescription {errorDescription}");
unreliableLogger?.Error($"error: {error} errorDescription {errorDescription}. ");
}
else
{
error = BrokerConstants.BrokerUnknownErrorCode;
errorDescription = "Error Code received, but no error could be extracted";
unreliableLogger?.Error("Error response received, but not error could be extracted");
errorDescription = "Error Code received, but no error could be extracted. ";
unreliableLogger?.Error("Error response received, but not error could be extracted. ");
}

var httpResponse = new HttpResponse();
Expand All @@ -236,11 +236,11 @@ internal static void SetBrokerResult(Intent data, int resultCode, ICoreLogger un
};
break;
default:
unreliableLogger?.Error("Unkwown broker response");
unreliableLogger?.Error("Unknown broker response. ");
s_androidBrokerTokenResponse = new MsalTokenResponse
{
Error = BrokerConstants.BrokerUnknownErrorCode,
ErrorDescription = "Broker result not returned from android broker.",
ErrorDescription = "Broker result not returned from android broker. ",
CorrelationId = s_correlationId
};
break;
Expand All @@ -258,7 +258,7 @@ public async Task<IEnumerable<IAccount>> GetAccountsAsync(string clientID, strin
{
if (!IsBrokerInstalledAndInvokable())
{
_logger.Warning("Android broker is either not installed or is not reachable so no accounts will be returned.");
_logger.Warning("Android broker is either not installed or is not reachable so no accounts will be returned. ");
return new List<IAccount>();
}

Expand All @@ -272,7 +272,7 @@ public async Task<IEnumerable<IAccount>> GetAccountsAsync(string clientID, strin
}
catch (Exception ex)
{
_logger.Error("Failed to get Android broker accounts from the broker.");
_logger.Error("Failed to get Android broker accounts from the broker. ");
HandleBrokerOperationError(ex);
throw;
}
Expand All @@ -285,7 +285,7 @@ public async Task RemoveAccountAsync(IApplicationConfiguration applicationConfig
{
if (!IsBrokerInstalledAndInvokable())
{
_logger.Warning("Android broker is either not installed or not reachable so no accounts will be removed.");
_logger.Warning("Android broker is either not installed or not reachable so no accounts will be removed. ");
return;
}

Expand All @@ -296,7 +296,7 @@ public async Task RemoveAccountAsync(IApplicationConfiguration applicationConfig
}
catch (Exception ex)
{
_logger.Error("Failed to remove Android broker account from the broker.");
_logger.Error("Failed to remove Android broker account from the broker. ");
HandleBrokerOperationError(ex);
throw;
}
Expand All @@ -315,7 +315,9 @@ private void HandleBrokerOperationError(Exception ex)
/// <summary>
/// Android Broker does not support logging in a "default" user.
/// </summary>
public Task<MsalTokenResponse> AcquireTokenSilentDefaultUserAsync(AuthenticationRequestParameters authenticationRequestParameters, AcquireTokenSilentParameters acquireTokenSilentParameters)
public Task<MsalTokenResponse> AcquireTokenSilentDefaultUserAsync(
AuthenticationRequestParameters authenticationRequestParameters,
AcquireTokenSilentParameters acquireTokenSilentParameters)
{
throw new MsalUiRequiredException(
MsalError.UserNullError,
Expand Down
2 changes: 1 addition & 1 deletion tests/Microsoft.Identity.Test.Common/TestConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public static string Defaultx5cValue
}
}

public const string Bearer = "bearer";
public const string Bearer = "Bearer";

public static IDictionary<string, string> ExtraQueryParameters
{
Expand Down
58 changes: 49 additions & 9 deletions tests/Microsoft.Identity.Test.Unit/BrokerTests/BrokerRequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,28 +378,68 @@ public void NoTokenFoundThrowsUIRequiredTest()
{
using (var harness = CreateBrokerHelper())
{
try
{
_brokerSilentAuthStrategy.ValidateResponseFromBroker(CreateErrorResponse(BrokerResponseConst.AndroidNoTokenFound));
}
catch (MsalUiRequiredException ex)
{
Assert.IsTrue(ex.ErrorCode == BrokerResponseConst.AndroidNoTokenFound);
return;
}

var response = new MsalTokenResponse
Assert.Fail("Wrong Exception thrown. ");
}
}

[TestMethod]
public void NoAccountFoundThrowsUIRequiredTest()
Copy link
Member

@bgavrilMS bgavrilMS Nov 18, 2020 •
edited by jennyf19
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the best place to add this the logic you've added is actually in AndroidBroker.cs; However we can't test that. Having tests beats a marginally better architecture. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the unit testing, especially since not sure how to manually test w/out messing up my authenticator app.

In reply to: 526108961 [](ancestors = 526108961)

{
using (var harness = CreateBrokerHelper())
{
try
{
Scope = TestConstants.s_scope.AsSingleString(),
TokenType = "Bearer",
Error = BrokerResponseConst.NoTokenFound
};
_brokerSilentAuthStrategy.ValidateResponseFromBroker(CreateErrorResponse(BrokerResponseConst.AndroidNoAccountFound));
}
catch (MsalUiRequiredException ex)
{
Assert.IsTrue(ex.ErrorCode == BrokerResponseConst.AndroidNoAccountFound);
return;
}

Assert.Fail("Wrong Exception thrown. ");
}
}

[TestMethod]
public void InvalidRefreshTokenUsedThrowsUIRequiredTest()
{
using (var harness = CreateBrokerHelper())
{
try
{
_brokerSilentAuthStrategy.ValidateResponseFromBroker(response);
_brokerSilentAuthStrategy.ValidateResponseFromBroker(CreateErrorResponse(BrokerResponseConst.AndroidInvalidRefreshToken));
}
catch(MsalUiRequiredException ex)
catch (MsalUiRequiredException ex)
{
Assert.IsTrue(ex.ErrorCode == BrokerResponseConst.NoTokenFound);
Assert.IsTrue(ex.ErrorCode == BrokerResponseConst.AndroidInvalidRefreshToken);
return;
}

Assert.Fail("Wrong Exception thrown.");
Assert.Fail("Wrong Exception thrown. ");
}
}

private static MsalTokenResponse CreateErrorResponse(string errorCode)
{
return new MsalTokenResponse
{
Scope = TestConstants.s_scope.AsSingleString(),
TokenType = TestConstants.Bearer,
Error = errorCode
};
}

private void ValidateBrokerResponse(MsalTokenResponse msalTokenResponse, Action<Exception> validationHandler)
{
try
Expand Down