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

Improve docs around adding authorization header mechanism able to refresh tokens #552

Open
lossyrob opened this issue Jun 26, 2023 · 5 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed

Comments

@lossyrob
Copy link
Member

Currently the way to add an Authorization header to a Client is to add a static token in the headers parameter. This works, however if the token can expire, the user has to stay on top of checking token expiration any time they want to use the client and call client._stac_io.update(headers=...).

I propose we add an authorization parameter to Client and StacApiIO that can either take a string or a function. This parameter would provide the Authorization: header for each request. If it's a string, just use that string - this would be the same as supplying it in the headers parameter, but would take precedence over any Authorization key in the headers. If the value were a function, it would call that function to get the value of the Authorization header for each request made by StacApiIO. That way, I could write a function that checks the expiration of the token and uses a new one in case it's about to expire. E.g.:

import os
import time
from typing import Optional

from azure.identity import DefaultAzureCredential
from azure.core.credentials import AccessToken
import pystac_client

class TokenProvider:
    _token: Optional[AccessToken]

    def __init__(self, app_id: str):
        self.app_id = app_id
        self._credential = DefaultAzureCredential()
        self._token = None
        
    def get_token(self) -> str:
        if self._token is None or self._token.expires_on < time.time() - 5:
            self._token = self._credential.get_token(self.app_id)
        return f"Bearer {self._token}"
        
tp = TokenProvider(os.environ["APP_ID"])

client = pystac_client.Client.open("https://foo.westeurope.cloudapp.azure.com/stac", authorization=tp.get_token)
@gadomski gadomski added the enhancement New feature or request label Jun 26, 2023
@TomAugspurger
Copy link
Collaborator

Do you have thoughts on how this would compare to something like the azure SDK's credential parameter? They're very similar (accept either a string or a callable that gets you a token). I think the primary difference is that azure's setup has one more layer: a TokenCredential subclass has a get_token method. I guess the upshot of that is to let you reuse TokenCredential instances across different services, which doesn't really apply here?

https://learn.microsoft.com/en-us/python/api/overview/azure/identity-readme?view=azure-python has some background.

@lossyrob
Copy link
Member Author

It's a similar pattern. I think we could get away with a callable here, and then have auth implementation define a class that serves as a provider, either giving by giving users examples to copy in or publishing something that you can easily pass to a Client/passes back a configured Client.

@gadomski
Copy link
Member

I'm not opposed to the idea outright, but I am getting a little bit of a twitch about adding another "tweak behavior" attribute to StacIO and/or Client. We've already got modifier and request_modifier, so at minimum this will make it harder to discover what the right way to go is when doing request customization. If we do add authorization, we'll need to doc it out really well, w/ examples, to show when you'd want to use each different customization attribute.

As an alternative, I wonder if there's some refactoring we can do of StacIO and the actual request methods to expose the right hooks for a StacIO subclass to do the job. E.g. something like:

class PlanetaryComputerStacIO(StacApiIO):
    def update_headers(self, headers: Dict[str, Any]):
        # This overrides a no-op superclass method
        ...

client = Client.open("https://planetarycomputer.microsoft.com/api/stac/v1", stac_io=PlanetaryComputerStacIO())

@TomAugspurger
Copy link
Collaborator

As an alternative, I wonder if there's some refactoring we can do of StacIO and the actual request methods to expose the right hooks for a StacIO subclass to do the job. E.g. something like:

I'll keep linking to https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/core/azure-core/CLIENT_LIBRARY_DEVELOPER.md on this topic :)

I think you really do need that "chained policy" concept for these things to compose well together and we can maybe get some code-reuse for the different authorization mechanisms.

As for this feature request: since we're already exposing so many of the details of the I/O currently that it's not much worse to add one more.

@lossyrob
Copy link
Member Author

I actually didn't catch request_modifier, which can be used for this purpose. E.g.:

from requests import Request

def req_modifier(req: Request) -> Request:
    req.headers["Authorization"] = f"Bearer {tp.get_token().token}"
    return req

client = pystac_client.Client.open("https://foo.westeurope.cloudapp.azure.com/stac", request_modifier=req_modifier)

Works with the above definition of tp.

I'm fine with leveraging this feature rather than adding a new authentication parameter. An example of implementing a token with an expiry would be good to include in the docs either/both in the Authentication tutorial and in the Client usage docs, where the latter could be more general information about request_modifier with Auth as an example. But TBH I skipped the docs and was just reading code, and missed this parameter, which to be fair says in the docstring that request_modifier can be used for Auth.

Unless we think a more top-level authorization parameter is a better idea than just better documenting the request_modifier for this use case, I'd suggest reorient this Issue to be about the documentation of request_modifier.

@gadomski gadomski added documentation Improvements or additions to documentation help wanted Extra attention is needed labels Jun 27, 2023
@gadomski gadomski changed the title Add authorization header mechanism able to refresh tokens Improve docs around adding authorization header mechanism able to refresh tokens Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants