-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement automatic OAuth token renewal #63
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
Conversation
- Add refreshAccessToken() to refresh expired tokens using refresh token - Update getToken()/getUserOctokit() to auto-refresh before returning - Extract deleteUserToken() helper (used in 3 places) - Add OAUTH_TOKEN_REFRESH_BUFFER_MS constant (5min proactive refresh) - GitHub access tokens expire after 8hrs, refresh tokens after 6 months
WalkthroughA new constant Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/github-oauth-service.ts (1)
417-421: Consider deleting stale tokens when refresh token is expired.When the refresh token is expired, the user cannot authenticate until they re-link their account. Leaving the stale access token in the database may cause confusion (e.g.,
getUserTokenreturns data but it's unusable). Consider cleaning up:// Check if refresh token itself is expired if (refreshTokenExpiresAt && new Date() >= refreshTokenExpiresAt) { console.log(`[OAuth] Refresh token expired for user ${townsUserId}`); + await this.deleteUserToken(townsUserId); return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/constants.ts(1 hunks)src/services/github-oauth-service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use<@{userId}>for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)
Files:
src/constants.tssrc/services/github-oauth-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)
Files:
src/constants.tssrc/services/github-oauth-service.ts
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-42)src/constants.ts (1)
OAUTH_TOKEN_REFRESH_BUFFER_MS(58-58)
🔇 Additional comments (4)
src/constants.ts (1)
52-58: LGTM!The constant is well-documented, and a 5-minute buffer is a sensible choice for proactive token refresh before the 8-hour expiration.
src/services/github-oauth-service.ts (3)
284-299: LGTM!The automatic token refresh flow is clean: check existence → check expiration → refresh if needed. The logging aids debugging token lifecycle issues.
388-397: Good refactoring!Centralizing token deletion into a helper method reduces duplication and ensures consistent cleanup behavior across disconnect, validation failure, and refresh failure paths.
458-470: LGTM!The expiration check logic is sound, and the null handling is appropriately documented. Using the buffer constant ensures proactive refresh before actual expiration.
Prevent race condition where concurrent requests detecting an expired token could all attempt refresh. Since GitHub refresh tokens are single-use, subsequent requests would fail and delete the valid new token from the first request. Uses in-flight promise deduplication per user to serialize refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/github-oauth-service.ts (1)
474-481: Consider distinguishing transient vs. permanent errors before token deletion.Currently, all refresh errors trigger token deletion (line 479). This includes transient network failures where the refresh token may still be valid on GitHub's side. Consider checking the error status code and only deleting on 400/401 errors that indicate the token is definitively invalid.
} catch (error) { console.error( `[OAuth] Failed to refresh token for user ${townsUserId}:`, error ); - await this.deleteUserToken(townsUserId); + // Only delete token if it's definitively invalid (not transient errors) + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const status = (error as any)?.status; + if (status === 400 || status === 401) { + await this.deleteUserToken(townsUserId); + } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/github-oauth-service.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use<@{userId}>for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)
Files:
src/services/github-oauth-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)
Files:
src/services/github-oauth-service.ts
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-42)src/constants.ts (1)
OAUTH_TOKEN_REFRESH_BUFFER_MS(58-58)
🔇 Additional comments (7)
src/services/github-oauth-service.ts (7)
72-73: Good implementation of in-flight refresh deduplication.This Map-based approach correctly prevents race conditions when multiple concurrent requests detect an expired token simultaneously. The pattern ensures only the first request performs the refresh while others await the same promise.
286-301: Clean implementation of automatic token refresh in getToken().The flow is logical: check for existing token, verify expiration, refresh if needed. The logging provides good observability for debugging token lifecycle issues.
309-317: LGTM!Delegating to
getToken()simplifies this method while ensuring tokens are automatically refreshed before creating the Octokit instance.
390-399: Good extraction of deleteUserToken helper.Centralizing the token deletion logic improves maintainability and ensures consistent behavior across
disconnect(),validateToken(), anddoRefreshAccessToken().
409-426: Proper implementation of in-flight request deduplication.The try/finally pattern ensures the promise is always removed from the Map, preventing memory leaks or stale entries. This correctly addresses the concurrent refresh race condition.
484-496: LGTM!The buffer-based expiration check provides proactive token refresh before actual expiration, improving user experience by avoiding failed requests due to expired tokens.
341-341: LGTM!Consistent use of the
deleteUserToken()helper.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.