fix: handle OAuth token revocation and improve error handling #54
Conversation
WalkthroughAdds a new module src/api/user-oauth-client.ts exporting typed helpers 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 |
![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 (4)
src/api/user-oauth-client.ts (1)
32-63: Tighten error typing instead of relying onany+ eslint-disableBoth helpers inspect
error.statususing(error as any)?.statusand disableno-unsafe-member-access. Consider keepingerrorasunknownand using a small type guard (e.g., checktypeof error === "object"and"status" in error) so you can drop the eslint-disable and avoidanywhile preserving the same behavior.Also applies to: 72-88
src/services/github-oauth-service.ts (1)
333-369: Token validation and cleanup behavior looks correct; consider minor reuse improvementsThe flow of:
- returning
falseif no token,- deleting the record and returning
falseon 401,- and treating other errors as transient (log +
true)matches the intended behavior and is safe against races. You might optionally reuse existing helpers (e.g., centralize user-scoped client creation) or have validateToken return a richer status (e.g.,
"not-linked" | "invalid" | "valid") to avoid the separateisLinkedcheck in callers, but that’s a nice-to-have.src/services/subscription-service.ts (1)
119-133: Graceful handling of profile lookup errors; consider user-facing messagingWrapping
getUserProfilein try/catch and returning aSubscribeFailureinstead of throwing aligns with the PR’s error-handling goals. One optional refinement: for unexpected/internal errors (e.g., network issues), you might want a more generic user-facing message while logging the detailed error elsewhere, to avoid surfacing low-level details directly to end users.src/handlers/github-subscription-handler.ts (1)
124-136: Two‑step OAuth check (linked + valid) matches intended UX; minor redundancy is acceptableThe sequence
isLinked→validateTokenlets you distinguish “never connected” from “was connected but token is now invalid,” which is important for tailored messages. Note that both methods hit the tokens table, so there’s some redundant DB work; if this becomes hot, you could havevalidateTokenreturn a more detailed status and skip the separateisLinkedcall. For now the clarity/UX trade-off seems reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/api/user-oauth-client.ts(1 hunks)src/handlers/github-subscription-handler.ts(1 hunks)src/index.ts(0 hunks)src/services/github-oauth-service.ts(1 hunks)src/services/subscription-service.ts(2 hunks)src/services/user-oauth-client.ts(0 hunks)src/utils/oauth-helpers.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/services/user-oauth-client.ts
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/services/github-oauth-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-43)
src/services/subscription-service.ts (1)
src/api/user-oauth-client.ts (4)
UserProfile(19-22)getUserProfile(72-89)RepositoryInfo(6-14)validateRepository(32-64)
src/handlers/github-subscription-handler.ts (1)
src/services/github-oauth-service.ts (1)
isLinked(328-331)
🪛 ESLint
src/api/user-oauth-client.ts
[error] 1-1: Unable to resolve path to module '@octokit/rest'.
(import-x/no-unresolved)
🔇 Additional comments (5)
src/utils/oauth-helpers.ts (1)
16-20: Authorization URL call now matchesGitHubOAuthService.getAuthorizationUrlsignaturePassing
(userId, channelId, spaceId, "subscribe", { repo, eventTypes })aligns with the service’s parameter order and fixes the previous channel/space swap. Looks good.src/services/subscription-service.ts (2)
3-8: Good move to shared OAuth client moduleImporting
getUserProfile/validateRepositoryand the sharedUserProfile/RepositoryInfotypes centralizes GitHub OAuth behavior and keeps this service focused on subscription orchestration. The direction looks solid.
136-149: Repository validation failure mapping aligns with new flowCatching errors from
validateRepositoryand returning a structuredSubscribeFailure(re-using the helper’s descriptive messages for 403/404) keeps the subscription handler simple and prevents unhandled exceptions from bubbling out ofcreateSubscription. This matches the intended OAuth-first, failure-as-data design.src/handlers/github-subscription-handler.ts (1)
100-122: Nice reuse viasendOAuthRedirectclosure for OAuth messagingThe helper cleanly centralizes URL generation and the two message variants (“not linked” vs “expired”), avoiding duplication and keeping the main flow readable while still passing through the repo + eventTypes context in
redirectData.src/api/user-oauth-client.ts (1)
1-1: @octokit/rest dependency is already installed—no action requiredThe dependency is properly declared in
package.jsonwith version"^22.0.1"as a regular dependency. The import will resolve correctly. If ESLint continues reporting it as unresolved, this indicates a linting or TypeScript configuration issue, not a missing dependency—verify your.eslintignore, ESLint plugin settings, ortsconfig.jsonpath resolution if the error persists in CI.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/handlers/github-subscription-handler.ts (2)
103-124: TokenStatus-based preflight check is solid; consider explicit handling per statusThe preflight
validateTokencall and tailored messaging forNotLinkedvs “expired/revoked” tokens look correct and align with the PR goals. The only small future-proofing tweak would be to switch to an explicitswitch (tokenStatus)so that any newTokenStatusvalues (e.g.,Unknown) don’t automatically get mapped into the “expired token” branch without a conscious choice.
156-160: UnifieddeliveryInforeads well; ensureinstallUrlis always set for polling modeThe ternary-based
deliveryInfoconstruction is clearer than inlined string building and keeps the success message tidy. Just make sureSubscriptionServicealways provides a non-emptyinstallUrlwheneverdeliveryMode !== "webhook"so you never end up interpolatingundefinedinto the hint text.src/services/github-oauth-service.ts (2)
35-42:TokenStatusenum API is clear and easy to consumeThe three-way split into
NotLinked,Invalid, andValidwith readable string values makes downstream handling straightforward and keeps logs/telemetry easy to interpret. Only minor naming/UX nit: your UI copy talks about “expired/revoked” tokens while the enum usesInvalid; if you ever expose these strings externally, consider aligning terminology, but it’s not required.
331-367: Consider refactoringvalidateTokento reusegetUserOctokitand handle 403 responses separatelyThe code duplication concern is valid:
validateToken(line 344) duplicates the Octokit instantiation pattern fromgetUserOctokit(lines 296–298). Refactoring to reusegetUserOctokitwould centralize token-to-client construction.Additionally, the web search confirms that GitHub's
GET /userendpoint returns both 401 and 403 as authentication error codes, with 403 indicating insufficient scope, temporary lockouts, or rate-limit blocks. The current logic treats all non-401 errors asValidand logs a warning, which could mask 403 responses that indicate an unusable token. IntroducingTokenStatus.Unknown(or similar) to distinguish transient errors from confirmed validity would provide crisper UX and prevent callers from assuming a token is valid when validation was inconclusive.However, since
getUserOctokitreturnsnull(notTokenStatus.NotLinked), reusing it requires handling the null case differently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/github-subscription-handler.ts(3 hunks)src/services/github-oauth-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-43)
🔇 Additional comments (1)
src/handlers/github-subscription-handler.ts (1)
4-7: Typed import ofGitHubOAuthServiceis idiomatic and keeps runtime bundle leanUsing
TokenStatusas a value import andGitHubOAuthServiceas a type-only import is a clean pattern; no issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/handlers/github-subscription-handler.ts (2)
103-142: Token pre‑validation and user messaging look solid; consider a defensive default branchThe flow correctly blocks subscription attempts when the token is not valid and gives tailored copy for not‑linked, invalid, and unknown states, which matches the new
TokenStatuscontract.To future‑proof this against additional
TokenStatusvalues, consider adding a defensive default branch that also returns early (e.g., log and treat asUnknown) so new statuses can’t accidentally fall through and proceed to subscription.
172-178: Guard against missinginstallUrlin polling mode
deliveryInfoassumes thatresult.installUrlis always set whendeliveryMode !== "webhook". IfinstallUrlcan ever beundefinedfor polling responses, the success message will render a broken markdown link.You could defensively handle this with a small guard:
- const deliveryInfo = - result.deliveryMode === "webhook" - ? "⚡ Real-time webhook delivery enabled!" - : "⏱️ Events are checked every 5 minutes (polling mode)\n\n" + - `💡 **Want real-time notifications?** [Install the GitHub App](${result.installUrl})`; + const deliveryInfo = + result.deliveryMode === "webhook" + ? "⚡ Real-time webhook delivery enabled!" + : result.installUrl + ? "⏱️ Events are checked every 5 minutes (polling mode)\n\n" + + `💡 **Want real-time notifications?** [Install the GitHub App](${result.installUrl})` + : "⏱️ Events are checked every 5 minutes (polling mode)";If the contract guarantees
installUrlin this case, just confirm that with tests and leave as is.src/services/github-oauth-service.ts (1)
35-43: TokenStatus model and validation logic are on point; consider widening the try/catch scopeThe
TokenStatusenum andvalidateTokenbehavior (NotLinked → no token, Valid → authenticated call ok, Invalid → 401 with local deletion, Unknown → 403 or other errors) are a good fit for the handler logic and the PR’s revoked‑token handling goal.One improvement:
getUserOctokit(townsUserId)is called outside thetryblock, so any DB or decryption failures will bubble up as uncaught errors instead of being surfaced asTokenStatus.Unknown. If you wantvalidateTokento be the single entrypoint that never throws for consumer code, you can wrap that call as well:- async validateToken(townsUserId: string): Promise<TokenStatus> { - const octokit = await this.getUserOctokit(townsUserId); - if (!octokit) { - return TokenStatus.NotLinked; - } - - try { + async validateToken(townsUserId: string): Promise<TokenStatus> { + try { + const octokit = await this.getUserOctokit(townsUserId); + if (!octokit) { + return TokenStatus.NotLinked; + } + await octokit.users.getAuthenticated(); return TokenStatus.Valid; } catch (error) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const status = (error as any)?.status; @@ console.warn(`Error validating token for ${townsUserId}:`, error); return TokenStatus.Unknown; } }This keeps the external contract (“returns a
TokenStatusinstead of throwing”) consistent even when storage or crypto issues occur, while still logging underlying failures.Also applies to: 334-370
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/github-subscription-handler.ts(3 hunks)src/services/github-oauth-service.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-43)
🔇 Additional comments (2)
src/handlers/github-subscription-handler.ts (1)
4-7: OAuth service import wiring looks goodImporting
TokenStatusand using a type-only import for the OAuth service keeps this handler decoupled from construction details and aligns with the new service API. No issues here.src/services/github-oauth-service.ts (1)
290-297: Use of decrypted token for user client and revocation is consistent
getUserTokenalready returns decryptedaccessToken/refreshToken, so usingtoken.accessTokenboth when constructing the user-scoped client and when revoking the token ensures callers don’t need to know about encryption details. This keeps crypto concerns nicely encapsulated inside the service.Also applies to: 305-316
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/handlers/github-subscription-handler.ts (1)
103-149: Well-structured token validation with clear user messaging.The switch statement handles all
TokenStatuscases appropriately:
- NotLinked: Clear prompt to connect GitHub
- Invalid: Explains token expiration/revocation with reconnection link
- Unknown: Helpful guidance about temporary issues (rate limiting)
- Default: Exhaustiveness check ensures compile-time safety
Minor optimization opportunity: The authorization URL is generated at line 107 before the switch statement, meaning it's created even for the
Validcase where it's not needed. Consider moving the URL generation inside each case to avoid unnecessary OAuth state creation.Apply this diff to defer URL generation:
// Check if user has linked their GitHub account and token is valid const tokenStatus = await oauthService.validateToken(userId); if (tokenStatus !== TokenStatus.Valid) { - const authUrl = await oauthService.getAuthorizationUrl( - userId, - channelId, - spaceId, - "subscribe", - { repo, eventTypes } - ); - switch (tokenStatus) { case TokenStatus.NotLinked: + const authUrlNotLinked = await oauthService.getAuthorizationUrl( + userId, + channelId, + spaceId, + "subscribe", + { repo, eventTypes } + ); await handler.sendMessage( channelId, `🔐 **GitHub Account Required**\n\n` + `To subscribe to repositories, you need to connect your GitHub account.\n\n` + - `[Connect GitHub Account](${authUrl})` + `[Connect GitHub Account](${authUrlNotLinked})` ); return; case TokenStatus.Invalid: + const authUrlInvalid = await oauthService.getAuthorizationUrl( + userId, + channelId, + spaceId, + "subscribe", + { repo, eventTypes } + ); await handler.sendMessage( channelId, `⚠️ **GitHub Token Expired**\n\n` + `Your GitHub token has expired or been revoked. Please reconnect your account.\n\n` + - `[Reconnect GitHub Account](${authUrl})` + `[Reconnect GitHub Account](${authUrlInvalid})` ); return; case TokenStatus.Unknown: + const authUrlUnknown = await oauthService.getAuthorizationUrl( + userId, + channelId, + spaceId, + "subscribe", + { repo, eventTypes } + ); await handler.sendMessage( channelId, `⚠️ **Unable to Verify GitHub Connection**\n\n` + `We couldn't verify your GitHub token. This could be temporary (rate limiting) or indicate a connection issue.\n\n` + - `Please try again in a few moments, or [reconnect your account](${authUrl}) if the problem persists.` + `Please try again in a few moments, or [reconnect your account](${authUrlUnknown}) if the problem persists.` ); return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/github-subscription-handler.ts(3 hunks)src/services/github-oauth-service.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-43)
🔇 Additional comments (2)
src/services/github-oauth-service.ts (1)
35-43: LGTM! Well-defined token status enum.The
TokenStatusenum provides clear, semantic values for different token validation states. The four states (NotLinked, Invalid, Valid, Unknown) appropriately cover all scenarios in the validation flow.src/handlers/github-subscription-handler.ts (1)
180-184: LGTM! Cleaner delivery info formatting.The refactor consolidates the conditional logic into a single ternary expression, making the code more concise while maintaining the same functionality.
This commit fixes critical bugs and improves the OAuth token validation flow:
**Critical Fixes:**
- Fix app crash when OAuth token is revoked (401 errors now handled gracefully)
- Fix parameter order bug in oauth-helpers.ts (channelId/spaceId were swapped)
- Add proactive token validation before subscription attempts
**OAuth Token Validation:**
- Add TokenStatus enum (Valid, Invalid, NotLinked, Unknown) for state tracking
- Add validateToken() method to GitHubOAuthService with automatic cleanup
- Auto-delete revoked/expired tokens from database on 401 detection
- Handle 403 separately (insufficient scope/rate limits) without deleting tokens
- Widen try/catch scope to catch DB/decryption errors as Unknown (never throws)
- Update subscription handler to validate tokens before use with switch statement
- Show tailored user messages for each TokenStatus state
**Error Handling Improvements:**
- Wrap errors with { cause } to preserve debugging info
- Add try/catch around getUserProfile() in subscription service
- Return SubscribeFailure instead of throwing unhandled errors
- Consistent error handling across all OAuth client methods
**Code Quality:**
- Refactor UserOAuthClient from stateless class to pure functions
- Move src/services/user-oauth-client.ts → src/api/user-oauth-client.ts
- Refactor validateToken() to reuse getUserOctokit() (eliminates duplication)
- Generate authUrl once in closure to avoid repetition across error cases
- Replace if/else with switch statement for exhaustiveness checking
- Convert let+if/else to const+ternary for deliveryInfo
- Document side effect: validateToken() auto-deletes tokens on 401
**Files Changed:**
- src/api/user-oauth-client.ts (new) - Pure functions for user OAuth API calls
- src/services/github-oauth-service.ts - Add TokenStatus enum and validateToken()
- src/handlers/github-subscription-handler.ts - Add token validation with switch
- src/services/subscription-service.ts - Add error handling, use functions
- src/utils/oauth-helpers.ts - Fix parameter order bug
- src/index.ts - Remove UserOAuthClient instantiation
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
shuhuiluo
force-pushed
the
fix/oauth-token-validation-and-refactoring
branch
from
77291e3 to
b613203
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/api/user-oauth-client.ts(1 hunks)src/handlers/github-subscription-handler.ts(3 hunks)src/index.ts(0 hunks)src/services/github-oauth-service.ts(4 hunks)src/services/subscription-service.ts(2 hunks)src/services/user-oauth-client.ts(0 hunks)src/utils/oauth-helpers.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/index.ts
- src/services/user-oauth-client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/oauth-helpers.ts
- src/services/subscription-service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/github-oauth-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubUserTokens(20-43)
🪛 ESLint
src/api/user-oauth-client.ts
[error] 1-1: Unable to resolve path to module '@octokit/rest'.
(import-x/no-unresolved)
🔇 Additional comments (4)
src/handlers/github-subscription-handler.ts (2)
103-149: LGTM! Excellent token status handling with exhaustiveness check.The token validation flow correctly branches on all
TokenStatusvalues with user-friendly messaging for each scenario. The TypeScript exhaustiveness check at lines 143-147 ensures compile-time safety if new status values are added in the future.
180-191: LGTM! Improved readability by extracting delivery info logic.The refactor from inline conditional to a named constant improves code readability without changing functionality.
src/services/github-oauth-service.ts (2)
328-336: LGTM! JSDoc now clearly documents the token deletion side effect.The updated documentation addresses the previous review concern by explicitly noting that invalid tokens are automatically deleted from the database on 401 responses. This makes the side effect clear to callers.
337-374: LGTM! Correct implementation of token validation with appropriate error handling.The implementation properly distinguishes between:
- 401 (revoked/invalid): Deletes token and returns
Invalid- 403 (rate limit/scope): Preserves token and returns
Unknownfor retry- Other errors: Returns
Unknownwith logging for debuggingThe deletion operation on line 355-357 is correctly awaited, ensuring that DB failures will propagate rather than silently fail.
| owner: { | ||
| login: data.owner.login, | ||
| type: data.owner.type as "User" | "Organization", | ||
| id: data.owner.id, | ||
| }, |
There was a problem hiding this comment.
Unsafe type assertion for owner.type may cause runtime issues.
Line 50 casts data.owner.type to "User" | "Organization" without validation. GitHub's API can return other owner types such as "Bot", which would violate the type constraint and potentially cause issues in downstream code that expects only User or Organization.
Apply this diff to add runtime validation:
- owner: {
- login: data.owner.login,
- type: data.owner.type as "User" | "Organization",
- id: data.owner.id,
- },
+ owner: {
+ login: data.owner.login,
+ type: (data.owner.type === "User" || data.owner.type === "Organization")
+ ? data.owner.type
+ : "User", // Fallback for Bot and other types
+ id: data.owner.id,
+ },Alternatively, expand the type union to include "Bot" if bot-owned repositories are expected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| owner: { | |
| login: data.owner.login, | |
| type: data.owner.type as "User" | "Organization", | |
| id: data.owner.id, | |
| }, | |
| owner: { | |
| login: data.owner.login, | |
| type: (data.owner.type === "User" || data.owner.type === "Organization") | |
| ? data.owner.type | |
| : "User", // Fallback for Bot and other types | |
| id: data.owner.id, | |
| }, |
🤖 Prompt for AI Agents
In src/api/user-oauth-client.ts around lines 48 to 52, the owner.type is being
asserted to "User" | "Organization" without validation; replace the unsafe cast
with a runtime check that reads data.owner.type and only assigns it when it
equals "User" or "Organization", otherwise either include "Bot" (or other
expected types) in the allowed union or handle unexpected values by mapping them
to a safe fallback or throwing/logging an error. Ensure the code branches: if
allowedTypes.includes(data.owner.type) assign that value, else handle explicitly
(e.g., add "Bot" to the type union, set a default like "Organization", or raise
a clear error) so downstream code never receives an invalid type.
This commit fixes critical bugs and improves the OAuth token validation flow:
Critical Fixes:
OAuth Token Validation:
Error Handling Improvements:
Code Quality:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.