Skip to content

Commit bf3620c

Browse files
Maffoochclaude
andcommitted
fix: address review feedback on OS message opt-out
- Renumber migration 0270 -> 0275 (depends on 0274_finding_sla_expiration_index) to resolve the leaf-node collision with the finding perf-index migrations that landed on dev. - Make the dismiss_os_message write concurrency-safe: read-modify-write user_state_details under an atomic() + select_for_update() row lock so a concurrent write to a different key can't clobber it (lost update). - Exclude the internal user_state_details JSON blob from UserContactInfoSerializer so it stays out of the public API. - Add a regression test asserting dismissal preserves other keys in user_state_details. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 258a1a1 commit bf3620c

4 files changed

Lines changed: 27 additions & 8 deletions

File tree

dojo/announcement/ui/views.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import re
33

44
from django.contrib import messages
5+
from django.db import transaction
56
from django.http import HttpResponse, HttpResponseForbidden, HttpResponseRedirect
67
from django.shortcuts import render
78
from django.urls import reverse
@@ -98,12 +99,17 @@ def dismiss_os_message(request):
9899
return HttpResponseForbidden()
99100
token = request.POST.get("token", "").strip()
100101
if token and re.fullmatch(r"[0-9a-f]{1,64}", token):
101-
contact = UserContactInfo.objects.get_or_create(user=request.user)[0]
102-
state = contact.user_state_details or {}
103-
if state.get(OS_MESSAGE_DISMISSED_KEY) != token:
104-
state[OS_MESSAGE_DISMISSED_KEY] = token
105-
contact.user_state_details = state
106-
contact.save(update_fields=["user_state_details"])
102+
# user_state_details is a shared JSON blob for many per-user flags, so
103+
# read-modify-write it under a row lock to avoid clobbering a concurrent
104+
# write to a different key (lost update).
105+
with transaction.atomic():
106+
contact, _ = UserContactInfo.objects.get_or_create(user=request.user)
107+
contact = UserContactInfo.objects.select_for_update().get(pk=contact.pk)
108+
state = contact.user_state_details or {}
109+
if state.get(OS_MESSAGE_DISMISSED_KEY) != token:
110+
state[OS_MESSAGE_DISMISSED_KEY] = token
111+
contact.user_state_details = state
112+
contact.save(update_fields=["user_state_details"])
107113
if request.headers.get("x-requested-with") == "XMLHttpRequest":
108114
return HttpResponse(status=204)
109115
referer = request.META.get("HTTP_REFERER")

dojo/db_migrations/0270_usercontactinfo_user_state_details.py renamed to dojo/db_migrations/0275_usercontactinfo_user_state_details.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
class Migration(migrations.Migration):
77

88
dependencies = [
9-
("dojo", "0269_normalize_blank_finding_components"),
9+
("dojo", "0274_finding_sla_expiration_index"),
1010
]
1111

1212
operations = [

dojo/user/api/serializer.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,9 @@ class UserContactInfoSerializer(serializers.ModelSerializer):
169169

170170
class Meta:
171171
model = UserContactInfo
172-
fields = "__all__"
172+
# user_state_details is an internal JSON blob for UI state (dismissed
173+
# banners, "don't show again" flags); keep it out of the public API.
174+
exclude = ("user_state_details",)
173175

174176
def validate(self, data):
175177
user = data.get("user", None) or self.instance.user

unittests/test_os_message.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,17 @@ def test_post_persists_hash_on_usercontactinfo(self):
333333
contact = UserContactInfo.objects.get(user=self.user)
334334
self.assertEqual(contact.user_state_details.get(os_message.OS_MESSAGE_DISMISSED_KEY), "abc123def456")
335335

336+
def test_dismiss_preserves_other_state_keys(self):
337+
"""Dismissing must not clobber unrelated keys in the shared user_state_details blob."""
338+
contact = UserContactInfo.objects.get_or_create(user=self.user)[0]
339+
contact.user_state_details = {"other_flag": 1}
340+
contact.save(update_fields=["user_state_details"])
341+
response = self.client.post(self.url, {"token": "abc123def456"}, HTTP_X_REQUESTED_WITH="XMLHttpRequest")
342+
self.assertEqual(response.status_code, 204)
343+
contact.refresh_from_db()
344+
self.assertEqual(contact.user_state_details.get("other_flag"), 1)
345+
self.assertEqual(contact.user_state_details.get(os_message.OS_MESSAGE_DISMISSED_KEY), "abc123def456")
346+
336347
def test_get_not_allowed(self):
337348
self.assertEqual(self.client.get(self.url).status_code, 405)
338349

0 commit comments

Comments
 (0)