Skip to content

Commit

Permalink
Add property bag to exceptions and populate it for WAM (#4112)
Browse files Browse the repository at this point in the history 
* Add property bag to exceptions and populate it for WAM

* Address PR

* fix

* comments
  • Loading branch information
@bgavrilMS
bgavrilMS committed May 4, 2023
1 parent 90d6daf commit da1371c
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 53 deletions.
25 changes: 25 additions & 0 deletions src/client/Microsoft.Identity.Client/MsalException.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Globalization;
using Microsoft.Identity.Client.Internal;
using Microsoft.Identity.Client.PlatformsCommon.Factories;
Expand All @@ -24,6 +25,23 @@ namespace Microsoft.Identity.Client
/// </remarks>
public class MsalException : Exception
{
/// <summary>
/// An <see cref="AdditionalExceptionData"/> property key, available when using desktop brokers.
/// </summary>
public const string BrokerErrorContext = "BrokerErrorContext";
/// <summary>
/// An <see cref="AdditionalExceptionData"/> property key, available when using desktop brokers.
/// </summary>
public const string BrokerErrorTag = "BrokerErrorTag";
/// <summary>
/// An <see cref="AdditionalExceptionData"/> property key, available when using desktop brokers.
/// </summary>
public const string BrokerErrorStatus = "BrokerErrorStatus";
/// <summary>
/// An <see cref="AdditionalExceptionData"/> property key, available when using desktop brokers.
/// </summary>
public const string BrokerErrorCode = "BrokerErrorCode";

private string _errorCode;

/// <summary>
Expand Down Expand Up @@ -113,6 +131,13 @@ private set
}
}

/// <summary>
/// A property bag with extra details for this exception.
/// </summary>
public IReadOnlyDictionary<string, string> AdditionalExceptionData { get; set; }
= CollectionHelpers.GetEmptyDictionary<string, string>();


/// <summary>
/// Creates and returns a string representation of the current exception.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/client/Microsoft.Identity.Client/Platforms/Features/RuntimeBroker/RuntimeBroker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ private void LogEventRaised(NativeInterop.Core sender, LogEventArgs args)
{
throw new MsalClientException(
"window_handle_required",
"The new WAM implementation now needs application developers to provide parent window handle to the broker. See https://aka.ms/msal-net-wam#parent-window-handles");
"A window handle must be configured. See https://aka.ms/msal-net-wam#parent-window-handles");
}

//if OperatingSystemAccount is passed then we use the user signed-in on the machine
Expand Down
106 changes: 56 additions & 50 deletions src/client/Microsoft.Identity.Client/Platforms/Features/RuntimeBroker/WamAdapters.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.Identity.Client.Internal;
using Microsoft.Identity.Client.Internal.Broker;
using Microsoft.Identity.Client.Internal.Requests;
using Microsoft.Identity.Client.NativeInterop;
using Microsoft.Identity.Client.OAuth2;
using Microsoft.Identity.Client.TelemetryCore.Internal.Events;
using Microsoft.Identity.Client.Utils;
Expand All @@ -23,7 +24,7 @@ namespace Microsoft.Identity.Client.Platforms.Features.RuntimeBroker
internal class WamAdapters
{
private const string WamErrorPrefix = "WAM Error ";

//MSA-PT Auth Params
private const string NativeInteropMsalRequestType = "msal_request_type";
private const string ConsumersPassthroughRequest = "consumer_passthrough";
Expand Down Expand Up @@ -52,16 +53,7 @@ private enum ResponseStatus
UserDataRemovalRequired = 14
};

/// <summary>
/// Create WAM AuthResult Error Response
/// </summary>
/// <param name="authResult"></param>
/// <param name="authenticationRequestParameters"></param>
/// <param name="logger"></param>
/// <exception cref="MsalClientException"></exception>
/// <exception cref="MsalUiRequiredException"></exception>
/// <exception cref="MsalServiceException"></exception>
private static void ThrowExceptionFromWamError(
private static MsalException CreateExceptionFromWamError(
NativeInterop.AuthResult authResult,
AuthenticationRequestParameters authenticationRequestParameters,
ILoggerAdapter logger)
Expand All @@ -72,23 +64,24 @@ private enum ResponseStatus
string errorMessage;

logger.Info("[WamBroker] Processing WAM exception");
logger.Verbose(()=>$"[WamBroker] TelemetryData: {authResult.TelemetryData}");
logger.Verbose(() => $"[WamBroker] TelemetryData: {authResult.TelemetryData}");

switch ((ResponseStatus)authResult.Error.Status)
{
case ResponseStatus.UserCanceled:
logger.Error($"[WamBroker] {MsalError.AuthenticationCanceledError} {MsalErrorMessage.AuthenticationCanceled}");
throw new MsalClientException(MsalError.AuthenticationCanceledError, MsalErrorMessage.AuthenticationCanceled);

var clientEx = new MsalClientException(MsalError.AuthenticationCanceledError, MsalErrorMessage.AuthenticationCanceled);
return clientEx;
case ResponseStatus.InteractionRequired:
case ResponseStatus.AccountUnusable:
errorMessage =
errorMessage =
$"{WamErrorPrefix} \n" +
$" Error Code: {errorCode} \n" +
$" Error Message: {authResult.Error.Context} \n" +
$" Internal Error Code: {internalErrorCode} \n";
logger.Error($"[WamBroker] {MsalError.FailedToAcquireTokenSilentlyFromBroker} {errorMessage}");
throw new MsalUiRequiredException(MsalError.FailedToAcquireTokenSilentlyFromBroker, errorMessage);
var ex = new MsalUiRequiredException(MsalError.FailedToAcquireTokenSilentlyFromBroker, errorMessage);
return ex;

case ResponseStatus.IncorrectConfiguration:
case ResponseStatus.ApiContractViolation:
Expand All @@ -98,16 +91,13 @@ private enum ResponseStatus
$" Error Message: {authResult.Error.Status} \n" +
$" WAM Error Message: {authResult.Error.Context} \n" +
$" Internal Error Code: {internalErrorCode} \n" +
$" Is Retryable: false \n" +
$" Possible causes: \n" +
$"- Invalid redirect uri - ensure you have configured the following url in the AAD portal App Registration: " +
$"{GetExpectedRedirectUri(authenticationRequestParameters.AppConfig.ClientId)} \n" +
$"- No Internet connection : \n" +
$"Please see https://aka.ms/msal-net-wam for details about Windows Broker integration";
$"- Invalid redirect uri - ensure you have configured the following url in the application registration in Azure Portal: " +
$"{GetExpectedRedirectUri(authenticationRequestParameters.AppConfig.ClientId)} \n";
logger.Error($"[WamBroker] WAM_provider_error_{errorCode} {errorMessage}");
serviceException = new MsalServiceException($"WAM_provider_error_{errorCode}", errorMessage);
serviceException.IsRetryable = false;
throw serviceException;
return serviceException;

case ResponseStatus.NetworkTemporarilyUnavailable:
case ResponseStatus.NoNetwork:
Expand All @@ -118,16 +108,18 @@ private enum ResponseStatus
$" Error Message: {authResult.Error.Status} \n" +
$" WAM Error Message: {authResult.Error.Context} \n" +
$" Internal Error Code: {internalErrorCode} \n" +
$" Is Retryable: true";
$" Possible cause: no Internet connection ";

logger.Error($"[WamBroker] WAM_network_error_{errorCode} {errorMessage}");
serviceException = new MsalServiceException(errorCode.ToString(), errorMessage);
serviceException.IsRetryable = true;
throw serviceException;
return serviceException;

default:
errorMessage = $"Unknown {authResult.Error} (error code {errorCode}) (internal error code {internalErrorCode})";
logger.Verbose(()=>$"[WamBroker] {MsalError.UnknownBrokerError} {errorMessage}");
throw new MsalServiceException(MsalError.UnknownBrokerError, errorMessage);
logger.Verbose(() => $"[WamBroker] {MsalError.UnknownBrokerError} {errorMessage}");
var ex2 = new MsalServiceException(MsalError.UnknownBrokerError, errorMessage);
return ex2;
}
}

Expand All @@ -138,7 +130,7 @@ private enum ResponseStatus
/// <param name="brokerOptions"></param>
/// <param name="logger"></param>
public static NativeInterop.AuthParameters GetCommonAuthParameters(
AuthenticationRequestParameters authenticationRequestParameters,
AuthenticationRequestParameters authenticationRequestParameters,
BrokerOptions brokerOptions,
ILoggerAdapter logger)
{
Expand Down Expand Up @@ -201,11 +193,26 @@ private enum ResponseStatus

AddPopParams(authenticationRequestParameters, authParams);

logger.Verbose(()=>"[WamBroker] Acquired Common Auth Parameters.");
logger.Verbose(() => "[WamBroker] Acquired Common Auth Parameters.");

return authParams;
}

private static MsalException DecorateExceptionWithRuntimeErrorProperties(MsalException exception, Error runtimeError)
{
var result = new Dictionary<string, string>()
{
{ MsalException.BrokerErrorContext, runtimeError.Context },
{ MsalException.BrokerErrorTag, $"0x{runtimeError.Tag:X}" },
{ MsalException.BrokerErrorStatus, runtimeError.Status.ToString() },
{ MsalException.BrokerErrorCode, (runtimeError.ErrorCode).ToString() },
};

exception.AdditionalExceptionData = result;

return exception;
}

/// <summary>
/// Configures the MSAL Runtime authentication request to use proof of possession .
/// </summary>
Expand All @@ -223,7 +230,7 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
}

private static void SetMSALIdentityProvider(
AuthenticationRequestParameters authenticationRequestParameters,
AuthenticationRequestParameters authenticationRequestParameters,
NativeInterop.AuthParameters authParams,
ILoggerAdapter logger)
{
Expand Down Expand Up @@ -262,17 +269,16 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
if (TokenReceivedFromWam(authResult, logger))
{
msalTokenResponse = ParseRuntimeResponse(authResult, authenticationRequestParameters, logger);
logger.Verbose(()=>"[WamBroker] Successfully retrieved token.");
}
else
{
logger.Error($"[WamBroker] {errorMessage} {authResult.Error}");
ThrowExceptionFromWamError(authResult, authenticationRequestParameters, logger);
logger.Verbose(() => "[WamBroker] Successfully retrieved token.");
return msalTokenResponse;
}

return msalTokenResponse;
logger.Info($"[WamBroker] {errorMessage} {authResult.Error}");
MsalException ex = CreateExceptionFromWamError(authResult, authenticationRequestParameters, logger);
ex = DecorateExceptionWithRuntimeErrorProperties(ex, authResult.Error);
throw ex;
}

/// <summary>
/// Token Received from WAM
/// </summary>
Expand All @@ -285,10 +291,10 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
//success result from WAM
if (authResult.IsSuccess)
return true;

//user switch result is not success and a token is received
//from MSALRuntime with an Error Response status
if (authResult.Error != null
if (authResult.Error != null
&& (ResponseStatus)authResult.Error.Status == ResponseStatus.UserSwitch)
{
logger.Info("[WamBroker] WAM response status account switch. Treating as success");
Expand All @@ -307,7 +313,7 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
/// <param name="logger"></param>
/// <exception cref="MsalServiceException"></exception>
private static MsalTokenResponse ParseRuntimeResponse(
NativeInterop.AuthResult authResult,
NativeInterop.AuthResult authResult,
AuthenticationRequestParameters authenticationRequestParameters,
ILoggerAdapter logger)
{
Expand All @@ -320,7 +326,7 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
logger.Warning("[WamBroker] No correlation ID in response");
correlationId = null;
}

//parsing Pop token from auth header if pop was performed. Otherwise use access token field.
var token = authResult.IsPopAuthorization ? authResult.AuthorizationHeader.Split(' ')[1] : authResult.AccessToken;

Expand All @@ -337,7 +343,7 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR
}

MsalTokenResponse msalTokenResponse = new MsalTokenResponse()
{
{
AuthorityUrl = authorityUrl,
AccessToken = token,
IdToken = authResult.RawIdToken,
Expand All @@ -354,7 +360,7 @@ private static void AddPopParams(AuthenticationRequestParameters authenticationR

return msalTokenResponse;
}
catch (NativeInterop.MsalRuntimeException ex)
catch (MsalRuntimeException ex)
{
logger.ErrorPii($"[WamBroker] Could not acquire token using WAM. {ex.Message}", string.Empty);
throw new MsalServiceException("wam_failed", $"Could not acquire token using WAM. {ex.Message}");
Expand Down Expand Up @@ -382,8 +388,8 @@ private static string GetExpectedRedirectUri(string clientId)
/// <exception cref="MsalServiceException"></exception>
public static bool TryConvertToMsalAccount(
NativeInterop.Account nativeAccount,
string clientId,
ILoggerAdapter logger,
string clientId,
ILoggerAdapter logger,
out IAccount msalAccount)
{
//native interop account will never be null, but good to check
Expand Down Expand Up @@ -411,10 +417,10 @@ private static string GetExpectedRedirectUri(string clientId)
nativeAccount.UserName,
nativeAccount.Environment,
new Dictionary<string, string>() {
{
clientId,
nativeAccount.AccountId
}
{
clientId,
nativeAccount.AccountId
}
});

return true;
Expand Down Expand Up @@ -454,7 +460,7 @@ private static string GetExpectedRedirectUri(string clientId)

logger.InfoPii(messageWithPii, builder.ToString());
}

logger.Error($"[WamBroker] WAM Account properties are missing. Cannot convert to MSAL Accounts.");
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/Microsoft.Identity.Test.Integration.netfx/HeadlessTests/PoPTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
using Microsoft.IdentityModel.Protocols.SignedHttpRequest;
using Microsoft.IdentityModel.Tokens;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.Identity.Test.Common.Core.Helpers;

namespace Microsoft.Identity.Test.Integration.HeadlessTests
{
Expand Down Expand Up @@ -452,7 +453,6 @@ public async Task NewPOP_WithKeyIdOnly_Async()
}

#if NET_CORE
[TestMethod]
public async Task WamUsernamePasswordRequestWithPOPAsync()
{
var labResponse = await LabUserHelper.GetDefaultUserAsync().ConfigureAwait(false);
Expand Down
28 changes: 28 additions & 0 deletions tests/Microsoft.Identity.Test.Integration.netfx/HeadlessTests/RuntimeBrokerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,34 @@ public async Task WamSilentAuthUserInteractionRequiredAsync()
}
}

[RunOn(TargetFrameworks.NetCore)]
public async Task WamInvalidROPC_ThrowsException_TestAsync()
{
var labResponse = await LabUserHelper.GetDefaultUserAsync().ConfigureAwait(false);
string[] scopes = { "User.Read" };
WamLoggerValidator wastestLogger = new WamLoggerValidator();

IPublicClientApplication pca = PublicClientApplicationBuilder
.Create(labResponse.App.AppId)
.WithAuthority(labResponse.Lab.Authority, "organizations")
.WithLogging(wastestLogger, enablePiiLogging: true) // it's important that the PII is turned on, otherwise context is 'pii'
.WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows))
.Build();

MsalServiceException ex = await AssertException.TaskThrowsAsync<MsalServiceException>(() =>
pca.AcquireTokenByUsernamePassword(
scopes,
"noUser",
"badPassword")
.ExecuteAsync())
.ConfigureAwait(false);

Assert.AreEqual("0x2142008A", ex.AdditionalExceptionData[MsalException.BrokerErrorTag]);
Assert.AreEqual("User name is malformed.", ex.AdditionalExceptionData[MsalException.BrokerErrorContext]); // message might change. not a big deal
Assert.AreEqual("ApiContractViolation", ex.AdditionalExceptionData[MsalException.BrokerErrorStatus]);
Assert.AreEqual("3399811229", ex.AdditionalExceptionData[MsalException.BrokerErrorCode]);
}

[RunOn(TargetFrameworks.NetCore)]
public async Task WamSilentAuthLoginHintNoAccontInCacheAsync()
{
Expand Down
2 changes: 1 addition & 1 deletion tests/Microsoft.Identity.Test.Unit/BrokerTests/BrokerRequestTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ public void BrokerInteractiveRequestTest()
null,
broker,
"install_url");
#if NET6_WIN || NET7_0
#if NET6_WIN
Assert.AreEqual(true, _brokerInteractiveRequest.Broker.IsBrokerInstalledAndInvokable(AuthorityType.Aad));
#else
Assert.AreEqual(false, _brokerInteractiveRequest.Broker.IsBrokerInstalledAndInvokable(AuthorityType.Aad));
Expand Down

0 comments on commit da1371c

Please sign in to comment.