Merge pull request #7188 from akatsoulas/visibility-filtering

Visibility filtering

Author: akatsoulas

.pre-commit-config.yaml

@@ -23,6 +23,18 @@ repos:
           - [email protected]
           - [email protected]
         files: "^svelte/"
+  - repo: local
+    hooks:
+      - id: check-group-visibility-leak
+        name: Check for group visibility leaks
+        entry: >
+          bash -c 'if grep -rn "\.groups\.all()" --include="*.py" --include="*.html" kitsune/ --exclude-dir=migrations | grep -v "# noqa: group-leak"; then
+          echo "ERROR - Found .groups.all() which bypasses GroupProfile visibility";
+          echo "Use profile.visible_group_profiles(viewer) or GroupProfile.objects.visible(viewer) instead";
+          echo "See docs/conventions.md Security Patterns section";
+          exit 1; fi'
+        language: system
+        pass_filenames: false
   - repo: https://github.com/pre-commit/mirrors-mypy
     rev: v1.15.0
     hooks:

CLAUDE.md

@@ -212,6 +212,51 @@ Kitsune uses Django's i18n URL patterns with specific routing conventions:
 - API endpoints follow REST conventions in `api.py` files
 - Each app has dedicated `urls.py` and often `urls_api.py`
 
+### Security Patterns
+
+**GroupProfile Visibility - CRITICAL:**
+
+GroupProfile has three visibility levels (PUBLIC, PRIVATE, MODERATED) that control who can see a group. **Never bypass these checks:**
+
+**DANGER - Privacy Leak:**
+```python
+# WRONG - Exposes ALL groups, ignoring visibility settings
+user.groups.all()  # Leaks PRIVATE groups!
+profile.user.groups.all()  # Leaks PRIVATE groups!
+
+# WRONG - Direct Group queryset bypasses GroupProfile visibility
+Group.objects.filter(user=some_user)  # No visibility filtering!
+```
+
+**SAFE - Respects visibility:**
+```python
+# Correct - Use Profile.visible_group_profiles()
+profile.visible_group_profiles(viewer=request.user)
+
+# Correct - Use GroupProfile manager
+GroupProfile.objects.visible(viewer).filter(group__user=some_user)
+
+# Correct - Check individual group visibility
+group_profile.can_view(request.user)
+```
+
+**Why this matters:**
+- Django's `User.groups` relationship returns ALL Group objects, ignoring GroupProfile visibility
+- PRIVATE groups would be exposed to anyone viewing a user profile
+- Search indexing would leak private group membership
+- API responses would expose sensitive group information
+
+**Safe patterns implemented:**
+- `Profile.visible_group_profiles(viewer)` - Get groups respecting visibility
+- `GroupProfile.objects.visible(viewer)` - Manager method for filtering
+- Search indexing excludes PRIVATE groups automatically
+- All views use visibility-aware methods
+
+**Never:**
+- Use `user.groups.all()` in templates, views, or API serializers
+- Query `Group` model directly when GroupProfile visibility matters
+- Bypass `.visible()` filtering for user-facing data
+
 ### Template System
 
 **Template Architecture:**
@@ -256,3 +301,25 @@ Kitsune uses Django's i18n URL patterns with specific routing conventions:
 - Dependabot automatically updates dependencies weekly
 - **Do not add trailing spaces at the end of files**
 - **Exception handling:** Be specific with exception types. Avoid catching plain `Exception` when the specific exception that could occur is known. For example, use `Model.DoesNotExist` when calling `.get()` on a Django queryset, or `KeyError` when accessing dictionary keys. This makes error handling more explicit and allows unexpected exceptions to be raised rather than silently caught.
+
+### Naming Conventions
+
+**Manager Methods:**
+- Custom manager methods should balance conciseness with explicitness
+- **Preferred:** Concise, verb-based names following Django's queryset API patterns: `all()`, `filter()`, `exclude()`, `visible()`, `active()`
+- **Alternative:** Verbose descriptive names when explicitness adds clarity (per "explicit is better than implicit")
+- Choose based on context: if the method name alone isn't clear, add descriptive prefixes
+- If a method returns a queryset, the name should read naturally when chained
+
+**Example:**
+```python
+# Preferred - concise, follows Django patterns, clear in context
+GroupProfile.objects.visible(user)
+Article.objects.active()
+Question.objects.recent()
+
+# Alternative - more explicit, acceptable when clarity is prioritized
+GroupProfile.objects.filter_by_visibility(user)
+Article.objects.get_active_articles()
+Question.objects.filter_by_recent()
+```

docs/conventions.md

@@ -19,6 +19,37 @@ When creating and/or modifying Python functions/methods, we add [type
 hints](https://docs.python.org/3/library/typing.html) to their arguments
 and result, but only when it makes sense. See [our Architectural Decision Record](architecture/decisions/0004-type-checking.md) for more details.
 
+## Security patterns
+
+### GroupProfile visibility
+
+**CRITICAL:** Never bypass GroupProfile visibility checks when displaying user's groups.
+
+GroupProfile has three visibility levels:
+- **PUBLIC** - Visible to everyone
+- **PRIVATE** - Visible only to members and group moderators
+- **MODERATED** - Visible to members of specific groups
+
+**WRONG - Privacy leak:**
+```python
+# This exposes ALL groups, including PRIVATE ones
+user.groups.all()
+profile.user.groups.all()
+```
+
+**CORRECT - Respects visibility:**
+```python
+# Use the safe accessor method
+profile.visible_group_profiles(viewer=request.user)
+
+# Or use the manager directly
+GroupProfile.objects.visible(viewer).filter(group__user=some_user)
+```
+
+**Pre-commit protection:** A pre-commit hook checks for `.groups.all()` patterns. To bypass for legitimate internal use (like admin or Django internals), add `# noqa: group-leak` comment.
+
+**Runtime monitoring:** In development, calls to `user.groups.all()` from views/templates will log security warnings.
+
 # Git conventions
 
 ## Git workflow

kitsune/announcements/admin.py

@@ -20,7 +20,7 @@ class AnnouncementAdmin(admin.ModelAdmin):
     search_fields = ["creator__username"]
 
     def get_groups(self, obj):
-        groups = obj.groups.all()
+        groups = obj.groups.all()  # noqa: group-leak
         if groups:
             return ", ".join([g.name for g in groups])
         return ""

kitsune/announcements/tasks.py

@@ -17,7 +17,7 @@ def send_group_email(announcement_id):
     except Announcement.DoesNotExist:
         return
 
-    groups = announcement.groups.all()
+    groups = announcement.groups.all()  # noqa: group-leak
     users = User.objects.filter(groups__in=groups).distinct()
     plain_content = bleach.clean(announcement.content_parsed, tags=[], strip=True).strip()
     email_kwargs = {

kitsune/groups/jinja2/groups/profile.html

@@ -26,23 +26,23 @@ <h1>{{ profile.group.name }}</h1>
       </div>
 
       <div id="group-leaders" class="editable">
-        {% if user_can_manage_leaders %}
+        {% if user_can_moderate %}
           <a class="edit" href="#group-leaders">{{ _('Edit group leaders') }}</a>
         {% endif %}
         <h2>{{ _('Group Leaders') }}</h2>
         <ul class="users leaders">
           {% for user in leaders %}
             <li>
               {{ user_row(user) }}
-              {% if user_can_manage_leaders %}
+              {% if user_can_moderate %}
                 <div class="remove edit-mode">
                   <a href="{{ url('groups.remove_leader', profile.slug, user.id) }}" title="{{ _('Remove user from leaders') }}">&#x2716;</a>
                 </div>
               {% endif %}
             </li>
           {% endfor %}
         </ul>
-        {% if user_can_manage_leaders %}
+        {% if user_can_moderate %}
           <form id="add-member-form" class="edit-mode" action="{{ url('groups.add_leader', profile.slug) }}" method="POST">
             {% csrf_token %}
             {{ errorlist(leader_form) }}

kitsune/groups/managers.py

@@ -0,0 +1,37 @@
+from django.contrib.auth.models import User
+from django.db.models import Exists, OuterRef, Q
+from django.db.models.functions import Length, Substr
+from treebeard.mp_tree import MP_NodeManager
+
+
+class GroupProfileManager(MP_NodeManager):
+    def visible(self, user: User | None = None):
+        """Returns a queryset of all group profiles visible to the given user."""
+        from kitsune.groups.models import GroupProfile
+
+        queryset = self.all()
+
+        if user and user.is_superuser:
+            return queryset
+
+        public = Q(visibility=GroupProfile.Visibility.PUBLIC)
+
+        if not (user and user.is_authenticated):
+            # Anonymous users only see public groups.
+            return queryset.filter(public)
+
+        # Check if the user is a member of the group.
+        member = Q(group__user=user)
+        # Check if the user is a leader of the group or one of its ancestors.
+        leader = Exists(
+            GroupProfile.objects.filter(leaders=user).filter(
+                path=Substr(OuterRef("path"), 1, Length("path"))
+            )
+        )
+        private = Q(visibility=GroupProfile.Visibility.PRIVATE) & (member | leader)
+
+        moderated = Q(visibility=GroupProfile.Visibility.MODERATED) & Q(
+            visible_to_groups__user=user
+        )
+
+        return queryset.filter(public | private | moderated).distinct()

kitsune/groups/models.py

@@ -1,11 +1,13 @@
 from django.conf import settings
 from django.contrib.auth.models import Group, User
 from django.db import models
-from django.db.models import Q
+from django.db.models import Exists, OuterRef, Value
+from django.db.models.functions import Length, Substr
 from django.template.defaultfilters import slugify
 from django.utils.translation import gettext_lazy as _lazy
 from treebeard.mp_tree import MP_Node
 
+from kitsune.groups.managers import GroupProfileManager
 from kitsune.sumo.models import ModelBase
 from kitsune.sumo.urlresolvers import reverse
 from kitsune.wiki.parser import wiki_to_html
@@ -59,6 +61,8 @@ class Visibility(models.TextChoices):
         help_text="Groups that can see this group when visibility is MODERATED",
     )
 
+    objects = GroupProfileManager()
+
     class Meta:
         ordering = ["slug"]
 
@@ -77,46 +81,72 @@ def save(self, *args, **kwargs):
 
         super().save(*args, **kwargs)
 
-    def can_edit(self, user):
-        """Check if user can edit this group.
+    def can_moderate_group(self, user):
+        """
+        Check if user can moderate this group (manage members and leaders).
 
-        If no leaders exist, delegates to parent leaders.
+        Direct leaders have full control. If this group has no leaders,
+        delegate to nearest ancestor with leaders.
         """
-        if user.is_superuser or self.leaders.filter(pk=user.pk).exists():
+        if not (user and user.is_authenticated):
+            return False
+
+        if user.is_superuser:
             return True
-        if not self.leaders.exists():
-            parent = self.get_parent()
-            if parent:
-                return parent.can_edit(user)
-        return False
 
-    def can_manage_members(self, user):
-        """Check if user can add/remove members."""
-        return self.can_edit(user)
+        # Is the user a leader of this group, if it has leaders,
+        # or its nearest ancestor group with leaders, if one exists.
+        return bool(
+            GroupProfile.objects.filter(
+                leaders__isnull=False,
+                path=Substr(Value(self.path), 1, Length("path")),
+            )
+            .order_by("-path")
+            .annotate(
+                is_leader=Exists(
+                    GroupProfile.objects.filter(
+                        pk=OuterRef("pk"),
+                        leaders=user,
+                    )
+                )
+            )
+            .values_list("is_leader", flat=True)
+            .first()
+        )
 
-    def can_manage_leaders(self, user):
-        """Check if user can add/remove leaders."""
-        return user.is_superuser or self.leaders.filter(pk=user.pk).exists()
+    def can_edit(self, user):
+        """Check if user can edit this group profile."""
+        return self.can_moderate_group(user)
 
     def can_view(self, user):
         """Check if user can view this group based on visibility setting."""
+        # PUBLIC groups are visible to everyone
+        if self.visibility == self.Visibility.PUBLIC:
+            return True
+
+        # Non-authenticated users can only see PUBLIC groups
+        if not (user and user.is_authenticated):
+            return False
+
+        # Superusers can see everything
         if user.is_superuser:
             return True
 
+        # Check based on visibility level
         match self.visibility:
-            case self.Visibility.PRIVATE:
-                return self.group.user_set.filter(pk=user.pk).exists()
-            case self.Visibility.PUBLIC:
-                return True
             case self.Visibility.MODERATED:
-                return self.visible_to_groups.filter(pk__in=user.groups.all()).exists()
+                # Visible to members of specified groups
+                return self.visible_to_groups.filter(user=user).exists()
+            case self.Visibility.PRIVATE:
+                # Members can see their own group
+                if self.group.user_set.filter(pk=user.pk).exists():
+                    return True
+                # Moderators (including delegated) can see private groups
+                return self.can_moderate_group(user)
+            case _:
+                return False
 
     def get_visible_children(self, user):
         """Get child groups visible to this user."""
-        if user.is_superuser:
-            return self.get_children()
-
-        public = Q(visibility=self.Visibility.PUBLIC)
-        private = Q(visibility=self.Visibility.PRIVATE) & Q(group__user_set__pk=user.pk)
-        moderated = Q(visibility=self.Visibility.MODERATED) & Q(visible_to_groups__in=user.groups.all())
-        return self.get_children().filter(public | private | moderated)
+        children = self.get_children()
+        return GroupProfile.objects.filter(pk__in=children).visible(user)