Replace slugged user profile URLs with own-profile views (#3172)

* rm slugged urls

* show only users profile

* no username needed

* rm url for lookup

* no slug

* rm unused queryset searchable

* mythos-red-team: Drop username from membership delete URL

* mythos-red-team: Remove unused profile privacy settings from form

* mythos-red-team: Look up the edited profile by pk

Author: JacobCoffee

apps/users/forms.py

@@ -19,14 +19,7 @@ class Meta:
             "last_name",
             "email",
             "bio",
-            "search_visibility",
-            "email_privacy",
-            "public_profile",
         ]
-        widgets = {
-            "search_visibility": forms.RadioSelect,
-            "email_privacy": forms.RadioSelect,
-        }
 
     def clean_username(self):
         """Validate that the username is unique (case-insensitive)."""

apps/users/managers.py

@@ -5,19 +5,12 @@
 
 
 class UserQuerySet(QuerySet):
-    """QuerySet with convenience filters for active and searchable users."""
+    """QuerySet with convenience filters for active users."""
 
     def active(self):
         """Filter to active users only."""
         return self.filter(is_active=True)
 
-    def searchable(self):
-        """Filter to users who have opted into public search visibility."""
-        return self.active().filter(
-            public_profile=True,
-            search_visibility__exact=self.model.SEARCH_PUBLIC,
-        )
-
 
 class UserManager(DjangoUserManager.from_queryset(UserQuerySet)):
     """Custom user manager with UserQuerySet methods available on the manager."""

apps/users/models.py

@@ -5,7 +5,6 @@
 from django.conf import settings
 from django.contrib.auth.models import AbstractUser
 from django.db import models
-from django.urls import reverse
 from django.utils import timezone
 from markupfield.fields import MarkupField
 from tastypie.models import create_api_key
@@ -51,10 +50,6 @@ class User(AbstractUser):
 
     objects = CustomUserManager()
 
-    def get_absolute_url(self):
-        """Return the URL for the user's profile page."""
-        return reverse("users:user_detail", kwargs={"slug": self.username})
-
     @property
     def has_membership(self):
         """Return True if the user has an associated PSF membership."""

apps/users/templates/users/membership_form.html

@@ -101,7 +101,7 @@ <h1 class="default-title">Register to become a PSF Basic Member</h1>
     </form>
 
     {% if request.user.has_membership %}
-    <form action="{% url 'users:user_membership_delete' user.username %}"
+    <form action="{% url 'users:user_membership_delete' %}"
           method="post"
           class="deletion-form"
           onsubmit="return confirm('Are you sure?');">

apps/users/templates/users/user_form.html

@@ -42,7 +42,7 @@ <h1 class="default-title">Edit information for: <strong>{% firstof user.get_full
             </p>
         </form>
 
-        <form action="{% url 'users:user_delete' user.username %}"
+        <form action="{% url 'users:user_delete' %}"
               method="post"
               class="deletion-form"
               onsubmit="return confirm('Are you sure? There is no way to get back your account.');">

apps/users/tests/test_forms.py

@@ -122,8 +122,6 @@ def test_unique_email(self):
             {
                 "username": "stanne",
                 "email": "test42@example.com",
-                "search_visibility": 0,
-                "email_privacy": 0,
             },
             instance=User.objects.get(username="stanne"),
         )
@@ -138,8 +136,6 @@ def test_case_insensitive_unique_username(self):
             {
                 "username": "Test42",
                 "email": "mikael@darktranquillity.com",
-                "search_visibility": 0,
-                "email_privacy": 0,
             },
             instance=User.objects.get(username="stanne"),
         )

apps/users/tests/test_views.py

@@ -21,15 +21,11 @@ def setUp(self):
             username="username",
             password="password",
             email="niklas@sundin.se",
-            search_visibility=User.SEARCH_PUBLIC,
             membership=None,
         )
         self.user2 = UserFactory(
             username="spameggs",
             password="password",
-            search_visibility=User.SEARCH_PRIVATE,
-            email_privacy=User.EMAIL_PRIVATE,
-            public_profile=False,
         )
 
     def assertUserCreated(self, data=None, template_name="account/verification_sent.html"):  # noqa: N802 - unittest assertion naming convention
@@ -136,90 +132,37 @@ def test_user_update_redirect(self):
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
 
-        # should return 200 if the user does want to see their user profile
+        # should redirect to the profile page after saving
         post_data = {
             "username": "username",
-            "search_visibility": 0,
-            "email_privacy": 1,
-            "public_profile": False,
             "email": "niklas@sundin.se",
             settings.HONEYPOT_FIELD_NAME: settings.HONEYPOT_VALUE,
         }
         response = self.client.post(url, post_data)
-        profile_url = reverse("users:user_detail", kwargs={"slug": "username"})
+        profile_url = reverse("users:user_detail")
         self.assertRedirects(response, profile_url)
 
-        # should return 404 for another user
-        another_user_url = reverse("users:user_detail", kwargs={"slug": "spameggs"})
-        response = self.client.get(another_user_url)
-        self.assertEqual(response.status_code, 404)
-
-        # should return 404 if the user is not logged-in
+        # should redirect to login if the user is not logged-in
         self.client.logout()
         response = self.client.get(profile_url)
-        self.assertEqual(response.status_code, 404)
+        self.assertRedirects(response, "{}?next={}".format(reverse("account_login"), profile_url))
 
     def test_user_detail(self):
-        # Ensure detail page is viewable without login, but that edit URLs
-        # do not appear
-        detail_url = reverse("users:user_detail", kwargs={"slug": self.user.username})
+        # Ensure the detail page shows the logged-in user's own profile
+        detail_url = reverse("users:user_detail")
         edit_url = reverse("users:user_profile_edit")
+        self.client.login(username=self.user2.username, password="password")
         response = self.client.get(detail_url)
-        self.assertTrue(self.user.is_active)
-        self.assertNotContains(response, edit_url)
-
-        # Ensure edit url is available to logged in users
-        self.client.login(username="username", password="password")
-        response = self.client.get(detail_url)
+        self.assertEqual(response.context["object"], self.user2)
         self.assertContains(response, edit_url)
 
-        # Ensure inactive accounts shouldn't be shown to users.
-        user = User.objects.create_user(
-            username="foobar",
-            password="baz",
-            email="paradiselost@example.com",
-        )
-        user.is_active = False
-        user.save()
-        self.assertFalse(user.is_active)
-        detail_url = reverse("users:user_detail", kwargs={"slug": user.username})
-        response = self.client.get(detail_url)
-        self.assertEqual(response.status_code, 404)
-
-    def test_special_usernames(self):
-        # Ensure usernames in the forms of:
-        # first.last
-        # user@host.com
-        # are allowed to view their profile pages since we allow them in
-        # the username field
-        u1 = User.objects.create_user(
-            username="user.name",
-            password="password",
-        )
-        detail_url = reverse("users:user_detail", kwargs={"slug": u1.username})
-        edit_url = reverse("users:user_profile_edit")
-
-        self.client.login(username=u1.username, password="password")
-        response = self.client.get(detail_url)
-        self.assertEqual(response.status_code, 200)
-
-        response = self.client.get(edit_url)
-        self.assertEqual(response.status_code, 200)
-
-        u2 = User.objects.create_user(
-            username="user@example.com",
-            password="password",
-        )
-
-        detail_url = reverse("users:user_detail", kwargs={"slug": u2.username})
-        edit_url = reverse("users:user_profile_edit")
-
-        self.client.login(username=u2.username, password="password")
-        response = self.client.get(detail_url)
-        self.assertEqual(response.status_code, 200)
-
-        response = self.client.get(edit_url)
-        self.assertEqual(response.status_code, 200)
+    def test_user_detail_not_addressable_by_username(self):
+        # Profiles used to live at /users/<username>/; existing and unknown
+        # usernames must be indistinguishable to prevent user enumeration.
+        existing = self.client.get(f"/users/{self.user.username}/")
+        unknown = self.client.get("/users/thisusernamedoesntexist/")
+        self.assertEqual(existing.status_code, 404)
+        self.assertEqual(unknown.status_code, 404)
 
     def test_user_new_account(self):
         self.assertUserCreated(
@@ -298,62 +241,61 @@ def test_is_active_login(self):
         self.assertRedirects(response, "{}?next={}".format(reverse("account_login"), url))
 
     def test_user_delete_needs_to_be_logged_in(self):
-        url = reverse("users:user_delete", kwargs={"slug": self.user.username})
+        url = reverse("users:user_delete")
         response = self.client.delete(url)
         self.assertRedirects(response, "{}?next={}".format(reverse("account_login"), url))
 
     def test_user_delete_invalid_request_method(self):
-        url = reverse("users:user_delete", kwargs={"slug": self.user.username})
+        url = reverse("users:user_delete")
         self.client.login(username=self.user.username, password="password")
         response = self.client.get(url)
         self.assertEqual(response.status_code, 405)
 
-    def test_user_delete_different_user(self):
-        url = reverse("users:user_delete", kwargs={"slug": self.user.username})
-        self.client.login(username=self.user2.username, password="password")
-        response = self.client.delete(url)
-        self.assertEqual(response.status_code, 403)
-
     def test_user_delete(self):
-        url = reverse("users:user_delete", kwargs={"slug": self.user.username})
+        url = reverse("users:user_delete")
         self.client.login(username=self.user.username, password="password")
         response = self.client.delete(url)
         self.assertRedirects(response, reverse("home"))
         self.assertRaises(User.DoesNotExist, User.objects.get, username=self.user.username)
         self.assertRaises(Membership.DoesNotExist, Membership.objects.get, creator=self.user)
+        self.assertTrue(User.objects.filter(username=self.user2.username).exists())
 
     def test_membership_delete_needs_to_be_logged_in(self):
-        url = reverse("users:user_membership_delete", kwargs={"slug": self.user2.username})
+        url = reverse("users:user_membership_delete")
         response = self.client.delete(url)
         self.assertRedirects(response, "{}?next={}".format(reverse("account_login"), url))
 
     def test_membership_delete_invalid_request_method(self):
-        url = reverse("users:user_membership_delete", kwargs={"slug": self.user2.username})
+        url = reverse("users:user_membership_delete")
         self.client.login(username=self.user2.username, password="password")
         response = self.client.get(url)
         self.assertEqual(response.status_code, 405)
 
-    def test_membership_delete_different_user_membership(self):
-        user = UserFactory()
-        self.assertTrue(user.has_membership)
-        url = reverse("users:user_membership_delete", kwargs={"slug": user.username})
+    def test_membership_delete_not_addressable_by_username(self):
+        # Membership deletion used to live at /users/membership/delete/<username>/
+        # and answered 403 for members and 404 for everyone else.
+        member = UserFactory()
+        self.assertTrue(member.has_membership)
         self.client.login(username=self.user2.username, password="password")
-        response = self.client.delete(url)
-        self.assertEqual(response.status_code, 403)
+        existing = self.client.delete(f"/users/membership/delete/{member.username}/")
+        unknown = self.client.delete("/users/membership/delete/thisusernamedoesntexist/")
+        self.assertEqual(existing.status_code, 404)
+        self.assertEqual(unknown.status_code, 404)
+        self.assertTrue(Membership.objects.filter(creator=member).exists())
 
     def test_membership_does_not_exist(self):
         self.assertFalse(self.user.has_membership)
-        url = reverse("users:user_membership_delete", kwargs={"slug": self.user.username})
+        url = reverse("users:user_membership_delete")
         self.client.login(username=self.user.username, password="password")
         response = self.client.delete(url)
         self.assertEqual(response.status_code, 404)
 
     def test_membership_delete(self):
         self.assertTrue(self.user2.has_membership)
-        url = reverse("users:user_membership_delete", kwargs={"slug": self.user2.username})
+        url = reverse("users:user_membership_delete")
         self.client.login(username=self.user2.username, password="password")
         response = self.client.delete(url)
-        self.assertRedirects(response, reverse("users:user_detail", kwargs={"slug": self.user2.username}))
+        self.assertRedirects(response, reverse("users:user_detail"))
         # TODO: We can't use 'self.user2.refresh_from_db()' because
         # of https://code.djangoproject.com/ticket/27846.
         with self.assertRaises(Membership.DoesNotExist):

apps/users/urls.py

@@ -1,19 +1,17 @@
 """URL configuration for user profiles, memberships, and sponsorship management."""
 
-from django.urls import path, re_path
+from django.urls import path
 
 from apps.users import views
 
 app_name = "users"
 urlpatterns = [
     path("edit/", views.UserUpdate.as_view(), name="user_profile_edit"),
+    path("profile/detail/", views.UserDetail.as_view(), name="user_detail"),
+    path("profile/delete/", views.UserDeleteView.as_view(), name="user_delete"),
     path("membership/", views.MembershipCreate.as_view(), name="user_membership_create"),
     path("membership/edit/", views.MembershipUpdate.as_view(), name="user_membership_edit"),
-    re_path(
-        r"^membership/delete/(?P<slug>[-a-zA-Z0-9_\@\.+]+)/$",
-        views.MembershipDeleteView.as_view(),
-        name="user_membership_delete",
-    ),
+    path("membership/delete/", views.MembershipDeleteView.as_view(), name="user_membership_delete"),
     path("membership/thanks/", views.MembershipThanks.as_view(), name="user_membership_thanks"),
     path("membership/affirm/", views.MembershipVoteAffirm.as_view(), name="membership_affirm_vote"),
     path("membership/affirm/done/", views.MembershipVoteAffirmDone.as_view(), name="membership_affirm_vote_done"),
@@ -44,6 +42,4 @@
         views.SponsorshipDetailView.as_view(),
         name="sponsorship_application_detail",
     ),
-    re_path(r"^(?P<slug>[-a-zA-Z0-9_\@\.+]+)/delete/$", views.UserDeleteView.as_view(), name="user_delete"),
-    re_path(r"^(?P<slug>[-a-zA-Z0-9_\@\.+]+)/$", views.UserDetail.as_view(), name="user_detail"),
 ]