feat: unified token management with Flow auto-refresh - #74
Conversation
The token refresh mechanism was silently downscoping tokens from 10 scopes to 3 (offline_access User.Read Mail.Read), causing permission loss and forced re-authentication every few hours. - Unify all 4 scope lists to single source of truth in config.js (10 scopes including offline_access + Contacts.Read) - token-storage.js now imports scopes from config.js instead of hardcoded 3-scope string - MS_SCOPES env var override warns if offline_access is missing - outlook-auth-server.js imports scopes from config.js - check-auth-status migrated from legacy token-manager to TokenStorage (reports authenticated when token is expired but refreshable) - 5 new tests covering scope unification, MS_SCOPES validation, and full-scope refresh (147/147 total passing)
…Storage - Add 4 Flow token methods to TokenStorage: getFlowAccessToken, saveFlowTokens, isFlowTokenExpired, getValidFlowAccessToken - Export tokenStorage singleton from auth/index.js - Migrate 5 power-automate handlers to async TokenStorage calls - Remove Flow methods from token-manager.js (keep createTestTokens only) - 18 new tests (166 total, all passing) - SDD cycle: explore → propose → spec → design → tasks → apply → verify → archive
- Add refreshFlowAccessToken() mirroring Graph refresh with Flow scope - Update getValidFlowAccessToken() to attempt refresh on expiry (was null) - Add _flowRefreshPromise for concurrent refresh dedup - Surgical invalidation on failure: null only flow_* keys, preserve Graph tokens - 9 new tests (175 total, all passing) - SDD cycle: explore -> propose -> spec -> design -> tasks -> apply -> verify -> archive
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
📝 WalkthroughWalkthroughThe PR centralizes Graph and Power Automate token handling in ChangesAuthentication and folder resolution updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Handler
participant TokenStorage
participant OAuthEndpoint
participant TokenFile
Handler->>TokenStorage: Request valid Flow access token
TokenStorage->>OAuthEndpoint: Refresh when token is expired
OAuthEndpoint-->>TokenStorage: Return token response
TokenStorage->>TokenFile: Persist refreshed or invalidated Flow fields
TokenStorage-->>Handler: Return token or null
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (2)
test/auth/token-storage.test.js (1)
66-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
MS_SCOPEScleanup intoafterEach. A failed assertion skips the trailingdelete, leaking the env var into subsequent tests (including the default-scopes test at line 55) and producing misleading cascading failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/auth/token-storage.test.js` around lines 66 - 92, Move MS_SCOPES environment cleanup from the individual tests into an afterEach hook for the TokenStorage test suite, ensuring it always deletes the variable even when assertions fail. Remove the duplicate inline cleanup statements while preserving both MS_SCOPES test behaviors.test/auth/tools.test.js (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock the singleton contract used by production.
The test mocks
TokenStorageas a constructor, so it reinforces per-request instantiation instead of validating sharedtokenStorageusage. Mock../../auth/indexand assert calls to the singleton’sgetValidAccessToken()method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/auth/tools.test.js` around lines 1 - 4, Update the auth tools test setup to mock ../../auth/index rather than TokenStorage, matching production’s shared tokenStorage singleton contract. In the tests around handleCheckAuthStatus, assert interactions with the singleton’s getValidAccessToken() method and remove constructor-based TokenStorage mocking or expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/token-storage.js`:
- Around line 136-141: Update the token assignment in the flow token storage
logic to preserve the existing flow_refresh_token when flowTokens.refresh_token
is undefined, matching the guard already used by refreshFlowAccessToken. Only
replace the stored refresh token when a new value is provided; keep the
access-token and expiry updates unchanged.
- Around line 452-460: Update the token exchange error handling around the
rejection in auth/token-storage.js to remove raw data from both console.error
and the rejected Error message. Replace the payload with safe metadata such as
response length and HTTP status, while preserving the original processing error
and existing rejection behavior.
- Around line 19-22: Update the token store configuration to reuse
appConfig.AUTH_CONFIG.tokenStorePath instead of constructing a path from HOME or
USERPROFILE. Preserve the shared configured path consumed by
outlook-auth-server.js and token-manager.js, including config.js fallbacks when
both environment variables are unset.
- Around line 233-234: Update the https.request calls in auth/token-storage.js
at lines 233-234 and 320-321, used by the Graph refresh and flow refresh paths,
to apply a timeout that destroys the request socket so the existing error
handlers clear _refreshPromise and _flowRefreshPromise. Also add the same
timeout to exchangeCodeForTokens at lines 415-416, preserving its existing error
handling while ensuring its non-cached request cannot hang indefinitely.
- Around line 369-383: Preserve flow credentials for transient failures: remove
flow-token invalidation from the request error handler in the flow refresh
logic, and update the non-2xx response branch to invalidate only permanent OAuth
failures such as 400 invalid_grant, not 429/5xx responses. Also adjust
getValidFlowAccessToken so it does not clear tokens for arbitrary refresh
rejections, while retaining invalidation for invalid_grant and the existing
rejection/promise cleanup behavior.
In `@auth/tools.js`:
- Around line 59-70: The authentication-status check in auth/tools.js should
reuse the shared token storage exported by auth/index.js instead of constructing
a new TokenStorage instance; update the imports and the status-check flow
accordingly. In test/auth/tools.test.js, mock the auth/index.js singleton export
rather than the TokenStorage constructor, while preserving the existing
authentication-status assertions.
---
Nitpick comments:
In `@test/auth/token-storage.test.js`:
- Around line 66-92: Move MS_SCOPES environment cleanup from the individual
tests into an afterEach hook for the TokenStorage test suite, ensuring it always
deletes the variable even when assertions fail. Remove the duplicate inline
cleanup statements while preserving both MS_SCOPES test behaviors.
In `@test/auth/tools.test.js`:
- Around line 1-4: Update the auth tools test setup to mock ../../auth/index
rather than TokenStorage, matching production’s shared tokenStorage singleton
contract. In the tests around handleCheckAuthStatus, assert interactions with
the singleton’s getValidAccessToken() method and remove constructor-based
TokenStorage mocking or expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b72f22d-f02e-4dde-b931-b1a1c7d480e8
📒 Files selected for processing (16)
auth/index.jsauth/token-manager.jsauth/token-storage.jsauth/tools.jsconfig.jsoutlook-auth-server.jspower-automate/list-environments.jspower-automate/list-flows.jspower-automate/list-runs.jspower-automate/run-flow.jspower-automate/toggle-flow.jstest/auth/index.test.jstest/auth/token-manager.test.jstest/auth/token-storage.test.jstest/auth/tools.test.jstest/power-automate/handlers.test.js
| tokenStorePath: path.join( | ||
| process.env.HOME || process.env.USERPROFILE, | ||
| '.outlook-mcp-tokens.json' | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reuse appConfig.AUTH_CONFIG.tokenStorePath instead of re-deriving it. config.js (line 8) falls back through HOME → USERPROFILE → os.homedir() → '/tmp', while this derivation throws TypeError: path argument must be of type string when both env vars are unset (common under launchd/systemd). It also creates a second source of truth for the token file that outlook-auth-server.js and token-manager.js both read from config.
🛠️ Proposed fix
- tokenStorePath: path.join(
- process.env.HOME || process.env.USERPROFILE,
- '.outlook-mcp-tokens.json'
- ),
+ tokenStorePath: appConfig.AUTH_CONFIG.tokenStorePath,📝 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.
| tokenStorePath: path.join( | |
| process.env.HOME || process.env.USERPROFILE, | |
| '.outlook-mcp-tokens.json' | |
| ), | |
| tokenStorePath: appConfig.AUTH_CONFIG.tokenStorePath, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/token-storage.js` around lines 19 - 22, Update the token store
configuration to reuse appConfig.AUTH_CONFIG.tokenStorePath instead of
constructing a path from HOME or USERPROFILE. Preserve the shared configured
path consumed by outlook-auth-server.js and token-manager.js, including
config.js fallbacks when both environment variables are unset.
HIGH severity: - Escape apostrophes in OData filter for folder names (folder-utils.js) - Preserve flow_refresh_token in saveFlowTokens when undefined - Add 30s timeout to all https.request calls (refresh, flow refresh, exchange) - Only invalidate flow tokens on permanent failures (invalid_grant), not transient (429/5xx/network) MEDIUM severity: - Use singleton tokenStorage from auth/index.js in check-auth-status - Remove raw response data from exchangeCodeForTokens error messages - Update warning message to mention both credential pairs 212 tests passing, 0 ESLint errors
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth/token-storage.js (1)
75-77: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
{ mode: 0o600 }only applies when the file is created. Existing token files (created before this change, typically0644) keep their permissions, so credentials stay world-readable after upgrade. Chmod explicitly after write.🔒 Proposed fix
await fs.writeFile(this.config.tokenStorePath, JSON.stringify(this.tokens, null, 2), { mode: 0o600, }); + await fs.chmod(this.config.tokenStorePath, 0o600);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/token-storage.js` around lines 75 - 77, Update the token persistence flow around the writeFile call to explicitly set the token store permissions to 0o600 after writing, including for existing files. Preserve the current path and JSON serialization while ensuring chmod applies on every save.
🧹 Nitpick comments (1)
auth/token-storage.js (1)
157-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPermanent-failure detection via
refreshError.message.includes('invalid_grant')is fragile.refreshFlowAccessTokenrejects withresponseBody.error_description(e.g.AADSTS700082: The refresh token has expired…), which typically does not contain the stringinvalid_grant, so this branch rarely fires. Net behavior is still correct only becauserefreshFlowAccessTokenalready invalidates flow tokens internally for400 + invalid_grant. Consider dropping the duplicated invalidation here, or attaching a structured flag (e.g.error.oauthError = responseBody.error) at the rejection site and checking that instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/token-storage.js` around lines 157 - 179, The permanent-failure check in the flow-token refresh catch block is unreliable because it inspects only refreshError.message. Update refreshFlowAccessToken and its caller to use the structured OAuth error response, such as attaching responseBody.error to the rejected error and checking that flag in the catch, or remove the duplicated invalidation if refreshFlowAccessToken already handles all permanent invalid_grant failures. Preserve invalidation for permanent failures without invalidating tokens on transient errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@email/folder-utils.js`:
- Around line 76-78: Update the case-insensitive fallback in resolveFolderPath
to paginate through all Graph folder pages instead of searching only the initial
response from callGraphAPI. Repeatedly follow the response’s `@odata.nextLink`
using callGraphAPI until a matching folder is found or no next link remains,
preserving the existing match and Inbox fallback behavior.
---
Outside diff comments:
In `@auth/token-storage.js`:
- Around line 75-77: Update the token persistence flow around the writeFile call
to explicitly set the token store permissions to 0o600 after writing, including
for existing files. Preserve the current path and JSON serialization while
ensuring chmod applies on every save.
---
Nitpick comments:
In `@auth/token-storage.js`:
- Around line 157-179: The permanent-failure check in the flow-token refresh
catch block is unreliable because it inspects only refreshError.message. Update
refreshFlowAccessToken and its caller to use the structured OAuth error
response, such as attaching responseBody.error to the rejected error and
checking that flag in the catch, or remove the duplicated invalidation if
refreshFlowAccessToken already handles all permanent invalid_grant failures.
Preserve invalidation for permanent failures without invalidating tokens on
transient errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cb90766-1ec7-48d6-b3a5-9f4be8cfced8
📒 Files selected for processing (5)
auth/token-storage.jsauth/tools.jsemail/folder-utils.jstest/auth/token-storage.test.jstest/auth/tools.test.js
| // If exact match fails, try to get all folders and do a case-insensitive comparison | ||
| const allFoldersResponse = await callGraphAPI(accessToken, 'GET', base, null, { $top: 100 }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Paginate the case-insensitive fallback.
Only the first 100 folders are searched. A matching folder on a later Graph page is treated as absent, causing resolveFolderPath to fall back to Inbox. Follow @odata.nextLink until a match is found or pages are exhausted; callGraphAPI already supports full next-link URLs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@email/folder-utils.js` around lines 76 - 78, Update the case-insensitive
fallback in resolveFolderPath to paginate through all Graph folder pages instead
of searching only the initial response from callGraphAPI. Repeatedly follow the
response’s `@odata.nextLink` using callGraphAPI until a matching folder is found
or no next link remains, preserving the existing match and Inbox fallback
behavior.
1 participant
Summary
This PR combines 3 related improvements to the OAuth token system, each building on the previous:
1. Fix persistent OAuth authentication
Token refresh requested a downscoped token (3 scopes) vs the original 10, causing silent scope downgrade and forced re-auth every few hours. Unifies all scope lists to a single source in config.js.
2. Migrate power-automate handlers to TokenStorage
5 Power Automate handlers imported getFlowAccessToken from �uth/token-manager.js (sync I/O, no refresh). Migrated to TokenStorage with async I/O and 4 new Flow methods. Removed Flow methods from oken-manager.js (kept createTestTokens for test mode).
3. Add Flow token auto-refresh
getValidFlowAccessToken() returned null on expiry with no refresh attempt. Added
efreshFlowAccessToken() mirroring the Graph refresh pattern with flowRefreshPromise dedup. Surgical invalidation on failure: nulls only low* keys, preserves Graph tokens.
Files Changed (16 files, ~844 lines)
Testing
Backwards Compatibility
Summary by CodeRabbit
New Features
Bug Fixes
Tests