fix: Two accounts with the same email #2642

aliraza-abbasi · 2023-05-05T13:12:15Z

Pre-Flight checklist

What are the relevant tickets?

#2633

What's this PR do?

Fixes the two account issue with one email #2633

How should this be manually tested?

Checkout to this branch and run xpro locally
Open Create Account on local xpro
Enter an email, with lowercase letters in the address
In another tab, open Create Account again and enter the same email, but all uppercase
You should get two verification emails, with different links.
Follow one of the links and complete the first screen of the account creation process
In a private window or another browser open the 2nd verification link and complete the first screen of the account creation process
You should see an error screen as the screenshot below on submit that a user already exists

Screenshots (if appropriate)

arslanashraf7 · 2023-05-11T12:40:58Z

authentication/pipeline/user.py

+    if User.objects.filter(email__iexact=data["email"]).exists():
+        raise InvalidEmail(backend)


What would happen if we throw UnexpectedExistingUserException here instead of InvalidEmail exception, I feel like throwing InvalidEmail might be a little too general because the email is valid & passes the validation but the problem is that it already exists.

The InvalidEmail exception is being handled here.

mitxpro/authentication/serializers.py

Line 92 in 333ab77

except InvalidEmail:

It returns the existing-user state in the API on the other hand UnexpectedExistingUserException returns the error state in the API and there is no error message. Here are screenshots where I raised UnexpectedExistingUserException.
Backend:

Frontend:

I was looking around and checking what might be the right exception to throw here and it looks like there is AuthAlreadyAssociated exception from social-core which we should throw here and add a catch statement in the serializer along with InvalidEmail.

arslanashraf7 · 2023-05-11T12:47:37Z

authentication/pipeline/user.py

-    if user:
-        updated_user = serializer.save()
-        return {"is_new": True, "user": updated_user, "username": updated_user.username}
-


In the above check where we are checking if it exists, Are we always going to get the user object in this step of the pipeline?

I added this check in the previous PR where we allowed a user to save his information again on Step 1 of the Registration Form. But after this issue is raised, we are not allowing users to go back to step 1 and save the information again. As we talked about before starting work on this issue. So, I've removed this check, because now we don't need this check here.

arslanashraf7 · 2023-05-11T12:49:49Z

authentication/serializers.py

+                        .strip()
+                        .lower()


If we have started to make the email lower in the pipeline step, Do we still need to again this lower function here ?

If we need it here, we should add test for this as well.

As mentioned in the above comment #2642 (comment). This is where the InvalidEmail exception is being handled. So adding .strip().lower() is required here to check if the user exists in the InvalidEmail Exception.

I've removed .strip().lower() from here and fixed the query to check the user with case insensitive email

arslanashraf7 · 2023-05-11T12:59:31Z

users/serializers.py

@@ -245,7 +245,7 @@ class UserSerializer(serializers.ModelSerializer):

    def validate_email(self, value):
        """Empty validation function, but this is required for WriteableSerializerMethodField"""
-        return {"email": value}
+        return {"email": value.strip().lower()}


Do we need to lower the email here too?

I added this check here if a user updates his/her email. It'll always be saved in lowercase. At the time of registration and on updating his/her account info

I've removed this .strip().lower() check from the pipeline so now it is definitely required here

arslanashraf7

I was able to test the PR and it seems to be working except one case that might confuse the user and we should try to fix that if possible. I also added a comment regarding throwing the right exception.

So there is a scenario when we get a duplicate username collision, I'm adding the logs for that below.

To get into this scenario:

Create a user with a an email address complete registration
Create another user with appended +1 in the email, and see the logs. The usernamify function generates the same username for this user as well and gets an error from DB.
On Master, The register form stays there and the Continue button is enabled again, When we click this it reattempts the user namify and this time create the right user. (If it's not reproduced with this step, try a manual intervention to create the username collision from usernamify function)
In case of this PR, When username collision happens, and user clicks the Continue button again it is redirected to the login page saying User already exists which can confuse the user. Maybe we should try to handle this case.

The error log:

db_1      | 2023-05-15 09:54:50.317 UTC [1120] ERROR:  duplicate key value violates unique constraint "users_user_username_key"
db_1      | 2023-05-15 09:54:50.317 UTC [1120] DETAIL:  Key (username)=(arslan-ashraf) already exists.
db_1      | 2023-05-15 09:54:50.317 UTC [1120] STATEMENT:  INSERT INTO "users_user" ("password", "last_login", "is_superuser", "created_on", "updated_on", "username", "email", "name", "is_staff", "is_active") VALUES ('<>, NULL, false, '2023-05-15T09:54:50.312660+00:00'::timestamptz, '2023-05-15T09:54:50.312681+00:00'::timestamptz, 'arslan-ashraf', 'arslan.ashraf+20@arbisoft.com', 'Arslan Ashraf20', false, false) RETURNING "users_user"."id"

arslanashraf7 · 2023-05-15T15:47:49Z

authentication/pipeline/user.py

+    if User.objects.filter(email__iexact=data["email"]).exists():
+        raise InvalidEmail(backend)


I was looking around and checking what might be the right exception to throw here and it looks like there is AuthAlreadyAssociated exception from social-core which we should throw here and add a catch statement in the serializer along with InvalidEmail.

aliraza-abbasi · 2023-05-18T13:19:07Z

I was able to test the PR and it seems to be working except one case that might confuse the user and we should try to fix that if possible. I also added a comment regarding throwing the right exception.

So there is a scenario when we get a duplicate username collision, I'm adding the logs for that below.

To get into this scenario:

1. Create a user with a an email address complete registration

2. Create another user with appended `+1` in the email, and see the logs. The usernamify function generates the same username for this user as well and gets an error from DB.

3. On Master, The register form stays there and the `Continue` button is enabled again, When we click this it reattempts the user namify and this time create the right user. (If it's not reproduced with this step, try a manual intervention to create the username collision from usernamify function)

4. In case of this PR, When username collision happens, and user clicks the `Continue` button again it is redirected to the login page saying `User already exists` which can confuse the user. Maybe we should try to handle this case.

The error log:

db_1      | 2023-05-15 09:54:50.317 UTC [1120] ERROR:  duplicate key value violates unique constraint "users_user_username_key"
db_1      | 2023-05-15 09:54:50.317 UTC [1120] DETAIL:  Key (username)=(arslan-ashraf) already exists.
db_1      | 2023-05-15 09:54:50.317 UTC [1120] STATEMENT:  INSERT INTO "users_user" ("password", "last_login", "is_superuser", "created_on", "updated_on", "username", "email", "name", "is_staff", "is_active") VALUES ('<>, NULL, false, '2023-05-15T09:54:50.312660+00:00'::timestamptz, '2023-05-15T09:54:50.312681+00:00'::timestamptz, 'arslan-ashraf', 'arslan.ashraf+20@arbisoft.com', 'Arslan Ashraf20', false, false) RETURNING "users_user"."id"

I have followed the steps provided, and I did not encounter the mentioned issue. Based on my understanding, this issue should not occur as a result of this PR. The PR specifically checks for existing users based on their email addresses, not their usernames.

arslanashraf7 · 2023-05-18T18:59:36Z

I was able to test the PR and it seems to be working except one case that might confuse the user and we should try to fix that if possible. I also added a comment regarding throwing the right exception.
So there is a scenario when we get a duplicate username collision, I'm adding the logs for that below.
To get into this scenario:

1. Create a user with a an email address complete registration

2. Create another user with appended `+1` in the email, and see the logs. The usernamify function generates the same username for this user as well and gets an error from DB.

3. On Master, The register form stays there and the `Continue` button is enabled again, When we click this it reattempts the user namify and this time create the right user. (If it's not reproduced with this step, try a manual intervention to create the username collision from usernamify function)

4. In case of this PR, When username collision happens, and user clicks the `Continue` button again it is redirected to the login page saying `User already exists` which can confuse the user. Maybe we should try to handle this case.

The error log:

db_1      | 2023-05-15 09:54:50.317 UTC [1120] ERROR:  duplicate key value violates unique constraint "users_user_username_key"
db_1      | 2023-05-15 09:54:50.317 UTC [1120] DETAIL:  Key (username)=(arslan-ashraf) already exists.
db_1      | 2023-05-15 09:54:50.317 UTC [1120] STATEMENT:  INSERT INTO "users_user" ("password", "last_login", "is_superuser", "created_on", "updated_on", "username", "email", "name", "is_staff", "is_active") VALUES ('<>, NULL, false, '2023-05-15T09:54:50.312660+00:00'::timestamptz, '2023-05-15T09:54:50.312681+00:00'::timestamptz, 'arslan-ashraf', 'arslan.ashraf+20@arbisoft.com', 'Arslan Ashraf20', false, false) RETURNING "users_user"."id"

I have followed the steps provided, and I did not encounter the mentioned issue. Based on my understanding, this issue should not occur as a result of this PR. The PR specifically checks for existing users based on their email addresses, not their usernames.

@aliraza-abbasi Sorry if I was unclear in my comment, I didn't mean that issue is being raised because of this PR, I'm saying that the behavior is changed in this PR when that issue raises:

In case of duplicate username collision, the previous implementations were used to handle it in a good manner. E.g. when a user submits the first page of the registration form and it the username collision happens, the user stays on the same page and when he clicks Continue again the user is created with an alternate username. So, the registration page 1/2 flow remains the same for the user.
On the other hand, While on this PR, The behavior is that when the user clicks on the Continue the first time but when he clicks the button again, We present them with a message that the user already exists message that can confuse the user.

aliraza-abbasi · 2023-05-18T22:09:09Z

@aliraza-abbasi Sorry if I was unclear in my comment, I didn't mean that issue is being raised because of this PR, I'm saying that the behavior is changed in this PR when that issue raises:

1. In case of duplicate username collision, the previous implementations were used to handle it in a good manner. E.g. when a user submits the first page of the registration form and it the username collision happens, the user stays on the same page and when he clicks Continue again the user is created with an alternate username. So, the registration page 1/2 flow remains the same for the user.

2. On the other hand, While on this PR, The behavior is that when the user clicks on the `Continue` the first time but when he clicks the button again, We present them with a message that the user already exists message that can confuse the user.

@arslanashraf7 My apologies for the confusion in my previous response. Clicking the "Continue" button again is equivalent to returning from Step 2 to Step 1. In this particular scenario I've done some testing and found a very sewer issue before making this PR, let's discuss it over a call tomorrow.

arslanashraf7

Looks good 👍 Just a couple of minor changes, Feel free to rebase/merge after that.

authentication/pipeline/user_test.py

authentication/pipeline/user.py

aliraza-abbasi added the Work in Progress label May 5, 2023

odlbot temporarily deployed to xpro-ci-pr-2642 May 5, 2023 13:13 Inactive

odlbot temporarily deployed to xpro-ci-pr-2642 May 5, 2023 13:56 Inactive

odlbot temporarily deployed to xpro-ci-pr-2642 May 8, 2023 10:34 Inactive

aliraza-abbasi force-pushed the aliraza/issue-2633-two-accounts-with-the-same-email branch from 0f0a72c to 159c110 Compare May 8, 2023 19:22

odlbot temporarily deployed to xpro-ci-pr-2642 May 8, 2023 19:23 Inactive

aliraza-abbasi added Needs Review and removed Work in Progress labels May 8, 2023

aliraza-abbasi force-pushed the aliraza/issue-2633-two-accounts-with-the-same-email branch from 159c110 to 7384709 Compare May 8, 2023 19:52

odlbot temporarily deployed to xpro-ci-pr-2642 May 8, 2023 19:53 Inactive

arslanashraf7 self-assigned this May 11, 2023

arslanashraf7 requested changes May 11, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels May 12, 2023

aliraza-abbasi force-pushed the aliraza/issue-2633-two-accounts-with-the-same-email branch from 7384709 to 2c02384 Compare May 12, 2023 11:36

odlbot temporarily deployed to xpro-ci-pr-2642 May 12, 2023 11:36 Inactive

odlbot deployed to xpro-ci-pr-2642 May 12, 2023 11:41 View deployment

aliraza-abbasi requested review from arslanashraf7 May 12, 2023 12:17

aliraza-abbasi added Needs Review and removed Waiting on Author labels May 12, 2023

arslanashraf7 requested changes May 15, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels May 15, 2023

aliraza-abbasi force-pushed the aliraza/issue-2633-two-accounts-with-the-same-email branch from 0c530e5 to 934e5de Compare May 18, 2023 13:21

aliraza-abbasi added Needs Review and removed Waiting on Author labels May 18, 2023

arslanashraf7 approved these changes May 22, 2023

View reviewed changes

authentication/pipeline/user_test.py Outdated Show resolved Hide resolved

authentication/pipeline/user.py Outdated Show resolved Hide resolved

arslanashraf7 added Waiting on Author and removed Needs Review labels May 22, 2023

aliraza-abbasi added 8 commits May 23, 2023 00:38

fix: Two accounts with the same email

b78c5e6

fix: black and flow fixes

f02a334

fix: black and tests fixes

33eddc1

fix: JS tests and minor fixes

ee5addf

fix: case insensitive email check if user exists

c361e48

fix: black formatting

9294de1

fix: reviewed changes

f889be4

fix: test docstring and unused import

9956f36

aliraza-abbasi force-pushed the aliraza/issue-2633-two-accounts-with-the-same-email branch from 934e5de to 9956f36 Compare May 22, 2023 20:18

aliraza-abbasi merged commit a0bea10 into master May 22, 2023

aliraza-abbasi deleted the aliraza/issue-2633-two-accounts-with-the-same-email branch May 22, 2023 22:00

This was referenced May 25, 2023

Release 0.123.1 #2657

Closed

Release 0.124.0 #2663

Closed

odlbot mentioned this pull request Jun 2, 2023

Release 0.124.0 #2667

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Two accounts with the same email #2642

fix: Two accounts with the same email #2642

aliraza-abbasi commented May 5, 2023 •

edited

Loading

arslanashraf7 May 11, 2023

aliraza-abbasi May 12, 2023

arslanashraf7 May 15, 2023

arslanashraf7 May 11, 2023

aliraza-abbasi May 12, 2023

arslanashraf7 May 11, 2023

arslanashraf7 May 11, 2023

aliraza-abbasi May 12, 2023

aliraza-abbasi May 12, 2023 •

edited

Loading

arslanashraf7 May 11, 2023

aliraza-abbasi May 12, 2023

aliraza-abbasi May 12, 2023

arslanashraf7 left a comment

arslanashraf7 May 15, 2023

aliraza-abbasi commented May 18, 2023

arslanashraf7 commented May 18, 2023

aliraza-abbasi commented May 18, 2023

arslanashraf7 left a comment

		if User.objects.filter(email__iexact=data["email"]).exists():
		raise InvalidEmail(backend)

fix: Two accounts with the same email #2642

fix: Two accounts with the same email #2642

Conversation

aliraza-abbasi commented May 5, 2023 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots (if appropriate)

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

aliraza-abbasi May 12, 2023 • edited Loading

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

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliraza-abbasi commented May 18, 2023

arslanashraf7 commented May 18, 2023

aliraza-abbasi commented May 18, 2023

arslanashraf7 left a comment

Choose a reason for hiding this comment

aliraza-abbasi commented May 5, 2023 •

edited

Loading

aliraza-abbasi May 12, 2023 •

edited

Loading