Reject login when external oauth login matches a different account #34

ayush-goyal · 2020-12-05T22:37:15Z

Closes #31

petschekr

LGTM aside from the wording of some of the error messages

petschekr · 2020-12-08T02:59:50Z

src/auth/strategies/util.ts

@@ -81,7 +88,7 @@ export async function ExternalServiceCallback(
    }

    if (!user) {
-        done(null, false, { "message": "Could not match login to existing account" });
+        done(null, false, { message: "Could not match login to an existing account. Please try again with a different login method." });


Kinda picky but this can also be triggered by someone choosing the wrong account in the account picker (i.e. for Google which allows multiple user accounts to be logged in) but if there isn't a Ground Truth account with that other account. So I'd say Please try again with a different account or login method instead.

Honestly, if the third-party IdP email doesn't correspond to the email they entered originally, we should:

Just tell people that, or

Change the UX so you can directly pick 3rd-party IdPs on the initial login screen (which may have other implications I'm not thinking of right now)

I don't know if picking 3rd party IdPs directly is an option anymore, since there are so many accounts whose 3rd party IdP email doesn't match their main email address on their account, so it wouldn't work.

petschekr · 2020-12-08T03:11:52Z

src/auth/strategies/util.ts

@@ -25,6 +25,12 @@ export async function ExternalServiceCallback(
    // If `user` exists, the user has already logged in with this service and is good-to-go
    let user = await User.findOne({ [`services.${serviceName}.id`]: id });

+    // Ensure that the oauth method used is linked to the user with the same email address that is entered
+    if (session.email && user && session.email !== user.email) {
+        done(null, false, { message: "Login method is linked to another email. Please try again with a different email address." });


This message is kind of confusing imo. Perhaps The 3rd party account you chose does not match the email address that you are trying to log in with. Please make sure your email address and the selected account match.

Adding to this, users will be confused by this if we aren't really clear. So it would be good to include the original email the user entered in the error message to. As in, if I enter [email protected] on the initial login screen, then choose a Google account with the email [email protected], the error message might show, "The Google account you signed in with ([email protected]) has a different email than what you originally entered ([email protected]). Please ensure your email address and the account you select have the same email address.

If someone uses their gt email, [email protected], but chooses to sign in with google ([email protected]), and on the first screen they enter their google email, how should the error message read?

evan10s

I think this is a great start, but I have a few concerns we need to address:

I am almost certain there are a few users who will be locked out if we merge this change as-is (I can probably find you an example account). That means we need to check the database for accounts where the account email != the third-party IdP email, or if we can't do that, then we ought to develop a way for those users to rectify the problem.
Do these changes also apply when someone is making a new account with a third-party IdP? Or just new sign-ins?

evan10s · 2020-12-08T03:49:08Z

src/auth/strategies/util.ts

@@ -25,6 +25,12 @@ export async function ExternalServiceCallback(
    // If `user` exists, the user has already logged in with this service and is good-to-go
    let user = await User.findOne({ [`services.${serviceName}.id`]: id });

+    // Ensure that the oauth method used is linked to the user with the same email address that is entered
+    if (session.email && user && session.email !== user.email) {
+        done(null, false, { message: "Login method is linked to another email. Please try again with a different email address." });


Adding to this, users will be confused by this if we aren't really clear. So it would be good to include the original email the user entered in the error message to. As in, if I enter [email protected] on the initial login screen, then choose a Google account with the email [email protected], the error message might show, "The Google account you signed in with ([email protected]) has a different email than what you originally entered ([email protected]). Please ensure your email address and the account you select have the same email address.

evan10s · 2020-12-08T03:49:51Z

src/auth/strategies/util.ts

@@ -25,6 +25,12 @@ export async function ExternalServiceCallback(
    // If `user` exists, the user has already logged in with this service and is good-to-go
    let user = await User.findOne({ [`services.${serviceName}.id`]: id });

+    // Ensure that the oauth method used is linked to the user with the same email address that is entered
+    if (session.email && user && session.email !== user.email) {


Make sure we're using the trimmed and lowercased email for all comparisons.

evan10s · 2020-12-08T03:51:17Z

src/auth/strategies/util.ts

@@ -81,7 +88,7 @@ export async function ExternalServiceCallback(
    }

    if (!user) {
-        done(null, false, { "message": "Could not match login to existing account" });
+        done(null, false, { message: "Could not match login to an existing account. Please try again with a different login method." });


Honestly, if the third-party IdP email doesn't correspond to the email they entered originally, we should:

Just tell people that, or

Change the UX so you can directly pick 3rd-party IdPs on the initial login screen (which may have other implications I'm not thinking of right now)

ayush-goyal · 2020-12-09T00:24:55Z

So it's a little weird how it works, for every user, there is an email field, and then each 3rd party IdP they use also has an associated email in the db. These two emails can be different.

So when the user enters their email on the first screen, the system finds a user object with the corresponding email. Then, based on this, they are redirected to the corresponding service based on what they use. Once they login, they are essentially logged in based on their 3rd part IdP.

This check that I implemented, it only makes sure that the initial email they enter matches the email address of the user which they select from the IdP.

For example, if a user has an account with email address [email protected], but uses GitHub login on which service their email is [email protected], they must enter [email protected] on the first login screen. That's essentially what my check is doing

evan10s

It sounds like the check you're doing is what makes the most sense here, just a small thing with the error message wording

evan10s · 2020-12-29T20:13:47Z

src/auth/strategies/util.ts

+
+    // Ensure that the oauth method used is linked to the user with the same email address that is entered
+    if (enteredEmail && user && enteredEmail !== user.email) {
+        done(null, false, { message: "The 3rd party account you chose does not match the email address that you are trying to log in with. Please make sure your email address and the selected account match." });


Can you clarify the exact expectation in the error message by adding what aspect of the third-party account doesn't match (you mention the email address the user entered but the rest is vague)?

Basically, I want them to login with a different 3rd party account. So Ill just tell them that. Let me know if this is good instead.

The {serviceName} account you selected is linked to a different email address than typed below. Please try again with a different 3rd party account.

Where service name is google, github, etc.

Reject login when external oauth login matches a different account

ebb509b

ayush-goyal requested review from rahulrajus and evan10s December 5, 2020 22:37

hackgt-beekeeper bot temporarily deployed to login-dev December 5, 2020 22:38 Inactive

petschekr approved these changes Dec 8, 2020

View reviewed changes

evan10s requested changes Dec 8, 2020

View reviewed changes

Fix email mismatch error messages

1de559b

hackgt-beekeeper bot temporarily deployed to login-dev December 9, 2020 01:35 Inactive

evan10s reviewed Dec 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject login when external oauth login matches a different account #34

Reject login when external oauth login matches a different account #34

ayush-goyal commented Dec 5, 2020

petschekr left a comment

petschekr Dec 8, 2020

evan10s Dec 8, 2020

ayush-goyal Dec 9, 2020

petschekr Dec 8, 2020

evan10s Dec 8, 2020

ayush-goyal Dec 9, 2020

evan10s left a comment

evan10s Dec 8, 2020

evan10s Dec 8, 2020

evan10s Dec 8, 2020

ayush-goyal commented Dec 9, 2020

evan10s left a comment

evan10s Dec 29, 2020

ayush-goyal Dec 29, 2020

Reject login when external oauth login matches a different account #34

Are you sure you want to change the base?

Reject login when external oauth login matches a different account #34

Conversation

ayush-goyal commented Dec 5, 2020

petschekr 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

evan10s 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

ayush-goyal commented Dec 9, 2020

evan10s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment