From 7cfbfccddd9fbdc55eb8438abdee43fbca619b85 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Fri, 14 Aug 2026 12:05:30 -0500 Subject: [PATCH] feat: Person UUID, related OIDC claims, and apis (#11415) * feat: UUIDs as person identifiers * feat: person uuid oidc claims and apis * chore: ruff ruff * fix: adjust how push is triggered * fix: keep the mypy ignore on the person model import Reformatting the import into a parenthesized block moved the ignore comment to the closing paren. mypy reports the simple_history HistoricalPerson and HistoricalEmail attribute errors against the 'from ... import (' line, so the comment has to sit there to suppress them. * refactor: register the anycase_uuid converter in the root URLconf Registering it in ietf/utils/converters.py made importing that module a side effect, and Django refuses to register a converter twice, so naming the converter from a second URLconf was a latent error. Define it there, register it once in ietf/urls.py before urlpatterns names it. * fix: create a Person and its primary UUID atomically A Person with no primary UUID cannot be named to any external system, so the create and the assign_primary_uuid() that follows it have to succeed or fail together. Covers all three production creation sites, including the draft submission one, and wraps the surrounding aliases and nominee email so a failure part way leaves nothing half-built. * refactor: give each UUID batch endpoint a single response serializer The resolved/unknown split needed a PolymorphicProxySerializer, which is an annotation helper rather than a real serializer, so the endpoints hand-built dicts and told consumers not to infer the outcome from which fields were present. Use one entry serializer per endpoint instead, discriminated on status, with the identifier fields nullable and always present, and actually serialize responses through it so the schema cannot drift from what is returned. Drops the ResolvedStatusEnum/UnknownStatusEnum overrides that existed only to keep the two single-valued status enums apart - there is now one StatusEnum. The entry fields are not read_only because read_only implies required=False, which left a generated client treating even status as optional. Also annotates retrieve with @extend_schema_view rather than overriding it just to call super(). * refactor: serve the pk-to-UUID conversion from a plain APIView Routing this lookup through a GenericViewSet forced the handler to be named create, because that is what SimpleRouter maps POST to on a collection route. Nothing is created: the view returned 200 while drf-spectacular inferred 201 from the action name, so the schema advertised a status code the endpoint never sends and a generated client would treat the real response as unexpected. An APIView.post() returns 200 with no annotation gymnastics. The viewset was buying nothing else - no retrieve, no mixins, and an empty queryset. api_key auth is unaffected, since HasApiKey just reads api_key_endpoint off the view. The URL is unchanged. Its name loses the router's -list suffix, and the schema test now checks the declared success codes so this cannot drift again. * feat: carry both UUID claims in one OIDC scope Splitting the current identifier and the superseded ones across two scopes was finer-grained than any consumer needs - there is no case for granting one and not the other, and the prior list is far too short for response size to matter. Also corrects the scope description, which claimed the prior list included the identifier in use now. It does not, and datatracker_uuid is where that lives. * fix: check for exactly one primary UUID, not just one or more The job logged that every Person has exactly one primary while only looking for Persons with none. The partial unique constraint should make more than one impossible, so finding one means the data is grossly inconsistent and worth reporting - and ensure_primary_uuid() cannot repair that case, since it would be picking a survivor arbitrarily, so it is reported and skipped rather than silently 'fixed'. Same change in the base-test-data check. * fix: let the UUID push enqueue use the default retry policy Celery's default is three attempts over well under a second, which is cheap enough on the request path that changed the UUID set and is the difference between riding out a broker blip or failover and dropping the push on the floor. The broker-error catch still keeps an outright outage from failing the datatracker operation. * chore: add dev API tokens for the person UUID endpoints Neither endpoint had an APP_API_TOKENS entry in the container config, so every call to them from a dev environment got a 403. * test: build Person UUIDs with the factories and read them through the accessors PersonFactory now makes its Person's primary UUID with PersonUUIDFactory instead of calling assign_primary_uuid() itself, so all UUID handling in tests goes through the factories. PersonFactory(primary_uuid=False) covers the no-UUIDs-at-all case, which no production path can reach, replacing the tests that created a Person and then deleted its UUID rows. Tests now assert through Person.primary_uuid and Person.prior_uuids rather than querying uuids directly, so the accessors are the example to copy. Direct queries remain only where they are the point: the test proving the accessors agree with the rows, and setup that deliberately builds inconsistent state. Also drops the retry kwarg assertion that went with the old retry=False. * fix: order prior_uuids deterministically A merge stamps every UUID it moves with the same time, so ordering the prior list on time alone left the order undefined in exactly the case where there is more than one prior. Break ties on the UUID, which also makes the claim that uuid_sets_for() matches this accessor true - it was already ordering on both. * docs: correct why prior_uuids breaks ties on the uuid The previous comment justified the tie-break by claiming a merge gives every UUID it moves the same timestamp. It does not: merge_persons() moves them with a queryset update that names only person and primary, and PersonUUID.time is a per-row default with no auto_now, so each keeps its original timestamp. The tie-break stands on narrower ground - it makes the order total instead of leaving equal timestamps to the database, and matches the ordering uuid_sets_for() already used - so only the comment changes. * test: clear over-zealous concerns about API return values --------- Co-authored-by: Jennifer Richards --- docker/configs/settings_local.py | 2 + ietf/api/urls.py | 19 + ietf/ietfauth/tests.py | 43 +- ietf/ietfauth/utils.py | 23 + ietf/ietfauth/views.py | 16 +- ietf/nomcom/utils.py | 20 +- ietf/person/admin.py | 55 +- ietf/person/api_uuid.py | 290 +++++++++ ietf/person/factories.py | 28 +- ietf/person/migrations/0006_personuuid.py | 67 ++ ietf/person/models.py | 63 ++ ietf/person/resources.py | 33 +- ietf/person/tasks.py | 73 ++- ietf/person/tests.py | 584 +++++++++++++++++- ietf/person/urls.py | 6 +- ietf/person/utils.py | 92 ++- ietf/person/views.py | 24 +- ietf/submit/utils.py | 7 +- ietf/urls.py | 9 +- ietf/utils/converters.py | 21 + .../management/commands/periodic_tasks.py | 11 + ietf/utils/test_data.py | 9 + ietf/utils/test_runner.py | 28 + 23 files changed, 1492 insertions(+), 31 deletions(-) create mode 100644 ietf/person/api_uuid.py create mode 100644 ietf/person/migrations/0006_personuuid.py create mode 100644 ietf/utils/converters.py 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")