Open jiasli opened 2 years ago
Thanks for bringing up this thought-provoking topic.
Indeed, MSAL tends to use generic exceptions. The PersistenceError
was actually an exception (pun not intended) to that convention, because there was a strong reason for PersistenceError
to exist.
While MSAL could potentially revisit that design choice, your centralized error handling helper seems an unusual choice in itself. I can understand that you design that helper to reuse some code, which is reasonable. Does it also intrinsically lose the context of each exception? For example, when you catch an requests.HttpError
in that monolithic error handler, you won't know which operation caused that HttpError, and then you won't be able to provide a really accurate error message, will you? That is probably why, catching exception on-the-spot seems to be more common.
By the way, if we examine this topic in the context of Python's built-in standard libraries, presumably many of them will share same set of generic exceptions. So, catching expected generic exceptions on-the-spot is a must.
Perhaps, you can get the best of both worlds, by implementing several smaller exception handlers. One of them can be def msal_exceptions_handler(ex)
which specialized in catching ValueError
. :-)
the necessity of capturing the error and handing it in all "joints" with MSAL.
In particular, how many joints are we talking about? I thought the entire MSAL usage has been wrapped inside a central helper. Would it be easy enough to implement on-the-spot exception handling in there?
This issue also happens for https://github.com/Azure/azure-cli/issues/20507 where ValueError
is raised for an invalid tenant.
Perhaps, you can get the best of both worlds, by implementing several smaller exception handlers. One of them can be
def msal_exceptions_handler(ex)
which specialized in catchingValueError
. :-)
This is exactly what I am thinking. Even though we have centralized helpers PublicClientApplication
and ConfidentialClientApplication
, it is still necessary to apply msal_exceptions_handler
on all methods, like acquire_token_interactive
, acquire_token_by_device_flow
, right? Unless we cast some black magic on __getattribute__
to make sure all methods are guarded by msal_exceptions_handler
.
I made a draft to demonstrate how it will look like if all MSAL interactions are wrapped with try-except clauses: https://github.com/Azure/azure-cli/pull/23311
As discussed in our meeting, if MsalValueError
can include an error_code
field, Azure CLI can safely log the error code in its client telemetry without storing the error message which potentially contains personal information. This can greatly help us understand users' failure reasons and frequencies.
@rayluo - I think this should be a bug, as it's related to supportability. App developers should be able to catch MSAL specific exceptions, observe the error code and claims etc. Can they do this now? Do you have a sample / wiki that shows how to?
Originally from https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/443#discussion_r895288236
Currently, MSAL raises Python's base error
ValueError
for scenarios such as tenant validation failure. Azure CLI can't really tell whether the error comes from MSAL or other places if we use a centralized error handling logic.Now we handle all
PersistenceError
(https://github.com/AzureAD/microsoft-authentication-extensions-for-python/pull/108) in a centralized place:https://github.com/Azure/azure-cli/blob/3dbcafbd273a5fefce793ba1e520dc61d8c468a8/src/azure-cli-core/azure/cli/core/util.py#L140-L151
This saves us from the necessity of capturing the error and handing it in all "joints" with MSAL.
But for this
ValueError
, we will have to capture it everywhere when MSAL interaction is made:https://github.com/Azure/azure-cli/blob/1d973cceb38980181eeaa45934497d2148a3e7b2/src/azure-cli-core/azure/cli/core/auth/identity.py#L141-L143
https://github.com/Azure/azure-cli/blob/1d973cceb38980181eeaa45934497d2148a3e7b2/src/azure-cli-core/azure/cli/core/auth/identity.py#L152
If we don't handle the error, the raw exception with traceback is shown, like in https://github.com/Azure/azure-cli/issues/20507, which is of course bad user experience.
I am thinking maybe MSAL can do the same for this
ValueError
(and other base error types raised) and create new error types which inherent fromValueError
, such asAuthorityError
,MsalValueError
.