Skip to content

Actionable exception from ADFS username password flow #456

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 9, 2022

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Feb 8, 2022

Inspired by Azure/azure-cli#21068 (comment) , this PR wraps most "expected" ADFS username password flow into one RuntimeError with actionable message.

That being said, this actionable message is meant for MSAL's app developer. If you are the app developer, you probably want to catch this RuntimeError and translate it into your own exception or error message, for example "... consider use 'az login' instead". @jiasli

Comment on lines +1426 to +1428
raise RuntimeError(
"ADFS is not configured properly. "
"Consider use acquire_token_interactive() instead.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we include the original exception as an "inner" exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the "inner" exception would show up in Python3's error trace already, therefore visible by default.

However, I would expect Azure CLI to do this, to have an actionable guidance most suitable in the Azure CLI's context:

try:
    result = app.acquire_token_by_username_password(...)
except RuntimeError:
    raise CLIError('Username password login failed. You may consider using "az login" instead.')

Copy link
Contributor

@jiasli jiasli Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't "Username password login failed" be too vague? This exception chaining pattern is difficult for Azure CLI to get the inner exception message, like that of the original (ValueError, RuntimeError) and print it in the terminal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably print the exception you caught. It would contain some (perhaps too much) info. Alas, it is hard to strike a balance here. :-(

try:
    result = app.acquire_token_by_username_password(...)
except RuntimeError as e:
    raise CLIError('Username password login failed: {}. You may consider using "az login" instead.'.format(e))

@rayluo rayluo merged commit 2f3c7bb into dev Feb 9, 2022
@rayluo rayluo deleted the actionable-runtime-error-for-adfs branch February 9, 2022 09:09
@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