Enable broker support on Linux for WSL #766
Conversation
|
/azp run MSAL-Python-SDL-CI |
| @@ -200,6 +200,7 @@ def _build_app(cls, | |||
| http_client=http_client or MinimalHttpClient(), | |||
| enable_broker_on_windows=broker_available, | |||
| enable_broker_on_mac=broker_available, | |||
| enable_broker_on_linux=broker_available, | |||
There was a problem hiding this comment.
praise: Thanks for finding this test file!
suggestion: If you haven't already, please also do python -m unittest tests.test_e2e and make sure we won't run into any issue. Note that you will need to setup an .env first, whose content is described at the beginning of this test_e2e.py file.
There was a problem hiding this comment.
We don't support POP and ssh-cert flows for linux yet, tested the other e2e flows.
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.
We will also need to change the precise dependency version error message here and a approximate version hint there
updated |
DharshanBJ
changed the title
|
@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(...) |
Co-authored-by: Ray Luo <[email protected]>
|
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? |
|
I have one oddity. When using the broker on Linux, I get the following text printed This does not happen on Windows: |
The "{{"ping","success"}}}" message is expected on WSL, it's from initializing the msal.wsl.proxy execuatble which is needed to talk to the windows broker on WSL. |
| @@ -704,7 +704,7 @@ def _decide_broker(self, allow_broker, enable_pii_log): | |||
|
|
|||
| def is_pop_supported(self): | |||
| """Returns True if this client supports Proof-of-Possession Access Token.""" | |||
| return self._enable_broker | |||
| return self._enable_broker and sys.platform != "linux" | |||
There was a problem hiding this comment.
Avoid using logic like "if var != one_thing", because you never know what the rest of things are. In this case, sys.platform has much more values than we should use.
Change this to sys.platform in ("win32", "darwin"). Same applies to other occurrences. Define a new internal helper when appropriate.
| is_ssh_cert_or_pop_request = ( | ||
| data.get("token_type") == "ssh-cert" or | ||
| data.get("token_type") == "pop" or | ||
| isinstance(auth_scheme, msal.auth_scheme.PopAuthScheme)) |
There was a problem hiding this comment.
There are two same snippets in this PR. How about refactoring it into an internal helper like this?
def _is_ssh_cert_or_pop_request(token_type, auth_scheme) -> bool:
...
@DharshanBJ , that feels like some sort of debug log. Could it be turned off or removed from that "msal.wsl.proxy"? CC: @jiasli |
| """ | ||
| if client_credential is not None: | ||
| raise ValueError("Public Client should not possess credentials") | ||
| # Using kwargs notation for now. We will switch to keyword-only arguments. | ||
| enable_broker_on_windows = kwargs.pop("enable_broker_on_windows", False) | ||
| enable_broker_on_mac = kwargs.pop("enable_broker_on_mac", False) | ||
| enable_broker_on_linux = kwargs.pop("enable_broker_on_linux", False) |
There was a problem hiding this comment.
The dev branch has received new changes. Please merge that into your branch, and then remove this line and add the new parameter into __init__().
| @@ -2162,7 +2180,9 @@ def acquire_token_interactive( | |||
| ) | |||
There was a problem hiding this comment.
One more thing. MSAL Python's caller (such as Azure CLI) can specify a parameter app.acquire_token_interactive(..., prompt="select_account") which will in turn call pymsalruntime.signin_interactively(). On Windows, that function will pop up the account selector, as requested. But on WSL, it does not pop any UI, and seemingly just return a token for the current user.
Is that expected, @DharshanBJ ? (Note that az login usually expects an account picker.) 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: |
No description provided.