Skip to content
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

feat: Implement the ability to login via multiple means when signed up via google/non-cal providers #14082

Merged
merged 19 commits into from Apr 10, 2024

Conversation

asadath1395
Copy link
Contributor

@asadath1395 asadath1395 commented Mar 14, 2024

What does this PR do?

Fixes #13978

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How should this be tested?

Scenario 1

  1. Create an account using google
  2. Create account password by navigating to /settings/security/password
  3. Follow the instructions in email to create the password
  4. Now try logging in via google and email + password
  5. Try changing the email and you should be able to login via google and newly created email + password

Scenario 2

Verify the steps in this PR #10288

Scenario 3

  1. Follow Scenario 1
  2. Now go to /settings/my-account/profile and try disconnecting the linked account
    image
  3. Now when you login via google, new account should be created and you should no longer be able to login to your old account using google

Mandatory Tasks

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

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Mar 14, 2024
@graphite-app graphite-app bot requested a review from a team March 14, 2024 08:37
Copy link

vercel bot commented Mar 14, 2024

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

A member of the Team first needs to authorize it.

@asadath1395 asadath1395 marked this pull request as draft March 14, 2024 08:37
@github-actions github-actions bot added authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in High priority Created by Linear-GitHub Sync ✨ feature New feature or request labels Mar 14, 2024
Copy link
Contributor

github-actions bot commented Mar 14, 2024

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

Copy link

graphite-app bot commented Mar 14, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (03/14/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (03/14/24)

1 label was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (03/25/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

github-actions bot commented Mar 14, 2024

📦 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! 🙌

@github-actions github-actions bot added the ❗️ migrations contains migration files label Mar 18, 2024
@asadath1395 asadath1395 marked this pull request as ready for review March 25, 2024 06:07
@@ -57,7 +57,7 @@ inferSSRProps<typeof getServerSideProps> & WithNonceProps<{}>) {
.string()
.min(1, `${t("error_required_field")}`)
.email(`${t("enter_valid_email")}`),
password: !!totpEmail ? z.literal("") : z.string().min(1, `${t("error_required_field")}`),
...(!!totpEmail ? {} : { password: z.string().min(1, `${t("error_required_field")}`) }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug fixed in this PR #10288 resurfaced again, so fixed that too in this PR

@@ -106,23 +106,16 @@ const ProfileView = () => {
const updateProfileMutation = trpc.viewer.updateProfile.useMutation({
onSuccess: async (res) => {
await update(res);
// signout user only in case of password reset
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of legacy code when user created an account using google, then changed the email which send password reset code, the flow is changed in this PR

@@ -807,22 +807,6 @@ export const AUTH_OPTIONS: AuthOptions = {
},
});

// safely delete password from UserPassword table if it exists
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously when user created an account and then logged in via google then his password was deleted, now we no longer do that because we let users log in via google and email + password

@graphite-app graphite-app bot requested a review from a team March 25, 2024 06:35
@keithwillcode keithwillcode added this to the v4.0 milestone Mar 27, 2024
@asadath1395 asadath1395 changed the title feat: Implement the ability to add an account password when signed up via non-cal providers feat: Implement the ability to login via multiple means when signed up via google/non-cal providers Apr 1, 2024
@asadath1395
Copy link
Contributor Author

@CarinaWolli Do you have the bandwidth to review this PR? Or maybe delegate it to someone from the team. Thank you!

@zomars
Copy link
Member

zomars commented Apr 10, 2024

This seems like a nice improvement! Will start reviewing soon. 🙏🏽

@zomars zomars added the high-risk Requires approval by Foundation team label Apr 10, 2024
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Tested locally and worked fine. Cannot test google login on Vercel preview deployments. Tests and checks are passing. Will merge and test on Canary before releasing.

@zomars zomars merged commit 4565c33 into calcom:main Apr 10, 2024
31 of 38 checks passed
@@ -320,7 +316,7 @@ export const updateProfileHandler = async ({ ctx, input }: UpdateProfileOptions)
});
}

if (updatedUser && hasEmailChangedOnCalProvider) {
if (updatedUser) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Woops, not putting hasEmailBeenChanged here caused email verification emails to be sent on every user profile update 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oversight from my side, sorry about that. Thanks for fixing it :)

@zomars
Copy link
Member

zomars commented Apr 23, 2024

I just stumbled into an error in prod when trying to unlink a google account. Will post more details soon.

EDIT:

Here's the redacted stack trace:

 Error:  {"name":"PrismaClientValidationError","clientVersion":"5.4.2"}
Something went wrong Error: An error occured while querying the database.
    at redactError (/var/task/apps/web/.next/server/chunks/30267.js:1:621)
    ... 8 lines matching cause stack trace ...
    at async /var/task/node_modules/@sentry/nextjs/cjs/common/wrapApiHandlerWithSentry.js:153:35 {
  code: 'INTERNAL_SERVER_ERROR',
  name: 'TRPCError',
  [cause]: Error: An error occured while querying the database.
      at redactError (/var/task/apps/web/.next/server/chunks/30267.js:1:621)
      at /var/task/apps/web/.next/server/chunks/46279.js:1:1628
      at async callRecursive (file:///var/task/node_modules/@trpc/server/dist/unstable-core-do-not-import/procedureBuilder.mjs:125:32)
      at async procedure (file:///var/task/node_modules/@trpc/server/dist/unstable-core-do-not-import/procedureBuilder.mjs:155:24)
      at async inputToProcedureCall (file:///var/task/node_modules/@trpc/server/dist/unstable-core-do-not-import/http/resolveHTTPResponse.mjs:47:22)
      at async Promise.all (index 0)
      at async resolveHTTPResponse (file:///var/task/node_modules/@trpc/server/dist/unstable-core-do-not-import/http/resolveHTTPResponse.mjs:184:37)
      at async file:///var/task/node_modules/@trpc/server/dist/adapters/node-http/nodeHTTPRequestHandler.mjs:82:9
      at async file:///var/task/node_modules/@trpc/server/dist/adapters/next.mjs:47:9
      at async /var/task/node_modules/@sentry/nextjs/cjs/common/wrapApiHandlerWithSentry.js:153:35
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in community Created by Linear-GitHub Sync ✨ feature New feature or request High priority Created by Linear-GitHub Sync high-risk Requires approval by Foundation team ❗️ migrations contains migration files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-3259] Allow people who created account with Google to add an email login
4 participants