Limit pending verifications in emails UI #4579

chadwhitacre · 2017-08-22T19:33:12Z

← Implement deploy hooks (#4590) — Redux of #4571 — "Ready to verify" (#4584) →

We want to limit pending email address verification attempts to one, to prevent abuse (#4557). We need to do this both in the UI and in the API/db. The former is in some ways easier than the latter so let's try starting with that?

Todo

style listing
- figure out what to do with action buttons
- bring in status icons
constrain to available actions only
get all action buttons working again
bring back add email (as button next to header?)
~~confirm on remove~~ → Confirm some things #4581
change base back to master after Implement deploy hooks #4590

chadwhitacre · 2017-08-22T21:38:09Z

chadwhitacre · 2017-08-22T21:57:14Z

Whoa! Monster mouse! 🐭 👹

chadwhitacre · 2017-08-23T11:24:18Z

I realized at #4571 (comment) that we can implement verification throttling with no additional schema changes by protecting against removal in the app layer. That's enough for here, though handling unsubscribes will still be a chore ...

chadwhitacre · 2017-08-23T12:02:25Z

Squashed and rebased, was 1b1ee1e.

rohitpaulk · 2017-08-23T14:25:24Z

@whit537: Comments added at #4571 (comment)

chadwhitacre · 2017-08-23T16:36:46Z

Okay! First chunk of work done in 153469b. Since we're already at 370 ∆ lines I will probably scope this just to the email listing, and implement the package page and API changes on other PRs.

chadwhitacre · 2017-08-23T16:42:33Z

Picking up from #4571 (comment) ...

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

Then they have to contact support. And that raises a good point: admins should be able to remove pending email verifications. If we get enough pressure on support then we consider re-enabling this as self-serve.

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.

Sorry ... how so?

For example, github:

👍

rohitpaulk · 2017-08-23T16:44:30Z

Sorry ... how so?

I enter a wrong email address. Person verifies the email, uses it (forgot password + reset password using email) to sign-in?

chadwhitacre · 2017-08-23T16:46:26Z

I enter a wrong email address. Person verifies the email, uses it to sign-in?

Okay, but how do we mitigate that? That's the achilles heel of email for auth, isn't it? :-/

rohitpaulk · 2017-08-23T16:47:13Z

Okay, but how do we mitigate that?

Allow the user to remove the email immediately, by themselves. (We should also remove the verification token internally, when they do that)

chadwhitacre · 2017-08-23T16:54:25Z

Allow the user to remove the email immediately, by themselves.

Okay, but then how do we protect against the other security risk: remove-and-add to work around verification throttling?

chadwhitacre · 2017-08-23T16:55:16Z

Remember that verification links expire after 24 h anyway. We could tighten that to five minutes once we have instant send (#4545).

rohitpaulk · 2017-08-23T16:57:25Z

Okay, but then how do we protect against the other security risk: remove-and-add to work around verification throttling?

Can't we maintain persistent history of sent emails separately, and use that info to throttle? Canceling of an email verification link doesn't have to necessary delete ALL history of an email ever being sent.

chadwhitacre · 2017-08-23T16:59:02Z

Sorry, throttling isn't the best word. The way we're headed here is to do both:

Throttle as we have done (though it'll look a little different with Send email messages immediately #4545).
Limit the number of pending verifications.

chadwhitacre · 2017-08-23T17:00:54Z

I think we should tighten the verification window to five minutes: that will protect against "honest mistakes" turning into inadvertent phishing.

I think we should only allow one pending verification at a time with no option to remove. Our use case is different than GitHub's because we have all npm maintainer emails in our system. We're playing with fire. 😳 🔥

rohitpaulk · 2017-08-23T17:01:18Z

Hmm, I see what you're getting at.

rohitpaulk · 2017-08-23T17:04:36Z

Our use case is different than GitHub's because we have all npm maintainer emails in our system

I understand the 'limit pending verifications' concern. However, what does 'having all npm maintainer emails in our system' have to do with user email verification? Is this because it is tied to claiming packages?

chadwhitacre · 2017-08-23T17:06:40Z

Is this because it is tied to claiming packages?

Yes. On my next PR I'll be modifying the UI on pages like https://gratipay.com/on/npm/express to limit the ability to initiate package claims when there are existing verifications (~= package claims) outstanding.

chadwhitacre · 2017-08-23T17:07:39Z

We've had three users already report spam/phishing to us (#4557), so this isn't theoretical.

rohitpaulk · 2017-08-23T17:12:07Z

I still think that we should decouple email verification and linking packages. What I propose is:

Only allow users that have verified an email corresponding to a npm package to 'claim' it.
Once we do have a verified email that corresponds to an npm package, allow user to claim the package directly, and send a notice (not verification, just a notice) out to all maintainers. The email body should clearly mention that the Gratipay user is a verified owner of that specific email address.

Now that the email only says 'link [email protected] to the abcd account on Gratipay' - it is (perceived as?) less dangerous. As all services do, we'll just add a note saying 'If this wasn't you, no action is required' at the bottom. Generic sign-up email spam is understandable, and won't startle users like an email saying 'May we receive money on your behalf'.

chadwhitacre · 2017-08-23T18:18:40Z

Okay, so can we hash out the user flow on https://gratipay.com/on/npm/express, then? Do we display all maintainer emails and only allow the user to initiate a message to addresses they've already verified? Do we only display email addresses they've already verified? Do we offer to send an email verification right from the package page? Do we make them click over to the Emails page to add the address there?

rohitpaulk · 2017-08-23T18:31:20Z

Okay, so can we hash out the user flow on https://gratipay.com/on/npm/express, then? Do we display all maintainer emails and only allow the user to initiate a message to addresses they've already verified? Do we only display email addresses they've already verified? Do we offer to send an email verification right from the package page? Do we make them click over to the Emails page to add the address there?

Basics, just to get things working:

If user is a maintainer, show 'Claim' button directly. Make it clear that the user is allowed to claim this package because they own that specific address, and also show the list of maintainers that we'll send emails to if they claim the package.
If the user is not a maintainer, show all emails, say that you have to add one of these (link to emails page) as a verified email address on Gratipay. Once verified, come back to this page and you'll be able to claim a package.

Streamlining (building upon the above):

When an email is verified, automatically fish out unclaimed packages related to that address, and take them to the claim page? (or a bulk claim page?)
Add 'link/verify' buttons to the emails on the 'claim' page, without requiring the user to go to the emails page and come back.

chadwhitacre · 2017-08-23T21:25:56Z

send a notice (not verification, just a notice) out to all maintainers.

That's #4425.

chadwhitacre · 2017-08-31T19:18:32Z

Rebased onto #4590, was 4f33932.

chadwhitacre · 2017-09-08T13:48:39Z

Rebased back onto master, was a6b4315.

rohitpaulk · 2017-09-08T14:05:29Z

www/~/%username/emails/index.spt

+for email in emails:
+    i = 0 if email.address == participant.email_address else 1 if email.verified else 2
+    groups[i].append(email)
+emails = sum(groups, [])


😃 Is this common usage?

Perils of not having flatten in the std lib? :P

Yes, that's how to spell flatten in Python. 🐭

rohitpaulk · 2017-09-08T14:21:35Z

www/~/%username/emails/index.spt


 emails = participant.get_emails()
+groups = [[], [], []]  # primary, verified, unverified
+for email in emails:
+    i = 0 if email.address == participant.email_address else 1 if email.verified else 2


We're essentially doing a sort_by(func) action here, can't we use https://docs.python.org/3/library/functions.html#sorted with the key param?

i.e. something like:

def get_sort_key(email): if email.address == participant.email_address: return 0 elif email.verified: return 1 else: return 2 sorted_emails = sorted(emails, key=get_sort_key)

Looks less cryptic to me than the awkward spelling of flatten, and the inlined inline if 😛

Done in 11a93a4

rohitpaulk · 2017-09-08T14:26:42Z

tests/ttw/test_email_verification.py

+from gratipay.testing import BrowserHarness, QueuedEmailHarness
+
+
+class Row(object):


- don't want before.py (see deploy/README) - run after.py directly on dyno vs. redirect

- dodge race condition around clicks and page reloading - factor out helpers into proper classes - simplify remove action to maintain proper state (presence of add form) - minor formatting tweak in sptfile

rohitpaulk · 2017-09-16T14:14:01Z

Rebased, previous head was @c3452ab8472658a42f2acdefa5cd865bbb645f33.

rohitpaulk · 2017-09-16T14:40:20Z

@whit537 This looks good to me. Ready to merge?

chadwhitacre · 2017-09-16T21:13:59Z

@rohitpaulk Yes!

chadwhitacre mentioned this pull request Aug 23, 2017

Limit pending email verifications #4571

Closed

2 tasks

chadwhitacre changed the title ~~Limit verifications in UI~~ Limit pending verifications Aug 23, 2017

chadwhitacre mentioned this pull request Aug 23, 2017

Persist email verifications #4573

Closed

3 tasks

rohitpaulk mentioned this pull request Aug 23, 2017

Support queueing emails without participants #4548

Merged

1 task

chadwhitacre mentioned this pull request Aug 23, 2017

Show open claims to participant #4426

Closed

3 tasks

chadwhitacre force-pushed the limit-verifications-in-ui branch from 1b1ee1e to a6fae92 Compare August 23, 2017 12:02

chadwhitacre force-pushed the limit-verifications-in-ui branch from a6fae92 to 153469b Compare August 23, 2017 16:32

chadwhitacre mentioned this pull request Aug 28, 2017

✈️ Give to package.json #4427

Closed

15 tasks

chadwhitacre force-pushed the limit-verifications-in-ui branch from 4f33932 to 64b3555 Compare August 31, 2017 19:18

chadwhitacre changed the base branch from master to deploy++ August 31, 2017 19:19

chadwhitacre mentioned this pull request Aug 31, 2017

Implement deploy hooks #4590

Merged

chadwhitacre force-pushed the deploy++ branch from 7f628f9 to 2bf4991 Compare September 1, 2017 19:15

chadwhitacre force-pushed the limit-verifications-in-ui branch 2 times, most recently from dfa177e to a6b4315 Compare September 1, 2017 19:18

rohitpaulk changed the base branch from deploy++ to master September 8, 2017 13:47

chadwhitacre force-pushed the limit-verifications-in-ui branch from a6b4315 to dd375b8 Compare September 8, 2017 13:48

rohitpaulk reviewed Sep 8, 2017

View reviewed changes

rohitpaulk force-pushed the limit-verifications-in-ui branch from dd375b8 to c3452ab Compare September 8, 2017 14:37

rohitpaulk closed this Sep 8, 2017

rohitpaulk reopened this Sep 8, 2017

chadwhitacre added 5 commits September 16, 2017 19:38

Implement deploy hooks

6c04359

Clean up hooks in deploy.sh

1bd7108

- don't want before.py (see deploy/README) - run after.py directly on dyno vs. redirect

Fix indentation

d1979c3

Clean up email listing

a5785dc

Get new mondo integration test passing

8eb5d0e

- dodge race condition around clicks and page reloading - factor out helpers into proper classes - simplify remove action to maintain proper state (presence of add form) - minor formatting tweak in sptfile

rohitpaulk force-pushed the limit-verifications-in-ui branch from c3452ab to 8eb5d0e Compare September 16, 2017 14:14

Use sorted() instead of a handmade loop

11a93a4

rohitpaulk added 2 commits September 17, 2017 16:20

Merge branch 'master' into limit-verifications-in-ui

4118310

Merge branch 'master' into limit-verifications-in-ui

6487dd8

rohitpaulk merged commit 048099f into master Sep 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit pending verifications in emails UI #4579

Limit pending verifications in emails UI #4579

chadwhitacre commented Aug 22, 2017 •

edited

Loading

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 31, 2017

chadwhitacre commented Sep 8, 2017

rohitpaulk Sep 8, 2017

chadwhitacre Sep 8, 2017

rohitpaulk Sep 8, 2017

rohitpaulk Sep 8, 2017 •

edited

Loading

rohitpaulk Sep 16, 2017

rohitpaulk Sep 8, 2017

rohitpaulk commented Sep 16, 2017

rohitpaulk commented Sep 16, 2017

chadwhitacre commented Sep 16, 2017

		from gratipay.testing import BrowserHarness, QueuedEmailHarness


		class Row(object):

Limit pending verifications in emails UI #4579

Limit pending verifications in emails UI #4579

Conversation

chadwhitacre commented Aug 22, 2017 • edited Loading

Todo

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 22, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017 • edited Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 • edited Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 • edited Loading

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 • edited Loading

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017 • edited Loading

chadwhitacre commented Aug 23, 2017

rohitpaulk commented Aug 23, 2017

chadwhitacre commented Aug 23, 2017

chadwhitacre commented Aug 31, 2017

chadwhitacre commented Sep 8, 2017

rohitpaulk Sep 8, 2017

Choose a reason for hiding this comment

chadwhitacre Sep 8, 2017

Choose a reason for hiding this comment

rohitpaulk Sep 8, 2017

Choose a reason for hiding this comment

rohitpaulk Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Sep 16, 2017

Choose a reason for hiding this comment

rohitpaulk Sep 8, 2017

Choose a reason for hiding this comment

rohitpaulk commented Sep 16, 2017

rohitpaulk commented Sep 16, 2017

chadwhitacre commented Sep 16, 2017

chadwhitacre commented Aug 22, 2017 •

edited

Loading

chadwhitacre commented Aug 23, 2017 •

edited

Loading

rohitpaulk commented Aug 23, 2017 •

edited

Loading

rohitpaulk commented Aug 23, 2017 •

edited

Loading

rohitpaulk commented Aug 23, 2017 •

edited

Loading

rohitpaulk commented Aug 23, 2017 •

edited

Loading

rohitpaulk Sep 8, 2017 •

edited

Loading