Bugfix/devexp 765 syncronize token managers

CHANGELOG.md

@@ -21,6 +21,10 @@ All notable changes to the **Sinch Python SDK** are documented in this file.
 ### SDK
 
 - **[dependency]** Set up minimum version for `requests` to `>=2.0.0` to prevent pulling in versions with known vulnerabilities (#152).
+- **[fix]** Fixed a race condition in OAuth token creation and renewal under concurrent requests: `TokenManagerBase` now uses a lock with double-checked locking so the initial token is fetched exactly once, and a new `refresh_auth_token(used_token)` deduplicates concurrent renewals by only fetching when the stale token still matches the cached one (#156).
+- **[refactor]** `HTTPTransport` now prepares and authenticates requests in `request()`, so the new `send_request(request_data)` receives an already-prepared `HttpRequest` and acts as a pure I/O primitive, simplifying subclassing (#156).
+- **[deprecation notice]** `HTTPTransport.send(endpoint)` is deprecated in favour of `send_request(request_data)`; the legacy method still works for backward compatibility, but will be removed in 3.0 (#156).
+- **[deprecation notice]** `TokenManagerBase.invalidate_expired_token()` and `handle_invalid_token()` (and the `TokenState.EXPIRED` value) are deprecated and will be removed in 3.0, as token renewal now goes through `refresh_auth_token()` (#156).
 
 
 ### SMS

README.md

@@ -193,7 +193,7 @@ The following example replaces the default `requests` backend with `httpx` and r
 import httpx
 from sinch import SinchClient
 from sinch.core.ports.http_transport import HTTPTransport
-from sinch.core.endpoint import HTTPEndpoint
+from sinch.core.models.http_request import HttpRequest
 from sinch.core.models.http_response import HTTPResponse
 
 
@@ -204,9 +204,7 @@ class MyHTTPImplementation(HTTPTransport):
             proxy=f"http://{proxy_user}:{proxy_password}@{proxy_url}"
         )
 
-    def send(self, endpoint: HTTPEndpoint) -> HTTPResponse:
-        request_data = self.prepare_request(endpoint)
-        request_data = self.authenticate(endpoint, request_data)
+    def send_request(self, request_data: HttpRequest) -> HTTPResponse:
 
         body = request_data.request_body
         response = self.http_client.request(

sinch/core/adapters/requests_http_transport.py

@@ -1,21 +1,28 @@
 import requests
 from sinch.core.ports.http_transport import HTTPTransport, HttpRequest
-from sinch.core.endpoint import HTTPEndpoint
 from sinch.core.models.http_response import HTTPResponse
 
-
 class HTTPTransportRequests(HTTPTransport):
+    """
+    Sync HTTP transport using the requests library.
+    """
+
     def __init__(self, sinch):
         super().__init__(sinch)
         self.http_session = requests.Session()
 
-    def send(self, endpoint: HTTPEndpoint) -> HTTPResponse:
-        request_data: HttpRequest = self.prepare_request(endpoint)
-        request_data: HttpRequest = self.authenticate(endpoint, request_data)
+    def send_request(self, request_data: HttpRequest) -> HTTPResponse:
+        """
+        Performs the HTTP call with requests and maps the result to an HTTPResponse.
 
+        :param request_data: The prepared request to send.
+        :type request_data: HttpRequest
+        :returns: The HTTP response.
+        :rtype: HTTPResponse
+        """
         self.sinch.configuration.logger.debug(
-            f"Sync HTTP {request_data.http_method} call with headers:"
-            f" {request_data.headers}, body: {request_data.request_body} and query_params: {request_data.query_params} to URL: {request_data.url}"
+            "Sync HTTP request %s call with headers: %s and body: %s to URL: %s",
+            request_data.http_method, request_data.headers, request_data.request_body, request_data.url
         )
         response = self.http_session.request(
             method=request_data.http_method,
@@ -30,8 +37,8 @@ def send(self, endpoint: HTTPEndpoint) -> HTTPResponse:
         response_body = self.deserialize_json_response(response)
 
         self.sinch.configuration.logger.debug(
-            f"Sync HTTP {response.status_code} response with headers: {response.headers}"
-            f"and body: {response_body} from URL: {request_data.url}"
+            "Sync HTTP response %s with headers: %s and body: %s from URL: %s",
+            response.status_code, response.headers, response_body, request_data.url
         )
 
         return HTTPResponse(

sinch/core/ports/http_transport.py

@@ -1,70 +1,119 @@
-from abc import ABC, abstractmethod
+import warnings
+from abc import ABC
 from platform import python_version
+from typing import Optional
+
+from requests import Response
 from sinch.core.endpoint import HTTPEndpoint
 from sinch.core.models.http_request import HttpRequest
 from sinch.core.models.http_response import HTTPResponse
 from sinch.core.exceptions import ValidationException, SinchException
 from sinch.core.enums import HTTPAuthentication
-from sinch.core.token_manager import TokenState
 from sinch import __version__ as sdk_version
 
 
 class HTTPTransport(ABC):
-    """Base class for HTTP transports.
+    """
+    Base class for HTTP transports.
 
-    Subclasses implement ``send`` to perform the raw HTTP call.
-    The public ``request`` method adds cross-cutting concerns on top:
-    authentication, logging hooks, and automatic token refresh on 401.
+    Subclasses implement :meth:`send_request` to perform the raw HTTP call. The public
+    :meth:`request` method adds cross-cutting concerns on top: request
+    preparation, authentication, and automatic token refresh on 401.
+
+    .. deprecated:: 2.1
+        Overriding :meth:`send` (the old ``send(endpoint)`` hook) is still
+        honored but deprecated; implement :meth:`send_request` instead. The
+        ``send`` override path will be removed in 3.0.
     """
 
     def __init__(self, sinch):
         self.sinch = sinch
+        self._legacy_send = self._uses_legacy_send()
+        if self._legacy_send:
+            warnings.warn(
+                f"{type(self).__name__} overrides `send(endpoint)`, which is deprecated and "
+                "will be removed in 3.0. Implement `send_request(request_data)` instead.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
 
-    # ------------------------------------------------------------------
-    # Subclass contract
-    # ------------------------------------------------------------------
+    def send_request(self, request_data: HttpRequest) -> HTTPResponse:
+        """
+        Performs a single HTTP round-trip for an already-prepared, authenticated request.
 
-    @abstractmethod
+        :param request_data: The prepared request to send.
+        :type request_data: HttpRequest
+        :returns: The HTTP response.
+        :rtype: HTTPResponse
+        """
+        raise NotImplementedError(
+            "Transport subclasses must implement `send_request(request_data)`."
+        )
+
     def send(self, endpoint: HTTPEndpoint) -> HTTPResponse:
-        """Execute a single HTTP round-trip and return the response.
+        """
+        Prepares, authenticates and performs a single round-trip for an endpoint.
+
+        .. deprecated:: 2.1
+            This hook is deprecated. Implement :meth:`send_request` instead;
+            the ``send`` override path will be removed in 3.0.
 
-        Implementations must prepare the request, authenticate, perform the
-        HTTP call, deserialize the response, and return an ``HTTPResponse``.
-        They should **not** handle token refresh — that is done by
-        ``request``.
+        :param endpoint: The endpoint to call.
+        :type endpoint: HTTPEndpoint
+        :returns: The HTTP response.
+        :rtype: HTTPResponse
+        """
+        raise NotImplementedError(
+            "`send(endpoint)` is deprecated. "
+            "Implement `send_request(request_data)` instead."
+        )
+
+    def _uses_legacy_send(self) -> bool:
+        """
+        Returns True when a subclass overrides the deprecated ``send`` hook but
+        not the new ``send_request`` hook.
         """
+        cls = type(self)
+        return cls.send is not HTTPTransport.send and cls.send_request is HTTPTransport.send_request
 
-    # ------------------------------------------------------------------
-    # Public API
-    # ------------------------------------------------------------------
 
     def request(self, endpoint: HTTPEndpoint) -> HTTPResponse:
-        """Send a request with automatic OAuth token refresh on 401.
+        """
+        Sends a request, renewing the token and retrying once on an expired-token 401.
 
-        If the server responds with 401 *and* the token is detected as
-        expired, the token is invalidated and **one** retry is attempted
-        with a fresh token.  A second consecutive 401 is handed straight
-        to the endpoint's error handler — no further retries.
+        :param endpoint: The endpoint to call.
+        :type endpoint: HTTPEndpoint
+        :returns: The handled HTTP response.
+        :rtype: HTTPResponse
         """
-        http_response = self.send(endpoint)
+        if self._legacy_send:
+            return self._legacy_request(endpoint)
+
+        request_data = self.prepare_request(endpoint)
+        request_data = self.authenticate(endpoint, request_data)
+        http_response = self.send_request(request_data)
 
         if self._should_refresh_token(endpoint, http_response):
-            self.sinch.configuration.token_manager.handle_invalid_token(
-                http_response
-            )
-            if (
-                self.sinch.configuration.token_manager.token_state
-                == TokenState.EXPIRED
-            ):
-                http_response = self.send(endpoint)
+            used_token = self._get_bearer_token_from_request(request_data)
+            new_token = self.sinch.configuration.token_manager.refresh_auth_token(used_token)
+            self._set_bearer_token(request_data, new_token.access_token)
+            http_response = self.send_request(request_data)
 
         return endpoint.handle_response(http_response)
 
-    # ------------------------------------------------------------------
-    # Internals
-    # ------------------------------------------------------------------
 
-    def authenticate(self, endpoint, request_data):
+    def authenticate(self, endpoint: HTTPEndpoint, request_data: HttpRequest) -> HttpRequest:
+        """
+        Stamps the credentials required by the endpoint's auth scheme onto the request.
+
+        :param endpoint: The endpoint being called, whose HTTP_AUTHENTICATION selects the scheme.
+        :type endpoint: HTTPEndpoint
+        :param request_data: The request to authenticate, mutated in place.
+        :type request_data: HttpRequest
+        :returns: The same request, with auth applied.
+        :rtype: HttpRequest
+        :raises ValidationException: If the credentials required by the scheme are missing.
+        """
         if endpoint.HTTP_AUTHENTICATION in (HTTPAuthentication.BASIC.value, HTTPAuthentication.OAUTH.value):
             if (
                 not self.sinch.configuration.key_id
@@ -87,10 +136,7 @@ def authenticate(self, endpoint, request_data):
 
         if endpoint.HTTP_AUTHENTICATION == HTTPAuthentication.OAUTH.value:
             token = self.sinch.authentication.get_auth_token().access_token
-            request_data.headers.update({
-                "Authorization": f"Bearer {token}",
-                "Content-Type": "application/json"
-            })
+            self._set_bearer_token(request_data, token)
         elif endpoint.HTTP_AUTHENTICATION == HTTPAuthentication.SMS_TOKEN.value:
             if not self.sinch.configuration.sms_api_token or not self.sinch.configuration.service_plan_id:
                 raise ValidationException(
@@ -101,14 +147,19 @@ def authenticate(self, endpoint, request_data):
                     is_from_server=False,
                     response=None
                 )
-            request_data.headers.update({
-                "Authorization": f"Bearer {self.sinch.configuration.sms_api_token}",
-                "Content-Type": "application/json"
-            })
+            self._set_bearer_token(request_data, self.sinch.configuration.sms_api_token)
 
         return request_data
 
     def prepare_request(self, endpoint: HTTPEndpoint) -> HttpRequest:
+        """
+        Builds the HttpRequest for an endpoint.
+
+        :param endpoint: The endpoint to build the request for.
+        :type endpoint: HTTPEndpoint
+        :returns: The prepared request.
+        :rtype: HttpRequest
+        """
         url_query_params = endpoint.build_query_params()
 
         return HttpRequest(
@@ -124,7 +175,16 @@ def prepare_request(self, endpoint: HTTPEndpoint) -> HttpRequest:
         )
 
     @staticmethod
-    def deserialize_json_response(response):
+    def deserialize_json_response(response: Response) -> dict:
+        """
+        Parses the JSON body of a response.
+
+        :param response: The raw HTTP response.
+        :type response: Response
+        :returns: The parsed body.
+        :rtype: dict
+        :raises SinchException: If the body is present but not valid JSON.
+        """
         if response.content:
             try:
                 response_body = response.json()
@@ -138,12 +198,75 @@ def deserialize_json_response(response):
             response_body = {}
 
         return response_body
+
+    def _legacy_request(self, endpoint: HTTPEndpoint) -> HTTPResponse:
+        """
+        Backward-compatible request loop for subclasses that override ``send``.
+
+        On an expired-token 401 the cached token is renewed through
+        :meth:`TokenManagerBase.refresh_auth_token`, which dedupes concurrent
+        renewals. The legacy ``send(endpoint)`` re-prepares and re-authenticates
+        on every call, so the second ``send`` picks up the refreshed token from
+        the cache automatically.
+
+        :param endpoint: The endpoint to call.
+        :type endpoint: HTTPEndpoint
+        :returns: The handled HTTP response.
+        :rtype: HTTPResponse
+        """
+        token_before = self.sinch.configuration.token_manager.token
+        http_response = self.send(endpoint)
+
+        if self._should_refresh_token(endpoint, http_response):
+            used_token = token_before.access_token if token_before else None
+            self.sinch.configuration.token_manager.refresh_auth_token(used_token)
+            http_response = self.send(endpoint)
+
+        return endpoint.handle_response(http_response)
 
     @staticmethod
-    def _should_refresh_token(endpoint, http_response):
-        """Return True when a 401 response should trigger a token refresh."""
-        return (
-            http_response.status_code == 401
-            and endpoint.HTTP_AUTHENTICATION
-            == HTTPAuthentication.OAUTH.value
-        )
+    def _should_refresh_token(endpoint: HTTPEndpoint, http_response: HTTPResponse) -> bool:
+        """
+        Returns True for an OAuth endpoint that got a 401 with an expired-token header.
+
+        :param endpoint: The endpoint that was called.
+        :type endpoint: HTTPEndpoint
+        :param http_response: The response received.
+        :type http_response: HTTPResponse
+        :returns: Whether the token should be refreshed and the request retried.
+        :rtype: bool
+        """
+        if endpoint.HTTP_AUTHENTICATION != HTTPAuthentication.OAUTH.value:
+            return False
+        if http_response.status_code != 401:
+            return False
+        www_authenticate = http_response.headers.get("www-authenticate") or ""
+        return "expired" in www_authenticate
+
+    @staticmethod
+    def _get_bearer_token_from_request(request_data: HttpRequest) -> Optional[str]:
+        """
+        Extracts the bearer token from the request's Authorization header.
+
+        :param request_data: The request.
+        :type request_data: HttpRequest
+        :returns: The bearer token, or None if absent or not a bearer.
+        :rtype: Optional[str]
+        """
+        auth = request_data.headers.get("Authorization", "")
+        return auth.removeprefix("Bearer ") if auth.startswith("Bearer ") else None
+
+    @staticmethod
+    def _set_bearer_token(request_data: HttpRequest, token: str) -> None:
+        """
+        Stamps the bearer token onto the request's Authorization header.
+
+        :param request_data: The request.
+        :type request_data: HttpRequest
+        :param token: The bearer token.
+        :type token: str
+        """
+        request_data.headers.update({
+            "Authorization": f"Bearer {token}",
+            "Content-Type": "application/json"
+        })