-
Notifications
You must be signed in to change notification settings - Fork 202
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
Enable broker support on Linux for WSL #766
base: dev
Are you sure you want to change the base?
Conversation
/azp run MSAL-Python-SDL-CI |
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.
Thanks, this PR looks good to me. And please make sure get an approval from Ray.
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.
Thanks for a clean PR! Implementation wise, it looks good. I added some suggestions above, mostly in terms of our workflow. Please make corresponding changes and then wait for the PyMsalRuntime release.
Co-authored-by: Ray Luo <[email protected]>
Co-authored-by: Ray Luo <[email protected]>
Co-authored-by: Ray Luo <[email protected]>
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.
We will also need to change the precise dependency version error message here and a approximate version hint there
updated |
@DharshanBJ, I would love to see this wrapped up. I arrived here after trying to debug errors with using msal on WSL. This PR worked "as-advertised" and made my life a whole lot easier. Before your PR, the following snippet kept returning a browser window with Testing with app = PublicClientApplication(
<client_id>,
authority=<authority>,
enable_broker_on_linux=True,
)
app.acquire_token_interactive(...) |
This work enables Data Scientists at Microsoft to develop and debug their pipelines better. Some of our models are deployed on linux environments. We believe that it is crucial to develop and test our work on the same environment that we deploy on, which is why I highly approve of this PR 😊 @rayluo any chance you could take a look at the state of this PR when you have the time? |
@DharshanBJ , that feels like some sort of debug log. Could it be turned off or removed from that "msal.wsl.proxy"? CC: @jiasli |
@thomasaarholt , we agree with that part. Is your work going to be deployed on WSL or on standalone Linux (which may or may not have Intune installed)? Keep in mind that this PR is mainly targeting WSL. On standalone Linux, unless Intune is installed, MSAL Python will still fall back to browser-based UI. If your work after deployment will mainly run on standalone Linux without Intune, you might want to still focus testing the browser-based experience, for the same reason that you mentioned above. FWIW, the default browser-based experience unfortunately deteriorates on WSL but technically still functions. In a freshly installed WSL ubuntu, it would end up with a message like this on the console "gio: |
Hi again!
Deployment happens on standalone linux, but we don't use the broker path for authentication there, so I'm not worried here. We're using this only to improve the dev-experience. |
Yes, this can be done, we'll remove this as part of the next msal.wsl.proxy release |
Co-authored-by: Ray Luo <[email protected]>
Co-authored-by: Ray Luo <[email protected]>
Co-authored-by: Ray Luo <[email protected]>
Co-authored-by: Ray Luo <[email protected]>
self._enable_broker = bool( | ||
enable_broker_on_windows and sys.platform == "win32" | ||
or enable_broker_on_mac and sys.platform == "darwin") | ||
or enable_broker_on_mac and sys.platform == "darwin" | ||
or enable_broker_on_linux and sys.platform == "linux" |
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.
question
Hi @DharshanBJ , I realize that the in the current implementation, the enable_broker_on_wsl
below is for WSL-only, which matches its name well. But the enable_broker_on_linux
logic here actually covers all linux including WSL, meaning, an enable_broker_on_linux=True
on WSL will also enable broker. Is that what we want?
In other words, does enable_broker_on_linux
meant to imply "all linux including WSL", or do we meant to have an enable_broker_on_non_wsl_linux
?
(After we clarify this in this conversation, we shall fine tune the parameter name or doc accordingly.)
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.
Hi @rayluo, yes, enable_broker_on_linux will enable broker on wsl and baremetal linux (all linux) and enable_broker_on_wsl enables broker only on wsl.
enable_broker_on_linux is meant to imply "all linux including WSL
@@ -1998,12 +2006,29 @@ def __init__( | |||
This parameter defaults to None, which means MSAL will not utilize a broker. | |||
|
|||
New in MSAL Python 1.31.0. | |||
|
|||
:param boolean enable_broker_on_linux: | |||
This setting is only effective if your app is running on Linux. |
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.
suggestion
Adding this clarification based on this conversation.
This setting is only effective if your app is running on Linux. | |
This setting is only effective if your app is running on Linux, including WSL. |
No description provided.