[Feature] Normalize OSF Verification Key [#OSF-6560] #5964

cslzchen · 2016-07-12T21:05:19Z

Update

Fix merge conflicts
Do not reveal user emails in email links
Normalize verification key
- forgot and reset password
- claim contributor-ship
- confirm registration or email
- update tests
Temporary Fix for OSF-6673
Several minor improvements
The following will be moved to a new feature ticket: https://openscience.atlassian.net/browse/OSF-6998.
- Users can resend confirmation email for contributor claim
- Bug fixing for OSF-6673

Purpose

Issue

When user forgets and resets password, verification key never gets deleted unless overwritten. In addition, validation only checks the key. Neither the user to whom it belongs nor whether it has expired is checked. Furthermore, verification key are used twice instead of "one-time" for several tasks.

Fix

Add User.verification_key_v2 which contains both the token string and an expiration time. Validation now checks username, token and expiration time. Renew the key once used.

Further Fix

There are two other functionality "User Claim Contributor-ship" and "Send/Resend Registration Confirmation Email" that have similar issue. Although they have their own way of verification, both are fixed with the same idea.

Changes

Normalized Verification Key

### defaults.py
# Expiration time for verification key
EXPIRATION_TIME_DICT = {
    'password': 30,         # 30 minutes for forgot and reset password
    'confirm': 24 * 60,     # 24 hours in minutes for confirm account and email
    'claim': 30 * 24 * 60    # 30 days in minutes for claim contributor-ship
}

### core.py
# generate verification key
def generate_verification_key(verification_type=None):
    """
    Generate a one-time verification key with an optional expiration time.
    The type of the verification key determines the expiration time defined in `website.settings.EXPIRATION_TIME_DICT`.

    :param verification_type: None, verify, confirm or claim
    :return: a string or a dictionary
    """
    token = security.random_string(30)
    # v1 with only the token
    if not verification_type:
        return token
    # v2 with a token and the expiration time
    expires = dt.datetime.utcnow() + dt.timedelta(minutes=settings.EXPIRATION_TIME_DICT[verification_type])
    return {
        'token': token,
        'expires': expires,
    }

### core.py/User
    verification_key = fields.StringField()

    verification_key_v2 = fields.DictionaryField(default=dict)
    # Format: {
    #   'token': <the verification token>
    #   'expires': <the verification expiration time>
    # }

    unclaimed_records = fields.DictionaryField(required=False)
    # Format: {
    #   <project_id>: {
    #       'name': <name that referrer provided>,
    #       'referrer_id': <user ID of referrer>,
    #       'token': <the verification token>,
    #       'expires': <the verification expiration time>,
    #       '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>
    #   }
    #   ...
    # }

    email_verifications = fields.DictionaryField(default=dict)
    # Format: {
    #   <token> : {
    #       'email': <email address>,
    #       'expiration': <datetime>,
    #       'confirmed': whether user is confirmed or not,
    #       'external_identity': user's external identity,
    #   }
    # }

Other

Resend confirmation will refresh the token.
Update get_or_create_user() with verification key.
Remove duplicated code get_claim_token() and get_confirm_token().
Remove "lingering" code fragments due to previous merge conflicts.
Update related routes, makos, factories and tests

Side effects

Password reset link becomes one time only. No longer valid once clicked.

Ticket

https://openscience.atlassian.net/browse/OSF-6560
https://openscience.atlassian.net/browse/SEC-33

…ime.

… right after use. [skip ci]

- use get_verfiication_key() for token generation - remove redundant get_confirm_token() and unused get_claim_token() - add 'expires' to each record - check token equality and expires when verifying claim

…skip ci]

… ci]" This reverts commit c8a0fac.

…ck. [skip ci]

- fix test_modles.TestUnregisteredUser - revert previous changes

…tion-key.

cslzchen · 2016-07-21T13:29:17Z

framework/auth/core.py

+    #   <token> : {
+    #       'email': <email address>,
+    #       'expiration': <datetime>,
+    #       'confirmed': whether user is confirmed or not


For CR: just updated the comments, no code change

…o feature/verification-key. - remove should-have-gone `reset_password()` form auth views - remove should-have-gone code segment which is part of `test_can_reset_password_if_form_success()` from web tests - update resetpassowrd.mako to include `${username}` in form action

cslzchen · 2016-08-04T20:56:48Z

framework/auth/views.py

-
-
-@collect_auth
-def reset_password(auth, **kwargs):


[To CR] Deprecated Code

cslzchen · 2016-09-02T16:15:04Z

As discussed with @mfraezz , the resend claim confirmation will be handled in another improvement/feature ticket: https://openscience.atlassian.net/browse/OSF-6998, which takes care of https://openscience.atlassian.net/browse/OSF-6673 along the way.

mfraezz · 2016-09-06T14:16:51Z

framework/auth/views.py

    """

-    # If user is already logged in, log user out
+    # TODO: discuss this with @MattF


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.

Can this be removed, then?

…tion-key.

cslzchen · 2016-09-06T18:41:56Z

framework/auth/views.py

-            'message_short': 'Invalid url.',
-            'message_long': 'The verification key in the URL is invalid or has expired.'
+            'message_short': 'Invalid Request.',
+            'message_long': 'The requested URL is invalid, has expired, or was already used',


update: a better message from product @saradbowman. The message is more informative without leaking the exact failure.

mfraezz · 2016-09-07T17:46:06Z

LGTM 🌳

Dropping this RTM

…o feature/verification-key.

brianjgeiger · 2016-09-20T20:19:54Z

@cslzchen There are some merge conflicts that need resolving.

- use verification key v2 - institution: no verificatiton key is generated - meetings: verification key v2 generated for resetting password

- replace `str verificiation_type` with `bool reset_password` - update usage and test

cslzchen · 2016-09-21T14:08:19Z

framework/auth/__init__.py

@@ -108,21 +108,24 @@ def register_unconfirmed(username, password, fullname, campaign=None):
    return user


-def get_or_create_user(fullname, address, is_spam=False):
-    """Get or create user by email address.
+def get_or_create_user(fullname, address, reset_password=True, is_spam=False):


add reset_password:

for new user from institution login, no need to ask user to reset password after creation, thus no verification key

for conference add_poster_by_email, create user and send user confirmation email to reset password, generate reset password verification key

cslzchen · 2016-09-21T14:13:55Z

website/settings/defaults.py

+EXPIRATION_TIME_DICT = {
+    'password': 30,         # 30 minutes for forgot and reset password
+    'confirm': 24 * 60,     # 24 hours in minutes for confirm account and email
+    'claim': 30 * 24 * 60   # 30 days in minutes for claim contributor-ship


set 30 days instead of 7 days claim token, change back to 7 days after resend claim confirmation is implemented: OSF-6998.

cslzchen · 2016-09-21T14:19:47Z

framework/auth/views.py

@@ -556,17 +553,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):


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

cslzchen · 2016-09-21T14:38:50Z

framework/auth/views.py

    """

-    # If user is already logged in, log user out
+    # if users are logged in, log them out and redirect back to this page
    if auth.logged_in:


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

cslzchen · 2016-09-21T14:41:20Z

framework/auth/views.py

+        # 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)


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

cslzchen added 5 commits July 12, 2016 10:11

Add verification_key_v2 which contains key, username and expiration t…

c1bf666

…ime.

Add generate_verification_key_v2().

16bab4f

Reorder imports order for auth core.

01c28f7

Forgot and reset password now use verfication key version 2.

9a6b2ba

Remove username from verification_key_v2, invalid verification_key_v2…

7437496

… right after use. [skip ci]

cslzchen changed the title ~~[WIP] [Feature] Check expiration time and username for verification key [OSF-#6560]~~ [WIP] [Feature] Check expiration time and username for verification key [#OSF-6560] Jul 13, 2016

cslzchen added 3 commits July 13, 2016 12:35

Add expiration check for user unclaimed_records.

ae0c145

- use get_verfiication_key() for token generation - remove redundant get_confirm_token() and unused get_claim_token() - add 'expires' to each record - check token equality and expires when verifying claim

Add expires to contributor claim.

7ecf373

Temporarily remove expires check for unclaimed_records. [skip ci]

b1a6581

cslzchen force-pushed the feature/verification-key branch from 14d7a1e to b1a6581 Compare July 15, 2016 15:37

cslzchen added 6 commits July 15, 2016 16:04

Renew email verifications token when resending confirmation. [skip ci]

c9dc259

Add empty claim_user_form_get and claim_user_form_post. [skip ci]

c8a0fac

Add expiration time to settings and enable contributor claim check. […

f46c8a0

…skip ci]

Revert "Add empty claim_user_form_get and claim_user_form_post. [skip…

d459d8f

… ci]" This reverts commit c8a0fac.

Fix web_url_for('reset_password_get') in conference views.

2e4ea05

Update tests for forgot and reset password, add attribute and key che…

617d2eb

…ck. [skip ci]

cslzchen force-pushed the feature/verification-key branch from b6c62c3 to 617d2eb Compare July 20, 2016 16:11

Raise ValueError in get_claim_url if no record on a given project.

6be7705

- fix test_modles.TestUnregisteredUser - revert previous changes

cslzchen force-pushed the feature/verification-key branch from 7c58b9b to 6be7705 Compare July 20, 2016 22:12

Merge remote-tracking branch 'upstream/develop' into feature/verifica…

e538758

…tion-key.

cslzchen changed the title ~~[WIP] [Feature] Check expiration time and username for verification key [#OSF-6560]~~ [Feature] Check expiration time and username for OSF verification key [#OSF-6560] Jul 21, 2016

cslzchen reviewed Jul 21, 2016
View reviewed changes

cslzchen force-pushed the feature/verification-key branch from 8a48de7 to 2039dca Compare August 4, 2016 20:55

cslzchen reviewed Aug 4, 2016
View reviewed changes

framework/auth/views.py

@collect_auth

def reset_password(auth, **kwargs):

Copy link

Contributor Author

cslzchen Aug 4, 2016

Choose a reason for hiding this comment

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

[To CR] Deprecated Code

Minor improvement and fixes after fixing conflicts.

d6e09ab

Temporary fix for #OSF-6673.

576afb0

cslzchen changed the title ~~[Feature] Normalize OSF Verification Key [#OSF-6560, #OSF-6673]~~ [Feature] Normalize OSF Verification Key [#OSF-6560] Sep 2, 2016

mfraezz reviewed Sep 6, 2016
View reviewed changes

cslzchen added 4 commits September 6, 2016 10:50

Minor sytle fix.

ffc82ec

Merge remote-tracking branch 'upstream/develop' into feature/verifica…

6a95638

…tion-key.

Remove deprecated api call to reset_password_post.

3385da7

Remove duplicated code for pushing status message.

bb65649

cslzchen reviewed Sep 6, 2016
View reviewed changes

Improve error message with product.

31d08eb

cslzchen force-pushed the feature/verification-key branch from 468d802 to 31d08eb Compare September 6, 2016 19:33

Remove #TODO.

f357253

Fix conflicts and merge remote-tracking branch 'upstream/develop' int…

474c1a2

…o feature/verification-key.

cslzchen added 3 commits September 20, 2016 17:42

Remove unnecessary use of generate_verification_key().

d589f24

Update verification key generation in auth.get_or_create_user().

c8bb69b

- use verification key v2 - institution: no verificatiton key is generated - meetings: verification key v2 generated for resetting password

Update auth.get_or_create_user():

e3a4137

- replace `str verificiation_type` with `bool reset_password` - update usage and test

cslzchen commented Sep 21, 2016

View reviewed changes

Minor fixes.

10419b7

cslzchen commented Sep 21, 2016

View reviewed changes

cslzchen force-pushed the feature/verification-key branch from 30ed520 to 10419b7 Compare September 21, 2016 14:43

brianjgeiger merged commit 53419a5 into CenterForOpenScience:develop Sep 28, 2016

cslzchen deleted the feature/verification-key branch February 17, 2017 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Normalize OSF Verification Key [#OSF-6560] #5964

[Feature] Normalize OSF Verification Key [#OSF-6560] #5964

cslzchen commented Jul 12, 2016 •

edited

Loading

cslzchen Jul 21, 2016

cslzchen Aug 4, 2016

cslzchen commented Sep 2, 2016

mfraezz Sep 6, 2016

cslzchen Sep 6, 2016 •

edited

Loading

mfraezz Sep 7, 2016

cslzchen Sep 6, 2016

mfraezz commented Sep 7, 2016

brianjgeiger commented Sep 20, 2016

cslzchen Sep 21, 2016

cslzchen Sep 21, 2016

cslzchen Sep 21, 2016

cslzchen Sep 21, 2016 •

edited

Loading

cslzchen Sep 21, 2016

[Feature] Normalize OSF Verification Key [#OSF-6560] #5964

[Feature] Normalize OSF Verification Key [#OSF-6560] #5964

Conversation

cslzchen commented Jul 12, 2016 • edited Loading

Update

Purpose

Issue

Fix

Further Fix

Changes

Normalized Verification Key

Other

Side effects

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen commented Sep 2, 2016

Choose a reason for hiding this comment

cslzchen Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfraezz commented Sep 7, 2016

brianjgeiger commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen commented Jul 12, 2016 •

edited

Loading

cslzchen Sep 6, 2016 •

edited

Loading

cslzchen Sep 21, 2016 •

edited

Loading