Skip to content

identity broker package - #32360

Merged
xiangyan99 merged 24 commits into
mainfrom
identity_broker
Oct 7, 2023
Merged

identity broker package#32360
xiangyan99 merged 24 commits into
mainfrom
identity_broker

Conversation

@xiangyan99

Copy link
Copy Markdown
Member

No description provided.

@pvaneck pvaneck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel like we should keep code duplication to a minimum and use whatever we can from azure.identity.

How about importing InteractiveBrowserCredential from azure.identity and making that the parent class of the "extended" class and just override the things that need changing:

from azure.identity import InteractiveBrowserCredential as TraditionalInteractiveBrowserCredential


class InteractiveBrowserCredential(TraditionalInteractiveBrowserCredential):
    """Updated docstring"""

    def __init__(self, **kwargs: Any) -> None:
        self._allow_broker = kwargs.pop("allow_broker", False)
        self._parent_window_handle = kwargs.pop("parent_window_handle", None)
        self._enable_msa_passthrough = kwargs.pop("enable_msa_passthrough", False)
        super().__init__(**kwargs)

    def _get_app(self, **kwargs: Any) -> msal.ClientApplication:
        ...

    @wrap_exceptions
    def _request_token(self, *scopes: str, **kwargs: Any) -> Dict:
        ...

@xiangyan99

Copy link
Copy Markdown
Member Author

I feel like we should keep code duplication to a minimum and use whatever we can from azure.identity.

How about importing InteractiveBrowserCredential from azure.identity and making that the parent class of the "extended" class and just override the things that need changing:

from azure.identity import InteractiveBrowserCredential as TraditionalInteractiveBrowserCredential


class InteractiveBrowserCredential(TraditionalInteractiveBrowserCredential):
    """Updated docstring"""

    def __init__(self, **kwargs: Any) -> None:
        self._allow_broker = kwargs.pop("allow_broker", False)
        self._parent_window_handle = kwargs.pop("parent_window_handle", None)
        self._enable_msa_passthrough = kwargs.pop("enable_msa_passthrough", False)
        super().__init__(**kwargs)

    def _get_app(self, **kwargs: Any) -> msal.ClientApplication:
        ...

    @wrap_exceptions
    def _request_token(self, *scopes: str, **kwargs: Any) -> Dict:
        ...

The problem is then we need to inherit from internal classes which we want to avoid.

@pvaneck

pvaneck commented Oct 6, 2023

Copy link
Copy Markdown
Member

The problem is then we need to inherit from internal classes which we want to avoid.

InteractiveBrowserCredential is a public credential type and having it as a parent class will majorly cut down on code duplication and make it clear that this broker browser credential is an extension of the traditional browser credential.

Comment thread sdk/identity/azure-identity-broker/MANIFEST.in Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_browser.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_user_password.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_utils.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_user_password.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_user_password.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_browser.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_user_password.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_utils.py Outdated
Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
xiangyan99 and others added 5 commits October 6, 2023 16:56
…_password.py

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
…_password.py

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
…ser.py

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>

@pvaneck pvaneck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noticed that the credential names changed. Was it preferable to have "Broker" in the name?

Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_browser.py Outdated
Comment thread sdk/identity/azure-identity-broker/azure/identity/broker/_browser.py Outdated
xiangyan99 and others added 2 commits October 6, 2023 17:27
…ser.py

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
…ser.py

Co-authored-by: Paul Van Eck <paulvaneck@microsoft.com>
@xiangyan99

Copy link
Copy Markdown
Member Author

Noticed that the credential names changed. Was it preferable to have "Broker" in the name?

I prefer to use a different name than InteractiveBrowserCredential. I will let architects decide the name.

@xiangyan99
xiangyan99 merged commit 2369143 into main Oct 7, 2023
@xiangyan99
xiangyan99 deleted the identity_broker branch October 7, 2023 01:49
Copilot AI mentioned this pull request Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

2 participants