diff --git a/docker/configs/settings_local.py b/docker/configs/settings_local.py index 94adc516a47..227da0a0ace 100644 --- a/docker/configs/settings_local.py +++ b/docker/configs/settings_local.py @@ -115,6 +115,8 @@ APP_API_TOKENS = { "ietf.api.red_api" : ["devtoken", "redtoken"], # Not a real secret "ietf.api.views_rpc" : ["devtoken"], # Not a real secret + "ietf.person.api_uuid" : ["devtoken"], # Not a real secret + "ietf.person.api_uuid_by_pk" : ["devtoken"], # Not a real secret } # Errata system api configuration diff --git a/ietf/api/urls.py b/ietf/api/urls.py index d2dc774efb6..8e843d720ff 100644 --- a/ietf/api/urls.py +++ b/ietf/api/urls.py @@ -9,6 +9,7 @@ from ietf import api from ietf.doc import views_ballot, api as doc_api from ietf.meeting import views as meeting_views +from ietf.person import api_uuid as person_uuid_api from ietf.submit import views as submit_views from ietf.utils.urls import url @@ -21,6 +22,14 @@ # core_router.register("email", person_api.EmailViewSet) # core_router.register("person", person_api.PersonViewSet) +# Person identity API router +person_router = PrefixedSimpleRouter( + use_regex_path=False, name_prefix="ietf.api.person_api" +) +person_router.register( + "uuid", person_uuid_api.PersonUUIDViewSet, basename="person-uuid" +) + # todo more general name for this API? red_router = PrefixedSimpleRouter(name_prefix="ietf.api.red_api") # red api router red_router.register("doc", doc_api.RfcViewSet) @@ -88,6 +97,16 @@ url(r'^person/email/$', api_views.active_email_list), # Related Email listing url(r'^person/email/(?P[^/\x00]+)/related/$', api_views.related_email_list), + # Transitional pk-to-UUID conversion. Before the router include below so it wins + # over the router's uuid/ routes. + path( + "person/uuid/by-person-pk/", + person_uuid_api.PersonUUIDByPersonPkView.as_view(), + name="ietf.api.person_api.person-uuid-by-pk", + ), + # Person UUID resolution API. After the ^person/email/ routes above so those keep + # matching first. + path("person/", include(person_router.urls)), # Draft submission API url(r'^submit/?$', submit_views.api_submit_tombstone), # Draft upload API diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index a77e5bd5d58..9b793bf6ccb 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -34,7 +34,13 @@ from ietf.ietfauth.utils import has_role from ietf.meeting.factories import MeetingFactory, RegistrationFactory, RegistrationTicketFactory from ietf.nomcom.factories import NomComFactory -from ietf.person.factories import PersonFactory, EmailFactory, UserFactory, PersonalApiKeyFactory +from ietf.person.factories import ( + PersonFactory, + EmailFactory, + UserFactory, + PersonalApiKeyFactory, + PersonUUIDFactory, +) from ietf.person.models import Person, Email from ietf.person.tasks import send_apikey_usage_emails_task from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory @@ -657,7 +663,9 @@ def test_change_password(self): ) user.set_password(VALID_PASSWORD) user.save() - p = Person.objects.create(name="Some One", ascii="Some One", user=user) + p = PersonFactory( + user=user, name="Some One", ascii="Some One", default_emails=False + ) Email.objects.create(address=user.username, person=p, origin=user.username) # log in @@ -758,7 +766,9 @@ def test_change_username(self): ) user.set_password(VALID_PASSWORD) user.save() - p = Person.objects.create(name="Some One", ascii="Some One", user=user) + p = PersonFactory( + user=user, name="Some One", ascii="Some One", default_emails=False + ) Email.objects.create(address=user.username, person=p, origin=user.username) Email.objects.create( address="othername@example.org", person=p, origin=user.username @@ -1162,7 +1172,16 @@ def test_oidc_code_auth(self): session["nonce"] = rndstr() args = { "response_type": "code", - "scope": ['openid', 'profile', 'email', 'roles', 'registration', 'dots', 'pronouns' ], + "scope": [ + "openid", + "profile", + "email", + "roles", + "registration", + "dots", + "pronouns", + "datatracker_uuid", + ], "nonce": session["nonce"], "redirect_uri": redirect_uris[0], "state": session["state"] @@ -1207,6 +1226,10 @@ def test_oidc_code_auth(self): self.assertIn(key, access_token_info) for key in ['iss', 'sub', 'aud', 'exp', 'iat', 'auth_time', 'nonce', 'at_hash']: self.assertIn(key, access_token_info['id_token']) + # Custom claims are served from userinfo, not the id_token. This guards + # against an accidental OIDC_IDTOKEN_INCLUDE_CLAIMS flip. + for key in ["datatracker_uuid", "datatracker_prior_uuids"]: + self.assertNotIn(key, access_token_info["id_token"]) # Get userinfo, check keys present, most common scenario userinfo = client.do_user_info_request(state=params["state"], scope=args['scope']) @@ -1218,6 +1241,18 @@ def test_oidc_code_auth(self): self.assertNotIn('hackathon_onsite', set(userinfo['reg_type'].split())) self.assertIn(active_group.acronym, [i[1] for i in userinfo['roles']]) self.assertNotIn(closed_group.acronym, [i[1] for i in userinfo['roles']]) + self.assertEqual(userinfo['datatracker_uuid'], str(person.primary_uuid)) + # Present and empty, not absent, for a Person that has never been merged + self.assertIn("datatracker_prior_uuids", userinfo) + self.assertEqual(userinfo["datatracker_prior_uuids"], []) + + # A UUID absorbed by a merge shows up in the prior list + absorbed = PersonUUIDFactory(person=person) + userinfo = client.do_user_info_request( + state=params["state"], scope=args["scope"] + ) + self.assertEqual(userinfo["datatracker_uuid"], str(person.primary_uuid)) + self.assertEqual(userinfo["datatracker_prior_uuids"], [str(absorbed.uuid)]) # Create a registration, with only email, no person (rare if at all) reg_person.delete() diff --git a/ietf/ietfauth/utils.py b/ietf/ietfauth/utils.py index 30d51cddd09..fcce43dc2dc 100644 --- a/ietf/ietfauth/utils.py +++ b/ietf/ietfauth/utils.py @@ -341,6 +341,29 @@ def scope_dots(self): dots = get_dots(self.user.person) return { 'dots': dots } + info_datatracker_uuid = ( + "Datatracker person identifiers", + ( + "Access to the stable identifier the datatracker uses for you when " + "telling other systems who you are, and to any identifiers it used for " + "you before they were superseded." + ), + ) + + def scope_datatracker_uuid(self): + # One scope for both claims: there is no case for granting the current + # identifier without the superseded ones that resolve to it. + person = self.user.person + return { + # An empty string is dropped by ScopeClaims._clean_dic, so an inconsistent + # Person yields an absent claim rather than a bogus identifier. + "datatracker_uuid": str(person.primary_uuid or ""), + # An empty list survives _clean_dic, so this claim is present-and-empty + # rather than absent for a Person that has never been merged. It holds only + # superseded identifiers - the current one is datatracker_uuid. + "datatracker_prior_uuids": [str(u) for u in person.prior_uuids], + } + def scope_pronouns(self): return { 'pronouns': self.user.person.pronouns() } diff --git a/ietf/ietfauth/views.py b/ietf/ietfauth/views.py index b5256b14f8b..15a37968a52 100644 --- a/ietf/ietfauth/views.py +++ b/ietf/ietfauth/views.py @@ -53,7 +53,7 @@ from django.contrib.auth.views import LoginView from django.contrib.sites.models import Site from django.core.exceptions import ObjectDoesNotExist, ValidationError -from django.db import IntegrityError +from django.db import IntegrityError, transaction from django.urls import reverse as urlreverse from django.http import Http404, HttpResponseRedirect, HttpResponseForbidden from django.shortcuts import render, redirect, get_object_or_404 @@ -69,6 +69,7 @@ from ietf.name.models import ExtResourceName from ietf.nomcom.models import NomCom from ietf.person.models import Person, Email, Alias, PersonalApiKey, PERSON_API_KEY_VALUES +from ietf.person.utils import assign_primary_uuid from ietf.review.models import ReviewerSettings, ReviewWish, ReviewAssignment from ietf.review.utils import unavailable_periods_to_list, get_default_filter_re from ietf.doc.fields import SearchableDocumentField @@ -232,12 +233,15 @@ def confirm_account(request, auth): if not person: name = form.cleaned_data["name"] ascii = form.cleaned_data["ascii"] - person = Person.objects.create(user=user, - name=name, - ascii=ascii) - for name in set([ person.name, person.ascii, person.plain_name(), person.plain_ascii(), ]): - Alias.objects.create(person=person, name=name) + # Atomic so a Person is never left without the primary UUID that + # external systems need to name them by. + with transaction.atomic(): + person = Person.objects.create(user=user, name=name, ascii=ascii) + assign_primary_uuid(person) + + for name in set([ person.name, person.ascii, person.plain_name(), person.plain_ascii(), ]): + Alias.objects.create(person=person, name=name) if not email_obj: email_obj = Email.objects.create(address=email, person=person, origin=user.username) diff --git a/ietf/nomcom/utils.py b/ietf/nomcom/utils.py index 46418714880..0edf4498fc0 100644 --- a/ietf/nomcom/utils.py +++ b/ietf/nomcom/utils.py @@ -18,6 +18,7 @@ from email.utils import parseaddr from textwrap import dedent +from django.db import transaction from django.db.models import Q, Count, F, QuerySet from django.conf import settings from django.contrib.sites.models import Site @@ -36,6 +37,7 @@ from ietf.utils.mail import send_mail_text, send_mail, get_payload_text from ietf.utils.log import log from ietf.person.name import unidecode_name +from ietf.person.utils import assign_primary_uuid from ietf.utils.timezone import date_today, datetime_from_date, DEADLINE_TZINFO import debug # pyflakes:ignore @@ -416,13 +418,17 @@ def make_nomineeposition(nomcom, candidate, position, author): def make_nomineeposition_for_newperson(nomcom, candidate_name, candidate_email, position, author): - # This is expected to fail if called with an existing email address - email = Email.objects.create(address=candidate_email, origin="nominee: %s" % nomcom.group.acronym) - person = Person.objects.create(name=candidate_name, - ascii=unidecode_name(candidate_name), - ) - email.person = person - email.save() + # This is expected to fail if called with an existing email address. + # Atomic so a Person is never left without the primary UUID that external systems + # need to name them by, and so a failure part way leaves no half-built nominee. + with transaction.atomic(): + email = Email.objects.create(address=candidate_email, origin="nominee: %s" % nomcom.group.acronym) + person = Person.objects.create(name=candidate_name, + ascii=unidecode_name(candidate_name), + ) + assign_primary_uuid(person) + email.person = person + email.save() # send email to secretariat and nomcomchair to warn about the new person subject = 'New person is created' diff --git a/ietf/person/admin.py b/ietf/person/admin.py index f46edcf8aeb..4d3ad745e63 100644 --- a/ietf/person/admin.py +++ b/ietf/person/admin.py @@ -3,9 +3,13 @@ import simple_history from django import forms +from django.contrib import messages +from django.db import transaction -from ietf.person.models import Email, Alias, Person, PersonalApiKey, PersonEvent, PersonApiKeyEvent, PersonExtResource +from ietf.person.models import Email, Alias, Person, PersonalApiKey, PersonEvent, \ + PersonApiKeyEvent, PersonExtResource, PersonUUID from ietf.person.name import name_parts +from ietf.person.utils import queue_person_uuid_push from ietf.utils.admin import SaferStackedInline, SaferTabularInline from ietf.utils.validators import validate_external_resource_value @@ -29,6 +33,53 @@ class AliasAdmin(admin.ModelAdmin): class AliasInline(SaferStackedInline): model = Alias + +@admin.action(description="Make this the person's primary UUID") +def set_primary(modeladmin, request, queryset): + """Re-designate a Person's primary UUID + + Acts on exactly one UUID at a time: promoting two at once would either violate the + one-primary-per-person constraint or silently ignore one of them. + """ + if queryset.count() != 1: + modeladmin.message_user( + request, "Select exactly one UUID.", level=messages.ERROR + ) + return + new_primary = queryset.first() + if new_primary.primary: + modeladmin.message_user(request, "That UUID is already primary.") + return + person = new_primary.person + with transaction.atomic(): + person.uuids.filter(primary=True).update(primary=False) + new_primary.primary = True + new_primary.save(update_fields=["primary"]) + queue_person_uuid_push(person) + modeladmin.message_user( + request, f"{new_primary.uuid} is now the primary UUID for {person}." + ) + + +class PersonUUIDAdmin(admin.ModelAdmin): + list_display = ["uuid", "person", "primary", "time"] # noqa: RUF012 + list_filter = ["primary"] # noqa: RUF012 + search_fields = ["uuid", "person__name"] # noqa: RUF012 + raw_id_fields = ["person"] # noqa: RUF012 + readonly_fields = ["uuid", "primary", "time"] # noqa: RUF012 + actions = [set_primary] # noqa: RUF012 +admin.site.register(PersonUUID, PersonUUIDAdmin) + + +class PersonUUIDInline(SaferStackedInline): + model = PersonUUID + extra = 0 + # primary is changed through the PersonUUID admin's set_primary action, which demotes + # the old primary first. Editing it here would trip the uniqueness constraint. + readonly_fields = ["uuid", "primary", "time"] # noqa: RUF012 + can_delete = False + + class PersonAdmin(simple_history.admin.SimpleHistoryAdmin): def plain_name(self, obj): if obj.plain: @@ -41,7 +92,7 @@ def plain_name(self, obj): readonly_fields = ("name_from_draft", ) search_fields = ["name", "ascii"] raw_id_fields = ["user"] - inlines = [ EmailInline, AliasInline, ] + inlines = [ EmailInline, AliasInline, PersonUUIDInline] # actions = None admin.site.register(Person, PersonAdmin) diff --git a/ietf/person/api_uuid.py b/ietf/person/api_uuid.py new file mode 100644 index 00000000000..0fbf452566e --- /dev/null +++ b/ietf/person/api_uuid.py @@ -0,0 +1,290 @@ +# Copyright The IETF Trust 2026, All Rights Reserved +"""Person UUID resolution API + +Lets an authorized application ask about a Person UUID it holds and learn the Person's +current identifier set. Responses carry identifiers only - no name, address or database +key. +""" + +from drf_spectacular.utils import ( + OpenApiExample, + extend_schema, + extend_schema_view, +) +from rest_framework import mixins, serializers, viewsets +from rest_framework.decorators import action +from rest_framework.response import Response +from rest_framework.views import APIView + +from ietf.person.models import Person, PersonUUID + +MAX_BATCH = 500 + +# A batch entry either resolved to a Person or it did not. Both outcomes use the same +# entry shape, discriminated by this field, so a consumer switches on it instead of +# inspecting which fields came back. +ENTRY_STATUSES = ("resolved", "unknown") + + +def uuid_sets_for(person_ids): + """Map each person_id to its (primary_uuid, [prior_uuids]) in a single query + + Keeps the batch endpoints' query count independent of the batch size: reading + Person.primary_uuid and Person.prior_uuids per row would be two queries per Person. + Ordering matches Person.prior_uuids. + """ + sets = {pid: (None, []) for pid in person_ids} + rows = ( + PersonUUID.objects.filter(person_id__in=person_ids) + .order_by("time", "uuid") + .values_list("person_id", "uuid", "primary") + ) + for pid, value, is_primary in rows: + _, priors = sets[pid] + if is_primary: + sets[pid] = (value, priors) + else: + priors.append(value) + return sets + + +class PersonUUIDResolutionSerializer(serializers.Serializer): + """A PersonUUID together with its Person's whole identifier set""" + + uuid = serializers.UUIDField(read_only=True) + is_primary = serializers.BooleanField(source="primary", read_only=True) + primary_uuid = serializers.UUIDField(source="person.primary_uuid", read_only=True) + prior_uuids = serializers.ListField( + source="person.prior_uuids", child=serializers.UUIDField(), read_only=True + ) + + +class PersonUUIDBatchRequestSerializer(serializers.Serializer): + uuids = serializers.ListField( + child=serializers.UUIDField(), allow_empty=False, max_length=MAX_BATCH + ) + + +class PersonUUIDBatchEntrySerializer(serializers.Serializer): + """One requested UUID and, when it resolved, its Person's identifier set + + Every field is always present. The identifier fields are null - prior_uuids empty - + when status is unknown. + + Output only, and deliberately not read_only=True field by field: read_only implies + required=False, which would leave a generated client treating even status as optional. + """ + + uuid = serializers.UUIDField() + status = serializers.ChoiceField(choices=ENTRY_STATUSES) + is_primary = serializers.BooleanField(allow_null=True) + primary_uuid = serializers.UUIDField(allow_null=True) + prior_uuids = serializers.ListField(child=serializers.UUIDField()) + + +class PersonUUIDBatchResponseSerializer(serializers.Serializer): + results = PersonUUIDBatchEntrySerializer(many=True) + + +class PersonPkBatchRequestSerializer(serializers.Serializer): + person_pks = serializers.ListField( + child=serializers.IntegerField(), allow_empty=False, max_length=MAX_BATCH + ) + + +class PersonPkBatchEntrySerializer(serializers.Serializer): + """One requested Person.pk and, when it resolved, that Person's identifier set + + Same one-shape-for-both-outcomes and output-only rules as + PersonUUIDBatchEntrySerializer. + """ + + person_pk = serializers.IntegerField() + status = serializers.ChoiceField(choices=ENTRY_STATUSES) + primary_uuid = serializers.UUIDField(allow_null=True) + prior_uuids = serializers.ListField(child=serializers.UUIDField()) + + +class PersonPkBatchResponseSerializer(serializers.Serializer): + results = PersonPkBatchEntrySerializer(many=True) + + +@extend_schema(tags=["person"]) +@extend_schema_view( + retrieve=extend_schema( + operation_id="person_uuid_retrieve", + summary="Resolve a Person UUID", + description=( + "Resolve any UUID the datatracker has issued for a Person to that Person's " + "current identifier set. A UUID that stopped being primary because of a " + 'merge still resolves, and the response carries the current primary. A 200 ' + 'whose primary_uuid differs from the requested uuid means "same person, new ' + 'identifier" - it is not an error.\n\n' + "A 404 means no Person has this UUID. It does not distinguish a UUID the " + "datatracker never issued from one it issued to a Person that has since been " + "deleted, because deleting a Person deletes its UUIDs.\n\n" + "primary_uuid is the person's current primary. Re-read it; do not assume it " + "is unchanged from a previous response, and do not assume a UUID you hold " + "remains primary.\n\n" + "Responses contain identifiers only. No name, address or database key is " + "returned." + ), + responses=PersonUUIDResolutionSerializer, + ) +) +class PersonUUIDViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet): + """Resolve Person UUIDs to their Person's current identifier set""" + + api_key_endpoint = "ietf.person.api_uuid" + queryset = PersonUUID.objects.select_related("person") + serializer_class = PersonUUIDResolutionSerializer + lookup_field = "uuid" + lookup_url_kwarg = "uuid" + lookup_value_converter = "anycase_uuid" + + @extend_schema( + operation_id="person_uuid_lookup", + summary="Resolve a batch of Person UUIDs", + description=( + f"Resolve up to {MAX_BATCH} UUIDs in one call. Always returns 200 with one " + "results entry per distinct requested UUID - no entry is ever omitted and no " + "unresolvable UUID fails the request.\n\n" + "Each entry carries a status of resolved or unknown, corresponding to the " + "200 and 404 outcomes of the single-UUID endpoint. Every entry has the same " + "fields either way, with the identifier fields null when status is unknown, " + "so switch on status rather than on which fields are present. Duplicate " + "inputs produce one entry. Entry order is not significant - match on uuid." + ), + request=PersonUUIDBatchRequestSerializer, + responses=PersonUUIDBatchResponseSerializer, + ) + @action(detail=False, methods=["post"]) + def lookup(self, request): + requested_serializer = PersonUUIDBatchRequestSerializer(data=request.data) + requested_serializer.is_valid(raise_exception=True) + requested = list(dict.fromkeys(requested_serializer.validated_data["uuids"])) + + found = {row.uuid: row for row in PersonUUID.objects.filter(uuid__in=requested)} + sets = uuid_sets_for({row.person_id for row in found.values()}) + + results = [] + for value in requested: + row = found.get(value) + if row is None: + results.append( + { + "uuid": value, + "status": "unknown", + "is_primary": None, + "primary_uuid": None, + "prior_uuids": [], + } + ) + continue + primary, priors = sets[row.person_id] + results.append( + { + "uuid": value, + "status": "resolved", + "is_primary": row.primary, + "primary_uuid": primary, + "prior_uuids": priors, + } + ) + return Response( + PersonUUIDBatchResponseSerializer({"results": results}).data + ) + + +@extend_schema(tags=["person"]) +class PersonUUIDByPersonPkView(APIView): + """Transitional pk-to-UUID conversion, for consumers migrating off Person.pk + + Batch-only on purpose: the one legitimate use is a single bulk conversion, and a + convenient per-request lookup would become a permanent pk-to-UUID service. Its own + api_key_endpoint so its tokens can be withdrawn without touching the resolution API. + + A plain APIView rather than a ViewSet: nothing here is a resource, and routing a + lookup through a ViewSet meant calling it "create", which made the schema claim a 201 + for a request that creates nothing and returns 200. + """ + + api_key_endpoint = "ietf.person.api_uuid_by_pk" + + @extend_schema( + deprecated=True, + operation_id="person_uuid_by_person_pk", + summary="TRANSITIONAL: resolve Person database keys to UUIDs", + description=( + "DEPRECATED FROM FIRST RELEASE, AND WILL BE WITHDRAWN. Resolves up to " + f"{MAX_BATCH} Person.pk values to their UUID sets, so an application that " + "keyed its records on the datatracker database key can convert them to " + "UUIDs once and stop using the key. Person.pk is the identifier this whole " + "feature exists to stop exporting: it does not survive a Person merge, " + "which is why holding it is unsafe.\n\n" + "Intended to be called once per consuming application, to migrate a table. " + "Do not call it at request time and do not build anything that keeps " + "needing it.\n\n" + "One results entry per distinct requested pk, with the same fields either " + "way and the identifier fields null when status is unknown. Switch on " + "status." + ), + request=PersonPkBatchRequestSerializer, + responses={200: PersonPkBatchResponseSerializer}, + examples=[ + OpenApiExample( + "Two pks, one of them unknown", + value={ + "results": [ + { + "person_pk": 12345, + "status": "resolved", + "primary_uuid": "6f9a1c30-6c7e-4f0a-9a3f-2f1d0b8a4e11", + "prior_uuids": ["0b21f8d4-1a55-4c9e-8f77-9c2b4a6e3d02"], + }, + { + "person_pk": 999999, + "status": "unknown", + "primary_uuid": None, + "prior_uuids": [], + }, + ] + }, + response_only=True, + ) + ], + ) + def post(self, request): + requested_serializer = PersonPkBatchRequestSerializer(data=request.data) + requested_serializer.is_valid(raise_exception=True) + requested = list( + dict.fromkeys(requested_serializer.validated_data["person_pks"]) + ) + + existing = set( + Person.objects.filter(pk__in=requested).values_list("pk", flat=True) + ) + sets = uuid_sets_for(existing) + + results = [] + for pk in requested: + if pk not in existing: + results.append( + { + "person_pk": pk, + "status": "unknown", + "primary_uuid": None, + "prior_uuids": [], + } + ) + continue + primary, priors = sets[pk] + results.append( + { + "person_pk": pk, + "status": "resolved", + "primary_uuid": primary, + "prior_uuids": priors, + } + ) + return Response(PersonPkBatchResponseSerializer({"results": results}).data) diff --git a/ietf/person/factories.py b/ietf/person/factories.py index 655f25994b3..110542e0796 100644 --- a/ietf/person/factories.py +++ b/ietf/person/factories.py @@ -20,7 +20,8 @@ import debug # pyflakes:ignore -from ietf.person.models import Person, Alias, Email, PersonalApiKey, PersonApiKeyEvent, PERSON_API_KEY_ENDPOINTS +from ietf.person.models import Person, Alias, Email, PersonalApiKey, PersonApiKeyEvent, \ + PERSON_API_KEY_ENDPOINTS, PersonUUID from ietf.person.name import normalize_name, unidecode_name @@ -64,6 +65,21 @@ def set_password(obj, create, extracted, **kwargs): # pylint: disable=no-self-ar obj.set_password( '%s+password' % obj.username ) # pylint: disable=no-value-for-parameter obj.save() + +class PersonUUIDFactory(factory.django.DjangoModelFactory): + """A UUID for a Person + + Defaults to a superseded, non-primary UUID, which is what a test asking for an extra + UUID wants. PersonFactory uses this with primary=True to make each Person's primary; + creating a second primary for the same Person violates a uniqueness constraint. + """ + person = factory.SubFactory("ietf.person.factories.PersonFactory") + primary = False + + class Meta: + model = PersonUUID + + class PersonFactory(factory.django.DjangoModelFactory): class Meta: model = Person @@ -79,6 +95,16 @@ class Meta: class Params: with_bio = factory.Trait(biography = "\n\n".join(fake.paragraphs())) # type: ignore + @factory.post_generation + def primary_uuid(obj, create, extracted, **kwargs): # pylint: disable=no-self-argument + """Give the Person the primary UUID every Person is supposed to have + + Pass primary_uuid=False for a Person with no UUIDs at all, which is otherwise + not reachable through any production path. + """ + if create and extracted is not False: + PersonUUIDFactory(person=obj, primary=True) + @factory.post_generation def default_aliases(obj, create, extracted, **kwargs): # pylint: disable=no-self-argument make_alias = getattr(AliasFactory, 'create' if create else 'build') diff --git a/ietf/person/migrations/0006_personuuid.py b/ietf/person/migrations/0006_personuuid.py new file mode 100644 index 00000000000..17a56808c6d --- /dev/null +++ b/ietf/person/migrations/0006_personuuid.py @@ -0,0 +1,67 @@ +# Copyright The IETF Trust 2026, All Rights Reserved + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import ietf.person.models + + +def forward(apps, schema_editor): + Person = apps.get_model("person", "Person") + PersonUUID = apps.get_model("person", "PersonUUID") + # uuid and time come from the field defaults + PersonUUID.objects.bulk_create( + [PersonUUID(person=person, primary=True) for person in Person.objects.all()], + batch_size=1000, + ) + + +def reverse(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + dependencies = [ + ("person", "0005_alter_historicalperson_pronouns_selectable_and_more"), + ] + + operations = [ + migrations.CreateModel( + name="PersonUUID", + fields=[ + ( + "uuid", + models.UUIDField( + default=ietf.person.models.unused_person_uuid, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("primary", models.BooleanField(default=False)), + ( + "time", + models.DateTimeField( + default=django.utils.timezone.now, editable=False + ), + ), + ( + "person", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="uuids", + to="person.person", + ), + ), + ], + ), + migrations.AddConstraint( + model_name="personuuid", + constraint=models.UniqueConstraint( + condition=models.Q(("primary", True)), + fields=("person",), + name="unique_primary_uuid_per_person", + ), + ), + migrations.RunPython(forward, reverse), + ] diff --git a/ietf/person/models.py b/ietf/person/models.py index bb6b4ffc737..8b05c5f8a61 100644 --- a/ietf/person/models.py +++ b/ietf/person/models.py @@ -44,6 +44,48 @@ def name_character_validator(value): ) +def unused_person_uuid(): + MAX_ATTEMPTS = 50 # ludicrously large + for _ in range(MAX_ATTEMPTS): + candidate = uuid.uuid4() + if not PersonUUID.objects.filter(uuid=candidate).exists(): + return candidate + raise RuntimeError("Unable to generate unused UUID") + + +class PersonUUID(models.Model): + """Surrogate key for a Person + + A Person has one or more UUIDs. Exactly one is primary: it is the identifier handed + to external systems. Merging Persons unions the sets and keeps a single primary, so a + UUID an external system holds keeps resolving to the surviving Person even after it + stops being primary. + + Deleting a Person deletes its UUIDs. Their values return to the pool + unused_person_uuid() draws from, so a deleted UUID could in principle be issued again + to a different Person. That requires uuid4() to reproduce a specific 122-bit value, + so the risk is accepted. + """ + uuid = models.UUIDField(primary_key=True, editable=False, default=unused_person_uuid) + person = models.ForeignKey( + "person.Person", related_name="uuids", on_delete=models.CASCADE + ) + primary = models.BooleanField(default=False) + time = models.DateTimeField(default=timezone.now, editable=False) + + class Meta: + constraints = [ # noqa: RUF012 + models.UniqueConstraint( + fields=["person"], + condition=models.Q(primary=True), + name="unique_primary_uuid_per_person", + ), + ] + + def __str__(self): + return str(self.uuid) + + class Person(models.Model): history = HistoricalRecords() user = OneToOneField(User, blank=True, null=True, on_delete=models.SET_NULL) @@ -201,6 +243,27 @@ def full_name_as_key(self): # this is mostly a remnant from the old views, needed in the menu return self.plain_name().lower().replace(" ", ".") + @property + def primary_uuid(self): + """The UUID to hand to external systems, or None if data is inconsistent""" + row = self.uuids.filter(primary=True).first() + return row.uuid if row else None + + @property + def prior_uuids(self): + """UUIDs that still resolve to this Person but are no longer primary + + Oldest first. Ties are broken on the UUID itself so the order is total rather + than left to the database, and so it matches uuid_sets_for() in + ietf.person.api_uuid. Nothing in the current code produces a tie: the timestamp + defaults per row and a merge moves UUIDs without rewriting it. + """ + return list( + self.uuids.filter(primary=False) + .order_by("time", "uuid") + .values_list("uuid", flat=True) + ) + def photo_name(self,thumb=False): hasher = Hashids(salt='Person photo name salt',min_length=5) _, first, _, last, _ = name_parts(self.ascii) diff --git a/ietf/person/resources.py b/ietf/person/resources.py index 42bb9f7732c..c8a8bf8af35 100644 --- a/ietf/person/resources.py +++ b/ietf/person/resources.py @@ -10,7 +10,18 @@ from ietf import api -from ietf.person.models import (Person, Email, Alias, PersonalApiKey, PersonEvent, PersonApiKeyEvent, HistoricalPerson, HistoricalEmail, PersonExtResource) # type: ignore +from ietf.person.models import ( # type: ignore + Person, + Email, + Alias, + PersonalApiKey, + PersonEvent, + PersonApiKeyEvent, + HistoricalPerson, + HistoricalEmail, + PersonExtResource, + PersonUUID, +) from ietf.utils.resources import UserResource @@ -35,6 +46,26 @@ class Meta: } api.person.register(PersonResource()) + +class PersonUUIDResource(ModelResource): + person = ToOneField(PersonResource, "person") + + class Meta: + cache = SimpleCache() + queryset = PersonUUID.objects.all() + serializer = api.Serializer() + # resource_name = 'personuuid' + ordering = ['uuid', ] # noqa: RUF012 + filtering = { # noqa: RUF012 + "uuid": ALL, + "primary": ALL, + "time": ALL, + "person": ALL_WITH_RELATIONS, + } + + +api.person.register(PersonUUIDResource()) + class EmailResource(ModelResource): person = ToOneField(PersonResource, 'person', null=True) class Meta: diff --git a/ietf/person/tasks.py b/ietf/person/tasks.py index f0c979fa267..4e0a34942df 100644 --- a/ietf/person/tasks.py +++ b/ietf/person/tasks.py @@ -7,11 +7,13 @@ from celery import shared_task from django.conf import settings +from django.db.models import Count, Q from django.utils import timezone from ietf.utils import log from ietf.utils.mail import send_mail -from .models import PersonalApiKey, PersonApiKeyEvent +from .models import Person, PersonalApiKey, PersonApiKeyEvent +from .utils import ensure_primary_uuid @shared_task @@ -57,3 +59,72 @@ def purge_personal_api_key_events_task(keep_days): count = len(old_events) old_events.delete() log.log(f"Deleted {count} PersonApiKeyEvents older than {keep_since}") + + +@shared_task +def check_person_uuids_task(fix=False): + """Report - and optionally repair - Persons whose UUID set is inconsistent + + Every Person is supposed to have exactly one primary UUID. Assignment is an explicit + assign_primary_uuid() call at each site that creates a Person, so a site added + without one leaves Persons that cannot be named to any external system. This finds + them. + + Checks for exactly one rather than at least one. A partial unique constraint should + make more than one impossible, so finding one means something is grossly wrong with + the data and worth saying out loud - and this is the job that claims the condition + holds, so it may as well test it. + """ + broken = ( + Person.objects.annotate( + uuid_count=Count("uuids", distinct=True), + primary_count=Count("uuids", filter=Q(uuids__primary=True), distinct=True), + ) + .exclude(primary_count=1) + .order_by("pk") + ) + + count = 0 + for person in broken: + count += 1 + if person.primary_count > 1: + # ensure_primary_uuid() cannot resolve this - it would pick one of the + # primaries arbitrarily. Needs a human to decide which one survives. + log.log( + f"Person {person.pk} ({person.name}): {person.primary_count} primary " + f"UUIDs, which the unique constraint should have prevented - not " + f"repairing automatically" + ) + continue + problem = "no UUIDs at all" if person.uuid_count == 0 else "no primary UUID" + log.log(f"Person {person.pk} ({person.name}): {problem}") + if fix: + row = ensure_primary_uuid(person) + log.log(f"Person {person.pk}: primary UUID is now {row.uuid}") + + if count == 0: + log.log("check_person_uuids: every Person has exactly one primary UUID") + else: + log.log( + f"check_person_uuids: {count} Person(s) " + f"{'repaired' if fix else 'need attention'}" + ) + return count + + +@shared_task +def push_person_uuids_task(person_pk): + """Push a Person's UUID set to Authentik + + Enqueued by ietf.person.utils.queue_person_uuid_push whenever the set changes. The + Authentik client does not exist yet, so for now this records the desired state that + will be pushed. + """ + person = Person.objects.filter(pk=person_pk).first() + if person is None: + log.log(f"Not pushing UUIDs for Person {person_pk}: no such Person") + return + log.log( + f"Person {person_pk} UUIDs: primary={person.primary_uuid} " + f"prior={[str(u) for u in person.prior_uuids]}" + ) diff --git a/ietf/person/tests.py b/ietf/person/tests.py index 42c2c1547cb..0af8271594f 100644 --- a/ietf/person/tests.py +++ b/ietf/person/tests.py @@ -4,19 +4,26 @@ import datetime import json +import uuid from unittest import mock from io import StringIO, BytesIO from PIL import Image from pyquery import PyQuery +import django.core.signing from django.core.exceptions import ValidationError +from django.db import connection, transaction +from django.db.utils import IntegrityError from django.http import HttpRequest from django.test import override_settings +from django.test.utils import CaptureQueriesContext from django.urls import reverse as urlreverse from django.utils import timezone from django.utils.encoding import iri_to_uri +import yaml + import debug # pyflakes:ignore from ietf.community.models import CommunityList @@ -26,12 +33,22 @@ from ietf.nomcom.models import NomCom from ietf.nomcom.test_data import nomcom_test_data from ietf.nomcom.factories import NomComFactory, NomineeFactory, NominationFactory, FeedbackFactory, PositionFactory -from ietf.person.factories import EmailFactory, PersonFactory, PersonApiKeyEventFactory -from ietf.person.models import Person, Alias, PersonApiKeyEvent -from ietf.person.tasks import purge_personal_api_key_events_task +from ietf.nomcom.utils import make_nomineeposition_for_newperson +from ietf.person.factories import ( + EmailFactory, + PersonFactory, + PersonApiKeyEventFactory, + PersonUUIDFactory, +) +from ietf.person.models import Person, Alias, PersonApiKeyEvent, PersonUUID +from ietf.person.tasks import (purge_personal_api_key_events_task, push_person_uuids_task, + check_person_uuids_task) from ietf.person.utils import (merge_persons, determine_merge_order, send_merge_notification, handle_users, get_extra_primary, dedupe_aliases, move_related_objects, merge_nominees, - handle_reviewer_settings, get_dots) + handle_reviewer_settings, get_dots, assign_primary_uuid, ensure_primary_uuid, + get_person_uuid_object) +from ietf.submit.utils import ensure_person_email_info_exists +from kombu.exceptions import OperationalError as KombuOperationalError from ietf.review.models import ReviewerSettings from ietf.utils.test_utils import TestCase, login_testing_unauthorized from ietf.utils.mail import outbox, empty_outbox @@ -116,6 +133,54 @@ def test_person_profile_without_email(self): r = self.client.get(url) self.assertContains(r, person.name, status_code=200) + def test_person_profile_by_uuid(self): + person_a = PersonFactory(name="A Fine Person") + url_a = urlreverse( + "ietf.person.views.profile_by_uuid", + kwargs={"uuid": person_a.primary_uuid}, + ) + + person_b = PersonFactory(name="Brilliant Person") + url_b = urlreverse( + "ietf.person.views.profile_by_uuid", + kwargs={"uuid": person_b.primary_uuid}, + ) + + r = self.client.get(url_a) + self.assertContains(r, person_a.name) + self.assertNotContains(r, person_b.name) + + r = self.client.get(url_b) + self.assertNotContains(r, person_a.name) + self.assertContains(r, person_b.name) + + # Move b's UUID to a as a prior UUID, as a merge would... + person_b.uuids.update(person=person_a, primary=False) + # ... and the old address redirects to a's canonical one + r = self.client.get(url_b) + self.assertRedirects(r, url_a) + r = self.client.get(url_b, follow=True) + self.assertContains(r, person_a.name) + self.assertNotContains(r, person_b.name) + + def test_person_profile_by_uuid_upper_case(self): + person = PersonFactory() + uuid_value = person.primary_uuid + url = urlreverse( + "ietf.person.views.profile_by_uuid", kwargs={"uuid": uuid_value} + ) + upper = url.replace(str(uuid_value), str(uuid_value).upper()) + self.assertNotEqual(url, upper) + r = self.client.get(upper) + self.assertContains(r, person.name, status_code=200) + + def test_person_profile_by_uuid_unknown(self): + url = urlreverse( + "ietf.person.views.profile_by_uuid", kwargs={"uuid": uuid.uuid4()} + ) + r = self.client.get(url) + self.assertEqual(r.status_code, 404) + def test_case_insensitive(self): # Case insensitive seach person = PersonFactory(name="Test Person") @@ -394,9 +459,12 @@ def test_merge_persons(self): request = HttpRequest() request.user = user source = PersonFactory() + PersonUUIDFactory(person=source) # give them an extra target = PersonFactory() mars = RoleFactory(name_id='chair',group__acronym='mars').group source_id = source.pk + source_uuids = {source.primary_uuid, *source.prior_uuids} + target_uuids = {target.primary_uuid, *target.prior_uuids} source_email = source.email_set.first() source_alias = source.alias_set.first() source_user = source.user @@ -415,6 +483,14 @@ def test_merge_persons(self): self.assertIn(nomination, target.nomination_set.all()) self.assertFalse(Person.objects.filter(id=source_id)) self.assertFalse(source_user.is_active) + self.assertEqual( + {target.primary_uuid, *target.prior_uuids}, + source_uuids | target_uuids, + ) + # The survivor keeps its own primary; the source's UUIDs become priors + self.assertIsNotNone(target.primary_uuid) + self.assertIn(target.primary_uuid, target_uuids) + self.assertEqual(set(target.prior_uuids), source_uuids) def test_merge_persons_reviewer_settings(self): secretariat_role = RoleFactory(group__acronym='secretariat', name_id='secr') @@ -478,6 +554,506 @@ def test_send_merge_request(self): self.assertIn(source.user.username, message.to) +class PersonUUIDTests(TestCase): + def test_every_creation_path_assigns_a_primary(self): + """Each production route that creates a Person gives it one primary UUID""" + # ietf.ietfauth.views.confirm_account + confirm_url = urlreverse( + "ietf.ietfauth.views.confirm_account", + kwargs={ + "auth": django.core.signing.dumps( + "uuidtest@example.com", salt="create_account" + ) + }, + ) + self.client.post( + confirm_url, + { + "name": "UUID Test", + "ascii": "UUID Test", + "password": "secret+password", + "password_confirmation": "secret+password", + }, + ) + created = Person.objects.get(name="UUID Test") + self.assertIsNotNone(created.primary_uuid) + self.assertEqual(created.prior_uuids, []) + + # ietf.nomcom.utils.make_nomineeposition_for_newperson + nomcom = NomComFactory(group__acronym="nomcom2021") + position = PositionFactory(nomcom=nomcom) + make_nomineeposition_for_newperson( + nomcom, + "New Nominee", + "newnominee@example.com", + position, + PersonFactory().email(), + ) + nominee_person = Person.objects.get(name="New Nominee") + self.assertIsNotNone(nominee_person.primary_uuid) + self.assertEqual(nominee_person.prior_uuids, []) + + # ietf.submit.utils.ensure_person_email_info_exists + ensure_person_email_info_exists( + "Draft Author", "draftauthor@example.com", "draft-uuid-test" + ) + author = Person.objects.get(name="Draft Author") + self.assertIsNotNone(author.primary_uuid) + self.assertEqual(author.prior_uuids, []) + + def test_factory_assigns_a_primary(self): + person = PersonFactory() + # Reads the rows directly: this is what proves the accessors below agree with + # the model, so it must not go through them + self.assertEqual(person.uuids.filter(primary=True).count(), 1) + self.assertIsNotNone(person.primary_uuid) + self.assertEqual(person.prior_uuids, []) + + def test_factory_can_skip_the_primary(self): + person = PersonFactory(primary_uuid=False) + self.assertEqual(person.uuids.count(), 0) + self.assertIsNone(person.primary_uuid) + self.assertEqual(person.prior_uuids, []) + + def test_prior_uuids_holds_the_superseded_ones_in_order(self): + person = PersonFactory() + primary = person.primary_uuid + older = PersonUUIDFactory( + person=person, time=timezone.now() - datetime.timedelta(days=2) + ) + newer = PersonUUIDFactory( + person=person, time=timezone.now() - datetime.timedelta(days=1) + ) + # The primary is not in the prior list, and the priors are oldest-first + self.assertEqual(person.primary_uuid, primary) + self.assertEqual(person.prior_uuids, [older.uuid, newer.uuid]) + + def test_assign_primary_uuid_is_idempotent(self): + person = PersonFactory() + first = person.primary_uuid + assign_primary_uuid(person) + self.assertEqual(person.uuids.count(), 1) + self.assertEqual(person.primary_uuid, first) + + def test_only_one_primary_per_person(self): + person = PersonFactory() + with self.assertRaises(IntegrityError), transaction.atomic(): + PersonUUID.objects.create(person=person, primary=True) + + def test_ensure_primary_uuid_promotes_earliest(self): + person = PersonFactory() + oldest = person.uuids.get() + PersonUUIDFactory(person=person) + person.uuids.update(primary=False) # deliberately inconsistent state + promoted = ensure_primary_uuid(person) + self.assertEqual(promoted.uuid, oldest.uuid) + self.assertEqual(person.uuids.filter(primary=True).count(), 1) + + def test_ensure_primary_uuid_creates_when_none(self): + person = PersonFactory(primary_uuid=False) + created = ensure_primary_uuid(person) + self.assertTrue(created.primary) + self.assertEqual(person.uuids.count(), 1) + + def test_get_person_uuid_object(self): + person = PersonFactory() + prior = PersonUUIDFactory(person=person) + self.assertIsNone(get_person_uuid_object(uuid.uuid4())) + primary_obj = get_person_uuid_object(person.primary_uuid) + self.assertEqual(primary_obj.person, person) + self.assertTrue(primary_obj.primary) + prior_obj = get_person_uuid_object(prior.uuid) + self.assertEqual(prior_obj.person, person) + self.assertFalse(prior_obj.primary) + + def test_deleting_a_person_deletes_its_uuids(self): + person = PersonFactory() + values = [person.primary_uuid, *person.prior_uuids] + person.delete() + self.assertEqual(PersonUUID.objects.filter(uuid__in=values).count(), 0) + for value in values: + self.assertIsNone(get_person_uuid_object(value)) + + def test_merge_chain_keeps_one_primary(self): + secretariat_role = RoleFactory(group__acronym="secretariat", name_id="secr") + request = HttpRequest() + request.user = secretariat_role.person.user + a, b, c = PersonFactory.create_batch(3) + a_uuids = {a.primary_uuid, *a.prior_uuids} + b_uuids = {b.primary_uuid, *b.prior_uuids} + c_primary = c.primary_uuid + merge_persons(request, a, b, file=StringIO()) + merge_persons(request, b, c, file=StringIO()) + c.refresh_from_db() + self.assertIsNotNone(c.primary_uuid) + self.assertEqual(c.primary_uuid, c_primary) + self.assertEqual(set(c.prior_uuids), a_uuids | b_uuids) + for value in a_uuids | b_uuids: + self.assertEqual(get_person_uuid_object(value).person, c) + + def test_merge_promotes_a_primary_for_a_target_without_one(self): + secretariat_role = RoleFactory(group__acronym="secretariat", name_id="secr") + request = HttpRequest() + request.user = secretariat_role.person.user + source = PersonFactory() + target = PersonFactory() + target.uuids.update(primary=False) # deliberately inconsistent state + merge_persons(request, source, target, file=StringIO()) + target.refresh_from_db() + self.assertIsNotNone(target.primary_uuid) + + @mock.patch("ietf.person.utils.transaction.on_commit", side_effect=lambda f: f()) + @mock.patch("ietf.person.tasks.push_person_uuids_task.apply_async") + def test_creating_a_person_does_not_push(self, mock_apply, mock_on_commit): + # A brand-new Person has no Authentik account, so there is nothing to push to. + person = PersonFactory() + self.assertFalse(mock_apply.called) + + person.name = person.name + " Jr" + person.save() + self.assertFalse(mock_apply.called) # nor does an unrelated save + + @mock.patch("ietf.person.utils.transaction.on_commit", side_effect=lambda f: f()) + @mock.patch("ietf.person.tasks.push_person_uuids_task.apply_async") + def test_push_is_dispatched_when_the_set_changes(self, mock_apply, mock_on_commit): + secretariat_role = RoleFactory(group__acronym="secretariat", name_id="secr") + request = HttpRequest() + request.user = secretariat_role.person.user + source = PersonFactory() + target = PersonFactory() + mock_apply.reset_mock() + + merge_persons(request, source, target, file=StringIO()) + self.assertTrue(mock_apply.called) + self.assertEqual( + mock_apply.call_args.kwargs["kwargs"], {"person_pk": target.pk} + ) + # Celery's default retry policy applies - short enough for the request path, + # and enough to ride out a broker blip. See queue_person_uuid_push(). + self.assertNotIn("retry", mock_apply.call_args.kwargs) + + # Promoting a primary for a Person that already existed does push + mock_apply.reset_mock() + other = PersonFactory() + other.uuids.update(primary=False) # deliberately inconsistent state + ensure_primary_uuid(other) + self.assertEqual(mock_apply.call_args.kwargs["kwargs"], {"person_pk": other.pk}) + + @mock.patch("ietf.person.utils.transaction.on_commit", side_effect=lambda f: f()) + @mock.patch("ietf.person.tasks.push_person_uuids_task.apply_async") + @mock.patch("ietf.person.utils.log.log") + def test_unreachable_broker_does_not_break_the_caller( + self, mock_log, mock_apply, mock_on_commit + ): + mock_apply.side_effect = KombuOperationalError("no broker here") + person = PersonFactory() + person.uuids.update(primary=False) # deliberately inconsistent state + ensure_primary_uuid(person) # must not raise + self.assertIsNotNone(person.primary_uuid) + self.assertIn("Could not queue UUID push", mock_log.call_args[0][0]) + + @mock.patch("ietf.person.tasks.log.log") + def test_push_person_uuids_task(self, mock_log): + person = PersonFactory() + prior = PersonUUIDFactory(person=person) + push_person_uuids_task(person_pk=person.pk) + message = mock_log.call_args[0][0] + self.assertIn(str(person.primary_uuid), message) + self.assertIn(str(prior.uuid), message) + + mock_log.reset_mock() + push_person_uuids_task(person_pk=person.pk + 10000) + self.assertIn("no such Person", mock_log.call_args[0][0]) + + @mock.patch("ietf.person.tasks.log.log") + def test_check_person_uuids_task(self, mock_log): + good = PersonFactory() + broken = PersonFactory(primary_uuid=False) + demoted = PersonFactory() + demoted.uuids.update(primary=False) # deliberately inconsistent state + + def logged(): + return "\n".join(call[0][0] for call in mock_log.call_args_list) + + self.assertEqual(check_person_uuids_task(), 2) + report = logged() + self.assertIn(f"Person {broken.pk}", report) + self.assertIn("no UUIDs at all", report) + self.assertIn(f"Person {demoted.pk}", report) + self.assertIn("no primary UUID", report) + self.assertNotIn(f"Person {good.pk} ", report) + self.assertIn("2 Person(s) need attention", report) + + mock_log.reset_mock() + self.assertEqual(check_person_uuids_task(fix=True), 2) + self.assertIn("2 Person(s) repaired", logged()) + for person in (broken, demoted): + self.assertIsNotNone(person.primary_uuid) + + mock_log.reset_mock() + self.assertEqual(check_person_uuids_task(), 0) + self.assertIn("every Person has exactly one primary UUID", logged()) + + +@override_settings( + APP_API_TOKENS={ + "ietf.person.api_uuid": ["uuid-api-token"], + "ietf.person.api_uuid_by_pk": ["by-pk-token"], + } +) +class PersonUUIDApiTests(TestCase): + def retrieve_url(self, uuid_value): + return urlreverse( + "ietf.api.person_api.person-uuid-detail", kwargs={"uuid": uuid_value} + ) + + @property + def lookup_url(self): + return urlreverse("ietf.api.person_api.person-uuid-lookup") + + @property + def by_pk_url(self): + return urlreverse("ietf.api.person_api.person-uuid-by-pk") + + def test_requires_a_valid_api_key(self): + person = PersonFactory() + url = self.retrieve_url(person.primary_uuid) + self.assertEqual(self.client.get(url).status_code, 403) + self.assertEqual( + self.client.get(url, headers={"X-Api-Key": "nope"}).status_code, 403 + ) + self.assertEqual( + self.client.get(url, headers={"X-Api-Key": "by-pk-token"}).status_code, 403 + ) + self.assertEqual( + self.client.get(url, headers={"X-Api-Key": "uuid-api-token"}).status_code, + 200, + ) + + def test_retrieve_primary(self): + person = PersonFactory() + r = self.client.get( + self.retrieve_url(person.primary_uuid), + headers={"X-Api-Key": "uuid-api-token"}, + ) + self.assertEqual(r.status_code, 200) + self.assertEqual( + r.json(), + { + "uuid": str(person.primary_uuid), + "is_primary": True, + "primary_uuid": str(person.primary_uuid), + "prior_uuids": [], + }, + ) + + def test_retrieve_superseded(self): + person = PersonFactory() + prior = PersonUUIDFactory(person=person) + r = self.client.get( + self.retrieve_url(prior.uuid), headers={"X-Api-Key": "uuid-api-token"} + ) + self.assertEqual(r.status_code, 200) + self.assertEqual( + r.json(), + { + "uuid": str(prior.uuid), + "is_primary": False, + "primary_uuid": str(person.primary_uuid), + "prior_uuids": [str(prior.uuid)], + }, + ) + + def test_response_carries_identifiers_only(self): + person = PersonFactory() + r = self.client.get( + self.retrieve_url(person.primary_uuid), + headers={"X-Api-Key": "uuid-api-token"}, + ) + self.assertEqual( + set(r.json().keys()), + {"uuid", "is_primary", "primary_uuid", "prior_uuids"}, + ) + + def test_retrieve_unknown(self): + r = self.client.get( + self.retrieve_url(uuid.uuid4()), headers={"X-Api-Key": "uuid-api-token"} + ) + self.assertEqual(r.status_code, 404) + + def test_retrieve_upper_case(self): + person = PersonFactory() + url = self.retrieve_url(person.primary_uuid) + upper = url.replace(str(person.primary_uuid), str(person.primary_uuid).upper()) + self.assertNotEqual(url, upper) + r = self.client.get(upper, headers={"X-Api-Key": "uuid-api-token"}) + self.assertEqual(r.status_code, 200) + self.assertEqual(r.json()["uuid"], str(person.primary_uuid)) + + def test_retrieve_malformed(self): + r = self.client.get( + "/api/person/uuid/not-a-uuid/", headers={"X-Api-Key": "uuid-api-token"} + ) + self.assertEqual(r.status_code, 404) + + def test_batch(self): + person = PersonFactory() + prior = PersonUUIDFactory(person=person) + missing = uuid.uuid4() + r = self.client.post( + self.lookup_url, + { + "uuids": [ + str(prior.uuid), + str(person.primary_uuid), + str(missing), + str(prior.uuid), + ] + }, + content_type="application/json", + headers={"X-Api-Key": "uuid-api-token"}, + ) + self.assertEqual(r.status_code, 200) + results = r.json()["results"] + # One entry per distinct requested UUID, duplicates collapsed + self.assertEqual(len(results), 3) + by_uuid = {entry["uuid"]: entry for entry in results} + # Same shape as a resolved entry, with the identifiers nulled out + self.assertEqual( + by_uuid[str(missing)], + { + "uuid": str(missing), + "status": "unknown", + "is_primary": None, + "primary_uuid": None, + "prior_uuids": [], + }, + ) + self.assertEqual(by_uuid[str(prior.uuid)]["status"], "resolved") + self.assertFalse(by_uuid[str(prior.uuid)]["is_primary"]) + self.assertEqual( + by_uuid[str(prior.uuid)]["primary_uuid"], str(person.primary_uuid) + ) + self.assertTrue(by_uuid[str(person.primary_uuid)]["is_primary"]) + + def test_batch_query_count_is_independent_of_size(self): + people = PersonFactory.create_batch(6) + # Resolve the UUIDs up front so the capture below sees only the API's queries + values = [str(p.primary_uuid) for p in people] + + def post(subset): + return self.client.post( + self.lookup_url, + {"uuids": subset}, + content_type="application/json", + headers={"X-Api-Key": "uuid-api-token"}, + ) + + with CaptureQueriesContext(connection) as small: + self.assertEqual(post(values[:2]).status_code, 200) + with CaptureQueriesContext(connection) as large: + self.assertEqual(post(values).status_code, 200) + self.assertEqual(len(small.captured_queries), len(large.captured_queries)) + + def test_batch_rejects_bad_input(self): + for payload in ( + {"uuids": []}, + {"uuids": ["not-a-uuid"]}, + {"uuids": [str(uuid.uuid4()) for _ in range(501)]}, + {}, + ): + r = self.client.post( + self.lookup_url, + payload, + content_type="application/json", + headers={"X-Api-Key": "uuid-api-token"}, + ) + self.assertEqual(r.status_code, 400, payload) + + def test_by_person_pk(self): + person = PersonFactory() + prior = PersonUUIDFactory(person=person) + r = self.client.post( + self.by_pk_url, + {"person_pks": [person.pk, person.pk + 10000]}, + content_type="application/json", + headers={"X-Api-Key": "by-pk-token"}, + ) + self.assertEqual(r.status_code, 200) + results = r.json()["results"] + self.assertEqual(len(results), 2) + self.assertEqual( + results[0], + { + "person_pk": person.pk, + "status": "resolved", + "primary_uuid": str(person.primary_uuid), + "prior_uuids": [str(prior.uuid)], + }, + ) + self.assertEqual( + results[1], + { + "person_pk": person.pk + 10000, + "status": "unknown", + "primary_uuid": None, + "prior_uuids": [], + }, + ) + + def test_by_person_pk_has_its_own_token(self): + person = PersonFactory() + r = self.client.post( + self.by_pk_url, + {"person_pks": [person.pk]}, + content_type="application/json", + headers={"X-Api-Key": "uuid-api-token"}, + ) + self.assertEqual(r.status_code, 403) + + def test_by_person_pk_rejects_over_cap(self): + r = self.client.post( + self.by_pk_url, + {"person_pks": list(range(501))}, + content_type="application/json", + headers={"X-Api-Key": "by-pk-token"}, + ) + self.assertEqual(r.status_code, 400) + + def test_schema(self): + r = self.client.get("/api/schema/") + self.assertEqual(r.status_code, 200) + schema = yaml.safe_load(r.content) + paths = schema["paths"] + self.assertIn("/api/person/uuid/{uuid}/", paths) + self.assertIn("/api/person/uuid/lookup/", paths) + self.assertIn("/api/person/uuid/by-person-pk/", paths) + self.assertEqual( + paths["/api/person/uuid/{uuid}/"]["get"]["operationId"], + "person_uuid_retrieve", + ) + self.assertEqual( + paths["/api/person/uuid/lookup/"]["post"]["operationId"], + "person_uuid_lookup", + ) + by_pk = paths["/api/person/uuid/by-person-pk/"]["post"] + self.assertEqual(by_pk["operationId"], "person_uuid_by_person_pk") + self.assertTrue(by_pk["deprecated"]) + self.assertIn("PersonUUIDResolution", schema["components"]["schemas"]) + for path, method in ( + ("/api/person/uuid/{uuid}/", "get"), + ("/api/person/uuid/lookup/", "post"), + ("/api/person/uuid/by-person-pk/", "post"), + ): + responses = paths[path][method]["responses"] + self.assertIn("200", responses, path) + # Consumers switch on status, so it has to be a declared, required field + for component in ("PersonUUIDBatchEntry", "PersonPkBatchEntry"): + entry = schema["components"]["schemas"][component] + self.assertIn("status", entry["required"], component) + self.assertTrue(entry["properties"]["primary_uuid"]["nullable"], component) + + class TaskTests(TestCase): @mock.patch("ietf.person.tasks.log.log") def test_purge_personal_api_key_events_task(self, mock_log): diff --git a/ietf/person/urls.py b/ietf/person/urls.py index f3eccd04b73..15338d48595 100644 --- a/ietf/person/urls.py +++ b/ietf/person/urls.py @@ -1,6 +1,8 @@ # Copyright The IETF Trust 2009-2025, All Rights Reserved # -*- coding: utf-8 -*- -from ietf.person import views, ajax +from django.urls import path + +from ietf.person import ajax, views from ietf.utils.urls import url urlpatterns = [ @@ -9,6 +11,8 @@ url(r'^merge/send_request/?$', views.send_merge_request), url(r'^search/(?P(person|email))/$', views.ajax_select2_search), url(r'^(?P[0-9]+)/email.json$', ajax.person_email_json), + path('', views.profile_by_uuid, + name='ietf.person.views.profile_by_uuid'), url(r'^(?P[^/]+)$', views.profile), url(r'^(?P[^/]+)/photo/?$', views.photo), ] diff --git a/ietf/person/utils.py b/ietf/person/utils.py index 5ed90591f9a..3f00a04cd0d 100755 --- a/ietf/person/utils.py +++ b/ietf/person/utils.py @@ -9,15 +9,92 @@ from django.contrib import admin from django.core.cache import cache from django.core.exceptions import ObjectDoesNotExist +from django.db import transaction from django.db.models import Q from django.http import Http404 +from kombu.exceptions import OperationalError as KombuOperationalError import debug # pyflakes:ignore -from ietf.person.models import Person, Alias, Email +from ietf.person.models import Person, Alias, Email, PersonUUID from ietf.utils import log from ietf.utils.mail import send_mail + +def get_person_uuid_object(uuid_value): + """Look up a UUID currently issued for a Person + + Returns the PersonUUID object, whose `person` is the Person it identifies and whose + `primary` says whether it is that Person's current identifier, or None if no such + UUID exists. + """ + return PersonUUID.objects.select_related("person").filter(uuid=uuid_value).first() + + +def queue_person_uuid_push(person): + """Enqueue an Authentik attribute push for this Person's UUID set + + Called explicitly by every site that changes the set for a Person that may already + have an Authentik account. Deferred to commit so a rolled-back transaction never + pushes state that does not exist. + + Queueing is best-effort: an unreachable broker is logged and ignored rather than + failing the datatracker operation that changed the UUID set. Celery's default retry + policy applies - three attempts over well under a second, enough to ride out a blip + or a broker failover without meaningfully delaying the caller. An outright outage + still cannot fail the operation, and the reconcile job is the backstop for a push + that never got queued. + """ + from ietf.person.tasks import push_person_uuids_task # avoid a circular import + + person_pk = person.pk + + def enqueue(): + try: + push_person_uuids_task.apply_async(kwargs={"person_pk": person_pk}) + except (KombuOperationalError, OSError) as err: + log.log(f"Could not queue UUID push for Person {person_pk}: {err}") + + transaction.on_commit(enqueue) + + +def assign_primary_uuid(person): + """Give a newly created Person its primary UUID + + Idempotent: returns the existing primary if the Person already has one, so a caller + that is unsure whether an earlier step already ran can call it safely. + + Deliberately does not queue an Authentik push. A Person that has just been created + has no Authentik account, so there would be nothing to push to; its UUID reaches + Authentik when the account is linked. + """ + existing = person.uuids.filter(primary=True).first() + if existing is not None: + return existing + return PersonUUID.objects.create(person=person, primary=True) + + +def ensure_primary_uuid(person): + """Make sure a Person has exactly one primary UUID + + Promotes the earliest existing UUID if there are any but none is primary, otherwise + creates one. Returns the primary PersonUUID. + """ + existing = person.uuids.filter(primary=True).first() + if existing is not None: + return existing + oldest = person.uuids.order_by("time", "uuid").first() + if oldest is None: + row = assign_primary_uuid(person) + else: + oldest.primary = True + oldest.save(update_fields=["primary"]) + row = oldest + # Unlike a brand-new Person, this one already existed and may be linked. + queue_person_uuid_push(person) + return row + + def merge_persons(request, source, target, file=sys.stdout, verbose=False): changes = [] @@ -49,6 +126,15 @@ def merge_persons(request, source, target, file=sys.stdout, verbose=False): if reviewer_changes: changes.extend(reviewer_changes) merge_nominees(source, target) + + # Move the source's UUIDs to the target, demoting the source's primary. The target's + # primary survives; the source's identifiers keep resolving, to the target. This runs + # before move_related_objects(), which would otherwise carry the UUIDs across still + # flagged primary and trip the one-primary-per-person constraint. + ensure_primary_uuid(target) + source.uuids.update(person=target, primary=False) + queue_person_uuid_push(target) + move_related_objects(source, target, file=file, verbose=verbose) dedupe_aliases(target) @@ -131,6 +217,10 @@ def move_related_objects(source, target, file, verbose=False): and f.auto_created and not f.concrete ] for related_object in related_objects: accessor = related_object.get_accessor_name() + if accessor == "uuids": + # PersonUUIDs move by their own rule - the source's primary has to be + # demoted on the way, or the target ends up with two. See merge_persons(). + continue field_name = related_object.field.name queryset = getattr(source, accessor).all() if verbose: diff --git a/ietf/person/views.py b/ietf/person/views.py index d0b5912431e..52137f0fb91 100644 --- a/ietf/person/views.py +++ b/ietf/person/views.py @@ -19,7 +19,12 @@ from ietf.person.models import Email, Person from ietf.person.fields import select2_id_name_json from ietf.person.forms import MergeForm, MergeRequestForm -from ietf.person.utils import handle_users, merge_persons, lookup_persons +from ietf.person.utils import ( + get_person_uuid_object, + handle_users, + lookup_persons, + merge_persons, +) from ietf.utils.mail import send_mail_text @@ -77,6 +82,23 @@ def profile(request, email_or_name): return render(request, 'person/profile.html', {'persons': persons, 'today': timezone.now()}) +def profile_by_uuid(request, uuid): + person_uuid = get_person_uuid_object(uuid) + if person_uuid is None: + raise Http404("No such person identifier") + if not person_uuid.primary: + # Self-heal a link that predates a merge by sending it to the canonical address. + return redirect( + "ietf.person.views.profile_by_uuid", + uuid=person_uuid.person.primary_uuid, + ) + return render( + request, + "person/profile.html", + {"persons": [person_uuid.person], "today": timezone.now()}, + ) + + def photo(request, email_or_name): persons = lookup_persons(email_or_name) if len(persons) > 1: diff --git a/ietf/submit/utils.py b/ietf/submit/utils.py index 457462e4f2c..b331d71631f 100644 --- a/ietf/submit/utils.py +++ b/ietf/submit/utils.py @@ -60,6 +60,7 @@ from ietf.utils.timezone import date_today from ietf.utils.xmldraft import InvalidMetadataError, XMLDraft, capture_xml2rfc_output from ietf.person.name import unidecode_name +from ietf.person.utils import assign_primary_uuid def validate_submission(submission): @@ -589,7 +590,11 @@ def ensure_person_email_info_exists(name, email, docname): person.name_from_draft = name log.assertion('isinstance(person.name, str)') person.ascii = unidecode_name(person.name) - person.save() + # Atomic so a Person is never left without the primary UUID that external + # systems need to name them by. + with transaction.atomic(): + person.save() + assign_primary_uuid(person) else: person.name_from_draft = name diff --git a/ietf/urls.py b/ietf/urls.py index e822b2042ef..61ab4b2de88 100644 --- a/ietf/urls.py +++ b/ietf/urls.py @@ -6,7 +6,7 @@ from django.contrib.sitemaps import views as sitemap_views from django.contrib.staticfiles.urls import staticfiles_urlpatterns from django.http import HttpResponse -from django.urls import include, path +from django.urls import include, path, register_converter from django.views import static as static_view from django.views.generic import TemplateView from django.views.defaults import server_error @@ -17,9 +17,16 @@ from ietf.group.urls import group_urls, grouptype_urls, stream_urls from ietf.ipr.sitemaps import IPRMap from ietf.liaisons.sitemaps import LiaisonMap +from ietf.utils.converters import AnyCaseUUIDConverter from ietf.utils.urls import url +# Register path converters here, in the root URLconf, before urlpatterns names any of +# them. Django refuses to register a converter twice, so registering at the point of +# definition would make importing that module from two URLconfs an error. +register_converter(AnyCaseUUIDConverter, "anycase_uuid") + + # sometimes, this code gets called more than once, which is an # that seems impossible to work around. try: diff --git a/ietf/utils/converters.py b/ietf/utils/converters.py new file mode 100644 index 00000000000..d4ebbbb42b3 --- /dev/null +++ b/ietf/utils/converters.py @@ -0,0 +1,21 @@ +# Copyright The IETF Trust 2026, All Rights Reserved +"""URL path converters + +Registered in the root URLconf, not here - Django does not allow registering a converter +twice, so a module-level register_converter() would break as soon as two URLconfs +imported this module. +""" + +from django.urls.converters import UUIDConverter + + +class AnyCaseUUIDConverter(UUIDConverter): + """UUIDConverter that also accepts upper-case hex + + Django's built-in "uuid" converter matches lower case only, so an upper-cased + identifier would 404 rather than resolve. + """ + + regex = ( + "[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}" + ) diff --git a/ietf/utils/management/commands/periodic_tasks.py b/ietf/utils/management/commands/periodic_tasks.py index c878ba49f10..9ba3c79edd9 100644 --- a/ietf/utils/management/commands/periodic_tasks.py +++ b/ietf/utils/management/commands/periodic_tasks.py @@ -245,6 +245,17 @@ def create_default_tasks(self): ), ) + PeriodicTask.objects.get_or_create( + name="Check Person UUIDs", + task="ietf.person.tasks.check_person_uuids_task", + kwargs=json.dumps({"fix": False}), + defaults={ + "enabled": False, + "crontab": self.crontabs["daily"], + "description": "Report Persons with no primary UUID", + }, + ) + PeriodicTask.objects.get_or_create( name="Run Yang model checks", task="ietf.submit.tasks.run_yang_model_checks_task", diff --git a/ietf/utils/test_data.py b/ietf/utils/test_data.py index c5d34727511..f7c4bb3efd4 100644 --- a/ietf/utils/test_data.py +++ b/ietf/utils/test_data.py @@ -22,6 +22,7 @@ from ietf.group.utils import setup_default_community_list_for_group from ietf.review.models import (ReviewRequest, ReviewerSettings, ReviewResultName, ReviewTypeName, ReviewTeamSettings ) from ietf.person.name import unidecode_name +from ietf.person.utils import assign_primary_uuid from ietf.utils.timezone import date_today @@ -40,6 +41,7 @@ def create_person(group, role_name, name=None, username=None, email_address=None user.set_password(password) user.save() person = Person.objects.create(name=name, ascii=unidecode_name(smart_str(name)), user=user) + assign_primary_uuid(person) email = Email.objects.create(address=email_address, person=person, origin=user.username) Role.objects.create(group=group, name_id=role_name, person=person, email=email) return person @@ -63,6 +65,7 @@ def make_immutable_base_data(): # system system_person = Person.objects.create(name="(System)", ascii="(System)") + assign_primary_uuid(system_person) Email.objects.create(address="", person=system_person, origin='test') # high-level groups @@ -120,6 +123,7 @@ def make_immutable_base_data(): for i in range(1, 10): u = User.objects.create(username="ad%s" % i) p = Person.objects.create(name="Ad No%s" % i, ascii="Ad No%s" % i, user=u) + assign_primary_uuid(p) email = Email.objects.create(address="ad%s@example.org" % i, person=p, origin=u.username) if i < 6: # active @@ -148,6 +152,7 @@ def make_immutable_base_data(): for i in range(1, 5): u = User.objects.create(username="irsgmember%s" % i) p = Person.objects.create(name="IRSG Member No%s" % i, ascii="IRSG Member No%s" % i, user=u) + assign_primary_uuid(p) email = Email.objects.create(address="irsgmember%s@example.org" % i, person=p, origin=u.username) Role.objects.create(name_id="member", group=irsg, person=p, email=email) @@ -250,6 +255,7 @@ def make_test_data(): u.set_password("plain+password") u.save() plainman = Person.objects.create(name="Plain Man", ascii="Plain Man", user=u) + assign_primary_uuid(plainman) email = Email.objects.create(address="plain@example.com", person=plainman, origin=u.username) # group personnel @@ -436,6 +442,7 @@ def make_review_data(doc): u.set_password("reviewer+password") u.save() reviewer = Person.objects.create(name="Some Réviewer", ascii="Some Reviewer", user=u) + assign_primary_uuid(reviewer) email = Email.objects.create(address="reviewer@example.com", person=reviewer, origin=u.username) for team in (team1, team2, team3): @@ -459,6 +466,7 @@ def make_review_data(doc): u.set_password("reviewsecretary+password") u.save() reviewsecretary = Person.objects.create(name="Réview Secretary", ascii="Review Secretary", user=u) + assign_primary_uuid(reviewsecretary) reviewsecretary_email = Email.objects.create(address="reviewsecretary@example.com", person=reviewsecretary, origin=u.username) Role.objects.create(name_id="secr", person=reviewsecretary, email=reviewsecretary_email, group=team1) @@ -466,6 +474,7 @@ def make_review_data(doc): u.set_password("reviewsecretary3+password") u.save() reviewsecretary3 = Person.objects.create(name="Réview Secretary3", ascii="Review Secretary3", user=u) + assign_primary_uuid(reviewsecretary3) reviewsecretary3_email = Email.objects.create(address="reviewsecretary3@example.com", person=reviewsecretary, origin=u.username) Role.objects.create(name_id="secr", person=reviewsecretary3, email=reviewsecretary3_email, group=team3) diff --git a/ietf/utils/test_runner.py b/ietf/utils/test_runner.py index 0a46fdf807a..f0245237670 100644 --- a/ietf/utils/test_runner.py +++ b/ietf/utils/test_runner.py @@ -251,6 +251,34 @@ def load_and_run_fixtures(verbosity): fn = getattr(module, components[-1]) fn() + check_base_data_person_uuids() + + +def check_base_data_person_uuids(): + """Every Person in the base test data must have exactly one primary UUID + + UUIDs are assigned by an explicit assign_primary_uuid() call at each site that + creates a Person (see ietf.person.utils), so a new site added without one would + otherwise go unnoticed until something asked for the Person's identifier. + """ + from django.db.models import Count, Q + + from ietf.person.models import Person + + broken = list( + Person.objects.annotate( + primary_count=Count("uuids", filter=Q(uuids__primary=True), distinct=True) + ) + .exclude(primary_count=1) + .values_list("pk", "name", "primary_count")[:10] + ) + if broken: + raise RuntimeError( + "Base test data has Persons without exactly one primary UUID - a Person " + "with none needs an assign_primary_uuid() call where it is created: " + + ", ".join(f"{pk} ({name}): {n} primary" for pk, name, n in broken) + ) + def safe_create_test_db(self, verbosity, *args, **kwargs): if old_create is None: raise RuntimeError("old_create has not been set, cannot proceed")