allow_broker becomes conditional per platform #510
Conversation
| if allow_broker and sys.platform != "win32": | ||
| raise ValueError("allow_broker=True is only supported on Windows") |
There was a problem hiding this comment.
Personally, I think this check defeats the purpose of the word "allow". If an error is raised, it should be called use_broker instead.
That being said, I prefer use_broker as it is more explicit (explicit is better than implicit).
There was a problem hiding this comment.
That is indeed a naming dilemma, unfortunately.
- The word "allow" was influenced by the less-tangible fact that MsalRuntime does not and may never support some scenarios, such as ADFS, B2C, device code flow, SSH cert (work-in-progress), etc.. MSAL automatically falls back to browser in those scenarios, so that the app developer does not need to.
- But we recently identified a situation that MSAL should not fallback to browser when running on a not-yet-supported OS/platform. Please refer to section 3.2 in this internal design doc.
Even with the newly found No.2, the No.1 still applies. So, there is no good name here. Ideas are welcome, although we may not actually change it at this point, because it has been shipped in MSAL Python 1.20.
| @@ -549,6 +544,9 @@ def __init__( | |||
| ) | |||
| else: | |||
| raise | |||
|
|
|||
| if allow_broker and sys.platform != "win32": | |||
There was a problem hiding this comment.
I am curious what if I set allow_broker on Windows 7? As this won't trigger error.
There was a problem hiding this comment.
On Windows 7, it will indeed proceed and silently fallback to browser. This is considered OK, because we foresee no broker for Windows 7.
rayluo
force-pushed
the
conditional-allow-broker
branch
from
86ffa6b
to
5742035
Compare
|
Abandoning this PR, in favor of #613 |
The new guideline (internal design doc) is to set it conditionally based on the OS platform. For example,
allow_broker = sys.platform=="win32". And, this PR will also make MSAL Python error out to blockallow_broker=Trueon non-Windows platforms.We intend to ship this PR as a patch 1.20.1, because the previous "silently ignore allow_broker=True on mac and Linux" behavior in MSAL Python 1.20.0 is now considered as a bug.
The document is also updated accordingly. Please proofread the doc here.