AzureAD / microsoft-authentication-library-for-python

Microsoft Authentication Library (MSAL) for Python makes it easy to authenticate to Microsoft Entra ID. General docs are available here https://learn.microsoft.com/entra/msal/python/ Stable APIs are documented here https://msal-python.readthedocs.io. Questions can be asked on www.stackoverflow.com with tag "msal" + "python".
https://stackoverflow.com/questions/tagged/azure-ad-msal+python
Other
801 stars 200 forks source link

Removes the middle-layer exception #458

Closed rayluo closed 2 years ago

rayluo commented 2 years ago

@jiasli Azure CLI can then just catch all exceptions happened in acquire_token_by_username_password(), and then chain it with your CLIError:

try:
    app.acquire_token_by_username_password(...)
except Exception as ex:
    raise CLIError('"az login -u ... -p ..." was unsuccessful. You may consider use "az login" instead.') from ex
jiasli commented 2 years ago

Actually we don't need this PR that much in order to roll back #456, as Azure CLI can get the inner exception from __cause__ if raise ... from ... clause is used. Using __cause__ is much more accurate than __context__ because __context__ means the error handling itself failed!

https://www.python.org/dev/peps/pep-3134/#rationale

For an explicitly chained exception, this PEP suggests __cause__ because of its specific meaning. For an implicitly chained exception, this PEP proposes the name __context__ because the intended meaning is more specific than temporal precedence but less specific than causation: an exception occurs in the context of handling another exception.

Also, acquire_token_by_username_password may also throw OSError: [WinError -2146893813] (https://github.com/Azure/azure-cli/issues/20231) during encryption/decryption, right? In thus case, asking the user to run az login is meaningless.

I feel we need more accurate error like MsalAuthenticationError or WstrustError to make sure Azure CLI can clearly identify this is a recoverable error caused by the username/password flow. That being said, we can simply replace the newly created RuntimeError from #456 with WstrustError and keep MSAL's recommendation. Then CLI can retrieve the inner exception with __cause__, but of course this requires dropping Python 2 support (https://github.com/AzureAD/microsoft-authentication-library-for-python/issues/406).

rayluo commented 2 years ago

Summarizing our offline conversation here:

Actually, the exception chaining mechanism is designed exactly for such purpose. We shouldn't fear to use it.

Exception chaining was indeed designed for that purpose. But I wonder how such a noble intention would pan out, if python developers have loooong chaining design. IMO, there are 3 conceptual layers: app <-> middle-layer library <-> low level api calls. In reality we can easily see 4, 5, 6+ layers, but still, I believe only the inner-most layer can provide the exact "crime scene", and only the outer-most layer can provide the most relevant actionable mitigation. Everything in between is meaningless noise, especially to end users. A long exception stack trace would just scare them away.

That is why, regardless of when MSAL would drop Python 2 support, I am still not convinced MSAL has strong reason to add/chain the middle-layer exception only for providing MSAL's recommendations. If, unlike in PR #456, MSAL in this PR will always bubble up the inner-most exception, then Azure CLI can just catch it (with or without chaining) and provide your actionable message.

The WstrustError sounds like a reasonable idea, though. We will give it some thoughts. The thing is not just about this one WstructError. It is about if we go with this direction, how many these kinds of errors we would need eventually, (and if we also potentially backport many historical RuntimeError into different AbcError, XyzError, etc.). Meanwhile, AzureCLI can just catch ValueError and RuntimeError + the current PR, as a solution for now.