#7717 Better existing email addr handling
When adding an email address to an account, we check to see if someone else has already claimed that address, and show error "Email address already claimed". This can be a form of email address enumeration (finding out if someone else's email is in the system).
We should avoid that, and have the result on the page be the same whether the email is already claimed or not. The email that we send out to the address can be different (since only the real owner will get it). The email can say something like "You tried to add EMAIL to your SITE_NAME account, but it is already claimed by your USERNAME account. You should use that account instead, or remove that address from that account. If this was not you who attempted this, you can safely ignore this email".
We should also check to see if a claimed address belongs to a disabled user or not.
I have implemented it with the email notification, but figured out that it is still possible to do email enumeration. If the email was claimed successfully - it will be displayed on the page in the claimed list, and this can be used as a condition to verify if it does exist or not in the system.
Also I need to discuss the case when the user was disabled, because there is a related bug: [#7761]
Related
Tickets:
#7761allura-fork: al/7717
I've rebased on fresh master and made couple minor fixes. See
je/7717Test for "not confirmed" has misleading comment (copied from "confirmed" test, I guess) and should check that verification email was sent and not "claim attempt" email.
Another issue is that you can end up in a situation when two users have the same confirmed email.
It's not so big of a problem, because email is sent to the user that owns email anyway, but he may click on verification link accidentally or something. And also having two emails might mess up user-by-email lookups, e.g.
User.by_email_address.I think we should send "claim attempt" email in this case too. Add a test for this.
There's also pyflakes-related test failures
Don't sure why these was removed in
25c1757a0f25f9b2b6bfd89a352bf2ea0baebdf3seems unrelated to the ticket.I haven't found je/7717 - probably you haven't pushed it. I have rebased it, fixed comments, and refactored a bit.
Yeah, I forgot to push it, sorry, but you fixed all of that in your latest commit, anyway :)
Good job!
I found one more way to get two confirmed emails by different accounts, though:
user1 and user2 have both confirmed the same email.
We need to invalidate all confirmation links for email being confirmed.
You are doing great job finding tricky usecases! :) Fixed. Let me know if the solution is ok, another option would be to cleanup nonce on existing emails, but I think this one will be faster & easier.
Fix looks ok to me
However, I'm getting "Unknown verification link" for both users even though they didn't verify email (tested on sandbox).
So,
I believe the problem is in comparison of
EmailAddressinstances infiltercall, but don't sure.I thought of another improvement we could do is to add expiration time for verification links (similar to password reset hash), but I think it can be separate ticket, and current approach is ok for now.
Ah, that was a typo, I had to filter and select only confirmed emails. tests pass, the test case is passed too(since email was not yet verified any of the users can confirm it). Also rebased again.