Skip to content

Commit c294172

Browse files
committed
fix(user): use forward FK to look up Global_Role on profile / user / group save
Global_Role.user and Global_Role.group both use related_name="+" under legacy authorization, so user.global_role / group.global_role are not reverse accessors. The hasattr() guard always returned False, so the profile / edit-user / edit-group save paths bound the form to a brand- new Global_Role with no PK, then save() INSERTed a duplicate row, violating the unique(user_id) / unique(group_id) constraint and 500'ing the request — surfaced when a user tried to toggle ui_use_tailwind on their own profile. Look up the existing Global_Role via the forward FK (Global_Role.objects.filter(user=...).first() and the group equivalent) so save() UPDATEs the existing row instead. Same fix applies to the delete_user authorization check that gates on "is the target user a global-role holder?" — switched from the broken hasattr to the forward-FK lookup. Regression test in unittests/test_user_ui_timestamps.py seeds an existing Global_Role for admin, POSTs /profile, and asserts no 500 + no duplicate row.
1 parent 8ebc439 commit c294172

3 files changed

Lines changed: 44 additions & 6 deletions

File tree

dojo/group/views.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,10 @@ def get_group(self, group_id: int):
134134
return get_object_or_404(Dojo_Group, id=group_id)
135135

136136
def get_global_role(self, group: Dojo_Group):
137-
# Try to pull the global role from the group object
138-
return group.global_role if hasattr(group, "global_role") else None
137+
# Global_Role.group uses related_name="+", so group.global_role is
138+
# not a reverse accessor — look up via the forward FK so we update
139+
# the existing row instead of inserting a duplicate.
140+
return Global_Role.objects.filter(group=group).first()
139141

140142
def get_group_form(self, request: HttpRequest, group: Dojo_Group):
141143
# Set up the args for the form

dojo/user/views.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
from rest_framework.exceptions import ValidationError as RFValidationError
3131

3232
from dojo.authorization.authorization import user_is_superuser_or_global_owner
33-
from dojo.authorization.models import Dojo_Group_Member, Product_Member, Product_Type_Member
33+
from dojo.authorization.models import Dojo_Group_Member, Global_Role, Product_Member, Product_Type_Member
3434
from dojo.decorators import dojo_ratelimit
3535
from dojo.filters import UserFilter
3636
from dojo.forms import (
@@ -257,7 +257,10 @@ def view_profile(request):
257257
user_contact = user.usercontactinfo if hasattr(user, "usercontactinfo") else None
258258
contact_form = UserContactInfoForm(user=user) if user_contact is None else UserContactInfoForm(instance=user_contact, user=user)
259259

260-
global_role = user.global_role if hasattr(user, "global_role") else None
260+
# Global_Role.user uses related_name="+", so user.global_role is not a
261+
# reverse accessor — look it up via the forward FK so we update the
262+
# existing row instead of trying to insert a duplicate.
263+
global_role = Global_Role.objects.filter(user=user).first()
261264
if global_role is None:
262265
previous_global_role = None
263266
global_role_form = GlobalRoleForm()
@@ -422,7 +425,9 @@ def edit_user(request, uid):
422425
user_contact = user.usercontactinfo if hasattr(user, "usercontactinfo") else None
423426
contact_form = UserContactInfoForm(user=user) if user_contact is None else UserContactInfoForm(instance=user_contact, user=user)
424427

425-
global_role = user.global_role if hasattr(user, "global_role") else None
428+
# Look up Global_Role via the forward FK; user.global_role is not a
429+
# reverse accessor (related_name="+").
430+
global_role = Global_Role.objects.filter(user=user).first()
426431
global_role_form = GlobalRoleForm() if global_role is None else GlobalRoleForm(instance=global_role)
427432

428433
if request.method == "POST":
@@ -521,7 +526,9 @@ def delete_user(request, uid):
521526
messages.ERROR,
522527
_("Only superusers are allowed to delete superusers. User was not removed."),
523528
extra_tags="alert-danger")
524-
elif not request.user.is_superuser and hasattr(user, "global_role") and user.global_role.role:
529+
elif (not request.user.is_superuser
530+
and (existing_global_role := Global_Role.objects.filter(user=user).first())
531+
and existing_global_role.role):
525532
messages.add_message(request,
526533
messages.ERROR,
527534
_("Only superusers are allowed to delete users with a global role. User was not removed."),

unittests/test_user_ui_timestamps.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,3 +102,32 @@ def test_user_list_page_renders(self):
102102
self.client.force_login(admin)
103103
resp = self.client.get(reverse("users"))
104104
self.assertEqual(resp.status_code, 200)
105+
106+
def test_profile_save_does_not_duplicate_global_role(self):
107+
# Regression: /profile POST previously did
108+
# global_role = user.global_role if hasattr(user, "global_role") else None
109+
# Under legacy authorization Global_Role.user uses related_name="+"
110+
# so the hasattr always returned False, the form bound to a fresh
111+
# Global_Role with no PK, and global_role.save() INSERTed a second
112+
# row that violated the unique(user_id) constraint and 500'd. The
113+
# forward-FK lookup must find the existing row and UPDATE it.
114+
from dojo.authorization.models import Global_Role, Role
115+
116+
admin = Dojo_User.objects.get(username="admin")
117+
# Seed an existing Global_Role row for this user (mirrors a Pro
118+
# snapshot or a stale legacy backfill).
119+
owner_role = Role.objects.filter(name="Owner").first()
120+
Global_Role.objects.update_or_create(user=admin, defaults={"role": owner_role})
121+
122+
self.client.force_login(admin)
123+
resp = self.client.post(reverse("view_profile"), data={
124+
"username": admin.username,
125+
"first_name": admin.first_name,
126+
"last_name": admin.last_name,
127+
"email": admin.email,
128+
"role": owner_role.id if owner_role else "",
129+
})
130+
# Must not 500. Either the form bounces back (200) or saves (302).
131+
self.assertIn(resp.status_code, (200, 302))
132+
# And there must still be exactly one Global_Role row for admin.
133+
self.assertEqual(Global_Role.objects.filter(user=admin).count(), 1)

0 commit comments

Comments
 (0)