Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Add email auth #4539

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions emails/signin_link.spt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{{ _("Sign in to Gratipay") }}

[---] text/html
{{ _( "Click the button below to sign in to Gratipay. "
"This link will expire in 1 hour and can only be used once.") }}
<br>
<br>
<a href="{{ signin_link }}" style="{{ button_style }}">{{ _("Sign in to Gratipay") }}</a>

[---] text/plain

{{ _( "Click the link below to sign in to Gratipay. "
"This link will expire in 1 hour and can only be used once.") }}

{{ signin_link }}
15 changes: 15 additions & 0 deletions emails/signup_link.spt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{{ _("Finish creating your account on Gratipay") }}

[---] text/html
{{ _( "Click the button below to create an account on Gratipay. "
"This link will expire in 1 hour and can only be used once.") }}
<br>
<br>
<a href="{{ signup_link }}" style="{{ button_style }}">{{ _("Create an account on Gratipay") }}</a>

[---] text/plain

{{ _( "Click the link below to create an account on Gratipay. "
"This link will expire in 1 hour and can only be used once.") }}

{{ signup_link }}
92 changes: 76 additions & 16 deletions gratipay/email.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@ def _have_ses(self, env):
and env.aws_ses_default_region


def put(self, to, template, _user_initiated=True, **context):
def put(self, to, template, _user_initiated=True, email=None, **context):
"""Put an email message on the queue.

:param Participant to: the participant to send the email message to
:param Participant to: the participant to send the email message to.
In cases where an email is not linked to a participant, this can be
``None``.
:param unicode template: the name of the template to use when rendering
the email, corresponding to a filename in ``emails/`` without the
file extension
:param unicode email: The email address to send this message to. If not
provided, the ``to`` participant's primary email is used.
:param bool _user_initiated: user-initiated emails are throttled;
system-initiated messages don't count against throttling
:param dict context: the values to use when rendering the template
Expand All @@ -73,19 +77,57 @@ def put(self, to, template, _user_initiated=True, **context):
:returns: ``None``

"""

assert to or email # Either participant or email address required.

with self.db.get_cursor() as cursor:
participant_id = to.id if to else None
cursor.run("""
INSERT INTO email_queue
(participant, spt_name, context, user_initiated)
VALUES (%s, %s, %s, %s)
""", (to.id, template, pickle.dumps(context), _user_initiated))
(participant,
email_address,
spt_name,
context,
user_initiated)
VALUES (%s, %s, %s, %s, %s)
""", (participant_id, email, template, pickle.dumps(context), _user_initiated))

if _user_initiated:
n = cursor.one('SELECT count(*) FROM email_queue '
'WHERE participant=%s AND user_initiated', (to.id,))
if n > self.allow_up_to:
nqueued = self._get_nqueued(cursor, to, email)
if nqueued > self.allow_up_to:
raise Throttled()


def _get_nqueued(self, cursor, participant, email_address):
"""Returns the number of messages already queued for the given
participant or email address. Prefers participant if provided, falls
back to email_address otherwise.

:param Participant participant: The participant to check queued messages
for.

:param unicode email_address: The email address to check queued messages
for.

:returns number of queued messages
"""

if participant:
return cursor.one("""
SELECT COUNT(*)
FROM email_queue
WHERE user_initiated
AND participant=%s
""", (participant.id, ))
else:
return cursor.one("""
SELECT COUNT(*)
FROM email_queue
WHERE user_initiated
AND email_address=%s
""", (email_address, ))


def flush(self):
"""Load messages queued for sending, and send them.
"""
Expand Down Expand Up @@ -142,22 +184,35 @@ def _prepare_email_message_for_ses(self, rec):
#. ``participant.email_address``.

"""
to = Participant.from_id(rec.participant)
participant = Participant.from_id(rec.participant)
spt = self._email_templates[rec.spt_name]
context = pickle.loads(rec.context)

context['participant'] = to
context['username'] = to.username
email = rec.email_address or participant.email_address

# Previously, email_address was stored in the 'email' key on `context`
# and not in the `email_address` field. Let's handle that case so that
# old emails don't suffer
#
# TODO: Remove this once we're sure old emails have gone out.
email = context.get('email', email)

if not email:
return None

context['email'] = email
if participant:
context['participant'] = participant
context['username'] = participant.username
context['button_style'] = (
"color: #fff; text-decoration:none; display:inline-block; "
"padding: 0 15px; background: #396; white-space: nowrap; "
"font: normal 14px/40px Arial, sans-serif; border-radius: 3px"
)
context.setdefault('include_unsubscribe', True)
email = context.setdefault('email', to.email_address)
if not email:
return None
langs = i18n.parse_accept_lang(to.email_lang or 'en')

accept_lang = (participant and participant.email_lang) or 'en'
langs = i18n.parse_accept_lang(accept_lang)
locale = i18n.match_lang(langs)
i18n.add_helpers_to_context(self.tell_sentry, context, locale)
context['escape'] = lambda s: s
Expand All @@ -172,7 +227,12 @@ def render(t, context):
message = {}
message['Source'] = 'Gratipay Support <[email protected]>'
message['Destination'] = {}
message['Destination']['ToAddresses'] = ["%s <%s>" % (to.username, email)] # "Name <[email protected]>"
if participant:
# "username <[email protected]>"
destination = "%s <%s>" % (participant.username, email)
else:
destination = email
message['Destination']['ToAddresses'] = [destination]
message['Message'] = {}
message['Message']['Subject'] = {}
message['Message']['Subject']['Data'] = spt['subject'].render(context).strip()
Expand Down
12 changes: 6 additions & 6 deletions gratipay/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@
from gratipay.utils.i18n import LocalizedErrorResponse


class ProblemChangingUsername(Exception):
class ProblemWithUsername(Exception):
def __str__(self):
return self.msg.format(self.args[0])

class UsernameIsEmpty(ProblemChangingUsername):
class UsernameIsEmpty(ProblemWithUsername):
msg = "You need to provide a username!"

class UsernameTooLong(ProblemChangingUsername):
class UsernameTooLong(ProblemWithUsername):
msg = "The username '{}' is too long."

class UsernameContainsInvalidCharacters(ProblemChangingUsername):
class UsernameContainsInvalidCharacters(ProblemWithUsername):
msg = "The username '{}' contains invalid characters."

class UsernameIsRestricted(ProblemChangingUsername):
class UsernameIsRestricted(ProblemWithUsername):
msg = "The username '{}' is restricted."

class UsernameAlreadyTaken(ProblemChangingUsername):
class UsernameAlreadyTaken(ProblemWithUsername):
msg = "The username '{}' is already taken."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can lead to username enumeration. Although usernames are public on Gratipay, I still think it is best practice to prevent username enumeration on the account creation endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EdOverflow Is there a way around this, that doesn't sacrifice UX?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it, username enumeration is not a risk for us because usernames are public (as you mentioned). Whether they are exposed via this endpoint or a normal /~username endpoint shouldn't make a difference

Copy link
Contributor

@EdOverflow EdOverflow Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitpaulk I have thought it through and agree with you. Let's leave it as it currently is.



Expand Down
19 changes: 11 additions & 8 deletions gratipay/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,23 +172,26 @@ def _check_no_team_balances(cursor):
def _check_orphans(cursor):
"""
Finds participants that
* does not have corresponding elsewhere account
* do not have a verified email address (i.e. did not signup via email)
* do not have corresponding elsewhere account
* have not been absorbed by other participant

These are broken because new participants arise from elsewhere
and elsewhere is detached only by take over which makes a note
in absorptions if it removes the last elsewhere account.
These are broken because participants without an email attached arise from
elsewhere (signup via third-party providers), and elsewhere is detached
only by take over which makes a note in absorptions if it removes the last
elsewhere account.

Especially bad case is when also claimed_time is set because
there must have been elsewhere account attached and used to sign in.

https://github.com/gratipay/gratipay.com/issues/617
"""
orphans = cursor.all("""
select username
from participants
where not exists (select * from elsewhere where elsewhere.participant=username)
and not exists (select * from absorptions where archived_as=username)
SELECT username
FROM participants
WHERE participants.email_address IS NULL
AND NOT EXISTS (SELECT * FROM elsewhere WHERE participant=username)
AND NOT EXISTS (SELECT * FROM absorptions WHERE archived_as=username)
""")
assert len(orphans) == 0, "missing elsewheres: {}".format(list(orphans))

Expand Down
4 changes: 2 additions & 2 deletions gratipay/models/account_elsewhere.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import xmltodict

import gratipay
from gratipay.exceptions import ProblemChangingUsername
from gratipay.exceptions import ProblemWithUsername
from gratipay.security.crypto import constant_time_compare
from gratipay.utils.username import safely_reserve_a_username

Expand Down Expand Up @@ -232,7 +232,7 @@ def opt_in(self, desired_username):
user.participant.set_as_claimed()
try:
user.participant.change_username(desired_username)
except ProblemChangingUsername:
except ProblemWithUsername:
pass
if user.participant.is_closed:
user.participant.update_is_closed(False)
Expand Down
Loading