Skip to content

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

Merged
Jiashuo Li (jiasli) merged 9 commits into
Azure:devfrom
medhir:dev
Mar 2, 2023
Merged

[Core] Add announcement for WAM-based login#25416
Jiashuo Li (jiasli) merged 9 commits into
Azure:devfrom
medhir:dev

Conversation

@medhir

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

Jiashuo Li (@jiasli) for awareness

<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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
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>
Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com>

@dcaro Damien Caro (dcaro) left a comment

Copy link
Copy Markdown
Contributor

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 thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Comment on lines +28 to +30
az config set core.allow_broker=true<br>
az account clear<br>
az login

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Jiashuo Li (@jiasli) can we simplify this bloc of code?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, Jiashuo Li (@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
Copy Markdown
Contributor

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.

Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
Jiashuo Li (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 Damien Caro (dcaro) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html Outdated
@jiasli Jiashuo Li (jiasli) changed the title [Auth] Add announcement for WAM-based login [Core] Add announcement for WAM-based login Feb 24, 2023
@jiasli

Copy link
Copy Markdown
Contributor

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