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] PREVIEW: Support Web Account Manager (WAM) login on Windows #23828

Merged
merged 2 commits into from
Sep 15, 2022

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Sep 9, 2022

Related command
az login

Description
Close #21201
Require AzureAD/microsoft-authentication-library-for-python#415
Rework #22774

Testing Guide

On Windows, users can opt in WAM login using:

az config set core.allow_broker=true

Then run

az login

Instead of launching a web browser, an account selector will be shown to ask the user to select a Windows account:

image

History Notes

[Core] PREVIEW: Support Web Account Manager (WAM) login on Windows. To opt in, run az config set core.allow_broker=true

Additional information

WAM will be enabled by default in year 2023.

@ghost ghost added the Auto-Assign Auto assign by bot label Sep 9, 2022
@ghost ghost requested a review from yonzhan September 9, 2022 07:29
@ghost ghost assigned jiasli Sep 9, 2022
@ghost ghost added this to the Sep 2022 (2022-10-12) - For Ignite milestone Sep 9, 2022
@ghost ghost added the Account az login/account label Sep 9, 2022
@jiasli jiasli mentioned this pull request Sep 9, 2022
3 tasks
@ghost ghost added the Core CLI core infrastructure label Sep 9, 2022
"flow with `az login --use-device-code`.",
self._msal_app.authority.authorization_endpoint)
elif ui == 'broker':
logger.warning("Please select the account you want to log in with.")
Copy link
Member Author

Choose a reason for hiding this comment

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

We may want to refine the warning message.

@@ -856,4 +856,8 @@ def _create_identity_instance(cli_ctx, *args, **kwargs):
# EXPERIMENTAL: Use core.use_msal_http_cache=False to turn off MSAL HTTP cache.
use_msal_http_cache = cli_ctx.config.getboolean('core', 'use_msal_http_cache', fallback=True)

return Identity(*args, encrypt=encrypt, use_msal_http_cache=use_msal_http_cache, **kwargs)
# PREVIEW: On Windows, use core.allow_broker=true to use broker (WAM) for authentication.
allow_broker = cli_ctx.config.getboolean('core', 'allow_broker', fallback=False)
Copy link
Member Author

@jiasli jiasli Sep 9, 2022

Choose a reason for hiding this comment

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

We may want to align the config name with Azure PowerShell. @Jacekey23 @chasewilson

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, Azure PowerShell will release separate versions for WAM, instead of using a config option.

Copy link
Member

@evelyn-ys evelyn-ys Sep 13, 2022

Choose a reason for hiding this comment

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

As mentioned, WAM will be enabled by default in year 2023. By then, should all linux users manually set core.allow_broker=False to avoid failure? Can we add --allow-broker parameter for az login to distinguish WAM and traditional browser way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we don't have the users manually adjust anything to avoid failure. That's not a great experience if the default login is an error. Is it possible to set the configuration appropriately from an environment variable? eg in PowerShell $IsLinux or $IsMacOS.

As for if we want to add a parameter like --allow-broker, we kind of run into the same issue. If we want to have the broker to be the default flow, we would need to set --allow-broker as a default parameter and only if it's Windows. Meaning we are dealing with the same issue.

Open to more discussion around this as it is a big change to how we handle the flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to align the config name with Azure PowerShell. @Jacekey23 @chasewilson

Could you point me to a complete list of our configurations? Even if it's just the code location where the input is managed. I'd just like to look at all of our configurations and make sure we're following a good format.

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, WAM will be enabled by default in year 2023. By then, should all linux users manually set core.allow_broker=False to avoid failure? Can we add --allow-broker parameter for az login to distinguish WAM and traditional browser way?

Ideally, we don't have the users manually adjust anything to avoid failure. That's not a great experience if the default login is an error. Is it possible to set the configuration appropriately from an environment variable? eg in PowerShell $IsLinux or $IsMacOS.

As for if we want to add a parameter like --allow-broker, we kind of run into the same issue. If we want to have the broker to be the default flow, we would need to set --allow-broker as a default parameter and only if it's Windows. Meaning we are dealing with the same issue.

Open to more discussion around this as it is a big change to how we handle the flow.

Those are all good points. MSAL Python foresaw this difficulty, so, one of the design goal is to save app developers from needing to conditionally enable/disable broker behavior. Our model is, the app developer only needs to opt-in when their app meets those prerequisites, and then MSAL Python will take care of those conditions such as the platforms. By the way, that is also why the parameter was named "allow_broker" rather than "enable_broker": after the app allows it, the MSAL will decide whether to actually use it. For example, currently MSAL Python effectively ignores allow_broker when current app is running on non-Windows. (You can try test Azure CLI dev branch on Linux to see how allow_broker=True is a no-op.)

In particular, please do not add "--allow-broker" as a parameter to "az login". The reason being, even though the broker UI is only visible/tangible during an "az login", the broker mechanism is holistic. It will also affect silent token refresh. So, it has to be a global toggle switch.

@jiasli jiasli requested review from medhir and rayluo September 9, 2022 07:33
@yonzhan
Copy link
Collaborator

yonzhan commented Sep 9, 2022

WAM support

Copy link
Member

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Approving with one more suggestion below.

src/azure-cli-core/azure/cli/core/auth/identity.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Account az login/account Auto-Assign Auto assign by bot Core CLI core infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add WAM support in Azure CLI
5 participants