Add WithOpenURL as an option for AcquireTokenInteractive
#422
Conversation
This allows the caller to provide an alternate implementation of the logic to launch the web browser to preform the interactive login flow, instead of using the system default browser. Fixes AzureAD#314
|
Kudos, SonarCloud Quality Gate passed!
|
|
@chlowell Interested in your thoughts on exposing an option here. We'd use this in |
| @@ -558,10 +559,33 @@ func WithRedirectURI(redirectURI string) interface { | |||
| } | |||
| } | |||
|
|
|||
| // WithOpenURL allows you to provide a function to open the browser to complete the interactive login, instead of launching the system default browser. | |||
| func WithOpenURL(openURL func(url string) error) interface { | |||
There was a problem hiding this comment.
Agreed with the functionality of the API, but maybe not with the form the API, i.e. can we have some form of property bag for all extensibility related to browser? Maybe something like
WithBrowserOptions(BrowserOptions)
This will avoid having to add to the top-level API everytime an advanced scenario needs to be open.
There was a problem hiding this comment.
By the way in MSAL .NET we allow folks to also customize the success message, failure message etc. It's just a matter of time before these are asked here as well.
There was a problem hiding this comment.
I also prefer property bags for methods but we're committed to the individual function pattern. An option function taking a property bag would be unconventional, and awkward next to the options we already have--wouldn't their settings belong in the bag as well?
By the way in MSAL .NET we allow folks to also customize the success message, failure message etc. It's just a matter of time before these are asked here as well.
See #357. If the contributor doesn't finish it, I will when I have the cycles.
There was a problem hiding this comment.
Also agreed with the functionality in principle. Does this design imply that the caller (i.e. app developer) is responsible for somehow detecting that the current environment has no browser, and then proactively providing a callback which (quoted from #314) "just print out the URL to the console and move on to polling"? How would they know they shall do that?
For what it's worth, in MSAL Python, we can attempt browser blindly, detects the error afterwards, and then (by default) show a hint on the console to end user, all without the app developer needing to know all those details.
There was a problem hiding this comment.
This option allows the caller to insert a callback that's entirely responsible for launching a browser and handling any error, replacing the MSAL Go code for that. We could do the same as MSAL Python, however that wouldn't address the azd scenario, letting the BROWSER environment variable specify an executable. MSAL Go could of course read BROWSER before doing what it does now and fall back to stdout afterward, but that still may not address everyone's scenario. I think adding this option is the best solution because I doubt we can do the right thing by default in all cases.
There was a problem hiding this comment.
Another scenario is to detect if Edge is installed on a machine and call Edge. Edge has a lot of functionality built in, that ensures Device CAP is satisfied and others. It's the only way to satisfy device CAP on linux today, until MSALs will be able to call the broker.
There was a problem hiding this comment.
Discussed with @chlowell offline. We clarified that, the new callback mechanism in this PR does not address any specific scenario in itself. We provide a flexibility to allow app developers to DIY. The onus is on app developer to detect and handle browser on different scenarios.
|
Kudos, SonarCloud Quality Gate passed!
|
|
It's available now in v1.1.0 |
This allows the caller to provide an alternate implementation of the logic to launch the web browser to preform the interactive login flow, instead of using the system default browser.
Fixes #314