Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
c1bf666
Add verification_key_v2 which contains key, username and expiration t…
cslzchen Jul 12, 2016
16bab4f
Add generate_verification_key_v2().
cslzchen Jul 12, 2016
01c28f7
Reorder imports order for auth core.
cslzchen Jul 12, 2016
9a6b2ba
Forgot and reset password now use verfication key version 2.
cslzchen Jul 12, 2016
7437496
Remove username from verification_key_v2, invalid verification_key_v2…
cslzchen Jul 12, 2016
ae0c145
Add expiration check for user unclaimed_records.
cslzchen Jul 13, 2016
7ecf373
Add `expires` to contributor claim.
cslzchen Jul 15, 2016
b1a6581
Temporarily remove `expires` check for unclaimed_records. [skip ci]
cslzchen Jul 15, 2016
c9dc259
Renew email verifications token when resending confirmation. [skip ci]
cslzchen Jul 15, 2016
c8a0fac
Add empty claim_user_form_get and claim_user_form_post. [skip ci]
cslzchen Jul 17, 2016
f46c8a0
Add expiration time to settings and enable contributor claim check. […
cslzchen Jul 19, 2016
d459d8f
Revert "Add empty claim_user_form_get and claim_user_form_post. [skip…
cslzchen Jul 19, 2016
2e4ea05
Fix web_url_for('reset_password_get') in conference views.
cslzchen Jul 20, 2016
617d2eb
Update tests for forgot and reset password, add attribute and key che…
cslzchen Jul 20, 2016
6be7705
Raise ValueError in get_claim_url if no record on a given project.
cslzchen Jul 20, 2016
e538758
Merge remote-tracking branch 'upstream/develop' into feature/verifica…
cslzchen Jul 20, 2016
93d947e
Fix conflicts and merge remote-tracking branch 'upstream/develop' int…
cslzchen Aug 4, 2016
d6e09ab
Minor improvement and fixes after fixing conflicts.
cslzchen Aug 4, 2016
a3114d4
start from longze's base
chennan47 Aug 8, 2016
9ceb096
fix the unclaimed contributor reset passowrd
chennan47 Aug 8, 2016
7968675
Merge remote-tracking branch 'upstream' into feature/verification-key.
cslzchen Aug 8, 2016
810abe3
merge
chennan47 Aug 8, 2016
12d1308
merge
chennan47 Aug 8, 2016
9f3b5c5
Merge remote-tracking branch 'upstream/develop' into feature/verifica…
cslzchen Aug 8, 2016
28f8f83
merge from longze
chennan47 Aug 8, 2016
52836d8
Merge pull request #11 from chennan47/feature/reset_password.
cslzchen Aug 8, 2016
96b0c5e
Fix conflicts and merge remote-tracking branch 'upstream/develop' int…
cslzchen Aug 31, 2016
1311351
Use email instead of username in `get_user()`. [skip ci]
cslzchen Aug 31, 2016
f627c6b
Normalize verification key generation based on type.
cslzchen Sep 1, 2016
8ad4a94
Forgot/Reset password use normalized verification key: [skip ci]
cslzchen Sep 1, 2016
042e2ca
Improve comment and logic for claim contributor-ship:
cslzchen Sep 1, 2016
6dd18d2
Claim contributor-ship uses normalized verification key.
cslzchen Sep 1, 2016
0ca06e0
Send/Resend confirmation uses normalized verification key. [skip ci]
cslzchen Sep 1, 2016
d9a5d64
Remove deprecated code.
cslzchen Sep 2, 2016
7e862d9
Update unit tests.
cslzchen Sep 2, 2016
576afb0
Temporary fix for #OSF-6673.
cslzchen Sep 2, 2016
ffc82ec
Minor sytle fix.
cslzchen Sep 6, 2016
6a95638
Merge remote-tracking branch 'upstream/develop' into feature/verifica…
cslzchen Sep 6, 2016
3385da7
Remove deprecated api call to `reset_password_post`.
cslzchen Sep 6, 2016
bb65649
Remove duplicated code for pushing status message.
cslzchen Sep 6, 2016
31d08eb
Improve error message with product.
cslzchen Sep 6, 2016
f357253
Remove `#TODO`.
cslzchen Sep 7, 2016
474c1a2
Fix conflicts and merge remote-tracking branch 'upstream/develop' int…
cslzchen Sep 20, 2016
d589f24
Remove unnecessary use of `generate_verification_key()`.
cslzchen Sep 20, 2016
c8bb69b
Update verification key generation in `auth.get_or_create_user()`.
cslzchen Sep 20, 2016
e3a4137
Update `auth.get_or_create_user()`:
cslzchen Sep 20, 2016
10419b7
Minor fixes.
cslzchen Sep 21, 2016
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
2 changes: 1 addition & 1 deletion api/base/authentication/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
class ODMBackend(object):

def authenticate(self, username=None, password=None):
return get_user(username=username, password=password) or None
return get_user(email=username, password=password) or None

def get_user(self, user_id):
return User.load(user_id)
229 changes: 142 additions & 87 deletions framework/auth/core.py

Large diffs are not rendered by default.

216 changes: 112 additions & 104 deletions framework/auth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,95 +36,74 @@


@collect_auth
def reset_password_get(auth, verification_key=None, **kwargs):
def reset_password_get(auth, uid=None, token=None):
"""
View for user to land on the reset password page.
HTTp Method: GET

:raises: HTTPError(http.BAD_REQUEST) if verification_key is invalid
:param auth: the authentication state
:param uid: the user id
:param token: the token in verification key
:return
:raises: HTTPError(http.BAD_REQUEST) if verification key for the user is invalid or has expired
"""

# If user is already logged in, log user out
# TODO: discuss this with @MattF
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

@cslzchen cslzchen Sep 6, 2016

Choose a reason for hiding this comment

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

Please ignore this # TODO, it was supposed to be added here https://github.com/CenterForOpenScience/osf.io/pull/5964/files#diff-9a64b4982d0bd80f7a498c65ddce2023R138.
As discussed on Friday, the decision is to log users out if they are already logged in instead of redirecting them to dashboard for both reset_password_get and forgot_password_get.

Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed, then?

# if users are logged in, log them out and redirect back to this page
if auth.logged_in:
Copy link
Collaborator Author

@cslzchen cslzchen Sep 21, 2016

Choose a reason for hiding this comment

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

"log user out and redirect back" is more secure and less confusing than just "go to dashboard"
same for forgot_password_get

return auth_logout(redirect_url=request.url)

# Check if request bears a valid verification_key
user_obj = get_user(verification_key=verification_key)
if not user_obj:
# Check if request bears a valid pair of `uid` and `token`
user_obj = User.load(uid)
if not (user_obj and user_obj.verify_password_token(token=token)):
# TODO: do we want to reveal detailed error message to the client?
error_data = {
'message_short': 'Invalid url.',
'message_long': 'The verification key in the URL is invalid or has expired.'
'message_short': 'Invalid Request.',
'message_long': 'The request URL is invalid, has been expired or already used',
Copy link
Member

Choose a reason for hiding this comment

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

Did product request this language change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but I will talk with them on this change. The old message is no longer proper after our verification key normalization.

}
raise HTTPError(400, data=error_data)

return {
'verification_key': verification_key,
}
raise HTTPError(http.BAD_REQUEST, data=error_data)


@collect_auth
def reset_password(auth, **kwargs):
""" Show reset password page.
"""
if auth.logged_in:
return auth_logout(redirect_url=request.url)
verification_key = kwargs['verification_key']

# Check if request bears a valid verification_key
user_obj = get_user(verification_key=verification_key)
if not user_obj:
error_data = {
'message_short': 'Invalid url.',
'message_long': 'The verification key in the URL is invalid or has expired.'
}
raise HTTPError(400, data=error_data)
# refresh the verification key (v2)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[To CR] Deprecated Code

user_obj.verification_key_v2 = generate_verification_key(verification_type='password')
user_obj.save()

return {
'verification_key': verification_key
'uid': user_obj._id,
'token': user_obj.verification_key_v2['token'],
}


@collect_auth
def forgot_password_get(auth, **kwargs):
"""
View to user to land on forgot password page.
HTTP Method: GET
"""

# If user is already logged in, redirect to dashboard page.
if auth.logged_in:
return redirect(web_url_for('dashboard'))

return {}


@collect_auth
def reset_password_post(auth, verification_key=None, **kwargs):
def reset_password_post(uid=None, token=None):
"""
View for user to submit reset password form.
HTTP Method: POST

:param uid: the user id
:param token: the token in verification key
:return:
:raises: HTTPError(http.BAD_REQUEST) if verification_key is invalid
"""

# If user is already logged in, log user out
if auth.logged_in:
return auth_logout(redirect_url=request.url)

form = ResetPasswordForm(request.form)

# Check if request bears a valid verification_key
user_obj = get_user(verification_key=verification_key)
if not user_obj:
# Check if request bears a valid pair of `uid` and `token`
user_obj = User.load(uid)
if not (user_obj and user_obj.verify_password_token(token=token)):
# TODO: do we want to reveal detailed error message to the client?
error_data = {
'message_short': 'Invalid url.',
'message_long': 'The verification key in the URL is invalid or has expired.'
'message_short': 'Invalid Request.',
'message_long': 'The request URL is invalid, has been expired or already used',
}
raise HTTPError(400, data=error_data)
raise HTTPError(http.BAD_REQUEST, data=error_data)

if form.validate():
# new random verification key, allows CAS to authenticate the user w/o password, one-time only.
# this overwrite also invalidates the verification key generated by forgot_password_post
user_obj.verification_key = generate_verification_key()
if not form.validate():
# Don't go anywhere
forms.push_errors_to_status(form.errors)
else:
# clear verification key (v2)
user_obj.verification_key_v2 = {}
# new verification key (v1) for CAS
user_obj.verification_key = generate_verification_key(verification_type=None)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cas login with username and verification key still uses v1 given it is instant verification

try:
user_obj.set_password(form.password.data)
user_obj.save()
Expand All @@ -133,71 +112,92 @@ def reset_password_post(auth, verification_key=None, **kwargs):
status.push_status_message(message, kind='warning', trust=False)
else:
status.push_status_message('Password reset', kind='success', trust=False)
# redirect to CAS and authenticate the user with the one-time verification key.
# redirect to CAS and authenticate the user automatically with one-time verification key.
return redirect(cas.get_login_url(
web_url_for('user_account', _absolute=True),
username=user_obj.username,
verification_key=user_obj.verification_key
))
else:
forms.push_errors_to_status(form.errors)
# Don't go anywhere

return {
'verification_key': verification_key
}, 400
'uid': user_obj._id,
'token': user_obj.verification_key_v2['token'],
}


@collect_auth
def forgot_password_post(auth, **kwargs):
def forgot_password_get(auth):
"""
View for user to submit forgot password form.
HTTP Method: POST
View for user to land on the forgot password page.
HTTP Method: GET

:param auth: the authentication context
:return
"""

# If user is already logged in, redirect to dashboard page.
# if users are logged in, log them out and redirect back to this page
if auth.logged_in:
return redirect(web_url_for('dashboard'))
return auth_logout(redirect_url=request.url)

return {}


def forgot_password_post():
"""
View for user to submit forgot password form.
HTTP Method: POST
:return {}
"""

form = ForgotPasswordForm(request.form, prefix='forgot_password')

if form.validate():
if not form.validate():
# Don't go anywhere
forms.push_errors_to_status(form.errors)
else:
email = form.email.data
status_message = ('If there is an OSF account associated with {0}, an email with instructions on how to '
'reset the OSF password has been sent to {0}. If you do not receive an email and believe '
'you should have, please contact OSF Support. ').format(email)
# check if the user exists
user_obj = get_user(email=email)
if user_obj:
# check forgot_password rate limit
if throttle_period_expired(user_obj.email_last_sent, settings.SEND_EMAIL_THROTTLE):
# new random verification key, allows OSF to check whether the reset_password request is valid,
# this verification key is used twice, one for GET reset_password and one for POST reset_password
# and it will be destroyed when POST reset_password succeeds
user_obj.verification_key = generate_verification_key()
user_obj.email_last_sent = datetime.datetime.utcnow()
user_obj.save()
reset_link = furl.urljoin(
settings.DOMAIN,
web_url_for(
'reset_password_get',
verification_key=user_obj.verification_key
)
)
mails.send_mail(
to_addr=email,
mail=mails.FORGOT_PASSWORD,
reset_link=reset_link
)
status.push_status_message(status_message, kind='success', trust=False)
else:
if not user_obj:
# do not reveal user existence information by pushing success message even when user not found
Copy link
Member

@mfraezz mfraezz Sep 6, 2016

Choose a reason for hiding this comment

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

This can be accomplished without code duplication by putting the call to push_status_message outside this conditional. Not a blocker

status.push_status_message(status_message, kind='success', trust=False)
else:
# rate limit forgot_password_post
if not throttle_period_expired(user_obj.email_last_sent, settings.SEND_EMAIL_THROTTLE):
status.push_status_message('You have recently requested to change your password. Please wait a '
'few minutes before trying again.', kind='error', trust=False)
else:
status.push_status_message(status_message, kind='success', trust=False)
else:
forms.push_errors_to_status(form.errors)
# Don't go anywhere
else:
# TODO [OSF-6673]: Use the feature in [OSF-6998] for user to resend claim email.
# if the user account is not claimed yet
if (user_obj.is_invited and
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mfraezz this part is added by @chennan47 as a quick fix for issue https://openscience.atlassian.net/browse/OSF-6673.

user_obj.unclaimed_records and
not user_obj.date_last_login and
not user_obj.is_claimed and
not user_obj.is_registered):
status.push_status_message('You cannot reset password on this account. Please contact OSF Support.',
kind='error', trust=False)
else:
# new random verification key (v2)
user_obj.verification_key_v2 = generate_verification_key(verification_type='password')
user_obj.email_last_sent = datetime.datetime.utcnow()
user_obj.save()
reset_link = furl.urljoin(
settings.DOMAIN,
web_url_for(
'reset_password_get',
uid=user_obj._id,
token=user_obj.verification_key_v2['token']
)
)
mails.send_mail(
to_addr=email,
mail=mails.FORGOT_PASSWORD,
reset_link=reset_link
)
status.push_status_message(status_message, kind='success', trust=False)

return {}

Expand Down Expand Up @@ -546,17 +546,25 @@ def unconfirmed_email_add(auth=None):
}, 200


def send_confirm_email(user, email, external_id_provider=None, external_id=None):
def send_confirm_email(user, email, renew=False, external_id_provider=None, external_id=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when user asks to resend confirmation email, refresh the token with renew = True

"""
Sends a confirmation email to `user` to a given email.
Sends `user` a confirmation to the given `email`.


:param user: the user
:param email: the email
:param renew: refresh the token
:param external_id_provider: user's external id provider
:param external_id: user's external id
:return:
:raises: KeyError if user does not have a confirmation token for the given email.
"""

confirmation_url = user.get_confirmation_url(
email,
external=True,
force=True,
renew=renew,
external_id_provider=external_id_provider
)

Expand Down Expand Up @@ -705,7 +713,7 @@ def resend_confirmation_post(auth):
if user:
if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE):
try:
send_confirm_email(user, clean_email)
send_confirm_email(user, clean_email, renew=True)
except KeyError:
# already confirmed, redirect to dashboard
status_message = 'This email {0} has already been confirmed.'.format(clean_email)
Expand Down
1 change: 0 additions & 1 deletion framework/sessions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# -*- coding: utf-8 -*-

from datetime import datetime

import httplib as http
import urllib
import urlparse
Expand Down
1 change: 1 addition & 0 deletions tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class Meta:
merged_by = None
email_verifications = {}
verification_key = None
verification_key_v2 = {}

@post_generation
def set_names(self, create, extracted):
Expand Down
1 change: 1 addition & 0 deletions tests/models/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ def test_merge(self, mock_get_mailchimp_api):
'username',
'mailing_lists',
'verification_key',
'verification_key_v2',
'_affiliated_institutions',
'contributor_added_email_records',
'requested_deactivation',
Expand Down
1 change: 1 addition & 0 deletions tests/test_conferences.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ def test_default_field_names(self):
assert_equal(conf.field_names['submission1'], 'poster')
assert_equal(conf.field_names['mail_subject'], 'Presentation title')


class TestConferenceIntegration(ContextTestCase):

@mock.patch('website.conferences.views.send_mail')
Expand Down
2 changes: 1 addition & 1 deletion tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ def test_verify_confirmation_token(self):
valid_token = u.get_confirmation_token('foo@bar.com')
assert_true(u.get_unconfirmed_email_for_token(valid_token))
manual_expiration = datetime.datetime.utcnow() - datetime.timedelta(0, 10)
u._set_email_token_expiration(valid_token, expiration=manual_expiration)
u.email_verifications[valid_token]['expiration'] = manual_expiration

with assert_raises(auth_exc.ExpiredTokenError):
u.get_unconfirmed_email_for_token(valid_token)
Expand Down
Loading