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

feat: Support Headers in OAuth get token #1811

Closed
s7clarke10 opened this issue Jul 6, 2023 · 2 comments
Closed

feat: Support Headers in OAuth get token #1811

s7clarke10 opened this issue Jul 6, 2023 · 2 comments

Comments

@s7clarke10
Copy link
Contributor

s7clarke10 commented Jul 6, 2023

Feature scope

Taps (catalog, state, stream maps, tests, etc.)

Description

I have a new API to ingest data from which requires OAuth Authentication. This is an unusual API as it also requires an additional header supplied as well to obtain a token. In this example the header in question is a subscription key. It appears that out of the box the SDK doesn't support headers when getting the OAuth token. Do you perceive any issues adding headers to an OAuth Request as per this working code below?

Please Note: The expires_in returned from the API in question came back as a string (quite unusual). For this reason I have added a conversion to an Int to ensure the comparison for expiration works.

It would replace this section

# Authentication and refresh
def update_access_token(self) -> None:
"""Update `access_token` along with: `last_refreshed` and `expires_in`.
Raises:
RuntimeError: When OAuth login fails.
"""
request_time = utc_now()
auth_request_payload = self.oauth_request_payload
token_response = requests.post(
self.auth_endpoint,
data=auth_request_payload,
timeout=60,
)
try:
token_response.raise_for_status()
except requests.HTTPError as ex:
msg = f"Failed OAuth login, response was '{token_response.json()}'. {ex}"
raise RuntimeError(msg) from ex
self.logger.info("OAuth authorization attempt was successful.")
token_json = token_response.json()
self.access_token = token_json["access_token"]
self.expires_in = token_json.get("expires_in", self._default_expiration)
if self.expires_in is None:
self.logger.debug(
"No expires_in receied in OAuth response and no "
"default_expiration set. Token will be treated as if it never "
"expires.",
)
self.last_refreshed = request_time
(my changes are the two commented lines).

    # Authentication and refresh      
    def update_access_token(self) -> None:
        """Update `access_token` along with: `last_refreshed` and `expires_in`.

        Raises:
            RuntimeError: When OAuth login fails.
        """
        request_time = utc_now()
        auth_request_payload = self.oauth_request_payload
        token_response = requests.post(
            self.auth_endpoint,
            headers=self._auth_headers,  # Added support for headers to be passed in
            data=auth_request_payload,
            timeout=60,
        )
        try:
            token_response.raise_for_status()
        except requests.HTTPError as ex:
            raise RuntimeError(
                f"Failed OAuth login, response was '{token_response.json()}'. {ex}",
            ) from ex

        self.logger.info("OAuth authorization attempt was successful.")

        token_json = token_response.json()
        self.access_token = token_json["access_token"]
        self.expires_in = int(token_json.get("expires_in", self._default_expiration))   # Need to set the token as int if it is returned as a string.
        if self.expires_in is None:
            self.logger.debug(
                "No expires_in receied in OAuth response and no "
                "default_expiration set. Token will be treated as if it never "
                "expires.",
            )
        self.last_refreshed = request_time

It would there also make sense if it makes sense to support headers, to have an optional auth_headers parameter when creating an OAuth Token e.g. replacing lines https://github.com/meltano/sdk/blob/77c3f90a0cd20e58daf494ac129905ed64a89944/singer_sdk/authenticators.py#L352C1-L373C42 with this code.

class OAuthAuthenticator(APIAuthenticatorBase):
    """API Authenticator for OAuth 2.0 flows."""

    def __init__(
        self,
        stream: RESTStream,
        auth_endpoint: str | None = None,
        oauth_scopes: str | None = None,
        default_expiration: int | None = None,
        auth_headers: dict | None = None,
    ) -> None:
        """Create a new authenticator.

        Args:
            stream: The stream instance to use with this authenticator.
            auth_endpoint: The OAuth 2.0 authorization endpoint.
            oauth_scopes: A comma-separated list of OAuth scopes.
            default_expiration: Default token expiry in seconds.
        """
        super().__init__(stream=stream)
        self._auth_endpoint = auth_endpoint
        self._default_expiration = default_expiration
        self._oauth_scopes = oauth_scopes
        if self._auth_headers is None:
            self._auth_headers = {}
        if auth_headers:
            self._auth_headers.update(auth_headers)
@s7clarke10 s7clarke10 added kind/Feature New feature or request valuestream/SDK labels Jul 6, 2023
@edgarrmondragon
Copy link
Collaborator

Hi @s7clarke10, thanks for logging!

It'd make sense to accept arbitrary headers to use for the Auth request. I'd probably just suggest calling the parameter oauth_headers to avoid confusion with auth headers used in other requests.

@edgarrmondragon
Copy link
Collaborator

Closed by #1815

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

No branches or pull requests

2 participants