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] WAM integration #22774

Closed
wants to merge 6 commits into from
Closed

[Core] WAM integration #22774

wants to merge 6 commits into from

Conversation

rayluo
Copy link
Member

@rayluo rayluo commented Jun 8, 2022

Related command

  • az login
  • And any subsequent command that would obtain a token via MSAL's acquire_token_silent().

Description

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

This PR is currently a draft to demonstrate how Azure CLI could enable the broker behavior from an MSAL feature branch.

Testing Guide

  1. Run the following commands on a Windows 10+ machine
  2. Somehow install Azure CLI from this feature branch, conceptually similar to this (however this command does not currently work)
    pip install git+https://github.com/azure/azure-cli@broker-integration
  3. Then run az login, and notice how a pop-up window (rather than a browser) would pop up.
  4. Try run some more (all?) different az foo bar command to make sure the token returned by broker would work

History Notes


This checklist is used to make sure that common guidelines for a pull request are followed.

@ghost ghost requested a review from yonzhan June 8, 2022 01:36
@ghost ghost added the Auto-Assign Auto assign by bot label Jun 8, 2022
@ghost ghost requested a review from wangzelin007 June 8, 2022 01:36
@ghost ghost assigned jiasli Jun 8, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone Jun 8, 2022
@ghost ghost added the Installation label Jun 8, 2022
@ghost ghost requested review from jiasli and jsntcy June 8, 2022 01:36
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 8, 2022

WAM

@@ -52,7 +52,7 @@
'jmespath',
'knack~=0.9.0',
'msal-extensions~=1.0.0',
'msal==1.18.0b1',
'msal[broker] @ git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@wam', # Temporary source
Copy link
Member

Choose a reason for hiding this comment

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

msal[broker] can also be manually installed with

pip install .\microsoft-authentication-library-for-python\[broker]

"http_cache": Identity._msal_http_cache
"http_cache": Identity._msal_http_cache,
# TODO: introduce a config option to allow turning broker off
"allow_broker": True
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
"allow_broker": True
"allow_broker": True,

:-p

Copy link
Member

Choose a reason for hiding this comment

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

Why? Isn't last comma supposed to be omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is less-defined, so there is no official preference that you are "supposed" to follow. However, since Python does allow the extra trailing comma, I prefer to take advantage of it.

@jiasli jiasli changed the title [msal] Broker integration [Core] WAM integration Jul 8, 2022
@jiasli
Copy link
Member

jiasli commented Jul 8, 2022

@rayluo, is WAM still considered auth code flow? If not, are we fully deprecating auth code flow for WAM-supported device?

@rayluo
Copy link
Member Author

rayluo commented Aug 2, 2022

@rayluo, is WAM still considered auth code flow? If not, are we fully deprecating auth code flow for WAM-supported device?

The broker (wam) behavior is indeed implemented in MSAL's acquire_token_interactive(), therefore replacing the traditional auth-code-based behavior.

This is meant to be a transparent change to Azure CLI, so Azure CLI does not need to deprecate anything. MSAL itself will still keep the auth code flow APIs because they will still be useful for web applications.

@jiasli
Copy link
Member

jiasli commented Sep 9, 2022

Some merge conflicts happen after supporting CAE (#23635). Also rework this PR in #23828.

@jiasli jiasli closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Installation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add WAM support in Azure CLI
3 participants