chore: Add logging to SAML endpoints #22968
Conversation
WalkthroughThe changes introduce structured logging and enhanced error traceability to multiple authentication-related API route handlers within the application. Logger instances with descriptive prefixes are initialized in OIDC and various SAML handlers. Error handling is improved by generating unique trace IDs using UUIDs and logging detailed error information along with contextual data such as tenant identifiers, client IDs, and RelayState values. Some error responses now include these trace IDs to assist in debugging. Additionally, request parameter extraction is refactored for clarity in some handlers. No changes were made to the signatures of exported or public entities. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -30,6 +33,7 @@ async function handler(req: NextRequest) { | |||
|
|
|||
| return NextResponse.redirect(redirect_url, 302); | |||
| } catch (err) { | |||
| log.error(`Error authorizing tenant ${tenant}: ${err}`); | |||
There was a problem hiding this comment.
Tenant contains the org id
| const uid = uuid(); | ||
| log.error( | ||
| `Error authenticating user with error ${error} for relayState ${requestData?.RelayState} trace:${uid}` | ||
| ); | ||
| return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 }); |
There was a problem hiding this comment.
Since we don't have any identifying information here, let's add a trace that the user can use when reaching out to support.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/08/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/app/api/auth/oidc/route.ts (1)
12-12: Fix typo in logger prefix.The logger prefix has a typo: "ODIC" should be "OIDC".
Apply this diff:
- const log = logger.getSubLogger({ prefix: ["[ODIC auth]"] }); + const log = logger.getSubLogger({ prefix: ["[OIDC auth]"] });apps/web/app/api/auth/saml/authorize/route.ts (1)
21-21: Fix typo in error message.There's a typo in the error log message: "initaiting" should be "initiating".
Apply this diff:
- log.error(`Error initaiting SAML login for tenant ${oAuthReq?.tenant}: ${err}`); + log.error(`Error initiating SAML login for tenant ${oAuthReq?.tenant}: ${err}`);apps/web/app/api/auth/saml/token/route.ts (2)
15-15: Good improvement: Added type safety to request parsing.Typing the parsed request data as
OAuthTokenReqimproves type safety and code clarity. Consider adding runtime validation for production robustness since this is a type assertion rather than runtime type checking.
17-24: Good error handling with traceability, but review information exposure.The try-catch implementation provides excellent traceability with UUID generation. However, consider the following:
- Potential sensitive information exposure: The client_id is logged, which might contain sensitive information depending on your security model.
- Error details in thrown error: The original error details are included in the thrown error message, potentially exposing internal system information to clients.
Consider this approach for better security:
} catch (error) { const uid = uuid(); - log.error(`Error getting auth token for client id ${oauthTokenReq?.client_id}: ${error} trace: ${uid}`); - throw new Error(`Error getting auth token with error ${error} trace: ${uid}`); + log.error(`Error getting auth token for client id ${oauthTokenReq?.client_id}: trace: ${uid}`, { error }); + throw new Error(`Error getting auth token. trace: ${uid}`); }This approach:
- Logs the full error details in structured format for internal debugging
- Provides a generic error message to clients while maintaining trace correlation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/app/api/auth/oidc/route.ts(2 hunks)apps/web/app/api/auth/saml/authorize/route.ts(1 hunks)apps/web/app/api/auth/saml/callback/route.ts(1 hunks)apps/web/app/api/auth/saml/token/route.ts(1 hunks)apps/web/app/api/auth/saml/userinfo/route.ts(1 hunks)packages/features/auth/lib/next-auth-options.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/app/api/auth/oidc/route.tspackages/features/auth/lib/next-auth-options.tsapps/web/app/api/auth/saml/token/route.tsapps/web/app/api/auth/saml/authorize/route.tsapps/web/app/api/auth/saml/callback/route.tsapps/web/app/api/auth/saml/userinfo/route.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/api/auth/oidc/route.tspackages/features/auth/lib/next-auth-options.tsapps/web/app/api/auth/saml/token/route.tsapps/web/app/api/auth/saml/authorize/route.tsapps/web/app/api/auth/saml/callback/route.tsapps/web/app/api/auth/saml/userinfo/route.ts
🧠 Learnings (2)
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
apps/web/app/api/auth/oidc/route.tsapps/web/app/api/auth/saml/authorize/route.ts
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/features/auth/lib/next-auth-options.tsapps/web/app/api/auth/saml/callback/route.ts
🧬 Code Graph Analysis (1)
apps/web/app/api/auth/saml/token/route.ts (3)
apps/web/app/api/parseRequestData.ts (1)
parseRequestData(30-51)packages/features/ee/sso/lib/jackson.ts (1)
OAuthTokenReq(16-16)apps/api/v2/src/lib/logger.bridge.ts (1)
error(77-79)
🔇 Additional comments (8)
packages/features/auth/lib/next-auth-options.ts (1)
34-34: LGTM! Good cleanup of duplicate import.Removing the duplicate import and organizing imports improves code maintainability.
apps/web/app/api/auth/oidc/route.ts (1)
16-16: LGTM! Good addition of contextual logging.Extracting the tenant parameter and including it in error logs provides valuable context for debugging authentication issues.
Also applies to: 36-36
apps/web/app/api/auth/saml/authorize/route.ts (1)
14-14: LGTM! Good refactoring of parameter extraction.Extracting OAuth request parameters upfront improves code structure and enables better error context logging.
Also applies to: 17-17
apps/web/app/api/auth/saml/callback/route.ts (2)
12-12: Excellent enhancement of error traceability.The addition of structured logging with unique trace IDs and contextual information (RelayState) significantly improves error diagnostics for SAML authentication issues.
Also applies to: 24-29
17-17: LGTM! Improved response handling.Destructuring both redirect_url and error enables better error handling from the SAML response parsing.
apps/web/app/api/auth/saml/userinfo/route.ts (1)
12-31: Excellent improvement to token extraction error handling.The addition of structured logging, safe parsing, and unique trace IDs significantly improves error diagnostics for token extraction issues.
apps/web/app/api/auth/saml/token/route.ts (2)
5-5: LGTM! Imports support the new logging functionality.The added imports for UUID generation and logging are appropriate for the enhanced error tracing and logging functionality being introduced.
Also applies to: 9-9
13-13: Good practice: Contextual sub-logger creation.The sub-logger with "[SAML token]" prefix provides clear context for log entries, making debugging and monitoring more effective.
| try { | ||
| const userInfo = await oauthController.userInfo(token); | ||
| return NextResponse.json(userInfo); | ||
| } catch (error) { | ||
| const uid = uuid(); | ||
| log.error(`trace: ${uid} Error getting user info from token: ${error}`); | ||
| throw new Error(`Error getting user info from token. trace: ${uid}`); | ||
| } |
There was a problem hiding this comment.
Fix error handling - don't throw unhandled errors.
The error handling throws a new Error after logging, which will result in an unhandled error in the API route. The error should be returned as an HTTP response instead.
Apply this diff to properly handle the error:
try {
const userInfo = await oauthController.userInfo(token);
return NextResponse.json(userInfo);
} catch (error) {
const uid = uuid();
log.error(`trace: ${uid} Error getting user info from token: ${error}`);
- throw new Error(`Error getting user info from token. trace: ${uid}`);
+ return NextResponse.json(
+ { message: `Error getting user info from token. trace: ${uid}` },
+ { status: 500 }
+ );
}📝 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.
| try { | |
| const userInfo = await oauthController.userInfo(token); | |
| return NextResponse.json(userInfo); | |
| } catch (error) { | |
| const uid = uuid(); | |
| log.error(`trace: ${uid} Error getting user info from token: ${error}`); | |
| throw new Error(`Error getting user info from token. trace: ${uid}`); | |
| } | |
| try { | |
| const userInfo = await oauthController.userInfo(token); | |
| return NextResponse.json(userInfo); | |
| } catch (error) { | |
| const uid = uuid(); | |
| log.error(`trace: ${uid} Error getting user info from token: ${error}`); | |
| return NextResponse.json( | |
| { message: `Error getting user info from token. trace: ${uid}` }, | |
| { status: 500 } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/api/auth/saml/userinfo/route.ts around lines 42 to 49, the
current error handling throws a new Error after logging, causing unhandled
errors in the API route. Instead of throwing, modify the catch block to return
an HTTP error response using NextResponse with an appropriate status code and
error message, including the trace ID for debugging.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
E2E results are ready! |
| const { oauthController } = await jackson(); | ||
| const token = extractAuthToken(req); |
There was a problem hiding this comment.
How about we create traceId once here and pass the traceId to extractAuthToken too. That makes the traceId more usable.
* Add logging to SAML endpoints * Update packages/features/auth/lib/next-auth-options.ts --------- Co-authored-by: Hariom Balhara <hariombalhara@gmail.com>
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?