diff --git a/apps/users/forms.py b/apps/users/forms.py index c84142c79..670c332b4 100644 --- a/apps/users/forms.py +++ b/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).""" diff --git a/apps/users/managers.py b/apps/users/managers.py index 5c7d8bfd9..37a86b8f9 100644 --- a/apps/users/managers.py +++ b/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.""" diff --git a/apps/users/models.py b/apps/users/models.py index b2eda9c6e..413c27e5e 100644 --- a/apps/users/models.py +++ b/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.""" diff --git a/apps/users/templates/users/membership_form.html b/apps/users/templates/users/membership_form.html index 7c65589b3..998cf8a2d 100644 --- a/apps/users/templates/users/membership_form.html +++ b/apps/users/templates/users/membership_form.html @@ -101,7 +101,7 @@

Register to become a PSF Basic Member

{% if request.user.has_membership %} -
diff --git a/apps/users/templates/users/user_form.html b/apps/users/templates/users/user_form.html index 0ccf7a900..3eab0c8d6 100644 --- a/apps/users/templates/users/user_form.html +++ b/apps/users/templates/users/user_form.html @@ -42,7 +42,7 @@

Edit information for: {% firstof user.get_full

-
diff --git a/apps/users/tests/test_forms.py b/apps/users/tests/test_forms.py index 92332a1df..c097bd078 100644 --- a/apps/users/tests/test_forms.py +++ b/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"), ) diff --git a/apps/users/tests/test_views.py b/apps/users/tests/test_views.py index 1c8109053..abf73781a 100644 --- a/apps/users/tests/test_views.py +++ b/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//; 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// + # 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): diff --git a/apps/users/urls.py b/apps/users/urls.py index 110090ff4..4173caf1a 100644 --- a/apps/users/urls.py +++ b/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[-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[-a-zA-Z0-9_\@\.+]+)/delete/$", views.UserDeleteView.as_view(), name="user_delete"), - re_path(r"^(?P[-a-zA-Z0-9_\@\.+]+)/$", views.UserDetail.as_view(), name="user_detail"), ] diff --git a/apps/users/views.py b/apps/users/views.py index 9a3264d2e..de37c356e 100644 --- a/apps/users/views.py +++ b/apps/users/views.py @@ -7,7 +7,6 @@ from django.contrib import messages from django.contrib.auth import get_user_model from django.contrib.auth.decorators import login_required -from django.contrib.auth.mixins import UserPassesTestMixin from django.core.mail import send_mail from django.db.models import Subquery from django.http import Http404 @@ -127,8 +126,8 @@ class UserUpdate(LoginRequiredMixin, UpdateView): """Edit the current user's profile information.""" form_class = UserProfileForm - slug_field = "username" template_name = "users/user_form.html" + success_url = reverse_lazy("users:user_detail") @method_decorator(check_honeypot) def dispatch(self, *args, **kwargs): @@ -136,21 +135,18 @@ def dispatch(self, *args, **kwargs): return super().dispatch(*args, **kwargs) def get_object(self, queryset=None): - """Return the current logged-in user.""" - return User.objects.get(username=self.request.user) + """Return a fresh copy of the logged-in user, so invalid input never mutates request.user.""" + return User.objects.get(pk=self.request.user.pk) -class UserDetail(DetailView): - """Display a user's public profile page.""" +class UserDetail(LoginRequiredMixin, DetailView): + """Display the logged-in user's own profile details.""" - slug_field = "username" + template_name = "users/user_detail.html" - def get_queryset(self): - """Return all users if viewing own profile, searchable users otherwise.""" - queryset = User.objects.select_related() - if self.request.user.username == self.kwargs["slug"]: - return queryset - return queryset.searchable() + def get_object(self, queryset=None): + """Return the current logged-in user.""" + return self.request.user class HoneypotSignupView(SignupView): @@ -178,35 +174,31 @@ def get_success_url(self): return reverse("users:user_profile_edit") -class UserDeleteView(LoginRequiredMixin, UserPassesTestMixin, DeleteView): +class UserDeleteView(LoginRequiredMixin, DeleteView): """Allow users to delete their own account.""" - model = User success_url = reverse_lazy("home") - slug_field = "username" - raise_exception = True http_method_names = ["post", "delete"] - def test_func(self): - """Only allow users to delete their own account.""" - return self.get_object() == self.request.user + def get_object(self, queryset=None): + """Return the current logged-in user.""" + return self.request.user -class MembershipDeleteView(LoginRequiredMixin, UserPassesTestMixin, DeleteView): +class MembershipDeleteView(LoginRequiredMixin, DeleteView): """Allow users to delete their own PSF membership.""" - model = Membership - slug_field = "creator__username" - raise_exception = True http_method_names = ["post", "delete"] def get_success_url(self): """Redirect to the user's profile page after deletion.""" - return reverse("users:user_detail", kwargs={"slug": self.request.user.username}) + return reverse("users:user_detail") - def test_func(self): - """Only allow the membership creator to delete it.""" - return self.get_object().creator == self.request.user + def get_object(self, queryset=None): + """Return the current user's membership or raise 404.""" + if self.request.user.has_membership: + return self.request.user.membership + raise Http404 class UserNominationsView(LoginRequiredMixin, TemplateView): diff --git a/pydotorg/context_processors.py b/pydotorg/context_processors.py index d9a74637d..658e77cb2 100644 --- a/pydotorg/context_processors.py +++ b/pydotorg/context_processors.py @@ -51,7 +51,7 @@ def user_nav_bar_links(request): "account": { "label": "Your Account", "urls": [ - {"url": reverse("users:user_detail", args=[user.username]), "label": "View profile"}, + {"url": reverse("users:user_detail"), "label": "View profile"}, {"url": reverse("users:user_profile_edit"), "label": "Edit profile"}, {"url": reverse("account_change_password"), "label": "Change password"}, ], diff --git a/pydotorg/tests/test_context_processors.py b/pydotorg/tests/test_context_processors.py index 0b941e12c..4d7ac9b84 100644 --- a/pydotorg/tests/test_context_processors.py +++ b/pydotorg/tests/test_context_processors.py @@ -41,7 +41,7 @@ def test_user_nav_bar_links_for_non_psf_members(self): "account": { "label": "Your Account", "urls": [ - {"url": reverse("users:user_detail", args=["foo"]), "label": "View profile"}, + {"url": reverse("users:user_detail"), "label": "View profile"}, {"url": reverse("users:user_profile_edit"), "label": "Edit profile"}, {"url": reverse("account_change_password"), "label": "Change password"}, ], @@ -70,7 +70,7 @@ def test_user_nav_bar_links_for_psf_members(self): "account": { "label": "Your Account", "urls": [ - {"url": reverse("users:user_detail", args=["foo"]), "label": "View profile"}, + {"url": reverse("users:user_detail"), "label": "View profile"}, {"url": reverse("users:user_profile_edit"), "label": "Edit profile"}, {"url": reverse("account_change_password"), "label": "Change password"}, ], diff --git a/templates/includes/authenticated.html b/templates/includes/authenticated.html index 457dc13a7..d453c5900 100644 --- a/templates/includes/authenticated.html +++ b/templates/includes/authenticated.html @@ -2,7 +2,7 @@