Add support for refreshing the oauth token #6
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds OAuth token refresh capability to the Linear integration, enabling automatic renewal of expired OAuth tokens without user intervention.
Key Changes:
- New OAuth token refresh module with functions to check token validity and refresh expired tokens
- Enhanced API client to detect authentication errors and retry requests with refreshed tokens
- Integration of token refresh callback into the main setup flow for OAuth-authenticated entries
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
custom_components/integration_linear/oauth.py |
New module providing async functions for token validation and refresh using Home Assistant's OAuth2 implementation |
custom_components/integration_linear/api.py |
Updated API client to accept optional token refresh callback and handle auth errors with automatic retry after token refresh |
custom_components/integration_linear/__init__.py |
Modified setup to create and pass token refresh callback to API client for OAuth entries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expires_at = token.get("expires_at", 0) | ||
| if expires_at and time.time() >= (expires_at - 60): | ||
| # Token is expired or about to expire, refresh it | ||
| LOGGER.debug("Token expired or about to expire, refreshing") |
There was a problem hiding this comment.
The condition if expires_at and time.time() >= (expires_at - 60) treats expires_at = 0 as falsy, which means if a token has no expiration or expires_at is missing/0, the function returns access_token without attempting refresh even if it's invalid. Consider handling the case where expires_at is 0 explicitly, or checking if the token has required fields before returning.
| expires_at = token.get("expires_at", 0) | |
| if expires_at and time.time() >= (expires_at - 60): | |
| # Token is expired or about to expire, refresh it | |
| LOGGER.debug("Token expired or about to expire, refreshing") | |
| expires_at = token.get("expires_at") | |
| # If expires_at is missing or 0, treat token as expired and refresh | |
| if not expires_at or time.time() >= (expires_at - 60): | |
| LOGGER.debug("Token expired, missing expiration, or about to expire, refreshing") |
| break | ||
|
|
||
| # Try to refresh token if we have a callback and this is an auth error | ||
| if is_auth_error and retry_on_auth_error and self._token_refresh_callback and not self._refresh_in_progress: |
There was a problem hiding this comment.
The _refresh_in_progress flag provides basic protection against concurrent refresh attempts but is not thread-safe. If multiple async tasks call _api_wrapper simultaneously with auth errors, they could all pass the check before any sets the flag to True, leading to multiple concurrent refresh attempts. Consider using asyncio.Lock instead of a boolean flag.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| self._api_token = new_token | ||
| # Create new headers dict with updated token | ||
| retry_headers = dict(headers) if headers else {} | ||
| retry_headers["Authorization"] = new_token |
There was a problem hiding this comment.
The _api_token is updated on line 602, but the retry request uses retry_headers['Authorization'] = new_token. However, subsequent requests will use self._api_token which should already be updated. Ensure consistency - if the token format requires a prefix like 'Bearer', it should be applied here as well. Verify that new_token has the same format as the original self._api_token.
| retry_headers["Authorization"] = new_token | |
| retry_headers["Authorization"] = f"Bearer {new_token}" |
| url: str, | ||
| data: dict | None = None, | ||
| headers: dict | None = None, | ||
| retry_on_auth_error: bool = False, |
There was a problem hiding this comment.
The retry_on_auth_error parameter defaults to False and is only set to True for _graphql_query calls. This means that any direct calls to _api_wrapper won't benefit from automatic token refresh. Consider whether this default should be True for OAuth-enabled clients, or document why GraphQL is the only method that needs this feature.
| retry_on_auth_error: bool = False, | |
| retry_on_auth_error: bool = True, |
|
|
||
| # Create token refresh callback for OAuth | ||
| async def refresh_token() -> str: | ||
| """Refresh OAuth token and return new access token.""" | ||
| return await async_get_valid_token(hass, entry) | ||
|
|
There was a problem hiding this comment.
The refresh_token callback calls async_get_valid_token, which internally calls async_refresh_token when the token is expired. However, async_get_valid_token also checks expiration and may return the existing token if not expired. When called from _api_wrapper due to an auth error, the token might not be marked as expired yet (within 60 seconds), so it could return the same invalid token. Consider calling async_refresh_token directly in the callback or ensuring the auth error forces a refresh regardless of expiration time.
| except Exception as refresh_exception: | ||
| LOGGER.error("Token refresh failed: %s", refresh_exception) | ||
| _raise_authentication_error() |
There was a problem hiding this comment.
The error logged on line 617 includes the exception details, but _raise_authentication_error() on line 618 likely raises a generic authentication error without preserving the original exception context. Consider passing the refresh_exception as the cause when raising the authentication error to maintain the full error chain for debugging.
solves #5