-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Normalize OSF Verification Key [#OSF-6560] #5964
Changes from 16 commits
c1bf666
16bab4f
01c28f7
9a6b2ba
7437496
ae0c145
7ecf373
b1a6581
c9dc259
c8a0fac
f46c8a0
d459d8f
2e4ea05
617d2eb
6be7705
e538758
93d947e
d6e09ab
a3114d4
9ceb096
7968675
810abe3
12d1308
9f3b5c5
28f8f83
52836d8
96b0c5e
1311351
f627c6b
8ad4a94
042e2ca
6dd18d2
0ca06e0
d9a5d64
7e862d9
576afb0
ffc82ec
6a95638
3385da7
bb65649
31d08eb
f357253
474c1a2
d589f24
c8bb69b
e3a4137
10419b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
# -*- coding: utf-8 -*- | ||
|
||
from copy import deepcopy | ||
import datetime as dt | ||
import itertools | ||
import logging | ||
import re | ||
import urlparse | ||
from copy import deepcopy | ||
|
||
import bson | ||
import pytz | ||
|
@@ -42,17 +43,19 @@ | |
logger = logging.getLogger(__name__) | ||
|
||
|
||
# Hide implementation of token generation | ||
def generate_confirm_token(): | ||
return security.random_string(30) | ||
|
||
|
||
def generate_claim_token(): | ||
# verification key v1 | ||
def generate_verification_key(): | ||
return security.random_string(30) | ||
|
||
|
||
def generate_verification_key(): | ||
return security.random_string(30) | ||
# verification key v2, expires in 30 minutes | ||
def generate_verification_key_v2(): | ||
token = security.random_string(30) | ||
expires = dt.datetime.utcnow() + dt.timedelta(minutes=settings.VERIFICATION_KEY_V2_EXPIRATION) | ||
return { | ||
'token': token, | ||
'expires': expires, | ||
} | ||
|
||
|
||
def validate_history_item(item): | ||
|
@@ -99,25 +102,60 @@ def validate_social(value): | |
validate_profile_websites(value.get('profileWebsites')) | ||
|
||
|
||
def validate_user_with_verification_key(username=None, verification_key=None): | ||
""" | ||
Validate requests with username and one-time verification key. | ||
|
||
:param username: user's username | ||
:param verification_key: one-time verification key | ||
:rtype: User or None | ||
""" | ||
|
||
if not username or not verification_key: | ||
return None | ||
user_obj = get_user(username=username) | ||
if user_obj: | ||
try: | ||
if user_obj.verification_key_v2: | ||
if user_obj.verification_key_v2['token'] == verification_key: | ||
if user_obj.verification_key_v2['expires'] > dt.datetime.utcnow(): | ||
return user_obj | ||
except AttributeError: | ||
# if user does not have verification_key_v2 | ||
return None | ||
return None | ||
|
||
|
||
# TODO - rename to _get_current_user_from_session /HRYBACKI | ||
def _get_current_user(): | ||
uid = session._get_current_object() and session.data.get('auth_user_id') | ||
return User.load(uid) | ||
|
||
|
||
# TODO: This should be a class method of User? | ||
def get_user(email=None, password=None, verification_key=None): | ||
"""Get an instance of User matching the provided params. | ||
|
||
def get_user(email=None, password=None, verification_key=None, username=None): | ||
""" | ||
Get an instance of User matching the provided params. Here are all valid usages: | ||
1. email and password | ||
2. email | ||
3. username (for verification key version 2) | ||
4. verification key (for verification key version 1) | ||
|
||
:param email: email | ||
:param password: password | ||
:param verification_key: verification key v1 | ||
:param username: username | ||
:return: The instance of User requested | ||
:rtype: User or None | ||
""" | ||
# tag: database | ||
if password and not email: | ||
raise AssertionError('If a password is provided, an email must also ' | ||
'be provided.') | ||
raise AssertionError('If a password is provided, an email must also be provided.') | ||
|
||
query_list = [] | ||
|
||
if username: | ||
query_list.append(Q('username', 'eq', username)) | ||
if email: | ||
email = email.strip().lower() | ||
query_list.append(Q('emails', 'eq', email) | Q('username', 'eq', email)) | ||
|
@@ -265,13 +303,14 @@ class User(GuidStoredObject, AddonModelMixin): | |
is_invited = fields.BooleanField(default=False, index=True) | ||
|
||
# Per-project unclaimed user data: | ||
# TODO: add validation | ||
# TODO: add a validation function that ensures that all required keys are present in the input values for that field | ||
unclaimed_records = fields.DictionaryField(required=False) | ||
# Format: { | ||
# <project_id>: { | ||
# 'name': <name that referrer provided>, | ||
# 'referrer_id': <user ID of referrer>, | ||
# 'token': <token used for verification urls>, | ||
# 'token': <verification key v1, used for verification urls>, | ||
# 'expires': <expiration time for this record>, | ||
# 'email': <email the referrer provided or None>, | ||
# 'claimer_email': <email the claimer entered or None>, | ||
# 'last_sent': <timestamp of last email sent to referrer or None> | ||
|
@@ -280,20 +319,27 @@ class User(GuidStoredObject, AddonModelMixin): | |
# } | ||
|
||
# Time of last sent notification email to newly added contributors | ||
contributor_added_email_records = fields.DictionaryField(default=dict) | ||
# Format : { | ||
# <project_id>: { | ||
# 'last_sent': time.time() | ||
# } | ||
# ... | ||
# } | ||
contributor_added_email_records = fields.DictionaryField(default=dict) | ||
|
||
# The user into which this account was merged | ||
merged_by = fields.ForeignField('user', default=None, index=True) | ||
|
||
# verification key used for resetting password | ||
# verification key v1, | ||
verification_key = fields.StringField() | ||
|
||
# verification key v2, with expiration time and one-time only | ||
verification_key_v2 = fields.DictionaryField(default=dict) | ||
# Format: { | ||
# 'token': <the verification key string> | ||
# 'expires': <the expiration time for the key> | ||
# } | ||
|
||
email_last_sent = fields.DateTimeField() | ||
|
||
# confirmed emails | ||
|
@@ -306,8 +352,11 @@ class User(GuidStoredObject, AddonModelMixin): | |
# see also ``unconfirmed_emails`` | ||
email_verifications = fields.DictionaryField(default=dict) | ||
# Format: { | ||
# <token> : {'email': <email address>, | ||
# 'expiration': <datetime>} | ||
# <token> : { | ||
# 'email': <email address>, | ||
# 'expiration': <datetime>, | ||
# 'confirmed': whether user is confirmed or not | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For CR: just updated the comments, no code change |
||
# } | ||
# } | ||
|
||
# TODO remove this field once migration (scripts/migration/migrate_mailing_lists_to_mailchimp_fields.py) | ||
|
@@ -601,7 +650,8 @@ def add_unclaimed_record(self, node, referrer, given_name, email=None): | |
record = { | ||
'name': given_name, | ||
'referrer_id': referrer_id, | ||
'token': generate_confirm_token(), | ||
'token': generate_verification_key(), | ||
'expires': dt.datetime.utcnow() + dt.timedelta(days=settings.CLAIM_TOKEN_EXPIRATION), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With suggested refactor above, this could become |
||
'email': clean_email | ||
} | ||
self.unclaimed_records[project_id] = record | ||
|
@@ -633,32 +683,50 @@ def is_active(self): | |
self.is_confirmed) | ||
|
||
def get_unclaimed_record(self, project_id): | ||
"""Get an unclaimed record for a given project_id. | ||
""" | ||
Get an unclaimed record for a given project_id. | ||
|
||
:raises: ValueError if there is no record for the given project. | ||
""" | ||
try: | ||
return self.unclaimed_records[project_id] | ||
except KeyError: # reraise as ValueError | ||
except KeyError: # re-raise as ValueError | ||
raise ValueError('No unclaimed record for user {self._id} on node {project_id}' | ||
.format(**locals())) | ||
.format(**locals())) | ||
|
||
def verify_claim_token(self, token, project_id): | ||
""" | ||
Verify the claim token for this user for a given node | ||
which she/he was added as a unregistered contributor for. | ||
|
||
:param token: the verification token | ||
:param project_id: the project node id | ||
:rtype boolean | ||
:return: whether or not a claim token is valid | ||
""" | ||
try: | ||
record = self.get_unclaimed_record(project_id) | ||
except ValueError: # No unclaimed record for given pid | ||
return False | ||
return record['token'] == token and record['expires'] > dt.datetime.utcnow() | ||
|
||
def get_claim_url(self, project_id, external=False): | ||
"""Return the URL that an unclaimed user should use to claim their | ||
account. Return ``None`` if there is no unclaimed_record for the given | ||
project ID. | ||
|
||
:param project_id: The project ID for the unclaimed record | ||
:raises: ValueError if a record doesn't exist for the given project ID | ||
:rtype: dict | ||
:returns: The unclaimed record for the project | ||
""" | ||
Return the URL that an unclaimed user should use to claim their account. | ||
Raise `ValueError` if there is no unclaimed_record for the given project ID. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For CR: updated comments to match the actual behavior: |
||
|
||
:param project_id: the project ID for the unclaimed record | ||
:param external: | ||
:rtype: string | ||
:returns: the claim url | ||
:raises: ValueError if there is no record for the given project. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For CR: not sure why explicitly throw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit that included the unit test said nothing of the rationale for including it. ¯_(ツ)_/¯ |
||
""" | ||
unclaimed_record = self.get_unclaimed_record(project_id) | ||
uid = self._primary_key | ||
base_url = settings.DOMAIN if external else '/' | ||
unclaimed_record = self.get_unclaimed_record(project_id) | ||
token = unclaimed_record['token'] | ||
return '{base_url}user/{uid}/{project_id}/claim/?token={token}'\ | ||
.format(**locals()) | ||
return '{base_url}user/{uid}/{project_id}/claim/?token={token}'.\ | ||
format(**locals()) | ||
|
||
def set_password(self, raw_password, notify=True): | ||
"""Set the password for this user to the hash of ``raw_password``. | ||
|
@@ -765,7 +833,7 @@ def add_unconfirmed_email(self, email, expiration=None): | |
if email in self.unconfirmed_emails: | ||
self.remove_unconfirmed_email(email) | ||
|
||
token = generate_confirm_token() | ||
token = generate_verification_key() | ||
|
||
# handle when email_verifications is None | ||
if not self.email_verifications: | ||
|
@@ -807,7 +875,7 @@ def _send_email_removal_confirmations(self, email): | |
removed_email=email, | ||
security_addr='primary email address ({})'.format(self.username)) | ||
|
||
def get_confirmation_token(self, email, force=False): | ||
def get_confirmation_token(self, email, force=False, renew=False): | ||
"""Return the confirmation token for a given email. | ||
|
||
:param str email: Email to get the token for. | ||
|
@@ -823,6 +891,10 @@ def get_confirmation_token(self, email, force=False): | |
# Old records will not have an expiration key. If it's missing, | ||
# assume the token is expired | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like, for old Other than for the password reset flow, when a v1 key is generated solely for communication with CAS, is there any time when a verification key will be generated without an expiration? If not, consider adding a 1 minute expiration to the keys sent to CAS, and deprecating the v1 StringField in favor of a DictionaryField of the same name (requires migration) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I will update CAS to check expiration as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not going to add this for CAS in this PR. The verification key is used immediately and distroyed. In addition, don't want CAS to block this. |
||
expiration = info.get('expiration') | ||
if renew: | ||
new_token = self.add_unconfirmed_email(email) | ||
self.save() | ||
return new_token | ||
if not expiration or (expiration and expiration < dt.datetime.utcnow()): | ||
if not force: | ||
raise ExpiredTokenError('Token for email "{0}" is expired'.format(email)) | ||
|
@@ -833,14 +905,14 @@ def get_confirmation_token(self, email, force=False): | |
return token | ||
raise KeyError('No confirmation token for email "{0}"'.format(email)) | ||
|
||
def get_confirmation_url(self, email, external=True, force=False): | ||
def get_confirmation_url(self, email, external=True, force=False, renew=False): | ||
"""Return the confirmation url for a given email. | ||
|
||
:raises: ExpiredTokenError if trying to access a token that is expired. | ||
:raises: KeyError if there is no token for the email. | ||
""" | ||
base = settings.DOMAIN if external else '/' | ||
token = self.get_confirmation_token(email, force=force) | ||
token = self.get_confirmation_token(email, force=force, renew=renew) | ||
return '{0}confirm/{1}/{2}/'.format(base, self._primary_key, token) | ||
|
||
def get_unconfirmed_email_for_token(self, token): | ||
|
@@ -875,16 +947,6 @@ def clean_email_verifications(self, given_token=None): | |
email_verifications.pop(token) | ||
self.email_verifications = email_verifications | ||
|
||
def verify_claim_token(self, token, project_id): | ||
"""Return whether or not a claim token is valid for this user for | ||
a given node which they were added as a unregistered contributor for. | ||
""" | ||
try: | ||
record = self.get_unclaimed_record(project_id) | ||
except ValueError: # No unclaimed record for given pid | ||
return False | ||
return record['token'] == token | ||
|
||
def confirm_email(self, token, merge=False): | ||
"""Confirm the email address associated with the token""" | ||
email = self.get_unconfirmed_email_for_token(token) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but if the main purpose of this PR is to normalize (as much as possible) user / password confirmation flow, and you're already de-duplicating token generation, why not have a single helper function for this? e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for forgot/reset password verification and contributor-ship claims. But for send/resend confirmation,
token
is used as key of the map. I will leave this unchanged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be handled by building
email_verifications
slightly differently inadd_unconfirmed_email
, i.e. the linescould be changed to something like
which also obviates the
_set_email_token_expiration
method. Again, not a blocker, but consider adding aTODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Since one of the goal for this PR is normalization, I will add this. :)