fix(scim): send name.givenName/familyName in outbound SCIM payload - #3836
Open
shaidar wants to merge 6 commits into
Open
fix(scim): send name.givenName/familyName in outbound SCIM payload#3836shaidar wants to merge 6 commits into
shaidar wants to merge 6 commits into
Conversation
LearnUserAdapter.to_dict() never included a `name` object in the SCIM payload sent to Keycloak - only displayName/userName/emails/active. Keycloak's SCIM plugin hardcodes name.givenName/name.familyName -> its core firstName/lastName attributes and never reads displayName for that purpose, so migrated accounts ended up with blank names in Keycloak (and, downstream, in Learn). Adds LearnUserAdapter._resolve_name() with three tiers of confidence: legal_address.first_name/last_name (real structured data) -> a best-effort split of User.name (for edX-only users who never had a legal_address name recorded) -> blank. The split is never persisted back onto legal_address, which is used for SDN compliance screening - writing a heuristic guess into a field compliance screening may rely on for an accurate legal name would be a real risk. Also adds remediate_keycloak_user_names, a standalone command to find and (with --apply) patch already-migrated Keycloak users whose names are blank/stale. sync_users_to_scim_remote only ever creates users that don't already exist remotely - it has no update path - so already-broken accounts can't be fixed by re-running the sync; this talks to Keycloak's Admin API directly instead. Defaults to dry-run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OpenAPI ChangesShow/hide changesUnexpected changes? Ensure your branch is up-to-date with |
for more information, see https://pre-commit.ci
…s absent
from_dict() used dict.get(key, default), which only falls back when the
key is absent, not when it's present with a JSON null. An inbound SCIM
payload like {"name": {"givenName": null}} would assign None to
legal_address.first_name/last_name, which are non-nullable CharFields -
raising an IntegrityError on save and failing the whole sync for that
user. Reproduced the exact failure, then fixed it to treat null the
same as absent, matching this method's existing convention for every
other field.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
from_dict() only checked `is not None` for inbound name.givenName/ familyName, so an explicit empty or whitespace-only string silently overwrote a previously-valid legal_address.first_name/last_name with blank data. Unlike the null case, an empty string doesn't violate the CharField's DB constraint, so this failed silently rather than raising an error - arguably worse, since it corrupts SDN-compliance-relevant data with no signal at all. Now treats blank/whitespace the same as absent/null, consistent with the rest of this method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Comment on lines
+81
to
+84
| if ( | ||
| keycloak_user.firstName == given_name | ||
| and keycloak_user.lastName == family_name | ||
| ): |
There was a problem hiding this comment.
Bug: The comparison between Keycloak user names and resolved names fails when Keycloak returns None for an unset name, as it incorrectly compares None with an empty string "".
Severity: LOW
Suggested Fix
Normalize the None values from the keycloak_user object to empty strings before the comparison. For example, change the check to (keycloak_user.firstName or "") == given_name and (keycloak_user.lastName or "") == family_name. This ensures that a None value from Keycloak is treated the same as an empty string from the local name resolver.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: users/management/commands/remediate_keycloak_user_names.py#L81-L84
Potential issue: The management command compares user names from Keycloak with names
resolved by `_resolve_name()`. The `_resolve_name()` function returns empty strings for
missing name parts, but the Keycloak API may return `null` for an unset name, which is
deserialized as `None`. The direct comparison `keycloak_user.lastName == family_name`
will evaluate to `False` when `keycloak_user.lastName` is `None` and `family_name` is
`""`. This incorrectly flags users with single-word names as needing an update, leading
to unnecessary API calls and inaccurate reporting from the command.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
N/A
Description (What does it do?)
Users migrated into Keycloak via SCIM ended up with blank
firstName/lastName("General" section in the Keycloak admin console) — and, downstream, blank names in Learn too, since Keycloak's own outbound sync just propagates whatever it has.Root cause:
LearnUserAdapter.to_dict()(users/adapters.py) never included anameobject in the SCIM payload — onlydisplayName,userName,emails,active. Confirmed by decompiling the installedscim-for-keycloakKeycloak plugin jar that:name.givenName/name.familyNameon an inbound SCIM request map directly (hardcoded) to Keycloak's corefirstName/lastNameattributes.displayNameis never read by the plugin for that purpose at all — sending it has zero effect on Keycloak's stored name.So the only fix is sending the structured
name.givenName/name.familyNamefields, which requires splitting whatever combined name data we have (Keycloak has no "full name" field to hand a single string to).Fix:
LearnUserAdapter._resolve_name()resolves(given_name, family_name)with three tiers of confidence:legal_address.first_name/last_name, if both are set — real structured data.User.name, split heuristically (last whitespace token = family name, remainder = given name) — a fallback for users who only came through the edX migration and never had alegal_addressname recorded. No naive split is correct for every name (breaks on single-name accounts, multi-word surnames, non-Western conventions), but it's the best data available.The tier-2 derived split is never persisted back onto
legal_address— that model is used for SDN/denied-persons compliance screening, and writing a heuristic guess into a field compliance may rely on for an accurate legal name is a real risk, not just a data-quality nitpick. It's computed fresh into_dict()for the outbound payload only.ATTR_MAPalso gained entries forname.givenName/name.familyName(and flatgivenName/familyName, matching upstreamdjango_scim's own defaultUserFilterQuery.attr_mapconvention) so SCIM filter/PATCH path resolution works for the new fields, andfrom_dict()writes inboundname.givenName/name.familyNamedirectly tolegal_address(real data from an external SCIM client, never a derived guess).Also included:
remediate_keycloak_user_names, a standalone management command for accounts that were already migrated before this fix and are stuck with blank/stale names in Keycloak.sync_users_to_scim_remoteonly ever creates users that don't already exist remotely — it has no update/PATCH path — so re-running the sync does nothing for already-existing accounts. This command talks to Keycloak's Admin API directly instead (reusing the existingb2b/keycloak_admin_api.pyKeycloakAdminClient, paginating users 100 at a time rather than one request per user):--applyperforms the actualPUToffirstName/lastName, using the same_resolve_name()tiers as the adapter so a backfill and a fresh migration always agree on what a user's name should be.--limitcaps how many get patched in a single--applyrun, for a cautious first pass against production.How can this be tested?
users/adapters_test.pywas rewritten — the existing test imported the basemitol.scim.adapters.UserAdapter(a re-export), not the actualLearnUserAdaptersubclass used in production, so it never exercised the real adapter at all. New tests cover all three_resolve_name()tiers into_dict(), andfrom_dict()writing tier-1 data back tolegal_address.users/management/tests/remediate_keycloak_user_names_test.pycovers: dry-run reporting without patching,--applypatching + re-verification, already-up-to-date users being skipped, unpatchable (no name data) users being reported separately,--limitcapping writes, unmatched Keycloak users being ignored, and pagination across multiple pages.Ran the full
users/suite locally with a fresh test DB (uv run pytest users/ --create-db): 138 passed, no regressions. Ranruff checkon all changed files with no findings (aside from one pre-existing, unrelated missing-module-docstring lint onusers/adapters.pythat predates this change).Additional Context
This is the independent, mergeable half of a larger fix. A companion PR in
mitodl/ol-django(mitodl/ol-django#544) makessync_users_to_scim_remotereturn its results instead ofNone, which a follow-up mitxonline PR (a newmigrate_and_sync_usersorchestrator command, replacing the old manualmigrate_edx_data+ ad hoc sync script process) depends on for post-sync verification. That follow-up isn't included here since it would call code that doesn't exist yet in the currently-publishedmitol-django-scim— it'll be opened as a draft PR blocked on the ol-django release.