-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add ChallengeAuthenticationPolicy #17489
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
Changes from all commits
75406a0
420909f
2f52480
8938731
15e2521
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| import time | ||
| import six | ||
|
|
||
| from . import SansIOHTTPPolicy | ||
| from . import HTTPPolicy, SansIOHTTPPolicy | ||
| from ...exceptions import ServiceRequestError | ||
|
|
||
| try: | ||
|
|
@@ -18,7 +18,27 @@ | |
| # pylint:disable=unused-import | ||
| from typing import Any, Dict, Optional | ||
| from azure.core.credentials import AccessToken, TokenCredential, AzureKeyCredential, AzureSasCredential | ||
| from azure.core.pipeline import PipelineRequest | ||
| from azure.core.pipeline import PipelineRequest, PipelineResponse | ||
|
|
||
|
|
||
| def _enforce_https(request): | ||
| # type: (PipelineRequest) -> None | ||
| """Raise ServiceRequestError if the request URL is non-HTTPS and the sender did not specify "enforce_https=False" | ||
| """ | ||
|
|
||
| # move 'enforce_https' from options to context so it persists | ||
| # across retries but isn't passed to a transport implementation | ||
| option = request.context.options.pop("enforce_https", None) | ||
|
|
||
| # True is the default setting; we needn't preserve an explicit opt in to the default behavior | ||
| if option is False: | ||
| request.context["enforce_https"] = option | ||
|
|
||
| enforce_https = request.context.get("enforce_https", True) | ||
| if enforce_https and not request.http_request.url.lower().startswith("https"): | ||
| raise ServiceRequestError( | ||
| "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." | ||
| ) | ||
|
|
||
|
|
||
| # pylint:disable=too-few-public-methods | ||
|
|
@@ -40,20 +60,7 @@ def __init__(self, credential, *scopes, **kwargs): # pylint:disable=unused-argu | |
| @staticmethod | ||
| def _enforce_https(request): | ||
| # type: (PipelineRequest) -> None | ||
|
|
||
| # move 'enforce_https' from options to context so it persists | ||
| # across retries but isn't passed to a transport implementation | ||
| option = request.context.options.pop("enforce_https", None) | ||
|
|
||
| # True is the default setting; we needn't preserve an explicit opt in to the default behavior | ||
| if option is False: | ||
| request.context["enforce_https"] = option | ||
|
|
||
| enforce_https = request.context.get("enforce_https", True) | ||
| if enforce_https and not request.http_request.url.lower().startswith("https"): | ||
| raise ServiceRequestError( | ||
| "Bearer token authentication is not permitted for non-TLS protected (non-https) URLs." | ||
| ) | ||
| return _enforce_https(request) | ||
|
|
||
| @staticmethod | ||
| def _update_headers(headers, token): | ||
|
|
@@ -94,6 +101,84 @@ def on_request(self, request): | |
| self._update_headers(request.http_request.headers, self._token.token) | ||
|
|
||
|
|
||
| class ChallengeAuthenticationPolicy(HTTPPolicy): | ||
| """Base class for policies that authorize requests with bearer tokens and expect authentication challenges | ||
|
|
||
| :param ~azure.core.credentials.TokenCredential credential: an object which can provide access tokens, such as a | ||
| credential from :mod:`azure.identity` | ||
| :param str scopes: required authentication scopes | ||
| """ | ||
|
|
||
| def __init__(self, credential, *scopes, **kwargs): # pylint:disable=unused-argument | ||
| # type: (TokenCredential, *str, **Any) -> None | ||
| super(ChallengeAuthenticationPolicy, self).__init__() | ||
| self._scopes = scopes | ||
| self._credential = credential | ||
| self._token = None # type: Optional[AccessToken] | ||
|
|
||
| def _need_new_token(self): | ||
| # type: () -> bool | ||
| return not self._token or self._token.expires_on - time.time() < 300 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to make '300' configurable?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not initially. We could add configuration later. |
||
|
|
||
| def authorize_request(self, request, *scopes, **kwargs): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the typical way for users to use the policy? Not in the pipeline? Why in addition to send, we also need to expose authorize_request, on_request & on_challenge?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a pipeline. Client library developers should subclass this to accommodate their particular services.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To help me understand, do you mean users are not supposed to use it directly. Instead, they should always inherit the class and use the customized one?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If they want to handle authentication challenges. There's no particular reason to use this policy otherwise, although doing so wouldn't break anything. The policy by itself authorizes requests with bearer tokens just fine.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks to me we set auth header in both authorize_request & on_request. My understanding is the one in on_request will override authorize_request? Why user needs to set it twice?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on_request is called first when a request arrives. authorize_request is a helper for subclasses to use, so they don't have to handle the credential or cached token (for example). Assuming neither method is overridden, authorize_request would overwrite a token applied by on_request. Which is fine, the policy is designed for a scenario like this:
|
||
| # type: (PipelineRequest, *str, **Any) -> None | ||
| """Acquire a token from the credential and authorize the request with it. | ||
|
|
||
| Keyword arguments are passed to the credential's get_token method. The token will be cached and used to | ||
| authorize future requests. | ||
|
|
||
| :param ~azure.core.pipeline.PipelineRequest request: the request | ||
| :param str scopes: required scopes of authentication | ||
| """ | ||
| self._token = self._credential.get_token(*scopes, **kwargs) | ||
| request.http_request.headers["Authorization"] = "Bearer " + self._token.token | ||
|
|
||
| def on_request(self, request): | ||
| # type: (PipelineRequest) -> None | ||
| """Called before the policy sends a request. | ||
|
|
||
| The base implementation authorizes the request with a bearer token. | ||
|
|
||
| :param ~azure.core.pipeline.PipelineRequest request: the request | ||
| """ | ||
|
|
||
| if self._token is None or self._need_new_token(): | ||
| self._token = self._credential.get_token(*self._scopes) | ||
| request.http_request.headers["Authorization"] = "Bearer " + self._token.token | ||
|
|
||
| def send(self, request): | ||
| # type: (PipelineRequest) -> PipelineResponse | ||
| """Authorizes a request with a bearer token, possibly handling an authentication challenge | ||
|
|
||
| :param ~azure.core.pipeline.PipelineRequest request: the request | ||
| """ | ||
| _enforce_https(request) | ||
|
|
||
| self.on_request(request) | ||
|
|
||
| response = self.next.send(request) | ||
|
|
||
| if response.http_response.status_code == 401: | ||
| self._token = None # any cached token is invalid | ||
| if "WWW-Authenticate" in response.http_response.headers and self.on_challenge(request, response): | ||
| response = self.next.send(request) | ||
|
|
||
| return response | ||
|
|
||
| def on_challenge(self, request, response): | ||
| # type: (PipelineRequest, PipelineResponse) -> bool | ||
| """Authorize request according to an authentication challenge | ||
|
|
||
| This method is called when the resource provider responds 401 with a WWW-Authenticate header. | ||
|
|
||
| :param ~azure.core.pipeline.PipelineRequest request: the request which elicited an authentication challenge | ||
| :param ~azure.core.pipeline.PipelineResponse response: the resource provider's response | ||
| :returns: a bool indicating whether the policy should send the request | ||
| """ | ||
| # pylint:disable=unused-argument,no-self-use | ||
| return False | ||
|
|
||
|
|
||
| class AzureKeyCredentialPolicy(SansIOHTTPPolicy): | ||
| """Adds a key header for the provided credential. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to use the exactly same method name? Looks confusing. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I hear you but the good name is taken 🤣
🤷
_enforce_https_impl?