-
Notifications
You must be signed in to change notification settings - Fork 203
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
feat: add integrated windows authentication support under public clients #652
base: dev
Are you sure you want to change the base?
Conversation
Thanks, @shajia-deshaw . Is there any way to test this? Can they be written into test cases? |
Sure @rayluo - I can add the test cases shortly (I had this tested with a driver script internally) from msal import PublicClientApplication
app = PublicClientApplication(
"<client_id>",
authority="https://login.microsoftonline.com/<tenant_id>")
token = app.acquire_token_integrated_windows_auth("<upn>")
print(token) |
@@ -172,6 +172,7 @@ class ClientApplication(object): | |||
ACQUIRE_TOKEN_FOR_CLIENT_ID = "730" | |||
ACQUIRE_TOKEN_BY_AUTHORIZATION_CODE_ID = "832" | |||
ACQUIRE_TOKEN_INTERACTIVE = "169" | |||
ACQUIRE_TOKEN_INTEGRATED_WINDOWS_AUTH_ID = "870" |
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.
I'm not sure how this number is mapped. I just added a random number for IWA btw.
@@ -243,7 +243,7 @@ def __add(self, event, now=None): | |||
at["refresh_on"] = str(now + refresh_in) # Schema wants a string | |||
self.modify(self.CredentialType.ACCESS_TOKEN, at, at) | |||
|
|||
if client_info and not event.get("skip_account_creation"): | |||
if (client_info or iwa) and not event.get("skip_account_creation"): |
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.
The access token returned by IWA did not meet the criteria to be added as an ACCOUNT
credential type in the TokenCache
i.e. it didn't have client_info
in the response. It was being added as a ACCESS_TOKEN
credential type. The cache test in the e2e tests were failing because of it since the get_accounts()
call didn't return anything and it assumed there was nothing being added to the cache. Hence, I added a special case of iwa
to be added to the ACCOUNT
credential type.
The other alternative is to add a method for get_access_tokens
similar to get_accounts
and use it to decide if we need to acquire new tokens silently for the items in cache.
Hello folks, just an update - I'm working with my employer to sign the CLA soon. |
@microsoft-github-policy-service agree company="D. E. Shaw & Co., L.P." |
Hello @ashok672 - Checking if you had a chance to review the PR? Thanks. |
I just wanted to check if this PR was going to be reviewed and merged. We're very interested in getting IWA support in the msal python library. Thanks! |
I don't think so, we are discussing to deprecate Integrated Windows Auth from all MSALs in favor of WAM (broker use). Integrated Windows Auth relies on SAML and HTTP level contracts which are easily broken by network changes, and this causes a lot of churn and support issues. WAM uses a different protocol (PRT) which is not affected. @iulico-1 @localden - can you provide a perspective as scenario owners? |
For the sake of completeness, we had a similar conversation in the feature request issue with the contributor, who mentioned that IWA also works on Linux and that was part of the reason that they worked out this PR. That is a scenario that not currently satisfied by broker. |
Thanks @rayluo for adding context! FTR, we've been using the patched version of MSAL with Kerberos / IWA flow for months in our internal linux services and applications -- Things have been working just fine. However, we believe this could be a general enhancement in the upstream MSAL library, benefiting similar users, as evident from the comments on the issue/PR. |
Adding my perspective here - on Windows and macOS, we are indeed shifting to authentication brokers; however, that leaves Linux scenarios in the open and we will need to continue falling back on the vanilla API. I am not inherently opposed to having a native IWA implementation in MSAL Python, but we'd need to coordinate how it falls back to broker and when. |
azure_region=azure_region, client_credential=client_secret) | ||
self.assertEqual( | ||
self.app.get_accounts(username=username), [], "Cache starts empty") | ||
result = self.app.acquire_token_integrated_windows_auth( |
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.
I am not convinced we need to introduce Windows integrated Auth flow as a separate API.
Windows Integrated Auth is conceptually the same as functionality as default account that brokers (WAM) expose.
Can we follow the same pattern we did with OS account on .net ?
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.
@iulico-1 - how is this going to work on Linux, where there is no WAM? We can't expect every customer to enroll their device either.
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.
On linux there is no WIA also :) this api will just fail with UIRequired on linux without broker integration.
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.
On linux there is no WIA also :) this api will just fail with UIRequired on linux without broker integration.
But the customer claimed otherwise.
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.
A form of WIA can be setup on Linux.
Overview
Adds support for Integrated Windows Authentication similar to the Java/.NET clients.
Fixes: #31