Skip to content
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

[Core] Unify ADAL and MSAL error handler #17072

Merged
merged 6 commits into from
Mar 5, 2021
Merged

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Feb 24, 2021

Description

Refine the error message reported by #16209, #16641, #17142.

In the current code, wrapping adal.AdalError and rephrasing the error message actually doesn't provide more information. Instead, it leaves the original server error unexposed.

error code CLI error server error
AADSTS70008 Credentials have expired due to inactivity. https://login.microsoftonline.com/error?code=70008
AADSTS50079 Configuration of your account was changed. https://login.microsoftonline.com/error?code=50079
AADSTS50173 The credential data used by CLI has been expired because you might have changed or reset the password. https://login.microsoftonline.com/error?code=50173

This PR

  1. wraps adal.AdalError in AuthenticationError with the original server error unchanged
  2. adds az login instruction as recommendation

Testing Guide

Test expired refresh token

  1. Create a Conditional Access policy in AAD tenant to make the refresh token only valid for 1 hour.
    image
  2. az login with the user that is managed by the policy
  3. After one hour, run az group list or az account get-access-token

Error message

> az group list
AADSTS70043: The refresh token has expired or is invalid due to sign-in frequency checks by conditional access. The token was issued on 2021-02-23T05:06:57.0691981Z and the maximum allowed lifetime for this request is 3600.
Trace ID: 4999f039-c867-4cc2-ab32-7dc259780c00
Correlation ID: 6517ef89-dd65-47e1-b5fa-7193461fb78a
Timestamp: 2021-02-24 07:13:32Z
To re-authenticate, please run `az login`. If the problem persists, please contact your tenant administrator.

> az account get-access-token
AADSTS70043: The refresh token has expired or is invalid due to sign-in frequency checks by conditional access. The token was issued on 2021-02-23T05:06:57.0691981Z and the maximum allowed lifetime for this request is 3600.
Trace ID: ab764987-048b-4745-bc32-ba7d16840b00
Correlation ID: a2d45c9f-41de-4e18-8bf5-8368975c7561
Timestamp: 2021-02-24 06:54:48Z
To re-authenticate, please run `az login`. If the problem persists, please contact your tenant administrator.

@jiasli
Copy link
Member Author

jiasli commented Feb 24, 2021

Exposing the original error message is the same behavior as logging in in the browser.

For example, with invalid client_id:

- 04b07795-8ddb-461a-bbee-02f9e1bf7b46
+ 04b07795-8ddb-461a-bbee-02f9e1bf7b41
                                     ^

https://login.microsoftonline.com/common/oauth2/authorize?response_type=code&client_id=04b07795-8ddb-461a-bbee-02f9e1bf7b41&redirect_uri=http://localhost:8400&state=vsjy0x6w4z3ojcnq1ezl&resource=https://management.core.windows.net/&prompt=select_account&sso_reload=true

image

@jiasli jiasli changed the title {Core} Show original ADAL error [Core] Show original AAD error when authentication fails Feb 24, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 24, 2021

Core

@jiasli jiasli changed the title [Core] Show original AAD error when authentication fails [Core] Unify ADAL and MSAL error handler Feb 25, 2021
@jiasli
Copy link
Member Author

jiasli commented Feb 25, 2021

The exposure of original AAD error paves the way for solving #16988 and #16989.

except AttributeError:
# In case of AdalError created as
# AdalError('More than one token matches the criteria. The result is ambiguous.')
raise CLIError(str(err))
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use one of the new Error Types?

Copy link
Member Author

Choose a reason for hiding this comment

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

This error is caused by a known unknown issue in ADAL (#15320). As we are migrating to MSAL, this ADAL issue is not planned to be investigated and fixed. @houk-ms which exception do you prefer to use?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed offline, raising UnknownError for now and also providing comment and a recommendation:

# In case of AdalError created as
# AdalError('More than one token matches the criteria. The result is ambiguous.')
# https://github.com/Azure/azure-cli/issues/15320
from azure.cli.core.azclierror import UnknownError
raise UnknownError(str(err), recommendation="Please run `az account clear`, then `az login`.")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants