Skip to content
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

Integration with Broker-on-Mac #596

Merged
merged 5 commits into from
Aug 21, 2024
Merged

Integration with Broker-on-Mac #596

merged 5 commits into from
Aug 21, 2024

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Sep 19, 2023

This PR has been tested by an internal bug bash.

It will remain as a draft until the dependency PyMsalRuntime 0.14 is released on PyPI.
This PR depends on PyMsalRuntime 0.17 for its full support for SSH Cert and ROPC.

msal/__main__.py Outdated Show resolved Hide resolved
Comment on lines +1837 to +1902
OPTIONAL.

* If your app does not opt in to use broker,
you do not need to provide a ``parent_window_handle`` here.

* If your app opts in to use broker,
``parent_window_handle`` is required.

- If your app is a GUI app running on modern Windows system,
you are required to also provide its window handle,
so that the sign-in window will pop up on top of your window.
- If your app is a console app runnong on Windows system,
you can use a placeholder
``PublicClientApplication.CONSOLE_WINDOW_HANDLE``.
- If your app is running on Mac,
you can use a placeholder
``PublicClientApplication.CONSOLE_WINDOW_HANDLE``.
Copy link
Collaborator Author

@rayluo rayluo Oct 13, 2023

Choose a reason for hiding this comment

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

FYI, those lines are the changes (or the lack thereof) of the parent_window_handle requirement between Windows and Mac.

The window handle is required on Windows. Such a decision was influenced by the MsalRuntime's desire to make window handle explicit (CC @MSamWils), and now on Mac this parameter remains required. (CC @kaisong1990 )

If the parameter remains required on Windows, do we want to keep it as required on Mac, or do we make it optional on Mac? Will the latter behavior - i.e. sometimes required, sometimes not - be confusing in itself?

Note: Regardless of MSAL's choice, downstream library can make different decision, such as choose to make this parameter optional, by using MSAL's predefined CONSOLE_WINDOW_HANDLE placeholder as a default value. (CC @xiangyan99 )

Choose a reason for hiding this comment

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

Sorry, you mean it is required on Mac because it is required on Windows?

I am a little confused.

Copy link
Collaborator Author

@rayluo rayluo Oct 13, 2023

Choose a reason for hiding this comment

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

Sorry, you mean it is required on Mac because it is required on Windows?

I am a little confused.

Put it this way. We speculate that one consistent API across platform would be ideal. Most Python scripts are console apps and they shall ideally be cross-platform. How do we want those app developers to write their source code?

  1. app.acquire_token_interactive(
        ...,
        parent_window_handle=app.CONSOLE_WINDOW_HANDLE,  # Same placeholder across all platforms, even though it can be a no-op on some platforms
        ...)
  2. if sys.platform == "win32":
        app.acquire_token_interactive(
            ...,
            parent_window_handle=app.CONSOLE_WINDOW_HANDLE,  # Required on Windows
        ...)
    else:
        app.acquire_token_interactive(
            ...,
            # No need to specify parent_window_handle at all
        ...)

Ray Luo and others added 2 commits December 8, 2023 17:30
Use new enable_broker

Use 2 flags, one per supported platform

Documents the requirement on parent_window_handle
@rayluo rayluo marked this pull request as ready for review August 21, 2024 22:08
@rayluo rayluo requested a review from a team as a code owner August 21, 2024 22:08
@rayluo rayluo merged commit b9d9b05 into dev Aug 21, 2024
12 checks passed
@rayluo rayluo deleted the mac branch August 21, 2024 22:25
@rayluo rayluo mentioned this pull request Sep 5, 2024
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.

4 participants