Refactor Topic ACL, update tests

tildes/tests/test_topic.py

@@ -2,11 +2,6 @@ from datetime import timedelta
 
 from freezegun import freeze_time
 from marshmallow.fields import URL
-from pyramid.security import (
-    Authenticated,
-    Everyone,
-    principals_allowed_by_permission,
-)
 from pytest import raises
 
 from tildes.lib.datetime import utc_now
@@ -103,67 +98,6 @@ def test_edit_markdown_on_text_topic(text_topic):
     assert text_topic.rendered_html != original_html
 
 
-def test_topic_viewing_permission(text_topic):
-    """Ensure that anyone can view a topic by default."""
-    principals = principals_allowed_by_permission(text_topic, 'view')
-    assert Everyone in principals
-
-
-def test_text_topic_editing_permission(text_topic):
-    """Ensure a text topic's owner (and nobody else) is able to edit it."""
-    principals = principals_allowed_by_permission(text_topic, 'edit')
-    assert principals == {text_topic.user.user_id}
-
-
-def test_deleted_topic_editing_permission(text_topic):
-    """Ensure a deleted topic can't be edited."""
-    text_topic.is_deleted = True
-    principals = principals_allowed_by_permission(text_topic, 'edit')
-    assert not principals
-
-
-def test_link_topic_editing_permission(link_topic):
-    """Ensure that nobody has edit permission on a link topic."""
-    principals = principals_allowed_by_permission(link_topic, 'edit')
-    assert not principals
-
-
-def test_topic_deleting_permission(text_topic):
-    """Ensure that the topic's owner (and nobody else) is able to delete it."""
-    principals = principals_allowed_by_permission(text_topic, 'delete')
-    assert principals == {text_topic.user.user_id}
-
-
-def test_deleted_topic_deleting_permission(text_topic):
-    """Ensure a deleted topic can't be deleted (again)."""
-    text_topic.is_deleted = True
-    assert not principals_allowed_by_permission(text_topic, 'delete')
-
-
-def test_topic_view_author_permission(text_topic):
-    """Ensure anyone can view a topic's author normally."""
-    principals = principals_allowed_by_permission(text_topic, 'view_author')
-    assert Everyone in principals
-
-
-def test_deleted_topic_view_author_forbidden(text_topic):
-    """Ensure nobody can view the author of a deleted topic."""
-    text_topic.is_deleted = True
-    principals = principals_allowed_by_permission(text_topic, 'view_author')
-    assert not principals
-
-
-def test_topic_comment_permission(text_topic):
-    """Ensure authed users have comment perms before deletion, not after."""
-    principals = principals_allowed_by_permission(text_topic, 'comment')
-    assert Authenticated in principals
-
-    text_topic.is_deleted = True
-
-    principals = principals_allowed_by_permission(text_topic, 'comment')
-    assert Authenticated not in principals
-
-
 def test_edit_grace_period(text_topic):
     """Ensure last_edited_time isn't set if the edit is inside grace period."""
     one_sec = timedelta(seconds=1)

tildes/tests/test_topic_permissions.py

@@ -0,0 +1,85 @@
+from pyramid.security import (
+    Authenticated,
+    Everyone,
+    principals_allowed_by_permission,
+)
+
+
+def test_topic_viewing_permission(text_topic):
+    """Ensure that anyone can view a topic by default."""
+    principals = principals_allowed_by_permission(text_topic, 'view')
+    assert Everyone in principals
+
+
+def test_deleted_topic_permissions_removed(topic):
+    """Ensure that deleted topics lose all permissions except "view"."""
+    topic.is_deleted = True
+
+    assert principals_allowed_by_permission(topic, 'view') == {Everyone}
+
+    all_permissions = [
+        perm for (_, _, perm) in topic.__acl__() if perm != 'view']
+    for permission in all_permissions:
+        assert not principals_allowed_by_permission(topic, permission)
+
+
+def test_text_topic_editing_permission(text_topic):
+    """Ensure a text topic's owner (and nobody else) is able to edit it."""
+    principals = principals_allowed_by_permission(text_topic, 'edit')
+    assert principals == {text_topic.user.user_id}
+
+
+def test_link_topic_editing_permission(link_topic):
+    """Ensure that nobody has edit permission on a link topic."""
+    principals = principals_allowed_by_permission(link_topic, 'edit')
+    assert not principals
+
+
+def test_topic_deleting_permission(text_topic):
+    """Ensure that the topic's owner (and nobody else) is able to delete it."""
+    principals = principals_allowed_by_permission(text_topic, 'delete')
+    assert principals == {text_topic.user.user_id}
+
+
+def test_topic_view_author_permission(text_topic):
+    """Ensure anyone can view a topic's author normally."""
+    principals = principals_allowed_by_permission(text_topic, 'view_author')
+    assert Everyone in principals
+
+
+def test_removed_topic_view_author_permission(topic):
+    """Ensure only admins and the author can view a removed topic's author."""
+    topic.is_removed = True
+    principals = principals_allowed_by_permission(topic, 'view_author')
+    assert principals == {'admin', topic.user_id}
+
+
+def test_topic_view_content_permission(text_topic):
+    """Ensure anyone can view a topic's content normally."""
+    principals = principals_allowed_by_permission(text_topic, 'view_content')
+    assert Everyone in principals
+
+
+def test_removed_topic_view_content_permission(topic):
+    """Ensure only admins and the author can view a removed topic's content."""
+    topic.is_removed = True
+    principals = principals_allowed_by_permission(topic, 'view_content')
+    assert principals == {'admin', topic.user_id}
+
+
+def test_topic_comment_permission(text_topic):
+    """Ensure authed users have comment perms on a topic by default."""
+    principals = principals_allowed_by_permission(text_topic, 'comment')
+    assert Authenticated in principals
+
+
+def test_locked_topic_comment_permission(topic):
+    """Ensure only admins can post (top-level) comments on locked topics."""
+    topic.is_locked = True
+    assert principals_allowed_by_permission(topic, 'comment') == {'admin'}
+
+
+def test_removed_topic_comment_permission(topic):
+    """Ensure only admins can post (top-level) comments on removed topics."""
+    topic.is_removed = True
+    assert principals_allowed_by_permission(topic, 'comment') == {'admin'}

tildes/tildes/models/topic/topic.py

@@ -226,39 +226,78 @@ class Topic(DatabaseModel):
 
     def __acl__(self) -> Sequence[Tuple[str, Any, str]]:
         """Pyramid security ACL."""
-        acl = [
-            (Allow, Everyone, 'view'),
-            (Allow, 'admin', 'lock'),
-            (Allow, 'admin', 'move'),
-            (Allow, 'admin', 'edit_title'),
-            (Allow, 'admin', 'tag'),
-        ]
-
-        if not (self.is_locked or self.is_deleted or self.is_removed):
-            acl.append((Allow, Authenticated, 'comment'))
-        else:
+        acl = []
+
+        # deleted topics allow "general" viewing, but nothing else
+        if self.is_deleted:
+            acl.append((Allow, Everyone, 'view'))
+            acl.append(DENY_ALL)
+
+        # view:
+        #  - everyone gets "general" viewing permission for all topics
+        acl.append((Allow, Everyone, 'view'))
+
+        # view_author:
+        #  - removed topics' author is only visible to the author and admins
+        #  - otherwise, everyone can view the author
+        if self.is_removed:
+            acl.append((Allow, 'admin', 'view_author'))
+            acl.append((Allow, self.user_id, 'view_author'))
+            acl.append((Deny, Everyone, 'view_author'))
+
+        acl.append((Allow, Everyone, 'view_author'))
+
+        # view_content:
+        #  - removed topics' content is only visible to the author and admins
+        #  - otherwise, everyone can view the content
+        if self.is_removed:
+            acl.append((Allow, 'admin', 'view_content'))
+            acl.append((Allow, self.user_id, 'view_content'))
+            acl.append((Deny, Everyone, 'view_content'))
+
+        acl.append((Allow, Everyone, 'view_content'))
+
+        # vote:
+        #  - removed topics can't be voted on by anyone
+        #  - otherwise, logged-in users except the author can vote
+        if self.is_removed:
+            acl.append((Deny, Everyone, 'vote'))
+
+        acl.append((Deny, self.user_id, 'vote'))
+        acl.append((Allow, Authenticated, 'vote'))
+
+        # comment:
+        #  - removed topics can only be commented on by admins
+        #  - locked topics can only be commented on by admins
+        #  - otherwise, logged-in users can comment
+        if self.is_removed:
             acl.append((Allow, 'admin', 'comment'))
+            acl.append((Deny, Everyone, 'comment'))
 
-        if not (self.is_deleted or self.is_removed):
-            acl.append((Allow, Everyone, 'view_content'))
-            acl.append((Allow, Everyone, 'view_author'))
+        if self.is_locked:
+            acl.append((Allow, 'admin', 'comment'))
+            acl.append((Deny, Everyone, 'comment'))
 
-            # everyone except the topic's author can vote on it
-            acl.append((Deny, self.user_id, 'vote'))
-            acl.append((Allow, Authenticated, 'vote'))
+        acl.append((Allow, Authenticated, 'comment'))
 
-            acl.append((Allow, self.user_id, 'tag'))
+        # edit:
+        #  - only text topics can be edited, only by the author
+        if self.is_text_type:
+            acl.append((Allow, self.user_id, 'edit'))
 
-        if not self.is_deleted:
-            acl.append((Allow, 'admin', 'view'))
+        # delete:
+        #  - only the author can delete
+        acl.append((Allow, self.user_id, 'delete'))
 
-            acl.append((Allow, self.user_id, 'view'))
-            acl.append((Allow, self.user_id, 'view_author'))
-            acl.append((Allow, self.user_id, 'view_content'))
+        # tag:
+        #  - only the author and admins can tag topics
+        acl.append((Allow, self.user_id, 'tag'))
+        acl.append((Allow, 'admin', 'tag'))
 
-            if self.is_text_type:
-                acl.append((Allow, self.user_id, 'edit'))
-            acl.append((Allow, self.user_id, 'delete'))
+        # admin tools
+        acl.append((Allow, 'admin', 'lock'))
+        acl.append((Allow, 'admin', 'move'))
+        acl.append((Allow, 'admin', 'edit_title'))
 
         acl.append(DENY_ALL)