Skip to content

allow_broker becomes conditional per platform #510

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

allow_broker becomes conditional per platform #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions msal/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ def __init__(
This factor would become mandatory
if a tenant's admin enables a corresponding Conditional Access (CA) policy.
The broker's presence allows Microsoft identity platform
to have higher confidence that the tokens are being issued to your device,
to have more confidence that the tokens are being issued to your device,
and that is more secure.

An additional benefit of broker is,
Expand All @@ -459,29 +459,24 @@ def __init__(
so that your broker-enabled apps (even a CLI)
could automatically SSO from a previously established signed-in session.

This parameter defaults to None, which means MSAL will not utilize a broker.
If this parameter is set to True,
MSAL will use the broker whenever possible,
and automatically fall back to non-broker behavior.
That also means your app does not need to enable broker conditionally,
you can always set allow_broker to True,
as long as your app meets the following prerequisite:
This parameter defaults to None, which means MSAL will not utilize a broker,
and your end users will have the traditional browser-based login experience.

* Installed optional dependency, e.g. ``pip install msal[broker]>=1.20,<2``.
(Note that broker is currently only available on Windows 10+)
You can set it to True, based on the OS platform.
Currently, MSAL supports broker on Windows 10+, and errors out on others.
So, for example, you can do ``allow_broker = sys.platform=="win32"``.

* Register a new redirect_uri for your desktop app as:
``ms-appx-web://Microsoft.AAD.BrokerPlugin/your_client_id``

* Tested your app in following scenarios:
In order to allow broker, your app must also meet the following prerequisite:

* Windows 10+
* Install optional dependency, e.g. ``pip install msal[broker]>=1.20,<2``.

* PublicClientApplication's following methods::
acquire_token_interactive(), acquire_token_by_username_password(),
acquire_token_silent() (or acquire_token_silent_with_error()).
* Register a new redirect_uri for your desktop app as:
``ms-appx-web://Microsoft.AAD.BrokerPlugin/your_client_id``

* AAD and MSA accounts (i.e. Non-ADFS, non-B2C)
* Test your app with AAD and MSA accounts (i.e. Non-ADFS, non-B2C)
in PublicClientApplication's following methods:
acquire_token_interactive(), acquire_token_by_username_password(),
acquire_token_silent() (or acquire_token_silent_with_error()).

New in version 1.20.0.
"""
Expand Down Expand Up @@ -549,6 +544,9 @@ def __init__(
)
else:
raise

if allow_broker and sys.platform != "win32":
Copy link
Contributor

@jiasli jiasli Oct 28, 2022 •
edited
Loading

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

raise ValueError("allow_broker=True is only supported on Windows")
Comment on lines +548 to +549
Copy link
Contributor

@jiasli jiasli Oct 28, 2022 •
edited
Loading

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

is_confidential_app = bool(
isinstance(self, ConfidentialClientApplication) or self.client_credential)
if is_confidential_app and allow_broker:
Expand Down
2 changes: 1 addition & 1 deletion sample/interactive_sample.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# Create a preferably long-lived app instance which maintains a token cache.
app = msal.PublicClientApplication(
config["client_id"], authority=config["authority"],
#allow_broker=True, # If opted in, you will be guided to meet the prerequisites, when applicable
#allow_broker=sys.platform in ["win32"], # If opted in, you will be guided to meet the prerequisites, when applicable
# See also: https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#wam-value-proposition
# token_cache=... # Default cache is in memory only.
# You can learn how to use SerializableTokenCache from
Expand Down
14 changes: 14 additions & 0 deletions tests/test_application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,3 +625,17 @@ def test_organizations_authority_should_emit_warnning(self):
self._test_certain_authority_should_emit_warnning(
authority="https://login.microsoftonline.com/organizations")


class TestBrokerAllowance(unittest.TestCase):
def test_opt_in_for_broker_should_error_out_on_nonsupported_platforms(self):
supported_platforms = ["win32"]
if sys.platform in supported_platforms:
try:
PublicClientApplication("client_id", allow_broker=True)
# It would either create an app instance successfully
except ImportError:
pass # Or detect the absence of MsalRuntime
else:
with self.assertRaises(ValueError): # We decide to error out
PublicClientApplication("client_id", allow_broker=True)