feat: show dialog when changing email after using Google Login #9611

JaideepGuntupalli · 2023-06-18T12:37:18Z

What does this PR do?

Improves the process of changing email after using Google Sign-In. Shows dialog as mentioned in #9515. Also changes ID provider from GOOGLE to CAL. Also sends a reset password mail.

Fixes #9515

https://www.loom.com/share/a99b5442c1524f0e8dfe4e43bdfe9946?sid=7c355736-03d6-4c76-b1ed-85bae506fe99

PS: I have fixed the toast issues

Type of change

Bug fix (non-breaking change which fixes an issue)
Chore (refactoring code, technical debt, workflow improvements)
New feature (non-breaking change which adds functionality)

How should this be tested?

Login using Google
Change the email
Reset your password using mail
Try to log back in

Mandatory Tasks

Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Questions I still have?

Should I make a new mail to send them instead of the same forgot password mail? @PeerRich
Should I log out the user directly after changing the mail since it becomes a weird unauthorized error and then resulting the user to logout?

vercel · 2023-06-18T12:37:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 3, 2023 10:29pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Aug 3, 2023 10:29pm

vercel · 2023-06-18T12:37:24Z

@JaideepGuntupalli is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

…e-Login

github-actions · 2023-06-18T12:56:27Z

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

PeerRich · 2023-06-18T20:53:49Z

oh hell yeah. this is a super common issue we have and will ease the pain for our customer support team!

…e-Login

github-actions · 2023-06-19T15:56:05Z

Thank you for following the naming conventions! 🙏

…ription of the change

JaideepGuntupalli · 2023-06-19T17:05:28Z

oh hell yeah. this is a super common issue we have and will ease the pain for our customer support team!

@PeerRich I have made the change, now the user will be logged out as soon as the request to change the email goes through. I have also added clear messaging on the logout screen too.

…e-Login

emrysal

Fundamentally this is fine; ideally this should be modified to be google agnostic - this issue also affects SAML users for example - by generalising it (given some suggestions) this can easily be achieved. Let me know what you think 👍

packages/lib/serverConfig.ts

apps/web/public/static/locales/en/common.json

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

apps/web/pages/settings/my-account/profile.tsx

JaideepGuntupalli · 2023-06-21T11:50:15Z

Fundamentally this is fine; ideally this should be modified to be google agnostic - this issue also affects SAML users for example - by generalising it (given some suggestions) this can easily be achieved. Let me know what you think 👍

So shall I make it generalized so that not only people who log in with Google, every other user who makes their account using different methods than CAL will need to go through this process? @emrysal Shall I hold off on changes related to generalization till others give in their input?

Another concern I have with this approach is, disconnecting accounts from SAML will not cause any issues right?
PS: I have been going through the codebase for only the past 10 days so I have no clue how Teams and SAML logins work.

Udit-takkar

I still get unauthorised in this PR when i change email and reset password email is sent and then i am redirected to logout screen. I fixed this unauthorised bug here fix: email update bug #10306. you would resolve the conflicts and update the branch and we should only signout when changed other provider(like gmail) to CAL.
I know you removed the identity provider id in update profile but if i try to login with gmail ( the provider) after changing my provider to CAL. I am still able to login for some reason and the DB removes the password field and sets identity provider id.
E2E tests are failing

Left few more comments in the code

PeerRich · 2023-07-28T14:29:22Z

/bonus 10 @JaideepGuntupalli would you mind addressing the comments by @Udit-takkar and fix the small merge conflict?

JaideepGuntupalli · 2023-07-28T14:31:24Z

sure currently outside but will do by tomorrow. this week i hv been busy.

algora-pbc · 2023-07-28T16:09:03Z

A bonus of $10 has been added by PeerRich.
@JaideepGuntupalli: You will receive $10 once you implement the requested changes and your PR is merged.

JaideepGuntupalli · 2023-08-03T16:51:02Z

Hey @PeerRich @Udit-takkar, sorry for the delay. I am actually busy moving back to college. I will try to get this done as soon as I can.

# Conflicts: # apps/web/pages/auth/logout.tsx # apps/web/pages/settings/my-account/profile.tsx

zomars · 2023-08-03T19:14:01Z

Reviewing now...

zomars · 2023-08-03T19:19:12Z

Fixed conflicts. Removed delay as it's not needed. Made some type fixes.

zomars

Thank you for your contribution 🙏 Will block to make some adjustments

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

apps/web/pages/auth/logout.tsx

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

zomars · 2023-08-03T20:13:43Z

apps/web/pages/api/auth/forgot-password.ts

-      .status(201)
-      .json({ message: "If this email exists in our system, you should receive a Reset email." });
+    // Don't leak info about whether the user exists
+    if (!user) return res.status(201).json({ message: "password_reset_email_sent" });


We don't need to send a localized error message here. The client should handle it. Just double checked and we don't actually use this message in the frontend anyways.

zomars · 2023-08-03T20:14:26Z

apps/web/pages/auth/forgot-password/index.tsx

@@ -40,8 +38,6 @@ export default function ForgotPassword({ csrfToken }: { csrfToken: string }) {
      const json = await res.json();
      if (!res.ok) {
        setError(json);
-      } else if ("resetLink" in json) {


This was needed for forgot-password E2E test but we don't rely on this anymore

zomars · 2023-08-03T20:14:56Z

apps/web/pages/auth/logout.tsx

+  const message = () => {
+    if (props.query?.passReset === "true") return "reset_your_password";
+    if (props.query?.emailChange === "true") return "email_change";
+    return "hope_to_see_you_soon";
+  };


More legible than a ternary if you ask me. No need for constants either.

zomars · 2023-08-03T20:15:12Z

apps/web/public/static/locales/en/common.json

@@ -84,6 +84,7 @@
  "event_awaiting_approval_recurring": "A recurring event is waiting for your approval",
  "someone_requested_an_event": "Someone has requested to schedule an event on your calendar.",
  "someone_requested_password_reset": "Someone has requested a link to change your password.",
+  "password_reset_email_sent": "If this email exists in our system, you should receive a reset email.",


Restored the original message

zomars · 2023-08-03T20:15:36Z

packages/features/auth/lib/passwordResetRequest.ts

+    language: t,
+    user,
+    resetLink,
+  });


Removed the return as we don't need it anymore

zomars · 2023-08-03T20:16:22Z

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

-  const userToUpdate = await prisma.user.findUnique({
-    where: {
-      id: user.id,
-    },
-  });
-
-  if (!userToUpdate) {
-    throw new TRPCError({ code: "NOT_FOUND", message: "User not found" });
-  }


We already fetch on user context. This extra query is not needed.

zomars · 2023-08-03T20:17:02Z

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

@@ -98,12 +94,26 @@ export const updateProfileHandler = async ({ ctx, input }: UpdateProfileOptions)
      });
    }
  }
-  const hasEmailBeenChanged = userToUpdate.email !== data.email;
+  const hasEmailBeenChanged = data.email && user.email !== data.email;


This was true even if there was no email in data/input.

zomars · 2023-08-03T20:17:31Z

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

+  // check if we are changing email and identity provider is not CAL
+  const hasEmailChangedOnNonCalProvider =
+    hasEmailBeenChanged && user.identityProvider !== IdentityProvider.CAL;
+  const hasEmailChangedOnCalProvider = hasEmailBeenChanged && user.identityProvider === IdentityProvider.CAL;
+
+  if (hasEmailChangedOnNonCalProvider) {
+    // Only validate if we're changing email
+    data.identityProvider = IdentityProvider.CAL;
+    data.identityProviderId = null;
+  } else if (hasEmailChangedOnCalProvider) {
+    // when the email changes, the user needs to sign in again.
+    signOutUser = true;
+  }


Split the behavior before updating and added again after updating.

zomars · 2023-08-03T20:17:48Z

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts

+  if (hasEmailChangedOnNonCalProvider) {
+    // Because the email has changed, we are now attempting to use the CAL provider-
+    // which has no password yet. We have to send the reset password email.
+    await passwordResetRequest(updatedUser);
+    signOutUser = true;
+    passwordReset = true;
+  }


Here we handle after updating.

zomars

Tested and it's working locally. Thank you for your contribution 🙏

JaideepGuntupalli added 2 commits June 18, 2023 17:46

improvement: changing email after using Google Sign in

9ad908b

cleanup: improved serverConfig.ts to make it work with MAILHOG locally

d64cd8b

github-actions bot added Medium priority Created by Linear-GitHub Sync 🧹 Improvements Improvements to existing features. Mostly UX/UI labels Jun 18, 2023

Merge branch 'main' into improvement/changing-email-after-using-Googl…

5e81e06

…e-Login

vercel bot had a problem deploying to Preview – ui June 18, 2023 12:46 Failure

keithwillcode requested a review from a team June 18, 2023 19:04

PeerRich changed the title ~~Improvement: When changing email after using Google Login~~ feat: show dialog when changing email after using Google Login Jun 18, 2023

Merge branch 'main' into improvement/changing-email-after-using-Googl…

891ce0e

…e-Login

PeerRich requested a review from a team June 18, 2023 20:54

sean-brydon self-requested a review June 18, 2023 20:57

vercel bot had a problem deploying to Preview – ui June 18, 2023 20:58 Failure

Merge branch 'main' into improvement/changing-email-after-using-Googl…

18200cd

…e-Login

vercel bot had a problem deploying to Preview – ui June 18, 2023 21:49 Failure

Merge branch 'main' into improvement/changing-email-after-using-Googl…

eed01bb

…e-Login

vercel bot deployed to Preview – ui June 19, 2023 16:00 View deployment

update: logging out user after the change, improved comments and desc…

18f14c5

…ription of the change

vercel bot deployed to Preview – ui June 19, 2023 16:47 View deployment

update: improved logout messaging

9dc147f

vercel bot deployed to Preview – ui June 19, 2023 17:10 View deployment

Merge branch 'main' into improvement/changing-email-after-using-Googl…

5b28edc

…e-Login

vercel bot deployed to Preview – ui June 20, 2023 12:50 View deployment

emrysal previously requested changes Jun 21, 2023

View reviewed changes

Udit-takkar reviewed Jul 27, 2023

View reviewed changes

Merge branch 'main' into pr/9611

8d99914

# Conflicts: # apps/web/pages/auth/logout.tsx # apps/web/pages/settings/my-account/profile.tsx

zomars added 2 commits August 3, 2023 12:16

Update forgot-password.ts

3976d88

fix return type mismatch

5b1942c

vercel bot deployed to Preview – ui August 3, 2023 19:24 View deployment

fix: no need for useState here

6aefc6c

vercel bot deployed to Preview – ui August 3, 2023 19:33 View deployment

zomars requested changes Aug 3, 2023

View reviewed changes

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts Outdated Show resolved Hide resolved

apps/web/pages/auth/logout.tsx Outdated Show resolved Hide resolved

packages/trpc/server/routers/loggedInViewer/updateProfile.handler.ts Outdated Show resolved Hide resolved

zomars added 2 commits August 3, 2023 12:49

revert: and refactor profile update handler

27920f6

fix: don't double query the user

9f2ce61

vercel bot deployed to Preview – ui August 3, 2023 20:06 View deployment

refactor: dead code

1bbd9da

zomars reviewed Aug 3, 2023

View reviewed changes

packages/features/auth/lib/passwordResetRequest.ts

language: t,

user,

resetLink,

});

Copy link

Member

zomars Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the return as we don't need it anymore

zomars reviewed Aug 3, 2023

View reviewed changes

vercel bot deployed to Preview – ui August 3, 2023 20:55 View deployment

zomars approved these changes Aug 4, 2023

View reviewed changes

zomars merged commit 123ecf3 into calcom:main Aug 4, 2023
28 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show dialog when changing email after using Google Login #9611

feat: show dialog when changing email after using Google Login #9611

JaideepGuntupalli commented Jun 18, 2023

vercel bot commented Jun 18, 2023 •

edited

Loading

vercel bot commented Jun 18, 2023

github-actions bot commented Jun 18, 2023 •

edited

Loading

PeerRich commented Jun 18, 2023

github-actions bot commented Jun 19, 2023 •

edited

Loading

JaideepGuntupalli commented Jun 19, 2023

emrysal left a comment

JaideepGuntupalli commented Jun 21, 2023 •

edited

Loading

Udit-takkar left a comment •

edited

Loading

PeerRich commented Jul 28, 2023

JaideepGuntupalli commented Jul 28, 2023

algora-pbc bot commented Jul 28, 2023

JaideepGuntupalli commented Aug 3, 2023

zomars commented Aug 3, 2023

zomars commented Aug 3, 2023

zomars left a comment

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars Aug 3, 2023

zomars left a comment

feat: show dialog when changing email after using Google Login #9611

feat: show dialog when changing email after using Google Login #9611

Conversation

JaideepGuntupalli commented Jun 18, 2023

What does this PR do?

Type of change

How should this be tested?

Mandatory Tasks

Questions I still have?

vercel bot commented Jun 18, 2023 • edited Loading

vercel bot commented Jun 18, 2023

github-actions bot commented Jun 18, 2023 • edited Loading

📦 Next.js Bundle Analysis for @calcom/web

PeerRich commented Jun 18, 2023

github-actions bot commented Jun 19, 2023 • edited Loading

JaideepGuntupalli commented Jun 19, 2023

emrysal left a comment

Choose a reason for hiding this comment

JaideepGuntupalli commented Jun 21, 2023 • edited Loading

Udit-takkar left a comment • edited Loading

Choose a reason for hiding this comment

PeerRich commented Jul 28, 2023

JaideepGuntupalli commented Jul 28, 2023

algora-pbc bot commented Jul 28, 2023

JaideepGuntupalli commented Aug 3, 2023

zomars commented Aug 3, 2023

zomars commented Aug 3, 2023

zomars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zomars left a comment

Choose a reason for hiding this comment

vercel bot commented Jun 18, 2023 •

edited

Loading

github-actions bot commented Jun 18, 2023 •

edited

Loading

github-actions bot commented Jun 19, 2023 •

edited

Loading

JaideepGuntupalli commented Jun 21, 2023 •

edited

Loading

Udit-takkar left a comment •

edited

Loading