Limit pending email verifications #4571

chadwhitacre · 2017-08-14T20:13:24Z

← Persist email verifications (#4573) — Support queuing emails without participants (#4548) →

Todo

Test/think through having more than one unverified email (two cases: merge/take-over, and pre-existing conditions)
Review envvars

rohitpaulk · 2017-08-15T01:52:07Z

gratipay/models/participant/email.py

@@ -106,6 +103,16 @@ def validate_email_verification_request(self, c, email, *packages):
        if not all(email in p.emails for p in packages):
            raise EmailNotOnFile()

+        unverified_email = c.one("""
+            SELECT address


A user could repeatedly add an email + remove it to circumvent this restriction, no? The throttling strategy we use will have to rely on previous state (as you've mentioned in the ticket description)

We need to start also thinking about not deleting email addresses, because when we start handling bounces and complaints (#4284) we'll want to retain that info for a given email address even if one participant removes it and another (or the same) adds it again. Hmm ...

A user could repeatedly add an email + remove it to circumvent this restriction, no?

Eureka! Turns out it makes the most UX sense to disallow removing emails when verification is pending, to properly communicate the reality that the pending verification still exists. But, if we truly disallow removing emails when verification is pending, then we'll have closed the repeatedly-add-and-remove loophole. Violà!

Proceeding on that basis in #4579 ...

Umm, what if a person mistakenly enters a wrong email address?

Turns out it makes the most UX sense to disallow removing emails when verification is pending, to properly communicate the reality that the pending verification still exists.

This is also a security risk, as far as I can tell. If we don't allow canceling verification requests - a mistake in entering a user's email could result in a malicious user taking over their account.

For example, github:

chadwhitacre · 2017-08-15T18:31:00Z

Alright, after wrestling to rebase this onto #4572 I think the way to architect this is to rename email_addresses (formerly emails) to email_verifications, and then make a new email_addresses table that holds just address as primary key and a nullable verification_id.

When a participant adds an email address we file the address in email_addresses (if needed) with a null verification, and we start a new verification in email_verifications (can have multiple verifications open across users for the same address, but only one per participant per address).

When a participant completes a verification we link to it in email_addresses.verification_id. We leave any other pending verifications open in email_verifications. We never delete from email_verifications.

When a participant removes an email address we null out the verification in email_addresses. We keep the record in the table because ... because ... ?

We link to email_addresses from email_messages.

We link to email_messages from email_verifications.

chadwhitacre · 2017-08-15T18:31:10Z

Something like that, anyway. :)

chadwhitacre · 2017-08-16T09:25:24Z

I've kicked out #4573 to persist email verifications. Once done will revisit here to see about constraining based on past verifications.

Remove security.spt.

By moving the `kw` assignment into a conditional block we ended up depending on previous iterations of the loop to assign `kw` in the no-doc case. Had the no-doc case been first, we would've hit a NameError.

Handles missing doc in npm change stream

…ticipants

TestNotifyParticipants uses QueuedEmailHarness

chadwhitacre · 2017-08-18T15:18:21Z

Working up a ttw test so we can get the user requirements here.

Persist email messages

chadwhitacre · 2017-08-22T18:49:48Z

Alright this is a mess. First of all the email listing should be styled like our other listings (homepage, search page, social profiles, etc.).

chadwhitacre · 2017-08-22T18:51:25Z

And after this PR we don't even want UI to add another email address when there is a pending verification.

chadwhitacre · 2017-08-22T18:51:52Z

But how does this interact with bulk npm claiming? We shouldn't allow interaction there either if the user has a pending verification.

chadwhitacre · 2017-08-22T18:54:49Z

But of course we need to protect against multiple verifications in the Python/API layer and not just in the JavaScript/UI.

chadwhitacre · 2017-08-22T18:56:09Z

And what about the case where someone legitimately enters a wrong address? They shouldn't be able to remove pending verifications either (UI and API). That's bound to generate some confusion. "I entered the wrong email and now I can't remove it or submit my correct email." It'll have to be a support request with an admin intervention to undo their condition.

mattbk · 2017-08-22T18:59:08Z

Are you trying to prevent people from ~~spamming~~ email bombing by adding secondary email addresses that point to someone else's address? Or is the verification system so monolithic that there's no way to throttle just one type of verification request?

chadwhitacre · 2017-08-22T19:25:19Z

chadwhitacre · 2017-08-23T11:16:57Z

Are you trying to prevent people from ~~spamming~~ email bombing by adding secondary email addresses that point to someone else's address? Or is the verification system so monolithic that there's no way to throttle just one type of verification request?

I'm not sure how to understand these as mutually exclusive. Did our recent slack chat (archiving stuck 😞 ) clarify this?

chadwhitacre · 2017-08-23T11:17:29Z

Closing in favor of #4579.

chadwhitacre mentioned this pull request Aug 14, 2017

Support queueing emails without participants #4548

Merged

1 task

rohitpaulk reviewed Aug 15, 2017

View reviewed changes

chadwhitacre mentioned this pull request Aug 15, 2017

Persist email messages #4572

Merged

1 task

chadwhitacre force-pushed the redo-throttling branch from 11be3e0 to 20cb963 Compare August 16, 2017 09:18

chadwhitacre mentioned this pull request Aug 16, 2017

Persist email verifications #4573

Closed

3 tasks

chadwhitacre changed the title ~~Redo throttling~~ Limit pending email verifications Aug 16, 2017

chadwhitacre changed the base branch from master to persist-verifications August 16, 2017 09:53

EdOverflow and others added 11 commits August 16, 2017 19:43

Remove security.spt.

d540649

Merge pull request #4575 from gratipay/rm-security

3d1de9b

Remove security.spt.

Bump version to 2100

5fe3425

Handles missign doc in npm change stream #4506

1c3427c

Dodge NameError/false assignment

864a839

By moving the `kw` assignment into a conditional block we ended up depending on previous iterations of the loop to assign `kw` in the no-doc case. Had the no-doc case been first, we would've hit a NameError.

Merge pull request #4574 from gratipay/handle-missing-doc-change-streem

e1b9820

Handles missing doc in npm change stream

Bump version to 2101

f556b4f

Switched from SentEmailHarness to QueuedEmailHarness in TestNotifyPar…

5391a4f

…ticipants

Fixed

258f22c

Minor fixes

d9796cf

Merge pull request #4576 from redixhumayun/master

57cb9db

TestNotifyParticipants uses QueuedEmailHarness

chadwhitacre force-pushed the redo-throttling branch from 20cb963 to a9e1faf Compare August 18, 2017 14:25

chadwhitacre force-pushed the redo-throttling branch from a9e1faf to 649da48 Compare August 18, 2017 16:08

rohitpaulk added 2 commits August 19, 2017 18:32

Merge branch 'master' into save-emails

77cf137

Merge pull request #4572 from gratipay/save-emails

d90fa7c

Persist email messages

chadwhitacre force-pushed the redo-throttling branch from 649da48 to 55e5d5d Compare August 19, 2017 16:44

chadwhitacre added 3 commits August 19, 2017 12:52

merge branch.sql into schema.sql

51e81b9

Bump version to 2102

2ba5b4f

Fix indentation

530bc71

chadwhitacre added 4 commits August 19, 2017 12:57

Start ttw test suite for email verification

13009bc

Stub out more tests

479d345

Too many pending verifications, outside

20631e9

Shelve work on persisting verifications

730806f

chadwhitacre force-pushed the redo-throttling branch from 55e5d5d to 730806f Compare August 19, 2017 16:58

chadwhitacre mentioned this pull request Aug 21, 2017

canada-weather gratipay/project-review#545

Closed

chadwhitacre mentioned this pull request Aug 23, 2017

Limit pending verifications in emails UI #4579

Merged

8 tasks

chadwhitacre closed this Aug 23, 2017

chadwhitacre deleted the redo-throttling branch August 23, 2017 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit pending email verifications #4571

Limit pending email verifications #4571

chadwhitacre commented Aug 14, 2017 •

edited

Loading

rohitpaulk Aug 15, 2017

chadwhitacre Aug 15, 2017

chadwhitacre Aug 23, 2017

rohitpaulk Aug 23, 2017

rohitpaulk Aug 23, 2017 •

edited

Loading

rohitpaulk Aug 23, 2017

chadwhitacre commented Aug 15, 2017 •

edited

Loading

chadwhitacre commented Aug 15, 2017

chadwhitacre commented Aug 16, 2017

chadwhitacre commented Aug 18, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017 •

edited

Loading

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017 •

edited

Loading

mattbk commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

Limit pending email verifications #4571

Limit pending email verifications #4571

Conversation

chadwhitacre commented Aug 14, 2017 • edited Loading

Todo

rohitpaulk Aug 15, 2017

Choose a reason for hiding this comment

chadwhitacre Aug 15, 2017

Choose a reason for hiding this comment

chadwhitacre Aug 23, 2017

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2017

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2017

Choose a reason for hiding this comment

chadwhitacre commented Aug 15, 2017 • edited Loading

chadwhitacre commented Aug 15, 2017

chadwhitacre commented Aug 16, 2017

chadwhitacre commented Aug 18, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017 • edited Loading

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017 • edited Loading

mattbk commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 14, 2017 •

edited

Loading

rohitpaulk Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 15, 2017 •

edited

Loading

chadwhitacre commented Aug 22, 2017 •

edited

Loading

chadwhitacre commented Aug 22, 2017 •

edited

Loading