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

Add option --prompt-hint to enable customized header text for WAM prompts and web mode. #11

Merged
merged 19 commits into from Apr 14, 2022

Conversation

goagain
Copy link
Contributor

@goagain goagain commented Apr 2, 2022

Overview

Taking advantage of this feature will give us a small UX improvement to let WAM based dialogs show header text to indicate what application is requesting auth. When an Auth prompt launches either by native window or web prompt, it’s often hard to know what actually triggered it. Enabling header text in WAM window could help users to know that.
Ref: microsoft-authentication-library-for-dotnet #3125

Change log

Add option --prompt-hint in command line and option prompt_hint in TOML config file.

Test Case

--debug --resource=6e979987-a7c8-4604-9b37-e51f06f08f1a --client=5af6def2-05ec-4cab-b9aa-323d75b5df40 --tenant=9188040d-6c67-4c5b-b112-36a304b66dad --mode=broker --prompt-hint="test header text"

Note

The resource ID 1a0aa568-29b5-4a0c-b895-89da91675dcc and client ID c332fbf4-f566-4550-ba1f-83a2f68fe068 are randomly generated. The tenant ID 9188040d-6c67-4c5b-b112-36a304b66dad is Microsoft account tenant ID. See Microsoft identity platform access tokens.

Overcome Sample

image

Update on Apr 7th, 2022:

Per discussed with @kyle-rader, caller is a better name than header-text. With API WithEmbeddedWebViewOptions, the title in web mode can also be modified. Therefore, web mode can be also supported.

Web Mode Overcome

image

Update on Apr 13th, 2022:

  • The final option name is --prompt-hint.
  • Unit tests have been added.

@goagain goagain marked this pull request as ready for review April 4, 2022 17:04
@shalinikhare27
Copy link
Contributor

I am curious why header text is an option? Isn't header text going to be automatically generated to let users know what triggered a prompt? Why would a user add header text as an option?

Co-authored-by: Shalini Khare <31912044+shalinikhare27@users.noreply.github.com>
@goagain
Copy link
Contributor Author

goagain commented Apr 4, 2022

I am curious why header text is an option? Isn't header text going to be automatically generated to let users know what triggered a prompt? Why would a user add header text as an option?

I think this option should be filled by the caller. Maybe @kyle-rader can explain more clear.

@reillysiemens
Copy link
Member

Technically this change looks sound. All the code is in the right spot and it's very clean, @goagain.

Following on from what @shalinikhare27 said though, I have some questions. I'm interested to know what you and @kyle-rader think about these issues.

  1. I thought the goal was just to allow us to customize the WAM prompt to let users know they were being prompted by the azureauth CLI, not to allow per resource-client-tenant triplet customization of the prompt (though I see how that could be helpful). Do we have enough of a use case to add this to the config file?
  2. Is --header-text the right name for this flag? It seems to me like this is narrowly scoped to be the header text for a WAM prompt. If we do want to allow this customization at a user level perhaps we could use --wam-prompt as a more specific name?
  3. There's some awkwardness about having this as an option which only makes sense in conjunction with a specific argument to another option. That is, I think this only makes sense when used with the --mode broker flag, but at best it would be a no-op with any other mode. Context dependent flags are often odd in my experience and might be indicative of a larger design issue.
  4. This flag is not only context dependent, but also operating system specific. @mvanchaa worked on a feature to make --mode broker unavailable on macOS where it doesn't make sense. On a Mac this flag would be like a vestigal organ of sorts.

@bgavrilMS
Copy link
Member

bgavrilMS commented Apr 5, 2022

I am curious why header text is an option? Isn't header text going to be automatically generated to let users know what triggered a prompt? Why would a user add header text as an option?

The default header text is something like "Let's get you signed in"

There are 2 scenarios where app developers wanted to use this:

  • apps like git + Git Cred Manager, which may run in the background, may get into a situtation where interactive auth is required. This tends to surprise the user, who does not know where the prompt comes from (i.e. "Git needs to authenticate")
  • apps like Visual Studio have many components that may trigger auth, so they want to have a way to differentiate between them ("The VS debugger needs to authenticate" vs "The VS Azure Explorer needs to authenticate").

From the SDK perspective:

  • we don't have enough info to provide a better header text. What would we use? The name of the executable? It wouldn't really work for the 2 scenarios above.
  • Identity SDKs do not output text to end-users, only to app developers. We are not setup to handle translations of text etc.

@bgavrilMS
Copy link
Member

The code looks good @kyle-rader , but I just want to make you aware of the experience with the latest MSAL. If your app targets only Work and School accounts, i.e. authority ends in /organization or /<tenant_id> we decided to bypass the Account Setting Pane, because it has reliability issues and WAM's AAD plugin can fully handle the auth experience.

So this header text will only appear if the applicaiton targets both AAD and MSA users.

bgavrilMS
bgavrilMS previously approved these changes Apr 5, 2022
@goagain goagain changed the title Add parameter --header-text to enable customized header text for WAM prompts Add option --caller to enable customized header text for WAM prompts and web mode. Apr 7, 2022
@goagain goagain marked this pull request as draft April 7, 2022 18:56
src/MSALWrapper/TokenFetcherPublicClient.cs Outdated Show resolved Hide resolved
src/AzureAuth/CommandMain.cs Outdated Show resolved Hide resolved
@kyle-rader
Copy link
Contributor

@goagain @reillysiemens I'm good with --prompt-hint

@goagain goagain changed the title Add option --caller to enable customized header text for WAM prompts and web mode. Add option --prompt-hint to enable customized header text for WAM prompts and web mode. Apr 13, 2022
Copy link
Member

@reillysiemens reillysiemens left a comment

Choose a reason for hiding this comment

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

I caught one test method that could use a better comment, otherwise I'd be happy to put a ✅ on this.

src/MSALWrapper.Test/TokenFetcherPublicClientTest.cs Outdated Show resolved Hide resolved
reillysiemens
reillysiemens previously approved these changes Apr 13, 2022
reillysiemens
reillysiemens previously approved these changes Apr 13, 2022
src/MSALWrapper/TokenFetcherPublicClient.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
mvanchaa
mvanchaa previously approved these changes Apr 13, 2022
@goagain goagain merged commit 5d3bff7 into main Apr 14, 2022
@goagain goagain deleted the users/ruitang/header-text branch April 14, 2022 19:16
@reillysiemens reillysiemens added the enhancement New feature or request label Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants