Don't require a valid email address to sign in #1251
Conversation
Users do not necessarily have a valid email in GitHub. We should not fail most actions if we cannot successfully send them an email. I have purposely left the error case on the `users/email/token/regenerate` endpoint, since that endpoint clearly has an email purpose in mind. I opted to change the signature of the existing name in order to force a compiler error on any code that hasn't been looked at to determine whether it should fail on this or not. Fixes rust-lang#1142
There was a problem hiding this comment.
I dont think this affects #1142; that seems to have been caused by ghostery. I'm also skeptical that it does what it intends... see comment. So i dont think we need this...
| @@ -128,8 +127,7 @@ impl<'a> NewUser<'a> { | |||
| .optional()?; | |||
|
|
|||
| if let Some(token) = token { | |||
| send_user_confirm_email(user_email, &user.gh_login, &token) | |||
| .map_err(|_| NotFound)?; | |||
| send_user_confirm_email(user_email, &user.gh_login, &token); | |||
There was a problem hiding this comment.
If there isn't an email, we shouldn't even be in this block because of the if let Some(user_email) = user.email.as_ref() { above, right? And if the email is present but not valid, we don't find that out synchronously. Emails get put in mailgun's queue and then their dashboard shows email sending success/failure.
There was a problem hiding this comment.
So what I think what we were seeing here is that the email address is being rejected on our end (probably in lettre) before we even generate an email and give it to mailgun. For instance, if somebody manages to put invalid as their email address in GitHub.
There was a problem hiding this comment.
As a more specific example, here is the email address that was giving one user issues: String.Format(\"{0}.{1}@live.com\", FirstName, LastName). After looking at the profile a dozen times, I only just now realized that this is the email address on their profile page.
There was a problem hiding this comment.
That example would make a great test case and would certainly clarify this PR that it's not sending the email that fails, but that there's verification of an email address's validity in lettre that fails and returns an error. That wasn't at all clear from the original PR.
|
Agreed. |
|
That being said, have always had the same problem, and I never could sign into crates.io. After reading this, I changed my email to a valid one and it worked... Hope this will keep working without keeping the email in github account, otherwise I won't publish much on crates.io, if I have to change that email every time... |
|
I'm reopening this because we should at least allow the user to create an account, even if they have a very invalid email address. In the near future users without a verified email address will not be able to publish crates, but at least the error will only affect the publish action specifically. |
|
I suggest that you let the user fill out an email address when signing up and don't rely on an email from github.com. Github also didn't let me set the email address again, so mine is now blank. Note that github however has my email address, which is tied to my account, but it's not set in my profile, so even they make a separation here. To avoid spam I would never set something like that to a valid email. |
|
I haven't checked the github api, but maybe you can request the primary account email rather than the "public profile email". That would probably solve this as well, but it still won't let the user choose whether they want to use the same email for github as for crates, but I personally think it makes sense if you use github to login to crates... |
|
Closing in favor of #1622. |
Invalid emails from GitHub This supersedes #1251.
Users do not necessarily have a valid email in GitHub. We should not
fail most actions if we cannot successfully send them an email. I have
purposely left the error case on the
users/email/token/regenerateendpoint, since that endpoint clearly has an email purpose in mind.
I opted to change the signature of the existing name in order to force a
compiler error on any code that hasn't been looked at to determine
whether it should fail on this or not.