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

Core auth flows #40084

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Core auth flows #40084

wants to merge 8 commits into from

Conversation

xiangyan99
Copy link
Member

No description provided.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@xiangyan99 xiangyan99 marked this pull request as ready for review March 17, 2025 19:10
@Copilot Copilot bot review requested due to automatic review settings March 17, 2025 19:10

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces support for the auth_flows keyword in bearer token authentication flows across synchronous and asynchronous pipelines.

  • Updated tests to pass the new auth_flows parameter to request-handling methods.
  • Modified the _authentication and _authentication_async policies to incorporate auth_flows when obtaining tokens.
  • Updated CHANGELOG and added a type-ignore comment for OpenTelemetry context detachment.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/core/corehttp/tests/async_tests/test_authentication_async.py Updated test cases to pass auth_flows and modified the token credential method.
sdk/core/corehttp/tests/test_authentication.py Updated assertions and token request options for auth_flows.
sdk/core/corehttp/corehttp/runtime/policies/_authentication.py Changed on_request signature and added auth_flows support in token acquisition.
sdk/core/corehttp/corehttp/runtime/policies/_authentication_async.py Updated asynchronous on_request to support auth_flows.
sdk/core/azure-core/azure/core/tracing/opentelemetry.py Added "# type: ignore" for a detachment call.
sdk/core/azure-core/CHANGELOG.md Documented the addition of auth_flows support in BearerTokenCredentialPolicy.
Comments suppressed due to low confidence (1)

sdk/core/corehttp/tests/async_tests/test_authentication_async.py:278

  • Avoid using mutable default arguments. Change the default value of options to None and initialize it to {} within the function.
async def get_token_info(self, *scopes, options={):
Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Can you explain the workflow here and how auth_flows will be used inside a credential's get_token_info method? Not quite sure why we need it at the policy level.

"""
self._enforce_https(request)

if self._token is None or self._need_new_token:
self._token = self._credential.get_token_info(*self._scopes)
options: TokenRequestOptions = {"auth_flows": auth_flows} if auth_flows else {} # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Add auth_flows to the TypedDict TokenRequestOptions class?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is optional. I would hold until the GA at least. :)

:raises: :class:`~corehttp.exceptions.ServiceRequestError`
"""

def on_request(self, request: PipelineRequest[HTTPRequestType]) -> None:
def on_request(
self, request: PipelineRequest[HTTPRequestType], *, auth_flows: Optional[list[dict[str, str]]] = None
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to take in auth_flows as a parameter here? Or can we just access it in this method via self._auth_flows?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have the parameter because we will support per-operation auth_flow. :)

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

Successfully merging this pull request may close these issues.

3 participants