B2c implementation #104
B2c implementation #104
Conversation
It will guide app developer to use validate_authority=False when needed.
This would become handy when we meet B2C authority with customized domain.
There was a problem hiding this comment.
LGTM.
Do I understand right that:
- we avoid doing authority validation when we detect an authority containing b2clogin.com ?
- for custom domains, the developer would have to turn on authority validation explicitly?
What about detecting that there is an authority with an extra segment (the policy) ?
Answered by Ray inline here, to make the Q&A stay together:
- Yes
- They need to do that, but they do not have to remember to do so. Our SDK will remind them with a meaningful error message when we detect an Instance Discovery failure. #Supportability
- Detecting the extra segment would be unreliable, due to the flexibility of that B2C authority format. For example, there use to be a
/tfpsignature and now it is gone.
| @@ -417,3 +435,49 @@ def test_acquire_token_obo(self): | |||
| result = cca.acquire_token_silent(downstream_scopes, account) | |||
| self.assertEqual(cca_result["access_token"], result["access_token"]) | |||
|
|
|||
| def _build_b2c_authority(self, policy): | |||
| base = "https://msidlabb2c.b2clogin.com/msidlabb2c.onmicrosoft.com" | |||
| return base + "/" + policy # We do not support base + "?p=" + policy | |||
There was a problem hiding this comment.
it's probably fine as supporting B2C in MSAL.Python it's a new feature.
There was a problem hiding this comment.
To be clear, that inline comment is inside an internal test case, so it is more like a remind-to-ourselves, but did not mean it as a limitation or deficiency. In fact, currently all the public materials we can find, uses the "base/policy" format, rather than "base?p=policy" format. Such as:
- The official B2C doc, like this one,
- The MSAL .Net wiki page
- The test cases in MSAL Java
That being said, I did heard from a previous internal presentation that the "base?p=policy" format would also work. We (the entire MSAL fleet) might actually want to switch to that format at a later time, because the current way - putting policy inside authority - has some intrinsic issues, but that is a different topic.
| if payload.get("error") == "invalid_instance": | ||
| raise ValueError( | ||
| "invalid_instance: " | ||
| "The authority you provided, %s, is not whitelisted. " |
There was a problem hiding this comment.
nit: maybe use "on the allow list" instead...idk.
There was a problem hiding this comment.
Actually I thought about several options when I drafted that sentence. To me, "allow" does not sound right, because customer can of course choose a customized domain Contoso.com and we MSFT is in no position to disallow that. The precise cause of this error message is that the Contoso.com is not a so-called "well known" domain to MSFT Identity platform. And then again, it feels a bit weird to comment on other's name as "not well known", so I think "not whitelisted" sounds more neutral in tone. What do you think?
There was a problem hiding this comment.
would like to see the actual developer experience, like what the developer would need to do. or i can just learn how to run your stuff, which i should do as well. :) I'm only concerned about sovereign clouds at this point. looking forward to further discussion.
ANSWERED inline by Ray: Because we managed to maintain the API surface unchanged, the actual developer experience is literally the same as non-B2C scenarios. Only when they need to use some B2C only behavior, such as Edit Profile in their web app, they develop a new page for it, which they would need to do anyway. See it in action in this sample PR.
Based on the way we implemented it, the previous implementation would still work, in a sense that the app dev would be guided to bypass the Instance Discovery. This commit merely adds a shortcut so that app dev would not have to explicitly toggle validate_authority=False.
There is an automated test case in this PR to cover username password flow (i.e. ROPC grant), and it passes.
Currently, this implementation is still considered in an early stage. We can use it as a prototype to work on some sample as proof-of-concept, in order to get the whole picture of end-to-end experience of the "new" API surface and calling patterns. The interesting part of this PR, is something NOT in this PR: You will see this PR did NOT touch the source file
application.pywhich contains current public API definition. That means, we keep the high level API surface unchanged for B2C and non-B2C scenarios.We can install this branch by:
We can organize a code walk-through when we officially start the PR code review at a later time.
This PR will eventually resolve #52 .