[Feature Request] Offer specific Exceptions instead of generic MsalServiceException

[BenjaminAbt]

Is your feature request related to a problem? Please describe. Currently there are only two exception types. MsalClientException, MsalServiceException.

All further information, which exact error case is present, are embedded in long magic strings.

This makes the coding very complex and error-prone. Furthermore, it actually contradicts the .NET design recommendation that specific exceptions and no generic exceptions should be thrown. https://docs.microsoft.com/en-us/dotnet/standard/exceptions/ https://docs.microsoft.com/en-us/dotnet/standard/exceptions/how-to-use-specific-exceptions-in-a-catch-block

In addition, it is very time-consuming to analyze errors in log systems, e.g. Application Insights, on the basis of their texts/exception messages and not on the basis of their types.

Describe the solution you'd like Please provide specific exceptions.

Example: when client credentials expire (its secrets), you just get a MsalServiceException with a very long text.

Just provide a e.g. MsalClientCredentialsExpiredException (Can inherit from MsalServiceException to stay compatible with existing code) with additional properties (which client id). This makes programming more secure and we don't need magic strings anymore.

[bgavrilMS]

Thanks for the suggestion @BenjaminAbt. Can you provide more details on the scenario though? What exact exceptions would you like to have their own type?

[BenjaminAbt]

Hello Bogdan, thank you for your quick reply.

In principle, I am not concerned with a single exception, even though I noticed it again because of the Client Secret example here. I am concerned about all exceptions. Really all of them.

Make the class MsalServiceException abstract. And at each point where an MsalServiceException instance was actually created, throw a specific one.

Why Thousands of applications out there use this library and every app need to parse exception messages to get the exact error. Millions of lines of code are behind this. And certainly there are thousands of bugs because of it. This is a huge impact.

This also affects me with virtually every new platform I develop for customers based on Azure/MSAL. And it annoys me again and again to have to work with generic exceptions during development as well as during logging. This does not have to be. It must not be like that.

You have the power to simplify this scenario for millions of developers. Thanks :-)

[BenjaminAbt]

It could look like this

public abstract class MsalException : Exception
{
    protected MsalException(string errorCode, string message)
        : base(message)
    {
        ErrorCode = errorCode;
    }

    protected MsalException(string errorCode, string message, Exception innerException)
        : base(message, innerException)
    {
        ErrorCode = errorCode;
    }

    public string ErrorCode { get; }
}

public abstract class MsalServiceException : MsalException
{
    protected MsalServiceException(string errorCode, string message)
        : base(errorCode, message)
    {
    }

    protected MsalServiceException(string errorCode, string message, Exception innerException)
        : base(errorCode, message, innerException)
    {
    }
}

public class MsalServieClientCredentialsExpiredException : MsalServiceException
{
    protected MsalServieClientCredentialsExpiredException(string clientId)
        : base("AADSTS7000222", $"A configuration issue is preventing authentication [.....] The provided client secret keys for app '{clientId}' are expired.")
    {
        ClientId = clientId;
    }

    public string ClientId { get; }
}

I want to rely on a type and not on magic strings. I want to catch the type.

try
{

}
// we dont want this
catch (MsalServiceException e)
{
    if (e.Message.Contains("The provided client secret keys for app"))
    {
         // this is maybe a client credential expiration error

        // imagine string operation here to get the affected client id
    }
}
// we want this
catch (MsalServieClientCredentialsExpiredException e)
{
    Console.Write("Affected Id: " + e.ClientId);
}

[bgavrilMS]

Got it, the technical solution is not that complicated. The problem here is that the suberrors (https://docs.microsoft.com/en-us/azure/active-directory/develop/reference-aadsts-error-codes#aadsts-error-codes) are not guaranteed to be stable by AAD.

We can provide typed exceptions for the 8 big error codes (invalid_request, invalid_grant etc.). we already provide MsalUiRequiredException for 2 of them.

CC @jmprieur

[BenjaminAbt]

are not guaranteed to be stable by AAD.

This is the problem that makes so much existing code buggy - and often this is critical code because it's all about identity. At the end of the day, it's Microsoft that can plan the behavior. We as consumers of this library don't notice until it goes bang. This is depressing - and does not create trust.

Microsoft has to orchestrate that. No one else can do that. Other identity providers get this right, too. This is an identity system. We as developers and customers must be able to trust AAD. The experience has to be better.

With other things, the community can fix such deficits itself, because the errors are stable (e.g. https://github.com/Giorgi/EntityFramework.Exceptions which solves the generic exception behavior of EFCore). But in this case, Microsoft has to fix it.