티스토리 수익 글 보기

티스토리 수익 글 보기

feat: Person UUID, related OIDC claims, and apis (#11415) (#11597) · ietf-tools/datatracker@df4394a · GitHub
Skip to content

Commit df4394a

Browse files
jennifer-richardsrjsparks
jennifer-richards
and
rjsparks
authored
feat: Person UUID, related OIDC claims, and apis (#11415) (#11597)
* 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: Robert Sparks <rjsparks@nostrum.com>
1 parent 3f84a81 commit df4394a

23 files changed

Lines changed: 1492 additions & 31 deletions

File tree

‎docker/configs/settings_local.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@
115115
APP_API_TOKENS = {
116116
"ietf.api.red_api" : ["devtoken", "redtoken"], # Not a real secret
117117
"ietf.api.views_rpc" : ["devtoken"], # Not a real secret
118+
"ietf.person.api_uuid" : ["devtoken"], # Not a real secret
119+
"ietf.person.api_uuid_by_pk" : ["devtoken"], # Not a real secret
118120
}
119121

120122
# Errata system api configuration

‎ietf/api/urls.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from ietf import api
1010
from ietf.doc import views_ballot, api as doc_api
1111
from ietf.meeting import views as meeting_views
12+
from ietf.person import api_uuid as person_uuid_api
1213
from ietf.submit import views as submit_views
1314
from ietf.utils.urls import url
1415

@@ -21,6 +22,14 @@
2122
# core_router.register("email", person_api.EmailViewSet)
2223
# core_router.register("person", person_api.PersonViewSet)
2324

25+
# Person identity API router
26+
person_router = PrefixedSimpleRouter(
27+
use_regex_path=False, name_prefix="ietf.api.person_api"
28+
)
29+
person_router.register(
30+
"uuid", person_uuid_api.PersonUUIDViewSet, basename="person-uuid"
31+
)
32+
2433
# todo more general name for this API?
2534
red_router = PrefixedSimpleRouter(name_prefix="ietf.api.red_api") # red api router
2635
red_router.register("doc", doc_api.RfcViewSet)
@@ -88,6 +97,16 @@
8897
url(r'^person/email/$', api_views.active_email_list),
8998
# Related Email listing
9099
url(r'^person/email/(?P<email>[^/\x00]+)/related/$', api_views.related_email_list),
100+
# Transitional pk-to-UUID conversion. Before the router include below so it wins
101+
# over the router's uuid/ routes.
102+
path(
103+
"person/uuid/by-person-pk/",
104+
person_uuid_api.PersonUUIDByPersonPkView.as_view(),
105+
name="ietf.api.person_api.person-uuid-by-pk",
106+
),
107+
# Person UUID resolution API. After the ^person/email/ routes above so those keep
108+
# matching first.
109+
path("person/", include(person_router.urls)),
91110
# Draft submission API
92111
url(r'^submit/?$', submit_views.api_submit_tombstone),
93112
# Draft upload API

‎ietf/ietfauth/tests.py‎

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,13 @@
3434
from ietf.ietfauth.utils import has_role
3535
from ietf.meeting.factories import MeetingFactory, RegistrationFactory, RegistrationTicketFactory
3636
from ietf.nomcom.factories import NomComFactory
37-
from ietf.person.factories import PersonFactory, EmailFactory, UserFactory, PersonalApiKeyFactory
37+
from ietf.person.factories import (
38+
PersonFactory,
39+
EmailFactory,
40+
UserFactory,
41+
PersonalApiKeyFactory,
42+
PersonUUIDFactory,
43+
)
3844
from ietf.person.models import Person, Email
3945
from ietf.person.tasks import send_apikey_usage_emails_task
4046
from ietf.review.factories import ReviewRequestFactory, ReviewAssignmentFactory
@@ -657,7 +663,9 @@ def test_change_password(self):
657663
)
658664
user.set_password(VALID_PASSWORD)
659665
user.save()
660-
p = Person.objects.create(name="Some One", ascii="Some One", user=user)
666+
p = PersonFactory(
667+
user=user, name="Some One", ascii="Some One", default_emails=False
668+
)
661669
Email.objects.create(address=user.username, person=p, origin=user.username)
662670

663671
# log in
@@ -758,7 +766,9 @@ def test_change_username(self):
758766
)
759767
user.set_password(VALID_PASSWORD)
760768
user.save()
761-
p = Person.objects.create(name="Some One", ascii="Some One", user=user)
769+
p = PersonFactory(
770+
user=user, name="Some One", ascii="Some One", default_emails=False
771+
)
762772
Email.objects.create(address=user.username, person=p, origin=user.username)
763773
Email.objects.create(
764774
address="othername@example.org", person=p, origin=user.username
@@ -1162,7 +1172,16 @@ def test_oidc_code_auth(self):
11621172
session["nonce"] = rndstr()
11631173
args = {
11641174
"response_type": "code",
1165-
"scope": ['openid', 'profile', 'email', 'roles', 'registration', 'dots', 'pronouns' ],
1175+
"scope": [
1176+
"openid",
1177+
"profile",
1178+
"email",
1179+
"roles",
1180+
"registration",
1181+
"dots",
1182+
"pronouns",
1183+
"datatracker_uuid",
1184+
],
11661185
"nonce": session["nonce"],
11671186
"redirect_uri": redirect_uris[0],
11681187
"state": session["state"]
@@ -1207,6 +1226,10 @@ def test_oidc_code_auth(self):
12071226
self.assertIn(key, access_token_info)
12081227
for key in ['iss', 'sub', 'aud', 'exp', 'iat', 'auth_time', 'nonce', 'at_hash']:
12091228
self.assertIn(key, access_token_info['id_token'])
1229+
# Custom claims are served from userinfo, not the id_token. This guards
1230+
# against an accidental OIDC_IDTOKEN_INCLUDE_CLAIMS flip.
1231+
for key in ["datatracker_uuid", "datatracker_prior_uuids"]:
1232+
self.assertNotIn(key, access_token_info["id_token"])
12101233

12111234
# Get userinfo, check keys present, most common scenario
12121235
userinfo = client.do_user_info_request(state=params["state"], scope=args['scope'])
@@ -1218,6 +1241,18 @@ def test_oidc_code_auth(self):
12181241
self.assertNotIn('hackathon_onsite', set(userinfo['reg_type'].split()))
12191242
self.assertIn(active_group.acronym, [i[1] for i in userinfo['roles']])
12201243
self.assertNotIn(closed_group.acronym, [i[1] for i in userinfo['roles']])
1244+
self.assertEqual(userinfo['datatracker_uuid'], str(person.primary_uuid))
1245+
# Present and empty, not absent, for a Person that has never been merged
1246+
self.assertIn("datatracker_prior_uuids", userinfo)
1247+
self.assertEqual(userinfo["datatracker_prior_uuids"], [])
1248+
1249+
# A UUID absorbed by a merge shows up in the prior list
1250+
absorbed = PersonUUIDFactory(person=person)
1251+
userinfo = client.do_user_info_request(
1252+
state=params["state"], scope=args["scope"]
1253+
)
1254+
self.assertEqual(userinfo["datatracker_uuid"], str(person.primary_uuid))
1255+
self.assertEqual(userinfo["datatracker_prior_uuids"], [str(absorbed.uuid)])
12211256

12221257
# Create a registration, with only email, no person (rare if at all)
12231258
reg_person.delete()

‎ietf/ietfauth/utils.py‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,29 @@ def scope_dots(self):
341341
dots = get_dots(self.user.person)
342342
return { 'dots': dots }
343343

344+
info_datatracker_uuid = (
345+
"Datatracker person identifiers",
346+
(
347+
"Access to the stable identifier the datatracker uses for you when "
348+
"telling other systems who you are, and to any identifiers it used for "
349+
"you before they were superseded."
350+
),
351+
)
352+
353+
def scope_datatracker_uuid(self):
354+
# One scope for both claims: there is no case for granting the current
355+
# identifier without the superseded ones that resolve to it.
356+
person = self.user.person
357+
return {
358+
# An empty string is dropped by ScopeClaims._clean_dic, so an inconsistent
359+
# Person yields an absent claim rather than a bogus identifier.
360+
"datatracker_uuid": str(person.primary_uuid or ""),
361+
# An empty list survives _clean_dic, so this claim is present-and-empty
362+
# rather than absent for a Person that has never been merged. It holds only
363+
# superseded identifiers - the current one is datatracker_uuid.
364+
"datatracker_prior_uuids": [str(u) for u in person.prior_uuids],
365+
}
366+
344367
def scope_pronouns(self):
345368
return { 'pronouns': self.user.person.pronouns() }
346369

‎ietf/ietfauth/views.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@
5353
from django.contrib.auth.views import LoginView
5454
from django.contrib.sites.models import Site
5555
from django.core.exceptions import ObjectDoesNotExist, ValidationError
56-
from django.db import IntegrityError
56+
from django.db import IntegrityError, transaction
5757
from django.urls import reverse as urlreverse
5858
from django.http import Http404, HttpResponseRedirect, HttpResponseForbidden
5959
from django.shortcuts import render, redirect, get_object_or_404
@@ -69,6 +69,7 @@
6969
from ietf.name.models import ExtResourceName
7070
from ietf.nomcom.models import NomCom
7171
from ietf.person.models import Person, Email, Alias, PersonalApiKey, PERSON_API_KEY_VALUES
72+
from ietf.person.utils import assign_primary_uuid
7273
from ietf.review.models import ReviewerSettings, ReviewWish, ReviewAssignment
7374
from ietf.review.utils import unavailable_periods_to_list, get_default_filter_re
7475
from ietf.doc.fields import SearchableDocumentField
@@ -232,12 +233,15 @@ def confirm_account(request, auth):
232233
if not person:
233234
name = form.cleaned_data["name"]
234235
ascii = form.cleaned_data["ascii"]
235-
person = Person.objects.create(user=user,
236-
name=name,
237-
ascii=ascii)
238236

239-
for name in set([ person.name, person.ascii, person.plain_name(), person.plain_ascii(), ]):
240-
Alias.objects.create(person=person, name=name)
237+
# Atomic so a Person is never left without the primary UUID that
238+
# external systems need to name them by.
239+
with transaction.atomic():
240+
person = Person.objects.create(user=user, name=name, ascii=ascii)
241+
assign_primary_uuid(person)
242+
243+
for name in set([ person.name, person.ascii, person.plain_name(), person.plain_ascii(), ]):
244+
Alias.objects.create(person=person, name=name)
241245

242246
if not email_obj:
243247
email_obj = Email.objects.create(address=email, person=person, origin=user.username)

‎ietf/nomcom/utils.py‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
from email.utils import parseaddr
1919
from textwrap import dedent
2020

21+
from django.db import transaction
2122
from django.db.models import Q, Count, F, QuerySet
2223
from django.conf import settings
2324
from django.contrib.sites.models import Site
@@ -36,6 +37,7 @@
3637
from ietf.utils.mail import send_mail_text, send_mail, get_payload_text
3738
from ietf.utils.log import log
3839
from ietf.person.name import unidecode_name
40+
from ietf.person.utils import assign_primary_uuid
3941
from ietf.utils.timezone import date_today, datetime_from_date, DEADLINE_TZINFO
4042

4143
import debug # pyflakes:ignore
@@ -416,13 +418,17 @@ def make_nomineeposition(nomcom, candidate, position, author):
416418

417419
def make_nomineeposition_for_newperson(nomcom, candidate_name, candidate_email, position, author):
418420

419-
# This is expected to fail if called with an existing email address
420-
email = Email.objects.create(address=candidate_email, origin="nominee: %s" % nomcom.group.acronym)
421-
person = Person.objects.create(name=candidate_name,
422-
ascii=unidecode_name(candidate_name),
423-
)
424-
email.person = person
425-
email.save()
421+
# This is expected to fail if called with an existing email address.
422+
# Atomic so a Person is never left without the primary UUID that external systems
423+
# need to name them by, and so a failure part way leaves no half-built nominee.
424+
with transaction.atomic():
425+
email = Email.objects.create(address=candidate_email, origin="nominee: %s" % nomcom.group.acronym)
426+
person = Person.objects.create(name=candidate_name,
427+
ascii=unidecode_name(candidate_name),
428+
)
429+
assign_primary_uuid(person)
430+
email.person = person
431+
email.save()
426432

427433
# send email to secretariat and nomcomchair to warn about the new person
428434
subject = 'New person is created'

‎ietf/person/admin.py‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,13 @@
33
import simple_history
44

55
from django import forms
6+
from django.contrib import messages
7+
from django.db import transaction
68

7-
from ietf.person.models import Email, Alias, Person, PersonalApiKey, PersonEvent, PersonApiKeyEvent, PersonExtResource
9+
from ietf.person.models import Email, Alias, Person, PersonalApiKey, PersonEvent, \
10+
PersonApiKeyEvent, PersonExtResource, PersonUUID
811
from ietf.person.name import name_parts
12+
from ietf.person.utils import queue_person_uuid_push
913

1014
from ietf.utils.admin import SaferStackedInline, SaferTabularInline
1115
from ietf.utils.validators import validate_external_resource_value
@@ -29,6 +33,53 @@ class AliasAdmin(admin.ModelAdmin):
2933
class AliasInline(SaferStackedInline):
3034
model = Alias
3135

36+
37+
@admin.action(description="Make this the person's primary UUID")
38+
def set_primary(modeladmin, request, queryset):
39+
"""Re-designate a Person's primary UUID
40+
41+
Acts on exactly one UUID at a time: promoting two at once would either violate the
42+
one-primary-per-person constraint or silently ignore one of them.
43+
"""
44+
if queryset.count() != 1:
45+
modeladmin.message_user(
46+
request, "Select exactly one UUID.", level=messages.ERROR
47+
)
48+
return
49+
new_primary = queryset.first()
50+
if new_primary.primary:
51+
modeladmin.message_user(request, "That UUID is already primary.")
52+
return
53+
person = new_primary.person
54+
with transaction.atomic():
55+
person.uuids.filter(primary=True).update(primary=False)
56+
new_primary.primary = True
57+
new_primary.save(update_fields=["primary"])
58+
queue_person_uuid_push(person)
59+
modeladmin.message_user(
60+
request, f"{new_primary.uuid} is now the primary UUID for {person}."
61+
)
62+
63+
64+
class PersonUUIDAdmin(admin.ModelAdmin):
65+
list_display = ["uuid", "person", "primary", "time"] # noqa: RUF012
66+
list_filter = ["primary"] # noqa: RUF012
67+
search_fields = ["uuid", "person__name"] # noqa: RUF012
68+
raw_id_fields = ["person"] # noqa: RUF012
69+
readonly_fields = ["uuid", "primary", "time"] # noqa: RUF012
70+
actions = [set_primary] # noqa: RUF012
71+
admin.site.register(PersonUUID, PersonUUIDAdmin)
72+
73+
74+
class PersonUUIDInline(SaferStackedInline):
75+
model = PersonUUID
76+
extra = 0
77+
# primary is changed through the PersonUUID admin's set_primary action, which demotes
78+
# the old primary first. Editing it here would trip the uniqueness constraint.
79+
readonly_fields = ["uuid", "primary", "time"] # noqa: RUF012
80+
can_delete = False
81+
82+
3283
class PersonAdmin(simple_history.admin.SimpleHistoryAdmin):
3384
def plain_name(self, obj):
3485
if obj.plain:
@@ -41,7 +92,7 @@ def plain_name(self, obj):
4192
readonly_fields = ("name_from_draft", )
4293
search_fields = ["name", "ascii"]
4394
raw_id_fields = ["user"]
44-
inlines = [ EmailInline, AliasInline, ]
95+
inlines = [ EmailInline, AliasInline, PersonUUIDInline]
4596
# actions = None
4697
admin.site.register(Person, PersonAdmin)
4798

0 commit comments

Comments
 (0)