-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support dynamically updating authentication headers #6915
Comments
Implementation options
|
My personal preference would be towards option 3. It’s simple, clean and would feel very familiar for people coming from “requests” lib. |
@Dreamsorcerer and maintainers, I welcome feedback on this. |
I created draft PR #6954 just to test out how the existing |
Just picking up some feedback from @webknjaz here: #6954 (comment) (thanks!) Personally, I don't feel adding an auth handler base class adds that much if the contract really is that it must be a co-routine function and just that. I'm not sure if that is related to the comment regarding the protocol... Libraries like |
For illustration, the expected usage is something like this: async def my_auth(request: ClientRequest) -> None:
token = await token_from_cache()
if not token:
token = await generate_token()
request.headers["Authorization"] = f"Bearer {token}"
async with ClientSession(auth=my_auth) as session:
url = "..."
async with session.get(url) as resp:
print(resp.status)
# OR
async with ClientSession() as session:
url = "..."
async with session.get(url, auth=my_auth) as resp:
print(resp.status) As a class-based solution, it could look like this: class MyAuth:
def __init__(self, identity_provider):
self._identity_provider = identity_provider
self._token_cache = ...
async def __call__(self, request: ClientRequest) -> None:
token = await self._token_cache.get()
if not token:
token = await generate_token(self._identity_provider)
await self._token_cache.set(token)
request.headers["Authorization"] = f"Bearer {token}"
my_identity_provider = ...
async with ClientSession(auth=MyAuth(my_identity_provider) as session:
url = "..."
async with session.get(url) as resp:
print(resp.status) |
My feeling is that we should probably just accept a coroutine and keep it generic. You can still pass a method from a more complex object, if you want to store state or something. I also think that calling this So, for the existing BasicAuth, you could end up with an approach like: auth = BasicAuth("user")
async with ClientSession(middleware=auth.middleware):
... With that new method being defined something like: https://github.com/aio-libs/aiohttp/pull/6954/files#diff-b797dd8733928df191ba2061121ab8b69976c185fcbfad4534891d3252b9ac30R165
In this example though, I'm wondering why we wouldn't have access to the ClientSession in our middleware. Would it be useful to access the session as well? Would it be an improvement if this code only updated Though I still think this would be more efficient without something being called on every request. How does async def set_token(session, event):
while True:
async with session.post("get_token") as resp:
token = await resp.json()
session.headers["Authorization"] = token["auth"]
event.set()
await asyncio.sleep(token["valid_duration"])
async with ClientSession() as session:
ready = asyncio.Event()
t = asyncio.create_task(set_token(session, event))
await ready.wait()
... # Your application code here...
t.cancel()
await t Maybe to make that safer, there could be a |
@webknjaz Any thoughts on this? |
@Dreamsorcerer thanks! Let me separate this out into a few distinct decision points below |
Auth-specific vs generic middleware/hookThis is really options 2-4 vs option 5 in #6915 (comment). I don't have a strong preference for either approach. Some advantages for doing something auth-specific:
Generic advantages:
The only thing with the generic solution is that there is some proxy-related auth logic in the client, so we need to make sure that this interacts as expected. |
Client session property vs request propertyI agree that in many case, one would set an auth handler on the session. However, the auth handler is in reality a property of an individual request, not the session. Different requests within a single session may require different authentication (or no authentication). The existing logic is correct in this sense whereby the session can provide a default auth value for use by any request. Similarly, the implicit session API ( |
Background token refresh vs on-send function callIn a typical token expiring case, one could rotate the auth header using a background task. But I think it would involve setting However, this would only work for default auth handlers on a per-session basis. Not for individual requests. I don't understand the concern about an on-the-fly function call to evaluate/populate the request's headers. Each |
Hook signature/APIIf we're going the auth-specific route, we may wish to adopt an interface whereby the given co-routine function when called and awaited, returns the actual header value. For example: async def my_auth() -> str:
token = await ...
return f"Bearer {token}" It would definitely be more restrictive. I'm not sure how many authentication flows there are that require request context or that require other request properties to be modified... In the more generic case, I suppose one could adopt the same, or a very similar, interface as used for the server. I guess the next issue would be whether it would support multiple (chained) middlewares. It could get complex quite quickly... |
All valid points, maybe we need some real world use cases to help decide what is and isn't needed.
I'm not sure I can think of a reason why you'd want to handle multiple auth headers in a single auth handler, but feel free to come up with scenarios and add them to the list. Then we can check that any given solution will work for all of these. |
To be clear, I was thinking about multiple auth handlers used for different connections within a single session. For example: my_auth = ...
async with ClientSession() as session:
url1 = "..."
async with session.post(url1, auth=my_auth) as resp:
print(resp.status)
url2 = "..."
async with session.get(url2, auth=None) as resp:
print(resp.status) |
Use case: session-wide OAuth Bearer token, periodically expiringIn this use case, we use a typical OAuth JWT bearer token which can be retrieved from an Identity Provider. The token contains an expiry time. The token should be refreshed for requests made after this time. Early token refresh is OK. Any request made in the session should have the |
One thought here, as an MVP, to support the above use case, could we add a setter to the property ‘ClientSession.headers’? Or are there strong reasons why this should remain a read-only property? |
I have created a PR for this, just to get started: PR #6983. If that's acceptable I will update docs and changelog to get this merged first before we decide on how to extend the Let me know if you're able to review this @Dreamsorcerer |
As discussed on PR #6983 this change is redundant. The other question still outstanding is whether we want to enhance the auth arg to support a simple coroutine function without having to wire up a background task. Let me formulate a use case to support that decision. |
Referencing other discussion thread requiring a custom authentication header based on the request body itself: Discussion #6627 One thought here is that if the authentication header value is just a function of the request body, one can easily compute the header value when generating the body on a request by request basis and pass both header and body as keyword arguments to the request method. |
<!-- Thank you for your contribution! --> ## What do these changes do? Adds a dedicated section on client authentication using the `auth` argument for HTTP Basic Auth. Also explains how to configure other authentication flows including periodic renewal of credentials. ## Are there changes in behavior for the user? No. Just makes it easier for developers to access authentication `aiohttp` features. Avoids users asking questions like this: #6908 ## Related issue number Issue #6915 ## Checklist - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes - [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [x] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: Sam Bull <aa6bs0@sambull.org>
Could you please show full example of updating headers dynamically with getting token? I cannot find this in docs. |
See the linked commit above your message: 981665a |
Yep, it's ok, but I cannot understand how can I update headers every hour (for example). I have class Periodic:
def __init__(self, func: Any, time: int) -> None:
self.func = func
self.time = time
self.is_started = False
self._task = None
async def start(self) -> None:
if not self.is_started:
self.is_started = True
self._task = asyncio.ensure_future(self._run())
async def stop(self) -> None:
if self.is_started:
self.is_started = False
self._task.cancel()
with suppress(asyncio.CancelledError):
await self._task
async def _run(self) -> None:
while True:
await asyncio.sleep(self.time)
self.func()
class SomeAiohttpClient:
…
@classmethod
def _update_authorization_header(cls, key: str, secret: str) -> None:
token = get_token(oauthkey=key, oauthsecret=secret)
cls.aiohttp_client.headers["Authorization"] = f"Bearer {token}"
logger.info("Bitbucket | Headers were updated")
@classmethod
async def get_aiohttp_client(
cls, config: SomeConfig
) -> ClientSession:
headers = cls._get_authorization_headers(config=config)
if cls.aiohttp_client is None:
cls.aiohttp_client = cls._get_aiohttp_client(headers=headers)
cls.update_headers_periodically = Periodic(
func=lambda: cls._update_authorization_header(
key=config.key, secret=config.secret
),
time=3600,
)
await cls.update_headers_periodically.start()
return cls.aiohttp_client @faph, have you implemented this part? |
Looks overly complicated to me. Why not just something as simple as:
|
Another use case we ended up with at work, is wanting to log every request made to an API (a bizarre client requirement). This also highlighted to me that a middleware might be recursive. I think we'd probably just want to document that and require users to implement whitelist/blacklist for these use cases. e.g. That use case might look something like:
|
@Dreamsorcerer @faph Want to add a case that's probably in the same theme - #10071 |
Is your feature request related to a problem?
See #6908 for discussion with @Dreamsorcerer
We are have a continuously running service that makes HTTP requests using
aiohttp
's client. These requests are sent with a bearer token in the authorization header. This token needs to be refreshed from time to time.Therefore we cannot set the header on the client object. We need to evaluate the header prior to making individual requests. This is cumbersome for an app developer.
Describe the solution you'd like
We would like to configure the client with the required data which can be used at each request to populate an authorization header. In its simplest form, this could be a simple co-routine that evaluates a token cache, generates a new token if required, and populate the header as required.
For the avoidance of doubt, the proposal is for wherever
aiohttp
supports anauth
argument (client session, request), it would support the "enhanced" auth handler.Currently,
aiohttp
supports aBasicAuth
object which is a customized namedtuple (fields: username, password) with anencode
method which returns the base64 encoded header value. Runtime checks are performed ensuring theauth
value is an instance of that class.TODO: expand on preferred optionSee options in comment below.Describe alternatives you've considered
Both the
requests
library and thehttpx
library have similar functionality. For reference:The
httpx
approach supports a more defined base class/interface for an auth handler, whereasrequests
just requires a callable. Fundamentally they do the same thing and inject an authorization header in the request headers.Related component
Client
Additional context
I am probably able to contribute a PR with an implementation, depending on the agreed scope of the changes.
Code of Conduct
The text was updated successfully, but these errors were encountered: