ef0b068d8ea6e9ff5582aeeb8626a139c54c654f max Mon Sep 14 09:08:07 2026 -0700 hgLogin: explain ORCID sign-ins, stop duplicate accounts, confirm typed addresses ORCID's OpenID Connect offers only the "openid" scope and carries no email claim, so every first ORCID sign-in landed on the generic "choose a username" page with no explanation of why, and typing an address that already belonged to an account silently created a second account sharing it. The "choose a username" page now says why a new account is always created. It is shown when the provider released no address at all, rather than when the provider is named "orcid", because a mirror can name a provider anything it likes in hg.conf (refs #38213). An address the user typed into that form is no longer taken on trust. If an activated account already holds it, the signup is refused and the user is pointed at a sign-in method that can show the address is theirs. Otherwise the account is created unactivated and the user is sent to confirm the address by mail, instead of being signed in next to a mail nobody had any reason to open. Clicking the provider again before confirming re-sends the link rather than signing in, and opening the link signs in an account that has no password, so a social-login user is not left on a login page with nothing to type. An address the provider itself released is trusted whether or not the provider set email_verified. CILogon leaves that flag at 0 even for a real institutional sign-in, and insisting on it would both force a mail round trip on every CILogon user and stop them ever reaching an account they already have (refs #38339). Also in this change: - confirmChangeEmail activates the account. Opening that link proves the mailbox is the user's, so an account made by a provider that released no address no longer stays unactivated even after its owner confirms an address properly. - an install with login.mailReturnAddr=NOEMAIL no longer tries to send an activation mail it has no way to deliver. - the activation token is drawn with makeRandomKey, the same source already used for the email-link login token and the OAuth state nonce. refs #38341, refs #38339 diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index 62daed5ebcc..3cd93d33a15 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -70,30 +70,31 @@ /* for earlyBotCheck() function at the beginning of main() */ #define delayFraction 1.0 /* standard penalty is 1.0 for most CGIs */ /* Forward declarations for functions used before their definitions. */ static void printSocialButtons(boolean dividerAbove, boolean dividerBelow, char *action); static void printEmailLinkButton(); static boolean emailLinkEnabled(); static boolean recovEmailChangeEnabled(); void changeRecovEmailPage(struct sqlConnection *conn); static void printUsernameNote(); void emailLinkPage(struct sqlConnection *conn); void displayLoginPage(struct sqlConnection *conn); void displayAccHelpPage(struct sqlConnection *conn); void completeAccountPage(struct sqlConnection *conn); void sendEmailLink(struct sqlConnection *conn); +static void loginAndReturn(struct sqlConnection *conn, char *userName, uint idx); /* ---- Global helper functions ---- */ char *browserName() /* Return the browser name like 'UCSC Genome Browser' */ { if isEmpty(cfgOption(CFG_LOGIN_BROWSER_NAME)) return cloneString("NULL_browserName"); else return cloneString(cfgOption(CFG_LOGIN_BROWSER_NAME)); } char *browserAddr() /* Return the browser address like 'http://genome.ucsc.edu' */ { if isEmpty(cfgOption(CFG_LOGIN_BROWSER_ADDR)) @@ -755,31 +756,36 @@ "%s?hgLogin.do.activateAccount=1&user=%s&token=%s\n", hgLoginUrl, cgiEncode(username), cgiEncode(encToken)); safef(subject, sizeof(subject),"%s account e-mail address confirmation", brwName); safef(msg, sizeof(msg), "Someone (probably you, from IP address %s) has requested an account %s with this e-mail address on the %s.\nTo confirm that this account really does belong to you on the %s, open this link in your browser:\n\n%s\n\nIf this is *not* you, do not follow the link. This confirmation code will expire in 7 days.\n\nIf this *is* you, after clicking the activation link, your new account gives you access to sessions you can create and name. Sessions allow you to save your Genome Browser screen configuration and share it with others with a link like https://genome.ucsc.edu/s/%s/YourSessionName\n\nFor more information on sessions, see our help page on the topic: https://genome.ucsc.edu/goldenPath/help/hgSessionHelp.html#Introduction\n\nAdditional resources:\nSubscribe to the Genome Browser Mailing List: https://groups.google.com/a/soe.ucsc.edu/group/genome-announce?hl=en\nGenome Browser User Guide: https://genome.ucsc.edu/goldenPath/help/hgTracksHelp.html\nTraining and Tutorials: https://genome.ucsc.edu/training/index.html\n\n%s\n%s", remoteAddr, username, brwName, brwName, activateURL, username, signature, returnAddr); sendActMailOut(email, subject, msg); } void setupNewAccount(struct sqlConnection *conn, char *email, char *username) /* Set up new user account and send activation mail to user */ { char query[256]; -char *token = generateRandomPassword(); +/* Draw the activation token from the same source as the other one-time tokens in this file + * (the email-link login token and the OAuth state nonce, both makeRandomKey) rather than from + * generateRandomPassword, whose output is far shorter and far less varied. The token is opaque + * -- it is hashed on the next line and only the hash is ever stored or mailed -- so nothing + * downstream depends on its shape. */ +char *token = makeRandomKey(128+33); char *tokenMD5 = generateTokenMD5(token); sqlSafef(query,sizeof(query), "UPDATE gbMembers SET lastUse=NOW(),emailToken='%s', emailTokenExpires=DATE_ADD(NOW(), INTERVAL 7 DAY), accountActivated='N' WHERE userName='%s'", tokenMD5, username ); sqlUpdate(conn, query); sendActivateMail(email, username, tokenMD5); return; } void printPwdEyeIcon(char *iconId, char *slashId) /* print a clickable eye icon as a normal sibling right after a password * input (not overlapping it); slashId is the <line> toggled to show * "hidden". No-op if disabled via hg.conf login.pwdEyeIcon. */ { @@ -882,30 +888,45 @@ char *username = cgiUsualString("user",""); /* Let the database decide whether the token is still current: setupNewAccount sets * emailTokenExpires seven days out and the activation mail says the code expires then, so a * link older than that must no longer work. An unknown user name gives NULL here, and an * account that has already been activated has an empty emailToken, so both fall through to * the same message as a wrong token. */ sqlSafef(query,sizeof(query), "SELECT emailToken FROM gbMembers WHERE userName='%s' AND emailTokenExpires > NOW()", username); char *emailToken = sqlQuickString(conn, query); if (isNotEmpty(emailToken) && sameString(emailToken, token)) { sqlSafef(query,sizeof(query), "UPDATE gbMembers SET lastUse=NOW(), dateActivated=NOW(), emailToken='', emailTokenExpires='', accountActivated='Y' WHERE userName='%s'", username); sqlUpdate(conn, query); + /* An account with no password was created by a social login, and the provider button is its + * only other way in. Opening this link proves the mailbox is theirs, which is the same + * proof the passwordless email link accepts (see emailLogin), so finish the job and sign + * them in rather than bouncing them to a login page with nothing to type. Accounts that do + * have a password keep the old behavior: they know it, and a mailed link should not be worth + * a session on its own. */ + sqlSafef(query, sizeof(query), + "SELECT * FROM gbMembers WHERE userName='%s' AND password=''", username); + struct gbMembers *m = gbMembersLoadByQuery(conn, query); + if (m != NULL) + { + loginAndReturn(conn, m->userName, m->idx); + gbMembersFree(&m); + return; + } freez(&errMsg); errMsg = cloneString("Your account has been activated."); } else { freez(&errMsg); errMsg = cloneString("This activation link is not valid, has expired, or has already " "been used."); } cartSetString(cart, "hgLogin_userName", username); displayLoginPage(conn); return; } @@ -1360,32 +1381,39 @@ freeMem(expected); if (!sigOk || isEmpty(user) || spc_email_isvalid(newEmail) == 0) { freez(&errMsg); errMsg = cloneString("This confirmation link is not valid or has already been used."); displayLoginPage(conn); return; } if (clock1() > atol(expStr)) { freez(&errMsg); errMsg = cloneString("This confirmation link has expired. Please request the change again."); displayLoginPage(conn); return; } +/* Opening this link proved the user reads mail at newEmail, which is exactly what activation + * asks for, so activate the account here too. Without this an account created by a social + * login that released no address (ORCID) stayed unactivated forever even after its owner + * confirmed an address the proper way, and so stayed invisible to email-link login and to + * auto-linking from another provider. Any activation token still outstanding pointed at the + * old address, so drop it rather than leave a stale link alive. */ sqlSafef(query, sizeof(query), - "UPDATE gbMembers SET email='%s', lastUse=NOW() WHERE userName='%s'", newEmail, user); + "UPDATE gbMembers SET email='%s', lastUse=NOW(), accountActivated='Y', " + "emailToken='', emailTokenExpires='' WHERE userName='%s'", newEmail, user); sqlUpdate(conn, query); /* Alert the previous address that the change happened, so a hijack is noticed. */ if (isNotEmpty(oldEmail) && differentWord(oldEmail, newEmail)) sendChangeEmailAlertMail(oldEmail, user, newEmail); char *encEmail = htmlEncode(newEmail); hPrintf("<div class=\"centeredContainer formBox\"><h2>%s</h2>", brwName); hPrintf("<h3>Your email address has been changed.</h3>"); hPrintf("<p>Your email address is now <b>%s</b>.</p></div>", encEmail); freeMem(encEmail); returnToURL(1500); } void confirmRecovEmail(struct sqlConnection *conn) /* Mark a recovery address confirmed. Reached by opening the signed link mailed to that address * (see sendRecovEmailConfirmMail); the signature and its expiry are the authorization, so this @@ -2293,65 +2321,105 @@ } static void linkIdentity(struct sqlConnection *conn, uint idx, struct oauthIdentity *id) /* Insert or refresh the gbMemberIdentity row linking idx to this provider identity. */ { char query[1024]; char *email = emptyForNull(id->email); sqlSafef(query, sizeof(query), "INSERT INTO gbMemberIdentity SET idx=%u, provider='%s', subject='%s', email='%s', " "created=NOW(), lastUse=NOW() " "ON DUPLICATE KEY UPDATE idx=%u, email='%s', lastUse=NOW()", idx, id->provider, id->subject, email, idx, email); sqlUpdate(conn, query); } +/* What we trust about an email address in a social login is that the *provider* released it, + * not that the provider set email_verified. CILogon leaves that flag at 0 even for a real + * institutional sign-in (#37984 note-50), and its addresses come from the university's own + * identity provider rather than from anything the person can type, so insisting on the flag + * would make every CILogon user confirm an address by mail and would still never let them reach + * an account they already have. What we do not trust is an address the *user* typed: either + * because the provider released none (ORCID releases only an ORCID iD, by design) or because + * they edited the one that was released. Those have to be confirmed by mail. + * To tighten this later, add the email_verified test back in the two places that call + * oauthAddressFromProvider() and in resolveIdentity's matching query; the flag is still carried + * through the cart in oauth_pending_email_verified, it is just not consulted. */ + +static boolean oauthAddressFromProvider(char *email) +/* TRUE when email is exactly the address the identity provider released, i.e. it came from the + * provider and not from the user typing into the form. */ +{ +char *released = cartUsualString(cart, "oauth_pending_email", ""); +return isNotEmpty(released) && isNotEmpty(email) && sameString(email, released); +} + void completeAccountPage(struct sqlConnection *conn) /* Ask a first-time social-login user to confirm a username (and email) for a new account. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *email = cartUsualString(cart, "oauth_pending_email", ""); char *name = cartUsualString(cart, "oauth_pending_name", ""); if (isEmpty(provider) || !pendingIdentityValid()) { clearPendingIdentity(); displayLoginPage(conn); return; } char *suggested = cartUsualString(cart, "hgLogin_userName", ""); if (isEmpty(suggested)) suggested = suggestUsername(conn, email, name); +/* Show back what the user typed, so an error does not wipe the address they have to correct; + * the provider's address is only the starting suggestion. */ +char *typedEmail = cartUsualString(cart, "hgLogin_email", ""); char *encSuggested = htmlEncode(suggested); // both go into value="" attributes; escape (XSS) -char *encEmail = htmlEncode(email); +char *encEmail = htmlEncode(isNotEmpty(typedEmail) ? typedEmail : email); +char *label = oauthProviderLabel(provider); hPrintf("<div id=\"completeAccountBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Choose a username</h3>"); hPrintf("<p>You signed in with %s. Pick a username for your new %s account. " - "You can change the suggested name below.</p>", - oauthProviderLabel(provider), brwName); + "You can change the suggested name below.</p>", label, brwName); +/* Explain why this is always a new account when the provider released no address at all (ORCID + * does this by design: its OpenID Connect offers only the "openid" scope, so the ORCID iD is all + * we ever get). Without an address we cannot tell a returning user from a new one, so every + * first sign-in lands here, which surprised real users (#38341). Keyed on whether an address + * arrived, not on the provider's name: a mirror can call a provider anything it likes in + * hg.conf, so a name test would silently miss it (#38213). */ +if (isEmpty(email)) + hPrintf("<p>A new %s account is created for any %s sign-in we have not seen before, because " + "%s does not share your email address with us. So you cannot sign in to an existing " + "account this way. Use another sign-in option if you do not want to create a new " + "account.</p>", brwName, label, label); printUsernameNote(); hPrintf("<span style='color:red;'>%s</span>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"completeAccountForm\">", hgLoginUrl); hPrintf("<div class=\"inputGroup\">" "<label for=\"userName\">Username</label>" "<input type=\"text\" name=\"hgLogin_userName\" value=\"%s\" size=\"30\" id=\"userName\">" "</div>", encSuggested); hPrintf("<div class=\"inputGroup\">" "<label for=\"emailAddr\">Email address</label>" "<input type=\"text\" name=\"hgLogin_email\" value=\"%s\" size=\"30\" id=\"emailAddr\">" "</div>", encEmail); +/* Say up front that the address has to be confirmed, so the confirmation page is not a surprise + * and people are less likely to type an address they cannot read. Same condition completeAccount + * uses to decide whether to send the mail. */ +if (!oauthAddressFromProvider(email) && !sameWord(returnAddr, "NOEMAIL")) + hPrintf("<p style=\"font-size:0.9em\">We will email a confirmation link to this address. " + "Open the link to finish creating your account.</p>"); hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.completeAccount\" value=\"Create account\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - completeAccountBox -->", getReturnToUrlForAttr()); cartSaveSession(cart); freeMem(encSuggested); freeMem(encEmail); } void completeAccount(struct sqlConnection *conn) /* Create the account for a first-time social-login user, link the identity, and log in. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *subject = cartUsualString(cart, "oauth_pending_subject", ""); if (isEmpty(provider) || isEmpty(subject) || !pendingIdentityValid()) @@ -2395,62 +2463,104 @@ char *email = cartUsualString(cart, "hgLogin_email", ""); if (isEmpty(email)) { freez(&errMsg); errMsg = cloneString("Please enter an email address."); completeAccountPage(conn); return; } if (spc_email_isvalid(email) == 0) { freez(&errMsg); errMsg = cloneString("Invalid email address format."); completeAccountPage(conn); return; } +boolean fromProvider = oauthAddressFromProvider(email); + +/* An address the user typed here must not be used to reach an account that already holds it. + * Typing an address someone else registered used to create a silent second account sharing it + * (#38341), which then makes both owners pick from a chooser on every later sign-in. Send the + * user to a sign-in method that can actually show the address is theirs instead. An address the + * provider released is not affected: resolveIdentity has already matched it against existing + * accounts and would not have sent us here. + * Only activated accounts count, the same rule resolveIdentity and chooseAccount apply: an + * unactivated row holds an address nobody ever proved they own, so letting one block a signup + * would let anyone reserve a stranger's address. */ +if (!fromProvider) + { + char query[1024]; + char *addrMatch = sqlAddressMatch(email); + sqlSafef(query, sizeof(query), + "SELECT count(*) FROM gbMembers WHERE %-s AND accountActivated='Y'", addrMatch); + freeMem(addrMatch); + if (sqlQuickNum(conn, query) > 0) + { + char buf[1024]; + safef(buf, sizeof(buf), + "An account with this email address already exists. %s did not give us that address, " + "so we cannot tell that it is yours and cannot sign you in to that account. To reach " + "it, sign in with a provider that does give us your email address, or with your " + "username and password. To create a new account instead, enter a different email " + "address.", oauthProviderLabel(provider)); + freez(&errMsg); + errMsg = cloneString(buf); + completeAccountPage(conn); + return; + } + } + char *name = cartUsualString(cart, "oauth_pending_name", ""); char *realName = isNotEmpty(name) ? name : user; /* The new account is created "activated" -- its email trusted for future auto-linking (see * resolveIdentity) -- only when the provider actually verified this address and the user kept - * it unchanged. If the address is unverified (the provider released none, e.g. ORCID, or the - * user typed a different one), create the account inactive and send the usual confirmation - * mail, so an unverified address can never be planted as a trusted one. The user still signs - * in now either way: their provider identity, not the email, is what logs them in. */ -char *verifiedEmail = cartUsualString(cart, "oauth_pending_email", ""); -boolean emailVerified = cartUsualBoolean(cart, "oauth_pending_email_verified", FALSE) - && isNotEmpty(verifiedEmail) && sameString(email, verifiedEmail); + * it unchanged. Otherwise it is created inactive and the usual confirmation mail goes out, so + * an unverified address can never be planted as a trusted one. An install that sends no mail + * has no way to confirm anything, so there it is activated on the spot, the same compromise + * signup() makes. */ +boolean canMail = !sameWord(returnAddr, "NOEMAIL"); +boolean activateNow = fromProvider || !canMail; struct dyString *q = sqlDyStringCreate( "INSERT INTO gbMembers SET userName='%s', realName='%s', password='', email='%s', " "lastUse=NOW(), dateActivated=NOW(), accountActivated='%s'", - user, realName, emptyForNull(email), emailVerified ? "Y" : "N"); + user, realName, emptyForNull(email), activateNow ? "Y" : "N"); sqlUpdate(conn, dyStringContents(q)); dyStringFree(&q); uint idx = sqlLastAutoId(conn); struct oauthIdentity pending; ZeroVar(&pending); pending.provider = provider; pending.subject = subject; pending.email = email; linkIdentity(conn, idx, &pending); clearPendingIdentity(); -if (!emailVerified) - setupNewAccount(conn, email, user); // send confirmation mail for the unverified address +if (activateNow) + { loginAndReturn(conn, user, idx); + return; + } +/* Unconfirmed address: send the confirmation mail and say so, rather than signing the user in + * and leaving a mail nobody has any reason to open. Activating is what makes the address usable + * for signing in by email link and for linking a later social login, so it is worth a click. */ +setupNewAccount(conn, email, user); +cartRemove(cart, "hgLogin_email"); +cartRemove(cart, "hgLogin_userName"); +redirectToLoginPage("hgLogin.do.displayActMailSuccess=1"); } void chooseAccountPage(struct sqlConnection *conn) /* Ask the user which of several accounts sharing an email address to sign in to. Used by * two flows: OAuth (oauth_pending_* in the cart -> the chosen account is linked to the social * identity) and the passwordless email link (emailLogin_* in the cart -> just sign in). */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); boolean emailMode = isEmpty(provider); if (emailMode && !emailLinkEnabled()) { // The email-link chooser must not run where passwordless login is switched off. displayLoginPage(conn); return; } @@ -2600,43 +2710,49 @@ } struct oauthIdentity pending; ZeroVar(&pending); pending.provider = provider; pending.subject = subject; pending.email = email; linkIdentity(conn, m->idx, &pending); clearPendingIdentity(); cartRemove(cart, "hgLogin_chosenIdx"); loginAndReturn(conn, m->userName, m->idx); gbMembersFree(&m); } static void resolveIdentity(struct sqlConnection *conn, struct oauthIdentity *id) /* Log in the user behind an authenticated provider identity: - * 1. If the provider gave a verified email matching MORE THAN ONE account, always let the + * 1. If the provider released an email matching MORE THAN ONE account, always let the * user pick which one -- even if this identity was linked before. Because login cookies * never expire, a user goes through OAuth very rarely, so an occasional pick is cheap * and it lets a person with several same-email accounts choose freely each time. - * 2. Else if the (provider,subject) is already linked, log into that account. - * 3. Else if the verified email matches exactly one account, auto-link and log in. + * 2. Else if the (provider,subject) is already linked, log into that account -- unless that + * account is still waiting for its address to be confirmed, in which case send the user + * back to their inbox rather than let them skip the confirmation forever. + * 3. Else if the email matches exactly one account, auto-link and log in. * 4. Else send the user to the "choose a username" page to finish a new account. * (Providers that don't release an email, e.g. ORCID, never reach step 1 or 3 and rely on * the stored link from step 2.) */ { struct gbMembers *matches = NULL; int n = 0; -if (id->emailVerified && isNotEmpty(id->email)) +/* Any address the provider released counts here, whether or not it set email_verified -- see + * the note above oauthAddressFromProvider(). Requiring the flag would send every CILogon user + * to the "choose a username" page even when they already have an account with that address, + * which is how the duplicate accounts in #38341 got made. */ +if (isNotEmpty(id->email)) { char query[1024]; /* Match the provider email against the primary address and any confirmed recovery address, * the same as password and email-link login do (see sqlAddressMatch). The isNotEmpty() * guard above keeps an empty id->email out of the query, so a blank recovEmail='' row can * never match. * Match only activated accounts. gbMembers has no unique key on email, and the plain * signup form will create an unactivated row for any address a person types -- the * activation mail goes to the address's real owner, who ignores it. Without this filter * someone could pre-register a victim's address, and the victim's first social login would * then auto-link to (and sign in as) the attacker's account. */ char *addrMatch = sqlAddressMatch(id->email); sqlSafef(query, sizeof(query), "SELECT * FROM gbMembers WHERE %-s AND accountActivated='Y' ORDER BY idx", addrMatch); freeMem(addrMatch); @@ -2644,30 +2760,47 @@ n = slCount(matches); } if (n > 1) { setPendingIdentity(id); gbMembersFreeList(&matches); chooseAccountPage(conn); return; } struct gbMembers *linked = memberForIdentity(conn, id); if (linked != NULL) { linkIdentity(conn, linked->idx, id); + /* The provider identity is proven, but the address on the account may not be: when the + * provider released none (ORCID) the user typed it themselves, and completeAccount left the + * account unactivated until the mailed link is opened. Signing in here would make that mail + * pointless -- the user would simply click the provider button again and never confirm -- so + * send them back to their inbox instead. Mail a fresh link each time, because the first one + * expires after seven days and this is the only way to activate such an account. An install + * that cannot send mail never creates an unactivated account here, but guard anyway rather + * than leave the user with nothing to click. */ + if (!sameString(linked->accountActivated, "Y") && !sameWord(returnAddr, "NOEMAIL") + && isNotEmpty(linked->email)) + { + setupNewAccount(conn, linked->email, linked->userName); + gbMembersFree(&linked); + gbMembersFreeList(&matches); + displayActMailSuccess(); + return; + } loginAndReturn(conn, linked->userName, linked->idx); gbMembersFree(&linked); gbMembersFreeList(&matches); return; } if (n == 1) { linkIdentity(conn, matches->idx, id); loginAndReturn(conn, matches->userName, matches->idx); gbMembersFreeList(&matches); return; } gbMembersFreeList(&matches);