Skip to content

Removes the middle-layer exception #458

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.

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

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Feb 10, 2022

@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
Copy link
Contributor

jiasli commented Feb 11, 2022

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] (Azure/azure-cli#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 (#406).

@rayluo
Copy link
Collaborator Author

rayluo commented Feb 11, 2022

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.

@rayluo rayluo merged commit 417792a into dev Feb 11, 2022
@rayluo rayluo deleted the actionable-runtime-error-for-adfs branch February 11, 2022 19:21
@rayluo rayluo mentioned this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants