Fix permission-related display issues after club deactivation (#715)

* Add test (should fail currently)

* Use in memory cache for CI tests

* Move permission checking out of serializers

* Clear cache between tests

* Move permissioning back to serializer

* Improve docs

* Minor style changes

* Revert changes to get_serializer_class

* Minor improvements to tests

backend/clubs/views.py

@@ -1151,6 +1151,18 @@ def get_queryset(self):
         else:
             return queryset.filter(Q(approved=True) | Q(ghost=True))
 
+    def _has_elevated_view_perms(self, instance):
+        """
+        Determine if the current user has elevated view privileges.
+        """
+        see_pending = self.request.user.has_perm("clubs.see_pending_clubs")
+        manage_club = self.request.user.has_perm("clubs.manage_club")
+        is_member = (
+            self.request.user.is_authenticated
+            and instance.membership_set.filter(person=self.request.user).exists()
+        )
+        return see_pending or manage_club or is_member
+
     @action(detail=True, methods=["post"])
     def upload(self, request, *args, **kwargs):
         """
@@ -2002,9 +2014,17 @@ def list(self, *args, **kwargs):
 
     def retrieve(self, *args, **kwargs):
         """
-        Retrieve data about a specific club. Responses cached for 1 hour
+        Retrieve data about a specific club. Responses cached for 1 hour. Caching is
+        disabled for users with elevated view perms so that changes without approval
+        granted don't spill over to public.
         """
-        key = f"clubs:{self.get_object().id}"
+        club = self.get_object()
+
+        # don't cache if user has elevated view perms
+        if self._has_elevated_view_perms(club):
+            return super().retrieve(*args, **kwargs)
+
+        key = f"clubs:{club.id}"
         cached = cache.get(key)
         if cached:
             return Response(cached)

backend/pennclubs/settings/ci.py

@@ -11,7 +11,7 @@
 TEST_OUTPUT_DIR = "test-results"
 
 # Use dummy cache for testing
-CACHES = {"default": {"BACKEND": "django.core.cache.backends.dummy.DummyCache"}}
+CACHES = {"default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"}}
 
 del PLATFORM_ACCOUNTS["REDIRECT_URI"]
 

backend/tests/clubs/test_views.py

@@ -167,6 +167,8 @@ def setUpTestData(cls):
         Tag.objects.create(name="Undergraduate")
 
     def setUp(self):
+        cache.clear()  # clear the cache between tests
+
         self.client = Client()
 
         self.club1 = Club.objects.create(
@@ -1065,6 +1067,58 @@ def test_club_approve(self):
         self.assertIsNotNone(self.club1.approved_on)
         self.assertIsNotNone(self.club1.approved_by)
 
+    def test_club_display_after_deactivation_for_permissioned_vs_non_permissioned(self):
+        """
+        Test club retrieval after deactivation script runs. Non-permissioned users
+        should see the last approved version of the club. Permissioned users (e.g.
+        admins, club members) should see the most up-to-date version.
+        """
+        # club is approved before deactivation
+        self.assertTrue(self.club1.approved)
+
+        call_command("deactivate", "all", "--force")
+
+        club = self.club1
+        club.refresh_from_db()
+
+        # after deactivation, club should not be approved and should not have approver
+        self.assertIsNone(club.approved)
+        self.assertIsNone(club.approved_by)
+
+        # non-permissioned users should see the last approved version
+        non_admin_resp = self.client.get(reverse("clubs-detail", args=(club.code,)))
+        self.assertEqual(non_admin_resp.status_code, 200)
+        non_admin_data = non_admin_resp.json()
+        self.assertTrue(non_admin_data["approved"])
+
+        # permissioned users should see the club as it is in the DB
+        self.client.login(username=self.user5.username, password="test")
+        admin_resp = self.client.get(reverse("clubs-detail", args=(club.code,)))
+        self.assertEqual(admin_resp.status_code, 200)
+        admin_data = admin_resp.json()
+        self.assertIsNone(admin_data["approved"])
+        self.client.logout()
+
+        cache.clear()
+
+        # reversing the order of operations shouldn't change anything
+        self.client.login(username=self.user5.username, password="test")
+        admin_resp = self.client.get(reverse("clubs-detail", args=(club.code,)))
+        self.assertEqual(admin_resp.status_code, 200)
+        admin_data = admin_resp.json()
+        self.assertIsNone(admin_data["approved"])
+        self.client.logout()
+
+        non_admin_resp = self.client.get(reverse("clubs-detail", args=(club.code,)))
+        self.assertEqual(non_admin_resp.status_code, 200)
+        non_admin_data = non_admin_resp.json()
+        self.assertTrue(non_admin_data["approved"])
+
+        # club object itself shouldn't have changed
+        club.refresh_from_db()
+        self.assertFalse(club.active)
+        self.assertIsNone(club.approved)
+
     def test_club_create_url_sanitize(self):
         """
         Test creating clubs with malicious URLs.