From 67b4f7bf377cb2bbf2d51fc8dbf7701e22eddac1 Mon Sep 17 00:00:00 2001 From: Deimos Date: Wed, 14 Nov 2018 19:49:23 -0700 Subject: [PATCH] Add ability to (manually) delete user accounts Until now, if people want their account deleted, I've just been banning it. This will let me do it properly, but some additional cleanup should be added in the future once I think through what's safe to get rid of from deleted accounts. --- ...06146_users_add_is_deleted_deleted_time.py | 54 +++++++++++ tildes/sql/init/triggers/users/users.sql | 17 ++++ tildes/tests/test_user.py | 19 ++++ tildes/tildes/auth.py | 5 +- tildes/tildes/models/user/user.py | 18 +++- tildes/tildes/resources/user.py | 2 +- tildes/tildes/templates/user.jinja2 | 97 ++++++++++--------- 7 files changed, 159 insertions(+), 53 deletions(-) create mode 100644 tildes/alembic/versions/a0e0b6206146_users_add_is_deleted_deleted_time.py create mode 100644 tildes/sql/init/triggers/users/users.sql diff --git a/tildes/alembic/versions/a0e0b6206146_users_add_is_deleted_deleted_time.py b/tildes/alembic/versions/a0e0b6206146_users_add_is_deleted_deleted_time.py new file mode 100644 index 0000000..ff2f3c1 --- /dev/null +++ b/tildes/alembic/versions/a0e0b6206146_users_add_is_deleted_deleted_time.py @@ -0,0 +1,54 @@ +"""Users: Add is_deleted/deleted_time + +Revision ID: a0e0b6206146 +Revises: 53567981cdf4 +Create Date: 2018-11-13 23:49:20.764289 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "a0e0b6206146" +down_revision = "53567981cdf4" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "users", sa.Column("deleted_time", sa.TIMESTAMP(timezone=True), nullable=True) + ) + op.add_column( + "users", + sa.Column("is_deleted", sa.Boolean(), server_default="false", nullable=False), + ) + op.create_index(op.f("ix_users_is_deleted"), "users", ["is_deleted"], unique=False) + + op.execute( + """ + CREATE OR REPLACE FUNCTION set_user_deleted_time() RETURNS TRIGGER AS $$ + BEGIN + NEW.deleted_time := current_timestamp; + + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + + CREATE TRIGGER delete_user_set_deleted_time_update + BEFORE UPDATE ON users + FOR EACH ROW + WHEN (OLD.is_deleted = false AND NEW.is_deleted = true) + EXECUTE PROCEDURE set_user_deleted_time(); + """ + ) + + +def downgrade(): + op.execute("DROP TRIGGER delete_user_set_deleted_time_update ON users") + op.execute("DROP FUNCTION set_user_deleted_time") + + op.drop_index(op.f("ix_users_is_deleted"), table_name="users") + op.drop_column("users", "is_deleted") + op.drop_column("users", "deleted_time") diff --git a/tildes/sql/init/triggers/users/users.sql b/tildes/sql/init/triggers/users/users.sql new file mode 100644 index 0000000..2e95227 --- /dev/null +++ b/tildes/sql/init/triggers/users/users.sql @@ -0,0 +1,17 @@ +-- Copyright (c) 2018 Tildes contributors +-- SPDX-License-Identifier: AGPL-3.0-or-later + +-- set user.deleted_time when it's deleted +CREATE OR REPLACE FUNCTION set_user_deleted_time() RETURNS TRIGGER AS $$ +BEGIN + NEW.deleted_time := current_timestamp; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER delete_user_set_deleted_time_update + BEFORE UPDATE ON users + FOR EACH ROW + WHEN (OLD.is_deleted = false AND NEW.is_deleted = true) + EXECUTE PROCEDURE set_user_deleted_time(); diff --git a/tildes/tests/test_user.py b/tildes/tests/test_user.py index 5313525..ce278ad 100644 --- a/tildes/tests/test_user.py +++ b/tildes/tests/test_user.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: AGPL-3.0-or-later from marshmallow.exceptions import ValidationError +from pyramid.security import principals_allowed_by_permission from pytest import raises from sqlalchemy.exc import IntegrityError @@ -115,3 +116,21 @@ def test_change_password_to_username(session_user): """Ensure users can't change password to the same as their username.""" with raises(ValidationError): session_user.change_password("session user password", session_user.username) + + +def test_deleted_user_no_message_permission(): + """Ensure nobody can message a deleted user.""" + deleted_user = User("Deleted_User", "password") + deleted_user.is_deleted = True + + principals = principals_allowed_by_permission(deleted_user, "message") + assert not principals + + +def test_banned_user_no_message_permission(): + """Ensure nobody can message a banned user.""" + banned_user = User("Banned_User", "password") + banned_user.is_banned = True + + principals = principals_allowed_by_permission(banned_user, "message") + assert not principals diff --git a/tildes/tildes/auth.py b/tildes/tildes/auth.py index 4f29e2a..226c2e2 100644 --- a/tildes/tildes/auth.py +++ b/tildes/tildes/auth.py @@ -51,8 +51,9 @@ def auth_callback(user_id: int, request: Request) -> Optional[Sequence[str]]: if not request.user: return None - # if the user is banned, log them out - is there a better place to do this? - if request.user.is_banned: + # if the user is deleted or banned, log them out + # (is there a better place to do this?) + if request.user.is_banned or request.user.is_deleted: request.session.invalidate() raise HTTPFound("/") diff --git a/tildes/tildes/models/user/user.py b/tildes/tildes/models/user/user.py index 2ffd55f..58ecdd8 100644 --- a/tildes/tildes/models/user/user.py +++ b/tildes/tildes/models/user/user.py @@ -51,6 +51,8 @@ class User(DatabaseModel): deletions, and updates to unread_user_ids in message_conversations. - last_exemplary_label_time will be set when a row for an exemplary label is inserted into comment_labels. + Internal: + - deleted_time will be set when is_deleted is set to true """ schema_class = UserSchema @@ -96,6 +98,10 @@ class User(DatabaseModel): ) open_new_tab_text: bool = Column(Boolean, nullable=False, server_default="false") theme_default: str = Column(Text) + is_deleted: bool = Column( + Boolean, nullable=False, server_default="false", index=True + ) + deleted_time: Optional[datetime] = Column(TIMESTAMP(timezone=True)) is_banned: bool = Column(Boolean, nullable=False, server_default="false") permissions: Any = Column(JSONB(none_as_null=True)) home_default_order: Optional[TopicSortOption] = Column(ENUM(TopicSortOption)) @@ -137,11 +143,13 @@ class User(DatabaseModel): acl.append((Allow, Everyone, "view")) # message: - # - banned users can't be messaged - # - otherwise, anyone can message a user except themself - if not self.is_banned: - acl.append((Deny, self.user_id, "message")) - acl.append((Allow, Authenticated, "message")) + # - deleted and banned users can't be messaged + # - otherwise, logged-in users can message anyone except themselves + if self.is_banned or self.is_deleted: + acl.append((Deny, Everyone, "message")) + + acl.append((Deny, self.user_id, "message")) + acl.append((Allow, Authenticated, "message")) # grant the user all other permissions on themself acl.append((Allow, self.user_id, ALL_PERMISSIONS)) diff --git a/tildes/tildes/resources/user.py b/tildes/tildes/resources/user.py index 7ce31cb..79b65ab 100644 --- a/tildes/tildes/resources/user.py +++ b/tildes/tildes/resources/user.py @@ -14,6 +14,6 @@ from tildes.schemas.user import UserSchema @use_kwargs(UserSchema(only=("username",)), locations=("matchdict",)) def user_by_username(request: Request, username: str) -> User: """Get a user specified by {username} in the route or 404 if not found.""" - query = request.query(User).filter(User.username == username) + query = request.query(User).include_deleted().filter(User.username == username) return get_resource(request, query) diff --git a/tildes/tildes/templates/user.jinja2 b/tildes/tildes/templates/user.jinja2 index 097b37f..0a5a033 100644 --- a/tildes/tildes/templates/user.jinja2 +++ b/tildes/tildes/templates/user.jinja2 @@ -13,63 +13,70 @@ {{ user.username }} {% endblock %} -{# Only show the heading if they can't see the type tabs #} +{# Only show the heading if they can't see the type tabs and the user isn't deleted #} {% block main_heading %} - {% if not request.has_permission('view_types', user) %} + {% if not user.is_deleted and not request.has_permission('view_types', user) %} {{ user.username }}'s recent activity {% endif %} {% endblock %} {% block content %} -{% if request.has_permission('view_types', user) %} -
- -
  • - Recent activity -
    • -
  • - Topics -
    • -
  • - Comments -
    • -     +{% if user.is_deleted %} +
    +

    This user has deleted their account

    +
    Back to the home page
    -{% endif %} - -{% if posts %} -
      - {% for post in posts if request.has_permission('view', post) %} -
    1. - {% if post is topic %} - {{ render_topic_for_listing(post, show_group=True) }} - {% elif post is comment %} -

      Comment on {{ post.topic.title }} in {{ group_linked(post.topic.group.path) }}

      - {{ render_single_comment(post) }} - {% endif %} -
      2. - {% endfor %} -
    +{% else %} + {% if request.has_permission('view_types', user) %} +
    + +
  • + Recent activity +
    • +
  • + Topics +
    • +
  • + Comments +
    • +     +
    + {% endif %} - {% if post_type and (posts.has_prev_page or posts.has_next_page) %} -