- **status**: code-review --> in-progress - **QA**: Dave Brondsema - **Comment**:
I think this is a good approach, but wow it has a pretty broad impact. I found several issues: You left a few pydevd lines. In `verify_addr` when other addresses are removed, we should log which were removed. In case there are issues and we need to check the logs to find out what happened. If [#7524] is merged yet, you can use those audit logs. Else just regular python logging is ok. `by_email_address` and `identify_sender` and `password_recovery_hash` methods use the user for the first record it finds. There could be multiple results though. Should limit it to confirmed address and I think that'll work then. These were true before this change, but might as well fix them now: `claim_only_addresses()` has a query on the `claimed_by_user_id` field which will run slowly since it's not the first field in any index. But `claim_only_addresses` isn't used anywhere, so you can just delete it :) And `verify_addr` has a query by `nonce` so we should add an index on `nonce`. Might as well make it a unique index too, so we never risk duplicate nonces. The `upsert` method behavior changed so it always just gets a new one and never returns an existing one. Perhaps that is the way it should work now, but probably good to review the logic of each `upsert` call, I am not so sure. If that's how it should be, it should be renamed to reflect what it does. We also have one reference in SF's internal repo to `EmailAddress.upsert` that I think needs to be changed (it assumes it'll get the existing record not a new one). That code also has a `email_record._id` which needs to be fixed. The migration fails for me with `ming.schema.Invalid: _id:[email protected] is not a valid ObjectId` probably because its loading old data with the new model and so it doesn't validate. And I don't think a single `update()` call to change all directly in mongo would work either since we need to assign new ObjectIds. Looking around for ideas, I also came across the restriction that _id field can't be changed. So this may require some research. One approach we've done in the past for really big changes is to copy the old model definition and call it `EmailAddressOld` and then in `EmailAddress` change the `__mongometa__ name = 'email_address'` so that the data is stored in a new collection. Then the migration can copy from one to the other. Also, once you get something working, I'm curious how long it takes to handle a few million user records? I am thinking it might be necessary to also have on-the-fly migrations so that everything works for people using the site before the m igration completes. If we do that, we'd still want the script to ensure all records get switched over to the new format. For on-the-fly migrations see http://merciless.sourceforge.net/tour.html#specifying-a-migration although that might not work with a 2-model approach. Lastly, and we may want to spin this off into a separate ticket, but giving a "Email address already claimed" message allows for "email enumeration" which we're trying to avoid (e.g. [#7543]). We should instead send a message to the given address and do something from there. I haven't thought through exactly what the process should be: email message says its already associated with username "foo", or allow new account to validate the address and remove it from old user, etc. --- ** [tickets:#7527] Email address associations need better user associations** **Status:** in-progress **Milestone:** forge-jul-25 **Created:** Wed Jul 02, 2014 09:08 PM UTC by Dave Brondsema **Last Updated:** Tue Jul 22, 2014 07:17 PM UTC **Owner:** Alexander Luberg A user can claim any address and even if they don't verify it, that still blocks someone else from trying to claim it. This can be fixed in auth.py like: ~~~~ ::diff - if M.EmailAddress.query.get(_id=new_addr['addr']): + if M.EmailAddress.query.get(_id=new_addr['addr'], confirmed=True): ~~~~ However this leads to another problem, if multiple users have the same email address claimed (but not verified). One user sees a "verify" link, but the other sees "Unknown addr obj [email protected]", on the preferences page. There probably are more issues when it gets to verification, too. --- Sent from sourceforge.net because [email protected] is subscribed to https://sourceforge.net/p/allura/tickets/ To unsubscribe from further messages, a project admin can change settings at https://sourceforge.net/p/allura/admin/tickets/options. Or, if this is a mailing list, you can unsubscribe from the mailing list.
