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] Add announcement for WAM-based login #25416

Merged
merged 9 commits into from
Mar 2, 2023
Merged

[Core] Add announcement for WAM-based login #25416

merged 9 commits into from
Mar 2, 2023

Conversation

medhir
Copy link
Contributor

@medhir medhir commented Feb 9, 2023

Related command
az login

Description
This PR updates the login success landing page to include an announcement for the date on which WAM broker will be used by default for Azure CLI login, alongside instructions for opt-in.

Testing Guide
N/A

History Notes


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

@yonzhan
Copy link
Collaborator

yonzhan commented Feb 9, 2023

@jiasli for awareness

@yonzhan yonzhan added this to the Feb 2023 (2023-03-07) milestone Feb 9, 2023
yonzhan
yonzhan previously approved these changes Feb 16, 2023
rayluo
rayluo previously approved these changes Feb 17, 2023
<p>You can close this window, or we will redirect you to the <a href="https://docs.microsoft.com/cli/azure/">Azure CLI documentation</a> in 10 seconds.</p>
<h3>You have logged into Microsoft Azure!</h3>
<p>You can close this window, or we will redirect you to the <a href="https://docs.microsoft.com/cli/azure/">Azure CLI documentation</a> in 2 minutes.</p>
<h3>Announcements</h3>
Copy link
Member

Choose a reason for hiding this comment

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

This is a clever usage of MSAL Python's success_template feature. Good job. I guess MSAL shall better officially document this parameter/behavior, then. :-)

Copy link
Member

Choose a reason for hiding this comment

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

One more thing. Currently, the broker is only available on Windows. Shall we keep using the old success template when Azure CLI is running on non-Windows platform, and only use the new one on Windows?

The conditional, per-platform guidance is inevitable in the long run, because we will probably need that pattern in the near future when the broker on other platforms becomes available.

@jiasli jiasli requested review from jiasli and dcaro February 17, 2023 06:28
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>
@medhir medhir dismissed stale reviews from rayluo and yonzhan via a552081 February 21, 2023 23:42
medhir and others added 4 commits February 21, 2023 15:42
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>
Copy link

@dcaro dcaro left a comment

Choose a reason for hiding this comment

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

Overall looking good. See my comments.

Comment on lines +28 to +30
az config set core.allow_broker=true<br>
az account clear<br>
az login
Copy link

Choose a reason for hiding this comment

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

@jiasli can we simplify this bloc of code?

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, @jiasli, is the "az account clear" necessary? My understanding is, after enabling broker, the subsequent "az foo bar" may fail and then Azure CLI's preexisting error message will guide end user to do an "az login" anyway.

Copy link
Member

Choose a reason for hiding this comment

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

after enabling broker, the subsequent "az foo bar" may fail and then Azure CLI's preexisting error message will guide end user to do an "az login" anyway.

Relying on the error impairs user experience, right. 🙂

My consideration is that we want users to run az account clear so that the existing MSAL cache is purged, and make sure no credentials are left behind.

jiasli and others added 2 commits February 22, 2023 09:55
Co-authored-by: Damien Caro <dcaro@microsoft.com>
Co-authored-by: Damien Caro <dcaro@microsoft.com>
dcaro
dcaro previously approved these changes Feb 22, 2023
Copy link

@dcaro dcaro left a comment

Choose a reason for hiding this comment

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

LGTM

@jiasli jiasli changed the title [Auth] Add announcement for WAM-based login [Core] Add announcement for WAM-based login Feb 24, 2023
@jiasli
Copy link
Member

jiasli commented Feb 24, 2023

Here is a preview of the webpage:

image

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.

7 participants