Invalid emails from GitHub #1622
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
carols10cents
force-pushed
the
invalid_emails_from_github
branch
4 times, most recently
from
ed303c7 to
7389454
Compare
carols10cents
force-pushed
the
invalid_emails_from_github
branch
from
7389454 to
8212f56
Compare
|
I wanted to raise this on the issue but I think we should discuss why we aren’t requiring a valid email to sign up? Would be a good topic for an upcoming meeting |
|
@ashleygwilliams there are things you can do on crates.io like follow a crate that we don't really need an email address for |
|
Thanks for doing the work to update this |
|
How does this work for things where you do require a valid email? Does it still rely on the public profile email from github, because in that case it's pushed the problem further down the pipeline. The good thing is that first thing on the user profile is the possibility to edit the account email address, so if people can sign up, they can then provide an email without relying on the github api, so that looks like a major improvement! Thanks for taking this on. |
|
@najamelan if you have a valid public profile email from github, the first time you sign in, we'll send an email to that address to confirm it. As you saw, if you'd like to use a different email address for crates.io, you can change it in your profile. |
|
I've deployed this PR to https://jtgeibel-staging-crates-io.herokuapp.com/ for anyone who would like to test out this change further. Also, cc #1404 which we should close once this PR is merged. I'm r+ on this PR, but I think we're looking to deploy current master before merging more changes. |
|
I (the person with the invalid mail address) tried logging into @jtgeibel's staging area, and can login successfully now. I also get to the dialog to change my mail address. That one doesn't send me a verification mail on my real address, but it might be due to the staging server not sending emails? Thanks for working on that so quickly! |
|
|
||
| #[test] | ||
| fn sending_to_valid_email_succeeds() { | ||
| let result = send_email("[email protected]", "test", "test"); |
There was a problem hiding this comment.
Does this require having a valid mailgun config locally in order to run?
There was a problem hiding this comment.
Ah I guess we're just writing to a tempfile. I'm wondering if we could make it a bit more specific that this is only testing the lettre part of our stack, but I guess at the end of the day that really is just the whole thing and we can trust that it's behaving the same in the file sender vs smtp
|
Sorry, I guess I had started a review rather than actually leaving that comment a few days ago >_> @bors r+ |
|
📌 Commit 8dae597 has been approved by |
Invalid emails from GitHub This supersedes #1251.
|
I'll deploy this once the current deploy has been stable for a bit. Probably won't get to it until tomorrow since it's already late afternoon over here. |
|
☀️ Test successful - checks-travis |
This supersedes #1251.