83dd847b7449dd0fa83c22a4d60854f8d20d2847 max Fri Sep 18 08:51:14 2026 -0700 hgLogin: stop telling a CILogon user that CILogon shares no email address #Preview2 week - bugs introduced now will need a build patch to fix QA found three messages on the social sign-in pages claiming the provider does not give us an address. That was written when ORCID, which really releases none, was the only provider reaching those pages. CILogon does release the institution's address; we drop it because CILogon never marks it verified and login.oauth.cilogon.trustEmail is what says we may take it anyway. Once the address is dropped, nothing downstream could tell "released nothing" from "released something we would not take". oauthFetchIdentity now keeps the dropped address in a separate field that matching and sign-in never look at, and the three messages read it: the choose a username page, the confirmation page, and the one that turns down a typed address that already belongs to an account. The address box on the choose a username page starts from it as well, since retyping what we were just handed is the last thing a person wants to do. It still has to clear the duplicate check and be confirmed by mail, exactly as a typed address does. Three more things on the confirmation page, all from the same QA pass: The generic "a confirmation email has been sent" paragraph printed below the "use this address instead" form, so it read as if a mail had already gone to whatever was about to be typed in. It now goes above the form. Correcting the address redisplayed the same page with the new address swapped in and nothing to say the change had worked. It now says so. The rejection reason for an address that is already in use was set but never printed, so a refused change looked like nothing happening at all; that is printed now too. Reworded the unconfirmed-account explanation as QA suggested. refs #38339 diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index 0fdc857f89f..3811c7338f9 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -402,79 +402,109 @@ { char *returnURL = getReturnToUrlForAttr(); hPrintf( "
You signed in with %s, and %s does not tell us whether the email " + "address it gave us really belongs to you, so we asked you for one. No %s " + "account uses %s yet, so we are making a new account for it. Confirming " + "the address is the last step.
", + encProvider, encProvider, brwName, encAddress); + else hPrintf("You signed in with %s, and %s did not tell us an email address, so we " "asked you for one. No %s account uses %s yet, so we are making a new " "account for it. Confirming the address is the last step.
", encProvider, encProvider, brwName, encAddress); + } else { char *encUser = htmlEncode(existingUser); - hPrintf("Your %s sign-in belongs to the %s account %s, but the address on " - "that account, %s, has never been confirmed. Confirm it once and %s will " - "sign you straight in from then on.
", + /* Say that the address was changed. Without this the page comes back looking exactly + * as it did before the change, the new address being the only sign anything happened. */ + if (justChanged) + hPrintf("The email address on the %s account %s is now %s.
", + brwName, encUser, encAddress); + else + hPrintf("Your %s sign-in is linked to the %s account %s, but the email " + "address on that account, %s, has not been confirmed. Once it is " + "confirmed, %s will sign you in directly.
", encProvider, brwName, encUser, encAddress, encProvider); - /* If that address is wrong the confirmation can never arrive, and this account has no - * other way in, so offer to replace it here. pendingIdentityValid() is the check that - * this really is the person who just came back from the provider. */ - if (pendingIdentityValid()) + freeMem(encUser); + } + } +/* A rejected address (changePendingEmail) leaves its reason here, and this page is where the + * user sees it -- nothing else prints it on the way through. */ +if (isNotEmpty(errMsg)) + hPrintf("%s
", errMsg); +hPrintf( + "A confirmation email has been sent to you. \n" + "Please click the confirmation link in the email to activate your account.
" + "You may have to look in your spam folder for an email from genome-www@soe.ucsc.edu, " + "especially if you use Microsoft Outlook or Hotmail.
"); +if (offerNewAddress) { - hPrintf("If %s is not an address you can read, enter the right one and we " - "will send the confirmation there instead.
", encAddress); + /* If that address is wrong the confirmation can never arrive, and this account has no other + * way in, so offer to replace it here. */ + hPrintf("If %s is not the right email address, enter the right one, and we will " + "send the confirmation there instead.
", encAddress); hPrintf(""); } - freeMem(encUser); - } +hPrintf("\n", returnURL); freeMem(encProvider); freeMem(encAddress); - } -hPrintf( - "A confirmation email has been sent to you. \n" - "Please click the confirmation link in the email to activate your account.
" - "You may have to look in your spam folder for an email from genome-www@soe.ucsc.edu, " - "especially if you use Microsoft Outlook or Hotmail.
" - "\n" - "", returnURL); cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_userName"); cartRemove(cart, "hgLogin_actMailProvider"); cartRemove(cart, "hgLogin_actMailTo"); cartRemove(cart, "hgLogin_actMailUser"); +cartRemove(cart, "hgLogin_actMailUnverified"); +cartRemove(cart, "hgLogin_actMailChanged"); } void sendActMailOut(char *email, char *subject, char *msg) /* send mail to email address */ { int result; result = mailViaPipeBounce(email, subject, msg, returnAddr); if (result == -1) { hPrintf( "" @@ -1951,30 +1981,32 @@ if (confirmRecov) sendRecovEmailConfirmMail(recovEmail, user, recovEmail, "N"); /* send out activate code mail, and display the mail confirmation box */ cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_email2"); cartRemove(cart, "hgLogin_userName"); cartRemove(cart, "user"); cartRemove(cart, "token"); /* This page is shared with the social-login flows, which leave it a note saying which provider * and address to explain. Those are cleared when that page renders, but the hand-off is a * JavaScript redirect and a user who never lands on it keeps them in the cart. Drop them here * so a plain signup can never inherit somebody else's explanation. */ cartRemove(cart, "hgLogin_actMailProvider"); cartRemove(cart, "hgLogin_actMailTo"); cartRemove(cart, "hgLogin_actMailUser"); +cartRemove(cart, "hgLogin_actMailUnverified"); +cartRemove(cart, "hgLogin_actMailChanged"); redirectToLoginPage("hgLogin.do.displayActMailSuccess=1"); } void accountHelp(struct sqlConnection *conn) /* email user username(s) or new password */ { char query[1024]; // room for an address-matching clause holding a long address twice char *email = cartUsualString(cart, "hgLogin_email", ""); char *username = cartUsualString(cart, "hgLogin_userName", ""); char *helpWith = cartUsualString(cart, "hgLogin_helpWith", ""); /* Passwordless email login link */ if (sameString(helpWith,"loginLink")) { sendEmailLink(conn); @@ -2367,46 +2399,51 @@ boolean ok = sameString(sig, expected); freeMem(expected); return ok; } static void setPendingIdentity(struct oauthIdentity *id) /* Stash an authenticated-but-not-yet-linked identity in the cart so it survives a form * round-trip (the "choose a username" or "choose an account" page). The signature is what * proves, on the way back, that we really verified this identity, for this browser, recently. */ { char timeStr[32]; safef(timeStr, sizeof(timeStr), "%ld", clock1()); cartSetString(cart, "oauth_pending_provider", id->provider); cartSetString(cart, "oauth_pending_subject", id->subject); cartSetString(cart, "oauth_pending_email", emptyForNull(id->email)); +/* Not signed, the same as oauth_pending_name: it decides nothing, it only picks the wording of + * the page that asks for an address. dropRequestSuppliedFlowVars keeps a request-supplied copy + * from standing in for ours. */ +cartSetString(cart, "oauth_pending_email_unverified", emptyForNull(id->emailUnverified)); char *emailVerified = id->emailVerified ? "1" : "0"; cartSetString(cart, "oauth_pending_email_verified", emailVerified); cartSetString(cart, "oauth_pending_name", emptyForNull(id->displayName)); cartSetString(cart, "oauth_pending_time", timeStr); cartSetString(cart, "oauth_pending_sig", oauthPendingSig(id->provider, id->subject, emptyForNull(id->email), emailVerified, timeStr)); } static void clearPendingIdentity() /* Remove the pending-identity cart variables. Call this on every path that finishes with the * pending identity -- success or definitive failure -- so a stale signature is not left behind * in the cart to be swept into a saved session. */ { cartRemove(cart, "oauth_pending_provider"); cartRemove(cart, "oauth_pending_subject"); cartRemove(cart, "oauth_pending_email"); +cartRemove(cart, "oauth_pending_email_unverified"); cartRemove(cart, "oauth_pending_email_verified"); cartRemove(cart, "oauth_pending_name"); cartRemove(cart, "oauth_pending_time"); cartRemove(cart, "oauth_pending_sig"); } 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()", @@ -2429,30 +2466,44 @@ static char *oauthProviderEmail() /* The address the provider released for the pending identity, or NULL if it released none or * released something that is not a usable address. * When this is non-NULL the "choose a username" page must not ask for an address at all. We * already have one, from a source the person cannot type into, so a text box would only invite * an edit -- and a box we then accept unchanged, without ever writing to it, is the worst of * both worlds: it looks like a question we check the answer to, and it is not. Either we have * an address and use it, or we do not have one and must confirm what the user types. */ { char *email = cartUsualString(cart, "oauth_pending_email", ""); if (isEmpty(email) || spc_email_isvalid(email) == 0) return NULL; return email; } +static char *oauthUnverifiedEmail() +/* The address the provider released for the pending identity and we would not take, because it + * did not say the address is verified and hg.conf does not trust the provider (see + * oauthFetchIdentity). NULL when the provider released nothing at all. + * This is the difference between "we were told nothing" and "we were told something we cannot + * act on", which the user needs to hear and which the form can start from. Never match on it: + * whatever the user does with it, it still has to be confirmed by mail. */ +{ +char *email = cartUsualString(cart, "oauth_pending_email_unverified", ""); +if (isEmpty(email)) + return NULL; +return email; +} + 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 *providerEmail = oauthProviderEmail(); char *suggested = cartUsualString(cart, "hgLogin_userName", ""); if (isEmpty(suggested)) @@ -2462,53 +2513,73 @@ hPrintf("
You signed in with %s. Pick a username for your new %s account. " "You can change the suggested name below.
", label, brwName); /* Explain why this is always a new account when the provider released no address (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). * Test whether anything arrived, not whether oauthProviderEmail() accepted it. A provider that * sends an address we cannot use -- spc_email_isvalid rejects every byte >= 127, so any * non-ASCII address -- also leaves us asking for one, but telling that user the provider shares - * no address would be simply untrue. */ -if (isEmpty(email)) + * no address would be simply untrue. + * Same for a provider that did release an address which we then dropped because it would not + * say the address is verified: CILogon does exactly this, and "does not share your email + * address with us" was plainly wrong for it (#38339). */ +char *unverified = oauthUnverifiedEmail(); +if (isEmpty(email) && unverified != NULL) + { + char *encUnverified = htmlEncode(unverified); + hPrintf("%s gave us the email address %s, but does not tell us whether that " + "address really belongs to you, so we cannot use it to sign you in to an account that " + "already has it. Enter an address below and confirm it once; after that this sign-in " + "will work on its own. If you already have an account, use another sign-in option " + "instead.
", label, encUnverified); + freeMem(encUnverified); + } +else if (isEmpty(email)) hPrintf("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.
", brwName, label, label); else if (providerEmail == NULL) hPrintf("We cannot use the email address %s gave us, so please enter one below.
", label); printUsernameNote(); hPrintf("%s", errMsg ? errMsg : ""); hPrintf("