Fix access control edge case

When specifying a single required group name as a string, we mistakenly
iterate each character of the string as a valid group name!

If groups were named a certain way, this could have made for an access
issue. Oops. Thankfully, there was never an issue.

Use stricter typing too, which would have caught this.

ws/decorators.py

@@ -1,4 +1,4 @@
-from collections.abc import Callable, Collection
+from collections.abc import Callable
 from functools import wraps
 
 from django.contrib.auth import REDIRECT_FIELD_NAME
@@ -44,7 +44,7 @@ def _wrapped_view(request, *args, **kwargs):
 
 
 def group_required(
-    group_names: str | Collection[str],
+    group_names: str | set[str],
     *,
     # A URL to redirect to after which the user _should_ have permissions
     # Be careful about redirect loops here!
@@ -60,7 +60,7 @@ def group_required(
     A good example of this is user_info_required - participants should be
     given the chance to supply user info and successfully redirect after.
     """
-    allowed_groups = (group_names) if isinstance(group_names, str) else group_names
+    allowed_groups = {group_names} if isinstance(group_names, str) else group_names
 
     def in_groups(user):
         if perm_utils.in_any_group(user, allowed_groups, allow_superusers):

ws/templatetags/perm_tags.py

@@ -21,7 +21,7 @@ def chair_activities(
 @register.filter
 def is_the_wimp(user: User, participant: models.Participant | None) -> bool:
     """Return True if the user has any upcoming WIMP trips."""
-    if perm_utils.in_any_group(user, ["WIMP"], allow_superusers=True):
+    if perm_utils.in_any_group(user, {"WIMP"}, allow_superusers=True):
         return True
     if not participant:
         return False

ws/tests/test_decorators.py

@@ -0,0 +1,42 @@
+from django.contrib.auth.models import Group
+from django.core.exceptions import PermissionDenied
+from django.http import HttpRequest, HttpResponse
+from django.test import RequestFactory, TestCase
+
+from ws import decorators
+from ws.tests import factories
+
+
+class GroupRequiredTest(TestCase):
+    def test_single_group_required(self) -> None:
+        """Reproduce a bug that only happened if groups had name collisions.
+
+        Specifically, we accidentally used substring comparison when we
+        meant to do exact string matching!
+
+        If a view was marked as requiring a *single* group, we accidentally
+        failed to convert that one string to a *collection* of strings, but
+        because Python strings *are* collections of strings... it "worked."
+
+        We never had access issues becaus the few groups we use are unique.
+        """
+        docs = Group.objects.create(name="leaders_with_medical_degrees")
+        # Not a real group, but using this to demonstrate a bug!
+        leaders = Group.objects.get(name="leaders")
+
+        @decorators.group_required("leaders_with_medical_degrees")
+        def mds_only(request: HttpRequest) -> HttpResponse:
+            return HttpResponse()
+
+        participant = factories.ParticipantFactory.create()
+        leaders.user_set.add(participant.user)
+
+        request = RequestFactory().get("/")
+        request.user = participant.user
+        request.participant = participant  # type: ignore[attr-defined]
+
+        with self.assertRaises(PermissionDenied):
+            mds_only(request)
+
+        docs.user_set.add(participant.user)
+        mds_only(request)

ws/tests/utils/test_perms.py

@@ -10,14 +10,14 @@
 
 
 class PermUtilTests(TestCase):
-    def test_chair_group(self):
+    def test_chair_group(self) -> None:
         self.assertEqual(perm_utils.chair_group(enums.Activity.WINTER_SCHOOL), "WSC")
         self.assertEqual(
             perm_utils.chair_group(enums.Activity.CLIMBING), "climbing_chair"
         )
         self.assertEqual(perm_utils.chair_group(enums.Activity.HIKING), "hiking_chair")
 
-    def test_is_chair_no_activity(self):
+    def test_is_chair_no_activity(self) -> None:
         """When activity is None, `is_chair` is always false!"""
         self.assertFalse(perm_utils.is_chair(AnonymousUser(), activity_enum=None))
 
@@ -26,14 +26,14 @@ def test_is_chair_no_activity(self):
         trip = factories.TripFactory.create(program=enums.Program.SERVICE.value)
         self.assertFalse(perm_utils.is_chair(par, trip.required_activity_enum()))
 
-    def test_anonymous_leaders(self):
+    def test_anonymous_leaders(self) -> None:
         """Anonymous users are never leaders, chairs, etc.."""
         anon = AnonymousUser()
         self.assertFalse(perm_utils.is_leader(anon), False)
         self.assertFalse(perm_utils.is_chair(anon, enums.Activity.CLIMBING))
-        self.assertFalse(perm_utils.in_any_group(anon, ["group_name"]))
+        self.assertFalse(perm_utils.in_any_group(anon, {"group_name"}))
 
-    def test_leader_on_trip_creator(self):
+    def test_leader_on_trip_creator(self) -> None:
         trip = TripFactory()
         self.assertTrue(
             perm_utils.leader_on_trip(trip.creator, trip, creator_allowed=True)
@@ -42,13 +42,13 @@ def test_leader_on_trip_creator(self):
             perm_utils.leader_on_trip(trip.creator, trip, creator_allowed=False)
         )
 
-    def test_leader_on_trip(self):
+    def test_leader_on_trip(self) -> None:
         trip = TripFactory()
         self.assertFalse(perm_utils.leader_on_trip(trip.creator, trip))
         trip.leaders.add(trip.creator)
         self.assertTrue(perm_utils.leader_on_trip(trip.creator, trip))
 
-    def test_make_chair(self):
+    def test_make_chair(self) -> None:
         """Users can be promoted to being activity chairs."""
         # To begin with, our user is not a chair (nobody is, for that matter)
         user = UserFactory.create()
@@ -78,7 +78,7 @@ def setUpClass(cls) -> None:
         cls.admin = factories.UserFactory.create(is_superuser=True)
         super().setUpClass()
 
-    def test_activity_chair(self):
+    def test_activity_chair(self) -> None:
         """The admin can be considered an activity chair in some contexts."""
         self.assertTrue(perm_utils.chair_or_admin(self.admin, enums.Activity.HIKING))
         self.assertTrue(perm_utils.is_chair(self.admin, enums.Activity.HIKING))
@@ -93,7 +93,7 @@ def test_activity_chair(self):
             )
         )
 
-    def test_chair_activities(self):
+    def test_chair_activities(self) -> None:
         """The admin qualifies as a chair when allow_superusers is set."""
         # This admin is not a chair in the normal context
         self.assertFalse(
@@ -108,6 +108,6 @@ def test_chair_activities(self):
         # Considered the chair for closed activities
         self.assertCountEqual(admin_allowed_activities, enums.Activity)
 
-    def test_admin_not_counted_in_list(self):
+    def test_admin_not_counted_in_list(self) -> None:
         """The admin isn't considered in the count of chairs."""
         self.assertFalse(perm_utils.activity_chairs(enums.Activity.CLIMBING))

ws/tests/views/test_itinerary.py

@@ -138,7 +138,7 @@ def setUp(self):
         self.client.force_login(self.user)
 
     def test_not_the_wimp(self):
-        self.assertFalse(perm_utils.in_any_group(self.user, ["WSC", "WIMP"]))
+        self.assertFalse(perm_utils.in_any_group(self.user, {"WSC", "WIMP"}))
         response = self.client.get("/trips/medical/")
         self.assertEqual(response.status_code, 403)
 

ws/utils/perms.py

@@ -1,5 +1,4 @@
 import functools
-from collections.abc import Collection
 
 from django.contrib.auth.models import AnonymousUser, Group, User
 from django.db.models import QuerySet
@@ -21,7 +20,7 @@ def is_leader(user: AnonymousUser | User) -> bool:
     Take advantage of the prefetched 'leaders' group for more efficient
     querying of a user's leader status.
     """
-    return in_any_group(user, ["leaders"], allow_superusers=False)
+    return in_any_group(user, {"leaders"}, allow_superusers=False)
 
 
 def leader_on_trip(
@@ -49,7 +48,7 @@ def chair_group(activity_enum: enums.Activity) -> str:
 
 def in_any_group(
     user: AnonymousUser | User,
-    group_names: Collection[str],
+    group_names: set[str],
     allow_superusers: bool = True,
 ) -> bool:
     """Return if the user belongs to any of the passed groups.
@@ -91,7 +90,7 @@ def is_chair(
     """
     if activity_enum is None:  # (e.g. when the required activity is None)
         return False
-    return in_any_group(user, [chair_group(activity_enum)], allow_superusers)
+    return in_any_group(user, {chair_group(activity_enum)}, allow_superusers)
 
 
 def chair_or_admin(

ws/views/itinerary.py

@@ -106,7 +106,7 @@ class TripMedicalView(DetailView):
     def _can_view(trip, request):
         """Leaders, chairs, and a trip WIMP can view this page."""
         return (
-            perm_utils.in_any_group(request.user, ["WIMP"])
+            perm_utils.in_any_group(request.user, {"WIMP"})
             or (trip.wimp and request.participant == trip.wimp)
             or perm_utils.leader_on_trip(request.participant, trip, True)
             or perm_utils.chair_or_admin(request.user, trip.required_activity_enum())