From 9775acd9bfe811adad162c9c24920a075aa62309 Mon Sep 17 00:00:00 2001 From: Deimos Date: Thu, 16 Aug 2018 13:22:41 -0600 Subject: [PATCH] Make some follow-up adjustments to 2FA Nothing too significant in here, just a few adjustments and other follow-ups that I wanted to do: * Make backup code usage a bit more lenient - allow uppercase, and doesn't need exact spacing as originally displayed. * Display the backup codes a little more nicely. * Change the message on the settings page based on whether 2FA is enabled or not. * Use webargs instead of request.params.get --- tildes/tildes/lib/string.py | 17 ++++++++ tildes/tildes/models/user/user.py | 11 +++--- tildes/tildes/models/user/user_invite_code.py | 12 +----- .../intercooler/two_factor_enabled.jinja2 | 11 +++--- tildes/tildes/templates/settings.jinja2 | 9 ++++- .../templates/settings_two_factor.jinja2 | 2 +- tildes/tildes/views/api/web/user.py | 39 ++++++++----------- tildes/tildes/views/login.py | 5 ++- tildes/tildes/views/settings.py | 1 + 9 files changed, 59 insertions(+), 48 deletions(-) diff --git a/tildes/tildes/lib/string.py b/tildes/tildes/lib/string.py index 75be1a5..b830d03 100644 --- a/tildes/tildes/lib/string.py +++ b/tildes/tildes/lib/string.py @@ -188,3 +188,20 @@ def _sanitize_characters(original: str) -> str: final_characters.append(char) return "".join(final_characters) + + +def separate_string(original: str, separator: str, segment_size: int) -> str: + """Separate a string into "segments", inserting a separator every X chars. + + This is useful for strings being used as "codes" such as invite codes and 2FA backup + codes, so that they can be displayed in a more easily-readable format. + """ + separated = "" + + for count, char in enumerate(original): + if count > 0 and count % segment_size == 0: + separated += separator + + separated += char + + return separated diff --git a/tildes/tildes/models/user/user.py b/tildes/tildes/models/user/user.py index 36099e5..c808c0f 100644 --- a/tildes/tildes/models/user/user.py +++ b/tildes/tildes/models/user/user.py @@ -4,7 +4,7 @@ from datetime import datetime from typing import Any, List, Optional, Sequence, Tuple from mypy_extensions import NoReturn -import pyotp +from pyotp import TOTP from pyramid.security import ( ALL_PERMISSIONS, Allow, @@ -167,12 +167,13 @@ class User(DatabaseModel): def is_correct_two_factor_code(self, code: str) -> bool: """Verify that a TOTP/backup code is correct.""" - totp = pyotp.TOTP(self.two_factor_secret) - code = code.strip() + totp = TOTP(self.two_factor_secret) - if totp.verify(code.replace(" ", "")): + code = code.strip().replace(" ", "").lower() + + if totp.verify(code): return True - elif code in self.two_factor_backup_codes: + elif self.two_factor_backup_codes and code in self.two_factor_backup_codes: # Need to set the attribute so SQLAlchemy knows it changed self.two_factor_backup_codes = [ backup_code diff --git a/tildes/tildes/models/user/user_invite_code.py b/tildes/tildes/models/user/user_invite_code.py index e40b166..0249581 100644 --- a/tildes/tildes/models/user/user_invite_code.py +++ b/tildes/tildes/models/user/user_invite_code.py @@ -7,6 +7,7 @@ import string from sqlalchemy import CheckConstraint, Column, ForeignKey, Integer, Text, TIMESTAMP from sqlalchemy.sql.expression import text +from tildes.lib.string import separate_string from tildes.models import DatabaseModel from .user import User @@ -36,16 +37,7 @@ class UserInviteCode(DatabaseModel): def __str__(self) -> str: """Format the code into a more easily readable version.""" - formatted = "" - - for count, char in enumerate(self.code): - # add a dash every 5 chars - if count > 0 and count % 5 == 0: - formatted += "-" - - formatted += char.upper() - - return formatted + return separate_string(self.code, "-", 5) def __init__(self, user: User) -> None: """Create a new (random) invite code owned by the user. diff --git a/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 b/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 index 2574d8a..e3f97fc 100644 --- a/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 +++ b/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 @@ -1,10 +1,11 @@

Congratulations! Two-factor authentication has been enabled.

-

These are your backup codes. Make sure to write them down and store them in a safe place. -In the event that you lose access to your authenticator device, you will need these to regain access to your account. Each code can only be used once.

+

These are your backup codes. In the event that you lose access to your authenticator device, you will need one of these codes to regain access to your account (or disable two-factor authentication). Each code can only be used once.

- +

Make sure to write them down and store them in a safe place.

+ +
    {% for code in backup_codes %} - - {{ code }}
    +
  1. {{ code }}
    2. {% endfor %} -     \ No newline at end of file +
diff --git a/tildes/tildes/templates/settings.jinja2 b/tildes/tildes/templates/settings.jinja2 index 5b86e88..fbdc4a6 100644 --- a/tildes/tildes/templates/settings.jinja2 +++ b/tildes/tildes/templates/settings.jinja2 @@ -96,8 +96,13 @@
To be able to regain access in case of lost password, compromise, etc.
  • - Set up two-factor authentication -
    For extra security, you can enable two-factor authentication.
    + {% if not request.user.two_factor_enabled %} + Enable two-factor authentication +
    For extra security, you can enable two-factor authentication.
    + {% else %} + Disable two-factor authentication +
    Disabling two-factor authentication requires a code from your device or a backup code.
    + {% endif %}
  • Toggle marking new comments (currently {{ 'enabled' if request.user.track_comment_visits else 'disabled' }}) diff --git a/tildes/tildes/templates/settings_two_factor.jinja2 b/tildes/tildes/templates/settings_two_factor.jinja2 index 1d8dc2b..db7b99e 100644 --- a/tildes/tildes/templates/settings_two_factor.jinja2 +++ b/tildes/tildes/templates/settings_two_factor.jinja2 @@ -42,7 +42,7 @@ >
    - +
    diff --git a/tildes/tildes/views/api/web/user.py b/tildes/tildes/views/api/web/user.py index 7e0fcd1..1c3f244 100644 --- a/tildes/tildes/views/api/web/user.py +++ b/tildes/tildes/views/api/web/user.py @@ -6,7 +6,6 @@ from typing import Optional from marshmallow import ValidationError from marshmallow.fields import String -import pyotp from pyramid.httpexceptions import ( HTTPForbidden, HTTPUnauthorized, @@ -18,6 +17,7 @@ from sqlalchemy.exc import IntegrityError from webargs.pyramidparser import use_kwargs from tildes.enums import LogEventType, TopicSortOption +from tildes.lib.string import separate_string from tildes.models.log import Log from tildes.models.user import User, UserInviteCode from tildes.schemas.fields import Enum, ShortTimePeriod @@ -91,17 +91,6 @@ def patch_change_email_address( return Response("Your email address has been updated") -def generate_backup_code() -> str: - """Generate a user-friendly (easy to read) backup code for 2FA.""" - parts = [] - # Generate 4 four-length random strings. - for _ in range(4): - parts.append("".join(random.choices(string.ascii_lowercase, k=4))) - - # Combine parts to make one, e.g. xxxx xxxx xxxx xxxx - return " ".join(parts) - - @ic_view_config( route_name="user", request_method="POST", @@ -109,20 +98,25 @@ def generate_backup_code() -> str: renderer="two_factor_enabled.jinja2", permission="change_two_factor", ) -def post_enable_two_factor(request: Request) -> dict: +@use_kwargs({"code": String()}) +def post_enable_two_factor(request: Request, code: str) -> dict: """Enable two-factor authentication for the user.""" user = request.context - totp = pyotp.TOTP(user.two_factor_secret) - code = str(request.params.get("code")) - if not totp.verify(code): + if not user.is_correct_two_factor_code(code): raise HTTPUnprocessableEntity("Invalid code, please try again.") - # Generate 10 backup codes. - backup_codes = [generate_backup_code() for _ in range(10)] - request.user.two_factor_enabled = True - request.user.two_factor_backup_codes = backup_codes + + # Generate 10 backup codes (16 lowercase letters each) + request.user.two_factor_backup_codes = [ + "".join(random.choices(string.ascii_lowercase, k=16)) for _ in range(10) + ] + + # format the backup codes to be easier to read for output + backup_codes = [ + separate_string(code, " ", 4) for code in request.user.two_factor_backup_codes + ] return {"backup_codes": backup_codes} @@ -134,10 +128,9 @@ def post_enable_two_factor(request: Request) -> dict: renderer="two_factor_disabled.jinja2", permission="change_two_factor", ) -def post_disable_two_factor(request: Request) -> Response: +@use_kwargs({"code": String()}) +def post_disable_two_factor(request: Request, code: str) -> Response: """Disable two-factor authentication for the user.""" - code = str(request.params.get("code")) - if not request.user.is_correct_two_factor_code(code): raise HTTPUnauthorized(body="Invalid code") diff --git a/tildes/tildes/views/login.py b/tildes/tildes/views/login.py index 885d910..a05082b 100644 --- a/tildes/tildes/views/login.py +++ b/tildes/tildes/views/login.py @@ -1,5 +1,6 @@ """Views related to logging in/out.""" +from marshmallow.fields import String from pyramid.httpexceptions import HTTPFound, HTTPUnauthorized, HTTPUnprocessableEntity from pyramid.renderers import render_to_response from pyramid.request import Request @@ -96,7 +97,8 @@ def post_login(request: Request, username: str, password: str) -> HTTPFound: ) @not_logged_in @rate_limit_view("login_two_factor") -def post_login_two_factor(request: Request) -> Response: +@use_kwargs({"code": String()}) +def post_login_two_factor(request: Request, code: str) -> Response: """Process a log in request with 2FA.""" # Look up the user for the supplied username user = ( @@ -105,7 +107,6 @@ def post_login_two_factor(request: Request) -> Response: .filter(User.username == request.session["two_factor_username"]) .one_or_none() ) - code = str(request.params.get("code")) if user.is_correct_two_factor_code(code): del request.session["two_factor_username"] diff --git a/tildes/tildes/views/settings.py b/tildes/tildes/views/settings.py index 00a8894..49fb264 100644 --- a/tildes/tildes/views/settings.py +++ b/tildes/tildes/views/settings.py @@ -1,6 +1,7 @@ """Views related to user settings.""" from io import BytesIO + import pyotp from pyramid.httpexceptions import HTTPForbidden, HTTPUnprocessableEntity from pyramid.request import Request