#7717 Better existing email addr handling

v1.2.0
closed
sf-2 (994)
General
2015-08-20
2014-09-23
No

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.

Discussion

1 2 > >> (Page 1 of 2)
  • Dave Brondsema

    Dave Brondsema - 2014-10-06
    • Size: --> 2
     
  • Alexander Luberg

    • status: open --> in-progress
    • assigned_to: Alexander Luberg
     
  • Alexander Luberg

    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: #7761

  • Alexander Luberg

    • status: in-progress --> blocked
     
  • Alexander Luberg

    • status: blocked --> in-progress
     
  • Alexander Luberg

    allura-fork: al/7717

     
  • Alexander Luberg

    • status: in-progress --> code-review
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-17
    • QA: Igor Bondarenko
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-17

    I've rebased on fresh master and made couple minor fixes. See je/7717

    Test 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.

    1. Claim email by user1 and verify
    2. Claim the same email by user2. Email will appear in the emails list, but no verification link sent. Good
    3. Click (by user2) on "verify" link next to email. Verification link will be sent and you can verify

    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

    Allura/allura/lib/app_globals.py:311: undefined name 'MockAMQ'
    Allura/allura/lib/app_globals.py:313: undefined name 'Connection'
    

    Don't sure why these was removed in 25c1757a0f25f9b2b6bfd89a352bf2ea0baebdf3 seems unrelated to the ticket.

     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-17
    • status: code-review --> in-progress
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-20
    • Milestone: forge-oct-17 --> forge-oct-31
     
  • Alexander Luberg

    • status: in-progress --> code-review
     
  • Alexander Luberg

    I haven't found je/7717 - probably you haven't pushed it. I have rebased it, fixed comments, and refactored a bit.

     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-22
    • status: code-review --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-22

    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:

    1. Claim email by user1 (don't verify, but save the link)
    2. Claim the same email by user2 (and verify)
    3. Verify email for user1 by going to link saved in (1).

    user1 and user2 have both confirmed the same email.

    We need to invalidate all confirmation links for email being confirmed.

     
  • Alexander Luberg

    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.

     
  • Alexander Luberg

    • status: in-progress --> code-review
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-27
    • status: code-review --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-27

    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,

    1. Claim email by user1
    2. Claim email by user2
    3. Both verification links don't work

    I believe the problem is in comparison of EmailAddress instances in filter call, 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.

     
  • Alexander Luberg

    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.

     
  • Alexander Luberg

    • status: in-progress --> code-review
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-10-28
    • status: code-review --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-30
    • Milestone: asf_release_1.0.0 --> unreleased
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-30
    • Labels: sf-2 --> sf-current, sf-2
     
  • Dave Brondsema

    Dave Brondsema - 2014-11-03
    • labels: sf-current, sf-2 --> sf-2
     
1 2 > >> (Page 1 of 2)

Log in to post a comment.