Skip to content

allow_broker becomes conditional per platform#510

Closed
rayluo wants to merge 1 commit intodevfrom
conditional-allow-broker
Closed

allow_broker becomes conditional per platform#510
rayluo wants to merge 1 commit intodevfrom
conditional-allow-broker

Conversation

@rayluo
Copy link
Contributor

@rayluo rayluo commented Oct 27, 2022

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 block allow_broker=True on 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.

Comment on lines 548 to 549
Copy link
Contributor

@jiasli jiasli Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is indeed a naming dilemma, unfortunately.

  1. 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.
  2. 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.

Copy link
Contributor

@jiasli jiasli Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious what if I set allow_broker on Windows 7? As this won't trigger error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

rayluo commented Nov 2, 2023

Abandoning this PR, in favor of #613

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants