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.

Sign up for GitHub

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebb509b
Reject login when external oauth login matches a different account
ayush-goyal Nov 28, 2020
1de559b
Fix email mismatch error messages
ayush-goyal Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/auth/strategies/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

done(null, false, { message: "Login method is linked to another email. Please try again with a different email address." });
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

return;
}

if (session && session.email && session.firstName && session.lastName) {
let signupEmail = session.email.trim().toLowerCase();
// Only create / modify user account if email and name exist on the session (set by login page)
Expand All @@ -36,18 +42,19 @@ export async function ExternalServiceCallback(
if (!user.services) {
user.services = {};
}

if (!user.services[serviceName]) {
user.services[serviceName] = {
id,
email: serviceEmail,
username
};
}

try {
user.markModified("services");
await user.save();
}
catch (err) {
} catch (err) {
done(err);
return;
}
Expand Down Expand Up @@ -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." });
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. Just tell people that, or
  2. 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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/routes/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ apiRoutes.get("/login-type", async (request, response) => {
});
});

apiRoutes.post("/signup-data", postParser, (request, response) => {
apiRoutes.post("/attach-session-data", postParser, (request, response) => {
function attachToSession(bodyProperty: keyof UserSessionData) {
if (!request.session) return;

Expand Down
19 changes: 12 additions & 7 deletions src/static/js/login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,19 @@ function setUpStep(step) {
}

if (!passwordLogin.value) {
await fetch(`/api/attach-session-data`, {
...commonFetchSettings,
body: serializeQueryString({
email: email.value.trim()
})
});

let { type } = await fetch(`/api/login-type?email=${encodeURIComponent(email.value.trim())}`).then(response => response.json());
if (["gatech", "google", "github", "facebook"].includes(type)) {
window.location.href = `/auth/${type}`;
return;
}

if (type === "local") {
let passwordContainer = document.getElementById("hidden-password");
passwordContainer.classList.remove("hidden");
Expand All @@ -77,12 +85,8 @@ function setUpStep(step) {
passwordLogin.focus();
return;
}
await fetch(`/api/signup-data`, {
...commonFetchSettings,
body: serializeQueryString({ email: email.value.trim() })
});
}
else {
} else {
// User has entered a value in the password field, so try login via local strategy
await fetch(`/auth/login`, {
...commonFetchSettings,
body: serializeQueryString({
Expand All @@ -102,7 +106,8 @@ function setUpStep(step) {
errorBlock.textContent = "Please enter your first and last name. We use it to identify you online and at events!"
return;
}
await fetch(`/api/signup-data`, {

await fetch(`/api/attach-session-data`, {
...commonFetchSettings,
body: serializeQueryString({
firstName: firstNameValue,
Expand Down