Conversation
|
@miha-yy is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces Microsoft Outlook (Entra ID) as a supported OAuth provider alongside Google throughout the authentication, account linking, and permissions flows. It adds new configuration, provider-agnostic logic, and Microsoft-specific utilities, while updating documentation and environment variable handling to reflect the expanded provider support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginForm
participant AuthUtils
participant MicrosoftOAuth
participant MicrosoftGraph
participant AppDB
User->>LoginForm: Clicks "Sign in with Microsoft"
LoginForm->>LoginForm: Shows Microsoft consent dialog
User->>LoginForm: Clicks "I agree"
LoginForm->>AuthUtils: Initiates Microsoft sign-in
AuthUtils->>MicrosoftOAuth: Redirects to Microsoft OAuth endpoint
MicrosoftOAuth->>User: User authorizes app
MicrosoftOAuth->>AuthUtils: Returns auth code
AuthUtils->>MicrosoftOAuth: Exchanges code for tokens
AuthUtils->>MicrosoftGraph: Fetches user profile and photo
AuthUtils->>AppDB: Stores tokens and profile data
AppDB-->>AuthUtils: Acknowledges storage
AuthUtils-->>LoginForm: Completes sign-in
LoginForm-->>User: Redirects to app
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/utils/outlook/client.ts (1)
11-16: Remove redundant property from AuthOptions type.The
AuthOptionstype contains bothexpiryDateandexpiresAtproperties which appear to serve the same purpose. This could lead to confusion about which one to use.Consider removing the redundant property:
type AuthOptions = { accessToken?: string | null; refreshToken?: string | null; - expiryDate?: number | null; expiresAt?: number | null; };apps/web/utils/auth.ts (1)
60-60: Use a string literal instead of template literal.The static analysis correctly identifies that a template literal is unnecessary here since no interpolation is used.
- photoUrl = `https://graph.microsoft.com/v1.0/me/photo/$value`; + photoUrl = "https://graph.microsoft.com/v1.0/me/photo/$value";🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/microsoft.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/web/.env.example(1 hunks)apps/web/app/(landing)/login/LoginForm.tsx(1 hunks)apps/web/env.ts(1 hunks)apps/web/utils/auth.ts(13 hunks)apps/web/utils/outlook/client.ts(1 hunks)apps/web/utils/outlook/scopes.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/app/(landing)/login/LoginForm.tsx (1)
apps/web/components/Typography.tsx (1)
SectionDescription(114-114)
apps/web/utils/outlook/scopes.ts (1)
apps/web/env.ts (1)
env(5-207)
🪛 Biome (1.9.4)
apps/web/utils/auth.ts
[error] 60-60: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
🔇 Additional comments (13)
apps/web/.env.example (1)
16-20: LGTM! Consistent with existing Google configuration pattern.The Microsoft environment variables follow the same structure and naming convention as the Google credentials, which maintains consistency. The helpful comments for generating secure secrets using OpenSSL are a nice touch.
apps/web/utils/outlook/scopes.ts (1)
1-14: Well-structured scope definition with appropriate permissions.The Microsoft Graph API scopes are comprehensive and appropriate for email integration. The conditional inclusion of
Contacts.ReadWritebased on the environment variable is good design for feature toggles. The documentation reference and TypeScript typing withas constare excellent practices.apps/web/env.ts (1)
15-18: Consistent environment validation following established patterns.The Microsoft environment variables are properly validated with the same pattern as Google credentials. The schema correctly requires these as non-empty strings, maintaining consistency with the existing authentication configuration.
apps/web/utils/outlook/client.ts (4)
18-25: LGTM!Clean helper function that properly initializes the Microsoft Graph client with authentication.
27-30: LGTM!Proper validation and error handling for access token requirement.
32-101: LGTM!Well-implemented token refresh logic with proper error handling and token persistence. The function correctly handles token expiration checks and invalid grant errors.
110-141: LGTM!Well-structured helper functions with proper error handling. The OAuth URL construction follows Microsoft's authorization flow requirements, and the API helper functions provide clean abstractions.
apps/web/utils/auth.ts (6)
24-79: Excellent provider abstraction pattern!The
PROVIDER_CONFIGobject provides a clean abstraction for handling multiple OAuth providers, making it easy to add new providers in the future while maintaining consistent behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
104-160: LGTM!Robust Microsoft provider configuration with comprehensive error handling and fallback mechanisms. The extensive logging will be helpful for debugging authentication issues.
187-197: LGTM!Clean integration with the
PROVIDER_CONFIGabstraction, making the profile fetching logic provider-agnostic and maintainable.
279-280: LGTM!Excellent implementation of multi-provider token management. The changes properly maintain provider context throughout the token lifecycle and correctly handle provider-specific refresh logic.
Also applies to: 287-288, 298-298, 413-414, 418-418, 439-453, 480-480, 491-492
517-525: LGTM!The
saveTokensfunction properly handles multi-provider scenarios while maintaining backward compatibility with the default "google" provider.Also applies to: 579-580
614-636: LGTM!Appropriate type extensions for NextAuth to support the multi-provider authentication flow and error handling.
|
Also, please add some short notes on how to set it up / get the env keys. Even just 2 lines + a link is fine so I don't need to investigate when setting it up myself. When the feature goes live for real we can add it to the README for future contributors / self-hosters. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
168-168: Fix markdown heading formatting.Remove the trailing colon from the heading to comply with markdown best practices.
-### Updating .env file with Microsoft OAuth credentials: +### Updating .env file with Microsoft OAuth credentials🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
168-168: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)apps/web/app/(landing)/login/LoginForm.tsx(3 hunks)apps/web/env.ts(1 hunks)apps/web/utils/gmail/permissions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/(landing)/login/LoginForm.tsx
- apps/web/env.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
168-168: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (2)
apps/web/utils/gmail/permissions.ts (1)
91-91: LGTM! Good defensive programming for multi-provider support.Adding the
provider: "google"condition ensures that Gmail-specific token cleanup only affects Google accounts, preventing unintended modifications to Microsoft accounts with the same ID. This is a smart improvement for data integrity in the multi-provider authentication system.README.md (1)
168-219: Excellent comprehensive Microsoft OAuth setup documentation.The setup instructions are thorough and well-structured, covering all necessary steps from Azure AD app registration to API permissions. The permissions listed are appropriate for Outlook integration, and the documentation addresses the maintainer's request for setup instructions.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
168-168: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/web/utils/actions/permissions.ts (1)
15-19: Acknowledge interim solution for multi-provider permissions.The early return for non-Google providers is a reasonable interim approach to avoid blocking Microsoft authentication while Gmail-specific permission checks are bypassed. The TODO comment correctly indicates this needs proper Outlook permission handling.
Would you like me to help implement the Outlook-specific permission checks to complete this TODO?
apps/web/utils/outlook/client.ts (1)
56-56: Fix unnecessary template literal.The static analysis tool correctly identified that this template literal doesn't use interpolation or special characters.
Apply this fix:
- return `https://graph.microsoft.com/v1.0/me/photo/$value`; + return "https://graph.microsoft.com/v1.0/me/photo/$value";🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/auth.ts (2)
61-61: Use string literal instead of template literal.The template literal is unnecessary here since no interpolation is being performed.
- photoUrl = `https://graph.microsoft.com/v1.0/me/photo/$value`; + photoUrl = "https://graph.microsoft.com/v1.0/me/photo/$value";🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
56-66: Consider photo URL accessibility and authentication.The hardcoded Microsoft Graph photo URL may not work correctly as it requires the same authentication token. Consider using a data URL or handling the photo retrieval differently.
let photoUrl = null; try { const photo = await client.getUserPhoto(); - if (photo) { - photoUrl = `https://graph.microsoft.com/v1.0/me/photo/$value`; - } + if (photo) { + // Convert to data URL for immediate use + photoUrl = `data:image/jpeg;base64,${photo}`; + } } catch (error) { logger.info("User has no profile photo", { error }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/(app)/[emailAccountId]/PermissionsCheck.tsx(2 hunks)apps/web/app/api/user/email-accounts/route.ts(1 hunks)apps/web/providers/EmailAccountProvider.tsx(2 hunks)apps/web/utils/actions/permissions.ts(1 hunks)apps/web/utils/actions/safe-action.ts(2 hunks)apps/web/utils/auth.ts(13 hunks)apps/web/utils/outlook/client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/api/user/email-accounts/route.ts
- apps/web/providers/EmailAccountProvider.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/app/(app)/[emailAccountId]/PermissionsCheck.tsx (1)
apps/web/providers/EmailAccountProvider.tsx (1)
useAccount(72-82)
🪛 Biome (1.9.4)
apps/web/utils/auth.ts
[error] 61-61: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/outlook/client.ts
[error] 56-56: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
🔇 Additional comments (8)
apps/web/app/(app)/[emailAccountId]/PermissionsCheck.tsx (1)
13-13: LGTM! Proper integration of provider-aware logic.The addition of
providerto both the destructured values and the useEffect dependency array correctly ensures the permissions check re-runs when the provider changes, supporting the multi-provider authentication flow.Also applies to: 25-25
apps/web/utils/actions/safe-action.ts (1)
58-60: LGTM! Proper context extension for multi-provider support.The inclusion of the related account in the Prisma query and adding the provider to the action context correctly enables provider-aware logic in downstream actions.
Also applies to: 73-73
apps/web/utils/outlook/client.ts (2)
18-38: Excellent wrapper class implementation!The
OutlookClientwrapper class approach perfectly addresses the previous concern about accessing private properties of the Microsoft Graph client. This design cleanly encapsulates both the client and access token while providing safe access methods.
151-162: LGTM! OAuth2 URL generation is correctly implemented.The OAuth2 URL generation properly constructs the Microsoft authorization endpoint with all required parameters including scopes and redirect URI.
apps/web/utils/auth.ts (4)
23-80: Excellent provider abstraction design.The PROVIDER_CONFIG object provides clean separation of concerns and makes the codebase more maintainable for multi-provider support. The error handling and logging are well implemented.
🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
105-130: Robust Microsoft provider configuration.The profile validation and error handling for missing email addresses is well implemented. The scope configuration and authorization parameters look correct for Microsoft Entra ID.
160-166: Provider-agnostic profile fetching implementation is solid.The refactored linkAccount logic correctly uses the provider abstraction to fetch profile data, maintaining compatibility across different OAuth providers while reducing code duplication.
600-600: JWT interface extension is appropriate.Adding the optional provider field to the JWT interface correctly supports the multi-provider authentication flow without breaking existing functionality.
| export const getOutlookClientWithRefresh = async ({ | ||
| accessToken, | ||
| refreshToken, | ||
| expiresAt, | ||
| emailAccountId, | ||
| }: { | ||
| accessToken?: string | null; | ||
| refreshToken: string | null; | ||
| expiresAt: number | null; | ||
| emailAccountId: string; | ||
| }): Promise<OutlookClient> => { | ||
| if (!refreshToken) throw new SafeError("No refresh token"); | ||
|
|
||
| // Check if token needs refresh | ||
| const expiryDate = expiresAt ? expiresAt * 1000 : null; | ||
| if (accessToken && expiryDate && expiryDate > Date.now()) { | ||
| return createOutlookClient(accessToken); | ||
| } | ||
|
|
||
| // Refresh token | ||
| try { | ||
| const response = await fetch( | ||
| "https://login.microsoftonline.com/common/oauth2/v2.0/token", | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/x-www-form-urlencoded", | ||
| }, | ||
| body: new URLSearchParams({ | ||
| client_id: env.MICROSOFT_CLIENT_ID!, | ||
| client_secret: env.MICROSOFT_CLIENT_SECRET!, | ||
| refresh_token: refreshToken, | ||
| grant_type: "refresh_token", | ||
| scope: SCOPES.join(" "), | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| const tokens = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(tokens.error_description || "Failed to refresh token"); | ||
| } | ||
|
|
||
| // Save new tokens | ||
| await saveTokens({ | ||
| tokens: { | ||
| access_token: tokens.access_token, | ||
| expires_at: Math.floor(Date.now() / 1000 + tokens.expires_in), | ||
| }, | ||
| accountRefreshToken: refreshToken, | ||
| emailAccountId, | ||
| provider: "microsoft-entra-id", | ||
| }); | ||
|
|
||
| return createOutlookClient(tokens.access_token); | ||
| } catch (error) { | ||
| const isInvalidGrantError = | ||
| error instanceof Error && | ||
| (error.message.includes("invalid_grant") || | ||
| error.message.includes("AADSTS50173")); | ||
|
|
||
| if (isInvalidGrantError) { | ||
| logger.warn("Error refreshing Outlook access token", { error }); | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify token refresh implementation and error handling.
The token refresh logic follows a similar pattern to Gmail implementation, which is good for consistency. The error handling properly detects invalid grant errors and logs them appropriately.
Please verify that the Microsoft OAuth environment variables are properly configured:
🏁 Script executed:
#!/bin/bash
# Description: Verify Microsoft OAuth environment variables are available
# Expected: Environment variables should be defined
echo "Checking for Microsoft OAuth environment variables..."
if [ -n "$MICROSOFT_CLIENT_ID" ]; then
echo "✓ MICROSOFT_CLIENT_ID is set"
else
echo "✗ MICROSOFT_CLIENT_ID is not set"
fi
if [ -n "$MICROSOFT_CLIENT_SECRET" ]; then
echo "✓ MICROSOFT_CLIENT_SECRET is set"
else
echo "✗ MICROSOFT_CLIENT_SECRET is not set"
fi
# Check if variables are in env file
if [ -f ".env.local" ]; then
echo "Checking .env.local..."
grep -q "MICROSOFT_CLIENT_ID" .env.local && echo "✓ MICROSOFT_CLIENT_ID found in .env.local"
grep -q "MICROSOFT_CLIENT_SECRET" .env.local && echo "✓ MICROSOFT_CLIENT_SECRET found in .env.local"
fiLength of output: 330
Ensure Microsoft OAuth environment variables are set
The token refresh flow in apps/web/utils/outlook/client.ts relies on env.MICROSOFT_CLIENT_ID and env.MICROSOFT_CLIENT_SECRET, but both are currently unset, causing runtime failures when attempting to refresh tokens. Please:
- Add
MICROSOFT_CLIENT_IDandMICROSOFT_CLIENT_SECRETto your environment (e.g. in.env.localor your deployment secrets). - Confirm that your local and production setups load these variables before starting the app.
- Verify at startup that
process.env.MICROSOFT_CLIENT_IDandprocess.env.MICROSOFT_CLIENT_SECRETare non-null.
File requiring attention:
• apps/web/utils/outlook/client.ts (usage of env.MICROSOFT_CLIENT_ID! and env.MICROSOFT_CLIENT_SECRET!)
🤖 Prompt for AI Agents
In apps/web/utils/outlook/client.ts around lines 76 to 144, the Microsoft OAuth
environment variables MICROSOFT_CLIENT_ID and MICROSOFT_CLIENT_SECRET are used
but currently unset, causing runtime failures during token refresh. Fix this by
adding MICROSOFT_CLIENT_ID and MICROSOFT_CLIENT_SECRET to your environment
configuration files like .env.local or your deployment secrets, ensure they are
loaded properly before app startup, and verify that
process.env.MICROSOFT_CLIENT_ID and process.env.MICROSOFT_CLIENT_SECRET are
non-null at runtime.
apps/web/utils/auth.ts
Outdated
| const client = getOutlookContactsClient({ accessToken }); | ||
| try { | ||
| const profileResponse = await client.getUserProfile(); | ||
| console.log("profileResponse", profileResponse); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statements from production code.
Console.log statements should not be present in production authentication code as they can expose sensitive information and clutter logs.
- console.log("profileResponse", profileResponse);
+ logger.info("Microsoft profile fetched successfully", {
+ email: profileResponse.mail || profileResponse.userPrincipalName
+ });- console.log("photoRESPONSE", photo);
+ logger.info("Microsoft profile photo fetched successfully");Also applies to: 59-59
🤖 Prompt for AI Agents
In apps/web/utils/auth.ts at lines 53 and 59, remove the console.log statements
to prevent exposing sensitive information and avoid cluttering production logs.
Replace these debug logs with proper error handling or use a secure logging
mechanism if necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
168-168: Remove trailing punctuation in heading
Trailing punctuation (colon) is discouraged in markdown headings (MD026). Remove the colon to satisfy markdownlint.-### Updating .env file with Microsoft OAuth credentials: +### Updating .env file with Microsoft OAuth credentials🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
168-168: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
168-168: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
🔇 Additional comments (1)
README.md (1)
170-171:Details
✅ Verification successful
Verify placeholder variables in .env.example
Ensure thatapps/web/.env.exampleincludesMICROSOFT_CLIENT_IDandMICROSOFT_CLIENT_SECRETso the README instructions align with the example.
🏁 Script executed:
#!/bin/bash # Verify that the example .env contains Microsoft placeholders grep -n 'MICROSOFT_CLIENT_ID' apps/web/.env.example grep -n 'MICROSOFT_CLIENT_SECRET' apps/web/.env.exampleLength of output: 159
Placeholders Verified in
.env.example
Theapps/web/.env.examplefile already includes bothMICROSOFT_CLIENT_ID(line 17) andMICROSOFT_CLIENT_SECRET(line 18), so the README instructions are accurate and no changes are needed.
miha-yy
force-pushed
the
feat/outlook-auth
branch
from
d7c2a30 to
87ca309
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/utils/auth.ts (2)
24-80: Remove console.log statements from production code.The provider configuration abstraction is excellent, but the console.log statements need to be removed as they can expose sensitive information in production logs.
This matches the previous review comment about console.log statements on lines 53 and 59.
409-423: Require Microsoft client credentials in env schema.The dynamic client credential selection logic is correct, but the Microsoft environment variables are marked as optional while being used with non-null assertions. This creates a runtime risk.
This matches the previous review comment about requiring Microsoft client credentials in the env schema.
🧹 Nitpick comments (2)
apps/web/utils/auth.ts (1)
105-130: Improve error handling and validation in Microsoft profile parsing.The Microsoft provider configuration looks good overall, but consider these improvements:
- The email validation logic should handle edge cases better
- Consider validating the profile structure before accessing properties
profile(profile) { + // Validate profile structure + if (!profile || typeof profile !== 'object') { + logger.error("[MicrosoftProvider] Invalid profile structure", { profile }); + throw new Error("Invalid Microsoft profile structure"); + } + const profileData = { id: profile.sub, name: profile.name, - email: profile.email || profile.preferred_username, + email: (profile.email || profile.preferred_username)?.toLowerCase()?.trim(), image: profile.picture, }; if (!profileData.email) { logger.error("[MicrosoftProvider] No email found in profile data", { profile, }); throw new Error("No email found in Microsoft profile data"); } return profileData; },README.md (1)
168-168: Fix trailing punctuation in heading.Remove the trailing colon from the heading to comply with markdown style guidelines.
-### Updating .env file with Microsoft OAuth credentials: +### Updating .env file with Microsoft OAuth credentials🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
168-168: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/microsoft.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
README.md(1 hunks)apps/web/.env.example(1 hunks)apps/web/app/(app)/[emailAccountId]/PermissionsCheck.tsx(2 hunks)apps/web/app/(landing)/login/LoginForm.tsx(3 hunks)apps/web/app/api/user/email-accounts/route.ts(1 hunks)apps/web/env.ts(1 hunks)apps/web/providers/EmailAccountProvider.tsx(2 hunks)apps/web/utils/actions/permissions.ts(1 hunks)apps/web/utils/actions/safe-action.ts(2 hunks)apps/web/utils/auth.ts(13 hunks)apps/web/utils/outlook/client.ts(1 hunks)apps/web/utils/outlook/scopes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/web/app/api/user/email-accounts/route.ts
- apps/web/providers/EmailAccountProvider.tsx
- apps/web/app/(app)/[emailAccountId]/PermissionsCheck.tsx
- apps/web/utils/actions/safe-action.ts
- apps/web/.env.example
- apps/web/utils/outlook/scopes.ts
- apps/web/env.ts
- apps/web/utils/actions/permissions.ts
- apps/web/app/(landing)/login/LoginForm.tsx
- apps/web/utils/outlook/client.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/utils/auth.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/env.ts (1)
env(5-207)
🪛 markdownlint-cli2 (0.17.2)
README.md
168-168: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
🔇 Additional comments (7)
apps/web/utils/auth.ts (6)
7-7: LGTM! Clean import organization for multi-provider support.The imports are well-organized, separating Gmail and Outlook scopes/clients appropriately for the new multi-provider architecture.
Also applies to: 14-17
160-167: LGTM! Excellent use of provider abstraction.The dynamic provider selection using
PROVIDER_CONFIGis a clean implementation that properly abstracts provider-specific logic while maintaining type safety.
249-250: LGTM! Proper provider tracking in authentication flow.The addition of provider information to tokens and database queries is correctly implemented, ensuring provider context is maintained throughout the authentication lifecycle.
Also applies to: 257-258, 268-268
383-383: LGTM! Provider-aware database queries are correctly implemented.The database queries properly filter by provider, ensuring tokens are refreshed using the correct provider's credentials.
Also applies to: 388-388
487-487: LGTM! Provider parameter handling is well-implemented.The
saveTokensfunction properly handles the provider parameter with appropriate defaults and type safety for the union parameter types.Also applies to: 495-495, 549-549
600-600: LGTM! JWT interface extension is appropriate.Adding the optional
providerfield to the JWT interface properly supports the multi-provider authentication flow.README.md (1)
169-225: Excellent documentation for Microsoft OAuth setup!The Microsoft OAuth setup instructions are comprehensive and well-structured. They provide:
- Clear step-by-step Azure Portal navigation
- Proper account type selection for multitenant support
- Correct redirect URI configuration
- Detailed API permissions list
- Environment variable setup
This documentation addresses the maintainer's request for setup instructions and will be valuable for both contributors and self-hosters.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/web/utils/auth.ts (3)
53-53: Remove console.log statements from production code.Also applies to: 59-59
392-406: Require Microsoft client credentials in env schema.
105-113: 💡 Verification agent🧩 Analysis chain
Validate Microsoft provider configuration consistency.
The Microsoft provider configuration looks correct, but ensure the scopes and client credentials are properly validated at startup.
🏁 Script executed:
#!/bin/bash # Description: Verify Microsoft OAuth scopes and environment variable usage # Expected: Confirm OUTLOOK_SCOPES are properly defined and Microsoft credentials are used correctly echo "=== Checking Outlook scopes definition ===" cat apps/web/utils/outlook/scopes.ts echo -e "\n=== Checking Microsoft environment variables in env files ===" fd -e example -e env | xargs rg -H "MICROSOFT_CLIENT" echo -e "\n=== Checking environment variable validation ===" rg -A 5 -B 5 "MICROSOFT_CLIENT" apps/web/env.tsLength of output: 2628
Enforce required Microsoft OAuth credentials in env schema
The
MICROSOFT_CLIENT_IDandMICROSOFT_CLIENT_SECRETare currently optional in your Zod schema, so missing credentials won’t fail at startup. Update them to required strings to catch misconfiguration early:• apps/web/env.ts
… - MICROSOFT_CLIENT_ID: z.string().optional(), + MICROSOFT_CLIENT_ID: z.string().min(1), - MICROSOFT_CLIENT_SECRET: z.string().optional(), + MICROSOFT_CLIENT_SECRET: z.string().min(1), MICROSOFT_ENCRYPT_SECRET: z.string().optional(), MICROSOFT_ENCRYPT_SALT: z.string().optional(), …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/auth.ts(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
🔇 Additional comments (1)
apps/web/utils/auth.ts (1)
470-470:Details
✅ Verification successful
Update default provider value to match naming convention.
The default provider value should be updated to match the corrected naming convention if the Microsoft provider naming inconsistency is fixed.
🏁 Script executed:
#!/bin/bash # Description: Check how the provider field is used throughout the codebase # Expected: Verify if "google" or "microsoft" values are used consistently echo "=== Checking provider field usage ===" rg -A 3 -B 3 'provider.*=.*"(google|microsoft)"' echo -e "\n=== Checking provider comparisons ===" rg -A 3 -B 3 'provider.*===.*"(google|microsoft)"' echo -e "\n=== Checking database schema for provider field ===" fd -e prisma -e sql | xargs rg -A 5 -B 5 providerLength of output: 6768
No updates required for default provider at this time
A quick audit confirms that the
providerfield is consistently"google"or"microsoft"across:
apps/web/utils/auth.ts(default param and comparisons)apps/web/utils/actions/permissions.ts(permission check)- Prisma schema & migration files
Since the Microsoft provider naming remains unchanged in this PR, retaining
provider = "google"is correct. We can revisit this default once the Microsoft naming convention is updated.
| const PROVIDER_CONFIG = { | ||
| google: { | ||
| name: "google", | ||
| tokenUrl: "https://oauth2.googleapis.com/token", | ||
| getProfileData: async (accessToken: string) => { | ||
| const contactsClient = getGoogleContactsClient({ accessToken }); | ||
| const profileResponse = await contactsClient.people.get({ | ||
| resourceName: "people/me", | ||
| personFields: "emailAddresses,names,photos", | ||
| }); | ||
|
|
||
| return { | ||
| email: profileResponse.data.emailAddresses | ||
| ?.find((e) => e.metadata?.primary) | ||
| ?.value?.toLowerCase(), | ||
| name: profileResponse.data.names?.find((n) => n.metadata?.primary) | ||
| ?.displayName, | ||
| image: profileResponse.data.photos?.find((p) => p.metadata?.primary) | ||
| ?.url, | ||
| }; | ||
| }, | ||
| }, | ||
| "microsoft-entra-id": { | ||
| name: "microsoft", | ||
| tokenUrl: "https://login.microsoftonline.com/common/oauth2/v2.0/token", | ||
| getProfileData: async (accessToken: string) => { | ||
| const client = getOutlookContactsClient({ accessToken }); | ||
| try { | ||
| const profileResponse = await client.getUserProfile(); | ||
| console.log("profileResponse", profileResponse); | ||
|
|
||
| // Get photo separately as it requires a different endpoint | ||
| let photoUrl = null; | ||
| try { | ||
| const photo = await client.getUserPhoto(); | ||
| console.log("photoRESPONSE", photo); | ||
| if (photo) { | ||
| photoUrl = photo; | ||
| } | ||
| } catch (error) { | ||
| logger.info("User has no profile photo", { error }); | ||
| } | ||
|
|
||
| return { | ||
| email: | ||
| profileResponse.mail?.toLowerCase() || | ||
| profileResponse.userPrincipalName?.toLowerCase(), | ||
| name: profileResponse.displayName, | ||
| image: photoUrl, | ||
| }; | ||
| } catch (error) { | ||
| logger.error("Error fetching Microsoft profile data", { error }); | ||
| throw error; | ||
| } | ||
| }, | ||
| }, | ||
| } as const; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling for the provider configuration.
The PROVIDER_CONFIG object lacks type safety and could fail at runtime with unknown providers. Consider these improvements:
- Add type definitions:
+type ProviderName = 'google' | 'microsoft-entra-id';
+type ProviderConfig = {
+ name: string;
+ tokenUrl: string;
+ getProfileData: (accessToken: string) => Promise<{
+ email?: string;
+ name?: string;
+ image?: string | null;
+ }>;
+};
-const PROVIDER_CONFIG = {
+const PROVIDER_CONFIG: Record<ProviderName, ProviderConfig> = {- Add runtime validation:
const provider =
- PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG];
+ PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG];
+if (!provider) {
+ throw new Error(`Unsupported provider: ${data.provider}`);
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/utils/auth.ts between lines 24 and 80, the PROVIDER_CONFIG object
lacks explicit type definitions and runtime validation, which risks runtime
errors with unknown providers. Define a TypeScript interface or type for the
provider configuration structure, including the name, tokenUrl, and
getProfileData method, and apply it to PROVIDER_CONFIG for compile-time safety.
Additionally, implement runtime validation to check if a requested provider
exists in PROVIDER_CONFIG before usage, throwing a clear error if not found, to
prevent unexpected failures.
| const provider = | ||
| PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG]; | ||
| const profileData = await provider.getProfileData(data.access_token); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for unsupported providers.
The provider lookup could fail silently if an unsupported provider is passed. Add validation to prevent runtime errors.
const provider =
PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG];
+if (!provider) {
+ logger.error("[linkAccount] Unsupported provider", { provider: data.provider });
+ throw new Error(`Unsupported provider: ${data.provider}`);
+}
const profileData = await provider.getProfileData(data.access_token);📝 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.
| const provider = | |
| PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG]; | |
| const profileData = await provider.getProfileData(data.access_token); | |
| const provider = | |
| PROVIDER_CONFIG[data.provider as keyof typeof PROVIDER_CONFIG]; | |
| if (!provider) { | |
| logger.error("[linkAccount] Unsupported provider", { provider: data.provider }); | |
| throw new Error(`Unsupported provider: ${data.provider}`); | |
| } | |
| const profileData = await provider.getProfileData(data.access_token); |
🤖 Prompt for AI Agents
In apps/web/utils/auth.ts around lines 143 to 145, the code retrieves a provider
from PROVIDER_CONFIG without checking if the provider exists, which can cause
runtime errors if an unsupported provider is passed. Add validation after the
provider lookup to check if the provider is undefined or invalid, and handle
this case by throwing an error or returning a meaningful response to prevent
silent failures.
| "microsoft-entra-id": { | ||
| name: "microsoft", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix provider naming inconsistency.
The provider key is "microsoft-entra-id" but the name property is "microsoft". This inconsistency could cause issues when the provider name is used for database lookups or comparisons.
"microsoft-entra-id": {
- name: "microsoft",
+ name: "microsoft-entra-id",📝 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.
| "microsoft-entra-id": { | |
| name: "microsoft", | |
| "microsoft-entra-id": { | |
| name: "microsoft-entra-id", |
🤖 Prompt for AI Agents
In apps/web/utils/auth.ts at lines 46 to 47, the provider key is
"microsoft-entra-id" but the name property is set to "microsoft", causing
inconsistency. Update the name property to match the key by changing it from
"microsoft" to "microsoft-entra-id" to ensure consistent naming for database
lookups and comparisons.
| private accessToken: string; | ||
|
|
||
| constructor(accessToken: string) { | ||
| this.accessToken = accessToken; |
There was a problem hiding this comment.
note, we need to handle token refreshes. the access token will be fairly short lived. with google it has an auto refresh mechanism. i don't know if there's a similar lib for microsoft that does the same thing
3 participants
Summary by CodeRabbit
New Features
Improvements
Documentation