-
Notifications
You must be signed in to change notification settings - Fork 11.7k
feat: enable microsoft sign ups #20679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@retrogtx is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (04/13/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (04/13/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mrge found 16 issues across 16 files. View them in mrge.io
apps/web/public/microsoft-logo.svg
Outdated
| @@ -0,0 +1,6 @@ | |||
| <svg width="100" height="100" viewBox="0 0 512 512" xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SVG lacks accessibility attributes (title and description) that would make it accessible to screen readers
apps/web/modules/signup-view.tsx
Outdated
| {t("continue_with_google")} | ||
| </Button> | ||
| ) : null} | ||
| {/* TODO replace true with isMicrosoftLoginEnabled */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded 'true' value instead of using the isMicrosoftLoginEnabled prop
apps/web/modules/signup-view.tsx
Outdated
| {t("continue_with_google")} | ||
| </Button> | ||
| ) : null} | ||
| {/* TODO replace true with isMicrosoftLoginEnabled */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment left in production code
|
@cubic-dev-ai review this PR |
@Ryukemeister I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 19 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/auth/lib/next-auth-options.ts">
<violation number="1" location="packages/features/auth/lib/next-auth-options.ts:998">
P1: Rule violated: **Avoid Logging Sensitive Information**
Logging the entire `user` object exposes PII (email) in error logs. Error-level logs are captured in production and could leak sensitive user information. Consider logging only non-PII identifiers or hashing the email for log correlation.</violation>
<violation number="2" location="packages/features/auth/lib/next-auth-options.ts:1250">
P1: Security: Missing SAML tenant validation when converting from GOOGLE to SAML. Other SAML conversions in this file validate that the SAML IdP has domain authority via `validateSamlAccountConversion()`. Without this check, any SAML provider could potentially hijack a Google-authenticated account.</violation>
</file>
<file name="apps/web/modules/auth/login-view.tsx">
<violation number="1" location="apps/web/modules/auth/login-view.tsx:44">
P3: Localize the Microsoft icon alt text instead of hardcoding English so it respects the app’s i18n requirements.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai can you update the PR description? |
|
I've reviewed the PR changes and prepared an updated description. Since I cannot directly modify the GitHub PR body, here's a comprehensive updated description you can copy to replace the current one: What does this PR do?Enables Microsoft authentication for Cal.com, allowing users to sign up and log in using their Microsoft accounts (Azure AD). This builds upon #16873 by @hbjORbj. Key ChangesAuthentication:
UI:
Profile Features:
Infrastructure:
Environment VariablesOUTLOOK_LOGIN_ENABLED=false
OUTLOOK_API_CREDENTIALS= # JSON credentials from Azure portal
OUTLOOK_CLIENT_ID=
OUTLOOK_CLIENT_SECRET=
AZURE_AD_TENANT_ID= # Use "common" for multi-tenantHow to Test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/auth/lib/next-auth-options.ts">
<violation number="1" location="packages/features/auth/lib/next-auth-options.ts:877">
P1: Rule violated: **Avoid Logging Sensitive Information**
Do not log the full `existingUser` object because it includes PII (email, name, etc.). Log only non-sensitive identifiers instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| orgRole = membership?.role; | ||
| } | ||
|
|
||
| // Don't spread ...token here - it may contain large OAuth tokens (access_token, refresh_token, id_token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was causing outlook login to crash because of cookie overload
| return `/auth/error?error=wrong-provider&provider=${existingUserWithEmail.identityProvider}`; | ||
| } else if ( | ||
| existingUserWithEmail.identityProvider === IdentityProvider.GOOGLE && | ||
| idP === IdentityProvider.SAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic rightly pointed out this was a bug
hbjORbj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting to draft as it needs some design refinements discussed internally
What does this PR do?
Enables Microsoft authentication for Cal.com, allowing users to sign up and log in using their Microsoft accounts (Azure AD). This builds upon #16873 by @hbjORbj.
The flow is exactly like google except for one case, in google we add google meet as a conferencing tool. however in microsoft we don't add a conferencing tool, mainly for two reasons: 1. it requires additional permissions unlike google meet 2. MS teams which is conferencing tool for microsoft is only available for school and org accounts unlike meet which is available for all accounts be it free or org.
Fixes #27060
Visual Demo:
signin.with.outlook.flow.mov
Key Changes
Authentication:
IdentityProvider.AZUREADenum value with database migrationxms_edovclaim validation for email domain verification (handles work/school and personal accounts)azure-adproviderUI:
Profile Features:
Infrastructure:
OUTLOOK_LOGIN_ENABLED,OUTLOOK_API_CREDENTIALS,AZURE_AD_TENANT_ID"microsoft"→signIn("azure-ad")Environment Variables
How to Test
{WEBAPP_URL}/api/auth/callback/azure-adOUTLOOK_LOGIN_ENABLED=true-- Added by @sean-brydon
Enable microsoft SALM google etc
Figma: https://www.figma.com/design/wleA2SR6rn60EK7ORxAfMy/Cal.com-New-Features?node-id=12-61094&m=dev