Rework token acquire/refresh logic #282
Conversation
WalkthroughThe pull request introduces significant modifications to the authentication mechanism in the Polestar API implementation. The changes focus on enhancing token management within the Changes
Sequence DiagramsequenceDiagram
participant Client
participant PolestarAuth
participant AuthService
Client->>PolestarAuth: get_token(force=False)
alt Token is valid
PolestarAuth-->>Client: Return existing token
else Token needs refresh
PolestarAuth->>AuthService: Attempt token refresh
alt Refresh successful
AuthService-->>PolestarAuth: New token
PolestarAuth-->>Client: Return new token
else Refresh fails
PolestarAuth->>AuthService: Request new authorization code
AuthService-->>PolestarAuth: New token
PolestarAuth-->>Client: Return new token
end
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
custom_components/polestar_api/pypolestar/auth.py (7)
104-105: Clarify usage of newforceparameter in docstring
The method docstring states "Ensure we have a valid access token," but does not specify what happens whenforce=True. It may be helpful to explicitly explain howforceimpacts token acquisition, particularly regarding forceful retrieval.
108-111: Reuse existingis_token_valid()check
These conditions partially repeat the logic inis_token_valid(). Consider usingif not force and self.is_token_valid(): …to reduce duplication and streamline code.
129-131: Include underlying exception info in error message
When re-raisingPolestarAuthException, consider adding context about the original exception to clarify why_authorization_codefailed.-except PolestarAuthException as exc: - raise PolestarAuthException("Unable to acquire initial token") from exc +except PolestarAuthException as exc: + raise PolestarAuthException( + f"Unable to acquire initial token, reason: {exc}" + ) from exc
132-150: Handle missing token fields inpayload
Currently, there is an assumption thataccess_token,refresh_token, andexpires_inare always present. Consider adding defensive checks to avoidKeyErrorif the OIDC provider returns an unexpected response.
151-167: Avoid duplication between_authorization_codeand_token_refresh
Much of the underlying logic around POSTing to the token endpoint overlaps. Consider unifying these calls into a shared method that accepts different parameters for clarity and maintainability.
173-177: Assess logging
The debug statement is valuable for tracing. This HTTP request can reveal sensitive data, so ensure that only non-sensitive parts of the request are logged if you ever expand logging.
179-196: Extend refresh request error handling
Similar to_authorization_code(), you might benefit from more robust error handling, especially around partial or malformed responses from the token endpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/polestar_api/pypolestar/auth.py(1 hunks)custom_components/polestar_api/pypolestar/polestar.py(1 hunks)
🔇 Additional comments (3)
custom_components/polestar_api/pypolestar/auth.py (2)
113-114: Confirm short-circuit logic
Logging a message and returning early if the token is valid is appropriate and appears consistent with the new parameters.
116-127: Fallback from token refresh to authorization code
Gracefully falling back to the authorization-code path if refresh fails is a good strategy. Just ensure the warning message is sufficiently clear for debugging token refresh issues.custom_components/polestar_api/pypolestar/polestar.py (1)
200-200: Consider explicitforceusage
Switching fromawait self.auth.get_token(refresh=True)toawait self.auth.get_token()aligns with the new method signature. If an unconditional refresh is required, consider callingawait self.auth.get_token(refresh=True, force=True)to ensure the token always updates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
custom_components/polestar_api/pypolestar/auth.py (5)
104-130: LGTM! Consider enhancing error handling specificity.The reworked token management flow with the new
forceparameter is well-structured and handles the token lifecycle effectively. The approach of attempting refresh before falling back to authorization code is robust.Consider making the error handling more specific by:
- Using a custom exception for refresh failures
- Adding more context to the error message in line 122
- "Failed to refresh token, retry with code", exc_info=exc + "Failed to refresh token (status=%s), falling back to authorization code", + getattr(exc, 'status_code', 'unknown'), + exc_info=exc
Line range hint
132-154: LGTM! Consider adding token validation.The centralized token response parsing is well-implemented with proper error handling and state management.
Consider adding basic validation for the token values:
try: self.access_token = payload["access_token"] + if not isinstance(self.access_token, str) or not self.access_token.strip(): + raise ValueError("Invalid access token format") self.refresh_token = payload["refresh_token"] self.token_lifetime = payload["expires_in"] + if not isinstance(self.token_lifetime, (int, float)) or self.token_lifetime <= 0: + raise ValueError("Invalid token lifetime") self.token_expiry = datetime.now(tz=timezone.utc) + timedelta( seconds=self.token_lifetime )
156-184: LGTM! Consider adding request validation.The implementation of the authorization code flow with PKCE is secure and well-structured.
Consider validating the token endpoint URL before making the request:
+ if not self.oidc_configuration.get("token_endpoint", "").startswith(("https://", "http://localhost")): + raise PolestarAuthException("Invalid token endpoint URL") + response = await self.client_session.post( self.oidc_configuration["token_endpoint"], data=token_request, timeout=HTTPX_TIMEOUT, )
186-206: LGTM! Consider adding retry mechanism.The token refresh implementation is clean and follows good practices by reusing the common token response parsing logic.
Consider adding a retry mechanism for transient failures:
+ async def _token_refresh(self, max_retries: int = 2) -> None: """Refresh existing token.""" + last_exception = None + for attempt in range(max_retries): + try: token_request = { "grant_type": "refresh_token", "client_id": OIDC_CLIENT_ID, "refresh_token": self.refresh_token, } self.logger.debug( "Call token endpoint with grant_type=%s", token_request["grant_type"] ) response = await self.client_session.post( self.oidc_configuration["token_endpoint"], data=token_request, timeout=HTTPX_TIMEOUT, ) response.raise_for_status() self._parse_token_response(response) + return + except httpx.TransportError as exc: + last_exception = exc + if attempt < max_retries - 1: + self.logger.warning("Retrying token refresh after transport error", exc_info=exc) + continue + if last_exception: + raise PolestarAuthException("Failed to refresh token after retries") from last_exception
Line range hint
104-206: Consider implementing token persistence.The token management implementation is robust and well-structured. To further enhance the system's reliability and user experience, consider implementing token persistence to maintain sessions across restarts.
Key considerations for token persistence:
- Secure storage of refresh tokens
- Handling of expired persisted tokens
- Migration strategy for existing sessions
Would you like me to provide a detailed design proposal for implementing secure token persistence?
Summary by CodeRabbit
Authentication Improvements
forceparameter.Code Quality