Use refreshable app tokens #187
Conversation
heyitsaamir
requested review from
MehakBindra,
Copilot and
lilyydu
and removed request for
Copilot and
lilyydu
There was a problem hiding this comment.
Pull Request Overview
This PR implements refreshable app tokens to fix issue #184 where tokens weren't being refreshed after an hour. The implementation follows TypeScript's approach by providing a token factory that automatically refreshes expired tokens on use, extending to proactive scenarios as well.
Key Changes:
- Consolidated token management from separate bot and graph token managers into a unified
TokenManager - Changed
app.idto use credentials instead of tokens, removingapp.nameproperty (breaking change) - Token passing now uses async factory functions that check expiration and refresh as needed
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/apps/src/microsoft/teams/apps/token_manager.py | New unified token manager handling bot, graph, and user tokens with automatic refresh logic |
| packages/apps/src/microsoft/teams/apps/routing/activity_context.py | Updated to use Token type and removed null check for app_token since it's now always a factory function |
| packages/apps/src/microsoft/teams/apps/http_plugin.py | Updated type annotations to use Token and removed separate graph_token parameter |
| packages/apps/src/microsoft/teams/apps/graph_token_manager.py | Removed in favor of unified TokenManager |
| packages/apps/src/microsoft/teams/apps/app_tokens.py | Removed dataclass in favor of TokenManager-based approach |
| packages/apps/src/microsoft/teams/apps/app_process.py | Refactored to use TokenManager methods and pass token factories to context |
| packages/apps/src/microsoft/teams/apps/app.py | Major refactoring removing token refresh logic, app.name property, and integrating TokenManager throughout |
| self.container.set_provider("name", providers.Object(self.name)) | ||
| self.container.set_provider("credentials", providers.Object(self.credentials)) | ||
| self.container.set_provider("bot_token", providers.Callable(lambda: self.tokens.bot)) | ||
| self.container.set_provider("graph_token", providers.Callable(lambda: self.tokens.graph)) |
There was a problem hiding this comment.
this wasn't being used anywhere, so removing until we need it
| self.container.set_provider("credentials", providers.Object(self.credentials)) | ||
| self.container.set_provider("bot_token", providers.Callable(lambda: self.tokens.bot)) | ||
| self.container.set_provider("graph_token", providers.Callable(lambda: self.tokens.graph)) | ||
| self.container.set_provider("bot_token", providers.Factory(self._get_or_refresh_bot_token)) |
There was a problem hiding this comment.
no longer using a cached token
| self.api = ApiClient( | ||
| "https://smba.trafficmanager.net/teams", | ||
| self.http_client.clone(ClientOptions(token=lambda: self.tokens.bot)), | ||
| self.http_client.clone(ClientOptions(token=self._get_or_refresh_bot_token)), |
There was a problem hiding this comment.
no longer using a cached token
| """The app's ID from credentials.""" | ||
| if not self.credentials: | ||
| return None | ||
| return self.credentials.client_id |
There was a problem hiding this comment.
using the credentials now, now that we are no longer refreshing the token on app-start
| channel_id="msteams", | ||
| service_url=self.api.service_url, | ||
| bot=Account(id=self.id, name=self.name, role="bot"), | ||
| bot=Account(id=self.id, role="bot"), |
There was a problem hiding this comment.
don't have access to "name", and I don't think it's actually useful
|
|
||
| return None | ||
|
|
||
| async def _refresh_tokens(self, force: bool = False) -> None: |
There was a problem hiding this comment.
refresh token logic is all in token manager now
| ) | ||
| api_client = ApiClient(service_url, self.http_client.clone(ClientOptions(token=self.tokens.bot))) | ||
| api_client = ApiClient( | ||
| service_url, self.http_client.clone(ClientOptions(token=self.token_manager.refresh_bot_token)) |
There was a problem hiding this comment.
passing in a factory for the token which can be refreshed if needed.
Context
This fixes #184 which stated that the token wasn't being refreshed after an hour. This was because we weren't refreshing it on use.
In typescript, we refresh the token on every app.process. But this actually still leaves proactive scenarios to used an cached, potentially expired, tokens. To remediate this, the token that gets passed to the API is a factory which refreshes the token if the cached token is expired.
For the record C# sets the token value to be refreshable (just as this PR is attempting to do).
Changes
This PR includes several changes
Testing
PR Dependency Tree
This tree was auto-generated by Charcoal