-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
…te and opt-in instructions
@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> |
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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.
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
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>
There was a problem hiding this 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.
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
az config set core.allow_broker=true<br> | ||
az account clear<br> | ||
az login |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/azure-cli-core/azure/cli/core/auth/landing_pages/success.html
Outdated
Show resolved
Hide resolved
Co-authored-by: Damien Caro <dcaro@microsoft.com>
Co-authored-by: Damien Caro <dcaro@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.