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

[Bug] MSAL.PY's error has no indicator for InteractionRequired #701

Open
jiasli opened this issue May 15, 2024 · 1 comment
Open

[Bug] MSAL.PY's error has no indicator for InteractionRequired #701

jiasli opened this issue May 15, 2024 · 1 comment

Comments

@jiasli
Copy link
Contributor

jiasli commented May 15, 2024

Describe the bug
Azure CLI relies on AADSTS50076 to detect MFA error (Azure/azure-cli#12516):

https://github.com/Azure/azure-cli/blob/fb6630fd3081483195b00a740e9e645b0a18e2d3/src/azure-cli-core/azure/cli/core/_profile.py#L765

                if 'AADSTS50076' in msg:
                    # The tenant requires MFA and can't be accessed with home tenant's refresh token
                    mfa_tenants.append(t)

and let the user know interaction is required:

https://github.com/Azure/azure-cli/blob/fb6630fd3081483195b00a740e9e645b0a18e2d3/src/azure-cli-core/azure/cli/core/_profile.py#L797-L802

        # Show warning for MFA tenants
        if mfa_tenants:
            logger.warning("The following tenants require Multi-Factor Authentication (MFA). "
                           "Use 'az login --tenant TENANT_ID' to explicitly login to a tenant.")
            for t in mfa_tenants:
                logger.warning("%s", t.tenant_id_name)

Today I was told by MSAL team that clients shouldn't parse error_description. However, MSAL.PY has no clear indicator that interaction is required, with or without WAM:

With WAM:

{
    "error": "broker_error",
    "error_description": "SubError: basic_action V2Error: invalid_grant AADSTS50076: Due to a configuration change made by your administrator, or because you moved to a new location, you must use multi-factor authentication to access 'xxx'. Trace ID: xxx Correlation ID: xxx Timestamp: 2024-05-15 01:13:49Z. Status: Response_Status.Status_InteractionRequired, Error code: 3399614476, Tag: 557973645",
    "msal_telemetry": "..."
}

Without WAM:

{
    "error": "invalid_grant",
    "error_description": "AADSTS50076: Due to a configuration change made by your administrator, or because you moved to a new location, you must use multi-factor authentication to access 'xxx'. Trace ID: xxx Correlation ID: xxx Timestamp: 2024-05-15 01:16:43Z",
    "error_codes": [
        50076
    ],
    "timestamp": "2024-05-15 01:16:43Z",
    "trace_id": "xxx",
    "correlation_id": "xxx",
    "error_uri": "https://login.microsoftonline.com/error?code=50076",
    "suberror": "basic_action",
    "classification": "basic_action"
}

This makes Azure CLI have no way of knowing whether the error can be recovered by performing interactive authentication.

The MSAL example unconditionally launches acquire_token_interactive if acquire_token_silent fails to get an access token:

if not result:
logging.info("No suitable token exists in cache. Let's get a new one from AAD.")
print("A local browser window will be open for you to sign in. CTRL+C to cancel.")
result = global_app.acquire_token_interactive( # Only works if your app is registered with redirect_uri as http://localhost

To Reproduce
az login with an account that requires MFA for non-home tenants.

Expected behavior
MSAL.PY's error should have an indicator for InteractionRequired.

What you see instead
MSAL.PY's error has no indicator for InteractionRequired.

The MSAL Python version you are using
1.28.0

Additional context
Add any other context about the problem here.

@rayluo
Copy link
Collaborator

rayluo commented May 15, 2024

Today I was told by MSAL team that clients shouldn't parse error_description.

Well, error_description contains a human-readable, format-less string, so, it is true that it is not meant to be consumed programmatically based on an assumption of its format. Detecting an error code is understandable and somewhat OK; a better approach may be consuming the error_codes. Just be aware that the error_codes is not in OAuth2 specs so it may be absent when operating with non-AAD authorities.

However, MSAL.PY has no clear indicator that interaction is required, with or without WAM:

There was a discussion on how to reclassify many errors from AAD. Would need to catch up with that to know its latest decision.

Meanwhile, that Azure CLI adjustment today is also reasonable. Also, keep in mind that an InteractionRequired indicator alone would not give you the error code. We will investigate whether error codes can also be surfaced from the broker code path.

@iulico-1 iulico-1 added the ado tracked in ado label Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants