From 134848b9bdb5f8bf090e741bc2f5508a8a4df32d Mon Sep 17 00:00:00 2001 From: Deimos Date: Mon, 13 Aug 2018 17:16:07 -0600 Subject: [PATCH 01/14] Add margin-bottom to Adding a table to a post wouldn't have any margin below it, so the next paragraph would be right up against the bottom. --- tildes/scss/_base.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/tildes/scss/_base.scss b/tildes/scss/_base.scss index a084f4d..61eda0e 100644 --- a/tildes/scss/_base.scss +++ b/tildes/scss/_base.scss @@ -196,6 +196,7 @@ table { border-collapse: collapse; border-spacing: 0; width: auto; + margin-bottom: 1rem; } td, th { From 2afcb216ea0318a28a6c1951053eed74e96e3aec Mon Sep 17 00:00:00 2001 From: Deimos Date: Mon, 13 Aug 2018 17:23:49 -0600 Subject: [PATCH 02/14] Skip tags when linkifying groups/users --- tildes/tests/test_markdown.py | 8 ++++++++ tildes/tildes/lib/markdown.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tildes/tests/test_markdown.py b/tildes/tests/test_markdown.py index 5ac9979..90bdceb 100644 --- a/tildes/tests/test_markdown.py +++ b/tildes/tests/test_markdown.py @@ -338,3 +338,11 @@ def test_username_ref_inside_pre_ignored(): processed = convert_markdown_to_safe_html(markdown) assert " tag doesn't get linked.""" + markdown = "Strikethrough works like: `this ~should not~ work`." + processed = convert_markdown_to_safe_html(markdown) + + assert " str: def postprocess_markdown_html(html: str) -> str: """Apply post-processing to HTML generated by markdown parser.""" # list of tag names to exclude from linkification - linkify_skipped_tags = ["pre"] + linkify_skipped_tags = ["code", "pre"] # search for text that looks like urls and convert to actual links html = bleach.linkify( From a588431feec331033082130344853252e6613836 Mon Sep 17 00:00:00 2001 From: Oden Date: Sun, 29 Jul 2018 22:13:50 -0700 Subject: [PATCH 03/14] Add two-factor authentication Adds optional two-factor authentication support using TOTP, and including backup codes in case of a lost 2FA device. --- ...332481a6e_add_two_factor_authentication.py | 38 +++++++++++ tildes/requirements-to-freeze.txt | 3 + tildes/requirements.txt | 3 + tildes/tildes/lib/ratelimit.py | 1 + tildes/tildes/models/user/user.py | 24 ++++++- tildes/tildes/routes.py | 9 +++ .../intercooler/login_two_factor.jinja2 | 17 +++++ .../intercooler/two_factor_disabled.jinja2 | 3 + .../intercooler/two_factor_enabled.jinja2 | 10 +++ tildes/tildes/templates/login.jinja2 | 2 +- tildes/tildes/templates/settings.jinja2 | 4 ++ .../templates/settings_two_factor.jinja2 | 53 +++++++++++++++ tildes/tildes/views/api/web/user.py | 66 +++++++++++++++++- tildes/tildes/views/login.py | 67 +++++++++++++++---- tildes/tildes/views/settings.py | 33 ++++++++- 15 files changed, 317 insertions(+), 16 deletions(-) create mode 100644 tildes/alembic/versions/67e332481a6e_add_two_factor_authentication.py create mode 100644 tildes/tildes/templates/intercooler/login_two_factor.jinja2 create mode 100644 tildes/tildes/templates/intercooler/two_factor_disabled.jinja2 create mode 100644 tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 create mode 100644 tildes/tildes/templates/settings_two_factor.jinja2 diff --git a/tildes/alembic/versions/67e332481a6e_add_two_factor_authentication.py b/tildes/alembic/versions/67e332481a6e_add_two_factor_authentication.py new file mode 100644 index 0000000..c316755 --- /dev/null +++ b/tildes/alembic/versions/67e332481a6e_add_two_factor_authentication.py @@ -0,0 +1,38 @@ +"""Add two-factor authentication + +Revision ID: 67e332481a6e +Revises: fab922a8bb04 +Create Date: 2018-07-31 02:53:50.182862 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "67e332481a6e" +down_revision = "fab922a8bb04" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "users", + sa.Column( + "two_factor_backup_codes", postgresql.ARRAY(sa.Text()), nullable=True + ), + ) + op.add_column( + "users", + sa.Column( + "two_factor_enabled", sa.Boolean(), server_default="false", nullable=False + ), + ) + op.add_column("users", sa.Column("two_factor_secret", sa.Text(), nullable=True)) + + +def downgrade(): + op.drop_column("users", "two_factor_secret") + op.drop_column("users", "two_factor_enabled") + op.drop_column("users", "two_factor_backup_codes") diff --git a/tildes/requirements-to-freeze.txt b/tildes/requirements-to-freeze.txt index 69c181d..39cc8a1 100644 --- a/tildes/requirements-to-freeze.txt +++ b/tildes/requirements-to-freeze.txt @@ -13,6 +13,7 @@ html5lib ipython mypy mypy-extensions +Pillow prometheus-client psycopg2 publicsuffix2 @@ -20,6 +21,7 @@ pydocstyle pylama pylama-pylint pylint==1.7.5 # pylama has issues with 1.8.1 +pyotp pyramid pyramid-debugtoolbar pyramid-ipython @@ -30,6 +32,7 @@ pyramid-webassets pytest pytest-mock PyYAML # needs to be installed separately for webassets +qrcode SQLAlchemy SQLAlchemy-Utils stripe diff --git a/tildes/requirements.txt b/tildes/requirements.txt index 2047941..aacc564 100644 --- a/tildes/requirements.txt +++ b/tildes/requirements.txt @@ -38,6 +38,7 @@ parso==0.3.1 PasteDeploy==1.5.2 pexpect==4.6.0 pickleshare==0.7.4 +Pillow==5.2.0 plaster==1.0 plaster-pastedeploy==0.6 pluggy==0.7.1 @@ -55,6 +56,7 @@ Pygments==2.2.0 pylama==7.4.3 pylama-pylint==3.0.1 pylint==1.7.5 +pyotp==2.2.6 pyramid==1.9.2 pyramid-debugtoolbar==4.4 pyramid-ipython==0.2 @@ -68,6 +70,7 @@ pytest-mock==1.10.0 python-dateutil==2.7.3 python-editor==1.0.3 PyYAML==3.13 +qrcode==6.0 redis==2.10.6 repoze.lru==0.7 requests==2.19.1 diff --git a/tildes/tildes/lib/ratelimit.py b/tildes/tildes/lib/ratelimit.py index 74a6e4b..96661b7 100644 --- a/tildes/tildes/lib/ratelimit.py +++ b/tildes/tildes/lib/ratelimit.py @@ -279,6 +279,7 @@ class RateLimitedAction: # each action must have a unique name to prevent key collisions _RATE_LIMITED_ACTIONS = ( RateLimitedAction("login", timedelta(hours=1), 20), + RateLimitedAction("login_two_factor", timedelta(hours=1), 20), RateLimitedAction("register", timedelta(hours=1), 50), ) diff --git a/tildes/tildes/models/user/user.py b/tildes/tildes/models/user/user.py index a69fa2f..36099e5 100644 --- a/tildes/tildes/models/user/user.py +++ b/tildes/tildes/models/user/user.py @@ -4,6 +4,7 @@ from datetime import datetime from typing import Any, List, Optional, Sequence, Tuple from mypy_extensions import NoReturn +import pyotp from pyramid.security import ( ALL_PERMISSIONS, Allow, @@ -21,7 +22,7 @@ from sqlalchemy import ( Text, TIMESTAMP, ) -from sqlalchemy.dialects.postgresql import ENUM +from sqlalchemy.dialects.postgresql import ARRAY, ENUM from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.orm import deferred from sqlalchemy.sql.expression import text @@ -62,6 +63,9 @@ class User(DatabaseModel): ), ) ) + two_factor_enabled: bool = Column(Boolean, nullable=False, server_default="false") + two_factor_secret: Optional[str] = deferred(Column(Text)) + two_factor_backup_codes: List[str] = deferred(Column(ARRAY(Text))) created_time: datetime = Column( TIMESTAMP(timezone=True), nullable=False, @@ -161,6 +165,24 @@ class User(DatabaseModel): # disable mypy on this line because it doesn't handle setters correctly self.password = new_password # type: ignore + 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() + + if totp.verify(code.replace(" ", "")): + return True + elif code in self.two_factor_backup_codes: + # Need to set the attribute so SQLAlchemy knows it changed + self.two_factor_backup_codes = [ + backup_code + for backup_code in self.two_factor_backup_codes + if backup_code != code + ] + return True + + return False + @property def email_address(self) -> NoReturn: """Return an error since reading the email address isn't possible.""" diff --git a/tildes/tildes/routes.py b/tildes/tildes/routes.py index dc977ec..82c1a47 100644 --- a/tildes/tildes/routes.py +++ b/tildes/tildes/routes.py @@ -20,6 +20,7 @@ def includeme(config: Configurator) -> None: config.add_route("groups", "/groups") config.add_route("login", "/login") + config.add_route("login_two_factor", "/login_two_factor") config.add_route("logout", "/logout", factory=LoggedInFactory) config.add_route("register", "/register") @@ -61,6 +62,14 @@ def includeme(config: Configurator) -> None: "/settings/account_recovery", factory=LoggedInFactory, ) + config.add_route( + "settings_two_factor", "/settings/two_factor", factory=LoggedInFactory + ) + config.add_route( + "settings_two_factor_qr_code", + "/settings/two_factor/qr_code", + factory=LoggedInFactory, + ) config.add_route( "settings_comment_visits", "/settings/comment_visits", factory=LoggedInFactory ) diff --git a/tildes/tildes/templates/intercooler/login_two_factor.jinja2 b/tildes/tildes/templates/intercooler/login_two_factor.jinja2 new file mode 100644 index 0000000..21afdb4 --- /dev/null +++ b/tildes/tildes/templates/intercooler/login_two_factor.jinja2 @@ -0,0 +1,17 @@ +

Two-factor authentication is enabled on this account. Please enter the code from your authenticator app below. If you do not have access to your authenticator device, enter a backup code.

+ +
+ + {% if keep %} + + {% endif %} + +
+ + +
+ +
+ +
+ diff --git a/tildes/tildes/templates/intercooler/two_factor_disabled.jinja2 b/tildes/tildes/templates/intercooler/two_factor_disabled.jinja2 new file mode 100644 index 0000000..a47c8b3 --- /dev/null +++ b/tildes/tildes/templates/intercooler/two_factor_disabled.jinja2 @@ -0,0 +1,3 @@ +

Two-factor authentication has been disabled. You will no longer need a code when logging in.

+ +

Keep in mind: if you ever reenable two-factor authentication, your previous backup codes will not be valid.

\ No newline at end of file diff --git a/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 b/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 new file mode 100644 index 0000000..2574d8a --- /dev/null +++ b/tildes/tildes/templates/intercooler/two_factor_enabled.jinja2 @@ -0,0 +1,10 @@ +

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.

+ + +{% for code in backup_codes %} + - {{ code }}
+{% endfor %} + \ No newline at end of file diff --git a/tildes/tildes/templates/login.jinja2 b/tildes/tildes/templates/login.jinja2 index 81027fe..2eaa99c 100644 --- a/tildes/tildes/templates/login.jinja2 +++ b/tildes/tildes/templates/login.jinja2 @@ -5,7 +5,7 @@ {% block main_heading %}Log in{% endblock %} {% block content %} -
+
diff --git a/tildes/tildes/templates/settings.jinja2 b/tildes/tildes/templates/settings.jinja2 index bd16d7e..5b86e88 100644 --- a/tildes/tildes/templates/settings.jinja2 +++ b/tildes/tildes/templates/settings.jinja2 @@ -95,6 +95,10 @@ Set up account recovery
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.
    +
  • Toggle marking new comments (currently {{ 'enabled' if request.user.track_comment_visits else 'disabled' }})
    Marks new comments in topics since your last visit, and which topics have any
    diff --git a/tildes/tildes/templates/settings_two_factor.jinja2 b/tildes/tildes/templates/settings_two_factor.jinja2 new file mode 100644 index 0000000..1d8dc2b --- /dev/null +++ b/tildes/tildes/templates/settings_two_factor.jinja2 @@ -0,0 +1,53 @@ +{% extends 'base_no_sidebar.jinja2' %} + +{% block title %}Set up two-factor authentication{% endblock %} + +{% block main_heading %}Set up two-factor authentication{% endblock %} + +{% block content %} +{% if request.user.two_factor_enabled %} +

    You already have two-factor authentication enabled. To disable it, enter a code from your authenticator device below and click the button. If you do not have access to your authenticator device, enter a backup code.

    + + +
    + + +
    + +
    + +
    + +{% else %} +

    To get started, you'll need to install an app such as Google Authenticator, Authy, FreeOTP, or any app that supports TOTP.

    + +

    Next, scan the below QR code with the app of your choice.

    + + + +

    Lastly, enter the 6-digit code displayed in the app.

    + +
    + +
    +
    + + +
    + +
    + +
    + +{% endif %} +{% endblock %} diff --git a/tildes/tildes/views/api/web/user.py b/tildes/tildes/views/api/web/user.py index dc4df27..7e0fcd1 100644 --- a/tildes/tildes/views/api/web/user.py +++ b/tildes/tildes/views/api/web/user.py @@ -1,10 +1,17 @@ """Web API endpoints related to users.""" +import random +import string from typing import Optional from marshmallow import ValidationError from marshmallow.fields import String -from pyramid.httpexceptions import HTTPForbidden, HTTPUnprocessableEntity +import pyotp +from pyramid.httpexceptions import ( + HTTPForbidden, + HTTPUnauthorized, + HTTPUnprocessableEntity, +) from pyramid.request import Request from pyramid.response import Response from sqlalchemy.exc import IntegrityError @@ -84,6 +91,63 @@ 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", + request_param="ic-trigger-name=enable-two-factor", + renderer="two_factor_enabled.jinja2", + permission="change_two_factor", +) +def post_enable_two_factor(request: Request) -> 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): + 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 + + return {"backup_codes": backup_codes} + + +@ic_view_config( + route_name="user", + request_method="POST", + request_param="ic-trigger-name=disable-two-factor", + renderer="two_factor_disabled.jinja2", + permission="change_two_factor", +) +def post_disable_two_factor(request: Request) -> 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") + + request.user.two_factor_enabled = False + request.user.two_factor_secret = None + request.user.two_factor_backup_codes = None + + return {} + + @ic_view_config( route_name="user", request_method="PATCH", diff --git a/tildes/tildes/views/login.py b/tildes/tildes/views/login.py index bab5b25..885d910 100644 --- a/tildes/tildes/views/login.py +++ b/tildes/tildes/views/login.py @@ -1,7 +1,9 @@ """Views related to logging in/out.""" -from pyramid.httpexceptions import HTTPFound, HTTPUnprocessableEntity +from pyramid.httpexceptions import HTTPFound, HTTPUnauthorized, HTTPUnprocessableEntity +from pyramid.renderers import render_to_response from pyramid.request import Request +from pyramid.response import Response from pyramid.security import NO_PERMISSION_REQUIRED, remember from pyramid.view import view_config from webargs.pyramidparser import use_kwargs @@ -24,6 +26,22 @@ def get_login(request: Request) -> dict: return {} +def finish_login(request: Request, user: User) -> None: + """Save the user ID into session.""" + # Username/password were correct - attach the user_id to the session + remember(request, user.user_id) + + # Depending on "keep me logged in", set session timeout to 1 year or 1 day + if request.params.get("keep"): + request.session.adjust_timeout_for_session(31_536_000) + else: + request.session.adjust_timeout_for_session(86_400) + + # set request.user before logging so the user is associated with the event + request.user = user + request.db_session.add(Log(LogEventType.USER_LOG_IN, request)) + + @view_config( route_name="login", request_method="POST", permission=NO_PERMISSION_REQUIRED ) @@ -57,22 +75,47 @@ def post_login(request: Request, username: str, password: str) -> HTTPFound: if user.is_banned: raise HTTPUnprocessableEntity("This account has been banned") - # Username/password were correct - attach the user_id to the session - remember(request, user.user_id) + # If 2FA is enabled, save username to session and make user enter code + if user.two_factor_enabled: + request.session["two_factor_username"] = username + return render_to_response( + "tildes:templates/intercooler/login_two_factor.jinja2", + {"keep": request.params.get("keep")}, + request=request, + ) - # Depending on "keep me logged in", set session timeout to 1 year or 1 day - if request.params.get("keep"): - request.session.adjust_timeout_for_session(31_536_000) - else: - request.session.adjust_timeout_for_session(86_400) - - # set request.user before logging so the user is associated with the event - request.user = user - request.db_session.add(Log(LogEventType.USER_LOG_IN, request)) + finish_login(request, user) raise HTTPFound(location="/") +@view_config( + route_name="login_two_factor", + request_method="POST", + permission=NO_PERMISSION_REQUIRED, +) +@not_logged_in +@rate_limit_view("login_two_factor") +def post_login_two_factor(request: Request) -> Response: + """Process a log in request with 2FA.""" + # Look up the user for the supplied username + user = ( + request.query(User) + .undefer_all_columns() + .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"] + finish_login(request, user) + + raise HTTPFound(location="/") + else: + raise HTTPUnauthorized(body="Invalid code, please try again.") + + @view_config(route_name="logout") def get_logout(request: Request) -> HTTPFound: """Process a log out request.""" diff --git a/tildes/tildes/views/settings.py b/tildes/tildes/views/settings.py index 3cbf865..00a8894 100644 --- a/tildes/tildes/views/settings.py +++ b/tildes/tildes/views/settings.py @@ -1,9 +1,12 @@ """Views related to user settings.""" -from pyramid.httpexceptions import HTTPUnprocessableEntity +from io import BytesIO +import pyotp +from pyramid.httpexceptions import HTTPForbidden, HTTPUnprocessableEntity from pyramid.request import Request from pyramid.response import Response from pyramid.view import view_config +import qrcode from webargs.pyramidparser import use_kwargs from tildes.schemas.user import EMAIL_ADDRESS_NOTE_MAX_LENGTH, UserSchema @@ -35,6 +38,13 @@ def get_settings_account_recovery(request: Request) -> dict: return {"note_max_length": EMAIL_ADDRESS_NOTE_MAX_LENGTH} +@view_config(route_name="settings_two_factor", renderer="settings_two_factor.jinja2") +def get_settings_two_factor(request: Request) -> dict: + """Generate the two-factor authentication page.""" + # pylint: disable=unused-argument + return {} + + @view_config( route_name="settings_comment_visits", renderer="settings_comment_visits.jinja2" ) @@ -60,6 +70,27 @@ def get_settings_password_change(request: Request) -> dict: return {} +@view_config(route_name="settings_two_factor_qr_code") +def get_settings_two_factor_qr_code(request: Request) -> Response: + """Generate the 2FA QR code.""" + # If 2FA is already enabled, don't expose the secret. + if request.user.two_factor_enabled: + raise HTTPForbidden("Already enabled") + + # Generate a new secret key if the user doesn't have one. + if request.user.two_factor_secret is None: + request.user.two_factor_secret = pyotp.random_base32() + + totp = pyotp.totp.TOTP(request.user.two_factor_secret) + otp_uri = totp.provisioning_uri(request.user.username, issuer_name="Tildes") + byte_io = BytesIO() + img = qrcode.make(otp_uri, border=2, box_size=4) + + img.save(byte_io, "PNG") + + return Response(byte_io.getvalue(), cache_control="private, no-cache") + + @view_config(route_name="settings_password_change", request_method="POST") @use_kwargs( { From 9775acd9bfe811adad162c9c24920a075aa62309 Mon Sep 17 00:00:00 2001 From: Deimos Date: Thu, 16 Aug 2018 13:22:41 -0600 Subject: [PATCH 04/14] 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 From 6a8290aa36c3138f941183bdda1798ec2fc89b52 Mon Sep 17 00:00:00 2001 From: Deimos Date: Thu, 16 Aug 2018 19:15:44 -0600 Subject: [PATCH 05/14] Switch to a general "permissions" column on users Previously there was a specific is_admin boolean column. This commit changes to have a general permissions column which is stored in JSON, and currently should either be a single string or list of strings. These strings are used as the user's principals for the authorization system. So now, setting a user as admin would involve adding the string "admin" to their permissions column, instead of just setting is_admin to True. As part of this change, I also moved the MutableDict associations onto specific columns, instead of being attached to JSONB by default (since this new column won't always be a dict). --- ...53_switch_to_general_permissions_column.py | 40 +++++++++++++++++++ tildes/tildes/auth.py | 7 +--- tildes/tildes/models/database_model.py | 5 --- tildes/tildes/models/log/log.py | 3 +- tildes/tildes/models/topic/topic.py | 3 +- tildes/tildes/models/user/user.py | 18 ++++++++- 6 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 tildes/alembic/versions/d33fb803a153_switch_to_general_permissions_column.py diff --git a/tildes/alembic/versions/d33fb803a153_switch_to_general_permissions_column.py b/tildes/alembic/versions/d33fb803a153_switch_to_general_permissions_column.py new file mode 100644 index 0000000..8f1fdf9 --- /dev/null +++ b/tildes/alembic/versions/d33fb803a153_switch_to_general_permissions_column.py @@ -0,0 +1,40 @@ +"""Switch to general permissions column + +Revision ID: d33fb803a153 +Revises: 67e332481a6e +Create Date: 2018-08-16 23:07:07.643208 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "d33fb803a153" +down_revision = "67e332481a6e" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "users", + sa.Column( + "permissions", postgresql.JSONB(astext_type=sa.Text()), nullable=True + ), + ) + op.drop_column("users", "is_admin") + + +def downgrade(): + op.add_column( + "users", + sa.Column( + "is_admin", + sa.BOOLEAN(), + server_default=sa.text("false"), + autoincrement=False, + nullable=False, + ), + ) + op.drop_column("users", "permissions") diff --git a/tildes/tildes/auth.py b/tildes/tildes/auth.py index 519ce67..8d3def3 100644 --- a/tildes/tildes/auth.py +++ b/tildes/tildes/auth.py @@ -56,12 +56,7 @@ def auth_callback(user_id: int, request: Request) -> Optional[Sequence[str]]: if user_id != request.user.user_id: raise AssertionError("auth_callback called with different user_id") - principals = [] - - if request.user.is_admin: - principals.append("admin") - - return principals + return request.user.auth_principals def includeme(config: Configurator) -> None: diff --git a/tildes/tildes/models/database_model.py b/tildes/tildes/models/database_model.py index 24a5297..0e166e3 100644 --- a/tildes/tildes/models/database_model.py +++ b/tildes/tildes/models/database_model.py @@ -4,9 +4,7 @@ from typing import Any, Optional, Type, TypeVar from marshmallow import Schema from sqlalchemy import event -from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.schema import MetaData from sqlalchemy.sql.schema import Table @@ -124,6 +122,3 @@ DatabaseModel = declarative_base( # pylint: disable=invalid-name # attach the listener for SQLAlchemy ORM attribute "set" events to all models event.listen(DatabaseModel, "attribute_instrument", attach_set_listener) - -# associate JSONB columns with MutableDict so value changes are detected -MutableDict.associate_with(JSONB) diff --git a/tildes/tildes/models/log/log.py b/tildes/tildes/models/log/log.py index ea1a4c5..c44c98d 100644 --- a/tildes/tildes/models/log/log.py +++ b/tildes/tildes/models/log/log.py @@ -7,6 +7,7 @@ from sqlalchemy import BigInteger, Column, event, ForeignKey, Integer, Table, TI from sqlalchemy.dialects.postgresql import ENUM, INET, JSONB from sqlalchemy.engine import Connection from sqlalchemy.ext.declarative import declared_attr +from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import relationship from sqlalchemy.sql.expression import text @@ -51,7 +52,7 @@ class BaseLog: @declared_attr def info(self) -> Column: """Return the info column.""" - return Column(JSONB) + return Column(MutableDict.as_mutable(JSONB)) @declared_attr def user(self) -> Any: diff --git a/tildes/tildes/models/topic/topic.py b/tildes/tildes/models/topic/topic.py index 4b0ad94..c79d168 100644 --- a/tildes/tildes/models/topic/topic.py +++ b/tildes/tildes/models/topic/topic.py @@ -16,6 +16,7 @@ from sqlalchemy import ( ) from sqlalchemy.dialects.postgresql import ENUM, JSONB from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import deferred, relationship from sqlalchemy.sql.expression import text from sqlalchemy_utils import Ltree @@ -102,7 +103,7 @@ class Topic(DatabaseModel): _markdown: Optional[str] = deferred(Column("markdown", Text)) rendered_html: Optional[str] = Column(Text) link: Optional[str] = Column(Text) - content_metadata: Dict[str, Any] = Column(JSONB) + content_metadata: Dict[str, Any] = Column(MutableDict.as_mutable(JSONB)) num_comments: int = Column(Integer, nullable=False, server_default="0", index=True) num_votes: int = Column(Integer, nullable=False, server_default="0", index=True) _tags: List[Ltree] = Column( diff --git a/tildes/tildes/models/user/user.py b/tildes/tildes/models/user/user.py index c808c0f..fe6f85c 100644 --- a/tildes/tildes/models/user/user.py +++ b/tildes/tildes/models/user/user.py @@ -22,7 +22,7 @@ from sqlalchemy import ( Text, TIMESTAMP, ) -from sqlalchemy.dialects.postgresql import ARRAY, ENUM +from sqlalchemy.dialects.postgresql import ARRAY, ENUM, JSONB from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.orm import deferred from sqlalchemy.sql.expression import text @@ -88,7 +88,7 @@ class User(DatabaseModel): ) open_new_tab_text: bool = Column(Boolean, nullable=False, server_default="false") is_banned: bool = Column(Boolean, nullable=False, server_default="false") - is_admin: bool = Column(Boolean, nullable=False, server_default="false") + permissions: Any = Column(JSONB) home_default_order: Optional[TopicSortOption] = Column(ENUM(TopicSortOption)) home_default_period: Optional[str] = Column(Text) _filtered_topic_tags: List[Ltree] = Column( @@ -204,3 +204,17 @@ class User(DatabaseModel): def num_unread_total(self) -> int: """Return total number of unread items (notifications + messages).""" return self.num_unread_messages + self.num_unread_notifications + + @property + def auth_principals(self) -> List[str]: + """Return the user's authorization principals (used for permissions).""" + if not self.permissions: + return [] + + if isinstance(self.permissions, str): + return [self.permissions] + + if isinstance(self.permissions, list): + return self.permissions + + raise ValueError("Unknown permissions format") From 1d8b74ca3ceec707954451523ad9fb41825dd109 Mon Sep 17 00:00:00 2001 From: Deimos Date: Thu, 16 Aug 2018 21:44:15 -0600 Subject: [PATCH 06/14] Add auth principals for some topic tools Allows (manually) granting permissions to allow users to re-tag topics, move them between groups, and edit their titles. This should probably be generalized in the near future, but this will do the trick for now. --- tildes/tildes/models/topic/topic.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tildes/tildes/models/topic/topic.py b/tildes/tildes/models/topic/topic.py index c79d168..932f840 100644 --- a/tildes/tildes/models/topic/topic.py +++ b/tildes/tildes/models/topic/topic.py @@ -264,14 +264,19 @@ class Topic(DatabaseModel): acl.append((Allow, self.user_id, "delete")) # tag: - # - only the author and admins can tag topics + # - allow tagging by the author, admins, and people with "topic.tag" principal acl.append((Allow, self.user_id, "tag")) acl.append((Allow, "admin", "tag")) + acl.append((Allow, "topic.tag", "tag")) - # admin tools + # tools that require specifically granted permissions acl.append((Allow, "admin", "lock")) + acl.append((Allow, "admin", "move")) + acl.append((Allow, "topic.move", "move")) + acl.append((Allow, "admin", "edit_title")) + acl.append((Allow, "topic.edit_title", "edit_title")) acl.append(DENY_ALL) From e4b8bb97247b6f0647903b7d6bda9607e625bf12 Mon Sep 17 00:00:00 2001 From: Deimos Date: Fri, 17 Aug 2018 12:41:31 -0600 Subject: [PATCH 07/14] Restrict accidental-ordered-list fix to post start Previously this was also trying to catch ones at the beginning of new paragraphs, but that seems to mostly just be causing unexpected issues when people create ordered lists with a blank line between items. This can probably be done properly in the future, but just restricting it to the start of posts is probably better for now. --- tildes/tests/test_markdown.py | 6 +----- tildes/tildes/lib/markdown.py | 13 ++++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/tildes/tests/test_markdown.py b/tildes/tests/test_markdown.py index 90bdceb..e0be956 100644 --- a/tildes/tests/test_markdown.py +++ b/tildes/tests/test_markdown.py @@ -70,11 +70,7 @@ def test_deliberate_ordered_list(): def test_accidental_ordered_list(): """Ensure a common "accidental" ordered list gets escaped.""" - markdown = ( - "What year did this happen?\n\n" - "1975. It was a long time ago.\n\n" - "But I remember it like it was yesterday." - ) + markdown = "1975. It was a long time ago." html = convert_markdown_to_safe_html(markdown) assert " str: """Escape markdown that's probably an accidental ordered list. It's a common markdown mistake to accidentally start a numbered list, by beginning a - post or paragraph with a number followed by a period. For example, someone might try - to write "1975. It was a long time ago.", and the result will be a comment that says - "1. It was a long time ago." since that gets parsed into a numbered list. + post with a number followed by a period. For example, someone might try to write + "1975. It was a long time ago.", and the result will be a comment that says "1. It + was a long time ago." since that gets parsed into a numbered list. This fixes that quirk of markdown by escaping anything that would start a numbered - list except for "1. ". This will cause a few other edge cases, but I believe they're - less common/important than fixing this common error. + list at the beginning of a post, except for "1. ". """ return BAD_ORDERED_LIST_REGEX.sub(r"\1\\. ", markdown) From 98cfa08a608da3e548183f796b00f6fd9479ef39 Mon Sep 17 00:00:00 2001 From: Jedi Burrell Date: Thu, 9 Aug 2018 06:40:35 -0400 Subject: [PATCH 08/14] Fix OK button location for sorting with JS off --- tildes/tildes/templates/topic.jinja2 | 9 ++++----- tildes/tildes/templates/topic_listing.jinja2 | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/tildes/tildes/templates/topic.jinja2 b/tildes/tildes/templates/topic.jinja2 index 78e60a8..a3512cb 100644 --- a/tildes/tildes/templates/topic.jinja2 +++ b/tildes/tildes/templates/topic.jinja2 @@ -166,12 +166,11 @@ >{{ option.description }} {% endfor %} + {# add a submit button for people with js disabled so this is still usable #} +
    - - {# add a submit button for people with js disabled so this is still usable #} - diff --git a/tildes/tildes/templates/topic_listing.jinja2 b/tildes/tildes/templates/topic_listing.jinja2 index ab4bf51..d8f9f56 100644 --- a/tildes/tildes/templates/topic_listing.jinja2 +++ b/tildes/tildes/templates/topic_listing.jinja2 @@ -64,12 +64,11 @@ + {# add a submit button for people with js disabled so this is still usable #} +
    • - - {# add a submit button for people with js disabled so this is still usable #} - {% if not is_default_view %} From d5c2d18ae7eb53a450b4ca99d9ee1f9c3ed41ece Mon Sep 17 00:00:00 2001 From: Deimos Date: Sat, 18 Aug 2018 15:10:33 -0600 Subject: [PATCH 09/14] Add rate limits for posting topics and comments These limits were determined by looking at site activity so far, and generally shouldn't have any impact on normal site usage. This also adds a new request method - apply_rate_limit, which can be used to check the rate limit and immediately raise an error if it's exceeded, instead of needing to check and handle the result separately. --- tildes/tildes/__init__.py | 9 +++++++++ tildes/tildes/lib/ratelimit.py | 2 ++ tildes/tildes/views/api/web/comment.py | 4 +++- tildes/tildes/views/decorators.py | 6 ++---- tildes/tildes/views/topic.py | 4 ++++ 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/tildes/tildes/__init__.py b/tildes/tildes/__init__.py index 7be2000..aaa7300 100644 --- a/tildes/tildes/__init__.py +++ b/tildes/tildes/__init__.py @@ -4,6 +4,7 @@ from typing import Any, Callable, Dict, Optional from paste.deploy.config import PrefixMiddleware from pyramid.config import Configurator +from pyramid.httpexceptions import HTTPTooManyRequests from pyramid.registry import Registry from pyramid.request import Request from redis import StrictRedis @@ -50,6 +51,7 @@ def main(global_config: Dict[str, str], **settings: str) -> PrefixMiddleware: # pylint: enable=unnecessary-lambda config.add_request_method(check_rate_limit, "check_rate_limit") + config.add_request_method(apply_rate_limit, "apply_rate_limit") config.add_request_method(current_listing_base_url, "current_listing_base_url") config.add_request_method(current_listing_normal_url, "current_listing_normal_url") @@ -120,6 +122,13 @@ def check_rate_limit(request: Request, action_name: str) -> RateLimitResult: return RateLimitResult.merged_result(results) +def apply_rate_limit(request: Request, action_name: str) -> None: + """Check the rate limit for an action, and raise HTTP 429 if it's exceeded.""" + result = request.check_rate_limit(action_name) + if not result.is_allowed: + raise result.add_headers_to_response(HTTPTooManyRequests()) + + def current_listing_base_url( request: Request, query: Optional[Dict[str, Any]] = None ) -> str: diff --git a/tildes/tildes/lib/ratelimit.py b/tildes/tildes/lib/ratelimit.py index 96661b7..7046736 100644 --- a/tildes/tildes/lib/ratelimit.py +++ b/tildes/tildes/lib/ratelimit.py @@ -281,6 +281,8 @@ _RATE_LIMITED_ACTIONS = ( RateLimitedAction("login", timedelta(hours=1), 20), RateLimitedAction("login_two_factor", timedelta(hours=1), 20), RateLimitedAction("register", timedelta(hours=1), 50), + RateLimitedAction("topic_post", timedelta(hours=1), 6, max_burst=4), + RateLimitedAction("comment_post", timedelta(hours=1), 30, max_burst=20), ) # (public) dict to be able to look up the actions by name diff --git a/tildes/tildes/views/api/web/comment.py b/tildes/tildes/views/api/web/comment.py index 84ee5f5..e17fc41 100644 --- a/tildes/tildes/views/api/web/comment.py +++ b/tildes/tildes/views/api/web/comment.py @@ -16,7 +16,7 @@ from tildes.models.comment import Comment, CommentNotification, CommentTag, Comm from tildes.models.topic import TopicVisit from tildes.schemas.comment import CommentSchema, CommentTagSchema from tildes.views import IC_NOOP -from tildes.views.decorators import ic_view_config +from tildes.views.decorators import ic_view_config, rate_limit_view def _increment_topic_comments_seen(request: Request, comment: Comment) -> None: @@ -57,6 +57,7 @@ def _increment_topic_comments_seen(request: Request, comment: Comment) -> None: permission="comment", ) @use_kwargs(CommentSchema(only=("markdown",))) +@rate_limit_view("comment_post") def post_toplevel_comment(request: Request, markdown: str) -> dict: """Post a new top-level comment on a topic with Intercooler.""" topic = request.context @@ -90,6 +91,7 @@ def post_toplevel_comment(request: Request, markdown: str) -> dict: permission="reply", ) @use_kwargs(CommentSchema(only=("markdown",))) +@rate_limit_view("comment_post") def post_comment_reply(request: Request, markdown: str) -> dict: """Post a reply to a comment with Intercooler.""" parent_comment = request.context diff --git a/tildes/tildes/views/decorators.py b/tildes/tildes/views/decorators.py index e8b3042..c0b0a6f 100644 --- a/tildes/tildes/views/decorators.py +++ b/tildes/tildes/views/decorators.py @@ -2,7 +2,7 @@ from typing import Any, Callable -from pyramid.httpexceptions import HTTPFound, HTTPTooManyRequests +from pyramid.httpexceptions import HTTPFound from pyramid.request import Request from pyramid.view import view_config @@ -35,10 +35,8 @@ def rate_limit_view(action_name: str) -> Callable: def decorator(func: Callable) -> Callable: def wrapper(*args: Any, **kwargs: Any) -> Any: request = args[0] - result = request.check_rate_limit(action_name) - if not result.is_allowed: - raise result.add_headers_to_response(HTTPTooManyRequests()) + request.apply_rate_limit(action_name) return func(*args, **kwargs) diff --git a/tildes/tildes/views/topic.py b/tildes/tildes/views/topic.py index 46e4539..5156272 100644 --- a/tildes/tildes/views/topic.py +++ b/tildes/tildes/views/topic.py @@ -30,6 +30,7 @@ from tildes.schemas.comment import CommentSchema from tildes.schemas.fields import Enum, ShortTimePeriod from tildes.schemas.topic import TopicSchema from tildes.schemas.topic_listing import TopicListingSchema +from tildes.views.decorators import rate_limit_view DefaultSettings = namedtuple("DefaultSettings", ["order", "period"]) @@ -64,6 +65,8 @@ def post_group_topics( except ValidationError: raise ValidationError({"tags": ["Invalid tags"]}) + request.apply_rate_limit("topic_post") + request.db_session.add(new_topic) request.db_session.add(LogTopic(LogEventType.TOPIC_POST, request, new_topic)) @@ -225,6 +228,7 @@ def get_topic(request: Request, comment_order: CommentSortOption) -> dict: @view_config(route_name="topic", request_method="POST", permission="comment") @use_kwargs(CommentSchema(only=("markdown",))) +@rate_limit_view("comment_post") def post_comment_on_topic(request: Request, markdown: str) -> HTTPFound: """Post a new top-level comment on a topic.""" topic = request.context From 54476a447d79644103bd69275b2ff0ff84ed2766 Mon Sep 17 00:00:00 2001 From: Deimos Date: Mon, 20 Aug 2018 16:54:47 -0600 Subject: [PATCH 10/14] Change url methods to treat routes individually Previously these methods for generating "base" and "normal" urls weren't treating each route individually and just had a single list of query vars that would be kept for all routes. This approach is a lot more flexible and allows separating out only the variables relevant for a particular route. --- tildes/tildes/__init__.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/tildes/tildes/__init__.py b/tildes/tildes/__init__.py index aaa7300..473731c 100644 --- a/tildes/tildes/__init__.py +++ b/tildes/tildes/__init__.py @@ -1,6 +1,6 @@ """Configure and initialize the Pyramid app.""" -from typing import Any, Callable, Dict, Optional +from typing import Any, Callable, Dict, Optional, Tuple from paste.deploy.config import PrefixMiddleware from pyramid.config import Configurator @@ -139,10 +139,17 @@ def current_listing_base_url( The `query` argument allows adding query variables to the generated url. """ - if request.matched_route.name not in ("home", "group", "user"): + base_vars_by_route: Dict[str, Tuple[str, ...]] = { + "group": ("order", "period", "per_page", "tag", "unfiltered"), + "home": ("order", "period", "per_page", "tag", "unfiltered"), + "user": ("per_page", "type"), + } + + try: + base_view_vars = base_vars_by_route[request.matched_route.name] + except KeyError: raise AttributeError("Current route is not supported.") - base_view_vars = ("order", "period", "per_page", "tag", "type", "unfiltered") query_vars = { key: val for key, val in request.GET.copy().items() if key in base_view_vars } @@ -165,10 +172,17 @@ def current_listing_normal_url( The `query` argument allows adding query variables to the generated url. """ - if request.matched_route.name not in ("home", "group", "user"): + normal_vars_by_route: Dict[str, Tuple[str, ...]] = { + "group": ("order", "period", "per_page"), + "home": ("order", "period", "per_page"), + "user": ("per_page",), + } + + try: + normal_view_vars = normal_vars_by_route[request.matched_route.name] + except KeyError: raise AttributeError("Current route is not supported.") - normal_view_vars = ("order", "period", "per_page") query_vars = { key: val for key, val in request.GET.copy().items() if key in normal_view_vars } From 59799c95db76980a323baa1ad4ae588a538638aa Mon Sep 17 00:00:00 2001 From: Deimos Date: Mon, 20 Aug 2018 18:59:06 -0600 Subject: [PATCH 11/14] Add extremely basic search Quite a few aspects of this are very hackish (especially as related to the templates and things that needed to be done to allow topic_listing.jinja2 to be inherited from for this new one), but it's a lot better than nothing. --- ...a19c_add_search_column_index_for_topics.py | 63 +++++++++++++++++++ tildes/scss/modules/_sidebar.scss | 8 +++ tildes/scss/modules/_site-header.scss | 1 + tildes/sql/init/triggers/topics/topics.sql | 15 +++++ tildes/tildes/__init__.py | 2 + tildes/tildes/models/topic/topic.py | 10 ++- tildes/tildes/models/topic/topic_query.py | 5 ++ tildes/tildes/routes.py | 2 + tildes/tildes/templates/home.jinja2 | 7 +++ tildes/tildes/templates/search.jinja2 | 31 +++++++++ tildes/tildes/templates/topic_listing.jinja2 | 22 +++++-- tildes/tildes/views/topic.py | 57 +++++++++++++++++ 12 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 tildes/alembic/versions/50c251c4a19c_add_search_column_index_for_topics.py create mode 100644 tildes/tildes/templates/search.jinja2 diff --git a/tildes/alembic/versions/50c251c4a19c_add_search_column_index_for_topics.py b/tildes/alembic/versions/50c251c4a19c_add_search_column_index_for_topics.py new file mode 100644 index 0000000..8eb4c41 --- /dev/null +++ b/tildes/alembic/versions/50c251c4a19c_add_search_column_index_for_topics.py @@ -0,0 +1,63 @@ +"""Add search column/index for topics + +Revision ID: 50c251c4a19c +Revises: d33fb803a153 +Create Date: 2018-08-20 19:18:04.129255 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "50c251c4a19c" +down_revision = "d33fb803a153" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "topics", sa.Column("search_tsv", postgresql.TSVECTOR(), nullable=True) + ) + op.create_index( + "ix_topics_search_tsv_gin", + "topics", + ["search_tsv"], + unique=False, + postgresql_using="gin", + ) + + op.execute( + """ + UPDATE topics + SET search_tsv = to_tsvector('pg_catalog.english', title) + || to_tsvector('pg_catalog.english', COALESCE(markdown, '')); + """ + ) + + op.execute( + """ + CREATE TRIGGER topic_update_search_tsv_insert + BEFORE INSERT ON topics + FOR EACH ROW + EXECUTE PROCEDURE tsvector_update_trigger(search_tsv, 'pg_catalog.english', title, markdown); + + CREATE TRIGGER topic_update_search_tsv_update + BEFORE UPDATE ON topics + FOR EACH ROW + WHEN ( + (OLD.title IS DISTINCT FROM NEW.title) + OR (OLD.markdown IS DISTINCT FROM NEW.markdown) + ) + EXECUTE PROCEDURE tsvector_update_trigger(search_tsv, 'pg_catalog.english', title, markdown); + """ + ) + + +def downgrade(): + op.drop_index("ix_topics_search_tsv_gin", table_name="topics") + op.drop_column("topics", "search_tsv") + + op.execute("DROP TRIGGER topic_update_search_tsv_insert ON topics") + op.execute("DROP TRIGGER topic_update_search_tsv_update ON topics") diff --git a/tildes/scss/modules/_sidebar.scss b/tildes/scss/modules/_sidebar.scss index d7a56f9..7106908 100644 --- a/tildes/scss/modules/_sidebar.scss +++ b/tildes/scss/modules/_sidebar.scss @@ -11,6 +11,14 @@ .sidebar-controls .btn { width: auto; } + + .form-search { + margin-bottom: 1rem; + + .btn { + font-weight: normal; + } + } } .sidebar-controls { diff --git a/tildes/scss/modules/_site-header.scss b/tildes/scss/modules/_site-header.scss index eff2914..708d5a9 100644 --- a/tildes/scss/modules/_site-header.scss +++ b/tildes/scss/modules/_site-header.scss @@ -12,6 +12,7 @@ } .site-header-context { + white-space: nowrap; overflow: hidden; text-overflow: ellipsis; } diff --git a/tildes/sql/init/triggers/topics/topics.sql b/tildes/sql/init/triggers/topics/topics.sql index c62891e..6825a85 100644 --- a/tildes/sql/init/triggers/topics/topics.sql +++ b/tildes/sql/init/triggers/topics/topics.sql @@ -12,3 +12,18 @@ CREATE TRIGGER delete_topic_set_deleted_time_update FOR EACH ROW WHEN (OLD.is_deleted = false AND NEW.is_deleted = true) EXECUTE PROCEDURE set_topic_deleted_time(); + + +CREATE TRIGGER topic_update_search_tsv_insert + BEFORE INSERT ON topics + FOR EACH ROW + EXECUTE PROCEDURE tsvector_update_trigger(search_tsv, 'pg_catalog.english', title, markdown); + +CREATE TRIGGER topic_update_search_tsv_update + BEFORE UPDATE ON topics + FOR EACH ROW + WHEN ( + (OLD.title IS DISTINCT FROM NEW.title) + OR (OLD.markdown IS DISTINCT FROM NEW.markdown) + ) + EXECUTE PROCEDURE tsvector_update_trigger(search_tsv, 'pg_catalog.english', title, markdown); diff --git a/tildes/tildes/__init__.py b/tildes/tildes/__init__.py index 473731c..6ec4b2f 100644 --- a/tildes/tildes/__init__.py +++ b/tildes/tildes/__init__.py @@ -142,6 +142,7 @@ def current_listing_base_url( base_vars_by_route: Dict[str, Tuple[str, ...]] = { "group": ("order", "period", "per_page", "tag", "unfiltered"), "home": ("order", "period", "per_page", "tag", "unfiltered"), + "search": ("order", "period", "per_page", "q"), "user": ("per_page", "type"), } @@ -175,6 +176,7 @@ def current_listing_normal_url( normal_vars_by_route: Dict[str, Tuple[str, ...]] = { "group": ("order", "period", "per_page"), "home": ("order", "period", "per_page"), + "search": ("order", "period", "per_page", "q"), "user": ("per_page",), } diff --git a/tildes/tildes/models/topic/topic.py b/tildes/tildes/models/topic/topic.py index 932f840..b822862 100644 --- a/tildes/tildes/models/topic/topic.py +++ b/tildes/tildes/models/topic/topic.py @@ -14,7 +14,7 @@ from sqlalchemy import ( Text, TIMESTAMP, ) -from sqlalchemy.dialects.postgresql import ENUM, JSONB +from sqlalchemy.dialects.postgresql import ENUM, JSONB, TSVECTOR from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import deferred, relationship @@ -111,12 +111,16 @@ class Topic(DatabaseModel): ) is_official: bool = Column(Boolean, nullable=False, server_default="false") is_locked: bool = Column(Boolean, nullable=False, server_default="false") + search_tsv: Any = deferred(Column(TSVECTOR)) user: User = relationship("User", lazy=False, innerjoin=True) group: Group = relationship("Group", innerjoin=True) - # Create a GiST index on the tags column - __table_args__ = (Index("ix_topics_tags_gist", _tags, postgresql_using="gist"),) + # Create specialized indexes + __table_args__ = ( + Index("ix_topics_tags_gist", _tags, postgresql_using="gist"), + Index("ix_topics_search_tsv_gin", "search_tsv", postgresql_using="gin"), + ) @hybrid_property def markdown(self) -> Optional[str]: diff --git a/tildes/tildes/models/topic/topic_query.py b/tildes/tildes/models/topic/topic_query.py index 4d3ad59..af7edf7 100644 --- a/tildes/tildes/models/topic/topic_query.py +++ b/tildes/tildes/models/topic/topic_query.py @@ -3,6 +3,7 @@ from typing import Any, Sequence from pyramid.request import Request +from sqlalchemy import func from sqlalchemy.sql.expression import and_, null from sqlalchemy_utils import Ltree @@ -137,3 +138,7 @@ class TopicQuery(PaginatedQuery): # pylint: disable=protected-access return self.filter(Topic._tags.descendant_of(tag)) # type: ignore + + def search(self, query: str) -> "TopicQuery": + """Restrict the topics to ones that match a search query (generative).""" + return self.filter(Topic.search_tsv.op("@@")(func.plainto_tsquery(query))) diff --git a/tildes/tildes/routes.py b/tildes/tildes/routes.py index 82c1a47..f39b70f 100644 --- a/tildes/tildes/routes.py +++ b/tildes/tildes/routes.py @@ -17,6 +17,8 @@ def includeme(config: Configurator) -> None: """Set up application routes.""" config.add_route("home", "/") + config.add_route("search", "/search") + config.add_route("groups", "/groups") config.add_route("login", "/login") diff --git a/tildes/tildes/templates/home.jinja2 b/tildes/tildes/templates/home.jinja2 index 4e7aca8..e8c9fd4 100644 --- a/tildes/tildes/templates/home.jinja2 +++ b/tildes/tildes/templates/home.jinja2 @@ -17,6 +17,13 @@ {% endblock %} {% block sidebar %} +
    +
    + + +
    + +

    Home

    The home page shows topics from groups that you're subscribed to.

    {% if request.user %} diff --git a/tildes/tildes/templates/search.jinja2 b/tildes/tildes/templates/search.jinja2 new file mode 100644 index 0000000..90e41f0 --- /dev/null +++ b/tildes/tildes/templates/search.jinja2 @@ -0,0 +1,31 @@ +{% extends 'topic_listing.jinja2' %} + +{% block title %}Search results for "{{ search }}"{% endblock %} + +{% block header_context_link %}Search: "{{ search }}"{% endblock %} + +{% block sidebar %} +
    +
    + + +
    + + +

    Search results

    + +

    Back to home page

    + + {% if request.user and request.user.subscriptions %} +
    +
      +
    • Subscriptions
      • + +
    + {% endif %} + +{% endblock %} diff --git a/tildes/tildes/templates/topic_listing.jinja2 b/tildes/tildes/templates/topic_listing.jinja2 index d8f9f56..ec91068 100644 --- a/tildes/tildes/templates/topic_listing.jinja2 +++ b/tildes/tildes/templates/topic_listing.jinja2 @@ -46,9 +46,14 @@
    - {% if tag %} + {% if tag is defined and tag %} {% endif %} + + {% if search is defined %} + + {% endif %} +
    + +
    + +

    ~{{ group.path }}

    {% if group.short_description %} diff --git a/tildes/tildes/views/topic.py b/tildes/tildes/views/topic.py index 5156272..3f22eba 100644 --- a/tildes/tildes/views/topic.py +++ b/tildes/tildes/views/topic.py @@ -164,6 +164,63 @@ def get_group_topics( } +@view_config(route_name="search", renderer="search.jinja2") +@use_kwargs(TopicListingSchema(only=("after", "before", "order", "per_page", "period"))) +@use_kwargs({"search": String(load_from="q")}) +def get_search( + request: Request, + order: Any, + period: Any, + after: str, + before: str, + per_page: int, + search: str, +) -> dict: + """Get a list of search results.""" + # pylint: disable=too-many-arguments + if order is missing: + order = TopicSortOption.NEW + + if period is missing: + period = None + + query = ( + request.query(Topic) + .join_all_relationships() + .search(search) + .apply_sort_option(order) + ) + + # restrict the time period, if not set to "all time" + if period: + query = query.inside_time_period(period) + + # apply before/after pagination restrictions if relevant + if before: + query = query.before_id36(before) + + if after: + query = query.after_id36(after) + + topics = query.get_page(per_page) + + period_options = [SimpleHoursPeriod(hours) for hours in (1, 12, 24, 72)] + + # add the current period to the bottom of the dropdown if it's not one of the + # "standard" ones + if period and period not in period_options: + period_options.append(period) + + return { + "search": search, + "topics": topics, + "order": order, + "order_options": TopicSortOption, + "period": period, + "period_options": period_options, + } + + @view_config( route_name="new_topic", renderer="new_topic.jinja2", permission="post_topic" ) From 595af8f9ae2e74dfa78043e9866d09e7e5c44e26 Mon Sep 17 00:00:00 2001 From: Jeff Kayser Date: Tue, 14 Aug 2018 15:40:05 -0400 Subject: [PATCH 12/14] Add buttons for bulk collapse/expand of comments Adds two buttons: one for collapsing all child comments (non-top-level ones) and one for uncollapsing all comments. --- tildes/scss/modules/_btn.scss | 16 ++++++++++++++++ .../js/behaviors/comment-collapse-all-button.js | 11 +++++++++++ .../js/behaviors/comment-expand-all-button.js | 11 +++++++++++ tildes/tildes/templates/topic.jinja2 | 3 +++ 4 files changed, 41 insertions(+) create mode 100644 tildes/static/js/behaviors/comment-collapse-all-button.js create mode 100644 tildes/static/js/behaviors/comment-expand-all-button.js diff --git a/tildes/scss/modules/_btn.scss b/tildes/scss/modules/_btn.scss index 0dc0641..90d901d 100644 --- a/tildes/scss/modules/_btn.scss +++ b/tildes/scss/modules/_btn.scss @@ -65,6 +65,22 @@ } } +.btn-comment-collapse-all { + margin-left: 0.4rem; + border-left-width: 1px; + @media (min-width: $size-md) { + margin-top: 0.35rem; + } +} + +.btn-comment-expand-all { + margin-left: 0.2rem; + border-left-width: 1px; + @media (min-width: $size-md) { + margin-top: 0.35rem; + } +} + .btn-comment-tag { display: inline-flex; align-items: center; diff --git a/tildes/static/js/behaviors/comment-collapse-all-button.js b/tildes/static/js/behaviors/comment-collapse-all-button.js new file mode 100644 index 0000000..64827a5 --- /dev/null +++ b/tildes/static/js/behaviors/comment-collapse-all-button.js @@ -0,0 +1,11 @@ +$.onmount('[data-js-comment-collapse-all-button]', function() { + $(this).click(function(event) { + $('.comment[data-comment-depth="1"]:not(.is-comment-collapsed)').each( + function(idx, child) { + $(child).find( + '[data-js-comment-collapse-button]:first').trigger('click'); + }); + + $(this).blur(); + }); +}); diff --git a/tildes/static/js/behaviors/comment-expand-all-button.js b/tildes/static/js/behaviors/comment-expand-all-button.js new file mode 100644 index 0000000..f870139 --- /dev/null +++ b/tildes/static/js/behaviors/comment-expand-all-button.js @@ -0,0 +1,11 @@ +$.onmount('[data-js-comment-expand-all-button]', function() { + $(this).click(function(event) { + $('.comment.is-comment-collapsed').each( + function(idx, child) { + $(child).find( + '[data-js-comment-collapse-button]:first').trigger('click'); + }); + + $(this).blur(); + }); +}); diff --git a/tildes/tildes/templates/topic.jinja2 b/tildes/tildes/templates/topic.jinja2 index a3512cb..cdd913f 100644 --- a/tildes/tildes/templates/topic.jinja2 +++ b/tildes/tildes/templates/topic.jinja2 @@ -153,6 +153,9 @@ {% endtrans %} + + +
    From 5170d283f7db4dac387932e3c632cedbc05801c4 Mon Sep 17 00:00:00 2001 From: Deimos Date: Tue, 21 Aug 2018 12:48:40 -0600 Subject: [PATCH 13/14] Adjust styles for comment collapse buttons Just a few minor style adjustments to simplify the styles and fix some bad wrapping behavior with the new bulk-collapse/expand buttons. --- tildes/scss/modules/_btn.scss | 18 ------------------ tildes/scss/modules/_topic.scss | 6 ++++++ tildes/tildes/templates/topic.jinja2 | 4 ++-- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/tildes/scss/modules/_btn.scss b/tildes/scss/modules/_btn.scss index 90d901d..7acc525 100644 --- a/tildes/scss/modules/_btn.scss +++ b/tildes/scss/modules/_btn.scss @@ -52,10 +52,8 @@ font-weight: normal; - border-left-width: 0; margin-right: 0.4rem; @media (min-width: $size-md) { - border-left-width: 1px; margin-right: 0.2rem; min-width: 0.8rem; } @@ -65,22 +63,6 @@ } } -.btn-comment-collapse-all { - margin-left: 0.4rem; - border-left-width: 1px; - @media (min-width: $size-md) { - margin-top: 0.35rem; - } -} - -.btn-comment-expand-all { - margin-left: 0.2rem; - border-left-width: 1px; - @media (min-width: $size-md) { - margin-top: 0.35rem; - } -} - .btn-comment-tag { display: inline-flex; align-items: center; diff --git a/tildes/scss/modules/_topic.scss b/tildes/scss/modules/_topic.scss index 82827aa..4e6ac05 100644 --- a/tildes/scss/modules/_topic.scss +++ b/tildes/scss/modules/_topic.scss @@ -248,8 +248,14 @@ .topic-comments { header { display: flex; + flex-wrap: wrap; + align-items: center; + + margin-bottom: 0.4rem; h2 { + margin-bottom: 0; + margin-right: 0.4rem; white-space: nowrap; } diff --git a/tildes/tildes/templates/topic.jinja2 b/tildes/tildes/templates/topic.jinja2 index cdd913f..b737087 100644 --- a/tildes/tildes/templates/topic.jinja2 +++ b/tildes/tildes/templates/topic.jinja2 @@ -153,8 +153,8 @@ {% endtrans %} - - + +
    From 0760f6441de661171c506c273e07abef143214c7 Mon Sep 17 00:00:00 2001 From: Deimos Date: Tue, 21 Aug 2018 13:42:31 -0600 Subject: [PATCH 14/14] Fix comments header styles applying to comments Previous way was a bad way of doing the styles and some of the rules were leaking through into the comments themselves. This should be more specific. --- tildes/scss/modules/_topic.scss | 26 ++++++++++++-------------- tildes/tildes/templates/topic.jinja2 | 2 +- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/tildes/scss/modules/_topic.scss b/tildes/scss/modules/_topic.scss index 4e6ac05..2307fcc 100644 --- a/tildes/scss/modules/_topic.scss +++ b/tildes/scss/modules/_topic.scss @@ -245,23 +245,21 @@ overflow: auto; } -.topic-comments { - header { - display: flex; - flex-wrap: wrap; - align-items: center; +.topic-comments-header { + display: flex; + flex-wrap: wrap; + align-items: center; - margin-bottom: 0.4rem; + margin-bottom: 0.4rem; - h2 { - margin-bottom: 0; - margin-right: 0.4rem; - white-space: nowrap; - } + h2 { + margin-bottom: 0; + margin-right: 0.4rem; + white-space: nowrap; + } - .form-listing-options { - margin-left: auto; - } + .form-listing-options { + margin-left: auto; } } diff --git a/tildes/tildes/templates/topic.jinja2 b/tildes/tildes/templates/topic.jinja2 index b737087..fa08a6f 100644 --- a/tildes/tildes/templates/topic.jinja2 +++ b/tildes/tildes/templates/topic.jinja2 @@ -144,7 +144,7 @@ {% if topic.num_comments > 0 %}
    -
    +

    {% trans num_comments=topic.num_comments %} {{ num_comments }} comment